On Mon Sep 28, 2026 at 5:42 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 | 11 +-
> drivers/gpu/nova-core/gsp/nvkv/decode.rs | 622
> ++++++++++++++++++++++++++++++-
> 2 files changed, 628 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs
> b/drivers/gpu/nova-core/gsp/nvkv.rs
> index 7ac3a459a98b..5791df07a7fa 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))]
> #![cfg_attr(not(CONFIG_KUNIT), expect(unused_macros))]
>
> use core::{
> @@ -23,7 +23,8 @@
> use kernel::{
> alloc::{
> allocator::KVmalloc,
> - Allocator, //
> + Allocator,
> + ArrayVec, //
> },
> bitfield,
> num::Bounded,
> @@ -148,6 +149,12 @@ fn default() -> Self {
> }
> }
>
> +/// A schema field for an array value under the NVKV key `KEY_ID`.
> +#[repr(transparent)]
> +pub(crate) struct Array<T: Default + Copy, const N: usize, const KEY_ID:
> KeyId> {
> + vec: ArrayVec<T, N>,
> +}
I still don't see why this needs to be declared in this module when no
encoder element ever uses it, and the module reexports everything from
`decode` anyway. Can this be moved next to the other schema kinds?
> +
> 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 c4c24fe1108e..24dad31296cb 100644
> --- a/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> +++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> @@ -3,11 +3,22 @@
>
> #![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
>
> -use kernel::prelude::*;
> +use core::{
> + convert::Infallible,
> + marker::PhantomData, //
> +};
> +
> +use kernel::{
> + alloc::ArrayVec,
> + prelude::*, //
> +};
> +use pin_init::init_array_from_fn;
>
> use crate::{
> gsp::nvkv::{
> + Array,
> Index,
> + Key,
> KeyId,
> Op,
> Opcode, //
> @@ -15,6 +26,353 @@
> num, //
> };
>
> +/// Defines a schema struct together with its [`Schema`] and [`Visit`]
> implementations that decode
> +/// into `$target`.
> +///
> +/// Each member of the struct should implement [`Schema`] and [`Visit`]. For
> every (key, index,
> +/// value) triple decoded from the NVKV stream, the generated parent `Visit`
> implementation will
nit: let's put backticks around (key, index, value) and link to `Visit`
(and other types mentioned in this doccomment, and possibly elsewhere in
this patch).
<...>
> +/// 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>);
I just noticed that the schema field types are conflating several
concepts together. On the one hand, you have types associating a key to
some kind of storage (Key, Array, Indexed - let's call these leaf
types), and on the other what looks like modifiers on said leaf types
(Accumulated).
Which leaves `Required` somewhere in between, as it takes a key
parameter and can only be applied to value types, i.e. you currently
cannot have a `Required<Array<...>>`. But `Accumulated<Array<...>>` does
work IIUC.
So I think `Required` should work the same way as `Accumulated`, i.e.
just wrap it around the type you want to make required, instead of
switching the leaf type.
And for symmetry, we should also have an `Optional` wrapper type, so we
can also make fields optional even if they are not `Key`s instead of
having specific behavior for `Key<Option>` (which again is not
composable).
Which would give us 3 leaf types (Key, Array, Indexed) and 3 modifiers
(Required, Optional, Accumulated), with a non-wrapped leaf meaning
"default if unspecified" - which is easier to understand than the
current types where `Required` is a synonym for "a `Key` except it's
required".
This means `Required` would then be used like `Required<Key<...>>`,
which is a bit more verbose, but again nicer to read.
<...>
> /// A decoded NVKV value.
> #[derive(Copy, Clone, Debug, PartialEq, Eq)]
> pub(crate) enum DecoderValue<'a> {
> @@ -51,7 +409,16 @@ fn try_from(value: DecoderValue<'a>) -> Result<Self> {
> impl_try_from_decoder_value!(&'a [u32], Array32);
> impl_try_from_decoder_value!(&'a [u64], Array64);
>
> -/// A visitor that consumes decoded NVKV and produces a `Target`.
Why change this doccomment? Let's introduce its final form in patch 5.
> +/// Lets `Key<Option<T>, KEY_ID>` accept whatever `Key<T, KEY_ID>` accepts.
> +impl<'a, T: TryFrom<DecoderValue<'a>, Error = Error>>
> TryFrom<DecoderValue<'a>> for Option<T> {
> + type Error = Error;
> +
> + fn try_from(value: DecoderValue<'a>) -> Result<Self> {
> + T::try_from(value).map(Some)
> + }
> +}
IIUC this block could go away if we add an `Optional` wrapper suggested
above.
> +
> +/// The state of one NVKV decode operation which produces a target value
> `Target`.
> pub(crate) trait Schema {
> type Target;
>
> @@ -65,7 +432,12 @@ fn init() -> impl Init<Self>
>
> /// Returns an initializer that makes the decoded `Target`.
> ///
> - /// After the returned initializer runs, the schema should be empty
> again.
> + /// After the returned initializer runs successfully, the schema must be
> empty again. For
> + /// example, this is required by [`Accumulated`] which finishes one
> object and then decodes the
> + /// next one with the same schema. Implementations generated by
> `nvkv_decode!` meet this
> + /// requirement. If the initializer fails, the `Schema` can be in a
> valid but non-fresh state.
> + /// Taking `self` instead of `&mut self` would avoid this contract, but
> it forces a copy of the
> + /// schema onto the stack.
Let's not cite specific implementations in the doccomment of a trait. We
want to say that the schema *must* be empty (as opposed to "should")
from patch 5, and that should be clear enough and sufficient. I'd also
move the last sentence to patch 5, and drop the rest.
> fn finish(&mut self) -> impl Init<Self::Target, Error> + '_;
> }
>
> @@ -295,6 +667,133 @@ fn visit(&mut self, key: KeyId, index: Index, value:
> DecoderValue<'d>) -> Result
> Ok(())
> }
>
> + // Tests that decoding via the `nvkv_decode!` macro works correctly.
> + #[test]
> + fn decode_typed_struct() -> Result {
> + const SCALAR32_KEY: KeyId = 0x1234;
> + const SCALAR64_KEY: KeyId = 0x1235;
> + const ARRAY8_KEY: KeyId = 0x1236;
> + const ARRAY32_KEY: KeyId = 0x1237;
> + const ARRAY64_KEY: KeyId = 0x1238;
> + const OPT_PRESENT_KEY: KeyId = 0x1239;
> + const OPT_ABSENT_KEY: KeyId = 0x123a;
> + const X_KEY: KeyId = 0x0100;
> + const Y_KEY: KeyId = 0x0101;
> + const SLOT_KEY: KeyId = 0x0200;
> +
> + const SCALAR32_VALUE: u32 = 0x89ab_cdef;
> + const SCALAR64_VALUE: u64 = 0x0123_4567_89ab_cdef;
> + const ARRAY8_VALUE: &[u8] = &[0x12, 0x34, 0x56];
> + const ARRAY32_VALUE: &[u32] = &[0x0123_4567, 0x89ab_cdef];
> + const ARRAY64_VALUE: &[u64] = &[0x0123_4567_89ab_cdef,
> 0xfedc_ba98_7654_3210];
> + const OPT_PRESENT_VALUE: u32 = 0x55;
> +
> + nvkv_decode! {
> + struct PairSchema => Pair {
> + x: Required<u32, X_KEY>,
> + y: Required<u32, Y_KEY>,
> + }
> + }
> +
> + struct Pair {
> + x: u32,
> + y: u32,
> + }
> +
> + nvkv_decode! {
> + struct TestSchema => TestDecodeable {
> + scalar32: Required<u32, SCALAR32_KEY>,
> + scalar64: Required<u64, SCALAR64_KEY>,
> + array8: Array<u8, 64, ARRAY8_KEY>,
> + array32: Array<u32, 64, ARRAY32_KEY>,
> + array64: Array<u64, 32, ARRAY64_KEY>,
> + opt_present: Key<Option<u32>, OPT_PRESENT_KEY>,
> + opt_absent: Key<Option<u32>, OPT_ABSENT_KEY>,
> + pairs: Accumulated<PairSchema>,
> + slots: Indexed<u32, 4, SLOT_KEY>,
> + }
> + }
> +
> + struct TestDecodeable {
> + scalar32: u32,
> + scalar64: u64,
> + array8: ArrayVec<u8, 64>,
> + array32: ArrayVec<u32, 64>,
> + array64: ArrayVec<u64, 32>,
> + opt_present: Option<u32>,
> + opt_absent: Option<u32>,
> + pairs: KVVec<Pair>,
> + slots: [u32; 4],
> + }
> +
> + let index0 = Index::new::<0>();
> + let index1 = Index::new::<1>();
> + let index2 = Index::new::<2>();
> + let mut encoder = Encoder::new();
> + encoder.encode_u32(SCALAR32_KEY, index0, SCALAR32_VALUE)?;
> + encoder.encode_u64(SCALAR64_KEY, index0, SCALAR64_VALUE)?;
> + encoder.encode_array8(ARRAY8_KEY, index0, ARRAY8_VALUE)?;
> + encoder.encode_array32(ARRAY32_KEY, index0, ARRAY32_VALUE)?;
> + encoder.encode_array64(ARRAY64_KEY, index0, ARRAY64_VALUE)?;
> + encoder.encode_u32(OPT_PRESENT_KEY, index0, OPT_PRESENT_VALUE)?;
> + encoder.encode_u32(X_KEY, index0, 1)?;
> + encoder.encode_u32(Y_KEY, index0, 2)?;
> + encoder.encode_u32(SLOT_KEY, index1, 20)?;
> + encoder.encode_u32(X_KEY, index1, 3)?;
> + encoder.encode_u32(Y_KEY, index1, 4)?;
> + encoder.encode_u32(SLOT_KEY, index0, 10)?;
> + encoder.encode_array32(SLOT_KEY, index2, &[30, 40])?;
> + let serialized = encoder.finish();
> +
> + let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
> + let mut schema = KBox::init(TestSchema::init(), GFP_KERNEL)?;
> + let decoded = KBox::try_init(decoder.decode(&mut *schema)?,
> GFP_KERNEL)?;
> +
> + assert_eq!(decoded.scalar32, SCALAR32_VALUE);
> + assert_eq!(decoded.scalar64, SCALAR64_VALUE);
> + assert_eq!(*decoded.array8, *ARRAY8_VALUE);
> + assert_eq!(*decoded.array32, *ARRAY32_VALUE);
> + assert_eq!(*decoded.array64, *ARRAY64_VALUE);
> + assert_eq!(decoded.opt_present, Some(OPT_PRESENT_VALUE));
> + assert_eq!(decoded.opt_absent, None);
> + assert_eq!(decoded.pairs.len(), 2);
> + assert_eq!(decoded.pairs[0].x, 1);
> + assert_eq!(decoded.pairs[0].y, 2);
> + assert_eq!(decoded.pairs[1].x, 3);
> + assert_eq!(decoded.pairs[1].y, 4);
> + assert_eq!(decoded.slots, [10, 20, 30, 40]);
> +
> + Ok(())
> + }
> +
> + // Tests that a schema too large for the stack decodes on the heap.
How are we testing this? Do we expect the stack to explode as a test
failure? That should be guaranteed by the use of `KBox` and `init`, so I
am not sure there is much to test here...
But if you decide to keep it, let's at least size the schema so the
stack *actually* explodes: 2KB is not enough, we want at least 16KB, and
for good measure let's go with at least 128KB to be sure. Otherwise I
think it's fine to drop this test.
