Re: [PATCH v4 4/7] gpu: nova-core: add NVKV decoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpu: nova-core: endianness bug when extracting u8/u32 arrays from native &[u64] stream
--

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

gpu: nova-core: add NVKV decoder

This commit adds an NVKV decoder for parsing messages from GSP for GMCAPI
calls. It reads an encoded stream of (key, index, value) tuples and invokes
a Schema/Visit trait interface to consume the values.

> diff --git a/drivers/gpu/nova-core/gsp/nvkv/decode.rs b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
> new file mode 100644
> index 0000000000000..6496fab1c0c62
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
[ ... ]
> +    /// Takes `count` bytes. If `count` is not a multiple of 8 (`u64` size), bytes are discarded up
> +    /// to the next multiple.
> +    fn take_u8s(&mut self, count: usize) -> Result<&'a [u8]> {
> +        let values = self.take_u64s(count.div_ceil(size_of::<u64>()))?;
> +        values.as_bytes().get(..count).ok_or(EINVAL)
> +    }

[Severity: High]
Does casting a native u64 slice to bytes extract the wrong payload on
big-endian architectures?

Because Cursor::take_u64s() reads host-native 64-bit integers, using
.as_bytes() reverses the byte layout on big-endian hosts compared to
little-endian.

For example, if a 3-byte payload is stored inside a 64-bit word, a big-endian
host will have the payload at the end of the word and zero-padding at the
beginning, causing .get(..count) to extract the zeros instead of the actual
data payload.

> +
> +    /// Takes `count` 32-bit values. If `count` is not a multiple of 2 (`u32` values per `u64`),
> +    /// bytes are discarded up to the next multiple.
> +    fn take_u32s(&mut self, count: usize) -> Result<&'a [u32]> {
> +        let values = self.take_u64s(count.div_ceil(size_of::<u64>() / size_of::<u32>()))?;
> +        <[u32]>::ref_from_prefix_with_elems(values.as_bytes(), count)
> +            .map(|(elems, _)| elems)
> +            .map_err(|_| EINVAL)
> +    }

[Severity: High]
Can this result in swapped 32-bit words on big-endian systems?

Similar to the byte array extraction above, passing the native memory
representation through .as_bytes() to recreate a [u32] slice will swap
the order of the 32-bit halves on big-endian architectures compared to
little-endian ones.

[ ... ]
> +    /// Decodes every pair into `schema` and returns the result of [`Schema::finish`].
> +    pub(crate) fn decode<'s, S: Schema + Visit<'a>>(
> +        &self,
> +        schema: &'s mut S,
> +    ) -> Result<impl Init<S::Target, Error> + 's> {
> +        let mut cursor = Cursor::new(self.data);
> +        while !cursor.is_empty() {
> +            let op: Op = cursor.take_u64()?.into();

[Severity: High]
Does this misalign the operation bitfields on big-endian hosts?

The 64-bit operation word is read as a host-native integer via take_u64().
If the NVKV stream comes from a little-endian GSP, parsing it directly as a
native u64 on a big-endian host will misalign the key, index, and value
fields inside Op.

Should this explicitly handle endianness (e.g., using little-endian read
functions) instead of relying on native host integers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016-b4-nvkv-v4-0-e8b92b91aa5b@nvidia.com?part=4