Re: [PATCH RFC] rust: add macros to define registers layout

Danilo Krummrich
2025-03-13T17:14:08Z

Hi Alex,

Thanks for working on a generic solution for this! Few comments below.

On Thu, Mar 13, 2025 at 11:48:25PM +0900, Alexandre Courbot wrote:
> Add two macros, reg_def!() and reg_def_rel!(), that define a given
> register's layout and provide accessors for absolute or relative
> offsets, respectively.
>
> The following example (taken from the rustdoc) helps understanding how
> they are used:
>
> reg_def!(Boot0@0x00000100, "Basic revision information about the chip";

Should we call the macro just `register!`?

> 3:0 minor_rev => as u8, "minor revision of the chip";
> 7:4 major_rev => as u8, "major revision of the chip";
> 28:20 chipset => try_into Chipset, "chipset model"

I think we probably need an argument indicating whether the register field is
{RW, RO, WO}, such that we can generate the corresponding accessors / set the
corresponding masks.

> );
>
> This defines a `Boot0` type which can be read or written from offset
> `0x100` of an `Io` region. It is composed of 3 fields, for instance
> `minor_rev` is made of the 4 less significant bits of the register. Each
> field can be accessed and modified using helper methods:
>
> // Read from offset `0x100`.
> let boot0 = Boot0.read(&bar);
> pr_info!("chip revision: {}.{}", boot0.major_rev(), boot0.minor_rev());
>
> // `Chipset::try_from` will be called with the value of the field and
> // returns an error if the value is invalid.
> let chipset = boot0.chipset()?;
>
> // Update some fields and write the value back.
> boot0.set_major_rev(3).set_minor_rev(10).write(&bar);
>
> Fields are made accessible using one of the following strategies:
>
> - `as <type>` simply casts the field value to the requested type.
> - `as_bit <type>` turns the field into a boolean and calls
> <type>::from()` with the obtained value. To be used with single-bit
> fields.
> - `into <type>` calls `<type>::from()` on the value of the field. It is
> expected to handle all the possible values for the bit range selected.
> - `try_into <type>` calls `<type>::try_from()` on the value of the field
> and returns its result.

I like that, including the conversion seems pretty convenient.

>
> The documentation strings are optional. If present, they will be added
> to the type or the field getter and setter methods they are attached to.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> I have written these initially for the nova-core driver, then it has
> been suggested that they might be useful outside of it as well, so here
> goes.

Feel free to add my Suggested-by. You can also refer to the corresponding task
in our nova-core task list.

>
> This is my first serious attempt at writing Rust macros and I am sure
> there is a lot that is wrong with them, but I'd like to get early
> feedback and see whether this is actually something we want for the
> kernel in general.
>
> The following in particular needs to be improved, suggestions are
> welcome:
>
> - Inner types other than `u32` need to be supported - this can probably
> just be an extra parameter of the macro.

Can't we figure this out from the bit mask in the macro?

> - The syntax can certainly be improved. I've tried to some with
> something that makes the register layout obvious, while fitting within
> the expectations of the Rust macro parser, but my lack of experience
> certainly shows here.

Did you consider proc macros for more flexibility?

> - We probably need an option to make some fields or whole registers
> read-only.

Ah, I see, you thought of this already.

> - The I/O offset and read/write methods should be optional, so the
> layout part can be used for things that are not registers.

I guess you think of shared memory? For DMA we already have the dma_read! and
dma_write! macros that may fit in.