[PATCH v2 00/11] gpu: nova-core: vbios: harden various array accesses and refactor

[PATCH v2 00/11] gpu: nova-core: vbios: harden various array accesses and refactor

[Eliot Courtney]

We have some code that accesses arrays based on values from firmware.
This patch series makes a bunch of those accesses more robust. This
series only touches accesses that are not guaranteed to be safe by local
invariants - some accesses are safe due to earlier checks and I haven't
modified those.

This series also refactors and removes some code that can be simplified.
In particular, it removes `FwSecBiosBuilder`. It also adds some more
stringent checking for PCI-AT and FWSEC images so duplicate ones will
result in an error.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
Changes in v2:
- Add Joel's reviewed-by tags.
- Remove unnecessary code like `falcon_data_offset` from
  `FwSecBiosBuilder`
- Push offset handling into `falcon_data_ptr` (renamed)
- Simplify `setup_falcon_data`
- Add checking for spurious PCI-AT and FWSEC images.
- Remove `FwSecBiosBuilder`
- Link to v1: https://patch.msgid.link/20260410-fix-vbios-v1-0-bc6f71d153d6@nvidia.com

---
Eliot Courtney (11):
      gpu: nova-core: vbios: fix various cases of reading past `BIOS_MAX_SCAN_LEN`
      gpu: nova-core: vbios: limit `BitToken` entry reads
      gpu: nova-core: vbios: use checked ops and accesses in `FwSecBiosImage::ucode`
      gpu: nova-core: vbios: use checked access in `FwSecBiosImage::header`
      gpu: nova-core: vbios: use checked accesses in `setup_falcon_data`
      gpu: nova-core: vbios: drop unused falcon_data_offset from FwSecBiosBuilder
      gpu: nova-core: vbios: keep PmuLookupTable local in setup_falcon_data
      gpu: nova-core: vbios: compute FWSEC-relative Falcon data offset
      gpu: nova-core: vbios: simplify setup_falcon_data
      gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images
      gpu: nova-core: vbios: reject extra PCI-AT and FWSEC images

 drivers/gpu/nova-core/vbios.rs | 299 +++++++++++++++++------------------------
 1 file changed, 126 insertions(+), 173 deletions(-)
---
base-commit: a7a080bb4236ebe577b6776d940d1717912ff6dd
change-id: 20260409-fix-vbios-d668e9c21d23

Best regards,
--  
Eliot Courtney <ecourtney@nvidia.com>

[PATCH v2 01/11] gpu: nova-core: vbios: fix various cases of reading past `BIOS_MAX_SCAN_LEN`

[Eliot Courtney]

Fix various cases that allow reading past `BIOS_MAX_SCAN_LEN` when
scanning the VBIOS.

Fix bug where `read_more_at_offset` would unnecessarily read more data.
This happens when the window to read has some part cached and some part
not. It would read `len` bytes instead of just the uncached portion,
which could read past `BIOS_MAX_SCAN_LEN`.

Also add more checked arithmetic to catch potential overflows.
`read_bios_image_at_offset` is called with a length from the VBIOS
header, so we should be more defensive here.

Fixes: 6fda04e7f0cd ("gpu: nova-core: vbios: Add base support for VBIOS construction and iteration")
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index ebda28e596c5..6de7e58e0da0 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -132,17 +132,14 @@ fn read_more(&mut self, len: usize) -> Result {
 
     /// Read bytes at a specific offset, filling any gap.
     fn read_more_at_offset(&mut self, offset: usize, len: usize) -> Result {
-        if offset > BIOS_MAX_SCAN_LEN {
+        let end = offset.checked_add(len).ok_or(EINVAL)?;
+
+        if end > BIOS_MAX_SCAN_LEN {
             dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
             return Err(EINVAL);
         }
 
-        // If `offset` is beyond current data size, fill the gap first.
-        let current_len = self.data.len();
-        let gap_bytes = offset.saturating_sub(current_len);
-
-        // Now read the requested bytes at the offset.
-        self.read_more(gap_bytes + len)
+        self.read_more(end.saturating_sub(self.data.len()))
     }
 
     /// Read a BIOS image at a specific offset and create a [`BiosImage`] from it.
@@ -155,8 +152,9 @@ fn read_bios_image_at_offset(
         len: usize,
         context: &str,
     ) -> Result<BiosImage> {
+        let end = offset.checked_add(len).ok_or(EINVAL)?;
         let data_len = self.data.len();
-        if offset + len > data_len {
+        if end > data_len {
             self.read_more_at_offset(offset, len).inspect_err(|e| {
                 dev_err!(
                     self.dev,
@@ -167,7 +165,7 @@ fn read_bios_image_at_offset(
             })?;
         }
 
-        BiosImage::new(self.dev, &self.data[offset..offset + len]).inspect_err(|err| {
+        BiosImage::new(self.dev, &self.data[offset..end]).inspect_err(|err| {
             dev_err!(
                 self.dev,
                 "Failed to {} at offset {:#x}: {:?}\n",
@@ -189,7 +187,7 @@ fn next(&mut self) -> Option<Self::Item> {
             return None;
         }
 
-        if self.current_offset > BIOS_MAX_SCAN_LEN {
+        if self.current_offset >= BIOS_MAX_SCAN_LEN {
             dev_err!(self.dev, "Error: exceeded BIOS scan limit, stopping scan\n");
             return None;
         }

-- 
2.53.0

[PATCH v2 02/11] gpu: nova-core: vbios: limit `BitToken` entry reads

[Eliot Courtney]

If `header.token_size` is smaller than `BitToken`, then we currently can
read past the end of `image.base.data`. Check that the token size is at
least as big as `BitToken`.

Fixes: dc70c6ae2441 ("gpu: nova-core: vbios: Add support to look up PMU table in FWSEC")
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 6de7e58e0da0..de856000de23 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -423,31 +423,31 @@ impl BitToken {
     /// Find a BIT token entry by BIT ID in a PciAtBiosImage
     fn from_id(image: &PciAtBiosImage, token_id: u8) -> Result<Self> {
         let header = &image.bit_header;
+        let entry_size = usize::from(header.token_size);
+
+        if entry_size < size_of::<BitToken>() {
+            return Err(EINVAL);
+        }
 
         // Offset to the first token entry
         let tokens_start = image.bit_offset + usize::from(header.header_size);
 
         for i in 0..usize::from(header.token_entries) {
-            let entry_offset = tokens_start + (i * usize::from(header.token_size));
-
-            // Make sure we don't go out of bounds
-            if entry_offset + usize::from(header.token_size) > image.base.data.len() {
-                return Err(EINVAL);
-            }
+            let entry_offset = tokens_start + (i * entry_size);
+            let entry = image
+                .base
+                .data
+                .get(entry_offset..)
+                .and_then(|data| data.get(..entry_size))
+                .ok_or(EINVAL)?;
 
             // Check if this token has the requested ID
-            if image.base.data[entry_offset] == token_id {
+            if entry[0] == token_id {
                 return Ok(BitToken {
-                    id: image.base.data[entry_offset],
-                    data_version: image.base.data[entry_offset + 1],
-                    data_size: u16::from_le_bytes([
-                        image.base.data[entry_offset + 2],
-                        image.base.data[entry_offset + 3],
-                    ]),
-                    data_offset: u16::from_le_bytes([
-                        image.base.data[entry_offset + 4],
-                        image.base.data[entry_offset + 5],
-                    ]),
+                    id: entry[0],
+                    data_version: entry[1],
+                    data_size: u16::from_le_bytes([entry[2], entry[3]]),
+                    data_offset: u16::from_le_bytes([entry[4], entry[5]]),
                 });
             }
         }

-- 
2.53.0

[PATCH v2 03/11] gpu: nova-core: vbios: use checked ops and accesses in `FwSecBiosImage::ucode`

[Eliot Courtney]

Use checked arithmetic and access for extracting the microcode since the
offsets are firmware derived.

Fixes: 47c4846e4319 ("gpu: nova-core: vbios: Add support for FWSEC ucode extraction")
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index de856000de23..632c8a90ea76 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -1029,16 +1029,21 @@ pub(crate) fn header(&self) -> Result<FalconUCodeDesc> {
 
     /// Get the ucode data as a byte slice
     pub(crate) fn ucode(&self, desc: &FalconUCodeDesc) -> Result<&[u8]> {
-        let falcon_ucode_offset = self.falcon_ucode_offset;
-
         // The ucode data follows the descriptor.
-        let ucode_data_offset = falcon_ucode_offset + desc.size();
-        let size = usize::from_safe_cast(desc.imem_load_size() + desc.dmem_load_size());
+        let data = self
+            .base
+            .data
+            .get(self.falcon_ucode_offset..)
+            .ok_or(ERANGE)?;
+        let size = usize::from_safe_cast(
+            desc.imem_load_size()
+                .checked_add(desc.dmem_load_size())
+                .ok_or(ERANGE)?,
+        );
 
         // Get the data slice, checking bounds in a single operation.
-        self.base
-            .data
-            .get(ucode_data_offset..ucode_data_offset + size)
+        data.get(desc.size()..)
+            .and_then(|data| data.get(..size))
             .ok_or(ERANGE)
             .inspect_err(|_| {
                 dev_err!(

-- 
2.53.0

[PATCH v2 04/11] gpu: nova-core: vbios: use checked access in `FwSecBiosImage::header`

[Eliot Courtney]

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 <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 632c8a90ea76..bc752d135cbf 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -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()
-            .map_err(|_| EINVAL)?;
-        let hdr = u32::from_le_bytes(hdr_bytes);
-        let ver = (hdr & 0xff00) >> 8;
-
-        let data = self.base.data.get(falcon_ucode_offset..).ok_or(EINVAL)?;
+        // Read the version byte from the header.
+        let ver = data.get(1).copied().ok_or(EINVAL)?;
         match ver {
             2 => {
                 let v2 = FalconUCodeDescV2::from_bytes_copy_prefix(data)

-- 
2.53.0

[PATCH v2 05/11] gpu: nova-core: vbios: use checked accesses in `setup_falcon_data`

[Eliot Courtney]

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 <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index bc752d135cbf..d8633e61178b 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -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!(

-- 
2.53.0

[PATCH v2 06/11] gpu: nova-core: vbios: drop unused falcon_data_offset from FwSecBiosBuilder

[Eliot Courtney]

This is unused, so we can remove it.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index d8633e61178b..d63af95eb642 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -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,

-- 
2.53.0

[PATCH v2 07/11] gpu: nova-core: vbios: keep PmuLookupTable local in setup_falcon_data

[Eliot Courtney]

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

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index d63af95eb642..01f65d50cbb3 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -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)

-- 
2.53.0

[PATCH v2 08/11] gpu: nova-core: vbios: compute FWSEC-relative Falcon data offset

[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 <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 48 +++++++++++++++++-------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 01f65d50cbb3..0c0e0402e715 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -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.
     ///
-    /// 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

-- 
2.53.0

[PATCH v2 09/11] gpu: nova-core: vbios: simplify setup_falcon_data

[Eliot Courtney]

Remove `pmu_in_first_fwsec` and reorganize some code.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 59 +++++++++++++++++++-----------------------
 1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 0c0e0402e715..d71ff5de794f 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -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(())
     }
 

-- 
2.53.0

[PATCH v2 10/11] gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images

[Eliot Courtney]

`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 <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 98 +++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index d71ff5de794f..5cc251c73800 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -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

-- 
2.53.0

[PATCH v2 11/11] gpu: nova-core: vbios: reject extra PCI-AT and FWSEC images

[Eliot Courtney]

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

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 5cc251c73800..a9294b68ea77 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -251,13 +251,20 @@ pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
             // Convert to a specific image type
             match BiosImageType::try_from(image.pcir.code_type) {
                 Ok(BiosImageType::PciAt) => {
+                    if pci_at_image.is_some() {
+                        dev_err!(dev, "More than 1 PCI-AT image found\n");
+                        return Err(EINVAL);
+                    }
                     pci_at_image = Some(PciAtBiosImage::try_from(image)?);
                 }
                 Ok(BiosImageType::FwSec) => {
                     if first_fwsec_image.is_none() {
                         first_fwsec_image = Some(image);
-                    } else {
+                    } else if second_fwsec_image.is_none() {
                         second_fwsec_image = Some(image);
+                    } else {
+                        dev_err!(dev, "More than 2 FwSec images found\n");
+                        return Err(EINVAL);
                     }
                 }
                 _ => {

-- 
2.53.0

Re: [PATCH v2 11/11] gpu: nova-core: vbios: reject extra PCI-AT and FWSEC images

[Timur Tabi]

On Tue, 2026-04-14 at 20:54 +0900, Eliot Courtney wrote:
> 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?

Re: [PATCH v2 11/11] gpu: nova-core: vbios: reject extra PCI-AT and FWSEC images

[Joel Fernandes]

> On Apr 14, 2026, at 7:40 PM, Timur Tabi <ttabi@nvidia.com> wrote:
> 
> On Tue, 2026-04-14 at 20:54 +0900, Eliot Courtney wrote:
>> 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.




> 

Re: [PATCH v2 09/11] gpu: nova-core: vbios: simplify setup_falcon_data

[Joel Fernandes]

On 4/14/2026 7:54 AM, Eliot Courtney wrote:
> 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/DHSJTFBXA2EM.1I22OK3J5NPRL@nvidia.com/

With the commit message updated,

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.


> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 59 +++++++++++++++++++-----------------------
>  1 file changed, 26 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 0c0e0402e715..d71ff5de794f 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -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(())
>      }
>  
> 

Re: [PATCH v2 10/11] gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images

[Joel Fernandes]

On 4/14/2026 7:54 AM, Eliot Courtney wrote:
> `FwSecBiosBuilder` now only contains `falcon_ucode_offset` which just
> gets passed directly into `FwSecBiosImage`. Remove `FwSecBiosBuilder`
> and construct `FwSecBiosImage` directly, as a simplification.
> 

nice :) The reason for the builder object was, we wanted to have a
'calculate' phase and a 'build object now' phase to avoid the problem of
partially constructed objects. However, as you showed we can accomplish
that without an intermediary objects.

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.


> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 98 +++++++++++++++++-------------------------
>  1 file changed, 39 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index d71ff5de794f..5cc251c73800 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -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
> 

Re: [PATCH v2 07/11] gpu: nova-core: vbios: keep PmuLookupTable local in setup_falcon_data

[Joel Fernandes]

On 4/14/2026 7:54 AM, Eliot Courtney wrote:
> This does not need to be stored, so we can remove it.

Need more words/description, but patch is good.

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.


> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index d63af95eb642..01f65d50cbb3 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -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)
> 

Re: [PATCH v2 08/11] gpu: nova-core: vbios: compute FWSEC-relative Falcon data offset

[Joel Fernandes]

On 4/14/2026 7:54 AM, Eliot Courtney wrote:
> 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 <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 48 +++++++++++++++++-------------------------
>  1 file changed, 19 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 01f65d50cbb3..0c0e0402e715 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -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.

With these changed, please add:

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.


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

Re: [PATCH v2 06/11] gpu: nova-core: vbios: drop unused falcon_data_offset from FwSecBiosBuilder

[Joel Fernandes]

On 4/14/2026 7:54 AM, Eliot Courtney wrote:
> This is unused, so we can remove it.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.


> ---
>  drivers/gpu/nova-core/vbios.rs | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index d8633e61178b..d63af95eb642 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -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,
> 

Re: [PATCH v2 05/11] gpu: nova-core: vbios: use checked accesses in `setup_falcon_data`

[Joel Fernandes]

On 4/14/2026 7:54 AM, Eliot Courtney wrote:
> 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 <ecourtney@nvidia.com>

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.

> ---
>  drivers/gpu/nova-core/vbios.rs | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index bc752d135cbf..d8633e61178b 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -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!(
> 

Re: [PATCH v2 04/11] gpu: nova-core: vbios: use checked access in `FwSecBiosImage::header`

[Joel Fernandes]

On 4/14/2026 7:54 AM, Eliot Courtney wrote:
> 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 <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 632c8a90ea76..bc752d135cbf 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -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()
> -            .map_err(|_| EINVAL)?;
> -        let hdr = u32::from_le_bytes(hdr_bytes);
> -        let ver = (hdr & 0xff00) >> 8;
> -
> -        let data = self.base.data.get(falcon_ucode_offset..).ok_or(EINVAL)?;
> +        // Read the version byte from the header.
> +        let ver = data.get(1).copied().ok_or(EINVAL)?;
>          match ver {
>              2 => {
>                  let v2 = FalconUCodeDescV2::from_bytes_copy_prefix(data)

sweet :)

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>

Thanks.

Re: [PATCH v2 09/11] gpu: nova-core: vbios: simplify setup_falcon_data

[Eliot Courtney]

On Fri Apr 17, 2026 at 12:30 AM JST, Joel Fernandes wrote:
>
>
> On 4/14/2026 7:54 AM, Eliot Courtney wrote:
>> 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/DHSJTFBXA2EM.1I22OK3J5NPRL@nvidia.com/
>

Thanks for finding the links, you're right, I was being lazy. Will
update.

Re: [PATCH v2 11/11] gpu: nova-core: vbios: reject extra PCI-AT and FWSEC images

[Eliot Courtney]

On Wed Apr 15, 2026 at 9:02 AM JST, Joel Fernandes wrote:
>
>
>> On Apr 14, 2026, at 7:40 PM, Timur Tabi <ttabi@nvidia.com> wrote:
>> 
>> On Tue, 2026-04-14 at 20:54 +0900, Eliot Courtney wrote:
>>> 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.

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.

N.B. that the current code partially assumes that there are only two
FWSEC images because it requires the falcon data pointer to be in the
first or second one, and errors if it's after that.

WDYT?

Re: [PATCH v2 08/11] gpu: nova-core: vbios: compute FWSEC-relative Falcon data offset

[Eliot Courtney]

On Fri Apr 17, 2026 at 1:13 AM JST, Joel Fernandes wrote:
> On 4/14/2026 7:54 AM, Eliot Courtney wrote:
>> 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 <ecourtney@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/vbios.rs | 48 +++++++++++++++++-------------------------
>>  1 file changed, 19 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
>> index 01f65d50cbb3..0c0e0402e715 100644
>> --- a/drivers/gpu/nova-core/vbios.rs
>> +++ b/drivers/gpu/nova-core/vbios.rs
>> @@ -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.

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?