Message-Id: <F19649A8-3002-4BAC-8FBF-095CF67B3946@collabora.com>
Date: Fri, 25 Jul 2025 15:56:14 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Danilo Krummrich <dakr@...nel.org>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 Beata Michalska <beata.michalska@....com>,
 nouveau@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 15/19] gpu: nova-core: register: redesign relative
 registers

Hi Alex,

> On 18 Jul 2025, at 04:26, Alexandre Courbot <acourbot@...dia.com> wrote:
> 
> The relative registers are currently very unsafe to use: callers can
> specify any constant as the base address for access, meaning they can
> effectively interpret any I/O address as any relative register.
> 
> Ideally, valid base addresses for a family of registers should be
> explicitly defined in the code, and could only be used with the relevant
> registers
> 
> This patch changes the relative register declaration into this:
> 
>    register!(REGISTER_NAME @ BaseTrait[offset] ...
> 
> Where `BaseTrait` is the name of a ZST used as a parameter of the
> `RegisterBase<>` trait to define a trait unique to a class of register.
> This specialized trait is then implemented for every type that provides
> a valid base address, enabling said types to be passed as the base
> address provider for the register's I/O accessor methods.
> 
> This design thus makes it impossible to pass an unexpected base address
> to a relative register, and, since the valid bases are all known at
> compile-time, also guarantees that all I/O accesses are done within the
> valid bounds of the I/O range.
> 
> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>


I think it would be helpful to showcase a before/after in the commit message. IIUC, we'd go from:

/// Putting a `+` before the address of the register makes it relative to a base: the `read` and
/// `write` methods take a `base` argument that is added to the specified address before access:
///
/// ```no_run
/// register!(CPU_CTL @ +0x0000010, "CPU core control" {
///    0:0     start as bool, "Start the CPU core";
/// });


To:

/// ```no_run
/// // Type used to identify the base.
/// pub(crate) struct CpuCtlBase;
///
/// // ZST describing `CPU0`.
/// struct Cpu0;
/// impl RegisterBase<CpuCtlBase> for Cpu0 {
///     const BASE: usize = 0x100;
/// }
/// // Singleton of `CPU0` used to identify it.
/// const CPU0: Cpu0 = Cpu0;
///
/// // ZST describing `CPU1`.
/// struct Cpu1;
/// impl RegisterBase<CpuCtlBase> for Cpu1 {
///     const BASE: usize = 0x200;
/// }
/// // Singleton of `CPU1` used to identify it.
/// const CPU1: Cpu1 = Cpu1;

So you can still pass whatever base you want, the difference (in this
particular aspect) is whether it's specified in the macro itself, or as an
associated constant of RegisterBase<Foo>?

In any case, have you considered what happens when the number of "CPUs" in your
example grows larger? I can only speak for Tyr, where (IIUC), I'd have to
define 16 structs, each representing a single AS region, i.e.:

+pub(crate) const MMU_BASE: usize = 0x2400;
+pub(crate) const MMU_AS_SHIFT: usize = 6;
+
+const fn mmu_as(as_nr: usize) -> usize {
+ MMU_BASE + (as_nr << MMU_AS_SHIFT)
+
+pub(crate) struct AsRegister(usize);
+
+impl AsRegister {
+    fn new(as_nr: usize, offset: usize) -> Result<Self> {
+        if as_nr >= 32 {
+            Err(EINVAL)
+        } else {
+            Ok(AsRegister(mmu_as(as_nr) + offset))
+        }
+    }

It's still somewhat manageable, but I wonder if there are usecases out there
(in other drivers/devices) where this number will be even higher, which will
make this pattern impossible to implement.

Or maybe I misunderstood the usecase?

In any case, the patch itself looks fine to me.


[…]


— Daniel

Powered by blists - more mailing lists