Re: [PATCH v3 5/8] gpu: nova-core: add NVKV decoder

["Eliot Courtney" <ecourtney@nvidia.com>]

On Wed Oct 7, 2026 at 1:51 PM JST, Alexandre Courbot wrote:
>> +#![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
>> +
>> +use kernel::prelude::*;
>> +
>> +use crate::{
>> +    gsp::nvkv::{
>> +        Index,
>> +        KeyId,
>> +        Op,
>> +        Opcode, //
>> +    },
>> +    num, //
>> +};
>> +
>> +/// A decoded NVKV value.
>> +#[derive(Copy, Clone, Debug, PartialEq, Eq)]
>> +pub(crate) enum DecoderValue<'a> {
>> +    Scalar32(u32),
>> +    Scalar64(u64),
>> +    Array8(&'a [u8]),
>> +    Array32(&'a [u32]),
>> +    Array64(&'a [u64]),
>> +}
>> +
>> +/// Implements `TryFrom` from the given `DecoderValue` variant to the given type.
>> +///
>> +/// `TryFrom` is used by the `Schema` implementations in this file to convert from the
>> +/// `DecoderValue`s into the types to store. Provide the implementations for basic types here.
>
> nit: proper doclinks for referenced types (recurring problem throughout
> the file).

Fixed (here and other locations)

>
>> +macro_rules! impl_try_from_decoder_value {
>> +    ($ty:ty, $variant:ident) => {
>> +        impl<'a> TryFrom<DecoderValue<'a>> for $ty {
>> +            type Error = Error;
>> +
>> +            fn try_from(value: DecoderValue<'a>) -> Result<Self> {
>> +                if let DecoderValue::$variant(v) = value {
>> +                    Ok(v)
>> +                } else {
>> +                    Err(EINVAL)
>> +                }
>> +            }
>> +        }
>> +    };
>> +}
>> +
>> +impl_try_from_decoder_value!(u32, Scalar32);
>> +impl_try_from_decoder_value!(u64, Scalar64);
>> +impl_try_from_decoder_value!(&'a [u8], Array8);
>> +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`.
>> +pub(crate) trait Schema {
>> +    type Target;
>> +
>> +    /// Returns an initializer that creates an empty schema in place.
>> +    ///
>> +    /// Use [`KBox::init`] for the heap or `stack_pin_init!` for the stack (if sure that the value
>> +    /// is small enough to fit).
>> +    fn init() -> impl Init<Self>
>> +    where
>> +        Self: Sized;
>> +
>> +    /// Returns an initializer that makes the decoded `Target`.
>> +    ///
>> +    /// After the returned initializer runs, the schema should be empty again.
>> +    fn finish(&mut self) -> impl Init<Self::Target, Error> + '_;
>> +}
>> +
>> +/// A visitor that consumes decoded NVKV from a stream.
>
> Note that `Schema` is also defined as a "visitor that consumes decoded
> NVKV", which makes things a bit confusing. I guess that's because both
> traits implement different aspects of what a typical visitor does, but
> clarifying their relationship (and the reason for splitting) would be
> helpful.

Right, in order to avoid noisy types when you use a Schema type in a
struct, I split the trait here. So for Schemas that don't need it, you
can avoid having to put a lifetime on the struct that you plumb to the
scehma. Otherwise you'd be writing struct SomeSchema<'stream>
every time.

Let me add that info.

>
>> +///
>> +/// A schema that doesn't need to borrow data from the stream can implement this for all `'data`
>> +/// lifetimes, avoiding having to carry the lifetime parameter. A schema that borrows from the
>> +/// stream directly should implements it for its own lifetime only.
>> +pub(crate) trait Visit<'data> {
>
> Should this be named `Visitor` instead of `Visit`? Trait names should be
> nouns rather than verbs, and this would make the `Decoder::visit`
> method's prototype read more clearly imho. If you intended to use
> `Visit` as a noun, I don't think that works either since the `visit`
> method is potentially called many times (so it is not a single visit).

According to [1], traits should preferably be named as verbs, and it in
particular calls out the case where there's one fn in the trait:

```
This RFC proposes to amend the convention to further say: if there
is a single method that is the dominant functionality of the trait,
consider using the same name for the trait itself.
```

That said in the kernel there are more noun/noun phrase traits. So no
worries to rename it to Visitor. But I wouldn't say trait names should
be nouns as a rule, it seems that the prescriptive answer is to prefer
the opposite.

[1]: https://rust-lang.github.io/rfcs/0344-conventions-galore.html#trait-naming

>
>> +    /// Visits one decoded pair. Returns `Ok(true)` if the schema consumed it.
>> +    fn visit(&mut self, key: KeyId, index: Index, value: DecoderValue<'data>) -> Result<bool>;
>> +}
>> +
>> +/// A read position in an NVKV stream.
>> +struct Cursor<'a> {
>> +    data: &'a [u64],
>> +}
>> +
>> +impl<'a> Cursor<'a> {
>> +    /// Creates a cursor at the start of `data`.
>> +    fn new(data: &'a [u64]) -> Self {
>> +        Self { data }
>> +    }
>> +
>> +    /// Returns `true` if no `u64` values remain.
>> +    fn is_empty(&self) -> bool {
>> +        self.data.is_empty()
>> +    }
>> +
>> +    /// Takes the next `u64`.
>> +    fn take_u64(&mut self) -> Result<u64> {
>> +        // PANIC: `take_u64s(1)` returns exactly one element on success.
>> +        Ok(self.take_u64s(1)?[0])
>> +    }
>> +
>> +    /// Takes `count` bytes. If `count` is not a multiple of 8 (`u64` size), bytes are discarded up
>> +    /// to the next multiple.
>> +    fn take_u8s(&mut self, count: usize) -> Result<&'a [u8]> {
>> +        let values = self.take_u64s(count.div_ceil(8))?;
>
> s/8/size_of::<u64>()
>
>> +        values.as_bytes().get(..count).ok_or(EINVAL)
>> +    }
>> +
>> +    /// Takes `count` 32-bit values. If `count` is not a multiple of 2 (`u64` size), bytes are
>> +    /// discarded up to the next multiple.
>> +    fn take_u32s(&mut self, count: usize) -> Result<&'a [u32]> {
>> +        let values = self.take_u64s(count.div_ceil(2))?;
>
> ... and maybe `size_of::<u64>() / size_of::<u32>()` here?

Done

[...] (applied review comments)
>> +    // Tests that basic decoding into a manually implemented `Schema` works correctly.
>> +    #[test]
>> +    fn decode_raw_schema() -> Result {
>> +        // Decodes an IMM32 pair and a SEQ64 pair (the encoder emits a u64 as a single-element
>> +        // SEQ64) with a hand written `Schema`. Keys and value constants chosen to distinguish e.g.
>> +        // saving the wrong value to the wrong location.
>> +        const SCALAR32_KEY: KeyId = 0x1001;
>> +        const SCALAR64_KEY: KeyId = 0x1002;
>> +        const UNKNOWN_KEY: KeyId = 0x2001;
>> +
>> +        const SCALAR32_VALUE: u32 = 0x1111_2222;
>> +        const SCALAR64_VALUE: u64 = 0x3333_4444_5555_6666;
>> +
>> +        // The output type of the hand written `Schema`. In this case, we can have it also implement
>> +        // `Schema` on itself rather than having a separate carrier type, since the `Schema`
>> +        // implementation is completely stateless.
>> +        #[derive(Default)]
>> +        struct RawSchema {
>> +            scalar32: u32,
>> +            scalar64: u64,
>> +        }
>> +
>> +        impl Schema for RawSchema {
>> +            type Target = Self;
>> +
>> +            fn init() -> impl Init<Self> {
>> +                Self::default()
>> +            }
>> +
>> +            fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>> +                Ok(core::mem::take(self))
>> +            }
>> +        }
>> +
>> +        impl<'d> Visit<'d> for RawSchema {
>> +            fn visit(&mut self, key: KeyId, index: Index, value: DecoderValue<'d>) -> Result<bool> {
>> +                if index != Index::new::<0>() {
>> +                    return Err(EINVAL);
>> +                }
>> +                match key {
>> +                    SCALAR32_KEY => self.scalar32 = value.try_into()?,
>> +                    SCALAR64_KEY => self.scalar64 = value.try_into()?,
>> +                    _ => return Ok(false),
>> +                }
>> +                Ok(true)
>> +            }
>> +        }
>> +
>> +        let mut encoder = Encoder::new();
>> +        encoder.encode_u32(SCALAR32_KEY, Index::new::<0>(), SCALAR32_VALUE)?;
>> +        encoder.encode_u64(SCALAR64_KEY, Index::new::<0>(), SCALAR64_VALUE)?;
>> +        let serialized = encoder.finish();
>> +
>> +        let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
>> +        let mut schema = KBox::init(RawSchema::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);
>
> Nice test!

thanks~~

>
>> +
>> +        // An unknown key should fail with under `UnknownKeyPolicy::Error` and be skipped under
>> +        // `UnknownKeyPolicy::Ignore`.
>> +        let mut encoder = Encoder::new();
>> +        encoder.encode_u32(UNKNOWN_KEY, Index::new::<0>(), 1)?;
>> +
>> +        let serialized = encoder.finish();
>> +        let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Error);
>> +        let mut schema = KBox::init(RawSchema::init(), GFP_KERNEL)?;
>> +        assert!(decoder.decode(&mut *schema).is_err());
>> +
>> +        let decoder = Decoder::new(&serialized, UnknownKeyPolicy::Ignore);
>> +        let mut schema = KBox::init(RawSchema::init(), GFP_KERNEL)?;
>> +        let decoded = KBox::try_init(decoder.decode(&mut *schema)?, GFP_KERNEL)?;
>> +        assert_eq!(decoded.scalar32, 0);
>
> ... this part looks like it should be its own test though. That's
> trivial to achieve if you declare `RawSchema` at the module-level
> instead of inside the method.
>
> (also nit: let's assert `scalar64` as well, or none of the values at all
> - I'd lean towards none since that part is already covered by the
>   regular decoding test).

Done

>
>> +
>> +        Ok(())
>> +    }
>> +
>> +    /// Records each visit as (key, index, value), for tests on hand-built streams.
>
> nit: `(key, index, value)`
>
>> +    #[derive(Default)]
>> +    struct Recorder<'d> {
>> +        visits: KVVec<(KeyId, u64, DecoderValue<'d>)>,
>> +    }
>> +
>> +    impl<'d> Schema for Recorder<'d> {
>> +        type Target = KVVec<(KeyId, u64, DecoderValue<'d>)>;
>> +
>> +        fn init() -> impl Init<Self> {
>> +            Self::default()
>> +        }
>> +
>> +        fn finish(&mut self) -> impl Init<Self::Target, Error> + '_ {
>> +            Ok(core::mem::take(&mut self.visits))
>> +        }
>> +    }
>> +
>> +    impl<'d> Visit<'d> for Recorder<'d> {
>> +        fn visit(&mut self, key: KeyId, index: Index, value: DecoderValue<'d>) -> Result<bool> {
>> +            self.visits.push((key, index.get(), value), GFP_KERNEL)?;
>> +            Ok(true)
>> +        }
>> +    }
>> +
>> +    // Tests the decoder on hand-built `u64` values that the encoder does not produce: SEQ32,
>> +    // multi-value SEQ64, zero counts, a non-zero index and padded arrays.
>> +    #[test]
>> +    fn decode_raw_u64s() -> Result {
>> +        const SEQ32_KEY: KeyId = 0x2000;
>> +        const SEQ64_KEY: KeyId = 0x2010;
>> +        const EMPTY_SEQ64_KEY: KeyId = 0x2020;
>> +        const EMPTY_SEQ32_KEY: KeyId = 0x2021;
>> +        const EMPTY_ARRAY8_KEY: KeyId = 0x2030;
>> +        const EMPTY_ARRAY32_KEY: KeyId = 0x2031;
>> +        const EMPTY_ARRAY64_KEY: KeyId = 0x2032;
>> +        const ARRAY8_KEY: KeyId = 0x2040;
>> +        const ARRAY32_KEY: KeyId = 0x2041;
>> +
>> +        let index3 = Index::new::<3>();
>
> nit: variable only used once, can be created inline.

Done

>
> I really like how testing coverage has improved in this revision!

thanks for your suggestion~!