On Mon Sep 14, 2026 at 12:46 PM JST, Alexandre Courbot wrote:
> 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
Thanks for catching this. This builds on 1.85.0 without issue, but I
checked on 1.98.1 and it fails to build. I'll add building with stable
to my checklist.
>
>> #![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?
This will be used soon. For example [1] uses it.
[1]: https://lore.kernel.org/all/[email protected]/
>
>> +
>> 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.
I thought quite a bit about this while writing this code, since we need
the escape hatch to imperative decode (custom Schema impl basically). To
be able to get it down to a match on the key, we need to know ahead of
time which keys a Schema will consume. That duplicates the info from the
visit() implementation.
I thought up a few methods but it's unclear to me which one is best, so
I just left it for now. Please LMK if you think this is urgent, I can
try in a follow up to improve this. Here are my ideas (when I say O(1)
lookup I mean modulo how the compiler decides to do it with the set of
key IDs it gets):
1. current code - just visit()
pros: key source of truth not duplicates
cons: O(field) visit as you say
2. Associated const KEY_ID: Option<KeyId> - None if a Schema accepts multiple
keys.
You can match on each associated const in the macro.
pros: O(1) if the current key goes to a field with KEY_ID = Some(...)
cons: O(#fields accepting multiple keys) if current key is one of them
3. fn accepts() -> bool
You can match on `if F::accepts(key)` for each field. We could potentially make
this const with Gary's const traits polyfill.
pros: O(1) if you write an inline-able+optimizable implementation.
4. Associated const KEYS table; use tricks to concat tables
pros: O(1) lookup
cons: actually MSRV can't get this to optimize down to O(1)
if you use slice::contains(), but stable can.
I don't like #2. With the current #1 we can decide later how to optimize.
#3 and #4 feel mostly equal to me, maybe #3 is slightly better.
>
>> + }
>> +
>> + #[inline(always)]
>
> In this patch as well these should probably be just `#[inline]`.
Done.
[...]
>> +/// 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?
Think we can just remove the ability to construct this without using
init(). Then callers can use stack_pin_init! if they really want it on
the stack.
>
>> + }
>> + }
>> +
>> + 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?
Done.
>
>> +}
>> +
>> +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`?
Yeah. So it happens that the stack copies are elided in this particular
case. But I think it's better anyway to do it as you suggest.
>
>> + 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.
I added some documentation about how it works. But this is just an
implementation of a finite state machine which tracks the completion of
each child Schema based on the index advancing. It needs some
bookkeeping to handle edge cases like you didn't receive anything / you
got to the end (`current_started`).
>
>> + } 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.
It's documented on `Key` but not here, let me add a link to it.
>
>> +
>> +/// 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.
Yeah agreed on all points.
>
>> +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?
Yerp
>
> <...>
>> + // 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.
Will do.
