Re: [PATCH v2 6/8] gpu: nova-core: add NVKV typed encoding

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

On Fri Sep 11, 2026 at 8:18 PM JST, Alexandre Courbot wrote:
> On Fri Sep 11, 2026 at 2:28 PM JST, Eliot Courtney wrote:
>> On Fri Sep 11, 2026 at 2:17 PM JST, Alexandre Courbot wrote:
>>> On Thu Sep 10, 2026 at 5:10 PM JST, Alexandre Courbot wrote:
>>>> On Thu Aug 27, 2026 at 11:12 PM JST, Eliot Courtney wrote:
>>>>> For struct-like GMCAPI messages encoding field by field manually is
>>>>> noisy. Add some type machinery and a macro to automate encoding of
>>>>> struct-like messages. The `Encodeable` trait can be implemented by any
>>>>> type to say that it can be encoded into an NVKV `Encoder`. Add a simple
>>>>> `nvkv_encode!` macro that works on structs and encodes each field in
>>>>> order. Provide some base types, such as `Key` which statically
>>>>> associates a NVKV key with some value, to avoid having to make a lot of
>>>>> newtypes and implement `Encodeable` on them.
>>>>>
>>>>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>>>>> ---
>>>>>  drivers/gpu/nova-core/gsp/nvkv.rs        |  49 ++++++++-
>>>>>  drivers/gpu/nova-core/gsp/nvkv/encode.rs | 178 +++++++++++++++++++++++++++++++
>>>>>  2 files changed, 226 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs b/drivers/gpu/nova-core/gsp/nvkv.rs
>>>>> index cbeee7f376b6..10dcbb9e602c 100644
>>>>> --- a/drivers/gpu/nova-core/gsp/nvkv.rs
>>>>> +++ b/drivers/gpu/nova-core/gsp/nvkv.rs
>>>>> @@ -10,8 +10,13 @@
>>>>>  //! naturally maps to storing a &str with the GPU name.
>>>>>  
>>>>>  #![expect(unused_imports)]
>>>>> +#![cfg_attr(not(CONFIG_KUNIT), expect(unused_macros))]
>>>>>  
>>>>> -use core::ops::Deref;
>>>>> +use core::marker::PhantomData;
>>>>> +use core::ops::{
>>>>> +    Deref,
>>>>> +    DerefMut, //
>>>>> +};
>>>>>  
>>>>>  use kernel::{
>>>>>      alloc::{
>>>>> @@ -92,6 +97,48 @@ fn deref(&self) -> &Self::Target {
>>>>>  /// The index of an NVKV value.
>>>>>  pub(crate) type Index = Bounded<u64, 12>;
>>>>>  
>>>>> +/// A static association between an NVKV key `KEY_ID` and the storage of its value.
>>>>> +///
>>>>> +/// Use with the encoder or decoder macros `nvkv_encode!` and `nvkv_decode!` to let them know how to
>>>>> +/// map the value `Key<T, KEY_ID, As>` to/from encoded data. For brevity, `As` inserts an additional
>>>>> +/// conversion (`From`) to avoid having to implement [`Encodable`] for many types. For example,
>>>>> +/// enums that are easily convertible to a u32 can have `As = u32` and rely on the existing encoding
>>>>> +/// for u32.
>>>>> +#[repr(transparent)]
>>>>> +pub(crate) struct Key<T, const KEY_ID: KeyId, As = T>(pub(crate) T, PhantomData<As>);
>>>>
>>>> Does the `T` need to be `pub(crate)`? The series builds fine with it
>>>> being private.
>>>>
>>>> Also the relationship between `Key` and `IndexedKey` is a bit unclear
>>>> with the current type layout. IIUC `Key` is basically a specialization
>>>> of `IndexedKey` with an index of 0. And yet `Key` is declared in the
>>>> root `nvkv` module while `IndexedKey` is in the `encode` submodule...
>>>> I'm also wondering whether it would make sense to make the relationship
>>>> completely explicit by making `Key` a newtype embedding a `IndexedKey`
>>>> with the invariant that the index is `0`, but not sure about that one so
>>>> your call.
>>>
>>> Ah, I guess that's because `IndexedKey` is local to `encoder`. In this
>>> case keeping it there does indeed make sense. After complaining about
>>> the visibility of other declarations, I should have noticed that one
>>> too. :P
>>>
>>> What worries me more is the fact that no index larger than `0` is ever
>>> created. This looks more and more like a bug to me.
>>
>> We currently don't send anything that wants an index other than zero.
>> But, in the future we will. The way I did this so far is by having
>> 'IndexedKey' which represents the most general thing that can properly
>> encode anything accepted by the NVKV format. Since it's cumbersome to
>> and rare to use, I added Key as a wrapper on top. It would be possible
>> to hard code 0 in the index and only have `Key` but then we'd just have
>> to change it later. Since nothing wants to set a non-zero index
>> currently, it's private to `encoder`.
>
> But surely we should be able to test the non-zero path, even if we don't
> have any real-life user at the moment, right? As this is part of the
> NVKV spec, we should ensure this is correct.
>
> If `IndexedKey` is supposed to move down one module when it gains a
> user, I think we should place it in its expected destination from the
> get-go.

You're right, let me add a test and think about moving it.