Re: [PATCH v6 0/5] Introduce bitfield and move register macro to rust/kernel/

[Joel Fernandes <joelagnelf@nvidia.com>]

Danilo, Yuri, Miguel, John, all,

On 10/7/2025 9:16 AM, Danilo Krummrich wrote:
> On Tue Oct 7, 2025 at 12:36 PM CEST, Alexandre Courbot wrote:
>> Because letting it fully mature within nova-core also has the drawback
>> that we might miss the perspective of other potential users, which may
>> make us draw ourselves into a corner that will make the macro less
>> useful generally speaking. We are at a stage where we can still make
>> design changes if needed, but we need to hear from other users, and
>> these won't come as long as the macro is in nova-core.
> 
> There are two different things here that are getting mixed up a bit.
> 
>   (1) Moving the register!() code out of nova-core to make it accessible for
>       other drivers.
> 
>   (2) Generalize the bitfield implementation that so far is baked into the
>       register!() code.
> 
> Both of those make sense, but they don't have to happen at the same time
> necessarily.
> 
> Now, I'm not saying that we necessarily have to change the approach here. The
> current merge window isn't even closed, so we have plently of time left, i.e.
> there's no rush with with patch series.
> 
> However, if it helps, I'm perfectly fine to take the register!() implementation
> into the I/O entry in a first step and in a second step generalize the bitfield
> implementation and move it out of the register!() code.
> 
> Again, there's no rush as far as I'm concerned, yet the latter approach might
> add a bit more structure and hence run a bit smoother.

In my view it is better to move both bitfield and register macros together
because if we only moved register, it means we would have no bitfield support
for the page table / mm use case I just posted a patch for (which is why I
started looking into Bitfield support initially) unless we create a copy of just
the bitfield code within nova which we definitely shouldn't I think. So I think
it is best to move both.

I think we can address Yuri’s concerns shortly. All the outstanding concerns
(including Yuri's) are as follows (and sorry Yuri to send the v6 when we were
still discussing v5, but please let me know if I missed anything):

1. Undefined bitfields to be zero'ed to avoid undefined behavior:  Unlike C,
this is not a concern in Rust because we will be *defining* the raw value
explicitly as an inner integer. In register! macro usecases in fact, we do
store/read the entire inner value some times so we have to support that (without
zero'ing).

2. The setter API should fail or trim inputs based on the (un)defined bitfields:
This should not be an issue after Alex's idea on moving this issue to the type
system [1]. The compilation simply fails for invalid inputs.

3. Build pattern chaining of .set_XX(...).set_YY(...):  Yuri requested to drop
this. I discussed with Alex and we feel this is idiomatic rust and we ought to
support it.

4. Concerns about "a.set_foo(XX)" pattern ignoring returning value resulting in
a NOOP: Mark the setter API #[must_use] to prevent patterns where the return
value is discarded, ex: a.set_foo(XX);

5. MAINTAINERS file updates: Miguel and Yuri replied to this so we can adopt
their guidance. Currently I am considering Alex and me as M: and Yuri as R:. For
git tree, let me know which tree we want to use or could create a new one. For
the IO (register macro) change, I can add add an entry to the existing IO record.

Does this all sound good? If not please raise your concern as a reply and let us
discuss it.

As next steps, the only thing left is to address #2 (unless anyone replies with
any objections). However, IMO #2 can be addressed once the code moves out as
well (it might be a bit of time as we need the bounded int changes in case we
want to address #2 before moving code outside of nova). Thoughts? Everything
else above (expect #2) is good to go and I can roll it into v7 soon.

Any other thoughts or concerns? Thanks.

[1] https://lore.kernel.org/all/20251002-bounded_ints-v1-0-dd60f5804ea4@nvidia.com/