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

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:

quoted

On Thu Aug 27, 2026 at 11:12 PM JST, Eliot Courtney wrote:

quoted

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