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

["Alexandre Courbot" <acourbot@nvidia.com>]

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@nvidia.com>
> ---
>  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.

> +
> +impl<T, const KEY_ID: KeyId, As> From<T> for Key<T, KEY_ID, As> {
> +    fn from(value: T) -> Self {
> +        Self(value, PhantomData)
> +    }
> +}
> +
> +impl<'a, T, const KEY_ID: KeyId, As, const N: usize> From<&'a [T; N]> for Key<&'a [T], KEY_ID, As> {
> +    fn from(value: &'a [T; N]) -> Self {
> +        Self(&value[..], PhantomData)
> +    }
> +}
> +
> +impl<T, const KEY_ID: KeyId, As> Deref for Key<T, KEY_ID, As> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &Self::Target {
> +        &self.0
> +    }
> +}
> +
> +impl<T, const KEY_ID: KeyId, As> DerefMut for Key<T, KEY_ID, As> {
> +    fn deref_mut(&mut self) -> &mut Self::Target {
> +        &mut self.0
> +    }
> +}
> +
> +impl<T: Default, const KEY_ID: KeyId, As> Default for Key<T, KEY_ID, As> {
> +    fn default() -> Self {
> +        Self(T::default(), PhantomData)
> +    }
> +}
> +
>  bitfield! {
>      /// The op word that starts each NVKV operation.
>      struct Op(u64) {
> diff --git a/drivers/gpu/nova-core/gsp/nvkv/encode.rs b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
> index 6c1a9cbd90e8..0047be65e8a9 100644
> --- a/drivers/gpu/nova-core/gsp/nvkv/encode.rs
> +++ b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
> @@ -8,11 +8,153 @@
>  use super::{
>      EncodedStream,
>      Index,
> +    Key,
>      KeyId,
>      Op,
>      Opcode, //
>  };
>  
> +/// A type that can encode itself into an [`Encoder`].
> +pub(crate) trait Encodable {
> +    /// Encodes `self` into `encoder`.
> +    fn encode(&self, encoder: &mut Encoder) -> Result;
> +}
> +
> +/// Defines a struct together with its [`Encodable`] implementation.
> +///
> +/// The implementation encodes each field in declaration order. Each field type must implement
> +/// [`Encodable`], which is done already for types like `Key<T, KEY_ID>`.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// nvkv_encode! {
> +///     struct Request {
> +///         id: Key<u32, 0x0001>,
> +///         name: Key<&'static [u8], 0x0002>,
> +///     }
> +/// }
> +/// ```
> +macro_rules! nvkv_encode {
> +    (
> +        $(#[$attr:meta])*
> +        $vis:vis struct $name:ident {
> +            $(
> +                $(#[$field_attr:meta])*
> +                $field_vis:vis $field:ident : $ty:ty
> +            ),* $(,)?
> +        }
> +    ) => {
> +        $(#[$attr])*
> +        $vis struct $name {
> +            $(
> +                $(#[$field_attr])*
> +                $field_vis $field: $ty,
> +            )*
> +        }
> +
> +        impl $crate::gsp::nvkv::Encodable for $name {
> +            #[inline(always)]

The `#[inline(always)]` of this patch should probably just be `#[inline]`.

> +            fn encode(&self, encoder: &mut $crate::gsp::nvkv::Encoder) -> ::kernel::error::Result {
> +                $( $crate::gsp::nvkv::Encodable::encode(&self.$field, encoder)?; )*
> +                Ok(())
> +            }
> +        }
> +    };
> +}
> +pub(crate) use nvkv_encode;
> +
> +/// A value with a specific index that encodes under the NVKV key `KEY_ID`.
> +struct IndexedKey<T, const KEY_ID: KeyId> {
> +    index: Index,
> +    value: T,
> +}
> +
> +impl<T, const KEY_ID: KeyId> IndexedKey<T, KEY_ID> {
> +    /// Creates a key with the given index and value.
> +    pub(crate) fn new(index: Index, value: T) -> Self {

`pub(crate)` visibility on a private type method.

Also this constructor seems to only ever be called with an `index` of
`0` throughout the series? Does this mean we also have no test coverage
for indices larger than `0`?

> +        Self { index, value }
> +    }
> +}
> +
> +impl<const KEY_ID: KeyId> Encodable for IndexedKey<u32, KEY_ID> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        encoder.encode_u32(KEY_ID, self.index, self.value)
> +    }
> +}
> +
> +impl<const KEY_ID: KeyId> Encodable for IndexedKey<u64, KEY_ID> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        encoder.encode_u64(KEY_ID, self.index, self.value)
> +    }
> +}
> +
> +impl<const KEY_ID: KeyId> Encodable for IndexedKey<&[u8], KEY_ID> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        encoder.encode_array8(KEY_ID, self.index, self.value)
> +    }
> +}
> +
> +impl<const KEY_ID: KeyId> Encodable for IndexedKey<&[u32], KEY_ID> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        encoder.encode_array32(KEY_ID, self.index, self.value)
> +    }
> +}
> +
> +impl<const KEY_ID: KeyId> Encodable for IndexedKey<&[u64], KEY_ID> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        encoder.encode_array64(KEY_ID, self.index, self.value)
> +    }
> +}
> +
> +impl<const N: usize, const KEY_ID: KeyId> Encodable for IndexedKey<[u8; N], KEY_ID> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        encoder.encode_array8(KEY_ID, self.index, &self.value)
> +    }
> +}
> +
> +impl<const N: usize, const KEY_ID: KeyId> Encodable for IndexedKey<[u32; N], KEY_ID> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        encoder.encode_array32(KEY_ID, self.index, &self.value)
> +    }
> +}
> +
> +impl<const N: usize, const KEY_ID: KeyId> Encodable for IndexedKey<[u64; N], KEY_ID> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        encoder.encode_array64(KEY_ID, self.index, &self.value)
> +    }
> +}
> +
> +impl<T, const KEY_ID: KeyId, As> Encodable for Key<T, KEY_ID, As>
> +where
> +    IndexedKey<As, KEY_ID>: Encodable,
> +    As: From<T>,
> +    T: Copy,
> +{
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        IndexedKey::new(Index::new::<0>(), As::from(self.0)).encode(encoder)
> +    }
> +}
> +
> +impl<T: Encodable> Encodable for Option<T> {
> +    #[inline(always)]
> +    fn encode(&self, encoder: &mut Encoder) -> Result {
> +        if let Some(value) = self {
> +            value.encode(encoder)?;
> +        }
> +        Ok(())
> +    }
> +}
> +
>  /// An encoder for an NVKV stream.
>  pub(crate) struct Encoder {
>      stream: EncodedStream,
> @@ -207,4 +349,40 @@ fn encode_all_value_kinds() -> Result {
>  
>          Ok(())
>      }
> +
> +    // Tests that encoding via the `nvkv_encode!` macro works correctly.
> +    #[test]
> +    fn encode_typed_struct() -> Result {
> +        const U32_KEY: KeyId = 0x0001;
> +        const U64_KEY: KeyId = 0x0002;
> +        const NAME_KEY: KeyId = 0x0003;
> +        const FIXED_KEY: KeyId = 0x0004;
> +        const OPT_KEY: KeyId = 0x0005;
> +
> +        nvkv_encode! {
> +            struct TypedRequest {
> +                a: Key<u32, { U32_KEY }>,
> +                b: Key<u64, { U64_KEY }>,
> +                name: Key<&'static [u8], { NAME_KEY }>,
> +                fixed: Key<[u8; 4], { FIXED_KEY }>,
> +                opt: Option<Key<u32, { OPT_KEY }>>,

The `{` and `}` brackets are not needed here.

> +            }
> +        }
> +
> +        let request = TypedRequest {
> +            a: 0x89ab_cdef.into(),
> +            b: 0x0123_4567_89ab_cdef.into(),
> +            name: b"name\0".into(),
> +            fixed: [1u8, 2, 3, 4].into(),
> +            opt: None,
> +        };
> +
> +        let mut encoder = Encoder::new();
> +        request.encode(&mut encoder)?;
> +        let encoded = encoder.finish();
> +
> +        assert_eq!(encoded.len(), 7);

I guess we should also check the raw output here, otherwise there are
whole classes of bugs this test won't catch.