Re: [PATCH v2 4/8] gpu: nova-core: add NVKV encoder

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

On Tue Sep 8, 2026 at 12:07 AM JST, Alexandre Courbot wrote:
> On Thu Aug 27, 2026 at 11:12 PM JST, Eliot Courtney wrote:
> <...>
>> +/// An encoded NVKV byte stream.
>> +///
>> +/// # Invariants
>> +///
>> +/// The byte length is always a multiple of `size_of::<u64>()`.
>> +pub(crate) struct EncodedStream(Vec<u8, StreamAllocator>);
>> +
>> +impl EncodedStream {
>> +    /// Creates an empty stream.
>> +    fn new() -> Self {
>> +        // INVARIANT: An empty stream's byte length is 0, a multiple of `size_of::<u64>()`.
>> +        Self(Vec::new())
>> +    }
>> +
>> +    /// Appends a single `u64` to the stream.
>> +    fn push_u64(&mut self, value: u64) -> Result {
>> +        // INVARIANT: Appending `size_of::<u64>()` bytes keeps the byte length a multiple of
>> +        // `size_of::<u64>()`.
>> +        Ok(self.0.extend_from_slice(&value.to_ne_bytes(), GFP_KERNEL)?)
>> +    }
>> +
>> +    /// Appends `data` as bytes to the stream, zero-padded to a `u64` boundary.
>> +    fn extend_with_padding<T: IntoBytes + Immutable + ?Sized>(&mut self, data: &T) -> Result {
>> +        let bytes = data.as_bytes();
>> +        let padded = bytes.len().next_multiple_of(size_of::<u64>());
>> +        // Reserve so that a failed allocation can't leave the invariant violated.
>> +        self.0.reserve(padded, GFP_KERNEL)?;
>> +        self.0.extend_from_slice(bytes, GFP_KERNEL)?;
>> +        // INVARIANT: The padding ensures the total length remains a multiple of
>> +        // `size_of::<u64>()`.
>> +        Ok(self.0.extend_with(padded - bytes.len(), 0u8, GFP_KERNEL)?)
>> +    }
>> +}
>> +
>> +// The Deref to &[u64] relies on this alignment guarantee.
>
> nit: `&[u64]`.
>
> <...>
>> +/// Describes the format of the following NVKV operation.
>> +#[derive(Debug, Copy, Clone, PartialEq, Eq)]
>> +#[repr(u8)]
>> +enum Opcode {
>> +    /// A 32-bit value in the op word.
>> +    Imm32 = 0,
>> +    /// 32-bit values for consecutive keys, starting at the op word's key.
>> +    Seq32 = 1,
>> +    /// 64-bit values for consecutive keys, starting at the op word's key.
>> +    Seq64 = 2,
>> +    /// An array of bytes.
>> +    Array8 = 3,
>> +    /// An array of 32-bit elements.
>> +    Array32 = 4,
>> +    /// An array of 64-bit elements.
>> +    Array64 = 5,
>> +}
>> +
>> +// TODO[FPRI]: This is a temporary solution to be replaced with the corresponding derive macros once
>> +// they land.
>
> Actually, can't you use the nova-core local `bounded_enum!` macro to
> define `OpCode`? This would generate the implementations below automatically.
>
>> +impl TryFrom<Bounded<u64, 4>> for Opcode {
>> +    type Error = Error;
>> +
>> +    fn try_from(value: Bounded<u64, 4>) -> Result<Self> {
>> +        match value.get() {
>> +            0 => Ok(Self::Imm32),
>> +            1 => Ok(Self::Seq32),
>> +            2 => Ok(Self::Seq64),
>> +            3 => Ok(Self::Array8),
>> +            4 => Ok(Self::Array32),
>> +            5 => Ok(Self::Array64),
>> +            _ => Err(EINVAL),
>> +        }
>> +    }
>> +}
>> +
>> +impl From<Opcode> for Bounded<u64, 4> {
>> +    fn from(value: Opcode) -> Self {
>> +        Bounded::from_expr(value as u64)
>> +    }
>> +}
>
> Sashiko has a point that `from` requires `#[inline(always)]`. Ideally,
> we prefer to avoid using `from_expr` when we can, which in this case we
> can by doing an exhaustive enumeration. Which is exactly what the
> `bounded_enum` does, so leveraging it also solves this issue. :)
>
>> diff --git a/drivers/gpu/nova-core/gsp/nvkv/encode.rs b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
>> new file mode 100644
>> index 000000000000..6c1a9cbd90e8
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
>> @@ -0,0 +1,210 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>> +
>> +#![cfg_attr(not(CONFIG_KUNIT), expect(dead_code))]
>> +
>> +use kernel::prelude::*;
>> +
>> +use super::{
>> +    EncodedStream,
>> +    Index,
>> +    KeyId,
>> +    Op,
>> +    Opcode, //
>> +};
>> +
>> +/// An encoder for an NVKV stream.
>> +pub(crate) struct Encoder {
>> +    stream: EncodedStream,
>> +}
>> +
>> +impl Encoder {
>> +    /// Creates an empty encoder.
>> +    pub(crate) fn new() -> Self {
>> +        Self {
>> +            stream: EncodedStream::new(),
>> +        }
>> +    }
>> +
>> +    /// Returns the encoded data.
>> +    #[must_use = "encoded stream must be consumed"]
>> +    pub(crate) fn finish(self) -> EncodedStream {
>> +        self.stream
>> +    }
>> +
>> +    #[inline(always)]
>
> Unless not inlining implies a build error (as is the case for
> `Bounded::from_expr`), I think the convention is to stick to `#[inline]`
> for these.
>
> Same applies for other methods.

Thanks, particularly on noticing the bounded_enum!