* [RFC PATCH v2 0/3] rust: bounded integer types and use in register macro
@ 2025-10-09 12:37 Alexandre Courbot
2025-10-09 12:37 ` [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation Alexandre Courbot
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Alexandre Courbot @ 2025-10-09 12:37 UTC (permalink / raw)
To: Danilo Krummrich, Joel Fernandes, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau, linux-kernel, rust-for-linux, Alexandre Courbot,
Edwin Peer
Second revision, much more complete, of the RFC/PoC for an idea I
submitted a bit earlier [1] regarding how to handle truncated bits in
the register/bitfield macro field setters.
Currently, the register macro allows to define fields that are shorter
than the primitive type used to set them. For instance, in the following
register:
register!(NV_PFALCON_FALCON_DMATRFBASE1 @ PFalconBase[0x00000128] {
8:0 base as u16;
});
The `base` field is 9 bits long, but is set using the following
setter method:
fn set_base(self, value: u16) -> Self
which discards bits of `value` higher than 9 without any error or
warning if some happen to be set. And so you happily start a DMA
transfer from the wrong base address...
While this behavior is not UB, this can also be a source of bugs.
Several ideas have been submitted to not let these unnoticed, including
making the register setters fallible, or panicking or printing a warning
whenever extra bits are discarded. These solutions are either too
risky (like panicking), or add extra code and checks on what can be a
critical path as registers are commonly accessed in interrupt handlers.
In pure Rust fashion, we would prefer to guarantee at compile-time,
whenever possible, that no bits are discarded when setting register
fields with a non-round number of bits.
This PoC proposes a possible solution for that. It introduces a new
`BoundedInt` type that wraps a primitive integer type and, using a const
generic parameter, limits the number of bits that can actually be used
within it. This is similar in spirit to `NonZero` or `Alignment` in that
it provides a static guarantee that the value it contains is limited to
a certain subset of the possible values of its primitive type.
Instances of `BoundedInt` can be constructed infallibly when the
compiler is able to guarantee that the passed value fits within the
allowed bounds, or fallibly using `try_new(primitive)`.
This type is then used by the register/bitfield macros to let the fields
getter and (more importantly) setter methods work with the exact number
of bits they can handle. For instance, the above method would become:
fn set_base(self, value: impl Into<BoundedInt<u32, 9>>) -> Self
which guarantees that no bits are ever discarded by the setter, since
the type of `value` carries an invariant that only the 9 lowest bits can
ever be set.
It is then the responsibility of the caller to build the adequate
`BoundedInt`, which very often can be done infallibly, but all the cases
that require a fallible operation are cases that the caller should have
checked anyway (lest you beam out the wrong memory region on your DMA
transfer!).
Compared to v1, this version of the RFC is much more complete. It
notably provides many `From` implementations for conversions from/to
bounded types that cannot fail, which removes a lot of the friction one
would expect when introducing new integer types.
Another side effect of this is that the bitfield definitions are
considerably simplified, since their type can now be automatically
inferred. This means that
8:0 base as u16;
could become simply
8:0 base;
And the getter/setters would work with a `BoundedInt<u32, 9>` (provided
the bitfield type is backed by a `u32`).
For convenience, this PoC is based on drm-rust-next. If we decide to
proceed with it, we would do it after the patchset extracting and moving
the bitfield logic [2] lands, as the two would conflict heavily.
Patch 1 is a pre-requisite that should be merged to nova-core, but can
be considered external to this series.
Patch 2 is the `BoundedInt` implementation.
Patch 3 makes nova-core use `BoundedInt` for its register macro, and
serves to illustrate how callers of the register field accessors are
affected by the change (rather minimally imho).
Feedback is welcome! If there is no pushback I will remove the RFC tag
for the next revision as I think the `BoundedInt` implementation is
already rather exhaustive.
[1] https://lore.kernel.org/rust-for-linux/DD5D59FH4JTT.2G5WEXF3RBCQJ@nvidia.com/
[2] https://lore.kernel.org/rust-for-linux/DD68A3TZD9CV.2CL7R7K4UAICU@kernel.org/T/
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Changes in v2:
- Thorough implementation of `BoundedInt`.
- nova-core fully adapted to use `BoundedInt`.
- Link to v1: https://lore.kernel.org/r/20251002-bounded_ints-v1-0-dd60f5804ea4@nvidia.com
---
Alexandre Courbot (3):
gpu: nova-core: register: use field type for Into implementation
rust: kernel: add bounded integer types
gpu: nova-core: use BoundedInt
drivers/gpu/nova-core/falcon.rs | 118 ++++---
drivers/gpu/nova-core/falcon/hal/ga102.rs | 5 +-
drivers/gpu/nova-core/fb/hal/ga100.rs | 3 +-
drivers/gpu/nova-core/gpu.rs | 9 +-
drivers/gpu/nova-core/regs.rs | 139 ++++-----
drivers/gpu/nova-core/regs/macros.rs | 264 ++++++++--------
rust/kernel/lib.rs | 1 +
rust/kernel/num.rs | 499 ++++++++++++++++++++++++++++++
8 files changed, 783 insertions(+), 255 deletions(-)
---
base-commit: 299eb32863e584cfff7c6b667c3e92ae7d4d2bf9
change-id: 20251001-bounded_ints-1d0457d9ae26
Best regards,
--
Alexandre Courbot <acourbot@nvidia.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation
2025-10-09 12:37 [RFC PATCH v2 0/3] rust: bounded integer types and use in register macro Alexandre Courbot
@ 2025-10-09 12:37 ` Alexandre Courbot
2025-10-09 15:17 ` Joel Fernandes
` (2 more replies)
2025-10-09 12:37 ` [PATCH RFC v2 2/3] rust: kernel: add bounded integer types Alexandre Courbot
2025-10-09 12:37 ` [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt Alexandre Courbot
2 siblings, 3 replies; 26+ messages in thread
From: Alexandre Courbot @ 2025-10-09 12:37 UTC (permalink / raw)
To: Danilo Krummrich, Joel Fernandes, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau, linux-kernel, rust-for-linux, Alexandre Courbot,
Edwin Peer
The getter method of a field works with the field type, but its setter
expects the type of the register. This leads to an asymmetry in the
From/Into implementations required for a field with a dedicated type.
For instance, a field declared as
pub struct ControlReg(u32) {
3:0 mode as u8 ?=> Mode;
...
}
currently requires the following implementations:
impl TryFrom<u8> for Mode {
...
}
impl From<Mode> for u32 {
...
}
Change this so the `From<Mode>` now needs to be implemented for `u8`,
i.e. the primitive type of the field. This is more consistent, and will
become a requirement once we start using the TryFrom/Into derive macros
to implement these automatically.
Reported-by: Edwin Peer <epeer@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/gpu/nova-core/falcon.rs | 38 +++++++++++++++++++++++++-----------
drivers/gpu/nova-core/regs/macros.rs | 10 +++++-----
2 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 37e6298195e4..3f505b870601 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -22,11 +22,11 @@
pub(crate) mod sec2;
// TODO[FPRI]: Replace with `ToPrimitive`.
-macro_rules! impl_from_enum_to_u32 {
+macro_rules! impl_from_enum_to_u8 {
($enum_type:ty) => {
- impl From<$enum_type> for u32 {
+ impl From<$enum_type> for u8 {
fn from(value: $enum_type) -> Self {
- value as u32
+ value as u8
}
}
};
@@ -46,7 +46,7 @@ pub(crate) enum FalconCoreRev {
Rev6 = 6,
Rev7 = 7,
}
-impl_from_enum_to_u32!(FalconCoreRev);
+impl_from_enum_to_u8!(FalconCoreRev);
// TODO[FPRI]: replace with `FromPrimitive`.
impl TryFrom<u8> for FalconCoreRev {
@@ -81,7 +81,7 @@ pub(crate) enum FalconCoreRevSubversion {
Subversion2 = 2,
Subversion3 = 3,
}
-impl_from_enum_to_u32!(FalconCoreRevSubversion);
+impl_from_enum_to_u8!(FalconCoreRevSubversion);
// TODO[FPRI]: replace with `FromPrimitive`.
impl TryFrom<u8> for FalconCoreRevSubversion {
@@ -125,7 +125,7 @@ pub(crate) enum FalconSecurityModel {
/// Also known as High-Secure, Privilege Level 3 or PL3.
Heavy = 3,
}
-impl_from_enum_to_u32!(FalconSecurityModel);
+impl_from_enum_to_u8!(FalconSecurityModel);
// TODO[FPRI]: replace with `FromPrimitive`.
impl TryFrom<u8> for FalconSecurityModel {
@@ -157,7 +157,7 @@ pub(crate) enum FalconModSelAlgo {
#[default]
Rsa3k = 1,
}
-impl_from_enum_to_u32!(FalconModSelAlgo);
+impl_from_enum_to_u8!(FalconModSelAlgo);
// TODO[FPRI]: replace with `FromPrimitive`.
impl TryFrom<u8> for FalconModSelAlgo {
@@ -179,7 +179,7 @@ pub(crate) enum DmaTrfCmdSize {
#[default]
Size256B = 0x6,
}
-impl_from_enum_to_u32!(DmaTrfCmdSize);
+impl_from_enum_to_u8!(DmaTrfCmdSize);
// TODO[FPRI]: replace with `FromPrimitive`.
impl TryFrom<u8> for DmaTrfCmdSize {
@@ -202,7 +202,6 @@ pub(crate) enum PeregrineCoreSelect {
/// RISC-V core is active.
Riscv = 1,
}
-impl_from_enum_to_u32!(PeregrineCoreSelect);
impl From<bool> for PeregrineCoreSelect {
fn from(value: bool) -> Self {
@@ -213,6 +212,15 @@ fn from(value: bool) -> Self {
}
}
+impl From<PeregrineCoreSelect> for bool {
+ fn from(value: PeregrineCoreSelect) -> Self {
+ match value {
+ PeregrineCoreSelect::Falcon => false,
+ PeregrineCoreSelect::Riscv => true,
+ }
+ }
+}
+
/// Different types of memory present in a falcon core.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub(crate) enum FalconMem {
@@ -236,7 +244,7 @@ pub(crate) enum FalconFbifTarget {
/// Non-coherent system memory (System DRAM).
NoncoherentSysmem = 2,
}
-impl_from_enum_to_u32!(FalconFbifTarget);
+impl_from_enum_to_u8!(FalconFbifTarget);
// TODO[FPRI]: replace with `FromPrimitive`.
impl TryFrom<u8> for FalconFbifTarget {
@@ -263,7 +271,6 @@ pub(crate) enum FalconFbifMemType {
/// Physical memory addresses.
Physical = 1,
}
-impl_from_enum_to_u32!(FalconFbifMemType);
/// Conversion from a single-bit register field.
impl From<bool> for FalconFbifMemType {
@@ -275,6 +282,15 @@ fn from(value: bool) -> Self {
}
}
+impl From<FalconFbifMemType> for bool {
+ fn from(value: FalconFbifMemType) -> Self {
+ match value {
+ FalconFbifMemType::Virtual => false,
+ FalconFbifMemType::Physical => true,
+ }
+ }
+}
+
/// Type used to represent the `PFALCON` registers address base for a given falcon engine.
pub(crate) struct PFalconBase(());
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 754c14ee7f40..73811a115762 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -482,7 +482,7 @@ impl $name {
register!(
@leaf_accessor $name $hi:$lo $field
{ |f| <$into_type>::from(if f != 0 { true } else { false }) }
- $into_type => $into_type $(, $comment)?;
+ bool $into_type => $into_type $(, $comment)?;
);
};
@@ -499,7 +499,7 @@ impl $name {
$(, $comment:literal)?;
) => {
register!(@leaf_accessor $name $hi:$lo $field
- { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
+ { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
::core::result::Result<
$try_into_type,
<$try_into_type as ::core::convert::TryFrom<$type>>::Error
@@ -513,7 +513,7 @@ impl $name {
$(, $comment:literal)?;
) => {
register!(@leaf_accessor $name $hi:$lo $field
- { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
+ { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
};
// Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
@@ -527,7 +527,7 @@ impl $name {
// Generates the accessor methods for a single field.
(
@leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
- { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
+ { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
) => {
::kernel::macros::paste!(
const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
@@ -559,7 +559,7 @@ pub(crate) fn $field(self) -> $res_type {
pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
const MASK: u32 = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
- let value = (u32::from(value) << SHIFT) & MASK;
+ let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
self.0 = (self.0 & !MASK) | value;
self
--
2.51.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH RFC v2 2/3] rust: kernel: add bounded integer types
2025-10-09 12:37 [RFC PATCH v2 0/3] rust: bounded integer types and use in register macro Alexandre Courbot
2025-10-09 12:37 ` [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation Alexandre Courbot
@ 2025-10-09 12:37 ` Alexandre Courbot
2025-10-09 21:33 ` Joel Fernandes
2025-10-11 10:33 ` Alice Ryhl
2025-10-09 12:37 ` [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt Alexandre Courbot
2 siblings, 2 replies; 26+ messages in thread
From: Alexandre Courbot @ 2025-10-09 12:37 UTC (permalink / raw)
To: Danilo Krummrich, Joel Fernandes, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau, linux-kernel, rust-for-linux, Alexandre Courbot
Add the BoundedInt type, which restricts the number of bits allowed to
be used in a given integer value. This is useful to carry guarantees
when setting bitfields.
Alongside this type, many `From` and `TryFrom` implementations are
provided to reduce friction when using with regular integer types. Proxy
implementations of common integer traits are also provided.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
rust/kernel/lib.rs | 1 +
rust/kernel/num.rs | 499 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 500 insertions(+)
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index fcffc3988a90..21c1f452ee6a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -101,6 +101,7 @@
pub mod mm;
#[cfg(CONFIG_NET)]
pub mod net;
+pub mod num;
pub mod of;
#[cfg(CONFIG_PM_OPP)]
pub mod opp;
diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
new file mode 100644
index 000000000000..b2aad95ce51c
--- /dev/null
+++ b/rust/kernel/num.rs
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Numerical types for the kernel.
+
+use kernel::prelude::*;
+
+/// Integer type for which only the bits `0..NUM_BITS` are valid.
+///
+/// # Invariants
+///
+/// Stored values are represented with at most `NUM_BITS` bits.
+#[repr(transparent)]
+#[derive(Clone, Copy, Debug, Default, Hash)]
+pub struct BoundedInt<T, const NUM_BITS: u32>(T);
+
+/// Returns `true` if `$value` can be represented with at most `$NUM_BITS` on `$type`.
+macro_rules! is_in_bounds {
+ ($value:expr, $type:ty, $num_bits:expr) => {{
+ let v = $value;
+ v & <$type as Boundable<NUM_BITS>>::MASK == v
+ }};
+}
+
+/// Trait for primitive integer types that can be used with `BoundedInt`.
+pub trait Boundable<const NUM_BITS: u32>
+where
+ Self: Sized + Copy + core::ops::BitAnd<Output = Self> + core::cmp::PartialEq,
+ Self: TryInto<u8> + TryInto<u16> + TryInto<u32> + TryInto<u64>,
+{
+ /// Mask of the valid bits for this type.
+ const MASK: Self;
+
+ /// Returns `true` if `value` can be represented with at most `NUM_BITS`.
+ ///
+ /// TODO: post-RFC: replace this with a left-shift followed by right-shift operation. This will
+ /// allow us to handle signed values as well.
+ fn is_in_bounds(value: Self) -> bool {
+ is_in_bounds!(value, Self, NUM_BITS)
+ }
+}
+
+impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u8 {
+ const MASK: u8 = crate::bits::genmask_u8(0..=(NUM_BITS - 1));
+}
+
+impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u16 {
+ const MASK: u16 = crate::bits::genmask_u16(0..=(NUM_BITS - 1));
+}
+
+impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u32 {
+ const MASK: u32 = crate::bits::genmask_u32(0..=(NUM_BITS - 1));
+}
+
+impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u64 {
+ const MASK: u64 = crate::bits::genmask_u64(0..=(NUM_BITS - 1));
+}
+
+impl<T, const NUM_BITS: u32> BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+{
+ /// Checks that `value` is valid for this type at compile-time and build a new value.
+ ///
+ /// This relies on [`build_assert!`] to perform validation at compile-time. If `value` cannot
+ /// be inferred to be in bounds at compile-time, use the fallible [`Self::try_new`] instead.
+ ///
+ /// When possible, use one of the `new_const` methods instead of this method as it statically
+ /// validates `value` instead of relying on the compiler's optimizations.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::num::BoundedInt;
+ ///
+ /// # fn some_number() -> u32 { 0xffffffff }
+ ///
+ /// assert_eq!(BoundedInt::<u8, 1>::new(1).get(), 1);
+ /// assert_eq!(BoundedInt::<u16, 8>::new(0xff).get(), 0xff);
+ ///
+ /// // Triggers a build error as `0x1ff` doesn't fit into 8 bits.
+ /// // assert_eq!(BoundedInt::<u32, 8>::new(0x1ff).get(), 0x1ff);
+ ///
+ /// let v: u32 = some_number();
+ /// // Triggers a build error as `v` cannot be asserted to fit within 4 bits...
+ /// // let _ = BoundedInt::<u32, 4>::new(v);
+ /// // ... but this works as the compiler can assert the range from the mask.
+ /// let _ = BoundedInt::<u32, 4>::new(v & 0xf);
+ /// ```
+ pub fn new(value: T) -> Self {
+ crate::build_assert!(
+ T::is_in_bounds(value),
+ "Provided parameter is larger than maximal supported value"
+ );
+
+ Self(value)
+ }
+
+ /// Attempts to convert `value` into a value bounded by `NUM_BITS`.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::num::BoundedInt;
+ ///
+ /// assert_eq!(BoundedInt::<u8, 1>::try_new(1).map(|v| v.get()), Ok(1));
+ /// assert_eq!(BoundedInt::<u16, 8>::try_new(0xff).map(|v| v.get()), Ok(0xff));
+ ///
+ /// // `0x1ff` doesn't fit into 8 bits.
+ /// assert_eq!(BoundedInt::<u32, 8>::try_new(0x1ff), Err(EOVERFLOW));
+ /// ```
+ pub fn try_new(value: T) -> Result<Self> {
+ if !T::is_in_bounds(value) {
+ Err(EOVERFLOW)
+ } else {
+ Ok(Self(value))
+ }
+ }
+
+ /// Returns the contained value as a primitive type.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::num::BoundedInt;
+ ///
+ /// let v = BoundedInt::<u32, 4>::new_const::<7>();
+ /// assert_eq!(v.get(), 7u32);
+ /// ```
+ pub fn get(self) -> T {
+ if !T::is_in_bounds(self.0) {
+ // SAFETY: Per the invariants, `self.0` cannot have bits set outside of `MASK`, so
+ // this block will
+ // never be reached.
+ unsafe { core::hint::unreachable_unchecked() }
+ }
+ self.0
+ }
+
+ /// Increase the number of bits usable for `self`.
+ ///
+ /// This operation cannot fail.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::num::BoundedInt;
+ ///
+ /// let v = BoundedInt::<u32, 4>::new_const::<7>();
+ /// let larger_v = v.enlarge::<12>();
+ /// // The contained values are equal even though `larger_v` has a bigger capacity.
+ /// assert_eq!(larger_v, v);
+ /// ```
+ pub const fn enlarge<const NEW_NUM_BITS: u32>(self) -> BoundedInt<T, NEW_NUM_BITS>
+ where
+ T: Boundable<NEW_NUM_BITS>,
+ T: Copy,
+ {
+ build_assert!(NEW_NUM_BITS >= NUM_BITS);
+
+ // INVARIANT: the value did fit within `NUM_BITS`, so it will all the more fit within
+ // `NEW_NUM_BITS` which is larger.
+ BoundedInt(self.0)
+ }
+
+ /// Shrink the number of bits usable for `self`.
+ ///
+ /// Returns `EOVERFLOW` if the value of `self` cannot be represented within `NEW_NUM_BITS`.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::num::BoundedInt;
+ ///
+ /// let v = BoundedInt::<u32, 12>::new_const::<7>();
+ /// let smaller_v = v.shrink::<4>()?;
+ /// // The contained values are equal even though `smaller_v` has a smaller capacity.
+ /// assert_eq!(smaller_v, v);
+ ///
+ /// # Ok::<(), Error>(())
+ /// ```
+ pub fn shrink<const NEW_NUM_BITS: u32>(self) -> Result<BoundedInt<T, NEW_NUM_BITS>>
+ where
+ T: Boundable<NEW_NUM_BITS>,
+ T: Copy,
+ {
+ BoundedInt::<T, NEW_NUM_BITS>::try_new(self.get())
+ }
+
+ /// Casts `self` into a `BoundedInt` using a different storage type, but using the same
+ /// number of bits for representation.
+ ///
+ /// This method cannot fail as the number of bits used for representation doesn't change.
+ ///
+ /// # Examples
+ ///
+ /// ```
+ /// use kernel::num::BoundedInt;
+ ///
+ /// let v = BoundedInt::<u32, 4>::new_const::<7>();
+ /// let smaller_v: BoundedInt<u8, _> = v.cast();
+ /// // The contained values are equal even though `smaller_v` has a smaller storage type.
+ /// assert_eq!(u32::from(smaller_v.get()), v.get());
+ /// ```
+ pub fn cast<U>(self) -> BoundedInt<U, NUM_BITS>
+ where
+ U: TryFrom<T> + Boundable<NUM_BITS>,
+ {
+ // SAFETY: the contained value is represented using `NUM_BITS`, and `U` can be bounded to
+ // `NUM_BITS`, hence the conversion cannot fail.
+ let value = unsafe { U::try_from(self.0).unwrap_unchecked() };
+
+ // INVARIANT: although the storage type has changed, the value is still represented within
+ // `NUM_BITS`.
+ BoundedInt(value)
+ }
+}
+
+/// Validating the value as a const expression cannot be done as a regular method, as the
+/// arithmetic expressions we rely on to check the bounds are not const. Thus, implement
+/// `new_const` using a macro.
+macro_rules! impl_const_new {
+ ($($type:ty)*) => {
+ $(
+ impl<const NUM_BITS: u32> BoundedInt<$type, NUM_BITS> {
+ /// Creates a bounded value for `VALUE`, statically validated.
+ ///
+ /// This method should be used instead of [`Self::new`] when the value is a constant
+ /// expression.
+ ///
+ /// # Examples
+ /// ```
+ /// use kernel::num::BoundedInt;
+ ///
+ #[doc = ::core::concat!(
+ "let v = BoundedInt::<",
+ ::core::stringify!($type),
+ ", 4>::new_const::<7>();")]
+ /// assert_eq!(v.get(), 7);
+ /// ```
+ pub const fn new_const<const VALUE: $type>() -> Self {
+ build_assert!(is_in_bounds!(VALUE, $type, NUM_BITS));
+
+ Self(VALUE)
+ }
+ }
+ )*
+ };
+}
+
+impl_const_new!(u8 u16 u32 u64);
+
+/// Declares a new `$trait` and implements it for all bounded types represented using `$num_bits`.
+///
+/// This is used to declare properties as traits that we can use for later implementations.
+macro_rules! impl_size_rule {
+ ($trait:ident, $($num_bits:literal)*) => {
+ trait $trait {}
+
+ $(
+ impl<T> $trait for BoundedInt<T, $num_bits> where T: Boundable<$num_bits> {}
+ )*
+ };
+}
+
+// Bounds that are larger than a `u64`.
+impl_size_rule!(LargerThanU64, 64);
+
+// Bounds that are larger than a `u32`.
+impl_size_rule!(LargerThanU32,
+ 32 33 34 35 36 37 38 39
+ 40 41 42 43 44 45 46 47
+ 48 49 50 51 52 53 54 55
+ 56 57 58 59 60 61 62 63
+);
+// Anything larger than `u64` is also larger than `u32`.
+impl<T> LargerThanU32 for T where T: LargerThanU64 {}
+
+// Bounds that are larger than a `u16`.
+impl_size_rule!(LargerThanU16,
+ 16 17 18 19 20 21 22 23
+ 24 25 26 27 28 29 30 31
+);
+// Anything larger than `u32` is also larger than `u16`.
+impl<T> LargerThanU16 for T where T: LargerThanU32 {}
+
+// Bounds that are larger than a `u8`.
+impl_size_rule!(LargerThanU8, 8 9 10 11 12 13 14 15);
+// Anything larger than `u16` is also larger than `u8`.
+impl<T> LargerThanU8 for T where T: LargerThanU16 {}
+
+// Bounds that are larger than a boolean.
+impl_size_rule!(LargerThanBool, 1 2 3 4 5 6 7);
+// Anything larger than `u8` is also larger than `bool`.
+impl<T> LargerThanBool for T where T: LargerThanU8 {}
+
+/// Generates `From` implementations from a primitive type into a bounded integer that is
+/// guaranteed to being able to contain it.
+macro_rules! impl_from_primitive {
+ ($($type:ty => $trait:ident),*) => {
+ $(
+ impl<T, const NUM_BITS: u32> From<$type> for BoundedInt<T, NUM_BITS>
+ where
+ Self: $trait,
+ T: From<$type>,
+ {
+ fn from(value: $type) -> Self {
+ Self(T::from(value))
+ }
+ }
+ )*
+ }
+}
+
+impl_from_primitive!(
+ bool => LargerThanBool,
+ u8 => LargerThanU8,
+ u16 => LargerThanU16,
+ u32 => LargerThanU32,
+ u64 => LargerThanU64
+);
+
+impl_size_rule!(FitsIntoBool, 1);
+
+impl_size_rule!(FitsIntoU8, 2 3 4 5 6 7 8);
+
+// Anything that fits into a `bool` also fits into a `u8`.
+impl<T> FitsIntoU8 for T where T: FitsIntoBool {}
+
+impl_size_rule!(FitsIntoU16, 9 10 11 12 13 14 15 16);
+
+// Anything that fits into a `u8` also fits into a `u16`.
+impl<T> FitsIntoU16 for T where T: FitsIntoU8 {}
+
+impl_size_rule!(FitsIntoU32,
+ 17 18 19 20 21 22 23 24
+ 25 26 27 28 29 30 31 32
+);
+
+// Anything that fits into a `u16` also fits into a `u32`.
+impl<T> FitsIntoU32 for T where T: FitsIntoU16 {}
+
+impl_size_rule!(FitsIntoU64,
+ 33 34 35 36 37 38 39 40
+ 41 42 43 44 45 46 47 48
+ 49 50 51 52 53 54 55 56
+ 57 58 59 60 61 62 63 64
+);
+
+// Anything that fits into a `u32` also fits into a `u64`.
+impl<T> FitsIntoU64 for T where T: FitsIntoU32 {}
+
+/// Generates `From` implementations from a bounded integer into a primitive type that is
+/// guaranteed to being able to contain it.
+macro_rules! impl_into_primitive {
+ ($($trait:ident => $type:ty),*) => {
+ $(
+ impl<T, const NUM_BITS: u32> From<BoundedInt<T, NUM_BITS>> for $type
+ where
+ T: Boundable<NUM_BITS>,
+ BoundedInt<T, NUM_BITS>: $trait
+ {
+ fn from(value: BoundedInt<T, NUM_BITS>) -> Self {
+ // SAFETY: per the `BoundedInt` invariants, less than 8 bits are used to the conversion
+ // cannot fail.
+ unsafe { value.get().try_into().unwrap_unchecked() }
+ }
+ }
+ )*
+ }
+}
+
+impl_into_primitive!(
+ FitsIntoU8 => u8,
+ FitsIntoU16 => u16,
+ FitsIntoU32 => u32,
+ FitsIntoU64 => u64
+);
+
+// Conversion to boolean must be handled separately as it does not have `TryFrom` implementation
+// from integers.
+impl<T> From<BoundedInt<T, 1>> for bool
+where
+ T: Boundable<1>,
+ BoundedInt<T, 1>: FitsIntoBool,
+ T: PartialEq + Zeroable,
+{
+ fn from(value: BoundedInt<T, 1>) -> Self {
+ value.get() != Zeroable::zeroed()
+ }
+}
+
+/// Trait similar to `TryInto` to avoid conflicting implementations errors.
+pub trait TryIntoBounded<T: Boundable<NUM_BITS>, const NUM_BITS: u32> {
+ /// Attempts to convert `self` into a value bounded by `NUM_BITS`.
+ fn try_into(self) -> Result<BoundedInt<T, NUM_BITS>>;
+}
+
+/// Any value can be attempted to be converted into a bounded integer of any size.
+impl<T, U, const NUM_BITS: u32> TryIntoBounded<T, NUM_BITS> for U
+where
+ T: Boundable<NUM_BITS>,
+ U: TryInto<T>,
+{
+ fn try_into(self) -> Result<BoundedInt<T, NUM_BITS>> {
+ self.try_into()
+ .map_err(|_| EOVERFLOW)
+ .and_then(BoundedInt::try_new)
+ }
+}
+
+/// `BoundedInts` can be compared if their respective storage types can be.
+impl<T, U, const NUM_BITS: u32, const NUM_BITS_U: u32> PartialEq<BoundedInt<U, NUM_BITS_U>>
+ for BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+ U: Boundable<NUM_BITS_U>,
+ T: PartialEq<U>,
+{
+ fn eq(&self, other: &BoundedInt<U, NUM_BITS_U>) -> bool {
+ self.get() == other.get()
+ }
+}
+
+impl<T, const NUM_BITS: u32> Eq for BoundedInt<T, NUM_BITS> where T: Boundable<NUM_BITS> {}
+
+/// `BoundedInts` can be ordered if their respective storage types can be.
+impl<T, U, const NUM_BITS: u32, const NUM_BITS_U: u32> PartialOrd<BoundedInt<U, NUM_BITS_U>>
+ for BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+ U: Boundable<NUM_BITS_U>,
+ T: PartialOrd<U>,
+{
+ fn partial_cmp(&self, other: &BoundedInt<U, NUM_BITS_U>) -> Option<core::cmp::Ordering> {
+ self.get().partial_cmp(&other.get())
+ }
+}
+
+impl<T, const NUM_BITS: u32> Ord for BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+ T: Ord,
+{
+ fn cmp(&self, other: &Self) -> core::cmp::Ordering {
+ self.get().cmp(&other.get())
+ }
+}
+
+/// Allow comparison with non-bounded values.
+impl<T, const NUM_BITS: u32> PartialEq<T> for BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+ T: PartialEq,
+{
+ fn eq(&self, other: &T) -> bool {
+ self.get() == *other
+ }
+}
+
+/// Allow ordering with non-bounded values.
+impl<T, const NUM_BITS: u32> PartialOrd<T> for BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+ T: PartialOrd,
+{
+ fn partial_cmp(&self, other: &T) -> Option<core::cmp::Ordering> {
+ self.get().partial_cmp(other)
+ }
+}
+
+impl<T, const NUM_BITS: u32> core::fmt::Display for BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+ T: core::fmt::Display,
+{
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ self.0.fmt(f)
+ }
+}
+
+impl<T, const NUM_BITS: u32> core::fmt::LowerHex for BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+ T: core::fmt::LowerHex,
+{
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ self.0.fmt(f)
+ }
+}
+
+impl<T, const NUM_BITS: u32> core::fmt::UpperHex for BoundedInt<T, NUM_BITS>
+where
+ T: Boundable<NUM_BITS>,
+ T: core::fmt::UpperHex,
+{
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ self.0.fmt(f)
+ }
+}
--
2.51.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 12:37 [RFC PATCH v2 0/3] rust: bounded integer types and use in register macro Alexandre Courbot
2025-10-09 12:37 ` [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation Alexandre Courbot
2025-10-09 12:37 ` [PATCH RFC v2 2/3] rust: kernel: add bounded integer types Alexandre Courbot
@ 2025-10-09 12:37 ` Alexandre Courbot
2025-10-09 16:40 ` Yury Norov
2 siblings, 1 reply; 26+ messages in thread
From: Alexandre Courbot @ 2025-10-09 12:37 UTC (permalink / raw)
To: Danilo Krummrich, Joel Fernandes, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau, linux-kernel, rust-for-linux, Alexandre Courbot
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.
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,
});
(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.
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@gmail.com/
[2] https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
drivers/gpu/nova-core/falcon.rs | 134 ++++++++-------
drivers/gpu/nova-core/falcon/hal/ga102.rs | 5 +-
drivers/gpu/nova-core/fb/hal/ga100.rs | 3 +-
drivers/gpu/nova-core/gpu.rs | 9 +-
drivers/gpu/nova-core/regs.rs | 139 ++++++++--------
drivers/gpu/nova-core/regs/macros.rs | 264 +++++++++++++++---------------
6 files changed, 283 insertions(+), 271 deletions(-)
diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index 3f505b870601..71cb09cf7d67 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -6,6 +6,7 @@
use hal::FalconHal;
use kernel::device;
use kernel::dma::DmaAddress;
+use kernel::num::{Boundable, BoundedInt};
use kernel::prelude::*;
use kernel::sync::aref::ARef;
use kernel::time::Delta;
@@ -22,11 +23,14 @@
pub(crate) mod sec2;
// TODO[FPRI]: Replace with `ToPrimitive`.
-macro_rules! impl_from_enum_to_u8 {
- ($enum_type:ty) => {
- impl From<$enum_type> for u8 {
+macro_rules! impl_from_enum_to_bounded {
+ ($enum_type:ty, $length:literal) => {
+ impl<T> From<$enum_type> for BoundedInt<T, $length>
+ where
+ T: From<u8> + Boundable<$length>,
+ {
fn from(value: $enum_type) -> Self {
- value as u8
+ BoundedInt::new(T::from(value as u8))
}
}
};
@@ -46,16 +50,19 @@ pub(crate) enum FalconCoreRev {
Rev6 = 6,
Rev7 = 7,
}
-impl_from_enum_to_u8!(FalconCoreRev);
+impl_from_enum_to_bounded!(FalconCoreRev, 4);
// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconCoreRev {
+impl<T> TryFrom<BoundedInt<T, 4>> for FalconCoreRev
+where
+ T: Boundable<4>,
+{
type Error = Error;
- fn try_from(value: u8) -> Result<Self> {
+ fn try_from(value: BoundedInt<T, 4>) -> Result<Self> {
use FalconCoreRev::*;
- let rev = match value {
+ let rev = match u8::from(value) {
1 => Rev1,
2 => Rev2,
3 => Rev3,
@@ -81,24 +88,25 @@ pub(crate) enum FalconCoreRevSubversion {
Subversion2 = 2,
Subversion3 = 3,
}
-impl_from_enum_to_u8!(FalconCoreRevSubversion);
+impl_from_enum_to_bounded!(FalconCoreRevSubversion, 2);
// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconCoreRevSubversion {
- type Error = Error;
-
- fn try_from(value: u8) -> Result<Self> {
+impl<T> From<BoundedInt<T, 2>> for FalconCoreRevSubversion
+where
+ T: Boundable<2>,
+{
+ fn from(value: BoundedInt<T, 2>) -> Self {
use FalconCoreRevSubversion::*;
- let sub_version = match value & 0b11 {
+ match u8::from(value) {
0 => Subversion0,
1 => Subversion1,
2 => Subversion2,
3 => Subversion3,
- _ => return Err(EINVAL),
- };
-
- Ok(sub_version)
+ // TODO: somehow the compiler cannot infer that `value` cannot be > 3. Find a way to
+ // handle this gracefully, or switch back to fallible ops.
+ _ => panic!(),
+ }
}
}
@@ -125,16 +133,19 @@ pub(crate) enum FalconSecurityModel {
/// Also known as High-Secure, Privilege Level 3 or PL3.
Heavy = 3,
}
-impl_from_enum_to_u8!(FalconSecurityModel);
+impl_from_enum_to_bounded!(FalconSecurityModel, 2);
// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconSecurityModel {
+impl<T> TryFrom<BoundedInt<T, 2>> for FalconSecurityModel
+where
+ T: Boundable<2>,
+{
type Error = Error;
- fn try_from(value: u8) -> Result<Self> {
+ fn try_from(value: BoundedInt<T, 2>) -> Result<Self> {
use FalconSecurityModel::*;
- let sec_model = match value {
+ let sec_model = match u8::from(value) {
0 => None,
2 => Light,
3 => Heavy,
@@ -157,14 +168,17 @@ pub(crate) enum FalconModSelAlgo {
#[default]
Rsa3k = 1,
}
-impl_from_enum_to_u8!(FalconModSelAlgo);
+impl_from_enum_to_bounded!(FalconModSelAlgo, 8);
// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconModSelAlgo {
+impl<T> TryFrom<BoundedInt<T, 8>> for FalconModSelAlgo
+where
+ T: Boundable<8>,
+{
type Error = Error;
- fn try_from(value: u8) -> Result<Self> {
- match value {
+ fn try_from(value: BoundedInt<T, 8>) -> Result<Self> {
+ match u8::from(value) {
1 => Ok(FalconModSelAlgo::Rsa3k),
_ => Err(EINVAL),
}
@@ -179,14 +193,17 @@ pub(crate) enum DmaTrfCmdSize {
#[default]
Size256B = 0x6,
}
-impl_from_enum_to_u8!(DmaTrfCmdSize);
+impl_from_enum_to_bounded!(DmaTrfCmdSize, 3);
// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for DmaTrfCmdSize {
+impl<T> TryFrom<BoundedInt<T, 3>> for DmaTrfCmdSize
+where
+ T: Boundable<3>,
+{
type Error = Error;
- fn try_from(value: u8) -> Result<Self> {
- match value {
+ fn try_from(value: BoundedInt<T, 3>) -> Result<Self> {
+ match u8::from(value) {
0x6 => Ok(Self::Size256B),
_ => Err(EINVAL),
}
@@ -202,25 +219,20 @@ pub(crate) enum PeregrineCoreSelect {
/// RISC-V core is active.
Riscv = 1,
}
+impl_from_enum_to_bounded!(PeregrineCoreSelect, 1);
-impl From<bool> for PeregrineCoreSelect {
- fn from(value: bool) -> Self {
- match value {
+impl<T> From<BoundedInt<T, 1>> for PeregrineCoreSelect
+where
+ T: Boundable<1> + Zeroable,
+{
+ fn from(value: BoundedInt<T, 1>) -> Self {
+ match bool::from(value) {
false => PeregrineCoreSelect::Falcon,
true => PeregrineCoreSelect::Riscv,
}
}
}
-impl From<PeregrineCoreSelect> for bool {
- fn from(value: PeregrineCoreSelect) -> Self {
- match value {
- PeregrineCoreSelect::Falcon => false,
- PeregrineCoreSelect::Riscv => true,
- }
- }
-}
-
/// Different types of memory present in a falcon core.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub(crate) enum FalconMem {
@@ -244,14 +256,17 @@ pub(crate) enum FalconFbifTarget {
/// Non-coherent system memory (System DRAM).
NoncoherentSysmem = 2,
}
-impl_from_enum_to_u8!(FalconFbifTarget);
+impl_from_enum_to_bounded!(FalconFbifTarget, 2);
// TODO[FPRI]: replace with `FromPrimitive`.
-impl TryFrom<u8> for FalconFbifTarget {
+impl<T> TryFrom<BoundedInt<T, 2>> for FalconFbifTarget
+where
+ T: Boundable<2>,
+{
type Error = Error;
- fn try_from(value: u8) -> Result<Self> {
- let res = match value {
+ fn try_from(value: BoundedInt<T, 2>) -> Result<Self> {
+ let res = match u8::from(value) {
0 => Self::LocalFb,
1 => Self::CoherentSysmem,
2 => Self::NoncoherentSysmem,
@@ -271,26 +286,21 @@ pub(crate) enum FalconFbifMemType {
/// Physical memory addresses.
Physical = 1,
}
+impl_from_enum_to_bounded!(FalconFbifMemType, 1);
/// Conversion from a single-bit register field.
-impl From<bool> for FalconFbifMemType {
- fn from(value: bool) -> Self {
- match value {
+impl<T> From<BoundedInt<T, 1>> for FalconFbifMemType
+where
+ T: Boundable<1> + Zeroable,
+{
+ fn from(value: BoundedInt<T, 1>) -> Self {
+ match bool::from(value) {
false => Self::Virtual,
true => Self::Physical,
}
}
}
-impl From<FalconFbifMemType> for bool {
- fn from(value: FalconFbifMemType) -> Self {
- match value {
- FalconFbifMemType::Virtual => false,
- FalconFbifMemType::Physical => true,
- }
- }
-}
-
/// Type used to represent the `PFALCON` registers address base for a given falcon engine.
pub(crate) struct PFalconBase(());
@@ -440,7 +450,7 @@ pub(crate) fn reset(&self, bar: &Bar0) -> Result {
self.reset_wait_mem_scrubbing(bar)?;
regs::NV_PFALCON_FALCON_RM::default()
- .set_value(regs::NV_PMC_BOOT_0::read(bar).into())
+ .set_value(u32::from(regs::NV_PMC_BOOT_0::read(bar)))
.write(bar, &E::ID);
Ok(())
@@ -507,18 +517,18 @@ fn dma_wr<F: FalconFirmware<Target = E>>(
.set_base((dma_start >> 8) as u32)
.write(bar, &E::ID);
regs::NV_PFALCON_FALCON_DMATRFBASE1::default()
- .set_base((dma_start >> 40) as u16)
+ .try_set_base(dma_start >> 40)?
.write(bar, &E::ID);
let cmd = regs::NV_PFALCON_FALCON_DMATRFCMD::default()
.set_size(DmaTrfCmdSize::Size256B)
.set_imem(target_mem == FalconMem::Imem)
- .set_sec(if sec { 1 } else { 0 });
+ .set_sec(BoundedInt::new(if sec { 1 } else { 0 }));
for pos in (0..num_transfers).map(|i| i * DMA_LEN) {
// Perform a transfer of size `DMA_LEN`.
regs::NV_PFALCON_FALCON_DMATRFMOFFS::default()
- .set_offs(load_offsets.dst_start + pos)
+ .try_set_offs(load_offsets.dst_start + pos)?
.write(bar, &E::ID);
regs::NV_PFALCON_FALCON_DMATRFFBOFFS::default()
.set_offs(src_start + pos)
diff --git a/drivers/gpu/nova-core/falcon/hal/ga102.rs b/drivers/gpu/nova-core/falcon/hal/ga102.rs
index 0b1cbe7853b3..08c8b4123ce8 100644
--- a/drivers/gpu/nova-core/falcon/hal/ga102.rs
+++ b/drivers/gpu/nova-core/falcon/hal/ga102.rs
@@ -55,7 +55,7 @@ fn signature_reg_fuse_version_ga102(
// `ucode_idx` is guaranteed to be in the range [0..15], making the `read` calls provable valid
// at build-time.
- let reg_fuse_version = if engine_id_mask & 0x0001 != 0 {
+ let reg_fuse_version: u16 = if engine_id_mask & 0x0001 != 0 {
regs::NV_FUSE_OPT_FPF_SEC2_UCODE1_VERSION::read(bar, ucode_idx).data()
} else if engine_id_mask & 0x0004 != 0 {
regs::NV_FUSE_OPT_FPF_NVDEC_UCODE1_VERSION::read(bar, ucode_idx).data()
@@ -64,7 +64,8 @@ fn signature_reg_fuse_version_ga102(
} else {
dev_err!(dev, "unexpected engine_id_mask {:#x}", engine_id_mask);
return Err(EINVAL);
- };
+ }
+ .into();
// TODO[NUMM]: replace with `last_set_bit` once it lands.
Ok(u16::BITS - reg_fuse_version.leading_zeros())
diff --git a/drivers/gpu/nova-core/fb/hal/ga100.rs b/drivers/gpu/nova-core/fb/hal/ga100.rs
index 871c42bf033a..5449902f2489 100644
--- a/drivers/gpu/nova-core/fb/hal/ga100.rs
+++ b/drivers/gpu/nova-core/fb/hal/ga100.rs
@@ -2,6 +2,7 @@
struct Ga100;
+use kernel::num::BoundedInt;
use kernel::prelude::*;
use crate::driver::Bar0;
@@ -18,7 +19,7 @@ pub(super) fn read_sysmem_flush_page_ga100(bar: &Bar0) -> u64 {
pub(super) fn write_sysmem_flush_page_ga100(bar: &Bar0, addr: u64) {
regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI::default()
- .set_adr_63_40((addr >> FLUSH_SYSMEM_ADDR_SHIFT_HI) as u32)
+ .set_adr_63_40(BoundedInt::new(addr >> FLUSH_SYSMEM_ADDR_SHIFT_HI).cast())
.write(bar);
regs::NV_PFB_NISO_FLUSH_SYSMEM_ADDR::default()
.set_adr_39_08((addr >> FLUSH_SYSMEM_ADDR_SHIFT) as u32)
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 5da9ad726483..7ed382d82fc7 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
+use kernel::num::BoundedInt;
use kernel::{device, devres::Devres, error::code::*, pci, prelude::*, sync::Arc};
use crate::driver::Bar0;
@@ -131,15 +132,15 @@ fn try_from(value: u8) -> Result<Self> {
}
pub(crate) struct Revision {
- major: u8,
- minor: u8,
+ major: BoundedInt<u8, 4>,
+ minor: BoundedInt<u8, 4>,
}
impl Revision {
fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self {
Self {
- major: boot0.major_revision(),
- minor: boot0.minor_revision(),
+ major: boot0.major_revision().cast(),
+ minor: boot0.minor_revision().cast(),
}
}
}
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 206dab2e1335..1542d72e4a65 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -17,18 +17,19 @@
// PMC
register!(NV_PMC_BOOT_0 @ 0x00000000, "Basic revision information about the GPU" {
- 3:0 minor_revision as u8, "Minor revision of the chip";
- 7:4 major_revision as u8, "Major revision of the chip";
- 8:8 architecture_1 as u8, "MSB of the architecture";
- 23:20 implementation as u8, "Implementation version of the architecture";
- 28:24 architecture_0 as u8, "Lower bits of the architecture";
+ 3:0 minor_revision, "Minor revision of the chip";
+ 7:4 major_revision, "Major revision of the chip";
+ 8:8 architecture_1, "MSB of the architecture";
+ 23:20 implementation, "Implementation version of the architecture";
+ 28:24 architecture_0, "Lower bits of the architecture";
});
impl NV_PMC_BOOT_0 {
/// Combines `architecture_0` and `architecture_1` to obtain the architecture of the chip.
pub(crate) fn architecture(self) -> Result<Architecture> {
Architecture::try_from(
- self.architecture_0() | (self.architecture_1() << Self::ARCHITECTURE_0_RANGE.len()),
+ u8::from(self.architecture_0())
+ | (u8::from(self.architecture_1()) << Self::ARCHITECTURE_0_RANGE.len()),
)
}
@@ -49,7 +50,7 @@ pub(crate) fn chipset(self) -> Result<Chipset> {
register!(NV_PBUS_SW_SCRATCH_0E_FRTS_ERR => NV_PBUS_SW_SCRATCH[0xe],
"scratch register 0xe used as FRTS firmware error code" {
- 31:16 frts_err_code as u16;
+ 31:16 frts_err_code;
});
// PFB
@@ -58,17 +59,17 @@ pub(crate) fn chipset(self) -> Result<Chipset> {
// GPU to perform sysmembar operations (see `fb::SysmemFlush`).
register!(NV_PFB_NISO_FLUSH_SYSMEM_ADDR @ 0x00100c10 {
- 31:0 adr_39_08 as u32;
+ 31:0 adr_39_08;
});
register!(NV_PFB_NISO_FLUSH_SYSMEM_ADDR_HI @ 0x00100c40 {
- 23:0 adr_63_40 as u32;
+ 23:0 adr_63_40;
});
register!(NV_PFB_PRI_MMU_LOCAL_MEMORY_RANGE @ 0x00100ce0 {
- 3:0 lower_scale as u8;
- 9:4 lower_mag as u8;
- 30:30 ecc_mode_enabled as bool;
+ 3:0 lower_scale;
+ 9:4 lower_mag;
+ 30:30 ecc_mode_enabled => bool;
});
impl NV_PFB_PRI_MMU_LOCAL_MEMORY_RANGE {
@@ -87,7 +88,7 @@ pub(crate) fn usable_fb_size(self) -> u64 {
}
register!(NV_PFB_PRI_MMU_WPR2_ADDR_LO@0x001fa824 {
- 31:4 lo_val as u32, "Bits 12..40 of the lower (inclusive) bound of the WPR2 region";
+ 31:4 lo_val, "Bits 12..40 of the lower (inclusive) bound of the WPR2 region";
});
impl NV_PFB_PRI_MMU_WPR2_ADDR_LO {
@@ -98,7 +99,7 @@ pub(crate) fn lower_bound(self) -> u64 {
}
register!(NV_PFB_PRI_MMU_WPR2_ADDR_HI@0x001fa828 {
- 31:4 hi_val as u32, "Bits 12..40 of the higher (exclusive) bound of the WPR2 region";
+ 31:4 hi_val, "Bits 12..40 of the higher (exclusive) bound of the WPR2 region";
});
impl NV_PFB_PRI_MMU_WPR2_ADDR_HI {
@@ -123,7 +124,7 @@ pub(crate) fn higher_bound(self) -> u64 {
// `PGC6_AON_SECURE_SCRATCH_GROUP_05` register (which it needs to read GFW_BOOT).
register!(NV_PGC6_AON_SECURE_SCRATCH_GROUP_05_PRIV_LEVEL_MASK @ 0x00118128,
"Privilege level mask register" {
- 0:0 read_protection_level0 as bool, "Set after FWSEC lowers its protection level";
+ 0:0 read_protection_level0 => bool, "Set after FWSEC lowers its protection level";
});
// OpenRM defines this as a register array, but doesn't specify its size and only uses its first
@@ -133,7 +134,7 @@ pub(crate) fn higher_bound(self) -> u64 {
register!(
NV_PGC6_AON_SECURE_SCRATCH_GROUP_05_0_GFW_BOOT => NV_PGC6_AON_SECURE_SCRATCH_GROUP_05[0],
"Scratch group 05 register 0 used as GFW boot progress indicator" {
- 7:0 progress as u8, "Progress of GFW boot (0xff means completed)";
+ 7:0 progress, "Progress of GFW boot (0xff means completed)";
}
);
@@ -145,13 +146,13 @@ pub(crate) fn completed(self) -> bool {
}
register!(NV_PGC6_AON_SECURE_SCRATCH_GROUP_42 @ 0x001183a4 {
- 31:0 value as u32;
+ 31:0 value;
});
register!(
NV_USABLE_FB_SIZE_IN_MB => NV_PGC6_AON_SECURE_SCRATCH_GROUP_42,
"Scratch group 42 register used as framebuffer size" {
- 31:0 value as u32, "Usable framebuffer size, in megabytes";
+ 31:0 value, "Usable framebuffer size, in megabytes";
}
);
@@ -165,8 +166,8 @@ pub(crate) fn usable_fb_size(self) -> u64 {
// PDISP
register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 {
- 3:3 status_valid as bool, "Set if the `addr` field is valid";
- 31:8 addr as u32, "VGA workspace base address divided by 0x10000";
+ 3:3 status_valid => bool, "Set if the `addr` field is valid";
+ 31:8 addr, "VGA workspace base address divided by 0x10000";
});
impl NV_PDISP_VGA_WORKSPACE_BASE {
@@ -185,40 +186,40 @@ pub(crate) fn vga_workspace_addr(self) -> Option<u64> {
pub(crate) const NV_FUSE_OPT_FPF_SIZE: usize = 16;
register!(NV_FUSE_OPT_FPF_NVDEC_UCODE1_VERSION @ 0x00824100[NV_FUSE_OPT_FPF_SIZE] {
- 15:0 data as u16;
+ 15:0 data;
});
register!(NV_FUSE_OPT_FPF_SEC2_UCODE1_VERSION @ 0x00824140[NV_FUSE_OPT_FPF_SIZE] {
- 15:0 data as u16;
+ 15:0 data;
});
register!(NV_FUSE_OPT_FPF_GSP_UCODE1_VERSION @ 0x008241c0[NV_FUSE_OPT_FPF_SIZE] {
- 15:0 data as u16;
+ 15:0 data;
});
// PFALCON
register!(NV_PFALCON_FALCON_IRQSCLR @ PFalconBase[0x00000004] {
- 4:4 halt as bool;
- 6:6 swgen0 as bool;
+ 4:4 halt => bool;
+ 6:6 swgen0 => bool;
});
register!(NV_PFALCON_FALCON_MAILBOX0 @ PFalconBase[0x00000040] {
- 31:0 value as u32;
+ 31:0 value => u32;
});
register!(NV_PFALCON_FALCON_MAILBOX1 @ PFalconBase[0x00000044] {
- 31:0 value as u32;
+ 31:0 value => u32;
});
register!(NV_PFALCON_FALCON_RM @ PFalconBase[0x00000084] {
- 31:0 value as u32;
+ 31:0 value => u32;
});
register!(NV_PFALCON_FALCON_HWCFG2 @ PFalconBase[0x000000f4] {
- 10:10 riscv as bool;
- 12:12 mem_scrubbing as bool, "Set to 0 after memory scrubbing is completed";
- 31:31 reset_ready as bool, "Signal indicating that reset is completed (GA102+)";
+ 10:10 riscv => bool;
+ 12:12 mem_scrubbing => bool, "Set to 0 after memory scrubbing is completed";
+ 31:31 reset_ready => bool, "Signal indicating that reset is completed (GA102+)";
});
impl NV_PFALCON_FALCON_HWCFG2 {
@@ -229,101 +230,101 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
}
register!(NV_PFALCON_FALCON_CPUCTL @ PFalconBase[0x00000100] {
- 1:1 startcpu as bool;
- 4:4 halted as bool;
- 6:6 alias_en as bool;
+ 1:1 startcpu => bool;
+ 4:4 halted => bool;
+ 6:6 alias_en => bool;
});
register!(NV_PFALCON_FALCON_BOOTVEC @ PFalconBase[0x00000104] {
- 31:0 value as u32;
+ 31:0 value => u32;
});
register!(NV_PFALCON_FALCON_DMACTL @ PFalconBase[0x0000010c] {
- 0:0 require_ctx as bool;
- 1:1 dmem_scrubbing as bool;
- 2:2 imem_scrubbing as bool;
- 6:3 dmaq_num as u8;
- 7:7 secure_stat as bool;
+ 0:0 require_ctx => bool;
+ 1:1 dmem_scrubbing => bool;
+ 2:2 imem_scrubbing => bool;
+ 6:3 dmaq_num;
+ 7:7 secure_stat => bool;
});
register!(NV_PFALCON_FALCON_DMATRFBASE @ PFalconBase[0x00000110] {
- 31:0 base as u32;
+ 31:0 base => u32;
});
register!(NV_PFALCON_FALCON_DMATRFMOFFS @ PFalconBase[0x00000114] {
- 23:0 offs as u32;
+ 23:0 offs;
});
register!(NV_PFALCON_FALCON_DMATRFCMD @ PFalconBase[0x00000118] {
- 0:0 full as bool;
- 1:1 idle as bool;
- 3:2 sec as u8;
- 4:4 imem as bool;
- 5:5 is_write as bool;
- 10:8 size as u8 ?=> DmaTrfCmdSize;
- 14:12 ctxdma as u8;
- 16:16 set_dmtag as u8;
+ 0:0 full => bool;
+ 1:1 idle => bool;
+ 3:2 sec;
+ 4:4 imem => bool;
+ 5:5 is_write => bool;
+ 10:8 size ?=> DmaTrfCmdSize;
+ 14:12 ctxdma;
+ 16:16 set_dmtag;
});
register!(NV_PFALCON_FALCON_DMATRFFBOFFS @ PFalconBase[0x0000011c] {
- 31:0 offs as u32;
+ 31:0 offs => u32;
});
register!(NV_PFALCON_FALCON_DMATRFBASE1 @ PFalconBase[0x00000128] {
- 8:0 base as u16;
+ 8:0 base;
});
register!(NV_PFALCON_FALCON_HWCFG1 @ PFalconBase[0x0000012c] {
- 3:0 core_rev as u8 ?=> FalconCoreRev, "Core revision";
- 5:4 security_model as u8 ?=> FalconSecurityModel, "Security model";
- 7:6 core_rev_subversion as u8 ?=> FalconCoreRevSubversion, "Core revision subversion";
+ 3:0 core_rev ?=> FalconCoreRev, "Core revision";
+ 5:4 security_model ?=> FalconSecurityModel, "Security model";
+ 7:6 core_rev_subversion => FalconCoreRevSubversion, "Core revision subversion";
});
register!(NV_PFALCON_FALCON_CPUCTL_ALIAS @ PFalconBase[0x00000130] {
- 1:1 startcpu as bool;
+ 1:1 startcpu => bool;
});
// Actually known as `NV_PSEC_FALCON_ENGINE` and `NV_PGSP_FALCON_ENGINE` depending on the falcon
// instance.
register!(NV_PFALCON_FALCON_ENGINE @ PFalconBase[0x000003c0] {
- 0:0 reset as bool;
+ 0:0 reset => bool;
});
register!(NV_PFALCON_FBIF_TRANSCFG @ PFalconBase[0x00000600[8]] {
- 1:0 target as u8 ?=> FalconFbifTarget;
- 2:2 mem_type as bool => FalconFbifMemType;
+ 1:0 target ?=> FalconFbifTarget;
+ 2:2 mem_type => FalconFbifMemType;
});
register!(NV_PFALCON_FBIF_CTL @ PFalconBase[0x00000624] {
- 7:7 allow_phys_no_ctx as bool;
+ 7:7 allow_phys_no_ctx => bool;
});
/* PFALCON2 */
register!(NV_PFALCON2_FALCON_MOD_SEL @ PFalcon2Base[0x00000180] {
- 7:0 algo as u8 ?=> FalconModSelAlgo;
+ 7:0 algo ?=> FalconModSelAlgo;
});
register!(NV_PFALCON2_FALCON_BROM_CURR_UCODE_ID @ PFalcon2Base[0x00000198] {
- 7:0 ucode_id as u8;
+ 7:0 ucode_id => u8;
});
register!(NV_PFALCON2_FALCON_BROM_ENGIDMASK @ PFalcon2Base[0x0000019c] {
- 31:0 value as u32;
+ 31:0 value => u32;
});
// OpenRM defines this as a register array, but doesn't specify its size and only uses its first
// element. Be conservative until we know the actual size or need to use more registers.
register!(NV_PFALCON2_FALCON_BROM_PARAADDR @ PFalcon2Base[0x00000210[1]] {
- 31:0 value as u32;
+ 31:0 value => u32;
});
// PRISCV
register!(NV_PRISCV_RISCV_BCR_CTRL @ PFalconBase[0x00001668] {
- 0:0 valid as bool;
- 4:4 core_select as bool => PeregrineCoreSelect;
- 8:8 br_fetch as bool;
+ 0:0 valid => bool;
+ 4:4 core_select => PeregrineCoreSelect;
+ 8:8 br_fetch => bool;
});
// The modules below provide registers that are not identical on all supported chips. They should
@@ -333,7 +334,7 @@ pub(crate) mod gm107 {
// FUSE
register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00021c04 {
- 0:0 display_disabled as bool;
+ 0:0 display_disabled => bool;
});
}
@@ -341,6 +342,6 @@ pub(crate) mod ga100 {
// FUSE
register!(NV_FUSE_STATUS_OPT_DISPLAY @ 0x00820c04 {
- 0:0 display_disabled as bool;
+ 0:0 display_disabled => bool;
});
}
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index 73811a115762..54c7f6fc746b 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -398,12 +398,9 @@ fn from(reg: $name) -> u32 {
register!(@fields_dispatcher $name { $($fields)* });
};
- // Captures the fields and passes them to all the implementers that require field information.
- //
- // Used to simplify the matching rules for implementers, so they don't need to match the entire
- // complex fields rule even though they only make use of part of it.
+ // Dispatch fields for the bounded integers syntax.
(@fields_dispatcher $name:ident {
- $($hi:tt:$lo:tt $field:ident as $type:tt
+ $($hi:tt:$lo:tt $field:ident
$(?=> $try_into_type:ty)?
$(=> $into_type:ty)?
$(, $comment:literal)?
@@ -411,123 +408,25 @@ fn from(reg: $name) -> u32 {
)*
}
) => {
- register!(@field_accessors $name {
- $(
- $hi:$lo $field as $type
- $(?=> $try_into_type)?
- $(=> $into_type)?
- $(, $comment)?
- ;
- )*
- });
- register!(@debug $name { $($field;)* });
- register!(@default $name { $($field;)* });
- };
-
- // Defines all the field getter/methods methods for `$name`.
- (
- @field_accessors $name:ident {
- $($hi:tt:$lo:tt $field:ident as $type:tt
- $(?=> $try_into_type:ty)?
- $(=> $into_type:ty)?
- $(, $comment:literal)?
- ;
- )*
- }
- ) => {
- $(
- register!(@check_field_bounds $hi:$lo $field as $type);
- )*
-
#[allow(dead_code)]
impl $name {
- $(
- register!(@field_accessor $name $hi:$lo $field as $type
- $(?=> $try_into_type)?
- $(=> $into_type)?
- $(, $comment)?
- ;
- );
- )*
- }
- };
-
- // Boolean fields must have `$hi == $lo`.
- (@check_field_bounds $hi:tt:$lo:tt $field:ident as bool) => {
- #[allow(clippy::eq_op)]
- const _: () = {
- ::kernel::build_assert!(
- $hi == $lo,
- concat!("boolean field `", stringify!($field), "` covers more than one bit")
- );
- };
- };
-
- // Non-boolean fields must have `$hi >= $lo`.
- (@check_field_bounds $hi:tt:$lo:tt $field:ident as $type:tt) => {
- #[allow(clippy::eq_op)]
- const _: () = {
- ::kernel::build_assert!(
- $hi >= $lo,
- concat!("field `", stringify!($field), "`'s MSB is smaller than its LSB")
- );
- };
- };
-
- // Catches fields defined as `bool` and convert them into a boolean value.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool => $into_type:ty
- $(, $comment:literal)?;
- ) => {
- register!(
- @leaf_accessor $name $hi:$lo $field
- { |f| <$into_type>::from(if f != 0 { true } else { false }) }
- bool $into_type => $into_type $(, $comment)?;
+ $(
+ register!(@private_field_accessors $name $hi:$lo $field);
+ register!(@public_field_accessors $name $hi:$lo $field
+ $(?=> $try_into_type)?
+ $(=> $into_type)?
+ $(, $comment)?
);
+ )*
+ }
+
+ register!(@debug $name { $($field;)* });
+ register!(@default $name { $($field;)* });
+
};
- // Shortcut for fields defined as `bool` without the `=>` syntax.
(
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as bool $(, $comment:literal)?;
- ) => {
- register!(@field_accessor $name $hi:$lo $field as bool => bool $(, $comment)?;);
- };
-
- // Catches the `?=>` syntax for non-boolean fields.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt ?=> $try_into_type:ty
- $(, $comment:literal)?;
- ) => {
- register!(@leaf_accessor $name $hi:$lo $field
- { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
- ::core::result::Result<
- $try_into_type,
- <$try_into_type as ::core::convert::TryFrom<$type>>::Error
- >
- $(, $comment)?;);
- };
-
- // Catches the `=>` syntax for non-boolean fields.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt => $into_type:ty
- $(, $comment:literal)?;
- ) => {
- register!(@leaf_accessor $name $hi:$lo $field
- { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
- };
-
- // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
- (
- @field_accessor $name:ident $hi:tt:$lo:tt $field:ident as $type:tt
- $(, $comment:literal)?;
- ) => {
- register!(@field_accessor $name $hi:$lo $field as $type => $type $(, $comment)?;);
- };
-
- // Generates the accessor methods for a single field.
- (
- @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
- { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
+ @private_field_accessors $name:ident $hi:tt:$lo:tt $field:ident
) => {
::kernel::macros::paste!(
const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
@@ -535,38 +434,135 @@ impl $name {
const [<$field:upper _SHIFT>]: u32 = Self::[<$field:upper _MASK>].trailing_zeros();
);
- $(
- #[doc="Returns the value of this field:"]
- #[doc=$comment]
- )?
- #[inline(always)]
- pub(crate) fn $field(self) -> $res_type {
- ::kernel::macros::paste!(
+ ::kernel::macros::paste!(
+ fn [<$field _internal>](self) ->
+ ::kernel::num::BoundedInt<u32, { $hi + 1 - $lo }> {
const MASK: u32 = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
+ // Ensure the returned type has the same width as the field.
+ ::kernel::static_assert!(
+ MASK >> SHIFT == <u32 as ::kernel::num::Boundable<{ $hi + 1 - $lo }>>::MASK
);
+
let field = ((self.0 & MASK) >> SHIFT);
- $process(field)
+ ::kernel::num::BoundedInt::<u32, { $hi + 1 - $lo }>::new(field)
}
- ::kernel::macros::paste!(
- $(
- #[doc="Sets the value of this field:"]
- #[doc=$comment]
- )?
- #[inline(always)]
- pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
+ fn [<set_ $field _internal>](
+ mut self,
+ value: ::kernel::num::BoundedInt<u32, { $hi + 1 - $lo }>,
+ ) -> Self
+ {
const MASK: u32 = $name::[<$field:upper _MASK>];
const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
- let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
+ // Ensure the returned type has the same width as the field.
+ ::kernel::static_assert!(
+ MASK >> SHIFT == <u32 as ::kernel::num::Boundable<{ $hi + 1 - $lo }>>::MASK
+ );
+
+ let value = (value.get() << SHIFT) & MASK;
self.0 = (self.0 & !MASK) | value;
self
}
+
+ fn [<try_set_ $field _internal>]<T>(
+ mut self,
+ value: T,
+ ) -> ::kernel::error::Result<Self>
+ where T: ::kernel::num::TryIntoBounded<u32, { $hi + 1 - $lo }>,
+ {
+ const MASK: u32 = $name::[<$field:upper _MASK>];
+ const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
+ // Ensure the returned type has the same width as the field.
+ ::kernel::static_assert!(
+ MASK >> SHIFT == <u32 as ::kernel::num::Boundable<{ $hi + 1 - $lo }>>::MASK
+ );
+ let value = value.try_into()?;
+
+ let value = (value.get() << SHIFT) & MASK;
+ self.0 = (self.0 & !MASK) | value;
+
+ Ok(self)
+ }
);
};
+ // Generates the public accessors for fields infallibly (`=>`) converted to a type.
+ (
+ @public_field_accessors $name:ident $hi:tt:$lo:tt $field:ident => $into_type:ty
+ $(, $comment:literal)?
+ ) => {
+ ::kernel::macros::paste!(
+ pub(crate) fn $field(self) -> $into_type
+ {
+ self.[<$field _internal>]().into()
+ }
+
+ pub(crate) fn [<set_ $field>](self, value: $into_type) -> Self
+ {
+ self.[<set_ $field _internal>](value.into())
+ }
+ );
+ };
+
+ // Generates the public accessors for fields fallibly (`?=>`) converted to a type.
+ (
+ @public_field_accessors $name:ident $hi:tt:$lo:tt $field:ident ?=> $try_into_type:ty
+ $(, $comment:literal)?
+ ) => {
+ ::kernel::macros::paste!(
+ pub(crate) fn $field(self) ->
+ Result<
+ $try_into_type,
+ <$try_into_type as ::core::convert::TryFrom<
+ ::kernel::num::BoundedInt<u32, { $hi + 1 - $lo }>
+ >>::Error
+ >
+ {
+ self.[<$field _internal>]().try_into()
+ }
+
+ pub(crate) fn [<set_ $field>](self, value: $try_into_type) -> Self
+ {
+ self.[<set_ $field _internal>](value.into())
+ }
+ );
+ };
+
+ // Generates the public accessors for fields not converted to a type.
+ (
+ @public_field_accessors $name:ident $hi:tt:$lo:tt $field:ident $(, $comment:literal)?
+ ) => {
+ ::kernel::macros::paste!(
+ pub(crate) fn $field(self) ->
+ ::kernel::num::BoundedInt<u32, { $hi + 1 - $lo }>
+ {
+ self.[<$field _internal>]()
+ }
+
+ pub(crate) fn [<set_ $field>]<T>(
+ self,
+ value: T,
+ ) -> Self
+ where T: Into<::kernel::num::BoundedInt<u32, { $hi + 1 - $lo }>>,
+ {
+ self.[<set_ $field _internal>](value.into())
+ }
+
+ pub(crate) fn [<try_set_ $field>]<T>(
+ self,
+ value: T,
+ ) -> ::kernel::error::Result<Self>
+ where T: ::kernel::num::TryIntoBounded<u32, { $hi + 1 - $lo }>,
+ {
+ Ok(self.[<set_ $field _internal>](value.try_into()?))
+ }
+ );
+ };
+
+
// Generates the `Debug` implementation for `$name`.
(@debug $name:ident { $($field:ident;)* }) => {
impl ::core::fmt::Debug for $name {
@@ -582,6 +578,8 @@ fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
};
// Generates the `Default` implementation for `$name`.
+ // TODO: hack - we should not use the internal method. Maybe add private methods returning the
+ // defaults value for each field?
(@default $name:ident { $($field:ident;)* }) => {
/// Returns a value for the register where all fields are set to their default value.
impl ::core::default::Default for $name {
@@ -591,7 +589,7 @@ fn default() -> Self {
::kernel::macros::paste!(
$(
- value.[<set_ $field>](Default::default());
+ value.[<set_ $field _internal>](Default::default());
)*
);
--
2.51.0
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation
2025-10-09 12:37 ` [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation Alexandre Courbot
@ 2025-10-09 15:17 ` Joel Fernandes
2025-10-09 15:41 ` Joel Fernandes
2025-10-09 20:10 ` Edwin Peer
2025-10-16 14:51 ` Joel Fernandes
2 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes @ 2025-10-09 15:17 UTC (permalink / raw)
To: Alexandre Courbot, Danilo Krummrich, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau, linux-kernel, rust-for-linux, Edwin Peer
Hi Alex,
On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
> The getter method of a field works with the field type, but its setter
> expects the type of the register. This leads to an asymmetry in the
> From/Into implementations required for a field with a dedicated type.
> For instance, a field declared as
>
> pub struct ControlReg(u32) {
> 3:0 mode as u8 ?=> Mode;
> ...
> }
>
> currently requires the following implementations:
>
> impl TryFrom<u8> for Mode {
> ...
> }
>
> impl From<Mode> for u32 {
> ...
> }
>
> Change this so the `From<Mode>` now needs to be implemented for `u8`,
> i.e. the primitive type of the field. This is more consistent, and will
> become a requirement once we start using the TryFrom/Into derive macros
> to implement these automatically.
>
> Reported-by: Edwin Peer <epeer@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
As these are incremental improvements, could you please rebase on top of the v6
bitfield series so it does not conflict?
https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation
2025-10-09 15:17 ` Joel Fernandes
@ 2025-10-09 15:41 ` Joel Fernandes
2025-10-10 8:24 ` Alexandre Courbot
0 siblings, 1 reply; 26+ messages in thread
From: Joel Fernandes @ 2025-10-09 15:41 UTC (permalink / raw)
To: Alexandre Courbot, Danilo Krummrich, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org,
rust-for-linux@vger.kernel.org, Edwin Peer
> On Oct 9, 2025, at 11:17 AM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>
> Hi Alex,
>
>> On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
>> The getter method of a field works with the field type, but its setter
>> expects the type of the register. This leads to an asymmetry in the
>> From/Into implementations required for a field with a dedicated type.
>> For instance, a field declared as
>>
>> pub struct ControlReg(u32) {
>> 3:0 mode as u8 ?=> Mode;
>> ...
>> }
>>
>> currently requires the following implementations:
>>
>> impl TryFrom<u8> for Mode {
>> ...
>> }
>>
>> impl From<Mode> for u32 {
>> ...
>> }
>>
>> Change this so the `From<Mode>` now needs to be implemented for `u8`,
>> i.e. the primitive type of the field. This is more consistent, and will
>> become a requirement once we start using the TryFrom/Into derive macros
>> to implement these automatically.
>>
>> Reported-by: Edwin Peer <epeer@nvidia.com>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>
> As these are incremental improvements, could you please rebase on top of the v6
> bitfield series so it does not conflict?
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6
On second thought, I could just carry this patch on top of my v6 series and avoid too much conflict.
So if it is ok with you, please only carry the last 2 patches of this series whenever applying.
For this patch:
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
I will review the other two patches as well. Thanks.
>
> Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 12:37 ` [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt Alexandre Courbot
@ 2025-10-09 16:40 ` Yury Norov
2025-10-09 17:18 ` Danilo Krummrich
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Yury Norov @ 2025-10-09 16:40 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Danilo Krummrich, Joel Fernandes, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux
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?
> 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,
});
> (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.
> 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@gmail.com/
> [2] https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>
> Signed-off-by: Alexandre Courbot <acourbot@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.
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);
Thanks,
Yury
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 16:40 ` Yury Norov
@ 2025-10-09 17:18 ` Danilo Krummrich
2025-10-09 18:28 ` Yury Norov
2025-10-09 18:29 ` Joel Fernandes
2025-10-10 9:19 ` Alexandre Courbot
2 siblings, 1 reply; 26+ messages in thread
From: Danilo Krummrich @ 2025-10-09 17:18 UTC (permalink / raw)
To: Yury Norov
Cc: Alexandre Courbot, Joel Fernandes, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux
On Thu Oct 9, 2025 at 6:40 PM CEST, Yury Norov wrote:
> 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?
Rust is a different language and has its own syntax, I think we should not try
to use C syntax instead.
>> 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
Note that try_set_base1() returns a Result [1], which is handled immediately by
the question mark operator [2]. I.e. if try_set_base1() returns an error it is
propagated to the caller right away without executing any of the code below.
> .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.
Where do you see a potential runtime overflows in the register!() code?
[1] https://rust.docs.kernel.org/kernel/error/type.Result.html
[2] https://doc.rust-lang.org/reference/expressions/operator-expr.html?highlight=question%20mark#the-question-mark-operator
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 17:18 ` Danilo Krummrich
@ 2025-10-09 18:28 ` Yury Norov
2025-10-09 18:37 ` Joel Fernandes
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Yury Norov @ 2025-10-09 18:28 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Alexandre Courbot, Joel Fernandes, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux
On Thu, Oct 09, 2025 at 07:18:33PM +0200, Danilo Krummrich wrote:
> On Thu Oct 9, 2025 at 6:40 PM CEST, Yury Norov wrote:
> > 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?
>
> Rust is a different language and has its own syntax, I think we should not try
> to use C syntax instead.
Up to you guys. But having in mind that C is the only language that
really works for system engineering, I would rather consider to stay
in line with it on semantic level.
If your goal is to make rust adopted by system engineers, you may
want to make your language somewhat familiar to what people already
know.
> >> 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
>
> Note that try_set_base1() returns a Result [1], which is handled immediately by
> the question mark operator [2]. I.e. if try_set_base1() returns an error it is
> propagated to the caller right away without executing any of the code below.
Thanks for the links. I am definitely the very beginning on the
learning curve for this.
> > .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.
>
> Where do you see a potential runtime overflows in the register!() code?
Assuming base is 10-bit,
let ret = some_c_wrapper() // 0..1024 or -EINVAL
regs::NV_PFALCON_FALCON_DMATRFBASE1::default()
.try_set_base1(ret)
Or maybe I misunderstood the question, because if there's no possibility
to overflow a field, what for the .try_set_xxx() is needed at all?
> [1] https://rust.docs.kernel.org/kernel/error/type.Result.html
> [2] https://doc.rust-lang.org/reference/expressions/operator-expr.html?highlight=question%20mark#the-question-mark-operator
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 16:40 ` Yury Norov
2025-10-09 17:18 ` Danilo Krummrich
@ 2025-10-09 18:29 ` Joel Fernandes
2025-10-10 9:19 ` Alexandre Courbot
2 siblings, 0 replies; 26+ messages in thread
From: Joel Fernandes @ 2025-10-09 18:29 UTC (permalink / raw)
To: Yury Norov, Alexandre Courbot
Cc: Danilo Krummrich, Jesung Yang, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau, linux-kernel,
rust-for-linux
On 10/9/2025 12:40 PM, Yury Norov wrote:
[..]
>> 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.
Because a getter has to convert the raw bitfield value from memory into an enum,
there is no guarantee that the memory in concern does not overflow the enum, say
if register bit meanings change, or something. ?=> was before fallible in both
directions hence the "?". Now with Alex's patch it is fallible only in one
direction.
>> 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@gmail.com/
>> [2] https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>>
>> Signed-off-by: Alexandre Courbot <acourbot@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
Danilo already replied here, but this code is fine as we early return due to the
try operator (?).
> .try_set_base2(base2 >> 40)? // skip
> .write(bar, &E::ID) else { pr_err!(); return -EINVAL };
Here I am guessing (based on your concern from an earlier thread) but is your
concern was that -EINVAL will get written to the register accidentally? That
wont happen, the above code is fine. Or do you mean something else?
>
> 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.
I'd be a bit careful when using "isn't safe" in the context of Rust. Rust's
notion of "safety" is about preventing undefined behavior (UB), not preventing
every possible runtime mistake. In rust, integer overflows for example are not
undefined so overflows are not "unsafe" (it might be a logical bug but that's
not what unsafety is about). In fact an overflow might be exactly what some
algorithm needs (wrap counters in RCU are example of harmless overflow). Another
example is a deadlock is not undefined behavior in Rust, it is defined, even if
bad :).
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 18:28 ` Yury Norov
@ 2025-10-09 18:37 ` Joel Fernandes
2025-10-09 19:19 ` Miguel Ojeda
2025-10-09 19:34 ` Danilo Krummrich
2 siblings, 0 replies; 26+ messages in thread
From: Joel Fernandes @ 2025-10-09 18:37 UTC (permalink / raw)
To: Yury Norov, Danilo Krummrich
Cc: Alexandre Courbot, Jesung Yang, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau, linux-kernel,
rust-for-linux
On 10/9/2025 2:28 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
>>
>> Note that try_set_base1() returns a Result [1], which is handled immediately by
>> the question mark operator [2]. I.e. if try_set_base1() returns an error it is
>> propagated to the caller right away without executing any of the code below.
>
> Thanks for the links. I am definitely the very beginning on the
> learning curve for this.
>
>>> .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.
>>
>> Where do you see a potential runtime overflows in the register!() code?
>
> Assuming base is 10-bit,
>
> let ret = some_c_wrapper() // 0..1024 or -EINVAL
> regs::NV_PFALCON_FALCON_DMATRFBASE1::default()
> .try_set_base1(ret)
>
> Or maybe I misunderstood the question, because if there's no possibility
> to overflow a field, what for the .try_set_xxx() is needed at all?
Because 'ret' is a value determined at runtime in this example, there is no way
for the compiler to know that ret will fit into the bounded int, at compile
time. So the "try_" means it is runtime checked and validated (via return of
Result). Sure it may well not fail, but the compiler doesn't know that.
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 18:28 ` Yury Norov
2025-10-09 18:37 ` Joel Fernandes
@ 2025-10-09 19:19 ` Miguel Ojeda
2025-10-09 19:34 ` Danilo Krummrich
2 siblings, 0 replies; 26+ messages in thread
From: Miguel Ojeda @ 2025-10-09 19:19 UTC (permalink / raw)
To: Yury Norov
Cc: Danilo Krummrich, Alexandre Courbot, Joel Fernandes, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, nouveau, linux-kernel, rust-for-linux
On Thu, Oct 9, 2025 at 8:28 PM Yury Norov <yury.norov@gmail.com> wrote:
>
> Up to you guys. But having in mind that C is the only language that
> really works for system engineering, I would rather consider to stay
> in line with it on semantic level.
>
> If your goal is to make rust adopted by system engineers, you may
> want to make your language somewhat familiar to what people already
> know.
I am not sure what you mean here, but just to clarify: our goal is to
improve Linux with Rust, not to increase its adoption (even if
obviously many of us would like that). And just in case, it is not
"our" language either -- most of us do not come from upstream Rust.
In general, we do try to make things as familiar as possible, but
sometimes it doesn't make much sense, so we don't follow C blindly.
For instance, C23 also got checked integer arithmetic macros, but that
doesn't mean we should start using names like `ckd_add`, returning
booleans and, worse, using macros for those. C23 also got `nullptr_t`
-- similar story.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 18:28 ` Yury Norov
2025-10-09 18:37 ` Joel Fernandes
2025-10-09 19:19 ` Miguel Ojeda
@ 2025-10-09 19:34 ` Danilo Krummrich
2 siblings, 0 replies; 26+ messages in thread
From: Danilo Krummrich @ 2025-10-09 19:34 UTC (permalink / raw)
To: Yury Norov
Cc: Alexandre Courbot, Joel Fernandes, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux
On Thu Oct 9, 2025 at 8:28 PM CEST, Yury Norov wrote:
> On Thu, Oct 09, 2025 at 07:18:33PM +0200, Danilo Krummrich wrote:
>> On Thu Oct 9, 2025 at 6:40 PM CEST, Yury Norov wrote:
>> > 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?
>>
>> Rust is a different language and has its own syntax, I think we should not try
>> to use C syntax instead.
>
> Up to you guys. But having in mind that C is the only language that
> really works for system engineering, I would rather consider to stay
> in line with it on semantic level.
I think you asked about syntax above; semantically it is (and should be) exactly
the same.
> If your goal is to make rust adopted by system engineers, you may
> want to make your language somewhat familiar to what people already
> know.
The goal is to add support for Rust in the Linux kernel; to adapt its advanced
type system features offering compile time checked lifetime and ownership
semantics leading to better memory safety and more compile time validation
overall.
In general I think we should stay as close to existing patterns as possible,
i.e. consistency does matter.
However, sometimes it is necessary to break with existing patterns and design
things slightly different to take full advantage of the capabilities we get from
the language (BoundedInt / BitInt is not one of those).
In other words, introducing a new language with capabilities that solve real
problems is pointless if we subsequently limit ourselfs to "what people already
know" for people who haven't been in touch with the language before.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation
2025-10-09 12:37 ` [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation Alexandre Courbot
2025-10-09 15:17 ` Joel Fernandes
@ 2025-10-09 20:10 ` Edwin Peer
2025-10-16 14:51 ` Joel Fernandes
2 siblings, 0 replies; 26+ messages in thread
From: Edwin Peer @ 2025-10-09 20:10 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Danilo Krummrich, Joel Fernandes, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, nouveau@lists.freedesktop.org,
linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org
> On Oct 9, 2025, at 5:37 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> The getter method of a field works with the field type, but its setter
> expects the type of the register. This leads to an asymmetry in the
> From/Into implementations required for a field with a dedicated type.
> For instance, a field declared as
>
> pub struct ControlReg(u32) {
> 3:0 mode as u8 ?=> Mode;
> ...
> }
>
> currently requires the following implementations:
>
> impl TryFrom<u8> for Mode {
> ...
> }
>
> impl From<Mode> for u32 {
> ...
> }
>
> Change this so the `From<Mode>` now needs to be implemented for `u8`,
> i.e. the primitive type of the field. This is more consistent, and will
> become a requirement once we start using the TryFrom/Into derive macros
> to implement these automatically.
>
> Reported-by: Edwin Peer <epeer@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> drivers/gpu/nova-core/falcon.rs | 38 +++++++++++++++++++++++++-----------
> drivers/gpu/nova-core/regs/macros.rs | 10 +++++-----
> 2 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 37e6298195e4..3f505b870601 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -22,11 +22,11 @@
> pub(crate) mod sec2;
>
> // TODO[FPRI]: Replace with `ToPrimitive`.
> -macro_rules! impl_from_enum_to_u32 {
> +macro_rules! impl_from_enum_to_u8 {
> ($enum_type:ty) => {
> - impl From<$enum_type> for u32 {
> + impl From<$enum_type> for u8 {
> fn from(value: $enum_type) -> Self {
> - value as u32
> + value as u8
> }
> }
> };
> @@ -46,7 +46,7 @@ pub(crate) enum FalconCoreRev {
> Rev6 = 6,
> Rev7 = 7,
> }
> -impl_from_enum_to_u32!(FalconCoreRev);
> +impl_from_enum_to_u8!(FalconCoreRev);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconCoreRev {
> @@ -81,7 +81,7 @@ pub(crate) enum FalconCoreRevSubversion {
> Subversion2 = 2,
> Subversion3 = 3,
> }
> -impl_from_enum_to_u32!(FalconCoreRevSubversion);
> +impl_from_enum_to_u8!(FalconCoreRevSubversion);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconCoreRevSubversion {
> @@ -125,7 +125,7 @@ pub(crate) enum FalconSecurityModel {
> /// Also known as High-Secure, Privilege Level 3 or PL3.
> Heavy = 3,
> }
> -impl_from_enum_to_u32!(FalconSecurityModel);
> +impl_from_enum_to_u8!(FalconSecurityModel);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconSecurityModel {
> @@ -157,7 +157,7 @@ pub(crate) enum FalconModSelAlgo {
> #[default]
> Rsa3k = 1,
> }
> -impl_from_enum_to_u32!(FalconModSelAlgo);
> +impl_from_enum_to_u8!(FalconModSelAlgo);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconModSelAlgo {
> @@ -179,7 +179,7 @@ pub(crate) enum DmaTrfCmdSize {
> #[default]
> Size256B = 0x6,
> }
> -impl_from_enum_to_u32!(DmaTrfCmdSize);
> +impl_from_enum_to_u8!(DmaTrfCmdSize);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for DmaTrfCmdSize {
> @@ -202,7 +202,6 @@ pub(crate) enum PeregrineCoreSelect {
> /// RISC-V core is active.
> Riscv = 1,
> }
> -impl_from_enum_to_u32!(PeregrineCoreSelect);
>
> impl From<bool> for PeregrineCoreSelect {
> fn from(value: bool) -> Self {
> @@ -213,6 +212,15 @@ fn from(value: bool) -> Self {
> }
> }
>
> +impl From<PeregrineCoreSelect> for bool {
> + fn from(value: PeregrineCoreSelect) -> Self {
> + match value {
> + PeregrineCoreSelect::Falcon => false,
> + PeregrineCoreSelect::Riscv => true,
> + }
> + }
> +}
> +
> /// Different types of memory present in a falcon core.
> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> pub(crate) enum FalconMem {
> @@ -236,7 +244,7 @@ pub(crate) enum FalconFbifTarget {
> /// Non-coherent system memory (System DRAM).
> NoncoherentSysmem = 2,
> }
> -impl_from_enum_to_u32!(FalconFbifTarget);
> +impl_from_enum_to_u8!(FalconFbifTarget);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconFbifTarget {
> @@ -263,7 +271,6 @@ pub(crate) enum FalconFbifMemType {
> /// Physical memory addresses.
> Physical = 1,
> }
> -impl_from_enum_to_u32!(FalconFbifMemType);
>
> /// Conversion from a single-bit register field.
> impl From<bool> for FalconFbifMemType {
> @@ -275,6 +282,15 @@ fn from(value: bool) -> Self {
> }
> }
>
> +impl From<FalconFbifMemType> for bool {
> + fn from(value: FalconFbifMemType) -> Self {
> + match value {
> + FalconFbifMemType::Virtual => false,
> + FalconFbifMemType::Physical => true,
> + }
> + }
> +}
> +
> /// Type used to represent the `PFALCON` registers address base for a given falcon engine.
> pub(crate) struct PFalconBase(());
>
> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
The examples in the doc comments should also be updated. Otherwise, LGTM.
Reviewed-by: Edwin Peer <epeer@nvidia.com>
Regards,
Edwin Peer
> index 754c14ee7f40..73811a115762 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/drivers/gpu/nova-core/regs/macros.rs
> @@ -482,7 +482,7 @@ impl $name {
> register!(
> @leaf_accessor $name $hi:$lo $field
> { |f| <$into_type>::from(if f != 0 { true } else { false }) }
> - $into_type => $into_type $(, $comment)?;
> + bool $into_type => $into_type $(, $comment)?;
> );
> };
>
> @@ -499,7 +499,7 @@ impl $name {
> $(, $comment:literal)?;
> ) => {
> register!(@leaf_accessor $name $hi:$lo $field
> - { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
> + { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
> ::core::result::Result<
> $try_into_type,
> <$try_into_type as ::core::convert::TryFrom<$type>>::Error
> @@ -513,7 +513,7 @@ impl $name {
> $(, $comment:literal)?;
> ) => {
> register!(@leaf_accessor $name $hi:$lo $field
> - { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
> + { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
> };
>
> // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
> @@ -527,7 +527,7 @@ impl $name {
> // Generates the accessor methods for a single field.
> (
> @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
> - { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
> + { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
> ) => {
> ::kernel::macros::paste!(
> const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
> @@ -559,7 +559,7 @@ pub(crate) fn $field(self) -> $res_type {
> pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
> const MASK: u32 = $name::[<$field:upper _MASK>];
> const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> - let value = (u32::from(value) << SHIFT) & MASK;
> + let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
> self.0 = (self.0 & !MASK) | value;
>
> self
>
> --
> 2.51.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 2/3] rust: kernel: add bounded integer types
2025-10-09 12:37 ` [PATCH RFC v2 2/3] rust: kernel: add bounded integer types Alexandre Courbot
@ 2025-10-09 21:33 ` Joel Fernandes
2025-10-10 8:49 ` Alexandre Courbot
2025-10-11 10:33 ` Alice Ryhl
1 sibling, 1 reply; 26+ messages in thread
From: Joel Fernandes @ 2025-10-09 21:33 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Danilo Krummrich, Yury Norov, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux
Hi Alex,
Great effort, thanks. I replied with few comments below. Since the patch is
large, it would be great if could be possibly split. Maybe the From
primitives deserve a separate patch.
On Thu, Oct 09, 2025 at 09:37:09PM +0900, Alexandre Courbot wrote:
> Add the BoundedInt type, which restricts the number of bits allowed to
> be used in a given integer value. This is useful to carry guarantees
> when setting bitfields.
>
> Alongside this type, many `From` and `TryFrom` implementations are
> provided to reduce friction when using with regular integer types. Proxy
> implementations of common integer traits are also provided.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> rust/kernel/lib.rs | 1 +
> rust/kernel/num.rs | 499 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 500 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index fcffc3988a90..21c1f452ee6a 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -101,6 +101,7 @@
> pub mod mm;
> #[cfg(CONFIG_NET)]
> pub mod net;
> +pub mod num;
> pub mod of;
> #[cfg(CONFIG_PM_OPP)]
> pub mod opp;
> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
> new file mode 100644
> index 000000000000..b2aad95ce51c
> --- /dev/null
> +++ b/rust/kernel/num.rs
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Numerical types for the kernel.
> +
> +use kernel::prelude::*;
> +
> +/// Integer type for which only the bits `0..NUM_BITS` are valid.
> +///
> +/// # Invariants
> +///
> +/// Stored values are represented with at most `NUM_BITS` bits.
> +#[repr(transparent)]
> +#[derive(Clone, Copy, Debug, Default, Hash)]
> +pub struct BoundedInt<T, const NUM_BITS: u32>(T);
> +
> +/// Returns `true` if `$value` can be represented with at most `$NUM_BITS` on `$type`.
> +macro_rules! is_in_bounds {
> + ($value:expr, $type:ty, $num_bits:expr) => {{
> + let v = $value;
> + v & <$type as Boundable<NUM_BITS>>::MASK == v
> + }};
> +}
> +
> +/// Trait for primitive integer types that can be used with `BoundedInt`.
> +pub trait Boundable<const NUM_BITS: u32>
> +where
> + Self: Sized + Copy + core::ops::BitAnd<Output = Self> + core::cmp::PartialEq,
> + Self: TryInto<u8> + TryInto<u16> + TryInto<u32> + TryInto<u64>,
> +{
> + /// Mask of the valid bits for this type.
> + const MASK: Self;
> +
> + /// Returns `true` if `value` can be represented with at most `NUM_BITS`.
> + ///
> + /// TODO: post-RFC: replace this with a left-shift followed by right-shift operation. This will
> + /// allow us to handle signed values as well.
> + fn is_in_bounds(value: Self) -> bool {
> + is_in_bounds!(value, Self, NUM_BITS)
> + }
> +}
> +
> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u8 {
> + const MASK: u8 = crate::bits::genmask_u8(0..=(NUM_BITS - 1));
> +}
> +
> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u16 {
> + const MASK: u16 = crate::bits::genmask_u16(0..=(NUM_BITS - 1));
> +}
> +
> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u32 {
> + const MASK: u32 = crate::bits::genmask_u32(0..=(NUM_BITS - 1));
> +}
> +
> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u64 {
> + const MASK: u64 = crate::bits::genmask_u64(0..=(NUM_BITS - 1));
> +}
> +
> +impl<T, const NUM_BITS: u32> BoundedInt<T, NUM_BITS>
> +where
> + T: Boundable<NUM_BITS>,
> +{
> + /// Checks that `value` is valid for this type at compile-time and build a new value.
> + ///
> + /// This relies on [`build_assert!`] to perform validation at compile-time. If `value` cannot
> + /// be inferred to be in bounds at compile-time, use the fallible [`Self::try_new`] instead.
> + ///
> + /// When possible, use one of the `new_const` methods instead of this method as it statically
> + /// validates `value` instead of relying on the compiler's optimizations.
This sounds like, users might use the less-optimal API first with the same
build_assert issues we had with the IO accessors, since new() sounds very obvious.
How about the following naming?
new::<VALUE>() // Primary constructor for constants using const generics.
try_new(value) // Keep as-is for fallible runtime
new_from_expr(value) // For compile-time validated runtime values
If new::<VALUE>() does not work for the user, the compiler will fail.
Or, new_from_expr() could be from_value(), Ok with either naming or a better name.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// use kernel::num::BoundedInt;
> + ///
> + /// # fn some_number() -> u32 { 0xffffffff }
> + ///
> + /// assert_eq!(BoundedInt::<u8, 1>::new(1).get(), 1);
> + /// assert_eq!(BoundedInt::<u16, 8>::new(0xff).get(), 0xff);
> + ///
> + /// // Triggers a build error as `0x1ff` doesn't fit into 8 bits.
> + /// // assert_eq!(BoundedInt::<u32, 8>::new(0x1ff).get(), 0x1ff);
> + ///
> + /// let v: u32 = some_number();
> + /// // Triggers a build error as `v` cannot be asserted to fit within 4 bits...
> + /// // let _ = BoundedInt::<u32, 4>::new(v);
> + /// // ... but this works as the compiler can assert the range from the mask.
> + /// let _ = BoundedInt::<u32, 4>::new(v & 0xf);
> + /// ```
> + pub fn new(value: T) -> Self {
> + crate::build_assert!(
> + T::is_in_bounds(value),
> + "Provided parameter is larger than maximal supported value"
> + );
> +
> + Self(value)
> + }
> +
> + /// Attempts to convert `value` into a value bounded by `NUM_BITS`.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// use kernel::num::BoundedInt;
> + ///
> + /// assert_eq!(BoundedInt::<u8, 1>::try_new(1).map(|v| v.get()), Ok(1));
> + /// assert_eq!(BoundedInt::<u16, 8>::try_new(0xff).map(|v| v.get()), Ok(0xff));
> + ///
> + /// // `0x1ff` doesn't fit into 8 bits.
> + /// assert_eq!(BoundedInt::<u32, 8>::try_new(0x1ff), Err(EOVERFLOW));
> + /// ```
> + pub fn try_new(value: T) -> Result<Self> {
> + if !T::is_in_bounds(value) {
> + Err(EOVERFLOW)
> + } else {
> + Ok(Self(value))
> + }
> + }
> +
> + /// Returns the contained value as a primitive type.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// use kernel::num::BoundedInt;
> + ///
> + /// let v = BoundedInt::<u32, 4>::new_const::<7>();
> + /// assert_eq!(v.get(), 7u32);
> + /// ```
> + pub fn get(self) -> T {
> + if !T::is_in_bounds(self.0) {
> + // SAFETY: Per the invariants, `self.0` cannot have bits set outside of `MASK`, so
> + // this block will
> + // never be reached.
> + unsafe { core::hint::unreachable_unchecked() }
> + }
Does this if block help the compiler generate better code? I wonder if code
gen could be checked to confirm the rationale.
> + self.0
> + }
> +
> + /// Increase the number of bits usable for `self`.
> + ///
> + /// This operation cannot fail.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// use kernel::num::BoundedInt;
> + ///
> + /// let v = BoundedInt::<u32, 4>::new_const::<7>();
> + /// let larger_v = v.enlarge::<12>();
> + /// // The contained values are equal even though `larger_v` has a bigger capacity.
> + /// assert_eq!(larger_v, v);
> + /// ```
> + pub const fn enlarge<const NEW_NUM_BITS: u32>(self) -> BoundedInt<T, NEW_NUM_BITS>
> + where
> + T: Boundable<NEW_NUM_BITS>,
> + T: Copy,
Boundable already implies copy so T: Copy is redundant.
> + {
> + build_assert!(NEW_NUM_BITS >= NUM_BITS);
> +
> + // INVARIANT: the value did fit within `NUM_BITS`, so it will all the more fit within
> + // `NEW_NUM_BITS` which is larger.
> + BoundedInt(self.0)
> + }
> +
> + /// Shrink the number of bits usable for `self`.
> + ///
> + /// Returns `EOVERFLOW` if the value of `self` cannot be represented within `NEW_NUM_BITS`.
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// use kernel::num::BoundedInt;
> + ///
> + /// let v = BoundedInt::<u32, 12>::new_const::<7>();
> + /// let smaller_v = v.shrink::<4>()?;
> + /// // The contained values are equal even though `smaller_v` has a smaller capacity.
> + /// assert_eq!(smaller_v, v);
> + ///
> + /// # Ok::<(), Error>(())
> + /// ```
> + pub fn shrink<const NEW_NUM_BITS: u32>(self) -> Result<BoundedInt<T, NEW_NUM_BITS>>
> + where
> + T: Boundable<NEW_NUM_BITS>,
> + T: Copy,
Here too.
[...]
> +impl_const_new!(u8 u16 u32 u64);
> +
> +/// Declares a new `$trait` and implements it for all bounded types represented using `$num_bits`.
> +///
> +/// This is used to declare properties as traits that we can use for later implementations.
> +macro_rules! impl_size_rule {
> + ($trait:ident, $($num_bits:literal)*) => {
> + trait $trait {}
> +
> + $(
> + impl<T> $trait for BoundedInt<T, $num_bits> where T: Boundable<$num_bits> {}
> + )*
> + };
> +}
> +
> +// Bounds that are larger than a `u64`.
> +impl_size_rule!(LargerThanU64, 64);
> +
> +// Bounds that are larger than a `u32`.
> +impl_size_rule!(LargerThanU32,
> + 32 33 34 35 36 37 38 39
If num_bits == 32 (number of bits), how could BoundedInt<T, 32> a
LargerThanU32? It should be AtleastU32 or something.
Or the above list should start from 33. Only a >= 33-bit wide integer can be
LargerThanU32.
Thanks.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation
2025-10-09 15:41 ` Joel Fernandes
@ 2025-10-10 8:24 ` Alexandre Courbot
2025-10-10 8:26 ` Alexandre Courbot
0 siblings, 1 reply; 26+ messages in thread
From: Alexandre Courbot @ 2025-10-10 8:24 UTC (permalink / raw)
To: Joel Fernandes, Danilo Krummrich, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org,
rust-for-linux@vger.kernel.org, Edwin Peer, Nouveau
On Fri Oct 10, 2025 at 12:41 AM JST, Joel Fernandes wrote:
>
>
>> On Oct 9, 2025, at 11:17 AM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>
>> Hi Alex,
>>
>>> On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
>>> The getter method of a field works with the field type, but its setter
>>> expects the type of the register. This leads to an asymmetry in the
>>> From/Into implementations required for a field with a dedicated type.
>>> For instance, a field declared as
>>>
>>> pub struct ControlReg(u32) {
>>> 3:0 mode as u8 ?=> Mode;
>>> ...
>>> }
>>>
>>> currently requires the following implementations:
>>>
>>> impl TryFrom<u8> for Mode {
>>> ...
>>> }
>>>
>>> impl From<Mode> for u32 {
>>> ...
>>> }
>>>
>>> Change this so the `From<Mode>` now needs to be implemented for `u8`,
>>> i.e. the primitive type of the field. This is more consistent, and will
>>> become a requirement once we start using the TryFrom/Into derive macros
>>> to implement these automatically.
>>>
>>> Reported-by: Edwin Peer <epeer@nvidia.com>
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>
>> As these are incremental improvements, could you please rebase on top of the v6
>> bitfield series so it does not conflict?
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6
>
> On second thought, I could just carry this patch on top of my v6 series and avoid too much conflict.
>
> So if it is ok with you, please only carry the last 2 patches of this series whenever applying.
>
> For this patch:
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
>
> I will review the other two patches as well. Thanks.
The idea is for this patch to go *before* your series, to avoid the
asymmetry in the From/Into implementions of bitfields. We could also put
it after, but it would become larger as a result and I think it can be
merge soon after -rc1 is tagged anyway.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation
2025-10-10 8:24 ` Alexandre Courbot
@ 2025-10-10 8:26 ` Alexandre Courbot
2025-10-10 14:59 ` Joel Fernandes
0 siblings, 1 reply; 26+ messages in thread
From: Alexandre Courbot @ 2025-10-10 8:26 UTC (permalink / raw)
To: Alexandre Courbot, Joel Fernandes, Danilo Krummrich, Yury Norov,
Jesung Yang, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org,
rust-for-linux@vger.kernel.org, Edwin Peer, Nouveau
On Fri Oct 10, 2025 at 5:24 PM JST, Alexandre Courbot wrote:
> On Fri Oct 10, 2025 at 12:41 AM JST, Joel Fernandes wrote:
>>
>>
>>> On Oct 9, 2025, at 11:17 AM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>>
>>> Hi Alex,
>>>
>>>> On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
>>>> The getter method of a field works with the field type, but its setter
>>>> expects the type of the register. This leads to an asymmetry in the
>>>> From/Into implementations required for a field with a dedicated type.
>>>> For instance, a field declared as
>>>>
>>>> pub struct ControlReg(u32) {
>>>> 3:0 mode as u8 ?=> Mode;
>>>> ...
>>>> }
>>>>
>>>> currently requires the following implementations:
>>>>
>>>> impl TryFrom<u8> for Mode {
>>>> ...
>>>> }
>>>>
>>>> impl From<Mode> for u32 {
>>>> ...
>>>> }
>>>>
>>>> Change this so the `From<Mode>` now needs to be implemented for `u8`,
>>>> i.e. the primitive type of the field. This is more consistent, and will
>>>> become a requirement once we start using the TryFrom/Into derive macros
>>>> to implement these automatically.
>>>>
>>>> Reported-by: Edwin Peer <epeer@nvidia.com>
>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>
>>> As these are incremental improvements, could you please rebase on top of the v6
>>> bitfield series so it does not conflict?
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6
>>
>> On second thought, I could just carry this patch on top of my v6 series and avoid too much conflict.
>>
>> So if it is ok with you, please only carry the last 2 patches of this series whenever applying.
>>
>> For this patch:
>> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
>>
>> I will review the other two patches as well. Thanks.
>
> The idea is for this patch to go *before* your series, to avoid the
> asymmetry in the From/Into implementions of bitfields. We could also put
> it after, but it would become larger as a result and I think it can be
> merge soon after -rc1 is tagged anyway.
I forgot to mention that this patch is not really part of the series ;
it's just here to allow it to apply on top of drm-rust-next should
anyone want to try it.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 2/3] rust: kernel: add bounded integer types
2025-10-09 21:33 ` Joel Fernandes
@ 2025-10-10 8:49 ` Alexandre Courbot
2025-10-10 15:23 ` Joel Fernandes
0 siblings, 1 reply; 26+ messages in thread
From: Alexandre Courbot @ 2025-10-10 8:49 UTC (permalink / raw)
To: Joel Fernandes
Cc: Danilo Krummrich, Yury Norov, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux, Nouveau
On Fri Oct 10, 2025 at 6:33 AM JST, Joel Fernandes wrote:
> Hi Alex,
>
> Great effort, thanks. I replied with few comments below. Since the patch is
> large, it would be great if could be possibly split. Maybe the From
> primitives deserve a separate patch.
I'm all for smaller patches when it makes reviewing easier, but in this
case all it would achieve is making the second patch append code right
after the next. :) Is there a benefit in doing so?
>
> On Thu, Oct 09, 2025 at 09:37:09PM +0900, Alexandre Courbot wrote:
>> Add the BoundedInt type, which restricts the number of bits allowed to
>> be used in a given integer value. This is useful to carry guarantees
>> when setting bitfields.
>>
>> Alongside this type, many `From` and `TryFrom` implementations are
>> provided to reduce friction when using with regular integer types. Proxy
>> implementations of common integer traits are also provided.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> rust/kernel/lib.rs | 1 +
>> rust/kernel/num.rs | 499 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 500 insertions(+)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index fcffc3988a90..21c1f452ee6a 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -101,6 +101,7 @@
>> pub mod mm;
>> #[cfg(CONFIG_NET)]
>> pub mod net;
>> +pub mod num;
>> pub mod of;
>> #[cfg(CONFIG_PM_OPP)]
>> pub mod opp;
>> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
>> new file mode 100644
>> index 000000000000..b2aad95ce51c
>> --- /dev/null
>> +++ b/rust/kernel/num.rs
>> @@ -0,0 +1,499 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Numerical types for the kernel.
>> +
>> +use kernel::prelude::*;
>> +
>> +/// Integer type for which only the bits `0..NUM_BITS` are valid.
>> +///
>> +/// # Invariants
>> +///
>> +/// Stored values are represented with at most `NUM_BITS` bits.
>> +#[repr(transparent)]
>> +#[derive(Clone, Copy, Debug, Default, Hash)]
>> +pub struct BoundedInt<T, const NUM_BITS: u32>(T);
>> +
>> +/// Returns `true` if `$value` can be represented with at most `$NUM_BITS` on `$type`.
>> +macro_rules! is_in_bounds {
>> + ($value:expr, $type:ty, $num_bits:expr) => {{
>> + let v = $value;
>> + v & <$type as Boundable<NUM_BITS>>::MASK == v
>> + }};
>> +}
>> +
>> +/// Trait for primitive integer types that can be used with `BoundedInt`.
>> +pub trait Boundable<const NUM_BITS: u32>
>> +where
>> + Self: Sized + Copy + core::ops::BitAnd<Output = Self> + core::cmp::PartialEq,
>> + Self: TryInto<u8> + TryInto<u16> + TryInto<u32> + TryInto<u64>,
>> +{
>> + /// Mask of the valid bits for this type.
>> + const MASK: Self;
>> +
>> + /// Returns `true` if `value` can be represented with at most `NUM_BITS`.
>> + ///
>> + /// TODO: post-RFC: replace this with a left-shift followed by right-shift operation. This will
>> + /// allow us to handle signed values as well.
>> + fn is_in_bounds(value: Self) -> bool {
>> + is_in_bounds!(value, Self, NUM_BITS)
>> + }
>> +}
>> +
>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u8 {
>> + const MASK: u8 = crate::bits::genmask_u8(0..=(NUM_BITS - 1));
>> +}
>> +
>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u16 {
>> + const MASK: u16 = crate::bits::genmask_u16(0..=(NUM_BITS - 1));
>> +}
>> +
>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u32 {
>> + const MASK: u32 = crate::bits::genmask_u32(0..=(NUM_BITS - 1));
>> +}
>> +
>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u64 {
>> + const MASK: u64 = crate::bits::genmask_u64(0..=(NUM_BITS - 1));
>> +}
>> +
>> +impl<T, const NUM_BITS: u32> BoundedInt<T, NUM_BITS>
>> +where
>> + T: Boundable<NUM_BITS>,
>> +{
>> + /// Checks that `value` is valid for this type at compile-time and build a new value.
>> + ///
>> + /// This relies on [`build_assert!`] to perform validation at compile-time. If `value` cannot
>> + /// be inferred to be in bounds at compile-time, use the fallible [`Self::try_new`] instead.
>> + ///
>> + /// When possible, use one of the `new_const` methods instead of this method as it statically
>> + /// validates `value` instead of relying on the compiler's optimizations.
>
> This sounds like, users might use the less-optimal API first with the same
> build_assert issues we had with the IO accessors, since new() sounds very obvious.
> How about the following naming?
>
> new::<VALUE>() // Primary constructor for constants using const generics.
> try_new(value) // Keep as-is for fallible runtime
> new_from_expr(value) // For compile-time validated runtime values
>
> If new::<VALUE>() does not work for the user, the compiler will fail.
>
> Or, new_from_expr() could be from_value(), Ok with either naming or a better name.
Agreed, the preferred method should appear first. IIRC Alice also made a
similar suggestion about v1 during the DRM sync, sorry for not picking
it up.
>
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::BoundedInt;
>> + ///
>> + /// # fn some_number() -> u32 { 0xffffffff }
>> + ///
>> + /// assert_eq!(BoundedInt::<u8, 1>::new(1).get(), 1);
>> + /// assert_eq!(BoundedInt::<u16, 8>::new(0xff).get(), 0xff);
>> + ///
>> + /// // Triggers a build error as `0x1ff` doesn't fit into 8 bits.
>> + /// // assert_eq!(BoundedInt::<u32, 8>::new(0x1ff).get(), 0x1ff);
>> + ///
>> + /// let v: u32 = some_number();
>> + /// // Triggers a build error as `v` cannot be asserted to fit within 4 bits...
>> + /// // let _ = BoundedInt::<u32, 4>::new(v);
>> + /// // ... but this works as the compiler can assert the range from the mask.
>> + /// let _ = BoundedInt::<u32, 4>::new(v & 0xf);
>> + /// ```
>> + pub fn new(value: T) -> Self {
>> + crate::build_assert!(
>> + T::is_in_bounds(value),
>> + "Provided parameter is larger than maximal supported value"
>> + );
>> +
>> + Self(value)
>> + }
>> +
>> + /// Attempts to convert `value` into a value bounded by `NUM_BITS`.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::BoundedInt;
>> + ///
>> + /// assert_eq!(BoundedInt::<u8, 1>::try_new(1).map(|v| v.get()), Ok(1));
>> + /// assert_eq!(BoundedInt::<u16, 8>::try_new(0xff).map(|v| v.get()), Ok(0xff));
>> + ///
>> + /// // `0x1ff` doesn't fit into 8 bits.
>> + /// assert_eq!(BoundedInt::<u32, 8>::try_new(0x1ff), Err(EOVERFLOW));
>> + /// ```
>> + pub fn try_new(value: T) -> Result<Self> {
>> + if !T::is_in_bounds(value) {
>> + Err(EOVERFLOW)
>> + } else {
>> + Ok(Self(value))
>> + }
>> + }
>> +
>> + /// Returns the contained value as a primitive type.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::BoundedInt;
>> + ///
>> + /// let v = BoundedInt::<u32, 4>::new_const::<7>();
>> + /// assert_eq!(v.get(), 7u32);
>> + /// ```
>> + pub fn get(self) -> T {
>> + if !T::is_in_bounds(self.0) {
>> + // SAFETY: Per the invariants, `self.0` cannot have bits set outside of `MASK`, so
>> + // this block will
>> + // never be reached.
>> + unsafe { core::hint::unreachable_unchecked() }
>> + }
>
> Does this if block help the compiler generate better code? I wonder if code
> gen could be checked to confirm the rationale.
Benno suggested that it would on a different patch:
https://lore.kernel.org/rust-for-linux/DBL1ZGZCSJF3.29HNS9BSN89C6@kernel.org/
OTOH as shown in patch 3/3, this doesn't exempt us from handling
impossible values when using this in a match expression...
>
>> + self.0
>> + }
>> +
>> + /// Increase the number of bits usable for `self`.
>> + ///
>> + /// This operation cannot fail.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::BoundedInt;
>> + ///
>> + /// let v = BoundedInt::<u32, 4>::new_const::<7>();
>> + /// let larger_v = v.enlarge::<12>();
>> + /// // The contained values are equal even though `larger_v` has a bigger capacity.
>> + /// assert_eq!(larger_v, v);
>> + /// ```
>> + pub const fn enlarge<const NEW_NUM_BITS: u32>(self) -> BoundedInt<T, NEW_NUM_BITS>
>> + where
>> + T: Boundable<NEW_NUM_BITS>,
>> + T: Copy,
>
> Boundable already implies copy so T: Copy is redundant.
Thanks. I need to do a thorough review of all the contraints as I've
changed them quite a bit as the implementation matured.
>
>> + {
>> + build_assert!(NEW_NUM_BITS >= NUM_BITS);
>> +
>> + // INVARIANT: the value did fit within `NUM_BITS`, so it will all the more fit within
>> + // `NEW_NUM_BITS` which is larger.
>> + BoundedInt(self.0)
>> + }
>> +
>> + /// Shrink the number of bits usable for `self`.
>> + ///
>> + /// Returns `EOVERFLOW` if the value of `self` cannot be represented within `NEW_NUM_BITS`.
>> + ///
>> + /// # Examples
>> + ///
>> + /// ```
>> + /// use kernel::num::BoundedInt;
>> + ///
>> + /// let v = BoundedInt::<u32, 12>::new_const::<7>();
>> + /// let smaller_v = v.shrink::<4>()?;
>> + /// // The contained values are equal even though `smaller_v` has a smaller capacity.
>> + /// assert_eq!(smaller_v, v);
>> + ///
>> + /// # Ok::<(), Error>(())
>> + /// ```
>> + pub fn shrink<const NEW_NUM_BITS: u32>(self) -> Result<BoundedInt<T, NEW_NUM_BITS>>
>> + where
>> + T: Boundable<NEW_NUM_BITS>,
>> + T: Copy,
>
> Here too.
>
> [...]
>> +impl_const_new!(u8 u16 u32 u64);
>> +
>> +/// Declares a new `$trait` and implements it for all bounded types represented using `$num_bits`.
>> +///
>> +/// This is used to declare properties as traits that we can use for later implementations.
>> +macro_rules! impl_size_rule {
>> + ($trait:ident, $($num_bits:literal)*) => {
>> + trait $trait {}
>> +
>> + $(
>> + impl<T> $trait for BoundedInt<T, $num_bits> where T: Boundable<$num_bits> {}
>> + )*
>> + };
>> +}
>> +
>> +// Bounds that are larger than a `u64`.
>> +impl_size_rule!(LargerThanU64, 64);
>> +
>> +// Bounds that are larger than a `u32`.
>> +impl_size_rule!(LargerThanU32,
>> + 32 33 34 35 36 37 38 39
>
> If num_bits == 32 (number of bits), how could BoundedInt<T, 32> a
> LargerThanU32? It should be AtleastU32 or something.
>
> Or the above list should start from 33. Only a >= 33-bit wide integer can be
> LargerThanU32.
The name is a bit ambiguous indeed. An accurate one would be
`LargerOrEqualThanU32`, but `AtLeastU32` should also work.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-09 16:40 ` Yury Norov
2025-10-09 17:18 ` Danilo Krummrich
2025-10-09 18:29 ` Joel Fernandes
@ 2025-10-10 9:19 ` Alexandre Courbot
2025-10-10 17:20 ` Yury Norov
2 siblings, 1 reply; 26+ messages in thread
From: Alexandre Courbot @ 2025-10-10 9:19 UTC (permalink / raw)
To: Yury Norov
Cc: Danilo Krummrich, Joel Fernandes, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux
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@gmail.com/
>> [2] https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
>>
>> Signed-off-by: Alexandre Courbot <acourbot@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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation
2025-10-10 8:26 ` Alexandre Courbot
@ 2025-10-10 14:59 ` Joel Fernandes
0 siblings, 0 replies; 26+ messages in thread
From: Joel Fernandes @ 2025-10-10 14:59 UTC (permalink / raw)
To: Alexandre Courbot, Danilo Krummrich, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross
Cc: nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org,
rust-for-linux@vger.kernel.org, Edwin Peer, Nouveau
On 10/10/2025 4:26 AM, Alexandre Courbot wrote:
> On Fri Oct 10, 2025 at 5:24 PM JST, Alexandre Courbot wrote:
>> On Fri Oct 10, 2025 at 12:41 AM JST, Joel Fernandes wrote:
>>>
>>>
>>>> On Oct 9, 2025, at 11:17 AM, Joel Fernandes <joelagnelf@nvidia.com> wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>>> On 10/9/2025 8:37 AM, Alexandre Courbot wrote:
>>>>> The getter method of a field works with the field type, but its setter
>>>>> expects the type of the register. This leads to an asymmetry in the
>>>>> From/Into implementations required for a field with a dedicated type.
>>>>> For instance, a field declared as
>>>>>
>>>>> pub struct ControlReg(u32) {
>>>>> 3:0 mode as u8 ?=> Mode;
>>>>> ...
>>>>> }
>>>>>
>>>>> currently requires the following implementations:
>>>>>
>>>>> impl TryFrom<u8> for Mode {
>>>>> ...
>>>>> }
>>>>>
>>>>> impl From<Mode> for u32 {
>>>>> ...
>>>>> }
>>>>>
>>>>> Change this so the `From<Mode>` now needs to be implemented for `u8`,
>>>>> i.e. the primitive type of the field. This is more consistent, and will
>>>>> become a requirement once we start using the TryFrom/Into derive macros
>>>>> to implement these automatically.
>>>>>
>>>>> Reported-by: Edwin Peer <epeer@nvidia.com>
>>>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>>>
>>>> As these are incremental improvements, could you please rebase on top of the v6
>>>> bitfield series so it does not conflict?
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/jfern/linux.git/log/?h=nova.dev.bitfield.submitted.v6
>>>
>>> On second thought, I could just carry this patch on top of my v6 series and avoid too much conflict.
>>>
>>> So if it is ok with you, please only carry the last 2 patches of this series whenever applying.
>>>
>>> For this patch:
>>> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
>>>
>>> I will review the other two patches as well. Thanks.
>>
>> The idea is for this patch to go *before* your series, to avoid the
>> asymmetry in the From/Into implementions of bitfields. We could also put
>> it after, but it would become larger as a result and I think it can be
>> merge soon after -rc1 is tagged anyway.
>
> I forgot to mention that this patch is not really part of the series ;
> it's just here to allow it to apply on top of drm-rust-next should
> anyone want to try it.
These are incremental improvements, I don't see the point in making the
improvements before bitfield extraction within Nova. Very least, we can apply
the bitfield extraction within nova and then make the improvements (within
nova), since the extraction work happened well before (and it was a non-trivial
amount of work), before any of these improvements. Let us work together to make
sure we don't have to throw away old work due to new patches please. :)
thanks,
- Joel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 2/3] rust: kernel: add bounded integer types
2025-10-10 8:49 ` Alexandre Courbot
@ 2025-10-10 15:23 ` Joel Fernandes
0 siblings, 0 replies; 26+ messages in thread
From: Joel Fernandes @ 2025-10-10 15:23 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Danilo Krummrich, Yury Norov, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux, Nouveau
On 10/10/2025 4:49 AM, Alexandre Courbot wrote:
> On Fri Oct 10, 2025 at 6:33 AM JST, Joel Fernandes wrote:
>> Hi Alex,
>>
>> Great effort, thanks. I replied with few comments below. Since the patch is
>> large, it would be great if could be possibly split. Maybe the From
>> primitives deserve a separate patch.
>
> I'm all for smaller patches when it makes reviewing easier, but in this
> case all it would achieve is making the second patch append code right
> after the next. :) Is there a benefit in doing so?
I think there is a benefit, because reviewers don't have to scroll through huge
emails :). Plus separate commit messages would be added in too, to reason about
new code. There's other possible logical splits too, was just giving example but
I am Ok with it if you still want to keep it singular. :)
>>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u16 {
>>> + const MASK: u16 = crate::bits::genmask_u16(0..=(NUM_BITS - 1));
>>> +}
>>> +
>>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u32 {
>>> + const MASK: u32 = crate::bits::genmask_u32(0..=(NUM_BITS - 1));
>>> +}
>>> +
>>> +impl<const NUM_BITS: u32> Boundable<NUM_BITS> for u64 {
>>> + const MASK: u64 = crate::bits::genmask_u64(0..=(NUM_BITS - 1));
>>> +}
>>> +
>>> +impl<T, const NUM_BITS: u32> BoundedInt<T, NUM_BITS>
>>> +where
>>> + T: Boundable<NUM_BITS>,
>>> +{
>>> + /// Checks that `value` is valid for this type at compile-time and build a new value.
>>> + ///
>>> + /// This relies on [`build_assert!`] to perform validation at compile-time. If `value` cannot
>>> + /// be inferred to be in bounds at compile-time, use the fallible [`Self::try_new`] instead.
>>> + ///
>>> + /// When possible, use one of the `new_const` methods instead of this method as it statically
>>> + /// validates `value` instead of relying on the compiler's optimizations.
>>
>> This sounds like, users might use the less-optimal API first with the same
>> build_assert issues we had with the IO accessors, since new() sounds very obvious.
>> How about the following naming?
>>
>> new::<VALUE>() // Primary constructor for constants using const generics.
>> try_new(value) // Keep as-is for fallible runtime
>> new_from_expr(value) // For compile-time validated runtime values
>>
>> If new::<VALUE>() does not work for the user, the compiler will fail.
>>
>> Or, new_from_expr() could be from_value(), Ok with either naming or a better name.
>
> Agreed, the preferred method should appear first. IIRC Alice also made a
> similar suggestion about v1 during the DRM sync, sorry for not picking
> it up.
Cool, sounds good.
[..]>>> + /// Returns the contained value as a primitive type.
>>> + ///
>>> + /// # Examples
>>> + ///
>>> + /// ```
>>> + /// use kernel::num::BoundedInt;
>>> + ///
>>> + /// let v = BoundedInt::<u32, 4>::new_const::<7>();
>>> + /// assert_eq!(v.get(), 7u32);
>>> + /// ```
>>> + pub fn get(self) -> T {
>>> + if !T::is_in_bounds(self.0) {
>>> + // SAFETY: Per the invariants, `self.0` cannot have bits set outside of `MASK`, so
>>> + // this block will
>>> + // never be reached.
>>> + unsafe { core::hint::unreachable_unchecked() }
>>> + }
>>
>> Does this if block help the compiler generate better code? I wonder if code
>> gen could be checked to confirm the rationale.
>
> Benno suggested that it would on a different patch:
>
> https://lore.kernel.org/rust-for-linux/DBL1ZGZCSJF3.29HNS9BSN89C6@kernel.org/
>
> OTOH as shown in patch 3/3, this doesn't exempt us from handling
> impossible values when using this in a match expression...
Interesting, TIL.
>>> + self.0
>>> + }
>>> +
>>> + /// Increase the number of bits usable for `self`.
>>> + ///
>>> + /// This operation cannot fail.
>>> + ///
>>> + /// # Examples
>>> + ///
>>> + /// ```
>>> + /// use kernel::num::BoundedInt;
>>> + ///
>>> + /// let v = BoundedInt::<u32, 4>::new_const::<7>();
>>> + /// let larger_v = v.enlarge::<12>();
>>> + /// // The contained values are equal even though `larger_v` has a bigger capacity.
>>> + /// assert_eq!(larger_v, v);
>>> + /// ```
>>> + pub const fn enlarge<const NEW_NUM_BITS: u32>(self) -> BoundedInt<T, NEW_NUM_BITS>
>>> + where
>>> + T: Boundable<NEW_NUM_BITS>,
>>> + T: Copy,
>>
>> Boundable already implies copy so T: Copy is redundant.
>
> Thanks. I need to do a thorough review of all the contraints as I've
> changed them quite a bit as the implementation matured.
Cool. :)
>> [...]
>>> +impl_const_new!(u8 u16 u32 u64);
>>> +
>>> +/// Declares a new `$trait` and implements it for all bounded types represented using `$num_bits`.
>>> +///
>>> +/// This is used to declare properties as traits that we can use for later implementations.
>>> +macro_rules! impl_size_rule {
>>> + ($trait:ident, $($num_bits:literal)*) => {
>>> + trait $trait {}
>>> +
>>> + $(
>>> + impl<T> $trait for BoundedInt<T, $num_bits> where T: Boundable<$num_bits> {}
>>> + )*
>>> + };
>>> +}
>>> +
>>> +// Bounds that are larger than a `u64`.
>>> +impl_size_rule!(LargerThanU64, 64);
>>> +
>>> +// Bounds that are larger than a `u32`.
>>> +impl_size_rule!(LargerThanU32,
>>> + 32 33 34 35 36 37 38 39
>>
>> If num_bits == 32 (number of bits), how could BoundedInt<T, 32> a
>> LargerThanU32? It should be AtleastU32 or something.
>>
>> Or the above list should start from 33. Only a >= 33-bit wide integer can be
>> LargerThanU32.
>
> The name is a bit ambiguous indeed. An accurate one would be
> `LargerOrEqualThanU32`, but `AtLeastU32` should also work.
Sure, I prefer AtLeastU32 since it is shorter :)
thanks,
- Joel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-10 9:19 ` Alexandre Courbot
@ 2025-10-10 17:20 ` Yury Norov
2025-10-10 17:58 ` Danilo Krummrich
2025-10-13 13:58 ` Joel Fernandes
0 siblings, 2 replies; 26+ messages in thread
From: Yury Norov @ 2025-10-10 17:20 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Danilo Krummrich, Joel Fernandes, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux
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@gmail.com/
> >> [2] https://lore.kernel.org/rust-for-linux/20251003154748.1687160-1-joelagnelf@nvidia.com/
> >>
> >> Signed-off-by: Alexandre Courbot <acourbot@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?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-10 17:20 ` Yury Norov
@ 2025-10-10 17:58 ` Danilo Krummrich
2025-10-13 13:58 ` Joel Fernandes
1 sibling, 0 replies; 26+ messages in thread
From: Danilo Krummrich @ 2025-10-10 17:58 UTC (permalink / raw)
To: Yury Norov
Cc: Alexandre Courbot, Joel Fernandes, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux
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.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 2/3] rust: kernel: add bounded integer types
2025-10-09 12:37 ` [PATCH RFC v2 2/3] rust: kernel: add bounded integer types Alexandre Courbot
2025-10-09 21:33 ` Joel Fernandes
@ 2025-10-11 10:33 ` Alice Ryhl
1 sibling, 0 replies; 26+ messages in thread
From: Alice Ryhl @ 2025-10-11 10:33 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Danilo Krummrich, Joel Fernandes, Yury Norov, Jesung Yang,
Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, nouveau, linux-kernel, rust-for-linux
On Thu, Oct 09, 2025 at 09:37:09PM +0900, Alexandre Courbot wrote:
> Add the BoundedInt type, which restricts the number of bits allowed to
> be used in a given integer value. This is useful to carry guarantees
> when setting bitfields.
>
> Alongside this type, many `From` and `TryFrom` implementations are
> provided to reduce friction when using with regular integer types. Proxy
> implementations of common integer traits are also provided.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> + pub const fn enlarge<const NEW_NUM_BITS: u32>(self) -> BoundedInt<T, NEW_NUM_BITS>
> + where
> + T: Boundable<NEW_NUM_BITS>,
> + T: Copy,
> + {
> + build_assert!(NEW_NUM_BITS >= NUM_BITS);
This assertion can be evaluated in const context.
const {
assert!(NEW_NUM_BITS >= NUM_BITS);
}
Alice
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt
2025-10-10 17:20 ` Yury Norov
2025-10-10 17:58 ` Danilo Krummrich
@ 2025-10-13 13:58 ` Joel Fernandes
1 sibling, 0 replies; 26+ messages in thread
From: Joel Fernandes @ 2025-10-13 13:58 UTC (permalink / raw)
To: Yury Norov, Alexandre Courbot
Cc: Danilo Krummrich, Jesung Yang, Miguel Ojeda, Alex Gaynor,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau, linux-kernel,
rust-for-linux
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@nvidia.com/
Or did you mean something else?
thanks,
- Joel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation
2025-10-09 12:37 ` [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation Alexandre Courbot
2025-10-09 15:17 ` Joel Fernandes
2025-10-09 20:10 ` Edwin Peer
@ 2025-10-16 14:51 ` Joel Fernandes
2 siblings, 0 replies; 26+ messages in thread
From: Joel Fernandes @ 2025-10-16 14:51 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Danilo Krummrich, Yury Norov, Jesung Yang, Miguel Ojeda,
Alex Gaynor, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, nouveau,
linux-kernel, rust-for-linux, Edwin Peer
On Thu, Oct 09, 2025 at 09:37:08PM +0900, Alexandre Courbot wrote:
> The getter method of a field works with the field type, but its setter
> expects the type of the register. This leads to an asymmetry in the
> From/Into implementations required for a field with a dedicated type.
> For instance, a field declared as
>
> pub struct ControlReg(u32) {
> 3:0 mode as u8 ?=> Mode;
> ...
> }
>
> currently requires the following implementations:
>
> impl TryFrom<u8> for Mode {
> ...
> }
>
> impl From<Mode> for u32 {
> ...
> }
>
> Change this so the `From<Mode>` now needs to be implemented for `u8`,
> i.e. the primitive type of the field. This is more consistent, and will
> become a requirement once we start using the TryFrom/Into derive macros
> to implement these automatically.
>
> Reported-by: Edwin Peer <epeer@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
thanks,
- Joel
> ---
> drivers/gpu/nova-core/falcon.rs | 38 +++++++++++++++++++++++++-----------
> drivers/gpu/nova-core/regs/macros.rs | 10 +++++-----
> 2 files changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 37e6298195e4..3f505b870601 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -22,11 +22,11 @@
> pub(crate) mod sec2;
>
> // TODO[FPRI]: Replace with `ToPrimitive`.
> -macro_rules! impl_from_enum_to_u32 {
> +macro_rules! impl_from_enum_to_u8 {
> ($enum_type:ty) => {
> - impl From<$enum_type> for u32 {
> + impl From<$enum_type> for u8 {
> fn from(value: $enum_type) -> Self {
> - value as u32
> + value as u8
> }
> }
> };
> @@ -46,7 +46,7 @@ pub(crate) enum FalconCoreRev {
> Rev6 = 6,
> Rev7 = 7,
> }
> -impl_from_enum_to_u32!(FalconCoreRev);
> +impl_from_enum_to_u8!(FalconCoreRev);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconCoreRev {
> @@ -81,7 +81,7 @@ pub(crate) enum FalconCoreRevSubversion {
> Subversion2 = 2,
> Subversion3 = 3,
> }
> -impl_from_enum_to_u32!(FalconCoreRevSubversion);
> +impl_from_enum_to_u8!(FalconCoreRevSubversion);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconCoreRevSubversion {
> @@ -125,7 +125,7 @@ pub(crate) enum FalconSecurityModel {
> /// Also known as High-Secure, Privilege Level 3 or PL3.
> Heavy = 3,
> }
> -impl_from_enum_to_u32!(FalconSecurityModel);
> +impl_from_enum_to_u8!(FalconSecurityModel);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconSecurityModel {
> @@ -157,7 +157,7 @@ pub(crate) enum FalconModSelAlgo {
> #[default]
> Rsa3k = 1,
> }
> -impl_from_enum_to_u32!(FalconModSelAlgo);
> +impl_from_enum_to_u8!(FalconModSelAlgo);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconModSelAlgo {
> @@ -179,7 +179,7 @@ pub(crate) enum DmaTrfCmdSize {
> #[default]
> Size256B = 0x6,
> }
> -impl_from_enum_to_u32!(DmaTrfCmdSize);
> +impl_from_enum_to_u8!(DmaTrfCmdSize);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for DmaTrfCmdSize {
> @@ -202,7 +202,6 @@ pub(crate) enum PeregrineCoreSelect {
> /// RISC-V core is active.
> Riscv = 1,
> }
> -impl_from_enum_to_u32!(PeregrineCoreSelect);
>
> impl From<bool> for PeregrineCoreSelect {
> fn from(value: bool) -> Self {
> @@ -213,6 +212,15 @@ fn from(value: bool) -> Self {
> }
> }
>
> +impl From<PeregrineCoreSelect> for bool {
> + fn from(value: PeregrineCoreSelect) -> Self {
> + match value {
> + PeregrineCoreSelect::Falcon => false,
> + PeregrineCoreSelect::Riscv => true,
> + }
> + }
> +}
> +
> /// Different types of memory present in a falcon core.
> #[derive(Debug, Clone, Copy, PartialEq, Eq)]
> pub(crate) enum FalconMem {
> @@ -236,7 +244,7 @@ pub(crate) enum FalconFbifTarget {
> /// Non-coherent system memory (System DRAM).
> NoncoherentSysmem = 2,
> }
> -impl_from_enum_to_u32!(FalconFbifTarget);
> +impl_from_enum_to_u8!(FalconFbifTarget);
>
> // TODO[FPRI]: replace with `FromPrimitive`.
> impl TryFrom<u8> for FalconFbifTarget {
> @@ -263,7 +271,6 @@ pub(crate) enum FalconFbifMemType {
> /// Physical memory addresses.
> Physical = 1,
> }
> -impl_from_enum_to_u32!(FalconFbifMemType);
>
> /// Conversion from a single-bit register field.
> impl From<bool> for FalconFbifMemType {
> @@ -275,6 +282,15 @@ fn from(value: bool) -> Self {
> }
> }
>
> +impl From<FalconFbifMemType> for bool {
> + fn from(value: FalconFbifMemType) -> Self {
> + match value {
> + FalconFbifMemType::Virtual => false,
> + FalconFbifMemType::Physical => true,
> + }
> + }
> +}
> +
> /// Type used to represent the `PFALCON` registers address base for a given falcon engine.
> pub(crate) struct PFalconBase(());
>
> diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
> index 754c14ee7f40..73811a115762 100644
> --- a/drivers/gpu/nova-core/regs/macros.rs
> +++ b/drivers/gpu/nova-core/regs/macros.rs
> @@ -482,7 +482,7 @@ impl $name {
> register!(
> @leaf_accessor $name $hi:$lo $field
> { |f| <$into_type>::from(if f != 0 { true } else { false }) }
> - $into_type => $into_type $(, $comment)?;
> + bool $into_type => $into_type $(, $comment)?;
> );
> };
>
> @@ -499,7 +499,7 @@ impl $name {
> $(, $comment:literal)?;
> ) => {
> register!(@leaf_accessor $name $hi:$lo $field
> - { |f| <$try_into_type>::try_from(f as $type) } $try_into_type =>
> + { |f| <$try_into_type>::try_from(f as $type) } $type $try_into_type =>
> ::core::result::Result<
> $try_into_type,
> <$try_into_type as ::core::convert::TryFrom<$type>>::Error
> @@ -513,7 +513,7 @@ impl $name {
> $(, $comment:literal)?;
> ) => {
> register!(@leaf_accessor $name $hi:$lo $field
> - { |f| <$into_type>::from(f as $type) } $into_type => $into_type $(, $comment)?;);
> + { |f| <$into_type>::from(f as $type) } $type $into_type => $into_type $(, $comment)?;);
> };
>
> // Shortcut for non-boolean fields defined without the `=>` or `?=>` syntax.
> @@ -527,7 +527,7 @@ impl $name {
> // Generates the accessor methods for a single field.
> (
> @leaf_accessor $name:ident $hi:tt:$lo:tt $field:ident
> - { $process:expr } $to_type:ty => $res_type:ty $(, $comment:literal)?;
> + { $process:expr } $prim_type:tt $to_type:ty => $res_type:ty $(, $comment:literal)?;
> ) => {
> ::kernel::macros::paste!(
> const [<$field:upper _RANGE>]: ::core::ops::RangeInclusive<u8> = $lo..=$hi;
> @@ -559,7 +559,7 @@ pub(crate) fn $field(self) -> $res_type {
> pub(crate) fn [<set_ $field>](mut self, value: $to_type) -> Self {
> const MASK: u32 = $name::[<$field:upper _MASK>];
> const SHIFT: u32 = $name::[<$field:upper _SHIFT>];
> - let value = (u32::from(value) << SHIFT) & MASK;
> + let value = (u32::from($prim_type::from(value)) << SHIFT) & MASK;
> self.0 = (self.0 & !MASK) | value;
>
> self
>
> --
> 2.51.0
>
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2025-10-16 14:51 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-09 12:37 [RFC PATCH v2 0/3] rust: bounded integer types and use in register macro Alexandre Courbot
2025-10-09 12:37 ` [PATCH RFC v2 1/3] gpu: nova-core: register: use field type for Into implementation Alexandre Courbot
2025-10-09 15:17 ` Joel Fernandes
2025-10-09 15:41 ` Joel Fernandes
2025-10-10 8:24 ` Alexandre Courbot
2025-10-10 8:26 ` Alexandre Courbot
2025-10-10 14:59 ` Joel Fernandes
2025-10-09 20:10 ` Edwin Peer
2025-10-16 14:51 ` Joel Fernandes
2025-10-09 12:37 ` [PATCH RFC v2 2/3] rust: kernel: add bounded integer types Alexandre Courbot
2025-10-09 21:33 ` Joel Fernandes
2025-10-10 8:49 ` Alexandre Courbot
2025-10-10 15:23 ` Joel Fernandes
2025-10-11 10:33 ` Alice Ryhl
2025-10-09 12:37 ` [PATCH RFC v2 3/3] gpu: nova-core: use BoundedInt Alexandre Courbot
2025-10-09 16:40 ` Yury Norov
2025-10-09 17:18 ` Danilo Krummrich
2025-10-09 18:28 ` Yury Norov
2025-10-09 18:37 ` Joel Fernandes
2025-10-09 19:19 ` Miguel Ojeda
2025-10-09 19:34 ` Danilo Krummrich
2025-10-09 18:29 ` Joel Fernandes
2025-10-10 9:19 ` Alexandre Courbot
2025-10-10 17:20 ` Yury Norov
2025-10-10 17:58 ` Danilo Krummrich
2025-10-13 13:58 ` Joel Fernandes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).