[PATCH] nova-core: vbios: Rework BiosImage composition to use AsRef and traits

[PATCH] nova-core: vbios: Rework BiosImage composition to use AsRef and traits

[Joel Fernandes]

Currently, the BiosImage type in vbios code is implemented as a
type-wrapping enum with the sole purpose of implementing a type that is
common to all specific image types. To make this work, macros were used
to overcome limitations of using enums. Ugly match statements were also
required to route methods from the enum type to the specific image type.

Simplify the code by using AsRef to convert references from specific
image types to the common BiosImage type to specific image types.
Introduce a new BiosImageTrait trait to implement common methods and
route the methods using AsRef. This allows usage of common code that
applies to all image types.  This is a lot cleaner and does not require
macros or type-wrapping enums. The code is much more readable now.

Add the following new types:
BiosImageType: A regular non-type-wrapping enum for the image type.
BiosImage: A common struct embedded into specific bios image types (used
to be BiosImageBase).
BiosImageTrait: A trait that helps to route methods from a specific
image type to common BiosImage typ.

Remove the now obsolete BiosImage enum.

Cc: Benno Lossin <lossin@kernel.org>
Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
---
Rebased on drm-rust-next branch.

 drivers/gpu/nova-core/vbios.rs | 255 +++++++++++++++++----------------
 1 file changed, 134 insertions(+), 121 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index e6a060714205..548c24d9f70b 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -21,6 +21,34 @@
 /// indicates the last image. Bit 0-6 are reserved, bit 7 is last image bit.
 const LAST_IMAGE_BIT_MASK: u8 = 0x80;
 
+/// BIOS Image Type from PCI Data Structure code_type field.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[repr(u8)]
+enum BiosImageType {
+    /// PC-AT compatible BIOS image (x86 legacy)
+    PciAt = 0x00,
+    /// EFI (Extensible Firmware Interface) BIOS image
+    Efi = 0x03,
+    /// NBSI (Notebook System Information) BIOS image
+    Nbsi = 0x70,
+    /// FwSec (Firmware Security) BIOS image
+    FwSec = 0xE0,
+}
+
+impl TryFrom<u8> for BiosImageType {
+    type Error = Error;
+
+    fn try_from(code: u8) -> Result<Self> {
+        match code {
+            0x00 => Ok(Self::PciAt),
+            0x03 => Ok(Self::Efi),
+            0x70 => Ok(Self::Nbsi),
+            0xE0 => Ok(Self::FwSec),
+            _ => Err(EINVAL),
+        }
+    }
+}
+
 // PMU lookup table entry types. Used to locate PMU table entries
 // in the Fwsec image, corresponding to falcon ucodes.
 #[expect(dead_code)]
@@ -197,32 +225,37 @@ pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
 
         // Parse all VBIOS images in the ROM
         for image_result in VbiosIterator::new(dev, bar0)? {
-            let full_image = image_result?;
+            let image = image_result?;
 
             dev_dbg!(
                 dev,
-                "Found BIOS image: size: {:#x}, type: {}, last: {}\n",
-                full_image.image_size_bytes(),
-                full_image.image_type_str(),
-                full_image.is_last()
+                "Found BIOS image: size: {:#x}, type: {:?}, last: {}\n",
+                image.image_size_bytes(),
+                image.image_type(),
+                image.is_last()
             );
 
-            // Get references to images we will need after the loop, in order to
-            // setup the falcon data offset.
-            match full_image {
-                BiosImage::PciAt(image) => {
-                    pci_at_image = Some(image);
+            // Convert to a specific image type
+            match BiosImageType::try_from(image.pcir.code_type) {
+                Ok(BiosImageType::PciAt) => {
+                    pci_at_image = Some(PciAtBiosImage::try_from(image)?);
                 }
-                BiosImage::FwSec(image) => {
+                Ok(BiosImageType::FwSec) => {
+                    let fwsec = FwSecBiosBuilder {
+                        base: image,
+                        falcon_data_offset: None,
+                        pmu_lookup_table: None,
+                        falcon_ucode_offset: None,
+                    };
                     if first_fwsec_image.is_none() {
-                        first_fwsec_image = Some(image);
+                        first_fwsec_image = Some(fwsec);
                     } else {
-                        second_fwsec_image = Some(image);
+                        second_fwsec_image = Some(fwsec);
                     }
                 }
-                // For now we don't need to handle these
-                BiosImage::Efi(_image) => {}
-                BiosImage::Nbsi(_image) => {}
+                _ => {
+                    // Ignore other image types or unknown types
+                }
             }
         }
 
@@ -594,108 +627,72 @@ fn find_in_data(
     }
 }
 
-// Use a macro to implement BiosImage enum and methods. This avoids having to
-// repeat each enum type when implementing functions like base() in BiosImage.
-macro_rules! bios_image {
-    (
-        $($variant:ident: $class:ident),* $(,)?
-    ) => {
-        // BiosImage enum with variants for each image type
-        enum BiosImage {
-            $($variant($class)),*
-        }
-
-        impl BiosImage {
-            /// Get a reference to the common BIOS image data regardless of type
-            fn base(&self) -> &BiosImageBase {
-                match self {
-                    $(Self::$variant(img) => &img.base),*
-                }
-            }
-
-            /// Returns a string representing the type of BIOS image
-            fn image_type_str(&self) -> &'static str {
-                match self {
-                    $(Self::$variant(_) => stringify!($variant)),*
-                }
-            }
-        }
-    }
-}
-
-impl BiosImage {
+/// Trait for all BIOS image types
+#[expect(dead_code)]
+trait BiosImageTrait: AsRef<BiosImage> {
     /// Check if this is the last image.
     fn is_last(&self) -> bool {
-        let base = self.base();
-
-        // For NBSI images (type == 0x70), return true as they're
-        // considered the last image
-        if matches!(self, Self::Nbsi(_)) {
-            return true;
-        }
-
-        // For other image types, check the NPDE first if available
-        if let Some(ref npde) = base.npde {
-            return npde.is_last();
-        }
-
-        // Otherwise, fall back to checking the PCIR last_image flag
-        base.pcir.is_last()
+        self.as_ref().is_last()
     }
 
     /// Get the image size in bytes.
     fn image_size_bytes(&self) -> usize {
-        let base = self.base();
-
-        // Prefer NPDE image size if available
-        if let Some(ref npde) = base.npde {
-            return npde.image_size_bytes();
-        }
-
-        // Otherwise, fall back to the PCIR image size
-        base.pcir.image_size_bytes()
+        self.as_ref().image_size_bytes()
     }
 
-    /// Create a [`BiosImageBase`] from a byte slice and convert it to a [`BiosImage`] which
-    /// triggers the constructor of the specific BiosImage enum variant.
-    fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
-        let base = BiosImageBase::new(dev, data)?;
-        let image = base.into_image().inspect_err(|e| {
-            dev_err!(dev, "Failed to create BiosImage: {:?}\n", e);
-        })?;
-
-        Ok(image)
+    /// Get the BIOS image type.
+    fn image_type(&self) -> Result<BiosImageType> {
+        self.as_ref().image_type()
     }
 }
 
-bios_image! {
-    PciAt: PciAtBiosImage,   // PCI-AT compatible BIOS image
-    Efi: EfiBiosImage,       // EFI (Extensible Firmware Interface)
-    Nbsi: NbsiBiosImage,     // NBSI (Nvidia Bios System Interface)
-    FwSec: FwSecBiosBuilder, // FWSEC (Firmware Security)
-}
-
 /// The PciAt BIOS image is typically the first BIOS image type found in the BIOS image chain.
 ///
 /// It contains the BIT header and the BIT tokens.
 struct PciAtBiosImage {
-    base: BiosImageBase,
+    base: BiosImage,
     bit_header: BitHeader,
     bit_offset: usize,
 }
 
+impl AsRef<BiosImage> for PciAtBiosImage {
+    fn as_ref(&self) -> &BiosImage {
+        &self.base
+    }
+}
+
+impl BiosImageTrait for PciAtBiosImage {}
+
+#[expect(dead_code)]
 struct EfiBiosImage {
-    base: BiosImageBase,
+    base: BiosImage,
     // EFI-specific fields can be added here in the future.
 }
 
+impl AsRef<BiosImage> for EfiBiosImage {
+    fn as_ref(&self) -> &BiosImage {
+        &self.base
+    }
+}
+
+impl BiosImageTrait for EfiBiosImage {}
+
+#[expect(dead_code)]
 struct NbsiBiosImage {
-    base: BiosImageBase,
+    base: BiosImage,
     // NBSI-specific fields can be added here in the future.
 }
 
+impl AsRef<BiosImage> for NbsiBiosImage {
+    fn as_ref(&self) -> &BiosImage {
+        &self.base
+    }
+}
+
+impl BiosImageTrait for NbsiBiosImage {}
+
 struct FwSecBiosBuilder {
-    base: BiosImageBase,
+    base: BiosImage,
     /// These are temporary fields that are used during the construction of the
     /// [`FwSecBiosBuilder`].
     ///
@@ -710,41 +707,28 @@ struct FwSecBiosBuilder {
     falcon_ucode_offset: Option<usize>,
 }
 
+impl AsRef<BiosImage> for FwSecBiosBuilder {
+    fn as_ref(&self) -> &BiosImage {
+        &self.base
+    }
+}
+
+impl BiosImageTrait for FwSecBiosBuilder {}
+
 /// The [`FwSecBiosImage`] structure contains the PMU table and the Falcon Ucode.
 ///
 /// The PMU table contains voltage/frequency tables as well as a pointer to the Falcon Ucode.
 pub(crate) struct FwSecBiosImage {
-    base: BiosImageBase,
+    base: BiosImage,
     /// The offset of the Falcon ucode.
     falcon_ucode_offset: usize,
 }
 
-// Convert from BiosImageBase to BiosImage
-impl TryFrom<BiosImageBase> for BiosImage {
-    type Error = Error;
-
-    fn try_from(base: BiosImageBase) -> Result<Self> {
-        match base.pcir.code_type {
-            0x00 => Ok(BiosImage::PciAt(base.try_into()?)),
-            0x03 => Ok(BiosImage::Efi(EfiBiosImage { base })),
-            0x70 => Ok(BiosImage::Nbsi(NbsiBiosImage { base })),
-            0xE0 => Ok(BiosImage::FwSec(FwSecBiosBuilder {
-                base,
-                falcon_data_offset: None,
-                pmu_lookup_table: None,
-                falcon_ucode_offset: None,
-            })),
-            _ => Err(EINVAL),
-        }
-    }
-}
-
 /// BIOS Image structure containing various headers and reference fields to all BIOS images.
 ///
-/// Each BiosImage type has a BiosImageBase type along with other image-specific fields. Note that
-/// Rust favors composition of types over inheritance.
+/// A BiosImage struct is embedded into all image types and implements common operations.
 #[expect(dead_code)]
-struct BiosImageBase {
+struct BiosImage {
     /// Used for logging.
     dev: ARef<device::Device>,
     /// PCI ROM Expansion Header
@@ -757,12 +741,41 @@ struct BiosImageBase {
     data: KVec<u8>,
 }
 
-impl BiosImageBase {
-    fn into_image(self) -> Result<BiosImage> {
-        BiosImage::try_from(self)
+impl BiosImage {
+    /// Get the image size in bytes.
+    fn image_size_bytes(&self) -> usize {
+        // Prefer NPDE image size if available
+        if let Some(ref npde) = self.npde {
+            npde.image_size_bytes()
+        } else {
+            // Otherwise, fall back to the PCIR image size
+            self.pcir.image_size_bytes()
+        }
+    }
+
+    /// Get the BIOS image type.
+    fn image_type(&self) -> Result<BiosImageType> {
+        BiosImageType::try_from(self.pcir.code_type)
+    }
+
+    /// Check if this is the last image.
+    fn is_last(&self) -> bool {
+        // For NBSI images (type == 0x70), return true as they're
+        // considered the last image
+        if self.pcir.code_type == BiosImageType::Nbsi as u8 {
+            return true;
+        }
+
+        // For other image types, check the NPDE first if available
+        if let Some(ref npde) = self.npde {
+            return npde.is_last();
+        }
+
+        // Otherwise, fall back to checking the PCIR last_image flag
+        self.pcir.is_last()
     }
 
-    /// Creates a new BiosImageBase from raw byte data.
+    /// Creates a new BiosImage from raw byte data.
     fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
         // Ensure we have enough data for the ROM header.
         if data.len() < 26 {
@@ -802,7 +815,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
         let mut data_copy = KVec::new();
         data_copy.extend_from_slice(data, GFP_KERNEL)?;
 
-        Ok(BiosImageBase {
+        Ok(BiosImage {
             dev: dev.into(),
             rom_header,
             pcir,
@@ -865,10 +878,10 @@ fn falcon_data_ptr(&self) -> Result<u32> {
     }
 }
 
-impl TryFrom<BiosImageBase> for PciAtBiosImage {
+impl TryFrom<BiosImage> for PciAtBiosImage {
     type Error = Error;
 
-    fn try_from(base: BiosImageBase) -> Result<Self> {
+    fn try_from(base: BiosImage) -> Result<Self> {
         let data_slice = &base.data;
         let (bit_header, bit_offset) = PciAtBiosImage::find_bit_header(data_slice)?;
 

base-commit: 299eb32863e584cfff7c6b667c3e92ae7d4d2bf9
-- 
2.34.1

Re: [PATCH] nova-core: vbios: Rework BiosImage composition to use AsRef and traits

[Alexandre Courbot]

Hi Joel,

On Mon Oct 6, 2025 at 7:08 AM JST, Joel Fernandes wrote:
<snip>
> +/// Trait for all BIOS image types
> +#[expect(dead_code)]
> +trait BiosImageTrait: AsRef<BiosImage> {

I see that this trait is implemented 4 times, but it has no user.
Actually I can remove the trait definition, its implementations, and the
`AsRef` implementations added by this patch, and the code builds just
fine - only with 50 less lines of code. Is this needed at all?

Re: [PATCH] nova-core: vbios: Rework BiosImage composition to use AsRef and traits

[Joel Fernandes]

On Mon, Oct 06, 2025 at 03:01:42PM +0900, Alexandre Courbot wrote:
> Hi Joel,
> 
> On Mon Oct 6, 2025 at 7:08 AM JST, Joel Fernandes wrote:
> <snip>
> > +/// Trait for all BIOS image types
> > +#[expect(dead_code)]
> > +trait BiosImageTrait: AsRef<BiosImage> {
> 
> I see that this trait is implemented 4 times, but it has no user.
> Actually I can remove the trait definition, its implementations, and the
> `AsRef` implementations added by this patch, and the code builds just
> fine - only with 50 less lines of code. Is this needed at all?
> 

Indeed! So in the previous edition of this code, the pattern was: iterate
over specific image types and then call the common methods, from the main
loop. Now the pattern is iterate over the common type and firectly call the
common methods.

So I inadvertently simplified the usecase thus removing the need for the
"function routing" in the first place!

That is good news, so I will just submit a new patch reworking this without
the new traits and AsRef.

That will result in 50 lines lesser as you mentioned! So please discard
this patch, thanks! Should we need routing to common methods, we can always
add that in the future.

(Just to note, we can discard another 20 or so lines once we have the support
to generate the enum TryFrokm boilerplate).