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

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Aug 17, 2026 at 2:56 PM CEST, Eliot Courtney wrote:
> +    fn push_bytes_with_padding(&mut self, bytes: &[u8]) -> Result {
> +        let num_entries = bytes.len().div_ceil(size_of::<u64>());
> +        self.backing.reserve(num_entries, GFP_KERNEL)?;
> +
> +        let spare = self.backing.spare_capacity_mut();
> +        let dst = spare.as_mut_ptr().cast::<u8>();
> +
> +        // 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(())
> +    }

Ick! That's a lot of unsafe code. I think we can avoid this by using KVVec<u8>
instead of KVVec<u64>, ideally in a new type that upholds the padding invariant.

Here's a diff of what I came up with; note that it also gets us rid of the
unsafe in take_u32s() in the decoder by using zerocopy.

(Technically it would also be possible to make Cursor operate on a byte stream
and let zerocopy to the rest, as all the take methods are fallible already. But
I think the invariant on EncodedStream makes sense.)

diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs b/drivers/gpu/nova-core/gsp/nvkv.rs
index 0afd6d5c48bd..564a9a93f7cd 100644
--- a/drivers/gpu/nova-core/gsp/nvkv.rs
+++ b/drivers/gpu/nova-core/gsp/nvkv.rs
@@ -27,6 +27,43 @@
 mod decode;
 pub(crate) use decode::*;

+/// An encoded NVKV byte stream.
+///
+/// # Invariants
+///
+/// The byte length is always a multiple of `size_of::<u64>()`.
+pub(crate) struct EncodedStream(KVVec<u8>);
+
+impl EncodedStream {
+    /// Creates an empty stream.
+    fn new() -> Self {
+        Self(KVVec::new())
+    }
+
+    /// Appends a single `u64` to the stream.
+    fn push_u64(&mut self, value: u64) -> Result {
+        Ok(self.0.extend_from_slice(&value.to_ne_bytes(), GFP_KERNEL)?)
+    }
+
+    /// Appends `bytes` to the stream, zero-padded to a `u64` boundary.
+    fn extend_with_padding(&mut self, bytes: &[u8]) -> Result {
+        self.0.extend_from_slice(bytes, GFP_KERNEL)?;
+        let padding = bytes.len().next_multiple_of(size_of::<u64>()) - bytes.len();
+        // INVARIANT: The padding ensures the total length remains a multiple of `size_of::<u64>()`.
+        Ok(self.0.extend_with(padding, 0u8, GFP_KERNEL)?)
+    }
+}
+
+impl Deref for EncodedStream {
+    type Target = [u64];
+
+    fn deref(&self) -> &[u64] {
+        let count = self.0.len() / size_of::<u64>();
+        <[u64]>::ref_from_bytes_with_elems(&self.0, count)
+            .expect("EncodedStream invariant violated: not u64-aligned")
+    }
+}
+
 /// The identifier of an NVKV key.
 pub(crate) type KeyId = u16;

diff --git a/drivers/gpu/nova-core/gsp/nvkv/decode.rs b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
index 9112dcf1aaca..d1271917c62e 100644
--- a/drivers/gpu/nova-core/gsp/nvkv/decode.rs
+++ b/drivers/gpu/nova-core/gsp/nvkv/decode.rs
@@ -381,9 +381,9 @@ fn take_u8s(&mut self, count: usize) -> Result<&[u8]> {

     fn take_u32s(&mut self, count: usize) -> Result<&[u32]> {
         let values = self.take_u64s(count.div_ceil(2))?;
-        // SAFETY: `values` is 8 byte aligned and only 4 byte alignment is required. All bit
-        // patterns are valid for `u32`.
-        Ok(unsafe { core::slice::from_raw_parts(values.as_ptr().cast::<u32>(), count) })
+        let bytes = values.as_bytes();
+        <[u32]>::ref_from_bytes_with_elems(&bytes[..count * size_of::<u32>()], count)
+            .map_err(|_| EINVAL)
     }

     fn take_u64s(&mut self, count: usize) -> Result<&[u64]> {
@@ -401,8 +401,11 @@ pub(crate) struct Decoder<'a> {

 impl<'a> Decoder<'a> {
     /// Creates a decoder for `data` that handles unknown keys per `policy`.
-    pub(crate) fn new(data: &'a [u64], policy: UnknownKeyPolicy) -> Self {
-        Self { data, policy }
+    pub(crate) fn new(data: &'a super::EncodedStream, policy: UnknownKeyPolicy) -> Self {
+        Self {
+            data: &**data,
+            policy,
+        }
     }

     fn visit<S: Schema>(
diff --git a/drivers/gpu/nova-core/gsp/nvkv/encode.rs b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
index 31ea5788e772..88f2a03a593f 100644
--- a/drivers/gpu/nova-core/gsp/nvkv/encode.rs
+++ b/drivers/gpu/nova-core/gsp/nvkv/encode.rs
@@ -156,51 +156,26 @@ fn encode(&self, encoder: &mut Encoder) -> Result {

 /// An encoder for an NVKV stream.
 pub(crate) struct Encoder {
-    backing: KVVec<u64>,
+    stream: super::EncodedStream,
 }

 impl Encoder {
     /// Creates an empty encoder.
     pub(crate) fn new() -> Self {
         Self {
-            backing: KVVec::new(),
+            stream: super::EncodedStream::new(),
         }
     }

-    /// Appends `bytes` to the stream, padded to a multiple of 8 bytes.
-    fn push_bytes_with_padding(&mut self, bytes: &[u8]) -> Result {
-        let num_entries = bytes.len().div_ceil(size_of::<u64>());
-        self.backing.reserve(num_entries, GFP_KERNEL)?;
-
-        let spare = self.backing.spare_capacity_mut();
-        let dst = spare.as_mut_ptr().cast::<u8>();
-
-        // 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(())
-    }
-
     /// Returns the encoded data.
     #[must_use = "encoded data must be consumed"]
-    pub(crate) fn finish(self) -> KVVec<u64> {
-        self.backing
+    pub(crate) fn finish(self) -> super::EncodedStream {
+        self.stream
     }

     #[inline(always)]
     fn encode_op(&mut self, op: Op) -> Result {
-        self.backing.push(op.into_raw(), GFP_KERNEL)?;
-        Ok(())
+        self.stream.push_u64(op.into_raw())
     }

     /// Encodes a 32-bit value as an IMM32 pair, with the value in the op word.
@@ -213,8 +188,7 @@ pub(crate) fn encode_u32(&mut self, key: KeyId, index: Index, value: u32) -> Res
                 .with_index(index)
                 .with_opcode(Opcode::Imm32)
                 .with_value(value),
-        )?;
-        Ok(())
+        )
     }

     /// Encodes a 64-bit value as a single-element SEQ64 pair.
@@ -222,7 +196,6 @@ pub(crate) fn encode_u32(&mut self, key: KeyId, index: Index, value: u32) -> Res
     pub(crate) fn encode_u64(&mut self, key: KeyId, index: Index, value: u64) -> Result {
         // TODO: Consider automatically merging sequential keys.
         const KEY_COUNT: u32 = 1;
-        self.backing.reserve(2, GFP_KERNEL)?;
         self.encode_op(
             Op::zeroed()
                 .with_key(key)
@@ -230,16 +203,13 @@ pub(crate) fn encode_u64(&mut self, key: KeyId, index: Index, value: u64) -> Res
                 .with_opcode(Opcode::Seq64)
                 .with_value(KEY_COUNT),
         )?;
-        self.backing.push_within_capacity(value)?;
-        Ok(())
+        self.stream.push_u64(value)
     }

     /// Encodes a byte array as an ARRAY8 pair, zero-padded to a multiple of 8 bytes.
     #[inline(always)]
     pub(crate) fn encode_array8(&mut self, key: KeyId, index: Index, array: &[u8]) -> Result {
         let value_count = u32::try_from(array.len()).map_err(|_| EMSGSIZE)?;
-        let num_entries = array.len().div_ceil(size_of::<u64>());
-        self.backing.reserve(num_entries + 1, GFP_KERNEL)?;
         self.encode_op(
             Op::zeroed()
                 .with_key(key)
@@ -247,16 +217,13 @@ pub(crate) fn encode_array8(&mut self, key: KeyId, index: Index, array: &[u8]) -
                 .with_opcode(Opcode::Array8)
                 .with_value(value_count),
         )?;
-        self.push_bytes_with_padding(array.as_bytes())?;
-        Ok(())
+        self.stream.extend_with_padding(array.as_bytes())
     }

     /// Encodes a 32-bit array as an ARRAY32 pair, zero-padded to a multiple of 8 bytes.
     #[inline(always)]
     pub(crate) fn encode_array32(&mut self, key: KeyId, index: Index, array: &[u32]) -> Result {
         let value_count = u32::try_from(array.len()).map_err(|_| EMSGSIZE)?;
-        let num_entries = array.len().div_ceil(2);
-        self.backing.reserve(num_entries + 1, GFP_KERNEL)?;
         self.encode_op(
             Op::zeroed()
                 .with_key(key)
@@ -264,15 +231,13 @@ pub(crate) fn encode_array32(&mut self, key: KeyId, index: Index, array: &[u32])
                 .with_opcode(Opcode::Array32)
                 .with_value(value_count),
         )?;
-        self.push_bytes_with_padding(array.as_bytes())?;
-        Ok(())
+        self.stream.extend_with_padding(array.as_bytes())
     }

     /// Encodes a 64-bit array as an ARRAY64 pair.
     #[inline(always)]
     pub(crate) fn encode_array64(&mut self, key: KeyId, index: Index, array: &[u64]) -> Result {
         let value_count = u32::try_from(array.len()).map_err(|_| EMSGSIZE)?;
-        self.backing.reserve(array.len() + 1, GFP_KERNEL)?;
         self.encode_op(
             Op::zeroed()
                 .with_key(key)
@@ -280,8 +245,7 @@ pub(crate) fn encode_array64(&mut self, key: KeyId, index: Index, array: &[u64])
                 .with_opcode(Opcode::Array64)
                 .with_value(value_count),
         )?;
-        self.push_bytes_with_padding(array.as_bytes())?;
-        Ok(())
+        self.stream.extend_with_padding(array.as_bytes())
     }
 }

@@ -292,8 +256,6 @@ mod tests {
     // Tests that each kind of value is encoded to NVKV wire format properly.
     #[test]
     fn encode_all_value_kinds() -> Result {
-        // All keys, indexes, and values are distinct but arbitrary values to make it easier for the
-        // test to catch bugs in the encoded output.
         const U32_KEY: KeyId = 0x1001;
         const U64_KEY: KeyId = 0x1002;
         const ARRAY8_KEY: KeyId = 0x1003;