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 <[email protected]> >>>> --- >>>> 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.
