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

[Danilo Krummrich <dakr@kernel.org>]

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.