Re: [PATCH v2 7/8] gpu: nova-core: add NVKV typed decoding

["Gary Guo" <gary@garyguo.net>]

On Thu Sep 17, 2026 at 12:42 PM BST, Eliot Courtney wrote:
> On Thu Sep 17, 2026 at 5:52 PM JST, Gary Guo wrote:
>> On Thu Sep 17, 2026 at 9:37 AM BST, Alexandre Courbot wrote:
>>> On Thu Sep 17, 2026 at 8:53 AM BST, Gary Guo wrote:
>>>> On Mon Sep 14, 2026 at 7:55 AM BST, Eliot Courtney wrote:
>>>>> 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:
>>>>>>>  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.
>>>>
>>>> Hmm, am I missing the obvious? Why not generate a `match` expression on IDs of
>>>> fields? It looks like in the example all keys would have a known ID to the
>>>> macro.
>>>
>>> Some keys may come from embedded structs, which the macro has no way to
>>> see.
>>
>> You can match all keys that you can see, and delegate to embedded structs if
>> keys are not known.
>>
>> This is essentially the same pattern that `#[serde(flatten)]` uses, just
>> replacing identifier names with keys.
>>
>> The keys don't need be part of the type system, and it just additional metadata
>> for the macro to generate correct impl.
>>
>> Best,
>> Gary
>
> This sounds approximately equivalent to the option #2 I mentioned
> (w.r.t. delegating if the keys are not known).
>
> I considered a serde-like approach where the metadata is annotated for
> the macro rather than by the type system, but I wanted to keep the macro
> simple and not introduce more macro-DSL than I need to. And also keep it
> kinda easy to write manual Schemas by not keeping the deserialization
> info in macro metadata.

I guess that given that your schema types are completely separate from the
struct definitions, it is okay.

Best,
Gary