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

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

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.