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

["Alexandre Courbot" <acourbot@nvidia.com>]

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`?

<...>
> --- 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.

> 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.

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`?

<...>
> --- 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?