gpu: nova-core: vbios: harden various array accesses and refactor

Use checked access in FwSecBiosImage::header for getting the header version since the value is firmware derived.

Fixes 47c4846e4319 ("gpu: nova-core: vbios: Add support for FWSEC ucode extraction")

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

drivers/gpu/nova-core/vbios.rs +5 -5

index 632c8a90ea76..bc752d135cbf 100644
@@ -996,17 +996,14 @@ fn build(self) -> Result<FwSecBiosImage> {
 impl FwSecBiosImage {
     /// Get the FwSec header ([`FalconUCodeDesc`]).
     pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
-        // Get the falcon ucode offset that was found in setup_falcon_data.
-        let falcon_ucode_offset = self.falcon_ucode_offset;
+        let data = self
+            .base
+            .data
+            .get(self.falcon_ucode_offset..)
+            .ok_or(EINVAL)?;
 
-        // Read the first 4 bytes to get the version.
-        let hdr_bytes: [u8; 4] = self.base.data[falcon_ucode_offset..falcon_ucode_offset + 4]
-            .try_into()

Use checked arithmetic for ucode_offset in setup_falcon_data. This prevents a malformed firmware from causing a panic.

Fixes dc70c6ae2441 ("gpu: nova-core: vbios: Add support to look up PMU table in FWSEC")

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

drivers/gpu/nova-core/vbios.rs +9 -8

index bc752d135cbf..d8633e61178b 100644
@@ -955,14 +955,15 @@ fn setup_falcon_data(
             .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
         {
             Ok(entry) => {
-                let mut ucode_offset = usize::from_safe_cast(entry.data);
-                ucode_offset -= pci_at_image.base.data.len();
-                if ucode_offset < first_fwsec.base.data.len() {
-                    dev_err!(self.base.dev, "Falcon Ucode offset not in second Fwsec.\n");
-                    return Err(EINVAL);
-                }
-                ucode_offset -= first_fwsec.base.data.len();
-                self.falcon_ucode_offset = Some(ucode_offset);
+                self.falcon_ucode_offset = Some(
+                    usize::from_safe_cast(entry.data)
+                        .checked_sub(pci_at_image.base.data.len())
+                        .and_then(|o| o.checked_sub(first_fwsec.base.data.len()))
+                        .ok_or(EINVAL)
+                        .inspect_err(|_| {
+                            dev_err!(self.base.dev, "Falcon Ucode offset not in second Fwsec.\n");
+                        })?,
+                );
             }
             Err(e) => {
                 dev_err!(

This is unused, so we can remove it.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 5 -----
 1 file changed, 5 deletions(-)

drivers/gpu/nova-core/vbios.rs +0 -5

index d8633e61178b..d63af95eb642 100644
@@ -256,7 +256,6 @@ pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
                 Ok(BiosImageType::FwSec) => {
                     let fwsec = FwSecBiosBuilder {
                         base: image,
-                        falcon_data_offset: None,
                         pmu_lookup_table: None,
                         falcon_ucode_offset: None,
                     };

@@ -631,8 +630,6 @@ struct FwSecBiosBuilder {
     /// Once FwSecBiosBuilder is constructed, the `falcon_ucode_offset` will be copied into a new
     /// [`FwSecBiosImage`].
     ///
-    /// The offset of the Falcon data from the start of Fwsec image.
-    falcon_data_offset: Option<usize>,
     /// The [`PmuLookupTable`] starts at the offset of the falcon data pointer.
     pmu_lookup_table: Option<PmuLookupTable>,
     /// The offset of the Falcon ucode.

@@ -934,8 +931,6 @@ fn setup_falcon_data(
             offset -= first_fwsec.base.data.len();
         }
 
-        self.falcon_data_offset = Some(offset);
-
         if pmu_in_first_fwsec {
             self.pmu_lookup_table = Some(PmuLookupTable::new(
                 &self.base.dev,

This does not need to be stored, so we can remove it.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

drivers/gpu/nova-core/vbios.rs +6 -19

index d63af95eb642..01f65d50cbb3 100644
@@ -256,7 +256,6 @@ pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
                 Ok(BiosImageType::FwSec) => {
                     let fwsec = FwSecBiosBuilder {
                         base: image,
-                        pmu_lookup_table: None,
                         falcon_ucode_offset: None,
                     };
                     if first_fwsec_image.is_none() {

@@ -630,8 +629,6 @@ struct FwSecBiosBuilder {
     /// Once FwSecBiosBuilder is constructed, the `falcon_ucode_offset` will be copied into a new
     /// [`FwSecBiosImage`].
     ///
-    /// The [`PmuLookupTable`] starts at the offset of the falcon data pointer.
-    pmu_lookup_table: Option<PmuLookupTable>,
     /// The offset of the Falcon ucode.
     falcon_ucode_offset: Option<usize>,
 }

@@ -931,24 +928,14 @@ fn setup_falcon_data(
             offset -= first_fwsec.base.data.len();
         }
 
-        if pmu_in_first_fwsec {
-            self.pmu_lookup_table = Some(PmuLookupTable::new(
-                &self.base.dev,
-                &first_fwsec.base.data[offset..],
-            )?);
+        let pmu_lookup_data = if pmu_in_first_fwsec {
+            &first_fwsec.base.data[offset..]
         } else {
-            self.pmu_lookup_table = Some(PmuLookupTable::new(
-                &self.base.dev,
-                &self.base.data[offset..],
-            )?);
-        }
+            self.base.data.get(offset..).ok_or(EINVAL)?
+        };
+        let pmu_lookup_table = PmuLookupTable::new(&self.base.dev, pmu_lookup_data)?;
 
-        match self
-            .pmu_lookup_table
-            .as_ref()
-            .ok_or(EINVAL)?
-            .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
-        {
+        match pmu_lookup_table.find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD) {
             Ok(entry) => {
                 self.falcon_ucode_offset = Some(
                     usize::from_safe_cast(entry.data)

Push the computation of the falcon data offset into a helper function. The subtraction to create the offset should be checked, and by doing this the check can be folded into the existing check in falcon_data_ptr.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 48 +++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

drivers/gpu/nova-core/vbios.rs +0 -0

index 01f65d50cbb3..0c0e0402e715 100644

drivers/gpu/nova-core/vbios.rs +18 -28

     ///
-    /// This is just a 4 byte structure that contains a pointer to the Falcon data in the FWSEC
-    /// image.
-    fn falcon_data_ptr(&self) -> Result<u32> {
+    /// The BIT table contains a 4-byte pointer to the Falcon data. Testing shows this pointer
+    /// treats the PCI-AT and FWSEC images as logically contiguous even when an EFI image sits in
+    /// between them, so subtract the PCI-AT image size here to convert it to a FWSEC-relative
+    /// offset.
+    fn falcon_data_offset(&self) -> Result<usize> {
         let token = self.get_bit_token(BIT_TOKEN_ID_FALCON_DATA)?;
-
-        // Make sure we don't go out of bounds
-        if usize::from(token.data_offset) + 4 > self.base.data.len() {
-            return Err(EINVAL);
-        }
-
-        // read the 4 bytes at the offset specified in the token
         let offset = usize::from(token.data_offset);
-        let bytes: [u8; 4] = self.base.data[offset..offset + 4].try_into().map_err(|_| {
-            dev_err!(self.base.dev, "Failed to convert data slice to array\n");
-            EINVAL
-        })?;
 
-        let data_ptr = u32::from_le_bytes(bytes);
+        // Read the 4-byte falcon data pointer at the offset specified in the token.
+        let data = &self.base.data;
+        let (ptr, _) = data
+            .get(offset..)
+            .and_then(u32::from_bytes_copy_prefix)
+            .ok_or(EINVAL)?;
 
-        if (usize::from_safe_cast(data_ptr)) < self.base.data.len() {
-            dev_err!(self.base.dev, "Falcon data pointer out of bounds\n");
-            return Err(EINVAL);
-        }
-
-        Ok(data_ptr)
+        usize::from_safe_cast(ptr)
+            .checked_sub(data.len())
+            .ok_or(EINVAL)
+            .inspect_err(|_| {
+                dev_err!(self.base.dev, "Falcon data pointer out of bounds\n");
+            })
     }
 }
 

@@ -908,15 +904,9 @@ fn setup_falcon_data(
         pci_at_image: &PciAtBiosImage,
         first_fwsec: &FwSecBiosBuilder,
     ) -> Result {
-        let mut offset = usize::from_safe_cast(pci_at_image.falcon_data_ptr()?);
+        let mut offset = pci_at_image.falcon_data_offset()?;
         let mut pmu_in_first_fwsec = false;
 
-        // The falcon data pointer assumes that the PciAt and FWSEC images
-        // are contiguous in memory. However, testing shows the EFI image sits in
-        // between them. So calculate the offset from the end of the PciAt image
-        // rather than the start of it. Compensate.
-        offset -= pci_at_image.base.data.len();
-
         // The offset is now from the start of the first Fwsec image, however
         // the offset points to a location in the second Fwsec image. Since
         // the fwsec images are contiguous, subtract the length of the first Fwsec

Quoted 15 lines · Eliot Courtney

Push the computation of the falcon data offset into a helper function. The subtraction to create the offset should be checked, and by doing this the check can be folded into the existing check in falcon_data_ptr.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 48 +++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

drivers/gpu/nova-core/vbios.rs +0 -0

index 01f65d50cbb3..0c0e0402e715 100644

drivers/gpu/nova-core/vbios.rs fn get_bit_token(&self, token_id: u8) -> Result<BitToken> { · line 768

Quoted 5 lines

drivers/gpu/nova-core/vbios.rs +1 -1

@@ -765,33 +765,29 @@ fn get_bit_token(&self, token_id: u8) -> Result<BitToken> {
         BitToken::from_id(self, token_id)
     }
 
-    /// Find the Falcon data pointer structure in the [`PciAtBiosImage`].
+    /// Find the Falcon data offset from the start of the FWSEC region.

The comment change is incorrect, this subtraction is just for normalizing. It basically normalizes the pointer wrt the PciAt image.

It is only after the following in the caller that we get the true offset within the FWSEC.

            offset -= first_fwsec.base.data.len();

I suggest, let us rename falcon_data_offset() to falcon_normalize_fwsec_offset() and update the comment above.

Quoted 25 lines · Joel Fernandes

Quoted 15 lines · Eliot Courtney

Push the computation of the falcon data offset into a helper function. The subtraction to create the offset should be checked, and by doing this the check can be folded into the existing check in falcon_data_ptr.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 48 +++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

drivers/gpu/nova-core/vbios.rs +0 -0

index 01f65d50cbb3..0c0e0402e715 100644

drivers/gpu/nova-core/vbios.rs fn get_bit_token(&self, token_id: u8) -> Result<BitToken> { · line 768

Quoted 5 lines

drivers/gpu/nova-core/vbios.rs +1 -1

@@ -765,33 +765,29 @@ fn get_bit_token(&self, token_id: u8) -> Result<BitToken> {
         BitToken::from_id(self, token_id)
     }
 
-    /// Find the Falcon data pointer structure in the [`PciAtBiosImage`].
+    /// Find the Falcon data offset from the start of the FWSEC region.

The comment change is incorrect, this subtraction is just for normalizing. It basically normalizes the pointer wrt the PciAt image.

Quoted 6 lines

It is only after the following in the caller that we get the true offset within the FWSEC.

            offset -= first_fwsec.base.data.len();

I suggest, let us rename falcon_data_offset() to falcon_normalize_fwsec_offset() and update the comment above.

Thanks for your reviews! W.r.t. this, my understanding is that the layout is something like:

PCI-AT | Efi? | FWSEC | FWSEC

And that the falcon data pointer that we get out of PCI-AT starts off like this (indicated by ^):

^ PCI-AT | Efi? | FWSEC | FWSEC

But the actual "address space" it's in is:

^ PCI-AT | FWSEC | FWSEC

Because it doesn't count whatever images are between PCI-AT and the first FWSEC as part of that space. So by subtracting the PCI-AT size, we convert it to this logical space:

^ FWSEC | FWSEC

Based on the above understanding doesn't it make sense to say that falcon_data_offset transforms the pointer to be relative from the start of the FWSEC region? Once we subtract off the first fwsec image length, it's then relative to the second FWSEC image. Please LMK if I've missed something. We could also emphasise in the doc that the "FWSEC region" means the contiguous region defined by the first two FWSEC images. WDYT?

Quoted 53 lines · Eliot Courtney

Quoted 25 lines · Joel Fernandes

Quoted 15 lines · Eliot Courtney

Push the computation of the falcon data offset into a helper function. The subtraction to create the offset should be checked, and by doing this the check can be folded into the existing check in falcon_data_ptr.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 48 +++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

drivers/gpu/nova-core/vbios.rs +0 -0

index 01f65d50cbb3..0c0e0402e715 100644

drivers/gpu/nova-core/vbios.rs fn get_bit_token(&self, token_id: u8) -> Result<BitToken> { · line 768

Quoted 5 lines

drivers/gpu/nova-core/vbios.rs +1 -1

@@ -765,33 +765,29 @@ fn get_bit_token(&self, token_id: u8) -> Result<BitToken> {
         BitToken::from_id(self, token_id)
     }
 
-    /// Find the Falcon data pointer structure in the [`PciAtBiosImage`].
+    /// Find the Falcon data offset from the start of the FWSEC region.

The comment change is incorrect, this subtraction is just for normalizing. It basically normalizes the pointer wrt the PciAt image.

Quoted 6 lines

It is only after the following in the caller that we get the true offset within the FWSEC.

            offset -= first_fwsec.base.data.len();

I suggest, let us rename falcon_data_offset() to falcon_normalize_fwsec_offset() and update the comment above.

Thanks for your reviews! W.r.t. this, my understanding is that the layout is something like:

PCI-AT | Efi? | FWSEC | FWSEC

And that the falcon data pointer that we get out of PCI-AT starts off like this (indicated by ^):

^ PCI-AT | Efi? | FWSEC | FWSEC

But the actual "address space" it's in is:

^ PCI-AT | FWSEC | FWSEC

Because it doesn't count whatever images are between PCI-AT and the first FWSEC as part of that space. So by subtracting the PCI-AT size, we convert it to this logical space:

^ FWSEC | FWSEC

Quoted 7 lines

Based on the above understanding doesn't it make sense to say that falcon_data_offset transforms the pointer to be relative from the start of the FWSEC region? Once we subtract off the first fwsec image length, it's then relative to the second FWSEC image. Please LMK if I've missed something. We could also emphasise in the doc that the "FWSEC region" means the contiguous region defined by the first two FWSEC images. WDYT?

You did not miss anything, your explanation above is spot-on.

You could also keep your function name and add a comment on top of falcon_data_offset(). Something like "offset from the end of PCI-AT (i.e., the start of the combined FWSEC region)". Sounds good?

Remove pmu_in_first_fwsec and reorganize some code.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 59 +++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

drivers/gpu/nova-core/vbios.rs +26 -33

index 0c0e0402e715..d71ff5de794f 100644
@@ -904,48 +904,41 @@ fn setup_falcon_data(
         pci_at_image: &PciAtBiosImage,
         first_fwsec: &FwSecBiosBuilder,
     ) -> Result {
-        let mut offset = pci_at_image.falcon_data_offset()?;
-        let mut pmu_in_first_fwsec = false;
+        let offset = pci_at_image.falcon_data_offset()?;
 
-        // The offset is now from the start of the first Fwsec image, however
-        // the offset points to a location in the second Fwsec image. Since
-        // the fwsec images are contiguous, subtract the length of the first Fwsec
-        // image from the offset to get the offset to the start of the second
-        // Fwsec image.
-        if offset < first_fwsec.base.data.len() {
-            pmu_in_first_fwsec = true;
+        // The offset is from the start of the first FwSec image, but it
+        // may point into the second FwSec image. Treat the two FwSec images
+        // as contiguous here and subtract the first image length when the
+        // target lies in the second one.
+        let pmu_lookup_data = if offset < first_fwsec.base.data.len() {
+            first_fwsec.base.data.get(offset..)
         } else {
-            offset -= first_fwsec.base.data.len();
-        }
-
-        let pmu_lookup_data = if pmu_in_first_fwsec {
-            &first_fwsec.base.data[offset..]
-        } else {
-            self.base.data.get(offset..).ok_or(EINVAL)?
+            self.base.data.get(offset - first_fwsec.base.data.len()..)
         };
-        let pmu_lookup_table = PmuLookupTable::new(&self.base.dev, pmu_lookup_data)?;
 
-        match pmu_lookup_table.find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD) {
-            Ok(entry) => {
-                self.falcon_ucode_offset = Some(
-                    usize::from_safe_cast(entry.data)
-                        .checked_sub(pci_at_image.base.data.len())
-                        .and_then(|o| o.checked_sub(first_fwsec.base.data.len()))
-                        .ok_or(EINVAL)
-                        .inspect_err(|_| {
-                            dev_err!(self.base.dev, "Falcon Ucode offset not in second Fwsec.\n");
-                        })?,
-                );
-            }
-            Err(e) => {
+        let pmu_lookup_table = pmu_lookup_data
+            .ok_or(EINVAL)
+            .and_then(|data| PmuLookupTable::new(&self.base.dev, data))?;
+
+        let entry = pmu_lookup_table
+            .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
+            .inspect_err(|e| {
                 dev_err!(
                     self.base.dev,
                     "PmuLookupTableEntry not found, error: {:?}\n",
                     e
                 );
-                return Err(EINVAL);
-            }
-        }
+            })?;
+
+        let falcon_ucode_offset = usize::from_safe_cast(entry.data)
+            .checked_sub(pci_at_image.base.data.len())
+            .and_then(|o| o.checked_sub(first_fwsec.base.data.len()))
+            .ok_or(EINVAL)
+            .inspect_err(|_| {
+                dev_err!(self.base.dev, "Falcon Ucode offset not in second Fwsec.\n");
+            })?;
+
+        self.falcon_ucode_offset = Some(falcon_ucode_offset);
         Ok(())
     }
 

Quoted 1 line · Eliot Courtney

Remove pmu_in_first_fwsec and reorganize some code.

The patch itself is a nice improvement but the patch commit message needs more description [1]. Perhaps some of Alex's comments from here? [2]

[1]
https://kernel.org/doc/html/v6.19/process/submitting-patches.html#describe-changes
[2] https://lore.kernel.org/all/[email protected]/

FwSecBiosBuilder now only contains falcon_ucode_offset which just gets passed directly into FwSecBiosImage. Remove FwSecBiosBuilder and construct FwSecBiosImage directly, as a simplification.

Signed-off-by Eliot Courtney <[email protected]>

---
 drivers/gpu/nova-core/vbios.rs | 98 +++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 59 deletions(-)

drivers/gpu/nova-core/vbios.rs +39 -59

index d71ff5de794f..5cc251c73800 100644
@@ -233,8 +233,8 @@ impl Vbios {
     pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
         // Images to extract from iteration
         let mut pci_at_image: Option<PciAtBiosImage> = None;
-        let mut first_fwsec_image: Option<FwSecBiosBuilder> = None;
-        let mut second_fwsec_image: Option<FwSecBiosBuilder> = None;
+        let mut first_fwsec_image: Option<BiosImage> = None;
+        let mut second_fwsec_image: Option<BiosImage> = None;
 
         // Parse all VBIOS images in the ROM
         for image_result in VbiosIterator::new(dev, bar0)? {

@@ -254,14 +254,10 @@ pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
                     pci_at_image = Some(PciAtBiosImage::try_from(image)?);
                 }
                 Ok(BiosImageType::FwSec) => {
-                    let fwsec = FwSecBiosBuilder {
-                        base: image,
-                        falcon_ucode_offset: None,
-                    };
                     if first_fwsec_image.is_none() {
-                        first_fwsec_image = Some(fwsec);
+                        first_fwsec_image = Some(image);
                     } else {
-                        second_fwsec_image = Some(fwsec);
+                        second_fwsec_image = Some(image);
                     }
                 }
                 _ => {

@@ -271,15 +267,23 @@ pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
         }
 
         // Using all the images, setup the falcon data pointer in Fwsec.
-        if let (Some(mut second), Some(first), Some(pci_at)) =
+        if let (Some(second), Some(first), Some(pci_at)) =
             (second_fwsec_image, first_fwsec_image, pci_at_image)
         {
-            second
-                .setup_falcon_data(&pci_at, &first)
+            let fwsec_image = FwSecBiosImage::new(pci_at, first, second)
                 .inspect_err(|e| dev_err!(dev, "Falcon data setup failed: {:?}\n", e))?;
-            Ok(Vbios {
-                fwsec_image: second.build()?,
-            })
+
+            if cfg!(debug_assertions) {
+                // Print the desc header for debugging
+                let desc = fwsec_image.header()?;
+                dev_dbg!(
+                    fwsec_image.base.dev,
+                    "PmuLookupTableEntry desc: {:#?}\n",
+                    desc
+                );
+            }
+
+            Ok(Vbios { fwsec_image })
         } else {
             dev_err!(
                 dev,

@@ -621,18 +625,6 @@ struct NbsiBiosImage {
     // NBSI-specific fields can be added here in the future.
 }
 
-struct FwSecBiosBuilder {
-    base: BiosImage,
-    /// These are temporary fields that are used during the construction of the
-    /// [`FwSecBiosBuilder`].
-    ///
-    /// Once FwSecBiosBuilder is constructed, the `falcon_ucode_offset` will be copied into a new
-    /// [`FwSecBiosImage`].
-    ///
-    /// The offset of the Falcon ucode.
-    falcon_ucode_offset: Option<usize>,
-}
-
 /// 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.

@@ -898,33 +890,34 @@ fn find_entry_by_type(&self, entry_type: u8) -> Result<PmuLookupTableEntry> {
     }
 }
 
-impl FwSecBiosBuilder {
-    fn setup_falcon_data(
-        &mut self,
-        pci_at_image: &PciAtBiosImage,
-        first_fwsec: &FwSecBiosBuilder,
-    ) -> Result {
+impl FwSecBiosImage {
+    /// Build the final `FwSecBiosImage` from the PCI-AT and FWSEC BIOS images
+    fn new(
+        pci_at_image: PciAtBiosImage,
+        first_fwsec: BiosImage,
+        second_fwsec: BiosImage,
+    ) -> Result<FwSecBiosImage> {
         let offset = pci_at_image.falcon_data_offset()?;
 
         // The offset is from the start of the first FwSec image, but it
         // may point into the second FwSec image. Treat the two FwSec images
         // as contiguous here and subtract the first image length when the
         // target lies in the second one.
-        let pmu_lookup_data = if offset < first_fwsec.base.data.len() {
-            first_fwsec.base.data.get(offset..)
+        let pmu_lookup_data = if offset < first_fwsec.data.len() {
+            first_fwsec.data.get(offset..)
         } else {
-            self.base.data.get(offset - first_fwsec.base.data.len()..)
+            second_fwsec.data.get(offset - first_fwsec.data.len()..)
         };
 
         let pmu_lookup_table = pmu_lookup_data
             .ok_or(EINVAL)
-            .and_then(|data| PmuLookupTable::new(&self.base.dev, data))?;
+            .and_then(|data| PmuLookupTable::new(&second_fwsec.dev, data))?;
 
         let entry = pmu_lookup_table
             .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
             .inspect_err(|e| {
                 dev_err!(
-                    self.base.dev,
+                    second_fwsec.dev,
                     "PmuLookupTableEntry not found, error: {:?}\n",
                     e
                 );

@@ -932,34 +925,21 @@ fn setup_falcon_data(
 
         let falcon_ucode_offset = usize::from_safe_cast(entry.data)
             .checked_sub(pci_at_image.base.data.len())
-            .and_then(|o| o.checked_sub(first_fwsec.base.data.len()))
+            .and_then(|o| o.checked_sub(first_fwsec.data.len()))
             .ok_or(EINVAL)
             .inspect_err(|_| {
-                dev_err!(self.base.dev, "Falcon Ucode offset not in second Fwsec.\n");
+                dev_err!(
+                    second_fwsec.dev,
+                    "Falcon Ucode offset not in second Fwsec.\n"
+                );
             })?;
 
-        self.falcon_ucode_offset = Some(falcon_ucode_offset);
-        Ok(())
+        Ok(FwSecBiosImage {
+            base: second_fwsec,
+            falcon_ucode_offset,
+        })
     }
 
-    /// Build the final FwSecBiosImage from this builder
-    fn build(self) -> Result<FwSecBiosImage> {
-        let ret = FwSecBiosImage {
-            base: self.base,
-            falcon_ucode_offset: self.falcon_ucode_offset.ok_or(EINVAL)?,
-        };
-
-        if cfg!(debug_assertions) {
-            // Print the desc header for debugging
-            let desc = ret.header()?;
-            dev_dbg!(ret.base.dev, "PmuLookupTableEntry desc: {:#?}\n", desc);
-        }
-
-        Ok(ret)
-    }
-}
-
-impl FwSecBiosImage {
     /// Get the FwSec header ([`FalconUCodeDesc`]).
     pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
         let data = self

Currently, these are silently overwritten. Instead, error if they appear twice in the VBIOS.

Quoted 2 lines · Eliot Courtney

Currently, these are silently overwritten. Instead, error if they appear twice in the VBIOS.

How do you know that there can only be one PCI-AT image or no more than 2 FWSEC images?

Quoted 9 lines · Timur Tabi

Quoted 2 lines · Eliot Courtney

Currently, these are silently overwritten. Instead, error if they appear twice in the VBIOS.

How do you know that there can only be one PCI-AT image or no more than 2 FWSEC images?

Also, which GPU is this extra PCI-AT observed on? When I wrote all this code, it was on GA102 and there was only one PCI-AT and 2 FWSEC.

Quoted 10 lines · Joel Fernandes

Quoted 9 lines · Timur Tabi

Quoted 2 lines · Eliot Courtney

Currently, these are silently overwritten. Instead, error if they appear twice in the VBIOS.

How do you know that there can only be one PCI-AT image or no more than 2 FWSEC images?

Quoted 1 line

Also, which GPU is this extra PCI-AT observed on? When I wrote all this code, it was on GA102 and there was only one PCI-AT and 2 FWSEC.

I think that it doesn't make sense for there to be more than one PCI-AT image. I haven't observed any, it's just that the current code will silently accept it. I think that we should explicitly define the behaviour here - erroring seems the most conservative to me, but other behaviours could be take the first or take the last.

This is more of a problem for FWSEC images, because the current code will take the first image as first_fwsec_image and the last image as second_fwsec_image, which means that if there are more than two, the falcon data offset will actually point to the wrong thing, since the code assumes that first and second fwsec image are contiguous for the purposes of the offset pointer space.

AFAICT, nouveau and openrm take the first PCI-AT and FWSEC images, but they use the first FWSEC image to adjust the offset and then just address directly into the whole VBIOS based on that. To match that behaviour I can update this this patch to change the nova behaviour to match - i.e. take first two FWSEC and first PCI-AT, instead of the current behaviour which is last PCI-AT and first+last FWSEC.

My opinion is I don't see a benefit of changing it to that because everything including Turing is working with current code.

Quoted 2 lines · Joel Fernandes

My opinion is I don't see a benefit of changing it to that because everything including Turing is working with current code.

Well, we can't really say that, because we've only tested Nova on a handful of systems. Adding code that detects and reports unexpected variations is probably a really good idea at this stage.