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

Alexandre Courbot
2025-03-13T15:02:37Z

On Fri Mar 14, 2025 at 12:00 AM JST, Boqun Feng wrote:
> 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";
>> 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"
>> );
>>
>> 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.
>>
>> 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.
>>
>> 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.
>> - 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.
>> - We probably need an option to make some fields or whole registers
>> read-only.
>> - The I/O offset and read/write methods should be optional, so the
>> layout part can be used for things that are not registers.
>> - The visibility of the helper macros is a bit of a headache - I haven't
>> found a way to completely hide them to the outside, so I have prefixed
>> them with `__` for now.
>> - Formatting - there are some pretty long lines, not sure how to break
>> them in an idiomatic way.
>>
>> Sorry if this is still a bit rough around the edges, but hopefully the
>> potential benefit is properly conveyed.
>> ---
>> rust/kernel/lib.rs | 1 +
>> rust/kernel/reg.rs | 284 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 285 insertions(+)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 398242f92a961c3a445d681c65449047a847968a..d610199f6675d22fa01d4db524d9989581f7b646 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -69,6 +69,7 @@
>> pub mod prelude;
>> pub mod print;
>> pub mod rbtree;
>> +mod reg;
>
> This is for io registers? Could you please move it into kernel::io
> instead of defining it as a top level mod?

It is (although one could argue that the bitfield accessors can probably
be useful for non-register types as well), and agreed that this would
fit better in kernel::io. Thanks for the suggestion.