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 <[email protected]> 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/[email protected]?part=4
