On Fri Oct 10, 2025 at 1:40 AM JST, Yury Norov wrote:> Hi Alexandre, > > On Thu, Oct 09, 2025 at 09:37:10PM +0900, Alexandre Courbot wrote: >> Use BoundedInt with the register!() macro and adapt the nova-core code >> accordingly. This makes it impossible to trim values when setting a >> register field, because either the value of the field has been inferred >> at compile-time to fit within the bounds of the field, or the user has >> been forced to check at runtime that it does indeed fit. > > In C23 we've got _BitInt(), which works like: > > unsigned _BitInt(2) a = 5; // compile-time error > > Can you consider a similar name and syntax in rust?I like the shorter `BitInt`! For the syntax, we will have to conform to what is idiomatic Rust. And I don't think we can make something similar to `= 5` work here - that would require overloading the `=` operator, which cannot be done AFAICT. A constructor is a requirement.> >> The use of BoundedInt actually simplifies register fields definitions, >> as they don't need an intermediate storage type (the "as ..." part of >> fields definitions). Instead, the internal storage type for each field >> is now the bounded integer of its width in bits, which can optionally be >> converted to another type that implements `From`` or `TryFrom`` for that >> bounded integer type. >> >> This means that something like >> >> register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 { >> 3:3 status_valid as bool, >> 31:8 addr as u32, >> }); >> >> Now becomes >> >> register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 { >> 3:3 status_valid => bool, >> 31:8 addr, >> }); > > That looks nicer, really. But now that you don't make user to provide > a representation type, how would one distinguish signed and unsigned > fields? Assuming that BoundedInt is intended to become a generic type, > people may want to use it as a storage for counters and other > non-bitfield type of things. Maybe: > > register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 { > s 3:0 cnt, > 7:4 flags, // implies unsigned - ? > u 31:8 addr, > });The expectation would be to use the `=>` syntax to convert the field to a signed type (similarly to how `status_valid` is turned into a `bool` in my example).> >> (here `status_valid` is infallibly converted to a bool for convenience >> and to remain compatible with the previous semantics) >> >> The field setter/getters are also simplified. If a field has no target >> type, then its setter expects any type that implements `Into` to the >> field's bounded integer type. Due to the many `From` implementations for >> primitive types, this means that most calls can be left unchanged. If >> the caller passes a value that is potentially larger than the field's >> capacity, it must use the `try_` variant of the setter, which returns an >> error if the value cannot be converted at runtime. >> >> For fields that use `=>` to convert to another type, both setter and >> getter are always infallible. >> >> For fields that use `?=>` to fallibly convert to another type, only the >> getter needs to be fallible as the setter always provide valid values by >> design. > > Can you share a couple examples? Not sure I understand this part, > especially how setters may not be fallible, and getters may fail.Imagine you have this enum: enum GpioState { Low = 0, High = 1, Floating = 2, } and this field: 2:0 gpio_state ?=> GpioState, When you set it, you must pass an instance of `GpioState` as argument, which means that the value will always be valid. However, when you try to access the field, you have no guarantee at all that the value of the field won't be `3` - the IO space might be inaccessible, or the register value be forged arbitrarily. Thus the getter needs to return a `Result<GpioState>`.> >> Outside of the register macro, the biggest changes occur in `falcon.rs`, >> which defines many enums for fields - their conversion implementations >> need to be changed from the original primitive type of the field to the >> new corresponding bounded int type. Hopefully the TryFrom/Into derive >> macros [1] can take care of implementing these, but it will need to be >> adapted to support bounded integers... :/ >> >> But overall, I am rather happy at how simple it was to convert the whole >> of nova-core to this. >> >> Note: This RFC uses nova-core's register!() macro for practical >> purposes, but the hope is to move this patch on top of the bitfield >> macro after it is split out [2]. >> >> [1] https://lore.kernel.org/rust-for-linux/cover.1755235180.git.y.j3ms.n at gmail.com/ >> [2] https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf at nvidia.com/ >> >> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> >> --- > > ... > >> regs::NV_PFALCON_FALCON_DMATRFBASE1::default() >> - .set_base((dma_start >> 40) as u16) >> + .try_set_base(dma_start >> 40)? >> .write(bar, &E::ID); > > Does it mean that something like the following syntax is possible? > > regs::NV_PFALCON_FALCON_DMATRFBASE1::default() > .try_set_base1(base1 >> 40)? // fail here > .try_set_base2(base2 >> 40)? // skip > .write(bar, &E::ID) else { pr_err!(); return -EINVAL }; > > This is my main concern: Rust is advertised a as runtime-safe language > (at lease safer than C), but current design isn't safe against one of > the most common errors: type overflow.Not sure I understand what you mean, but if you are talking about fields overflow, this cannot happen with the current design. The non-fallible setter can only be invoked if the compiler can prove that the argument does fit withing the field. Otherwise, one has to use the fallible setter (as this chunk does, because `dma_start >> 40` can still spill over the capacity of `base`), which performs a runtime check and returns `EOVERFLOW` if the value didn't fit.> > If your syntax above allows to handle errors in .try_set() path this way > or another, I think the rest is manageable. > > As a side note: it's a huge pain in C to grep for functions that > defined by using a macro. Here you do a similar thing. One can't > easily grep the 'try_set_base' implementation, and would have to > make a not so pleasant detour to the low-level internals. Maybe > switch it to: > > regs::NV_PFALCON_FALCON_DMATRFBASE1::default() > .try_set(base, dma_start >> 40)? > .write(bar, &E::ID);`base` here is passed by value, what type would it be? I don't think it is easily doable without jumping through many hoops. Using LSP with Rust actually makes it very easy to jump to either the definition of the register, or of the `try_set` block in the macro - I've done this many times. LSP is pretty much a requirement to code efficiently in Rust, so I think it is reasonable to rely on it here.
On Fri, Oct 10, 2025 at 06:19:17PM +0900, Alexandre Courbot wrote:> On Fri Oct 10, 2025 at 1:40 AM JST, Yury Norov wrote: > > Hi Alexandre, > > > > On Thu, Oct 09, 2025 at 09:37:10PM +0900, Alexandre Courbot wrote: > >> Use BoundedInt with the register!() macro and adapt the nova-core code > >> accordingly. This makes it impossible to trim values when setting a > >> register field, because either the value of the field has been inferred > >> at compile-time to fit within the bounds of the field, or the user has > >> been forced to check at runtime that it does indeed fit. > > > > In C23 we've got _BitInt(), which works like: > > > > unsigned _BitInt(2) a = 5; // compile-time error > > > > Can you consider a similar name and syntax in rust? > > I like the shorter `BitInt`! For the syntax, we will have to conform to > what is idiomatic Rust. And I don't think we can make something similar > to `= 5` work here - that would require overloading the `=` operator, > which cannot be done AFAICT. A constructor is a requirement.Sure, BitInt + constructor is nice.> >> The use of BoundedInt actually simplifies register fields definitions, > >> as they don't need an intermediate storage type (the "as ..." part of > >> fields definitions). Instead, the internal storage type for each field > >> is now the bounded integer of its width in bits, which can optionally be > >> converted to another type that implements `From`` or `TryFrom`` for that > >> bounded integer type. > >> > >> This means that something like > >> > >> register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 { > >> 3:3 status_valid as bool, > >> 31:8 addr as u32, > >> }); > >> > >> Now becomes > >> > >> register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 { > >> 3:3 status_valid => bool, > >> 31:8 addr, > >> }); > > > > That looks nicer, really. But now that you don't make user to provide > > a representation type, how would one distinguish signed and unsigned > > fields? Assuming that BoundedInt is intended to become a generic type, > > people may want to use it as a storage for counters and other > > non-bitfield type of things. Maybe: > > > > register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 { > > s 3:0 cnt, > > 7:4 flags, // implies unsigned - ? > > u 31:8 addr, > > });> The expectation would be to use the `=>` syntax to convert the field to > a signed type (similarly to how `status_valid` is turned into a `bool` > in my example).So, you suggest like this? register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 { 3:0 cnt => i8, 7:4 flags, // implied unsigned 31:8 addr, // implied unsigned }); That answers my question. Can you please highlight this use case in commit message? And just to wrap up: - all fields by default are unsigned integers; - one may use '=>' to switch to signed integers, enums or booleans; - all other types are not allowed. Is that correct?> >> (here `status_valid` is infallibly converted to a bool for convenience > >> and to remain compatible with the previous semantics) > >> > >> The field setter/getters are also simplified. If a field has no target > >> type, then its setter expects any type that implements `Into` to the > >> field's bounded integer type. Due to the many `From` implementations for > >> primitive types, this means that most calls can be left unchanged. If > >> the caller passes a value that is potentially larger than the field's > >> capacity, it must use the `try_` variant of the setter, which returns an > >> error if the value cannot be converted at runtime. > >> > >> For fields that use `=>` to convert to another type, both setter and > >> getter are always infallible. > >> > >> For fields that use `?=>` to fallibly convert to another type, only the > >> getter needs to be fallible as the setter always provide valid values by > >> design. > > > > Can you share a couple examples? Not sure I understand this part, > > especially how setters may not be fallible, and getters may fail. > > Imagine you have this enum: > > enum GpioState { > Low = 0, > High = 1, > Floating = 2, > } > > and this field: > > 2:0 gpio_state ?=> GpioState, > > When you set it, you must pass an instance of `GpioState` as argument, > which means that the value will always be valid. However, when you try > to access the field, you have no guarantee at all that the value of the > field won't be `3` - the IO space might be inaccessible, or the register > value be forged arbitrarily. Thus the getter needs to return a > `Result<GpioState>`.Ack, thanks.> >> Outside of the register macro, the biggest changes occur in `falcon.rs`, > >> which defines many enums for fields - their conversion implementations > >> need to be changed from the original primitive type of the field to the > >> new corresponding bounded int type. Hopefully the TryFrom/Into derive > >> macros [1] can take care of implementing these, but it will need to be > >> adapted to support bounded integers... :/ > >> > >> But overall, I am rather happy at how simple it was to convert the whole > >> of nova-core to this. > >> > >> Note: This RFC uses nova-core's register!() macro for practical > >> purposes, but the hope is to move this patch on top of the bitfield > >> macro after it is split out [2]. > >> > >> [1] https://lore.kernel.org/rust-for-linux/cover.1755235180.git.y.j3ms.n at gmail.com/ > >> [2] https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf at nvidia.com/ > >> > >> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> > >> --- > > > > ... > > > >> regs::NV_PFALCON_FALCON_DMATRFBASE1::default() > >> - .set_base((dma_start >> 40) as u16) > >> + .try_set_base(dma_start >> 40)? > >> .write(bar, &E::ID); > > > > Does it mean that something like the following syntax is possible? > > > > regs::NV_PFALCON_FALCON_DMATRFBASE1::default() > > .try_set_base1(base1 >> 40)? // fail here > > .try_set_base2(base2 >> 40)? // skip > > .write(bar, &E::ID) else { pr_err!(); return -EINVAL }; > > > > This is my main concern: Rust is advertised a as runtime-safe language > > (at lease safer than C), but current design isn't safe against one of > > the most common errors: type overflow. > > Not sure I understand what you mean, but if you are talking about fields > overflow, this cannot happen with the current design. The non-fallible > setter can only be invoked if the compiler can prove that the argument > does fit withing the field. Otherwise, one has to use the fallible > setter (as this chunk does, because `dma_start >> 40` can still spill > over the capacity of `base`), which performs a runtime check and returns > `EOVERFLOW` if the value didn't fit.Yeah, this design addresses my major question to the bitfields series from Joel: setters must be fallible. I played with this approach, and it does exactly what I have in mind. I still have a question regarding compile-time flavor of the setter. In C we've got a builtin_constant_p, and use it like: static inline int set_base(unsigned int base) { BUILD_BUG_ON_ZERO(const_true(base > MAX_BASE)); // Eliminated for compile-time 'base' if (base > MAX_BASE) return -EOVERFLOW; __set_base(base); return 0; } Can we do the same trick in rust? Would be nice to have a single setter for both compile and runtime cases.> > If your syntax above allows to handle errors in .try_set() path this way > > or another, I think the rest is manageable. > > > > As a side note: it's a huge pain in C to grep for functions that > > defined by using a macro. Here you do a similar thing. One can't > > easily grep the 'try_set_base' implementation, and would have to > > make a not so pleasant detour to the low-level internals. Maybe > > switch it to: > > > > regs::NV_PFALCON_FALCON_DMATRFBASE1::default() > > .try_set(base, dma_start >> 40)? > > .write(bar, &E::ID); > > `base` here is passed by value, what type would it be? I don't think it > is easily doable without jumping through many hoops. > > Using LSP with Rust actually makes it very easy to jump to either the > definition of the register, or of the `try_set` block in the macro - > I've done this many times. LSP is pretty much a requirement to code > efficiently in Rust, so I think it is reasonable to rely on it here.OK, then this one is also addressed. If LSP is a requirement, maybe it's worth to mention it in Documentation?
On Fri Oct 10, 2025 at 7:20 PM CEST, Yury Norov wrote:> On Fri, Oct 10, 2025 at 06:19:17PM +0900, Alexandre Courbot wrote: >> On Fri Oct 10, 2025 at 1:40 AM JST, Yury Norov wrote: > register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 { > 3:0 cnt => i8, > 7:4 flags, // implied unsigned > 31:8 addr, // implied unsigned > }); > > That answers my question. Can you please highlight this use case > in commit message? > > And just to wrap up: > > - all fields by default are unsigned integers; > - one may use '=>' to switch to signed integers, enums or booleans; > - all other types are not allowed.We do allow other types. In Rust we have the From [1] and TryFrom [2] traits (analogously Into and TryInto). This way, if some type T implements, for instance, From<u8> it means that we can always derive an instance of T from any u8. Similarly, if T implements TryFrom<u8> we can always derive a Result<T>, which either contains a valid instance of T or some error that we can handle instead. Hence, the following is valid is well: register!(NV_PFALCON_FALCON_HWCFG1 @ PFalconBase[0x0000012c] { 3:0 core_rev ?=> FalconCoreRev, "Core revision"; 5:4 security_model ?=> FalconSecurityModel, "Security model"; 7:6 core_rev_subversion as ?=> FalconCoreRevSubversion, "Core revision subversion"; }); The '?=>' notation means TryFrom, hence the generated accessor method - e.g. security_model() - returns a Result<FalconCoreRevSubversion>. In this exaple all three types are actually enums, but it would be the exact same for structs. In fact, enums in Rust are very different than enums in C anyways and can be complex types, such as: enum Message { Quit, Move { x: i32, y: i32 }, Write(String), ChangeColor(i32, i32, i32), } [1] https://rust.docs.kernel.org/core/convert/trait.From.html [2] https://rust.docs.kernel.org/core/convert/trait.TryFrom.html> I still have a question regarding compile-time flavor of the setter. > In C we've got a builtin_constant_p, and use it like: > > static inline int set_base(unsigned int base) > { > BUILD_BUG_ON_ZERO(const_true(base > MAX_BASE)); > > // Eliminated for compile-time 'base' > if (base > MAX_BASE) > return -EOVERFLOW; > > __set_base(base); > > return 0; > } > > Can we do the same trick in rust? Would be nice to have a single > setter for both compile and runtime cases.Technically, we could, but it would be very inefficient: Even if the function / method is called in an infallible way it would still return a Result<T> instead of just T, which the caller would need to handle. Analogously, for a setter the function / method would still return a Result, which must be handled. Ignoring a returned Result causes a compiler warning: warning: unused `core::result::Result` that must be used --> drivers/gpu/nova-core/driver.rs:64:9 | 64 | foo(); | ^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled = note: `#[warn(unused_must_use)]` on by default help: use `let _ = ...` to ignore the resulting value | 64 | let _ = foo(); | +++++++ warning: 1 warning emitted This is intentional, users should not be able to ignore error conditions willy-nilly: It is a potential source for bugs if errors are ignored. For instance, a caller might not check the returned Result initially because a compile time check is expected. However, when changed later on to derive the value at runtime it is easy to forget handling the Result instead.
Hi Yury, On 10/10/2025 1:20 PM, Yury Norov wrote: [...]>>>> regs::NV_PFALCON_FALCON_DMATRFBASE1::default() >>>> - .set_base((dma_start >> 40) as u16) >>>> + .try_set_base(dma_start >> 40)? >>>> .write(bar, &E::ID); >>> >>> Does it mean that something like the following syntax is possible? >>> >>> regs::NV_PFALCON_FALCON_DMATRFBASE1::default() >>> .try_set_base1(base1 >> 40)? // fail here >>> .try_set_base2(base2 >> 40)? // skip >>> .write(bar, &E::ID) else { pr_err!(); return -EINVAL }; >>> >>> This is my main concern: Rust is advertised a as runtime-safe language >>> (at lease safer than C), but current design isn't safe against one of >>> the most common errors: type overflow. >> >> Not sure I understand what you mean, but if you are talking about fields >> overflow, this cannot happen with the current design. The non-fallible >> setter can only be invoked if the compiler can prove that the argument >> does fit withing the field. Otherwise, one has to use the fallible >> setter (as this chunk does, because `dma_start >> 40` can still spill >> over the capacity of `base`), which performs a runtime check and returns >> `EOVERFLOW` if the value didn't fit. > > Yeah, this design addresses my major question to the bitfields series > from Joel: setters must be fallible. I played with this approach, and > it does exactly what I have in mind. > > I still have a question regarding compile-time flavor of the setter. > In C we've got a builtin_constant_p, and use it like: > > static inline int set_base(unsigned int base) > { > BUILD_BUG_ON_ZERO(const_true(base > MAX_BASE)); > > // Eliminated for compile-time 'base' > if (base > MAX_BASE) > return -EOVERFLOW; > > __set_base(base); > > return 0; > } > > Can we do the same trick in rust? Would be nice to have a single > setter for both compile and runtime cases.I don't think we could combine the setter and try setter variants on the rust side, because the former returns Self and the latter returns Result. Also, both the variants already have compile time asserts which may cover what you're referring to. The try setter variants in fact are not strictly needed, because the user can provide a bounded integer (after performing any fallible conversions on the caller side). Alex and me discussed adding that for a better user/caller experience [1]. [1] https://lore.kernel.org/all/C35B5306-98C6-447B-A239-9D6A6C548A4F at nvidia.com/ Or did you mean something else? thanks, - Joel