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

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

On Wed Aug 19, 2026 at 6:32 PM CEST, Danilo Krummrich wrote:
> 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.)

Actually, I forgot to add the optimization you made back in, here's the proper
diff:

(Also used T: IntoBytes as argument for extend_with_padding().)

diff --git a/drivers/gpu/nova-core/gsp/nvkv.rs b/drivers/gpu/nova-core/gsp/nvkv.rs
index 0afd6d5c48bd..98a54bdeff52 100644
--- a/drivers/gpu/nova-core/gsp/nvkv.rs
+++ b/drivers/gpu/nova-core/gsp/nvkv.rs
@@ -20,6 +20,7 @@
     num::Bounded,
     prelude::*, //
 };
+use zerocopy::Immutable;
 
 mod encode;
 pub(crate) use encode::*;
@@ -27,6 +28,49 @@
 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())
+    }
+
+    /// Reserves capacity for at least `additional` u64 entries.
+    fn reserve(&mut self, additional: usize) -> Result {
+        Ok(self.0.reserve(additional * size_of::<u64>(), GFP_KERNEL)?)
+    }
+
+    /// 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 `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();
+        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..4f599939d6e7 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,7 @@ 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.stream.reserve(2)?;
         self.encode_op(
             Op::zeroed()
                 .with_key(key)
@@ -230,8 +204,7 @@ 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.
@@ -239,7 +212,7 @@ pub(crate) fn encode_u64(&mut self, key: KeyId, index: Index, value: u64) -> Res
     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.stream.reserve(num_entries + 1)?;
         self.encode_op(
             Op::zeroed()
                 .with_key(key)
@@ -247,8 +220,7 @@ 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)
     }
 
     /// Encodes a 32-bit array as an ARRAY32 pair, zero-padded to a multiple of 8 bytes.
@@ -256,7 +228,7 @@ pub(crate) fn encode_array8(&mut self, key: KeyId, index: Index, array: &[u8]) -
     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.stream.reserve(num_entries + 1)?;
         self.encode_op(
             Op::zeroed()
                 .with_key(key)
@@ -264,15 +236,14 @@ 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)
     }
 
     /// 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.stream.reserve(array.len() + 1)?;
         self.encode_op(
             Op::zeroed()
                 .with_key(key)
@@ -280,8 +251,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)
     }
 }