`tock_registers::peripheral!`: Sound and testable register definitions #4001

jrvanwhy · 2024-05-21T23:05:45Z

This PR introduces peripheral! to tock_registers. peripheral! is a successor to register_structs! that makes several improvements:

It is sound. The types generated and used by peripheral! do not use & references into MMIO memory -- instead, only raw pointers are used. This avoids the unsoundness described in Can we have VolatileCell rust-lang/unsafe-code-guidelines#411.
It supports driver unit tests. Instead of only emitting a struct, it emits a trait representing the hardware's capabilities, which can be used by unit tests to fake or stub out hardware behavior. Of course, it also emits a struct that implements the trait by interacting with the real hardware.
It supports unsafe operations, as described in RFC: Unsafe WriteOnly, ReadWrite, and ReadOnly (?) variants for tock-register-interface? #3244.
It supports conditionally-compiled registers, as described in New register blocks macro doesn't support conditional compilation #3257.

There is one major drawback relative to register_structs!: peripheral! is only designed to support MMIO registers. This was done to keep the implementation complexity manageable.

For a basic introduction of its usage, see libraries/tock-register-interface/README.md, and for detailed documentation, see libraries/tock-register-interface/src/peripheral.rs.

This PR is only partially completed. At the moment, I suggest you review:

Also, note that I plan to continue force-pushing to this branch, so this PR's history doesn't become ridiculously long.

brghena · 2024-05-21T23:52:33Z

Awesome! Very excited to see some progress here.

Can you briefly compare to prior approaches, namely your #3405 PR?

brghena

Some thoughts from looking through the README

brghena · 2024-05-22T00:02:48Z

libraries/tock-register-interface/README.md

+        // Padding's length may also be inferred by the macro by specifying _ as
+        // the data type:
+        0x008 => _: _,


Does this include arbitrarily long gaps? If the gap is 100 bytes, will the system automatically make an array of padding?

Do you need to have 0x008 => _: _, at all? What would happen if you did:

0x006 => byte0: u8 {Read, Write}, // gap here 0x00C => byte1: u8 {Read, Write},

Presumably that would be a compile-time failure?

But this code would work?

0x006 => byte0: u8 {Read, Write}, // HUGE gap here 0x007 => _: _, 0x100 => byte1: u8 {Read, Write},

Oops, I was originally planning to have users specify the size of the padding (syntax A):

0x0 => ctrl: u8 { Write }, 0x1 => _: u8, // One byte of padding 0x2 => status: u16 { Read },

however, when I was working on the README, I realized the current design doesn't require the user to specify the size:

(0x0 => ctrl: WriteOnly<u8>), (0x1 => _reserved), (0x2 => status: ReadOnly<u16>),

I want to migrate as many uses away from register_structs! as possible (because it is unsound), so I want to make the new syntax no less powerful than register_structs! (apart from being MMIO-only, which was a necessary concession to keep the complexity reasonable). I originally tweaked the syntax to optionally allow the size to be inferred, which would've l 8000 ooked like (syntax B):

0x0 => ctrl: u8 { Write }, 0x1 => _: _, // Inferred padding (length will be 0x2 - 0x1) 0x2 => status: u16 { Read },

However I ultimately decided to simplify it and remove the length entirely, making the current syntax (syntax C):

0x0 => ctrl: u8 { Write }, 0x1 => _, // Inferred padding (length will be 0x2 - 0x1) 0x2 => status: u16 { Read },

However, I apparently missed a couple cases of syntax B in README when I made that change. I've updated the README and peripheral!'s docs to show syntax C, which is my current design. Sorry about the confusion, hopefully that clears some things up!

Syntax C is nice. I'm a big fan.

And I believe it mostly clears up my original confusion of whether gaps can be arbitrarily long. When you just have inferred padding, it feels like it could be any size. When you have an explicit inferred type there, it feels like it's constrained to an actual existing type size.

I like syntax C, with future expansion to allow 0x1 => _reserved_with_this_name: [u8; 16], to allow both:

A custom name for reserved fields.

An expected datatype for a reserved field to make its size explicit.

libraries/tock-register-interface/README.md

brghena · 2024-05-22T00:09:17Z

libraries/tock-register-interface/README.md

+```rust
+trait Registers {
+    type a<'s>: tock_registers::Register<DataType = u8> +
+            tock_registers::Read<LongName = ()> +
+            tock_registers::Write<LongName = ()> where Self: 's;
+    fn a(&self) -> Self::a<'_> { ... }
+}
+


Can you show an example with the bitfield parameter too?

There is an example later, under the "Example: Using registers and bitfields" heading. Maybe I should move the "Register Trait Bounds" heading to the end, so "Example: Using registers and bitfields" would come right after "Defining bitfields"?

I don't see a macro expansion for the bitfield parameter under that heading.

Oh, I misunderstood Branden's request. I added an register with a bitfield parameter and its trait expansion to the "Register Trait Bounds" section.

ppannuto

Super excited for this; it looks like a nice set of ergonomic improvements on top of the safety fixes, and is designed nicely for minimum churn with extant code.

One general question, should we use Fake or Mock consistently through the documents and types here? I have vanishingly little experience with real-world unit testing, but I thought Mock was the more standard term?

libraries/tock-register-interface/src/peripheral.rs

ppannuto · 2024-05-22T17:31:35Z

libraries/tock-register-interface/README.md

-        (0x100 => @END),
+        // If you have several adjacent registers, you can specify an array
+        // register:
+        0x00C => array: [u32; 4] { Read, Write },


Suggested change

0x00C => array: [u32; 4] { Read, Write },

0x00C => gpioPins: [u32; 4] { Read, Write },

Maybe something like this for the example? When I first parsed this, I thought array was a keyword for the macro indicating an array of regsiters.

If I would like to specify a specific bitfield type, would I write:

gpioPins: [u32(Gpio::Bank); 4] { Read, Write },

I had a request to allow registers to be specified by type alias, i.e.:

peripheral! { GpioPort { 0x0 => pins: GpioPins(GpioPin::Register) { Read, Write }, } } type GpioPins = [u8; 16];

That requires the traits to infer the fact the register is an array from the type itself. As a result, peripheral! treats the type as an opaque value (a syn::Type) and does not try to determine whether a register is an array or scalar. That also rules out the [u32(Gpio::Bank); 4] syntax.

@kupiakos FYI

I was thinking more like this syntax:

peripheral! { GpioPort { 0x0 => pins: [Gpio::Bank; 4] { Read, Write }, } }

Is there a specific reason to provide the primitive type backing a bitfield inside of peripheral!? The bitfield register type should map to exactly one primitive type. I could see some value in documentation like the explicit offsets.

That requires the traits to infer the fact the register is an array from the type itself.

Yes, but not via any specialization.

Another alternative syntax could be:

peripheral! { GpioPort { 0x0 => pins: [u32; 4](GpioPin::Register) { Read, Write }, } }

I was thinking more like this syntax:

peripheral! { GpioPort { 0x0 => pins: [Gpio::Bank; 4] { Read, Write }, } }

Is there a specific reason to provide the primitive type backing a bitfield inside of peripheral!? The bitfield register type should map to exactly one primitive type. I could see some value in documentation like the explicit offsets.

I would love for users to only have to specify the bitfield type and have the macro/registers infer it. However, currently bitfield types implement the RegisterLongName trait, which doesn't indicate the underlying primitive type that it applies to. I would love to change the trait to:

pub trait RegisterLongName { // The underlying data type of this register (the u8/u16/u32/etc // passed over the MMIO bus). type Primitive: UIntLike; }

However, many (most?) registers use the following RegisterLongName impl:

impl RegisterLongName for () {}

regardless of the primitive type. So adding Primitive would be a massively breaking change.

Instead, we could add a new trait that contains both the bitfield implementation and primitive, but that would result in sizable changes to the register_bitfield! infrastructure. I have two reasons for not implementing those changes:

I haven't used register bitfields enough to be confident implementing a v2 of it.

I'm on a deadline to implement these changes which does not support rewriting register_bitfield!.

Another alternative syntax could be:

peripheral! { GpioPort { 0x0 => pins: [u32; 4](GpioPin::Register) { Read, Write }, } }

This is the syntax I envisioned using; I added an example to the README. Definitely changeable though.

I figured out how to make peripheral! work without requiring the user to specify both the value type and the bitfield type and also without totally rewriting the bitfield stuff. This leads to the following syntax:

use tock_registers::{Aliased, peripheral, register_bitfields}; register_bitfields![u32, Control [ ... ] Pin [ ... ] Status [ ... ] Trigger [ ... ] ]; peripheral! { Foo { 0x0 => control: Control::Register { Write }, // Array registers can have bitfields as well. The bitfield applies to // each register in the array. 0x4 => pins: [Pin::Register; 4] { Read, Write }, // Registers can have different bitfields for read operations than for // write operations. In that case, specify the type as // Aliased<read bitfield, write bitfield> 0x8 => aliased: Aliased<Status::Register, Trigger::Register> { Read, Write }, // Array registers can a 1E80 lso be aliased: 0xc => motors: [Aliased<Status::Register, Trigger::Register>; 4] { Read, Write }, } }

The value type (u32 for all the registers here) is taken from the register_bitfields! invocation (transmitted through some new traits).

I personally prefer this; I always thought that specifying both the value type and the bitfield is redundant. However, this was a pretty big change to how the macro works, and making it involved deleting a bunch of complex code. Therefore I pushed it as a separate commit, so I can roll it back if you don't like the changes. I'm a little unsure of the change because I do not understand why the RegisterLongName and value type were specified separately in the first place.

I can't really make further progress on this PR until I know which way we're going on this issue (the design diverged pretty hard), so lets discuss this at this Friday's core WG call and reach a consensus on which way to go.

Oh, and feel free to register your opinion on this latest change by using thumbs up and thumbs down on this comment.

I think the reason to specify them separately in the old (well... current) syntax is because the width of the field determines the offset of the next field. While, in this new syntax, that's unnecessary since the offset of the next field is expressed explicitly. So this seems striclty better

By "old" syntax, are you referring to the syntax that predates offset tests? i.e.:

#[repr(C)] struct Registers { a: ReadWrite<u32, Ctrl::Register>, b: ReadWrite<u8, Status::Register>, }

If so that makes sense to me!

My opinions:

The new syntax is strictly an improvement.

It would be less confusing as a user to have the ::Register type be a #[repr(transparent)] struct over the data type with the expected size, rather than with another ZST and associated type - especially when u32 and u8, placed in the same location, are the actual primitive types being read.

In today's core WG call, we decided to move forward with this new syntax. I will squash the two commits in this PR together and move forward with the new design.

ppannuto · 2024-05-22T17:39:08Z

libraries/tock-register-interface/src/peripheral.rs

+/// This argument is used to support unusual chip designs (mainly some LiteX
+/// designs). Because the bus adapter can change the size of registers (e.g. a
+/// register with a data type of `u8` may take up more than 1 byte in the MMIO
+/// address space), `peripheral!` allows you to specify that an offset is to be
+/// inferred:
+/// ```
+/// tock_registers::peripheral! {
+///     #[allow_bus_adapter]
+///     Foo {
+///         // The first register is still at offset 0.
+///         0x0 => a: u8 { Read },
+///
+///         // This register has an inferred offset, which may depend on the bus
+///         // adapter passed to RealFoo.
+///         _ => b: u32 { Write },
+///     }
+/// }
+/// ```
+/// Note that a padding field cannot be followed by a field with an inferred
+/// offset.


I would like to preface this question with, 'this is bananas, and I hate that LiteX has created this headache'—

How is endianness expressed in the context of a bus adapter? i.e. if you have nominal u8's being pushed into u32 padding, does the u8 sit at the top or bottom of the resulting u32 (and what controls this)?

I just pushed my first revision of the BusAdapter trait: https://github.com/jrvanwhy/tock/blob/testable-registers-2/libraries/tock-register-interface/src/bus_adapter.rs

tock-registers doesn't see the individual memory accesses required to access a register value, the BusAdapter handles that. So endianess is the BusAdapter's responsibility.

ppannuto · 2024-05-22T17:46:27Z

libraries/tock-register-interface/src/peripheral.rs

+///         // This register is only present if the bounce feature is enabled.
+///         #[cfg(feature = "bounce")]
+///         0x1 => bounce: u8 { Read, Write },
+///
+///         // Often, you will need to place a register with an inferred offset
+///         // (or padding) after a register with a `cfg` attribute, because the
+///         // offset may change depending on the crate's enabled features.
+///         _ => next_register: u8 {},


This scares me a bit. I think I'd prefer forcing people to be more explicit, i.e. include the complimentary cfg(not with a fixed size padding.

More generally, I think we should restrict inferred offsets to only work with allow_bus_adapter. It can be a really rough bug to track down when memory map definitions are subtlety off. cfg is risky as-is. I think requiring an explicit address for post-cfg stuff will help minimize the chance of hidden errors by providing more opportunity for structure validation.

Upon more reflection, I'm starting to lean against allowing inferred offsets in any context. I think this is an area where explicit beats implicit. Inferred padding length is a reasonable ergonomic thing, but for any 'real' / accessible thing, I think we should require explicit placement / definition.

because the offset may change depending on the crate's enabled features.

Does this mean there is hardware where, say, the Control register is at address 0x8 but then on another variant of the chip that supports bounce the Control register would be at address 0xC? That's pretty scary.

It seems like this implicit placement would have to cascade to all future registers in the peripheral, so you could just use _ as the address for all of them? That seems like something we don't want.

ppannuto · 2024-05-22T18:01:49Z

libraries/tock-register-interface/src/peripheral.rs

+///
+///         // This register has an inferred offset, which may depend on the bus
+///         // adapter passed to RealFoo.
+///         _ => b: u32 { Write },


The more I think about this, the more complex it starts to feel to me. Does this mean that for something which is nominally

Foo { 0x0 => zero: u8 0x1 => one: u16 0x3 => three: u8 0x4 => four: u32 0x8 => eight: u8 }

on a "byte-aligned bus" the struct would be unchanged, but otherwise

// halfword-aligned bus becomes Foo { 0x0 => zero: u8 0x1 => _ 0x2 => one: u16 0x4 => three: u8 0x5 => _ 0x6 => four: u32 0xa => eight: u8 } // word-aligned bus becomes Foo { 0x0 => zero: u8 0x1 => _ 0x2 => _ 0x3 => _ 0x4 => one: u16 0x6 => _ 0x7 => _ 0x8 => three: u8 0x9 => _ 0xa => _ 0xb => _ 0xc => four: u32 }

What happens if a struct like this has padding??

// byte-aligned bus Nominal { 0x0 => zero: u8 0x1 => _: u8 0x2 => two: u16 } // half-word does... this? ExpandExplicitPadding { 0x0 => zero: u8 0x1 => _ // from bus expansion 0x2 => _: u8 // the originally defined padding 0x3 => _ // from bus expansion 0x4 => two: u16 } // or this? BusUsesExtantPadding { 0x0 => zero: u8 0x1 => _: u8 // originally defined padding meets bus expansion needs 0x2 => two: u16 } // but then a word-aligned bus would need to add padding no matter what?

I don't know what's better here, but the more I think about things, the more "inferred offset" scares me.

One, kind of verbose, concrete suggestion might be something like:

/// tock_registers::peripheral! { /// #[allow_bus_adapter(u8,u32)] /// Foo { /// 0x0,0x0 => a: u8 { Read }, /// 0x1,0x4 => b: u8 { Read },

where the allow_bus_adapter explicitly states which alignments are allowed, and then the register placement definitions explicitly show how things are laid out, based on the underlying alignment?

From tock-registers' perspective, the padding in LiteX registers is BusAdapters' problem, and does not show up in anything peripheral! does. peripheral! just sees that a register's size may differ from its data type's size.

My understanding is that LiteX registers do not have padding, @lschuermann to confirm.

To make your suggestion match my understanding, I think we could do:

use litex_registers::{C8B32, C32B32}; tock_registers::peripheral! { #[allow_bus_adapter(C8B32, C32B32)] Foo { [0x0, 0x0] => a: u16 { Read }, [0x8, 0x4] => b: u8 { Read }, } }

But I would have to implement it to be sure.

jrvanwhy · 2024-05-22T19:25:17Z

Awesome! Very excited to see some progress here.

Can you briefly compare to prior approaches, namely your #3405 PR?

Ultimately, #3405 was closed because it was unsound (apparently I closed it without saying why). It relied on deriving pointers to the MMIO registers from references to zero-sized-types, which is unsound under most (all?) of the pointer provenance models being considered for Rust.

Similarities:

The syntax for declaring registers is largely similar to [RFC] tock-registers macro rewrite try 2 #3405.
This has the same main goals (avoid the VolatileCell unsoundness and allow for unit testing), and also supports unsafe operations.

Differences:

I am much more confident in the soundness of this design, as it uses raw pointers and offsetting to access the registers with no funny business, unlike every previous attempt I've made.
This PR does not keep the field access syntax for accessing registers, instead using accessor methods (i.e. in [RFC] tock-registers macro rewrite try 2 #3405 you might say registers.ctrl.modify(...) but in this PR you write registers.ctrl().modify(...)).
This PR supports LiteX chips, and I don't think I could've added support into the previous designs.
This design is much more fleshed-out (I've been working on it quite a bit longer than I was working on [RFC] tock-registers macro rewrite try 2 #3405).
This design makes it much easier to implement fake versions of a register. In the previous design, you would be writing a lot of boilerplate for each register.
This design doesn't try to support non-MMIO register types (such as CSRs). Theoretically some of the traits are reusable (just as Readable/Writable can be used), but the macro is hardcoded to have "read" and "write" as the two fundamental operations.

jrvanwhy · 2024-05-22T20:08:57Z

One general question, should we use Fake or Mock consistently through the documents and types here? I have vanishingly little experience with real-world unit testing, but I thought Mock was the more standard term?

I've been using the terms the way they are used internally at Google; those definitions line up with the definitions of "Fake", "Stubs", and "Mocks" under the "From Martin Fowler about Mock and Stub" at https://stackoverflow.com/a/346440.

FakeRegister is specifically designed to support fakes, though it can be used to implement stubs and probably mocks as well.

The rest of this comment explains the different meanings in my own words:

Suppose, for example, that you have a function that depends on an RNG:

trait Rng {
    fn get_random(&mut self) -> u32;
}

// Returns true with 50% probability.
fn please_test_me<R: Rng>(rng: &R) -> bool {
    rng.get_random() % 2 != 0
}

The real implementation of Rng might make system calls, e.g. to open /dev/urandom and read bytes out of it. That will result in unpredictable behavior in a unit test.

A test using stubs would look like:

struct StubEvenRng;
impl Rng for StubEvenRng {
    fn get_random(&mut self) -> u32 {
        4
    }
}

struct StubOddRng;
impl Rng for StubOddRng {
    fn get_random(&mut self) -> u32 {
        7
    }
}

#[test]
fn unit_test() {
    assert!(!please_test_me(&mut StubEvenRng));
    assert!(please_test_me(&mut StubOddRng));
}

The stub is incredibly simple to write, but not very useful. E.g. you can't use it to thoroughly test a method that calls the RNG twice.

A mock is too long to write here (and would generally be autogenerated by a library), but using it would look like:

#[test]
fn unit_test() {
    let mut mock = MockRng::new();
    
    // Tell mock to return 4 then return 7
    mock.on_call_return(4);
    mock.on_call_return(7);
    
    assert!(!please_test_me(&mut mock));
    assert!(please_test_me(&mut mock));
}

You can use a mock to test a function that calls the RNG multiple times, because you can program it to make any sequence of outputs.

A fake is generally handwritten and doesn't require programming from within the unit test:

#[derive(Default)]
struct FakeRng {
    // Our pseudo-rng algorithm is to return incrementing integers
    counter: u32,
}

impl Rng for FakeRng {
    fn get_random(&mut self) -> u32 {
        self.counter += 1;
        self.counter
    }
}

#[test]
fn unit_test() {
    let mut fake = FakeRng::default();
    assert!(please_test_me(&mut fake));
    assert!(!please_test_me(&mut fake));
}

The fake is more work to write, but is a much more versatile tool. In many cases, you can write a single fake of something, then use it for many unit tests. If you wrote a fake version of every peripheral a Tock kernel needs, you could spin the entire kernel up on the host system using all fake hardware! That's not practical with mocks/stubs, and is a use case I want to support.

libraries/tock-register-interface/README.md

jrvanwhy · 2024-05-28T22:09:03Z

On the core WG call, someone asked for a document describing how code using tock-registers changes.

`register_structs!` version

First, here's a "before" example of a theoretical driver that uses tock_registers!:

use tock_registers::{register_bitfields, register_structs};
use tock_registers::registers::{ReadOnly, ReadWrite, WriteOnly};

register_bitfields![u32,
    Control [
        RANGE OFFSET(4) NUMBITS(2) [
            VeryHigh = 0,
            High = 1,
            Low = 2
        ],
        EN  OFFSET(3) NUMBITS(1) [],
        INT OFFSET(2) NUMBITS(1) []
    ]
]

register_structs! {
    Registers {
        (0x000 => cr: ReadWrite<u8, Control::Register>),
        (0x001 => s: ReadOnly<u8, Status::Register>),
        (0x002 => byte0: ReadWrite<u8>),
        (0x003 => byte1: ReadWrite<u8>),
        (0x004 => short: ReadWrite<u16>),
        (0x006 => _reserved),
        (0x008 => word: ReadWrite<u32>),
        (0x00C => array: [ReadWrite<u32>; 4]),
    }
}

struct Driver {
    registers: StaticRef<Registers>,
}

impl Driver {
    fn do_stuff(&self) {
        // Basic read + write of a register.
        self.registers.cr.set(self.registers.cr.get() + 1);

        // Reading from a bitfield
        let range: u8 = self.registers.cr.read(Control::RANGE);

        // Reading as an enum
        let range = self.registers.cr.read_as_enum(Control::RANGE);

        // Modification
        self.registers.cr.modify(Control::EN::SET);

        // Array access
        self.registers.array[2].set(3);
    }
}

`peripheral!` version

Here's the same theoretical driver implemen BE96 ted using peripheral!:

use tock_registers::{peripheral, register_bitfields, Read, ReadWrite, Register, Write};

// The register_bitfields! invocation is unchanged.
register_bitfields![u32,
    Control [
        RANGE OFFSET(4) NUMBITS(2) [
            VeryHigh = 0,
            High = 1,
            Low = 2
        ],
        EN  OFFSET(3) NUMBITS(1) [],
        INT OFFSET(2) NUMBITS(1) []
    ]
]

peripheral! {
    Registers {
        0x000 => cr: u8(Control::Register) { Read, Write },
        0x001 => s: u8(Status::Register) { Read },
        0x002 => byte0: u8 { Read, Write },
        0x003 => byte1: u8 { Read, Write },
        0x004 => short: u16 { Read, Write },
        0x006 => _,
        0x008 => word: u32 { Read, Write },
        0x00C => array: [u32; 4] { Read, Write },
    }
}

// Driver becomes generic over the Registers implementation.
struct Driver<R: Registers> {
    registers: R,
}

impl<R: Registers> Driver<R> {
    fn do_stuff(&self) {
        // cr is now a function rather than a field. get() was renamed
        // to read() and set() was renamed to write().
        self.registers.cr().write(self.registers.cr().read() + 1);

        // read() was renamed to read_field()
        let range: u8 = self.registers.cr().read(Control::RANGE);

        // Reading as an enum
        let range = self.registers.cr().read_as_enum(Control::RANGE);

        // Modification
        self.registers.cr().modify(Control::EN::SET);

        // You can no longer index into arrays using []. Instead, call
        // .get(), which will return an Option<ArrayElement<_>>, and
        // use that. Alternatively, call .iter() to get an
        // ArrayIterator<_> and use that instead.
        self.registers.array().get(2).unwrap().set(3);
    }
}

// In the board's main file, instantiate one of the following:
// 1. Driver<RealRegisters<tock_registers::ConstMmioPointer<0x12345678>>>
// 2. Driver<RealRegisters<tock_registers::DynMmioPointer>>

Note that I have not yet explained ConstMmioPointer and DynMmioPointer. They are types representing pointers to MMIO peripherals.

ConstMmioPointer is a zero-sized type (ZST) that contains the peripheral's address as a const generic argument. In that case RealRegisters -- and the per-register types -- would be ZSTs and therefore have no runtime overhead.

DynMmioPointer is a wrapper around *mut (). It takes up space, but if you have multiple copies of the same peripheral (e.g. OpenTitan has 4 UARTs) it is probably worth it to use a little extra RAM to avoid monomorphizing the entire driver several times.

kupiakos · 2024-05-28T23:37:12Z

libraries/tock-register-interface/README.md


-register_structs! {
+```rust
+tock_registers::peripheral! {
    Registers {


rustdoc is supported in this position as well, correct (as documented by peripheral!)? Consider adding a /// Documentation for Registers or similar line here

Yes, it is. I added lines like that in both README and peripheral! to make it clear that doc comments are supported on both peripheral definitions and fields.

kupiakos · 2024-05-28T23:40:13Z

libraries/tock-register-interface/src/peripheral.rs

+///         // triggers a memory-unsafe operation).
+///         0x2 => c: u8 { UnsafeRead },
+///
+///         // Any combination of readable, writable, safe, and unsafe is


What about 0x2 => c: u8 { UnsafeRead, Read }. What happens?

Ah, my wording was extremely unclear. That results in a compile error -- I support at most one read-type operation and at most one write-type operation. I reworded that example to make it clear what is and is not supported.

kupiakos · 2024-05-28T23:42:51Z

libraries/tock-register-interface/src/peripheral.rs

+/// ```
+/// Padding fields are ommitted from the generated trait.
+///
+/// # Doc comments


Consider either:

Removing this section and place the rustdoc comments inside the starting example (as it should be immediately apparent and obvious to users), or

Combining with the cfg attributes section below, as doc comment support is an extension of supporting attributes in general.

I did a bit of both. Added doc comments inside the starting example, then combined this with the cfg attribute section. I used the new combined section to document where the doc comments end up.

kupiakos · 2024-05-28T23:44:36Z

libraries/tock-register-interface/src/peripheral.rs

+/// instead of their safe equivalents.
+///
+/// # `cfg` attributes
+/// Peripheral declarations can have `cfg` attributes:


We have a set of attribute-position proc macros that yield cfg attributes - e.g. #[cfg_device] and #[cfg_host] both yield some long #[cfg]. Would they work in this position?

It's fine if they can't work here, as we have simple workarounds - but the lack of support for custom attributes should be documented if so

jrvanwhy · 2024-05-29T18:30:56Z

I had a sentence buried in README.md explaining that the rustdoc on peripheral! is more complete than the description in the README, but I think some of the reviewers missed it. I made it more prominent, and am copying it here so that you all are aware:

Note: This README introduces the `peripheral!` macro and describes its
most-commonly-used features. `peripheral!` has several more pieces of
functionality that are not described in this document; see its Rustdoc for
complete documentation.

bradjc · 2024-05-30T19:34:44Z

Does this work?

peripheral! {
    Registers {
        0x000 => cr: u8(Control::Register) { Read, Write },
        0x001 => _,
        0x008 => word: u32 { Read, Write },
        0x00C => _,
        0x010 => array: [u32; 4] { Read, Write },
    }
}

As in, can there be multiple _ registers?

I think you have a bug in your example (should be read_field()):

        // read() was renamed to read_field()
        let range: u8 = self.registers.cr().read(Control::RANGE);

I like this:

        // You can no longer index into arrays using []. Instead, call
        // .get(), which will return an Option<ArrayElement<_>>, and
        // use that. Alternatively, call .iter() to get an
        // ArrayIterator<_> and use that instead.
        self.registers.array().get(2).unwrap().set(3);

I'm concerned about this:

// In the board's main file, instantiate one of the following:
// 1. Driver<RealRegisters<tock_registers::ConstMmioPointer<0x12345678>>>
// 2. Driver<RealRegisters<tock_registers::DynMmioPointer>>

I assume that we can use the same default peripherals trick we do now, and most boards won't have to worry about these details, but board authors who want to specify exactly what is included can do so. If not, then I'm worried this is a lot of complexity to make board authors grapple with.

alevy · 2024-05-31T13:14:46Z

I assume that we can use the same default peripherals trick we do now, and most boards won't have to worry about these details

I don't see why not, and also I actually think these might be more appropriate to define in chip crates anyway

lschuermann

I did not do an in-depth review yet, but this looks good to me generally.

I'd try to keep changes to the API surface exposed by the Read (ex Readable) and Write (ex Writeable) to a minimum. That includes re-introducing a ReadWrite trait that's auto-derived on a Read + Write field. I'd be happy to make these changes if you like.

~~I think this is also (deliberately?) missing Aliased... How do we feel about that? It is at least somewhat common to have an aliased register in, e.g., UART peripherals as a TX sink / RX source.~~ Nevermind, it seems like the proposal does work for that, sorry!

lschuermann · 2024-05-31T15:39:35Z

libraries/tock-register-interface/src/register_traits/mod.rs

+/// A register that can be read, but which is not memory-safe to read.
+pub trait UnsafeRead: Register {
+    /// # Safety
+    /// Reading this register has hardware-specific safety requirements which
+    /// the caller must comply with.
+    unsafe fn read(self) -> <Self::DataType as DataType>::Value
+    where
+        Self::DataType: ScalarDataType;
+
+    /// Read from an unsafe array register. Instead of using `read_at_unchecked`
+    /// directly, callers are encouraged to call `get()` to get an
+    /// `ArrayElement` pointing at the register, then invoke `read` on that.
+    ///
+    /// # Safety
+    /// `index` must be less than `Self::LEN`.
+    /// Reading this register has hardware-specific safety requirements which
+    /// the caller must comply with.
+    unsafe fn read_at_unchecked(self, index: usize) -> <Self::DataType as DataType>::Value
+    where
+        Self::DataType: ArrayDataType;
+}
+
+/// A regsiter that can be written, but which is not memory-safe to write.
+pub trait UnsafeWrite: Register {
+    /// # Safety
+    /// Writing this register has hardware-specific safety requirements which
+    /// the caller must comply with.
+    unsafe fn write(self, value: <Self::DataType as DataType>::Value)
+    where
+        Self::DataType: ScalarDataType;
+
+    /// Write to an unsafe array register. Instead of using `write_at_unchecked`
+    /// directly, callers are encouraged to call `get()` to get an
+    /// `ArrayElement` pointing at the register, then invoke `read` on that.
+    ///
+    /// # Safety
+    /// `index` must be less than `Self::LEN`.
+    /// Writing this register has hardware-specific safety requirements which
+    /// the caller must comply with.
+    unsafe fn write_at_unchecked(self, index: usize, value: <Self::DataType as DataType>::Value)
+    where
+        Self::DataType: ArrayDataType;
+}


I'd prefer if we, for this particular PR, remove the content (i.e. methods) contained in these traits. I'm not strongly opposed to introducing the notion of "safety" into the register interface itself, but it's a can of worms that I think we should be carefully discussing in a follow-up PR. We do have some good arguments as to why our current approach of "unsafe construction, safe usage" as discussed in #3244 and #2593 is at least sufficient, if not preferable.

In short, with this PR merged we'd still be able to specify that a register is UnsafeRead or UnsafeWrite, but those registers would -- for the time being -- not expose any useful interfaces.

We should then reason about when exactly a register must be marked unsafe, which operations it should support, and how those operations are implemented. In particular, we may want an UnsafeRead or UnsafeWrite to not be traits with first-class access methods (such as the current read / write), but simply have an unsafe access() method that then returns a reference to something that implements Read or Write, and as such having the same API surface as the safe counterparts.

Requiring that all safe usage of all of a peripheral's register is correct at the point of the peripheral's construction results in facile load-bearing unsafe blocks and should be avoided. Consider that future edits to safe code will rarely check that unsafe preconditions are being upheld, so load-bearing safe code should be kept as local as possible.

we may want an UnsafeRead or UnsafeWrite to not be traits with first-class access methods... simply have an unsafe access() method that then returns a reference to something that implements...

This is morally equivalent to unsafe { &*ptr }; it's not in any way mutually exclusive with locally-unsafe reads/writes. I would also recommend people prefer the locally-unsafe read/writes as the unsafe block requires local reasoning about state.

A similar option with a similar caveat would be to have the trait method to retrieve the register type from the peripheral type be made unsafe for unsafe registers. That would block this PR on changing the safety of the register access and I'd suggest a syntax more like { unsafe Read, unsafe Write }.

One advantage of that is that clippy::missing_safety_doc can be made to enforce that # Safety doc strings are present for register fields that contain UnsafeRead or UnsafeWrite, instead of having a generic # Safety 🤷🏻 section in the trait.

However, a disadvantage is it's no longer clear how to allow for registers that are unsafe to write + safe to read or vice versa, along with the safety caveat.

Overall I'd still prefer some way for peripheral macro users to document safety conditions that will be raised at the point where the UB may be exercised.

I don't think I'm opposed to anything you've written. It's just that I'd like to defer having exactly this discussion to a follow-up PR.

Whether we want unsafe methods that read/write directly, or ones that yield another ephemeral reference that can then be used safely seems like a mostly stylistic question. While I don't disagree with the unsafe { &*ptr } argument above, it would be nice if we could avoid duplication of Read and UnsafeRead trait implementations, and it can also be nice to be able to "lease out" a safe reference to something that's generally UnsafeRead or UnsafeWrite when the unsafe code can assert that the respective operations are safe temporarily, for the lifetime of the handed-out reference.

Still, those are discussions that arguably we ought to have in a follow-up PR. This is complex enough already without diving in to these interface discussions. They're also orthogonal -- this PR is quite nice as it changes little about the Read/Write (ex Readable/Writeable) interfaces. In line with that, let's defer introduction of the UnsafeRead etc. traits to a follow-up PR.

This PR introduces `peripheral!` to `tock_registers`. `peripheral!` is a successor to `register_structs!` that makes several improvements: 1. It is sound. The types generated and used by `peripheral!` do not use `&` references into MMIO memory -- instead, only raw pointers are used. This avoids the unsoundness described in unsafe-code-guidelines issue 411. 2. It supports driver unit tests. Instead of only emitting a `struct`, it emits a trait representing the hardware's capabilities, which can be used by unit tests to fake or stub out hardware behavior. Of course, it also emits a struct that implements the trait by interacting with the real hardware. 3. It supports unsafe operations, as described in tock issue 3244. 4. It supports conditionally-compiled registers, as described in tock issue 3257. There is one major drawback relative to `register_structs!`: `peripheral!` is only designed to support MMIO registers. This was done to keep the implementation complexity manageable. For a basic introduction of its usage, see `libraries/tock-register-interface/README.md`, and for detailed documentation, see `libraries/tock-register-interface/src/peripheral.rs`.

With this change, the user can just specify a register bitfield type, rather than having to specify both the primitive type and the long name. This cuts down on duplication. This also simplifies how aliased registers are specified, making the user manually write out the `Aliased<>` type.

ppannuto · 2024-08-27T23:46:47Z

No action required from this comment, just linking an updated reference.

Upstream had a brief conversation around this today ( rust-embedded/wg#387 ), and consensus / consolidated around the outcome that designs should only use raw pointers for MMIO accesses [which this update already does]. There is a new issue in the embedded wg tracking their progress to that end: rust-embedded/wg#791, which I'm posting here in case their work is a useful reference at any point.

brghena · 2025-04-03T00:56:12Z

Something x86 includes is port-mapped I/O. For example, from the recent PR (#4385):

Definition:

tock/chips/pc/src/pit.rs

Line 85 in fa7269a

const PIT_MCR: u16 = 0x0043;
Usage:

tock/chips/pc/src/pit.rs

Line 147 in fa7269a

io::outb(PIT_MCR, pit_mcr.get());

The existing Tock registers interface does support this, but I believe this new system could in a very similar fashion to the bus_adapter example.

I don't think Port-Mapped I/O is going to be especially common in Architectures Tock supports, but I see it as a validation of this implementation direction, and I wanted to leave a note here about it.

jrvanwhy self-assigned this May 21, 2024

github-actions bot added the tock-libraries This affects libraries supported by the Tock project label May 21, 2024

brghena reviewed May 22, 2024

View reviewed changes

ppannuto reviewed May 22, 2024

View reviewed changes

jrvanwhy force-pushed the testable-registers-2 branch from 5bce296 to e84f756 Compare May 22, 2024 19:14

kupiakos reviewed May 24, 2024

View reviewed changes

libraries/tock-register-interface/README.md Show resolved Hide resolved

jrvanwhy force-pushed the testable-registers-2 branch from e84f756 to de186d4 Compare May 28, 2024 21:18

kupiakos reviewed May 28, 2024

View reviewed changes

jrvanwhy force-pushed the testable-registers-2 branch from de186d4 to 4f56791 Compare May 29, 2024 18:19

lschuermann reviewed May 31, 2024

View reviewed changes

lschuermann mentioned this pull request Jun 19, 2024

register_structs! macro: reached recursion limit during macro expansion #2941

Open

jrvanwhy force-pushed the testable-registers-2 branch from 092979d to 47a2705 Compare July 3, 2024 21:46

Johnathan Van Why added 2 commits July 9, 2024 14:40

jrvanwhy force-pushed the testable-registers-2 branch from 47a2705 to 6dd05b3 Compare July 9, 2024 21:41

lschuermann mentioned this pull request Aug 3, 2024

Move Segger RTT to own chip crate and fix soundness issues with externally shared memory #4129

Merged

2 tasks

ppannuto mentioned this pull request Jan 28, 2025

kernel::utilities::registers interfaces should have different types for address and value #4326

Closed

lschuermann mentioned this pull request Feb 24, 2025

rust: update nightly to 2025-02 #4348

Merged

2 tasks

	0x00C => array: [u32; 4] { Read, Write },
	0x00C => gpioPins: [u32; 4] { Read, Write },

tock_registers::peripheral!: Sound and testable register definitions #4001

Are you sure you want to change the base?

tock_registers::peripheral!: Sound and testable register definitions #4001

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

register_structs! version

peripheral! version

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

`tock_registers::peripheral!`: Sound and testable register definitions #4001

`tock_registers::peripheral!`: Sound and testable register definitions #4001

`register_structs!` version

`peripheral!` version