| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20250304-nova_timer-v2-3-8fb13f3f8cff@nvidia.com>
Date: Tue, 04 Mar 2025 22:53:59 +0900
From: Alexandre Courbot <acourbot@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>, David Airlie <airlied@...il.com>,
John Hubbard <jhubbard@...dia.com>, Ben Skeggs <bskeggs@...dia.com>,
Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
Trevor Gross <tmgross@...ch.edu>, Simona Vetter <simona@...ll.ch>
Cc: linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
nouveau@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
Alexandre Courbot <acourbot@...dia.com>
Subject: [PATCH RFC v2 3/5] gpu: nova-core: add register definition macro
Register data manipulation is one of the error-prone areas of a kernel
driver. It is particularly easy to mix addresses of registers, masks and
shifts of fields, and to proceed with invalid values.
This patch introduces the nv_reg!() macro, which creates a safe type
definition for a given register, along with field accessors and
value builder. The macro is designed to type the same field ranges as
the NVIDIA OpenRM project, to facilitate porting its register
definitions to Nova.
Here is for instance the definition of the Boot0 register:
nv_reg!(Boot0@...0000000, "Basic revision information about the GPU";
3:0 minor_rev as (u8), "minor revision of the chip";
7:4 major_rev as (u8), "major revision of the chip";
25:20 chipset try_into (Chipset), "chipset model"
);
This definition creates a Boot0 type that includes read() and write()
methods that will automatically use the correct register offset (0x0 in
this case).
Creating a type for each register lets us leverage the type system to
make sure register values don't get mix up.
It also allows us to create register-specific field extractor methods
(here minor_rev(), major_rev(), and chipset()) that present each field
in a convenient way and validate its data if relevant. The chipset()
accessor, in particular, uses the TryFrom<u32> implementation of Chipset
to build a Chipset instance and returns its associated error type if the
conversion has failed because of an invalid value.
The ending string at the end of each line is optional, and expands to
doc comments for the type itself, or each of the field accessors.
Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
---
drivers/gpu/nova-core/gpu.rs | 2 +-
drivers/gpu/nova-core/regs.rs | 195 ++++++++++++++++++++++++++++++++++--------
2 files changed, 158 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 7693a5df0dc11f208513dc043d8c99f85c902119..58b97c7f0b2ab1edacada8346b139f6336b68272 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -164,7 +164,7 @@ fn new(bar: &Devres<Bar0>) -> Result<Spec> {
let boot0 = regs::Boot0::read(&bar);
Ok(Self {
- chipset: boot0.chipset().try_into()?,
+ chipset: boot0.chipset()?,
revision: Revision::from_boot0(boot0),
})
}
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 50aefb150b0b1c9b73f07fca3b7a070885785485..a874cb2fa5bedee258a60e5c3b471f52e5f82469 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -1,55 +1,174 @@
// SPDX-License-Identifier: GPL-2.0
+use core::{fmt::Debug, marker::PhantomData, ops::Deref};
+
use crate::driver::Bar0;
+use crate::gpu::Chipset;
-// TODO
-//
-// Create register definitions via generic macros. See task "Generic register
-// abstraction" in Documentation/gpu/nova/core/todo.rst.
+pub(crate) struct Builder<T>(T, PhantomData<T>);
-const BOOT0_OFFSET: usize = 0x00000000;
+impl<T> From<T> for Builder<T> {
+ fn from(value: T) -> Self {
+ Builder(value, PhantomData)
+ }
+}
-// 3:0 - chipset minor revision
-const BOOT0_MINOR_REV_SHIFT: u8 = 0;
-const BOOT0_MINOR_REV_MASK: u32 = 0x0000000f;
+impl<T: Default> Default for Builder<T> {
+ fn default() -> Self {
+ Self(Default::default(), PhantomData)
+ }
+}
-// 7:4 - chipset major revision
-const BOOT0_MAJOR_REV_SHIFT: u8 = 4;
-const BOOT0_MAJOR_REV_MASK: u32 = 0x000000f0;
+impl<T> Deref for Builder<T> {
+ type Target = T;
-// 23:20 - chipset implementation Identifier (depends on architecture)
-const BOOT0_IMPL_SHIFT: u8 = 20;
-const BOOT0_IMPL_MASK: u32 = 0x00f00000;
+ fn deref(&self) -> &Self::Target {
+ &self.0
+ }
+}
-// 28:24 - chipset architecture identifier
-const BOOT0_ARCH_MASK: u32 = 0x1f000000;
+macro_rules! nv_reg_common {
+ ($name:ident $(, $type_comment:expr)?) => {
+ $(
+ #[doc=concat!($type_comment)]
+ )?
+ #[derive(Clone, Copy, Default)]
+ pub(crate) struct $name(u32);
-// 28:20 - chipset identifier (virtual register field combining BOOT0_IMPL and
-// BOOT0_ARCH)
-const BOOT0_CHIPSET_SHIFT: u8 = BOOT0_IMPL_SHIFT;
-const BOOT0_CHIPSET_MASK: u32 = BOOT0_IMPL_MASK | BOOT0_ARCH_MASK;
+ // TODO: should we display the raw hex value, then the value of all its fields?
+ impl Debug for $name {
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ f.debug_tuple(stringify!($name))
+ .field(&format_args!("0x{0:x}", &self.0))
+ .finish()
+ }
+ }
-#[derive(Copy, Clone)]
-pub(crate) struct Boot0(u32);
+ impl core::ops::BitOr for $name {
+ type Output = Self;
-impl Boot0 {
- #[inline]
- pub(crate) fn read(bar: &Bar0) -> Self {
- Self(bar.readl(BOOT0_OFFSET))
- }
+ fn bitor(self, rhs: Self) -> Self::Output {
+ Self(self.0 | rhs.0)
+ }
+ }
- #[inline]
- pub(crate) fn chipset(&self) -> u32 {
- (self.0 & BOOT0_CHIPSET_MASK) >> BOOT0_CHIPSET_SHIFT
- }
+ #[allow(dead_code)]
+ impl $name {
+ /// Returns a new builder for the register. Individual fields can be set by the methods
+ /// of the builder, and the current value obtained by dereferencing it.
+ #[inline]
+ pub(crate) fn new() -> Builder<Self> {
+ Default::default()
+ }
+ }
+ };
+}
- #[inline]
- pub(crate) fn minor_rev(&self) -> u8 {
- ((self.0 & BOOT0_MINOR_REV_MASK) >> BOOT0_MINOR_REV_SHIFT) as u8
- }
+macro_rules! nv_reg_field_accessor {
+ ($hi:tt:$lo:tt $field:ident $(as ($as_type:ty))? $(as_bit ($bit_type:ty))? $(into ($type:ty))? $(try_into ($try_type:ty))? $(, $comment:expr)?) => {
+ $(
+ #[doc=concat!("Returns the ", $comment)]
+ )?
+ #[inline]
+ pub(crate) fn $field(self) -> $( $as_type )? $( $bit_type )? $( $type )? $( core::result::Result<$try_type, <$try_type as TryFrom<u32>>::Error> )? {
+ const MASK: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
+ const SHIFT: u32 = MASK.trailing_zeros();
+ let field = (self.0 & MASK) >> SHIFT;
- #[inline]
- pub(crate) fn major_rev(&self) -> u8 {
- ((self.0 & BOOT0_MAJOR_REV_MASK) >> BOOT0_MAJOR_REV_SHIFT) as u8
+ $( field as $as_type )?
+ $(
+ // TODO: it would be nice to throw a compile-time error if $hi != $lo as this means we
+ // are considering more than one bit but returning a bool...
+ (if field != 0 { true } else { false }) as $bit_type
+ )?
+ $( <$type>::from(field) )?
+ $( <$try_type>::try_from(field) )?
+ }
}
}
+
+macro_rules! nv_reg_field_builder {
+ ($hi:tt:$lo:tt $field:ident $(as ($as_type:ty))? $(as_bit ($bit_type:ty))? $(into ($type:ty))? $(try_into ($try_type:ty))? $(, $comment:expr)?) => {
+ $(
+ #[doc=concat!("Sets the ", $comment)]
+ )?
+ #[inline]
+ pub(crate) fn $field(mut self, value: $( $as_type)? $( $bit_type )? $( $type )? $( $try_type)? ) -> Self {
+ const MASK: u32 = ((((1 << $hi) - 1) << 1) + 1) - ((1 << $lo) - 1);
+ const SHIFT: u32 = MASK.trailing_zeros();
+
+ let value = ((value as u32) << SHIFT) & MASK;
+ self.0.0 = self.0.0 | value;
+ self
+ }
+ };
+}
+
+macro_rules! nv_reg {
+ (
+ $name:ident@...fset:expr $(, $type_comment:expr)?;
+ $($hi:tt:$lo:tt $field:ident $(as ($as_type:ty))? $(as_bit ($bit_type:ty))? $(into ($type:ty))? $(try_into ($try_type:ty))? $(, $field_comment:expr)?);* $(;)?
+ ) => {
+ nv_reg_common!($name);
+
+ #[allow(dead_code)]
+ impl $name {
+ #[inline]
+ pub(crate) fn read(bar: &Bar0) -> Self {
+ Self(bar.readl($offset))
+ }
+
+ #[inline]
+ pub(crate) fn write(self, bar: &Bar0) {
+ bar.writel(self.0, $offset)
+ }
+
+ $(
+ nv_reg_field_accessor!($hi:$lo $field $(as ($as_type))? $(as_bit ($bit_type))? $(into ($type))? $(try_into ($try_type))? $(, $field_comment)?);
+ )*
+ }
+
+ #[allow(dead_code)]
+ impl Builder<$name> {
+ $(
+ nv_reg_field_builder!($hi:$lo $field $(as ($as_type))? $(as_bit ($bit_type))? $(into ($type))? $(try_into ($try_type))? $(, $field_comment)?);
+ )*
+ }
+ };
+ (
+ $name:ident@...ffset:expr $(, $type_comment:expr)?;
+ $($hi:tt:$lo:tt $field:ident $(as ($as_type:ty))? $(as_bit ($bit_type:ty))? $(into ($type:ty))? $(try_into ($try_type:ty))? $(, $field_comment:expr)?);* $(;)?
+ ) => {
+ nv_reg_common!($name);
+
+ #[allow(dead_code)]
+ impl $name {
+ #[inline]
+ pub(crate) fn read(bar: &Bar0, base: usize) -> Self {
+ Self(bar.readl(base + $offset))
+ }
+
+ #[inline]
+ pub(crate) fn write(self, bar: &Bar0, base: usize) {
+ bar.writel(self.0, base + $offset)
+ }
+
+ $(
+ nv_reg_field_accessor!($hi:$lo $field $(as ($as_type))? $(as_bit ($bit_type))? $(into ($type))? $(try_into ($try_type))? $(, $field_comment)?);
+ )*
+ }
+
+ #[allow(dead_code)]
+ impl Builder<$name> {
+ $(
+ nv_reg_field_builder!($hi:$lo $field $(as ($as_type))? $(as_bit ($bit_type))? $(into ($type))? $(try_into ($try_type))? $(, $field_comment)?);
+ )*
+ }
+ };
+}
+
+nv_reg!(Boot0@...0000000, "Basic revision information about the GPU";
+ 3:0 minor_rev as (u8), "minor revision of the chip";
+ 7:4 major_rev as (u8), "major revision of the chip";
+ 25:20 chipset try_into (Chipset), "chipset model"
+);
--
2.48.1
Powered by blists - more mailing lists