Re: [PATCH 2/6] gpu: nova-core: add NVKV encoder

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] NVKV array encoding relies on host memory layout, causing endianness corruption and unconditional KUnit test failures on big-endian architectures.
--

--- Patch [2]: [PATCH 2/6] gpu: nova-core: add NVKV encoder ---
commit 0223916dc52ad28f6ad72c2fdc7087b7939c7ed3
Author: Eliot Courtney <[email protected]>

gpu: nova-core: add NVKV encoder

This commit adds an encoder for the NVKV wire format, which encodes calls
as a sequence of 64-bit values. It includes functions to encode single
values and arrays, and adds a test for the encoding.

> diff --git a/drivers/gpu/nova-core/gsp/nvkv/encode.rs b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
> index 0000000000000..6da81a371651a 100644
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
[ ... ]
> +    fn push_bytes_with_padding(&mut self, bytes: &[u8]) -> Result {
[ ... ]
> +        // SAFETY: At least bytes.len() bytes of space are guaranteed since num_entries
> +        // worth of space was just reserved.
> +        unsafe { core::ptr::copy_nonoverlapping(bytes.as_ptr(), dst, bytes.len()) };
> +
> +        let padding = num_entries * size_of::<u64>() - bytes.len();
> +        if padding > 0 {
> +            // SAFETY: At least num_entries * size_of::<u64>() bytes of space are guaranteed.
> +            unsafe { core::ptr::write_bytes(dst.add(bytes.len()), 0, padding) };
> +        }
> +
> +        // SAFETY: These bytes were just initialized and every bit pattern is valid for u64.
> +        unsafe { self.backing.inc_len(num_entries) };
> +
> +        Ok(())
> +    }
[ ... ]
> +    pub(crate) fn encode_array32(&mut self, key: KeyId, index: Index, array: &[u32]) -> Result {
[ ... ]
> +        self.push_bytes_with_padding(array.as_bytes())?;
> +        Ok(())
> +    }
[ ... ]
> +    pub(crate) fn encode_array64(&mut self, key: KeyId, index: Index, array: &[u64]) -> Result {
[ ... ]
> +        self.push_bytes_with_padding(array.as_bytes())?;
> +        Ok(())
> +    }

[Severity: High]
Will this produce an incorrect byte stream on big-endian architectures?

Because push_bytes_with_padding() in drivers/gpu/nova-core/gsp/nvkv/encode.rs
copies host memory directly into the u64 backing storage, and encode_array32()
and encode_array64() extract host-endian bytes via array.as_bytes(), the
endianness of the generated stream depends on the host.

[ ... ]
> +        assert_eq!(
> +            encoded[4],
> +            u64::from_le_bytes([0xaa, 0xbb, 0xcc, 0, 0, 0, 0, 0])
> +        );

[Severity: High]
Will this test unconditionally fail on big-endian systems?

The test in drivers/gpu/nova-core/gsp/nvkv/encode.rs explicitly asserts that
the generated u64 must match a little-endian layout, which seems to prove
the host-endian implementation above will produce unexpected results on
big-endian architectures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2