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
