Re: [PATCH] gpu: nova-core: parse structs via zerocopy

[Alistair Popple <apopple@nvidia.com>]

On 2026-06-22 at 00:36 +1000, Nicolás Antinori <nico.antinori.7@gmail.com> wrote...
> Replace the unsafe `kernel::transmute::FromBytes` trait implementation
> for the `FalconUCodeDescV3`, `PcirStruct`, `BitHeader`, `NpdeStruct`,
> and `PmuLookupTableHeader` structs with the derivable
> `zerocopy::FromBytes` trait.
> 
> This change eliminates the manual unsafe implementations in favor of a
> derivable trait. When this trait is derived, validity checks are
> performed at compile time to make sure that the type can safely
> implement `FromBytes`.

Nice, this looks good although due to some code movement this doesn't compile
when applied to drm-rust-next[1]. Would you mind rebasing on top of that? The
changes required should be relatively straight forward, they are mostly the
result of some new users of transmute::FromBytes being added to vbios.rs.

Also what is you plan here? I have also been looking at some of these
conversions, specifically those involving the generated bindings. Really
appreciate your contributions, so no problem if you want to continue with some
of the conversions, just want to make sure we aren't duplicating effort here.

 - Alistair

[1] - https://gitlab.freedesktop.org/drm/rust/kernel/-/tree/drm-rust-next

> Link: https://github.com/Rust-for-Linux/linux/issues/1241
> Suggested-by: Miguel Ojeda <ojeda@kernel.org>
> Signed-off-by: Nicolás Antinori <nico.antinori.7@gmail.com>
> ---
> NOTE: Compile-tested only. I don't own a piece of hardware where I can
> test the nova driver.
> 
>  drivers/gpu/nova-core/firmware.rs |  6 +----
>  drivers/gpu/nova-core/vbios.rs    | 38 ++++++++-----------------------
>  2 files changed, 11 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index ad37994ac15a..0afd1d3fe5ad 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -86,7 +86,7 @@ pub(crate) struct FalconUCodeDescV2 {
> 
>  /// Structure used to describe some firmwares, notably FWSEC-FRTS.
>  #[repr(C)]
> -#[derive(Debug, Clone)]
> +#[derive(Debug, Clone, FromBytes)]
>  pub(crate) struct FalconUCodeDescV3 {
>      /// Header defined by `NV_BIT_FALCON_UCODE_DESC_HEADER_VDESC*` in OpenRM.
>      hdr: u32,
> @@ -117,10 +117,6 @@ pub(crate) struct FalconUCodeDescV3 {
>      _reserved: u16,
>  }
> 
> -// SAFETY: all bit patterns are valid for this type, and it doesn't use
> -// interior mutability.
> -unsafe impl FromBytes for FalconUCodeDescV3 {}
> -
>  /// Enum wrapping the different versions of Falcon microcode descriptors.
>  ///
>  /// This allows handling both V2 and V3 descriptor formats through a
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 8b7d17a24660..754812bcbdde 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -13,11 +13,8 @@
>          Alignment, //
>      },
>      sync::aref::ARef,
> -    transmute::FromBytes,
>  };
> 
> -use zerocopy::FromBytes as _;
> -
>  use crate::{
>      driver::Bar0,
>      firmware::{
> @@ -301,7 +298,7 @@ pub(crate) fn fwsec_image(&self) -> &FwSecBiosImage {
>  }
> 
>  /// PCI Data Structure as defined in PCI Firmware Specification
> -#[derive(Debug, Clone)]
> +#[derive(Debug, Clone, FromBytes)]
>  #[repr(C)]
>  struct PcirStruct {
>      /// PCI Data Structure signature ("PCIR" or "NPDS")
> @@ -330,12 +327,9 @@ struct PcirStruct {
>      max_runtime_image_len: u16,
>  }
> 
> -// SAFETY: all bit patterns are valid for `PcirStruct`.
> -unsafe impl FromBytes for PcirStruct {}
> -
>  impl PcirStruct {
>      fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
> -        let (pcir, _) = PcirStruct::from_bytes_copy_prefix(data).ok_or(EINVAL)?;
> +        let (pcir, _) = PcirStruct::read_from_prefix(data).map_err(|_| EINVAL)?;
> 
>          // Signature should be "PCIR" (0x52494350) or "NPDS" (0x5344504e).
>          if &pcir.signature != b"PCIR" && &pcir.signature != b"NPDS" {
> @@ -371,7 +365,7 @@ fn image_size_bytes(&self) -> usize {
>  /// This is the head of the BIT table, that is used to locate the Falcon data. The BIT table (with
>  /// its header) is in the [`PciAtBiosImage`] and the falcon data it is pointing to is in the
>  /// [`FwSecBiosImage`].
> -#[derive(Debug, Clone, Copy)]
> +#[derive(Debug, Clone, Copy, FromBytes)]
>  #[repr(C)]
>  struct BitHeader {
>      /// 0h: BIT Header Identifier (BMP=0x7FFF/BIT=0xB8FF)
> @@ -390,12 +384,9 @@ struct BitHeader {
>      checksum: u8,
>  }
> 
> -// SAFETY: all bit patterns are valid for `BitHeader`.
> -unsafe impl FromBytes for BitHeader {}
> -
>  impl BitHeader {
>      fn new(data: &[u8]) -> Result<Self> {
> -        let (header, _) = BitHeader::from_bytes_copy_prefix(data).ok_or(EINVAL)?;
> +        let (header, _) = BitHeader::read_from_prefix(data).map_err(|_| EINVAL)?;
> 
>          // Check header ID and signature
>          if header.id != 0xB8FF || &header.signature != b"BIT\0" {
> @@ -533,7 +524,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>  /// PCI Data Structure. It contains some fields that are redundant with the PCI Data Structure, but
>  /// are needed for traversing the BIOS images. It is expected to be present in all BIOS images
>  /// except for NBSI images.
> -#[derive(Debug, Clone)]
> +#[derive(Debug, Clone, FromBytes)]
>  #[repr(C)]
>  struct NpdeStruct {
>      /// 00h: Signature ("NPDE")
> @@ -548,12 +539,9 @@ struct NpdeStruct {
>      last_image: u8,
>  }
> 
> -// SAFETY: all bit patterns are valid for `NpdeStruct`.
> -unsafe impl FromBytes for NpdeStruct {}
> -
>  impl NpdeStruct {
>      fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
> -        let (npde, _) = NpdeStruct::from_bytes_copy_prefix(data)?;
> +        let (npde, _) = NpdeStruct::read_from_prefix(data).ok()?;
> 
>          // Signature should be "NPDE" (0x4544504E).
>          if &npde.signature != b"NPDE" {
> @@ -845,6 +833,7 @@ fn new(data: &[u8]) -> Result<Self> {
>  }
> 
>  #[repr(C)]
> +#[derive(FromBytes)]
>  struct PmuLookupTableHeader {
>      version: u8,
>      header_len: u8,
> @@ -852,9 +841,6 @@ struct PmuLookupTableHeader {
>      entry_count: u8,
>  }
> 
> -// SAFETY: all bit patterns are valid for `PmuLookupTableHeader`.
> -unsafe impl FromBytes for PmuLookupTableHeader {}
> -
>  /// The [`PmuLookupTableEntry`] structure is used to find the [`PmuLookupTableEntry`] for a given
>  /// application ID.
>  ///
> @@ -867,7 +853,7 @@ struct PmuLookupTable {
> 
>  impl PmuLookupTable {
>      fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
> -        let (header, _) = PmuLookupTableHeader::from_bytes_copy_prefix(data).ok_or(EINVAL)?;
> +        let (header, _) = PmuLookupTableHeader::read_from_prefix(data).map_err(|_| EINVAL)?;
> 
>          let header_len = usize::from(header.header_len);
>          let entry_len = usize::from(header.entry_len);
> @@ -1013,15 +999,11 @@ pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
>          let data = self.base.data.get(falcon_ucode_offset..).ok_or(EINVAL)?;
>          match ver {
>              2 => {
> -                let v2 = FalconUCodeDescV2::read_from_prefix(data)
> -                    .map_err(|_| EINVAL)?
> -                    .0;
> +                let (v2, _) = FalconUCodeDescV2::read_from_prefix(data).map_err(|_| EINVAL)?;
>                  Ok(FalconUCodeDesc::V2(v2))
>              }
>              3 => {
> -                let v3 = FalconUCodeDescV3::from_bytes_copy_prefix(data)
> -                    .ok_or(EINVAL)?
> -                    .0;
> +                let (v3, _) = FalconUCodeDescV3::read_from_prefix(data).map_err(|_| EINVAL)?;
>                  Ok(FalconUCodeDesc::V3(v3))
>              }
>              _ => {
> --
> 2.47.3
>