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

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:

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.