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 <[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.
You're right, let me add a test and think about moving it.
