Re: [PATCH v2 8/8] gpu: nova-core: add NVKV GSP_INIT schemas

["Eliot Courtney" <ecourtney@nvidia.com>]

On Mon Sep 14, 2026 at 9:12 PM JST, Alexandre Courbot wrote:
> On Mon Sep 14, 2026 at 2:42 PM JST, Eliot Courtney wrote:
>> On Mon Sep 14, 2026 at 1:11 PM JST, Alexandre Courbot wrote:
>>> On Thu Aug 27, 2026 at 11:12 PM JST, Eliot Courtney wrote:
>>> <...>
>>>> +impl RegKey {
>>>> +    // Define the Key IDs read/written by GSP.
>>>> +    const REGKEY_NAME_KEY: KeyId = 0x3070;
>>>> +    const REGKEY_VALUE_U32_KEY: KeyId = 0x3071;
>>>> +}
>>>> +
>>>> +impl Encodable for KVVec<RegKey> {
>>>> +    fn encode(&self, encoder: &mut Encoder) -> Result {
>>>> +        for regkey in self {
>>>> +            regkey.encode(encoder)?;
>>>> +        }
>>>> +        Ok(())
>>>
>>> Maybe this is just me misunderstanding, but how are the keys
>>> sequentially sent here? Because I don't see any mention of an index, and
>>> `Key::encode` hardcodes `Index::new::<0>()`, so how are these supposed
>>> to be decoded into an array? The `gsp_init_request` test below only adds
>>> one key to its `regkeys`, can we add at least another one to see what
>>> happens and verify that the received content decodes as expected on top
>>> of checking its length?
>>
>> You are not misunderstanding, it's just a bit odd. This is because
>> sequential regkeys are all sent using an index of 0, according to the
>> protocol. I can add a second regkey into the test to demonstrate this.
>> We currently don't and won't soon have a need to decode this kind of
>> repeated index 0 encoding scheme. We have `Accumualted` now, but that
>> relies on the index changing to know when the previous value has been
>> completely sent.
>>
>> To test that the content decodes we'd need to add either a test only
>> Schema for it, or add a schema that isn't (and won't be soon be) used.
>> Alternatively, we can test against the encoded byte content directly.
>> Which do you prefer?
>
> Whichever you think is adequate. :) As long as we test things our
> current code allows us to express.
>
> Although since the NVKV protocol is specified, I think we'll want to
> confirm the fitness of our implementation for the entirety of it, even
> the parts we are not yet using in practice. In that case I suppose it is
> acceptable to have a Schema that is only used in the tests for the sake
> of completeness (even if that means having a temporary
> `#[expect(dead_code]`, as long as the reason is documented).

Alright, I will add a test for this which adds a test only Schema for
regkey. In doing so though, I noticed that with the current design to
write a Schema for regkeys, it's fairly easy to end up with `Target =
(KVVec<KVVec<u8>>, KVVec<u32>)` which is not very nice.

I thought it might be nice to allow the possibility to borrow from the
encoded stream. Then we can at least have `Target = (KVVec<&'d [u8]>,
KVVec<u32>);` (Or use an ArrayVec and avoid forced heap allocation
entirely, but regkey is one of the locations where no clear max # of
them exists AFAICT).

The next version of this series will include this, basically means
adding an (optional) lifetime to the schema that represents the lifetime
of the data. To avoid infecting every struct with this lifetime if it's
just doing copying, I've split Schema into a Schema trait and a
Visit<'data> trait (otherwise you can't refer to Schema::Target without
being able to name a specific lifetime). The Visit<'data> trait is
generic over the 'data lifetime, whereas the Schema trait is not. If you
want 'data lifetime semantics you can add a 'data lifetime to your
struct that implements Schema and plumb that lifetime to the
Visit<'data> implementation. If not, you just let Visit<'data> work for
all lifetimes. The macro handles this transparently. It's not a big
change in LOC but it lets us avoid a copy.

What I'm envisioning is that we do one copy out of the command queue to
provide the encoded stream as a single slice. Then, Schemas can take
borrows of that if they want to. Then, when we convert to the `Target`
output type it can be copied into the final type we want. So that leaves
us with two copies, which is in line with variable length handling for
the r570 Cmdq code (GspSequence, ContinuationRecords).

I considered an additional approach where we deserialize directly from
the command queue ring buffer memory, but that means we need to handle
two slices all the way up into the Schema implementations, so it's a
larger change. Anyway, it's not incompatible with the approach in this
patch series or the approach I mention above, and would let us get down
to one copy for variable length data. We could consider doing that later
if we really want.

I was also looking at if maybe we can do the bidirectional type thing
without overcomplicating the macro, because it ended up being quite
gross to add round-trip-like tests like you suggested, since you need to
basically duplicate the struct. If you have the macro generate the
Schema struct and the storage struct, plus the encode impl, you can
avoid this. I tried a serde-like syntax which actually ends up being
doable with just macro_rules! altho is more complicated. That ends up
looking like this:

```
nvkv! {
    struct GspInitRequest<'a> {
        #[nvkv(key = GspInitRequest::PCI_DEVICE_ID_KEY)]
        pci_device_id: u32,

        #[nvkv(key = GspInitRequest::PCI_SUBDEVICE_ID_KEY)]
        pci_sub_device_id: u32,

...

        #[nvkv(kind = Repeated<RegKeySchema<'a>, { RegKey::REGKEY_NAME_KEY }>)]
        regkeys: KVVec<RegKey<'a>>,

        #[nvkv(kind = Optional<VfInfoSchema>)]
        vf_info: Option<VfInfo>,
    }
}
```

Another version avoids the macro complication, although it's less
obvious what the field types are on inspection (they are declared by the
macro using Schema::Target):

```
nvkv! {
    struct GspInitRequest<'a> {
        pci_device_id: Required<u32, { GspInitRequest::PCI_DEVICE_ID_KEY }>,
        pci_sub_device_id: Required<u32, { GspInitRequest::PCI_SUBDEVICE_ID_KEY }>,
...
        regkeys: Repeated<RegKeySchema<'a>, { RegKey::REGKEY_NAME_KEY }>,
        vf_info: Optional<VfInfoSchema>,
    }
}
```

Anyway sorry for the wall of text, any preferences on the above? Or we
leave it as is with the separate nvkv_encode/nvkv_decode macros with
some fairly verbose/gross round-trip tests.