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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The NVKV wire format encoder improperly depends on the host's native endianness, generating a different sequence of 64-bit values on big-endian versus little-endian architectures. This corrupts the GPU command stream on big-endian systems.
- [Low] Missing `#[inline(always)]` on the `From<Opcode>` implementation that delegates to an assertion requiring constant evaluation.
--

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

gpu: nova-core: add NVKV encoder

This commit introduces an NVKV encoder for GMCAPI wire formats, converting
sequences of key-value operations into a stream of 64-bit words. The driver
encodes primitives like 32-bit/64-bit values and arrays using specific opcodes.
Tests are included to verify the binary layout of the encoded output.

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

[Severity: High]
Does copying the memory representation directly using data.as_bytes() rely
on the host's native endianness? On big-endian architectures, this
host-endian byte slice will mix incorrectly when grouped into 64-bit words,
corrupting the NVKV stream for the GPU hardware which expects a fixed
little-endian layout.

> +        // 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)?)
> +    }
> +}
[ ... ]
> +impl Deref for EncodedStream {
> +    type Target = [u64];
> +
> +    fn deref(&self) -> &Self::Target {
> +        // An empty Vec's pointer isn't necessarily aligned by StreamAllocator::MIN_ALIGN.
> +        if self.0.is_empty() {
> +            return &[];
> +        }
> +
> +        // PANIC: By the type invariants the byte length is a multiple of size_of::<u64>(), and
> +        // the backing buffer of a non-empty vector has at least u64 alignment per
> +        // StreamAllocator's minimum alignment.
> +        <[u64]>::ref_from_bytes(&self.0).expect("EncodedStream invariant violated")

[Severity: High]
Can casting the host-endian byte stream directly to a slice of native-endian
u64s cause data corruption? Since the hardware expects a little-endian
layout, interpreting these bytes via ref_from_bytes on big-endian hosts will
result in functionally incorrect GPU commands.

> +    }
> +}
[ ... ]
> +impl From<Opcode> for Bounded<u64, 4> {
> +    fn from(value: Opcode) -> Self {
> +        Bounded::from_expr(value as u64)
> +    }
> +}

[Severity: Low]
Is it possible this function will fail compilation if not heuristically
inlined by the compiler? Since it delegates to Bounded::from_expr() which
uses build_assert!() on its parameter, subsystem guidelines require an
explicit #[inline(always)] annotation to ensure the optimizer can perform
constant folding and evaluate the assertion at compile time.

> 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..6c1a9cbd90e84
> --- /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 on big-endian hosts? The test explicitly asserts a
little-endian memory layout for the Array32 values (placing the first 32-bit
element in the lower 32 bits of the u64). On big-endian systems, the values
will be in the upper 32 bits, causing the test to fail and confirming that
the generated payloads will be unreadable by the GPU.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-b4-nvkv-v2-0-0de9d5c8658c@nvidia.com?part=4