Message-ID: <20250916095918.GA1647262@joelbox2>
Date: Tue, 16 Sep 2025 05:59:18 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Yury Norov <yury.norov@...il.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	dakr@...nel.org, acourbot@...dia.com,
	Alistair Popple <apopple@...dia.com>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	bjorn3_gh@...tonmail.com, Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	John Hubbard <jhubbard@...dia.com>, Timur Tabi <ttabi@...dia.com>,
	joel@...lfernandes.org, Elle Rhumsaa <elle@...thered-steel.dev>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	nouveau@...ts.freedesktop.org
Subject: Re: [PATCH v3 5/5] rust: Add KUNIT tests for bitfield

Hi Jury,
Sorry for late reply, I was busy with conference travel. Now I found a 3 hour
break before my train journey. :-)

On Wed, Sep 10, 2025 at 09:47:04PM -0400, Yury Norov wrote:
> On Wed, Sep 10, 2025 at 07:08:43PM -0400, Joel Fernandes wrote:
> > > You've got only one negative test that covers the .from() method.
> > > Can you add more?
> > 
> > Sure, but note that we can only add negative tests if there is a chance of
> > failure, which at runtime can mainly happen with the fallible usage (?=>
> > pattern). Also just to note, we already at ~300 lines of test code now :)
> > 
> > > 
> > > What if I create a bitfield from a runtime value that exceeds
> > > the capacity?
> > > 
> > >     bitfield! {
> > >         struct bf: u8 {
> > >             0:0       ready       as bool;
> > >             1:1       error       as bool;
> > >             3:2       state       as u32;
> > Here you mean 'as u8', otherwise it wont compile.
> 
> No, I meant u32. Can you explain this limitation in docs please? From
> a user perspective, the 'state' is a 2-bit variable, so any type wider
> than that should be OK to hold the data. If it's just an implementation
> limitation, maybe it's worth to relax it?

Yes it is a limitation because of the way the code does mask and shifts, it
requires the width to not exceed the width of the struct itself. Yes, I can
add a comment.

I think to do what you want, you have to write it as 'as u8 => u32'.
Otherwise it wont compile.

Just to note, the bitfield code reused the code in the register macro, so it
is existing code in nova-core. We can improve it, but I just did a code
movement with few features on top (sizes other than u32 for the overall
struct, visibility etc) so I look at such changes as an improvement on top
(which will be other patches in this series or later). We can certainly
improve the bitfield support now and as we go.

> > >        }
> > >     }
> > > 
> > >     let raw_value: u8 = 0xff;
> > >     let bf = bf::from(raw_value);
> > > 
> > > I guess you'd return None or similar.
> > 
> > No, we would ignore the extra bits sent. There is a .raw() method and 'bf' is
> > 8-bits, bf.raw() will return 0xff. So it is perfectly valid to do so.
> 
> So I'm lost. Do you ignore or keep untouched?

It would be ignored for the field, but kept in the struct. So .raw() will
return the full 8 bits, and the field will return a subset.

> Imagine a code relying on the behavior you've just described. So, I
> create a 5-bit bitfield residing in a u8 storage, and my user one
> day starts using that 3-bit tail for his own purposes.
> 
> Is that OK? Can you guarantee that any possible combination of methods
> that you've implemented or will implement in future will keep the tail
> untouched?
> 
> In bitmaps, even for a single-bit bitmap the API reserves the whole word,
> thus we have a similar problem. And we state clearly that any bit beyond
> the requested area is 'don't care'. It's OK for C. Is it OK for rust?
> 
> (And even that, we have a couple of functions that take care of tails
> for some good reasons.)
> 
> So the question is: do you
>  - provide the same minimal guarantees as C does (undefined behavior); or
>  - preserve tails untouched, so user can play with them; or
>  - clean the tails for user; or
>  - reject such requests?
> 
> Or something else? Whichever option you choose, please describe
> it explicitly.

I feel this is macro-user's policy, if the user wants to use hidden bits,
they should document it in their struct. They could mention it is 'dont care'
or 'do not touch' in code comments. Obviously if they decide not to expose it
in the fields, that would be one way to deter users of the struct from
touching it without knowing what they are done.  In that sense it is
undefined behavior, it is up to the user I'd say. Does that sound reasonable?

> > I don't
> > think we should return None here, this is also valid in C.
> 
> This doesn't sound like an argument in the rust world, isn't? :) I've
> been told many times that the main purpose of rust is the bullet-proof
> way of coding. Particularly: "rust is free of undefined behavior gray
> zone".  
> 

Since we only partially quoted my reply, lets take a step back and paste the
code snip here again. The following should not return None IMO:

     bitfield! {
         struct bf: u8 {
             0:0       ready       as bool;
             1:1       error       as bool;
             3:2       state       as u32;
        }
     }

     let raw_value: u8 = 0xff;
     let b = bf::from(raw_value);

Maybe I used 'ignore' incorrectly in my last reply. The above code snip is
perfectly valid code IMO. Because b.raw() will return 0xff. The fact that we
don't have defined bitfields for the value should not prevent us from
accessing the entire raw value right? If we want, we can set it up as policy
that there are really no undefined bits, everything is defined even if only a
few of them have accessors, and '.raw()' is the ultimate catch-all. Does that
sound reasonable?

> > Sure, I added such a test.
> > 
> > > The same question for the setters. What would happen for this:
> > > 
> > >     let bf = bf::default()
> > >              .set_state(0xf)
> > >              .set_ready(true);
> > > 
> > > I think that after the first out-of-boundary in set_state(), you
> > > should abort the call chain, make sure you're not touching memory
> > > in set_ready() and returning some type of error.
> > 
> > Here, on out of boundary, we just ignore the extra bits passed to set_state. I
> > think it would be odd if we errored out honestly. We are using 'as u8' in the
> > struct so we would accept any u8 as input, but then if we complained that extra
> > bits were sent, that would be odd.
> 
> That really depends on your purpose. If your end goal is the safest API
> in the world, and you're ready to sacrifice some performance (which is
> exactly opposite to the C case), then you'd return to your user with a
> simple question: are you sure you can fit this 8-bit number into a 3-bit
> storage?   

I think personally I am OK with rejecting requests about this, so we can
agree on this.

> > In C also this is valid. If you passed a
> > higher value than what the bitfield can hold, the compiler will still just use
> > the bits that it needs and ignore the rest.
> 
> In C we've got FIELD_{PREP,GET,MODIFY}, implementing the checks.
> So those who want to stay on safe side have a choice.

Ah ok. We can add these checks then for the accessors, I will do so in v4.

> > I added another test here as well, to ensure the behavior is as I describe.
> > 
> > > 
> > > And for this:
> > > 
> > >     let ret: u32 = -EINVAL;
> > >     bf = bf::default();
> > >     bf = bf.set_state(ret);
> > > 
> > > For compile-time initializes, it should be a compile-time error, right?
> > 
> > Yes, since the struct in this example is u8, this wont compile. Yes, I will add
> > a comment.
> 
> So, the following would work?
> 
>      bitfield! {
>          struct bf: u32 {
>              0:0       ready       as bool;
>              1:1       error       as bool;
>              3:2       state       as u32;
>              ...
>          }
>      }
> 
>      let state: u32 = some_C_wrapper(); // returns 0..3 or -EINVAL
>      bf = bf::default();
>      bf = bf.set_state(state);
> 
> That doesn't look right...

I agree with you, a better approach is to reject anything great than 2 bits.
We do agree on that, and I can make that change. *Currently* what happens is
we mask and shift ignoring all extra bits passed, instead of rejecting.

I hope we're on the same page now, but let me know any other concerns. Just
to emphasize again, I moved *existing* code out of the register macro related
to bitfields, so this has been mostly a code move. That said, we can
certainly improve it incrementally.

Thanks!

 - Joel

Powered by blists - more mailing lists