Re: [PATCH v2 15/19] gpu: nova-core: register: redesign relative registers

["Alexandre Courbot" <acourbot@nvidia.com>]

On Sat Jul 26, 2025 at 3:56 AM JST, Daniel Almeida wrote:
> Hi Alex,
>
>> On 18 Jul 2025, at 04:26, Alexandre Courbot <acourbot@nvidia.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@nvidia.com>
>
>
> I think it would be helpful to showcase a before/after in the commit message. IIUC, we'd go from:

Agreed, that would help understand the change better.

>
> /// 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>?

The difference between the two designs is that with the former one, the
code reading the register could pass any base it wanted (any number!),
whereas with the new one that base must come from an explicitly-defined
type (which implementation is controlled by the party defining the
register), which reduces the risk of typos and mixups. The type system
ensures that you cannot e.g. pass the base of a GPU to a CPU for
instance, whereas the previous approach had no such protection.

>
> 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.

If the range separating each instance is the same fixed number, then
this sounds like a good chance to use a register array with a stride of
`(1 << MMU_AS_SHIFT)`. But I suspect you figured that out. :)