Re: [PATCH 1/1] nova-core: Update firmware bindings to use zerocopy traits

[Alistair Popple <apopple@nvidia.com>]

On 2026-06-29 at 17:36 +1000, Alexandre Courbot <acourbot@nvidia.com> wrote...
> On Mon Jun 29, 2026 at 11:52 AM JST, Alistair Popple wrote:
> > Currently most of nova-core uses unsafe implementations of AsBytes and
> > FromBytes in order to read and write GSP commands using the generated
> > bindings. Whilst this is generally safe in practice there is nothing
> > that actually guarantees or checks this.
> >
> > This is exactly what the zerocopy library was introduced to do - provide
> > compile-time checks ensuring From/AsBytes is safe. Make use of these
> > checks by converting all our generated bindings to derive the required
> > traits for the zerocopy checks, removing many unsafe implementations in
> > the process.
> >
> > Note this does require the use of an unstable feature - trivial_bounds
> > - as some bindings end up needing to derive zerocopy::Unaligned.
> > A work-around is also required in the bindings to make some of the
> > __IncompleteArrayField<T> ZSTs monomorphic as the check macro can not
> > prove a generic type is aligned and thus forces T to derive
> > zerocopy::Unaligned, which is not satisfied by all types.
> >
> > Signed-off-by: Alistair Popple <apopple@nvidia.com>
> 
> I have counted and this removes 28 unsafe statements. :) (it also adds
> 2 tbf)
> 
> > ---
> >  drivers/gpu/nova-core/Makefile                |   4 +
> >  drivers/gpu/nova-core/gsp/cmdq.rs             |  21 +-
> >  .../gpu/nova-core/gsp/cmdq/continuation.rs    |  16 +-
> >  drivers/gpu/nova-core/gsp/commands.rs         |  19 +-
> >  drivers/gpu/nova-core/gsp/fw.rs               |  54 +----
> >  drivers/gpu/nova-core/gsp/fw/commands.rs      |  50 ++---
> >  drivers/gpu/nova-core/gsp/fw/r570_144.rs      |  41 ++++
> >  .../gpu/nova-core/gsp/fw/r570_144/bindings.rs | 185 ++++++++++++------
> >  drivers/gpu/nova-core/gsp/sequencer.rs        |   5 +-
> >  scripts/Makefile.build                        |   4 +-
> >  10 files changed, 223 insertions(+), 176 deletions(-)
> >
> > diff --git a/drivers/gpu/nova-core/Makefile b/drivers/gpu/nova-core/Makefile
> > index 4ae544f808f4..27a5752c4cf9 100644
> > --- a/drivers/gpu/nova-core/Makefile
> > +++ b/drivers/gpu/nova-core/Makefile
> > @@ -1,4 +1,8 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> > +# The GSP firmware bindings derive zerocopy's `IntoBytes` on `#[repr(C)]`
> > +# unions, which is gated behind the `zerocopy_derive_union_into_bytes` cfg.
> > +rustflags-y += --cfg zerocopy_derive_union_into_bytes
> > +
> >  obj-$(CONFIG_NOVA_CORE) += nova-core.o
> >  nova-core-y := nova_core.o
> > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> > index 0671ee8a9960..6e79ec688983 100644
> > --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> > @@ -29,6 +29,14 @@
> >      },
> >  };
> >  
> > +// TODO: Remove `as` once FromBytes is removed completely
> > +use zerocopy::{
> > +    FromBytes as ZCFromBytes,
> 
> Since zerocopy's `FromBytes` is the one we will keep long-term, should
> we rather rename the one from `transmute`?

Good idea - it ended up this way because I was going to split the series up to
convert one type at a time but ended up squashing it into one big change given
the mechanical nature of the conversions.

> <...>
> > --- a/drivers/gpu/nova-core/gsp/fw/r570_144.rs
> > +++ b/drivers/gpu/nova-core/gsp/fw/r570_144.rs
> > @@ -23,9 +23,50 @@
> >  )]
> >  use kernel::ffi;
> >  use pin_init::MaybeZeroable;
> > +use zerocopy_derive::{
> > +    FromBytes,
> > +    Immutable,
> > +    IntoBytes,
> > +    KnownLayout, //
> > +};
> >  
> >  include!("r570_144/bindings.rs");
> >  
> >  // SAFETY: This type has a size of zero, so its inclusion into another type should not affect their
> >  // ability to implement `Zeroable`.
> >  unsafe impl<T> kernel::prelude::Zeroable for __IncompleteArrayField<T> {}
> > +
> > +/// Monomorphic version of [`__IncompleteArrayField<PACKED_REGISTRY_ENTRY>`].
> > +///
> > +/// zerocopy's `IntoBytes` derive can only run its compile-time no-padding check
> > +/// on a concrete type; for the generic `__IncompleteArrayField<T>` it falls back
> > +/// to requiring every field be `Unaligned`, which `PACKED_REGISTRY_ENTRY`
> > +/// does not satisfy. Specializing to a concrete type lets the derive succeed.
> > +#[repr(C)]
> > +#[derive(Debug, Default, FromBytes, Immutable, IntoBytes, KnownLayout, MaybeZeroable)]
> > +pub struct __IncompletePackedRegistryEntry(
> > +    ::core::marker::PhantomData<PACKED_REGISTRY_ENTRY>,
> > +    [PACKED_REGISTRY_ENTRY; 0],
> > +);
> > +impl __IncompletePackedRegistryEntry {
> > +    #[inline]
> > +    pub const fn new() -> Self {
> > +        __IncompletePackedRegistryEntry(::core::marker::PhantomData, [])
> > +    }
> > +    #[inline]
> > +    pub fn as_ptr(&self) -> *const PACKED_REGISTRY_ENTRY {
> > +        self as *const _ as *const PACKED_REGISTRY_ENTRY
> > +    }
> > +    #[inline]
> > +    pub fn as_mut_ptr(&mut self) -> *mut PACKED_REGISTRY_ENTRY {
> > +        self as *mut _ as *mut PACKED_REGISTRY_ENTRY
> > +    }
> > +    #[inline]
> > +    pub unsafe fn as_slice(&self, len: usize) -> &[PACKED_REGISTRY_ENTRY] {
> > +        ::core::slice::from_raw_parts(self.as_ptr(), len)
> > +    }
> > +    #[inline]
> > +    pub unsafe fn as_mut_slice(&mut self, len: usize) -> &mut [PACKED_REGISTRY_ENTRY] {
> > +        ::core::slice::from_raw_parts_mut(self.as_mut_ptr(), len)
> > +    }
> > +}
> 
> Yikes, this is a one-shot so I guess we can live with that if neeeded.

Yes, this is a total hack but I couldn't figure out a better alternative.
Given this is the only type that has this problem I figured it was a bearable
trade-off to make, but am open to better suggestions if there are any.

> > diff --git a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
> > index ea350f9b2cc4..9c85c93f6eee 100644
> > --- a/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
> > +++ b/drivers/gpu/nova-core/gsp/fw/r570_144/bindings.rs
> > @@ -1,7 +1,7 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  
> >  #[repr(C)]
> > -#[derive(Default)]
> > +#[derive(Default, FromBytes, IntoBytes, Immutable, KnownLayout)]
> 
> This is the part my other email [1] was about: are we going, long-term,
> to replace this with a `#[derive(zerocopy_derive::most_traits)]`? If the
> Nova's bindings generator program can take care of producing the correct
> traits for each generated type then maybe that's even better, but at the
> same time I am wondering whether the kernel's bindgen invocation isn't
> going to automatically derive `most_traits` on all generated types. In
> this case, we could end up with duplicate implementations.

If that's coming soon I think it makes sense to wait simply to avoid churn
on the generated bindings. I didn't do anything too smart for Nova's binding
generator - it basically just derives all these traits for all our bindings.
Being able to have it derive `most_traits` would be cleaner. So happy to rebase
once that is merged.

> I think this point needs to be clarified before we can decide when to
> merge this patch.
> 
> [1] https://lore.kernel.org/all/DJLCF3LR0KMN.1TMKMZEZWKN8O@nvidia.com/
> 
> <...>
> >  #[repr(C)]
> > -#[derive(Debug, Default, MaybeZeroable)]
> > +#[derive(Debug, Default, MaybeZeroable, FromBytes, Immutable, KnownLayout, IntoBytes)]
> >  pub struct PACKED_REGISTRY_TABLE {
> >      pub size: u32_,
> >      pub numEntries: u32_,
> > -    pub entries: __IncompleteArrayField<PACKED_REGISTRY_ENTRY>,
> > +    pub entries: __IncompletePackedRegistryEntry,
> 
> This is part of the generated bindings, right? How does the generator
> program know it needs to use `__IncompletePackedRegistryEntry`?

Maigc[1] ... otherwise known as `sed` :-)

[1] - https://github.com/apopple-nvidia/nova-gsp-binding-generator/blob/zerocopy/Kbuild#L153

> <...>
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -314,10 +314,12 @@ $(obj)/%.lst: $(obj)/%.c FORCE
> >  #   - Stable since Rust 1.89.0: `feature(generic_arg_infer)`.
> >  #   - Expected to become stable: `feature(arbitrary_self_types)`.
> >  #   - To be determined: `feature(used_with_arg)`.
> > +#   - Required by nova-core's zerocopy firmware bindings, whose derives emit
> > +#     trivial `where` bounds: `feature(trivial_bounds)`.
> >  #
> >  # Please see https://github.com/Rust-for-Linux/linux/issues/2 for details on
> >  # the unstable features in use.
> > -rust_allowed_features := arbitrary_self_types,asm_goto,generic_arg_infer,used_with_arg
> > +rust_allowed_features := arbitrary_self_types,asm_goto,generic_arg_infer,used_with_arg,trivial_bounds
> 
> This also might be something that will land soon through the Rust tree;
> Miguel, do you know what the plan is by any chance?