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

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

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