On 2026-06-29 at 17:36 +1000, Alexandre Courbot <[email protected]> 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 <[email protected]>
>
> 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/[email protected]/
>
> <...>
> > #[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?
