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

["Eliot Courtney" <ecourtney@nvidia.com>]

On Thu Aug 20, 2026 at 1:47 AM JST, Danilo Krummrich wrote:
> 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().)

I think the `EncodedStream` newtype enforcing the padding stuff is a
good idea. IMO it's simpler if we keep that type private to encode.rs
though. Currently I suppose you didn't because we need some way of
owning the KVVec<u8> that can safely+nicely transform it to &[u64].

I was thinking what if we added a conversion on Vec based on
FromBytes/IntoBytes, mirroring cast/try_cast for io projections. Then we
could just cast to KVVec<u64> and return that. But one thing I'm not
sure about is the layout guarantee here though, w.r.t. casting a Vec
then freeing it as a different thing (ofc for things that have no custom
drop impl). I saw that the wording is "`self.layout` matches
the `ArrayLayout` of the preceding allocation.". The strictest
interpretation of this is that you can't change the alignment (even
increasing it by multiplying it) or length (capacity). I assume that's
what "match" means, which would prevent any implementation of `try_cast`
here, although AFAICT the current allocator implementations don't
necessarily preclude it.

So, if the above can't be done I will use your EncodedStream diff.

thanks~