On Thu Aug 27, 2026 at 11:12 PM JST, Eliot Courtney wrote:
> Similar to the typed encoding layer, add some decoding type machinery.
> Add a simple macro `nvkv_decode!` which implements `Schema` for a struct
> by composing visit calls to each member. Add some common `Schema` kinds,
> such as `Array` which collects an array value into a fixed maximum size
> array, and `Required` which fails a decode if the value is not sent.
>
> Signed-off-by: Eliot Courtney <[email protected]>
> ---
> drivers/gpu/nova-core/gsp/nvkv.rs | 12 +-
> drivers/gpu/nova-core/gsp/nvkv/decode.rs | 480
> ++++++++++++++++++++++++++++++-
> 2 files changed, 488 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs
> b/drivers/gpu/nova-core/gsp/nvkv.rs
> index 10dcbb9e602c..7d58ca91cbc3 100644
> --- a/drivers/gpu/nova-core/gsp/nvkv.rs
> +++ b/drivers/gpu/nova-core/gsp/nvkv.rs
> @@ -9,7 +9,7 @@
> //! function calls will map to some struct - for example,
> f(GPU_NAME_STRING_KEY, 0, b"some gpu")
> //! naturally maps to storing a &str with the GPU name.
>
> -#![expect(unused_imports)]
> +#![cfg_attr(not(CONFIG_KUNIT), expect(unused_imports))]
I am getting a build error on this patch:
error: unused import: `nvkv_encode`
--> ../drivers/gpu/nova-core/gsp/nvkv/encode.rs:65:16
|
65 | pub(crate) use nvkv_encode;
| ^^^^^^^^^^^
|
= note: `-D unused-imports` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(unused_imports)]`
error: unused import: `nvkv_decode`
--> ../drivers/gpu/nova-core/gsp/nvkv/decode.rs:104:16
|
104 | pub(crate) use nvkv_decode;
| ^^^^^^^^^^^
error: aborting due to 2 previous errors
> #![cfg_attr(not(CONFIG_KUNIT), expect(unused_macros))]
>
> use core::marker::PhantomData;
> @@ -21,7 +21,8 @@
> use kernel::{
> alloc::{
> allocator::KVmalloc,
> - Allocator, //
> + Allocator,
> + ArrayVec, //
> },
> bitfield,
> num::Bounded,
> @@ -139,6 +140,13 @@ fn default() -> Self {
> }
> }
>
> +/// A schema field for an array value under the NVKV key `KEY_ID`.
> +#[derive(Default)]
> +#[repr(transparent)]
> +pub(crate) struct Array<T: Default + Copy, const N: usize, const KEY_ID:
> KeyId> {
> + vec: ArrayVec<T, N>,
> +}
Why is this not defined under `decoder` if it is only used there?
> +
> bitfield! {
> /// The op word that starts each NVKV operation.
> struct Op(u64) {
> diff --git a/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> index ceb97e73e100..7f5310857764 100644
> --- a/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> +++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> @@ -3,16 +3,356 @@
>
> #![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
>
> -use kernel::prelude::*;
> +use core::convert::Infallible;
> +use core::marker::PhantomData;
> +
> +use kernel::{
> + alloc::ArrayVec,
> + prelude::*, //
> +};
> +use pin_init::init_array_from_fn;
>
> use crate::gsp::nvkv::{
> + Array,
> Index,
> + Key,
> KeyId,
> Op,
> Opcode, //
> };
> use crate::num;
>
> +/// Defines a schema struct together with its [`Schema`] implementation that
> decodes into `$target`.
> +///
> +/// Each member of the struct should implement `Schema`. For every (key,
> index, value) triple
> +/// decoded from the NVKV stream, the generated parent `Schema`
> implementation will call each member
> +/// in declaration order with that triple. If a member consumes that triple,
> it will stop there.
> +/// Otherwise it will keep going until all members are tried.
> +///
> +/// The schema struct holds the state required by the schema implementation
> to do the decode. It's
> +/// recommended to use one of the existing Schema kinds (`Required`,
> `Accumulated`, `Key`, `Array`,
> +/// `Indexed`) for each member.
> +///
> +/// # Examples
> +///
> +/// ```
> +/// nvkv_decode! {
> +/// struct RequestSchema => Request {
> +/// id: Required<u32, 0x0001>,
> +/// name: Array<u8, 64, 0x0002>,
> +/// }
> +/// }
> +/// ```
> +macro_rules! nvkv_decode {
> + (
> + $(#[$attr:meta])*
> + $vis:vis struct $name:ident => $target: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::Schema for $name {
> + type Target = $target;
> +
> + fn init() -> impl ::kernel::prelude::Init<Self> {
> + ::pin_init::init!(Self {
> + $( $field <- <$ty as $crate::gsp::nvkv::Schema>::init(),
> )*
> + })
> + }
> +
> + fn visit(
> + &mut self,
> + key: $crate::gsp::nvkv::KeyId,
> + index: $crate::gsp::nvkv::Index,
> + value: $crate::gsp::nvkv::DecoderValue<'_>,
> + ) -> ::kernel::error::Result<bool> {
> + Ok(false
> + $( || $crate::gsp::nvkv::Schema::visit(&mut self.$field,
> key, index, value)? )*)
Mmm looks like this is going to be `O(n)` with `n` being the number of
fields?
This is ok for a first implementation but eventually I hope we can
switch to a more efficient dispatch.
> + }
> +
> + #[inline(always)]
In this patch as well these should probably be just `#[inline]`.
> + fn finish(
> + &mut self,
> + ) -> impl ::kernel::prelude::Init<Self::Target,
> ::kernel::error::Error> + '_ {
> + let Self { $($field,)* } = self;
> + ::kernel::try_init!(Self::Target {
> + $( $field <- $crate::gsp::nvkv::Schema::finish($field),
> )*
> + }? ::kernel::error::Error)
> + }
> + }
> +
> + impl ::core::default::Default for $name {
> + fn default() -> Self {
> + $crate::gsp::nvkv::assert_schema_size_reasonable::<Self>();
> + Self {
> + $( $field: ::core::default::Default::default(), )*
> + }
> + }
> + }
> + };
> +}
> +pub(crate) use nvkv_decode;
> +
> +/// Asserts that a schema built by value is small enough.
> +pub(crate) fn assert_schema_size_reasonable<S>() {
> + // Clippy triggers this even if the enclosing function is never called,
> so skip if clippy is on.
> + const_assert!(
> + cfg!(clippy) || size_of::<S>() <= 1024,
> + "construct large schemas in place with `Schema::init` instead of
> `Default`"
> + );
> +}
> +
> +impl<T: for<'a> TryFrom<DecoderValue<'a>, Error = Error> + Default, const
> KEY_ID: KeyId> Schema
> + for Key<T, KEY_ID>
> +{
> + type Target = T;
> +
> + #[inline(always)]
> + fn visit<'a>(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'a>) -> Result<bool> {
> + if key != KEY_ID {
> + Ok(false)
> + } else if index != Index::new::<0>() {
> + // Single values being set must be at index 0.
> + Err(EINVAL)
> + } else {
> + // Overwrite and take the latest value here.
> + self.0 = value.try_into()?;
> + Ok(true)
> + }
> + }
> +
> + #[inline(always)]
> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> + Ok(core::mem::take(&mut self.0))
> + }
> +}
> +
> +impl<T: for<'a> TryFrom<DecoderValue<'a>, Error = Error>, const KEY_ID:
> KeyId> Schema
> + for Key<Option<T>, KEY_ID>
> +{
> + type Target = Option<T>;
> +
> + #[inline(always)]
> + fn visit<'a>(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'a>) -> Result<bool> {
> + if key != KEY_ID {
> + Ok(false)
> + } else if index != Index::new::<0>() {
> + // Single values being set must be at index 0.
> + Err(EINVAL)
> + } else {
> + // Overwrite and take the latest value here.
> + self.0 = Some(value.try_into()?);
> + Ok(true)
> + }
> + }
> +
> + #[inline(always)]
> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> + Ok(self.0.take())
> + }
> +}
> +
> +impl<T: Default + Copy, const N: usize, const KEY_ID: KeyId> Schema for
> Array<T, N, KEY_ID>
> +where
> + for<'a> &'a [T]: TryFrom<DecoderValue<'a>, Error = Error>,
> +{
> + type Target = ArrayVec<T, N>;
> +
> + fn init() -> impl Init<Self> {
> + init!(Self {
> + vec <- ArrayVec::init_with::<Infallible>(|_| Ok(())),
> + })
> + }
> +
> + fn visit<'a>(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'a>) -> Result<bool> {
> + if key != KEY_ID {
> + return Ok(false);
> + }
> + // Require to be at index 0
> + if index != Index::new::<0>() {
> + return Err(EINVAL);
> + }
> + // Reject oversized and take the latest value.
> + self.vec.clear();
> + self.vec.extend_from_slice(value.try_into()?)?;
> + Ok(true)
> + }
> +
> + #[inline(always)]
> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> + ArrayVec::init_with(move |dst| {
> + dst.extend_from_slice(&self.vec)?;
> + self.vec.clear();
> + Ok(())
> + })
> + }
> +}
> +
> +/// A schema field for a key that must be present.
> +///
> +/// `finish` fails with `EINVAL` if no value arrived for the key.
> +#[repr(transparent)]
> +pub(crate) struct Required<T, const KEY_ID: KeyId>(Key<Option<T>, KEY_ID>);
> +
> +impl<T: for<'a> TryFrom<DecoderValue<'a>, Error = Error>, const KEY_ID:
> KeyId> Schema
> + for Required<T, KEY_ID>
> +{
> + type Target = T;
> +
> + #[inline(always)]
> + fn visit<'a>(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'a>) -> Result<bool> {
> + self.0.visit(key, index, value)
> + }
> +
> + #[inline(always)]
> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> + (self.0).0.take().ok_or(EINVAL)
> + }
> +}
> +
> +impl<T, const KEY_ID: KeyId> Default for Required<T, KEY_ID> {
> + fn default() -> Self {
> + Self(None.into())
> + }
> +}
> +
> +/// Expects objects specified sequentially with index starting from zero.
> +pub(crate) struct Accumulated<S: Schema> {
> + current_index: Index,
> + current: S,
> + current_started: bool,
> + next: S,
> + accumulated: KVVec<S::Target>,
> +}
> +
> +impl<S: Schema + Default> Accumulated<S> {
> + /// Creates an empty accumulator.
> + pub(crate) fn new() -> Self {
> + Self {
> + current_index: Index::new::<0>(),
> + current: S::default(),
> + current_started: false,
> + next: S::default(),
> + accumulated: KVVec::new(),
Do we want to call `assert_schema_size_reasonable` somewhere here as
well? Also, should this be a `Default` implementation?
> + }
> + }
> +
> + fn take_vec(&mut self) -> Result<KVVec<S::Target>> {
> + if self.current_started {
> + self.accumulated
> + .try_push_init(self.current.finish(), GFP_KERNEL)?;
> + self.current_started = false;
> + }
> + self.current_index = Index::new::<0>();
> + Ok(core::mem::take(&mut self.accumulated))
> + }
This seems to be only called by `finish`, let's inline it there?
> +}
> +
> +impl<S: Schema + Default> Schema for Accumulated<S> {
If this ok that this doesn't provide an `init` implementation? Because
the default one returns a value on the stack, which IIUC can grow rather
consequently for an `Accumulated`?
> + type Target = KVVec<S::Target>;
> +
> + fn visit<'a>(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'a>) -> Result<bool> {
> + if index != self.current_index {
> + if !self.next.visit(key, Index::new::<0>(), value)? {
> + // Unrelated key to us.
> + return Ok(false);
> + }
> +
> + // Require that objects at index k have all their keys sent
> before the k + 1 th object
> + // can be completed. Require that objects are sent contiguously
> in order from index 0.
> + if !self.current_started || index != self.current_index + 1 {
> + return Err(EINVAL);
> + }
> +
> + // The current value must be finished. Push it and swap in
> `next`.
> + self.accumulated
> + .try_push_init(self.current.finish(), GFP_KERNEL)?;
> + core::mem::swap(&mut self.current, &mut self.next);
> + self.current_started = true;
> + self.current_index = index;
> + Ok(true)
I don't quite understand how this method works, notably how
`current_index` evolves. This might require more documentation on
`Accumulated` itself.
> + } else {
> + let consumed = self.current.visit(key, Index::new::<0>(),
> value)?;
> + self.current_started |= consumed;
> + Ok(consumed)
> + }
> + }
> +
> + #[inline(always)]
> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> + self.take_vec()
> + }
> +}
> +
> +impl<S: Schema + Default> Default for Accumulated<S> {
> + fn default() -> Self {
> + Self::new()
> + }
> +}
> +
> +/// A schema field that scatters indexed values into an array of `N` slots.
> +#[repr(transparent)]
> +pub(crate) struct Indexed<T, const N: usize, const KEY_ID: KeyId, As =
> T>([T; N], PhantomData<As>);
Can we elaborate a bit on what `As` is supposed to be? Not only on this
site, but generally speaking. I have a hard time coming with a
consistent definition, so a comment would help the reader forge their
understanding.
> +
> +/// Copies `elems`, converted to `T`, into `slots` at `start`.
> +///
> +/// Fails with `EINVAL` if the window does not fit in `slots`.
> +fn scatter_window<T: From<As>, As: Copy>(slots: &mut [T], start: usize,
> elems: &[As]) -> Result {
> + let end = start.checked_add(elems.len()).ok_or(EINVAL)?;
> + // Reject indices outside of the declared array size.
> + let dst = slots.get_mut(start..end).ok_or(EINVAL)?;
> + for (d, &e) in dst.iter_mut().zip(elems) {
> + *d = T::from(e);
> + }
> + Ok(())
> +}
> +
> +impl<T, const N: usize, const KEY_ID: KeyId, As> Schema for Indexed<T, N,
> KEY_ID, As>
Same question as `Accumulated` about the lack of an `init` method -
maybe we can use `init_array_from_fn` to avoid a stack copy.
Actually that makes me think that maybe the default `Schema::init`
implementation is not such good an idea, because it makes us overlook
types where we should override it.
> +where
> + T: From<As> + Default,
> + As: Copy + for<'a> TryFrom<DecoderValue<'a>, Error = Error>,
> + for<'a> &'a [As]: TryFrom<DecoderValue<'a>, Error = Error>,
> +{
> + type Target = [T; N];
> +
> + fn visit<'a>(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'a>) -> Result<bool> {
> + if key != KEY_ID {
> + return Ok(false);
> + }
> + let start = index.cast::<usize>().get();
> + // Accept both scalar vs scattered array setting for flexibility.
> + match <&[As]>::try_from(value) {
> + Ok(elems) => scatter_window(&mut self.0, start, elems)?,
> + Err(_) => scatter_window(&mut self.0, start,
> &[As::try_from(value)?])?,
> + }
> + Ok(true)
> + }
> +
> + #[inline(always)]
> + fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
> + init_array_from_fn(|i| Ok::<_, Error>(core::mem::take(&mut
> self.0[i])))
> + }
> +}
> +
> +impl<T: Default + Copy, const N: usize, const KEY_ID: KeyId, As> Default
> + for Indexed<T, N, KEY_ID, As>
> +{
> + fn default() -> Self {
> + assert_schema_size_reasonable::<Self>();
> + Self([T::default(); N], PhantomData)
> + }
Mmm that could be a pretty large object. Where are these `default`
methods called? Do we want to leverage `init` instead?
<...>
> + // Tests that a schema too large for the stack decodes on the heap.
> + #[test]
> + fn decode_large_schema_on_heap() -> Result {
> + const BLOB_KEY: KeyId = 0x1400;
> + const BLOB_VALUE: &[u8] = &[0xab; 100];
> +
> + nvkv_decode! {
> + struct BigSchema => BigDecodeable {
> + blob: Array<u8, 2048, { BLOB_KEY }>,
> + }
> + }
> +
> + struct BigDecodeable {
> + blob: ArrayVec<u8, 2048>,
> + }
> +
> + let mut encoder = Encoder::new();
> + encoder.encode_array8(BLOB_KEY, Index::new::<0>(), BLOB_VALUE)?;
> + let serialized = encoder.finish();
> +
> + let mut schema = KBox::init(BigSchema::init(), GFP_KERNEL)?;
> + let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
> + let decoded = KBox::try_init(decoder.decode(&mut *schema)?,
> GFP_KERNEL)?;
> +
> + assert_eq!(*decoded.blob, *BLOB_VALUE);
> + Ok(())
> + }
Same as the encoder, it would be nice to exercise the error paths a bit
more in the tests.
