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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpu: nova-core: NVKV encoder generates host-endian command streams instead of little-endian
--

commit cc2f9c62ac197576039ac68784d3a562225a92be
Author: Eliot Courtney <ecourtney@nvidia.com>

gpu: nova-core: add NVKV encoder

Add an encoder for NVKV, which is the wire format for GMCAPI. The
encoded stream is a sequence of 64-bit values.

> diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs b/drivers/gpu/nova-core/gsp/nvkv.rs
> new file mode 100644
> index 0000000000000..0957dce92f962
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gsp/nvkv.rs
[ ... ]
> +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)?)

[Severity: High]
Is it intentional to serialize u64 values, such as operation words and scalar
payloads, using native endianness instead of little-endian?

The GPU firmware interface expects a little-endian wire format. On big-endian
architectures, this will generate corrupted, misordered commands and cause
initialization to fail. Should this use to_le_bytes() instead?

> +    }
> +
> +    /// 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();

[Severity: High]
Does this convert multi-byte arrays, such as [u32], to bytes using the native
memory layout?

This outputs big-endian elements on big-endian hosts, which will also result
in corrupted command streams for the GPU firmware.

> +        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)?)
> +    }
> +}
[ ... ]
> 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 0000000000000..8b66ddb151892
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
[ ... ]
> +        // The ARRAY32 op word has the element count. The two elements follow in little endian.
> +        assert_eq!(
> +            encoded[5],
> +            Op::zeroed()
> +                .with_key(ARRAY32_KEY)
> +                .with_index(Index::new::<3>())
> +                .with_opcode(Opcode::Array32)
> +                .with_value(2u32)
> +                .into_raw()
> +        );
> +        assert_eq!(
> +            encoded[6],
> +            u64::from(ARRAY32_VALUE[1]) << 32 | u64::from(ARRAY32_VALUE[0])
> +        );

[Severity: High]
Will this test fail deterministically on big-endian hosts?

The test strictly expects little-endian element packing, which confirms the
wire format requirement, but the data is encoded using native endianness in
extend_with_padding() above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-b4-nvkv-v3-0-f04504c262c2@nvidia.com?part=4