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

[Alexandre Courbot]

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