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

["Alexandre Courbot" <acourbot@nvidia.com>]

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.