Danilo Krummrich
2025-Oct-07 13:16 UTC
[PATCH v6 0/5] Introduce bitfield and move register macro to rust/kernel/
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. - Danilo
Joel Fernandes
2025-Oct-07 21:08 UTC
[PATCH v6 0/5] Introduce bitfield and move register macro to rust/kernel/
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 at nvidia.com/