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

[PATCH v4 00/20] 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`, removes unused fields,
and moves type and constant definitions closer to their usages.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
Changes in v4:
- Split BIOS_MAX_SCAN_LEN patch into multiple per review comments
- Consolidate bios max scan length checking
- Convert more structs to use FromBytes since we assume little endian
- Remove unused PciRomHeader fields
- Drop unused types / imports
- Move constants+helpers closer to usage locations
- Link to v3: https://patch.msgid.link/20260421-fix-vbios-v3-0-8f648aef7a85@nvidia.com

Changes in v3:
- Use first PCI-AT and FWSEC images instead of erroring.
- Expand commit messages.
- Add Joel's Reviewed-by's (thanks!)
- Link to v2: https://patch.msgid.link/20260414-fix-vbios-v2-0-705d30d16bba@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 (20):
      gpu: nova-core: vbios: stop scanning at BIOS_MAX_SCAN_LEN
      gpu: nova-core: vbios: use checked arithmetic for bios image range end
      gpu: nova-core: vbios: avoid reading too far in read_more_at_offset
      gpu: nova-core: vbios: read BitToken using FromBytes
      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: read PMU lookup entries using FromBytes
      gpu: nova-core: vbios: store PMU lookup entries in a KVVec
      gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images
      gpu: nova-core: vbios: use the first PCI-AT and FWSEC images
      gpu: nova-core: vbios: use let-else in Vbios::new
      gpu: nova-core: vbios: remove unnecessary fields in PciRomHeader
      gpu: nova-core: vbios: drop unused image wrappers
      gpu: nova-core: vbios: drop redundant TryFrom import
      gpu: nova-core: vbios: move constants and functions to be associated

 Documentation/gpu/nova/core/vbios.rst |   2 +-
 drivers/gpu/nova-core/vbios.rs        | 658 ++++++++++++++--------------------
 2 files changed, 267 insertions(+), 393 deletions(-)
---
base-commit: 8bfe9d72cf2064f679c4192dba84be79eb70675d
change-id: 20260409-fix-vbios-d668e9c21d23

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

[PATCH v4 01/20] gpu: nova-core: vbios: stop scanning at BIOS_MAX_SCAN_LEN

[Eliot Courtney]

Current code lets `current_offset` go to `BIOS_MAX_SCAN_LEN` which is
one byte too far.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 6bcfb6c5cf44..7bec81a37340 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -272,7 +272,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.54.0

[PATCH v4 02/20] gpu: nova-core: vbios: use checked arithmetic for bios image range end

[Eliot Courtney]

`read_bios_image_at_offset` is called with a length from the VBIOS
header, so we should be more defensive here and use checked arithmetic.

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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 7bec81a37340..180928433766 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -238,8 +238,8 @@ fn read_bios_image_at_offset(
         len: usize,
         context: &str,
     ) -> Result<BiosImage> {
-        let data_len = self.data.len();
-        if offset + len > data_len {
+        let end = offset.checked_add(len).ok_or(EINVAL)?;
+        if end > self.data.len() {
             self.read_more_at_offset(offset, len).inspect_err(|e| {
                 dev_err!(
                     self.dev,
@@ -250,7 +250,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",

-- 
2.54.0

[PATCH v4 03/20] gpu: nova-core: vbios: avoid reading too far in read_more_at_offset

[Eliot Courtney]

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`.

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

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 180928433766..79eb01dabc6f 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -185,8 +185,13 @@ fn new(dev: &'a device::Device, bar0: &'a Bar0) -> Result<Self> {
 
     /// Read bytes from the ROM at the current end of the data vector.
     fn read_more(&mut self, len: usize) -> Result {
-        let current_len = self.data.len();
-        let start = ROM_OFFSET + current_len;
+        let start = self.data.len();
+        let end = start + len;
+
+        if end > BIOS_MAX_SCAN_LEN {
+            dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
+            return Err(EINVAL);
+        }
 
         // Ensure length is a multiple of 4 for 32-bit reads
         if len % core::mem::size_of::<u32>() != 0 {
@@ -200,9 +205,9 @@ fn read_more(&mut self, len: usize) -> Result {
 
         self.data.reserve(len, GFP_KERNEL)?;
         // Read ROM data bytes and push directly to `data`.
-        for addr in (start..start + len).step_by(core::mem::size_of::<u32>()) {
+        for addr in (start..end).step_by(core::mem::size_of::<u32>()) {
             // Read 32-bit word from the VBIOS ROM
-            let word = self.bar0.try_read32(addr)?;
+            let word = self.bar0.try_read32(ROM_OFFSET + addr)?;
 
             // Convert the `u32` to a 4 byte array and push each byte.
             word.to_ne_bytes()
@@ -215,17 +220,9 @@ 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 {
-            dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
-            return Err(EINVAL);
-        }
+        let end = offset.checked_add(len).ok_or(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.

-- 
2.54.0

[PATCH v4 04/20] gpu: nova-core: vbios: read BitToken using FromBytes

[Eliot Courtney]

If `header.token_size` is smaller than `BitToken`, then we currently can
read past the end of `image.base.data`. Use checked arithmetic for
computing offsets and simplify reading it in using `FromBytes`.

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 | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 79eb01dabc6f..2ff67273fdff 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -486,7 +486,7 @@ fn new(data: &[u8]) -> Result<Self> {
 
 /// BIT Token Entry: Records in the BIT table followed by the BIT header.
 #[derive(Debug, Clone, Copy)]
-#[expect(dead_code)]
+#[repr(C)]
 struct BitToken {
     /// 00h: Token identifier
     id: u8,
@@ -498,6 +498,9 @@ struct BitToken {
     data_offset: u16,
 }
 
+// SAFETY: all bit patterns are valid for `BitToken`.
+unsafe impl FromBytes for BitToken {}
+
 // Define the token ID for the Falcon data
 const BIT_TOKEN_ID_FALCON_DATA: u8 = 0x70;
 
@@ -505,32 +508,28 @@ 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);
 
         // 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));
+            let entry_offset = i
+                .checked_mul(entry_size)
+                .and_then(|offset| tokens_start.checked_add(offset))
+                .ok_or(EINVAL)?;
+            let entry = image
+                .base
+                .data
+                .get(entry_offset..)
+                .and_then(|data| data.get(..entry_size))
+                .ok_or(EINVAL)?;
 
-            // 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 (token, _) = BitToken::from_bytes_copy_prefix(entry).ok_or(EINVAL)?;
 
             // Check if this token has the requested ID
-            if image.base.data[entry_offset] == 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],
-                    ]),
-                });
+            if token.id == token_id {
+                return Ok(token);
             }
         }
 

-- 
2.54.0

[PATCH v4 05/20] 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 | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 2ff67273fdff..c62d918a3041 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -1110,16 +1110,18 @@ 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;
+        let size = usize::from_safe_cast(
+            desc.imem_load_size()
+                .checked_add(desc.dmem_load_size())
+                .ok_or(ERANGE)?,
+        );
 
         // 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());
-
-        // Get the data slice, checking bounds in a single operation.
         self.base
             .data
-            .get(ucode_data_offset..ucode_data_offset + size)
+            .get(self.falcon_ucode_offset..)
+            .and_then(|data| data.get(desc.size()..))
+            .and_then(|data| data.get(..size))
             .ok_or(ERANGE)
             .inspect_err(|_| {
                 dev_err!(

-- 
2.54.0

[PATCH v4 06/20] 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")
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
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 c62d918a3041..48a46684e279 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -1077,17 +1077,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.54.0

[PATCH v4 07/20] 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")
Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
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 48a46684e279..871f455bb720 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -1036,14 +1036,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.54.0

[PATCH v4 08/20] gpu: nova-core: vbios: drop unused falcon_data_offset from FwSecBiosBuilder

[Eliot Courtney]

This is unused, so we can remove it.

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
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 871f455bb720..8a0e16e6c9e8 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -338,7 +338,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,
                     };
@@ -712,8 +711,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.
@@ -1015,8 +1012,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.54.0

[PATCH v4 09/20] gpu: nova-core: vbios: keep PmuLookupTable local in setup_falcon_data

[Eliot Courtney]

This does not need to be stored in `FwSecBiosBuilder` so we can remove
it from there, and just create and use it locally in
`setup_falcon_data`.

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
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 8a0e16e6c9e8..cadc6dcffefb 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -338,7 +338,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() {
@@ -711,8 +710,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>,
 }
@@ -1012,24 +1009,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.54.0

[PATCH v4 10/20] 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 cadc6dcffefb..ca101b2b6095 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -846,33 +846,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");
+            })
     }
 }
 
@@ -989,15 +985,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.54.0

[PATCH v4 11/20] gpu: nova-core: vbios: simplify setup_falcon_data

[Eliot Courtney]

The code first computes `pmu_in_first_fwsec` or adjusts the offset and
then uses it in a branch just once to get the correct source for the PMU
table. This can be simplified to a single branch while also avoiding the
mutation of `offset`. Also, adjust the code after this to keep the
success case non-nested.

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/vbios.rs | 54 ++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index ca101b2b6095..470e0e2a81ab 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -985,48 +985,40 @@ 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();
+            self.base.data.get(offset - first_fwsec.base.data.len()..)
         }
+        .ok_or(EINVAL)?;
 
-        let pmu_lookup_data = if pmu_in_first_fwsec {
-            &first_fwsec.base.data[offset..]
-        } else {
-            self.base.data.get(offset..).ok_or(EINVAL)?
-        };
         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 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.54.0

[PATCH v4 12/20] gpu: nova-core: vbios: read PMU lookup entries using FromBytes

[Eliot Courtney]

This simplifies the construction of `PmuLookupTableEntry` and is
allowed now that the driver can assume it is little endian.

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

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 470e0e2a81ab..987eb1948314 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -897,19 +897,8 @@ struct PmuLookupTableEntry {
     data: u32,
 }
 
-impl PmuLookupTableEntry {
-    fn new(data: &[u8]) -> Result<Self> {
-        if data.len() < core::mem::size_of::<Self>() {
-            return Err(EINVAL);
-        }
-
-        Ok(PmuLookupTableEntry {
-            application_id: data[0],
-            target_id: data[1],
-            data: u32::from_le_bytes(data[2..6].try_into().map_err(|_| EINVAL)?),
-        })
-    }
-}
+// SAFETY: all bit patterns are valid for `PmuLookupTableEntry`.
+unsafe impl FromBytes for PmuLookupTableEntry {}
 
 #[repr(C)]
 struct PmuLookupTableHeader {
@@ -963,7 +952,13 @@ fn lookup_index(&self, idx: u8) -> Result<PmuLookupTableEntry> {
         }
 
         let index = (usize::from(idx)) * usize::from(self.header.entry_len);
-        PmuLookupTableEntry::new(&self.table_data[index..])
+        let (entry, _) = self
+            .table_data
+            .get(index..)
+            .and_then(PmuLookupTableEntry::from_bytes_copy_prefix)
+            .ok_or(EINVAL)?;
+
+        Ok(entry)
     }
 
     // find entry by type value

-- 
2.54.0

[PATCH v4 13/20] gpu: nova-core: vbios: store PMU lookup entries in a KVVec

[Eliot Courtney]

The current code copies the data into a KVec and parses it on demand. We
can simplify the code by storing the parsed entries.

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

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 987eb1948314..1ffaf0ef56a7 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -917,8 +917,7 @@ unsafe impl FromBytes for PmuLookupTableHeader {}
 /// The table of entries is pointed to by the falcon data pointer in the BIT table, and is used to
 /// locate the Falcon Ucode.
 struct PmuLookupTable {
-    header: PmuLookupTableHeader,
-    table_data: KVec<u8>,
+    entries: KVVec<PmuLookupTableEntry>,
 }
 
 impl PmuLookupTable {
@@ -929,48 +928,29 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
         let entry_len = usize::from(header.entry_len);
         let entry_count = usize::from(header.entry_count);
 
-        let required_bytes = header_len + (entry_count * entry_len);
+        let data = data
+            .get(header_len..header_len + entry_count * entry_len)
+            .ok_or(EINVAL)
+            .inspect_err(|_| {
+                dev_err!(dev, "PmuLookupTable data length less than required\n");
+            })?;
 
-        if data.len() < required_bytes {
-            dev_err!(dev, "PmuLookupTable data length less than required\n");
-            return Err(EINVAL);
+        let mut entries = KVVec::with_capacity(entry_count, GFP_KERNEL)?;
+        for i in 0..entry_count {
+            let (entry, _) = PmuLookupTableEntry::from_bytes_copy_prefix(&data[i * entry_len..])
+                .ok_or(EINVAL)?;
+            entries.push(entry, GFP_KERNEL)?;
         }
 
-        // Create a copy of only the table data
-        let table_data = {
-            let mut ret = KVec::new();
-            ret.extend_from_slice(&data[header_len..required_bytes], GFP_KERNEL)?;
-            ret
-        };
-
-        Ok(PmuLookupTable { header, table_data })
-    }
-
-    fn lookup_index(&self, idx: u8) -> Result<PmuLookupTableEntry> {
-        if idx >= self.header.entry_count {
-            return Err(EINVAL);
-        }
-
-        let index = (usize::from(idx)) * usize::from(self.header.entry_len);
-        let (entry, _) = self
-            .table_data
-            .get(index..)
-            .and_then(PmuLookupTableEntry::from_bytes_copy_prefix)
-            .ok_or(EINVAL)?;
-
-        Ok(entry)
+        Ok(PmuLookupTable { entries })
     }
 
     // find entry by type value
-    fn find_entry_by_type(&self, entry_type: u8) -> Result<PmuLookupTableEntry> {
-        for i in 0..self.header.entry_count {
-            let entry = self.lookup_index(i)?;
-            if entry.application_id == entry_type {
-                return Ok(entry);
-            }
-        }
-
-        Err(EINVAL)
+    fn find_entry_by_type(&self, entry_type: u8) -> Result<&PmuLookupTableEntry> {
+        self.entries
+            .iter()
+            .find(|entry| entry.application_id == entry_type)
+            .ok_or(EINVAL)
     }
 }
 

-- 
2.54.0

[PATCH v4 14/20] 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.

Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
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 1ffaf0ef56a7..c5dad9e065da 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -315,8 +315,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)? {
@@ -336,14 +336,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);
                     }
                 }
                 _ => {
@@ -353,15 +349,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,
@@ -702,18 +706,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.
@@ -954,32 +946,33 @@ 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()..)
         }
         .ok_or(EINVAL)?;
 
-        let pmu_lookup_table = PmuLookupTable::new(&self.base.dev, pmu_lookup_data)?;
+        let pmu_lookup_table = PmuLookupTable::new(&second_fwsec.dev, pmu_lookup_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
                 );
@@ -987,34 +980,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.54.0

[PATCH v4 15/20] gpu: nova-core: vbios: use the first PCI-AT and FWSEC images

[Eliot Courtney]

Currently, PCI-AT takes the final image if multiple exist. For FWSEC, it
takes the first one and the last one. Align both of these to nouveau
behavior by taking the first ones.

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

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index c5dad9e065da..ff21f85fdfb6 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -333,12 +333,16 @@ 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) => {
-                    pci_at_image = Some(PciAtBiosImage::try_from(image)?);
+                    // Silently ignore any extra PCI-AT images.
+                    if pci_at_image.is_none() {
+                        pci_at_image = Some(PciAtBiosImage::try_from(image)?);
+                    }
                 }
                 Ok(BiosImageType::FwSec) => {
+                    // Silently ignore any extra FwSec images beyond the first two.
                     if first_fwsec_image.is_none() {
                         first_fwsec_image = Some(image);
-                    } else {
+                    } else if second_fwsec_image.is_none() {
                         second_fwsec_image = Some(image);
                     }
                 }

-- 
2.54.0

[PATCH v4 16/20] gpu: nova-core: vbios: use let-else in Vbios::new

[Eliot Courtney]

Improve readability by moving the success path outside of a nested
branch.

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

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index ff21f85fdfb6..64d100b6699b 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -353,30 +353,30 @@ 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(second), Some(first), Some(pci_at)) =
+        let (Some(second), Some(first), Some(pci_at)) =
             (second_fwsec_image, first_fwsec_image, pci_at_image)
-        {
-            let fwsec_image = FwSecBiosImage::new(pci_at, first, second)
-                .inspect_err(|e| dev_err!(dev, "Falcon data setup failed: {:?}\n", e))?;
-
-            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 {
+        else {
             dev_err!(
                 dev,
                 "Missing required images for falcon data setup, skipping\n"
             );
-            Err(EINVAL)
+            return Err(EINVAL);
+        };
+
+        let fwsec_image = FwSecBiosImage::new(pci_at, first, second)
+            .inspect_err(|e| dev_err!(dev, "Falcon data setup failed: {:?}\n", e))?;
+
+        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 })
     }
 
     pub(crate) fn fwsec_image(&self) -> &FwSecBiosImage {

-- 
2.54.0

[PATCH v4 17/20] gpu: nova-core: vbios: remove unnecessary fields in PciRomHeader

[Eliot Courtney]

Remove unnecessary fields in PciRomHeader. This allows a simplification
to use `FromBytes` instead of reading fields piecemeal. A lot of these
checks were redundant as well since it checks the size of the `data`
first in `BiosImage`.

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

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 64d100b6699b..1dca7933fac5 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -546,67 +546,38 @@ fn from_id(image: &PciAtBiosImage, token_id: u8) -> Result<Self> {
 
 /// PCI ROM Expansion Header as defined in PCI Firmware Specification.
 ///
-/// This is header is at the beginning of every image in the set of images in the ROM. It contains
-/// a pointer to the PCI Data Structure which describes the image. For "NBSI" images (NoteBook
-/// System Information), the ROM header deviates from the standard and contains an offset to the
-/// NBSI image however we do not yet parse that in this module and keep it for future reference.
+/// This header is at the beginning of every image in the set of images in the ROM. It contains a
+/// pointer to the PCI Data Structure which describes the image.
 #[derive(Debug, Clone, Copy)]
-#[expect(dead_code)]
+#[repr(C)]
 struct PciRomHeader {
     /// 00h: Signature (0xAA55)
     signature: u16,
-    /// 02h: Reserved bytes for processor architecture unique data (20 bytes)
-    reserved: [u8; 20],
-    /// 16h: NBSI Data Offset (NBSI-specific, offset from header to NBSI image)
-    nbsi_data_offset: Option<u16>,
+    /// 02h: Reserved bytes for processor architecture unique data (22 bytes)
+    reserved: [u8; 22],
     /// 18h: Pointer to PCI Data Structure (offset from start of ROM image)
     pci_data_struct_offset: u16,
-    /// 1Ah: Size of block (this is NBSI-specific)
-    size_of_block: Option<u32>,
 }
 
+// SAFETY: all bit patterns are valid for `PciRomHeader`.
+unsafe impl FromBytes for PciRomHeader {}
+
 impl PciRomHeader {
     fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
-        if data.len() < 26 {
-            // Need at least 26 bytes to read pciDataStrucPtr and sizeOfBlock.
-            return Err(EINVAL);
-        }
-
-        let signature = u16::from_le_bytes([data[0], data[1]]);
+        let (rom_header, _) = PciRomHeader::from_bytes_copy_prefix(data)
+            .ok_or(EINVAL)
+            .inspect_err(|_| dev_err!(dev, "Not enough data for ROM header\n"))?;
 
         // Check for valid ROM signatures.
-        match signature {
+        match rom_header.signature {
             0xAA55 | 0x4E56 => {}
             _ => {
-                dev_err!(dev, "ROM signature unknown {:#x}\n", signature);
+                dev_err!(dev, "ROM signature unknown {:#x}\n", rom_header.signature);
                 return Err(EINVAL);
             }
         }
 
-        // Read the pointer to the PCI Data Structure at offset 0x18.
-        let pci_data_struct_ptr = u16::from_le_bytes([data[24], data[25]]);
-
-        // Try to read optional fields if enough data.
-        let mut size_of_block = None;
-        let mut nbsi_data_offset = None;
-
-        if data.len() >= 30 {
-            // Read size_of_block at offset 0x1A.
-            size_of_block = Some(u32::from_le_bytes([data[26], data[27], data[28], data[29]]));
-        }
-
-        // For NBSI images, try to read the nbsiDataOffset at offset 0x16.
-        if data.len() >= 24 {
-            nbsi_data_offset = Some(u16::from_le_bytes([data[22], data[23]]));
-        }
-
-        Ok(PciRomHeader {
-            signature,
-            reserved: [0u8; 20],
-            pci_data_struct_offset: pci_data_struct_ptr,
-            size_of_block,
-            nbsi_data_offset,
-        })
+        Ok(rom_header)
     }
 }
 
@@ -722,11 +693,11 @@ pub(crate) struct FwSecBiosImage {
 /// BIOS Image structure containing various headers and reference fields to all BIOS images.
 ///
 /// A BiosImage struct is embedded into all image types and implements common operations.
-#[expect(dead_code)]
 struct BiosImage {
     /// Used for logging.
     dev: ARef<device::Device>,
     /// PCI ROM Expansion Header
+    #[expect(dead_code)]
     rom_header: PciRomHeader,
     /// PCI Data Structure
     pcir: PcirStruct,
@@ -771,15 +742,8 @@ fn is_last(&self) -> bool {
 
     /// Creates a new BiosImage from raw byte data.
     fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
-        // Ensure we have enough data for the ROM header.
-        if data.len() < 26 {
-            dev_err!(dev, "Not enough data for ROM header\n");
-            return Err(EINVAL);
-        }
-
         // Parse the ROM header.
-        let rom_header = PciRomHeader::new(dev, &data[0..26])
-            .inspect_err(|e| dev_err!(dev, "Failed to create PciRomHeader: {:?}\n", e))?;
+        let rom_header = PciRomHeader::new(dev, data)?;
 
         // Get the PCI Data Structure using the pointer from the ROM header.
         let pcir_offset = usize::from(rom_header.pci_data_struct_offset);

-- 
2.54.0

[PATCH v4 18/20] gpu: nova-core: vbios: drop unused image wrappers

[Eliot Courtney]

These are unused currently, and it is probably sufficient to just check
the type of BIOS image in the future.

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

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 1dca7933fac5..ad571b39400d 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -669,18 +669,6 @@ struct PciAtBiosImage {
     bit_offset: usize,
 }
 
-#[expect(dead_code)]
-struct EfiBiosImage {
-    base: BiosImage,
-    // EFI-specific fields can be added here in the future.
-}
-
-#[expect(dead_code)]
-struct NbsiBiosImage {
-    base: BiosImage,
-    // NBSI-specific fields can be added here in the future.
-}
-
 /// 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.

-- 
2.54.0

[PATCH v4 19/20] gpu: nova-core: vbios: drop redundant TryFrom import

[Eliot Courtney]

This is unused.

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

diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index ad571b39400d..9cc2f008bbfb 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -2,8 +2,6 @@
 
 //! VBIOS extraction and parsing.
 
-use core::convert::TryFrom;
-
 use kernel::{
     device,
     io::Io,

-- 
2.54.0

[PATCH v4 20/20] gpu: nova-core: vbios: move constants and functions to be associated

[Eliot Courtney]

Move constants and functions to be inside the impls of the types they
are related to. This makes it more obvious what each type and value is
for.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 Documentation/gpu/nova/core/vbios.rst |   2 +-
 drivers/gpu/nova-core/vbios.rs        | 185 +++++++++++++++++-----------------
 2 files changed, 96 insertions(+), 91 deletions(-)

diff --git a/Documentation/gpu/nova/core/vbios.rst b/Documentation/gpu/nova/core/vbios.rst
index a4fe63422ede..9d3379ccfb30 100644
--- a/Documentation/gpu/nova/core/vbios.rst
+++ b/Documentation/gpu/nova/core/vbios.rst
@@ -232,7 +232,7 @@ Falcon data in the VBIOS which contains the PMU lookup table. This lookup table
 used to find the required Falcon ucode based on an application ID.
 
 The location of the PMU lookup table is found by scanning the BIT (`BIOS Information Table`_)
-tokens for a token with the id `BIT_TOKEN_ID_FALCON_DATA` (0x70) which indicates the
+tokens for a token with the Falcon data token id (0x70) which indicates the
 offset of the same from the start of the VBIOS image. Unfortunately, the offset
 does not account for the EFI image located between the PciAt and FwSec images.
 The `vbios.rs` code compensates for this with appropriate arithmetic.
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 9cc2f008bbfb..07b5235faff8 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -27,16 +27,6 @@
     num::FromSafeCast,
 };
 
-/// The offset of the VBIOS ROM in the BAR0 space.
-const ROM_OFFSET: usize = 0x300000;
-/// The maximum length of the VBIOS ROM to scan into.
-const BIOS_MAX_SCAN_LEN: usize = 0x100000;
-/// The size to read ahead when parsing initial BIOS image headers.
-const BIOS_READ_AHEAD_SIZE: usize = 1024;
-/// The bit in the last image indicator byte for the PCI Data Structure that
-/// indicates the last image. Bit 0-6 are reserved, bit 7 is last image bit.
-const LAST_IMAGE_BIT_MASK: u8 = 0x80;
-
 /// BIOS Image Type from PCI Data Structure code_type field.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 #[repr(u8)]
@@ -65,14 +55,6 @@ fn try_from(code: u8) -> Result<Self> {
     }
 }
 
-// PMU lookup table entry types. Used to locate PMU table entries
-// in the Fwsec image, corresponding to falcon ucodes.
-#[expect(dead_code)]
-const FALCON_UCODE_ENTRY_APPID_FIRMWARE_SEC_LIC: u8 = 0x05;
-#[expect(dead_code)]
-const FALCON_UCODE_ENTRY_APPID_FWSEC_DBG: u8 = 0x45;
-const FALCON_UCODE_ENTRY_APPID_FWSEC_PROD: u8 = 0x85;
-
 /// Vbios Reader for constructing the VBIOS data.
 struct VbiosIterator<'a> {
     dev: &'a device::Device,
@@ -110,73 +92,79 @@ struct VbiosIterator<'a> {
     }
 }
 
-/// Return the byte offset where the PCI Expansion ROM images begin in the GPU's ROM.
-///
-/// The GPU's ROM may begin with an Init-from-ROM (IFR) header that precedes
-/// the PCI Expansion ROM images (VBIOS).  When present, the PROM shadow
-/// method must parse this header to determine the offset where the PCI ROM
-/// images actually begin, and adjust all subsequent reads accordingly.
-///
-/// On most GPUs this is not needed because the IFR microcode has already
-/// applied the ROM offset so that PROM reads transparently skip the header.
-/// On GA100, for some reason, the IFR offset is not applied to PROM
-/// reads.  Therefore, the search for the PCI expansion must skip the IFR
-/// header, if found.
-fn vbios_rom_offset(dev: &device::Device, bar0: &Bar0) -> Result<usize> {
-    /// IFR signature.
-    const NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE: u32 = u32::from_le_bytes(*b"NVGI");
-    /// ROM directory signature.
-    const NV_ROM_DIRECTORY_IDENTIFIER: u32 = u32::from_le_bytes(*b"RFRD");
-    /// Offset of the NV_PMGR_ROM_ADDR_OFFSET register in IFR Extended section.
-    const IFR_SW_EXT_ROM_ADDR_OFFSET: usize = 4;
-    /// Size of Redundant Firmware Flash Status section.
-    const RFW_FLASH_STATUS_SIZE: usize = SZ_4K;
-    /// Offset in the ROM Directory of the PCI Option ROM offset
-    const PCI_OPTION_ROM_OFFSET: usize = 8;
-
-    let signature = bar0.read(NV_PBUS_IFR_FMT_FIXED0).signature();
-
-    if signature == NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE {
-        let fixed1 = bar0.read(NV_PBUS_IFR_FMT_FIXED1);
-
-        match fixed1.version() {
-            1 | 2 => {
-                let fixed_data_size = usize::from(fixed1.fixed_data_size());
-                let pmgr_rom_addr_offset = fixed_data_size + IFR_SW_EXT_ROM_ADDR_OFFSET;
-                bar0.try_read32(ROM_OFFSET + pmgr_rom_addr_offset)
-                    .map(usize::from_safe_cast)
-            }
-            3 => {
-                let fixed2 = bar0.read(NV_PBUS_IFR_FMT_FIXED2);
-                let total_data_size = usize::from(fixed2.total_data_size());
-                let flash_status_offset =
-                    usize::from_safe_cast(bar0.try_read32(ROM_OFFSET + total_data_size)?);
-                let dir_offset = flash_status_offset + RFW_FLASH_STATUS_SIZE;
-                let dir_sig = bar0.try_read32(ROM_OFFSET + dir_offset)?;
-                if dir_sig != NV_ROM_DIRECTORY_IDENTIFIER {
-                    dev_err!(dev, "could not find IFR ROM directory\n");
-                    return Err(EINVAL);
-                }
-                bar0.try_read32(ROM_OFFSET + dir_offset + PCI_OPTION_ROM_OFFSET)
-                    .map(usize::from_safe_cast)
-            }
-            _ => {
-                dev_err!(dev, "unsupported IFR header version {}\n", fixed1.version());
-                Err(EINVAL)
-            }
-        }
-    } else {
-        Ok(0)
-    }
-}
-
 impl<'a> VbiosIterator<'a> {
+    /// The offset of the VBIOS ROM in the BAR0 space.
+    const ROM_OFFSET: usize = 0x300000;
+    /// The maximum length of the VBIOS ROM to scan into.
+    const BIOS_MAX_SCAN_LEN: usize = 0x100000;
+    /// The size to read ahead when parsing initial BIOS image headers.
+    const BIOS_READ_AHEAD_SIZE: usize = 1024;
+
+    /// Return the byte offset where the PCI Expansion ROM images begin in the GPU's ROM.
+    ///
+    /// The GPU's ROM may begin with an Init-from-ROM (IFR) header that precedes the PCI Expansion
+    /// ROM images (VBIOS). When present, the PROM shadow method must parse this header to determine
+    /// the offset where the PCI ROM images actually begin, and adjust all subsequent reads
+    /// accordingly.
+    ///
+    /// On most GPUs this is not needed because the IFR microcode has already applied the ROM offset
+    /// so that PROM reads transparently skip the header. On GA100, for some reason, the IFR offset
+    /// is not applied to PROM reads. Therefore, the search for the PCI expansion must skip the IFR
+    /// header, if found.
+    fn rom_offset(dev: &device::Device, bar0: &Bar0) -> Result<usize> {
+        /// IFR signature.
+        const NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE: u32 = u32::from_le_bytes(*b"NVGI");
+        /// ROM directory signature.
+        const NV_ROM_DIRECTORY_IDENTIFIER: u32 = u32::from_le_bytes(*b"RFRD");
+        /// Offset of the NV_PMGR_ROM_ADDR_OFFSET register in IFR Extended section.
+        const IFR_SW_EXT_ROM_ADDR_OFFSET: usize = 4;
+        /// Size of Redundant Firmware Flash Status section.
+        const RFW_FLASH_STATUS_SIZE: usize = SZ_4K;
+        /// Offset in the ROM Directory of the PCI Option ROM offset.
+        const PCI_OPTION_ROM_OFFSET: usize = 8;
+
+        let signature = bar0.read(NV_PBUS_IFR_FMT_FIXED0).signature();
+
+        if signature == NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE {
+            let fixed1 = bar0.read(NV_PBUS_IFR_FMT_FIXED1);
+
+            match fixed1.version() {
+                1 | 2 => {
+                    let fixed_data_size = usize::from(fixed1.fixed_data_size());
+                    let pmgr_rom_addr_offset = fixed_data_size + IFR_SW_EXT_ROM_ADDR_OFFSET;
+                    bar0.try_read32(Self::ROM_OFFSET + pmgr_rom_addr_offset)
+                        .map(usize::from_safe_cast)
+                }
+                3 => {
+                    let fixed2 = bar0.read(NV_PBUS_IFR_FMT_FIXED2);
+                    let total_data_size = usize::from(fixed2.total_data_size());
+                    let flash_status_offset =
+                        usize::from_safe_cast(bar0.try_read32(Self::ROM_OFFSET + total_data_size)?);
+                    let dir_offset = flash_status_offset + RFW_FLASH_STATUS_SIZE;
+                    let dir_sig = bar0.try_read32(Self::ROM_OFFSET + dir_offset)?;
+                    if dir_sig != NV_ROM_DIRECTORY_IDENTIFIER {
+                        dev_err!(dev, "could not find IFR ROM directory\n");
+                        return Err(EINVAL);
+                    }
+                    bar0.try_read32(Self::ROM_OFFSET + dir_offset + PCI_OPTION_ROM_OFFSET)
+                        .map(usize::from_safe_cast)
+                }
+                _ => {
+                    dev_err!(dev, "unsupported IFR header version {}\n", fixed1.version());
+                    Err(EINVAL)
+                }
+            }
+        } else {
+            Ok(0)
+        }
+    }
+
     fn new(dev: &'a device::Device, bar0: &'a Bar0) -> Result<Self> {
         Ok(Self {
             dev,
             bar0,
             data: KVec::new(),
-            current_offset: vbios_rom_offset(dev, bar0)?,
+            current_offset: Self::rom_offset(dev, bar0)?,
             last_found: false,
         })
     }
@@ -186,7 +174,7 @@ fn read_more(&mut self, len: usize) -> Result {
         let start = self.data.len();
         let end = start + len;
 
-        if end > BIOS_MAX_SCAN_LEN {
+        if end > Self::BIOS_MAX_SCAN_LEN {
             dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
             return Err(EINVAL);
         }
@@ -205,7 +193,7 @@ fn read_more(&mut self, len: usize) -> Result {
         // Read ROM data bytes and push directly to `data`.
         for addr in (start..end).step_by(core::mem::size_of::<u32>()) {
             // Read 32-bit word from the VBIOS ROM
-            let word = self.bar0.try_read32(ROM_OFFSET + addr)?;
+            let word = self.bar0.try_read32(Self::ROM_OFFSET + addr)?;
 
             // Convert the `u32` to a 4 byte array and push each byte.
             word.to_ne_bytes()
@@ -267,7 +255,7 @@ fn next(&mut self) -> Option<Self::Item> {
             return None;
         }
 
-        if self.current_offset >= BIOS_MAX_SCAN_LEN {
+        if self.current_offset >= Self::BIOS_MAX_SCAN_LEN {
             dev_err!(self.dev, "Error: exceeded BIOS scan limit, stopping scan\n");
             return None;
         }
@@ -275,7 +263,7 @@ fn next(&mut self) -> Option<Self::Item> {
         // Parse image headers first to get image size.
         let image_size = match self.read_bios_image_at_offset(
             self.current_offset,
-            BIOS_READ_AHEAD_SIZE,
+            Self::BIOS_READ_AHEAD_SIZE,
             "parse initial BIOS image headers",
         ) {
             Ok(image) => image.image_size_bytes(),
@@ -416,6 +404,9 @@ struct PcirStruct {
 unsafe impl FromBytes for PcirStruct {}
 
 impl PcirStruct {
+    /// The bit in `last_image` that indicates the last image.
+    const LAST_IMAGE_BIT_MASK: u8 = 0x80;
+
     fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
         let (pcir, _) = PcirStruct::from_bytes_copy_prefix(data).ok_or(EINVAL)?;
 
@@ -439,7 +430,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
 
     /// Check if this is the last image in the ROM.
     fn is_last(&self) -> bool {
-        self.last_image & LAST_IMAGE_BIT_MASK != 0
+        self.last_image & Self::LAST_IMAGE_BIT_MASK != 0
     }
 
     /// Calculate image size in bytes from 512-byte blocks.
@@ -505,10 +496,10 @@ struct BitToken {
 // SAFETY: all bit patterns are valid for `BitToken`.
 unsafe impl FromBytes for BitToken {}
 
-// Define the token ID for the Falcon data
-const BIT_TOKEN_ID_FALCON_DATA: u8 = 0x70;
-
 impl BitToken {
+    /// BIT token ID for Falcon data.
+    const ID_FALCON_DATA: u8 = 0x70;
+
     /// 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;
@@ -604,6 +595,9 @@ struct NpdeStruct {
 unsafe impl FromBytes for NpdeStruct {}
 
 impl NpdeStruct {
+    /// The bit in `last_image` that indicates the last image.
+    const LAST_IMAGE_BIT_MASK: u8 = 0x80;
+
     fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
         let (npde, _) = NpdeStruct::from_bytes_copy_prefix(data)?;
 
@@ -627,7 +621,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
 
     /// Check if this is the last image in the ROM.
     fn is_last(&self) -> bool {
-        self.last_image & LAST_IMAGE_BIT_MASK != 0
+        self.last_image & Self::LAST_IMAGE_BIT_MASK != 0
     }
 
     /// Calculate image size in bytes from 512-byte blocks.
@@ -799,7 +793,7 @@ fn get_bit_token(&self, token_id: u8) -> Result<BitToken> {
     /// 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)?;
+        let token = self.get_bit_token(BitToken::ID_FALCON_DATA)?;
         let offset = usize::from(token.data_offset);
 
         // Read the 4-byte falcon data pointer at the offset specified in the token.
@@ -846,6 +840,17 @@ struct PmuLookupTableEntry {
 // SAFETY: all bit patterns are valid for `PmuLookupTableEntry`.
 unsafe impl FromBytes for PmuLookupTableEntry {}
 
+impl PmuLookupTableEntry {
+    /// PMU lookup table application ID for firmware security license ucode.
+    #[expect(dead_code)]
+    const APPID_FIRMWARE_SEC_LIC: u8 = 0x05;
+    /// PMU lookup table application ID for debug FWSEC ucode.
+    #[expect(dead_code)]
+    const APPID_FWSEC_DBG: u8 = 0x45;
+    /// PMU lookup table application ID for production FWSEC ucode.
+    const APPID_FWSEC_PROD: u8 = 0x85;
+}
+
 #[repr(C)]
 struct PmuLookupTableHeader {
     version: u8,
@@ -923,7 +928,7 @@ fn new(
         let pmu_lookup_table = PmuLookupTable::new(&second_fwsec.dev, pmu_lookup_data)?;
 
         let entry = pmu_lookup_table
-            .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
+            .find_entry_by_type(PmuLookupTableEntry::APPID_FWSEC_PROD)
             .inspect_err(|e| {
                 dev_err!(
                     second_fwsec.dev,

-- 
2.54.0

Re: [PATCH v4 14/20] gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images

[John Hubbard]

On 5/18/26 7:55 PM, 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.
> 
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 98 +++++++++++++++++-------------------------
>  1 file changed, 39 insertions(+), 59 deletions(-)
...> @@ -353,15 +349,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

Both this patch, and patch 16/20 are doing a tiny bit of work to
preserve this printing. And that looks good.

However, after careful consideration over several months, I have
come to believe that this printing is no longer earning its keep,
even for debug-level printing.

A significant fraction of the dmesg debug level output is consumed
by this one print, for example:

nova-core 0000:01:00.0: PmuLookupTableEntry desc: V3(
    FalconUCodeDescV3 {
        hdr: 78381825,
        stored_size: 59904,
        pkc_data_offset: 1444,
        interface_offset: 28,
        imem_phys_base: 0,
        imem_load_size: 57856,
        imem_virt_base: 0,
        dmem_phys_base: 0,
        dmem_load_size: 2048,
        engine_id_mask: 1024,
        ucode_id: 9,
        signature_count: 3,
        signature_versions: 7,
        _reserved: 37449,
    },
)

...and yet it is exceedingly rare to make use of that particular
data, even when debugging.

Let's just delete it. As always, bringup people can add it back in
temporarily if they need it. But they likely never will, because new
hardware doesn't hit this path anyway.



> +                let desc = fwsec_image.header()?;
> +                dev_dbg!(
> +                    fwsec_image.base.dev,
> +                    "PmuLookupTableEntry desc: {:#?}\n",
> +                    desc
> +                );
> +            }
> +
thanks,
-- 
John Hubbard

Re: [PATCH v4 14/20] gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images

[John Hubbard]

On 5/18/26 7:55 PM, 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.
> 
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 98 +++++++++++++++++-------------------------
>  1 file changed, 39 insertions(+), 59 deletions(-)
...
> -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

This needs a period at the end of the sentence.

The patch looks good, I had to look pretty hard in order find that
tiny nit. :)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> +    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()..)
>          }
>          .ok_or(EINVAL)?;
>  
> -        let pmu_lookup_table = PmuLookupTable::new(&self.base.dev, pmu_lookup_data)?;
> +        let pmu_lookup_table = PmuLookupTable::new(&second_fwsec.dev, pmu_lookup_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
>                  );
> @@ -987,34 +980,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 v4 01/20] gpu: nova-core: vbios: stop scanning at BIOS_MAX_SCAN_LEN

[John Hubbard]

On 5/18/26 7:54 PM, Eliot Courtney wrote:
> Current code lets `current_offset` go to `BIOS_MAX_SCAN_LEN` which is
> one byte too far.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 6bcfb6c5cf44..7bec81a37340 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -272,7 +272,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;
>          }
> 

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

Re: [PATCH v4 02/20] gpu: nova-core: vbios: use checked arithmetic for bios image range end

[John Hubbard]

On 5/18/26 7:54 PM, Eliot Courtney wrote:
> `read_bios_image_at_offset` is called with a length from the VBIOS
> header, so we should be more defensive here and use checked arithmetic.
> 
> 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 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 7bec81a37340..180928433766 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -238,8 +238,8 @@ fn read_bios_image_at_offset(
>          len: usize,
>          context: &str,
>      ) -> Result<BiosImage> {
> -        let data_len = self.data.len();
> -        if offset + len > data_len {
> +        let end = offset.checked_add(len).ok_or(EINVAL)?;
> +        if end > self.data.len() {
>              self.read_more_at_offset(offset, len).inspect_err(|e| {
>                  dev_err!(
>                      self.dev,
> @@ -250,7 +250,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",
> 

Re: [PATCH v4 03/20] gpu: nova-core: vbios: avoid reading too far in read_more_at_offset

[John Hubbard]

On 5/18/26 7:54 PM, Eliot Courtney wrote:
> 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`.
> 
> Fixes: 6fda04e7f0cd ("gpu: nova-core: vbios: Add base support for VBIOS construction and iteration")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 180928433766..79eb01dabc6f 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -185,8 +185,13 @@ fn new(dev: &'a device::Device, bar0: &'a Bar0) -> Result<Self> {
>  
>      /// Read bytes from the ROM at the current end of the data vector.
>      fn read_more(&mut self, len: usize) -> Result {
> -        let current_len = self.data.len();
> -        let start = ROM_OFFSET + current_len;
> +        let start = self.data.len();
> +        let end = start + len;
> +
> +        if end > BIOS_MAX_SCAN_LEN {
> +            dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
> +            return Err(EINVAL);
> +        }
>  
>          // Ensure length is a multiple of 4 for 32-bit reads
>          if len % core::mem::size_of::<u32>() != 0 {
> @@ -200,9 +205,9 @@ fn read_more(&mut self, len: usize) -> Result {
>  
>          self.data.reserve(len, GFP_KERNEL)?;
>          // Read ROM data bytes and push directly to `data`.
> -        for addr in (start..start + len).step_by(core::mem::size_of::<u32>()) {
> +        for addr in (start..end).step_by(core::mem::size_of::<u32>()) {
>              // Read 32-bit word from the VBIOS ROM
> -            let word = self.bar0.try_read32(addr)?;
> +            let word = self.bar0.try_read32(ROM_OFFSET + addr)?;
>  
>              // Convert the `u32` to a 4 byte array and push each byte.
>              word.to_ne_bytes()
> @@ -215,17 +220,9 @@ 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 {
> -            dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
> -            return Err(EINVAL);
> -        }
> +        let end = offset.checked_add(len).ok_or(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.
> 

Re: [PATCH v4 04/20] gpu: nova-core: vbios: read BitToken using FromBytes

[John Hubbard]

On 5/18/26 7:54 PM, Eliot Courtney wrote:
> If `header.token_size` is smaller than `BitToken`, then we currently can
> read past the end of `image.base.data`. Use checked arithmetic for
> computing offsets and simplify reading it in using `FromBytes`.
> 
> 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 | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 79eb01dabc6f..2ff67273fdff 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -486,7 +486,7 @@ fn new(data: &[u8]) -> Result<Self> {
>  
>  /// BIT Token Entry: Records in the BIT table followed by the BIT header.
>  #[derive(Debug, Clone, Copy)]
> -#[expect(dead_code)]
> +#[repr(C)]
>  struct BitToken {
>      /// 00h: Token identifier
>      id: u8,
> @@ -498,6 +498,9 @@ struct BitToken {
>      data_offset: u16,
>  }
>  
> +// SAFETY: all bit patterns are valid for `BitToken`.
> +unsafe impl FromBytes for BitToken {}
> +
>  // Define the token ID for the Falcon data
>  const BIT_TOKEN_ID_FALCON_DATA: u8 = 0x70;
>  
> @@ -505,32 +508,28 @@ 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);
>  
>          // 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));
> +            let entry_offset = i
> +                .checked_mul(entry_size)
> +                .and_then(|offset| tokens_start.checked_add(offset))
> +                .ok_or(EINVAL)?;
> +            let entry = image
> +                .base
> +                .data
> +                .get(entry_offset..)
> +                .and_then(|data| data.get(..entry_size))
> +                .ok_or(EINVAL)?;
>  
> -            // 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 (token, _) = BitToken::from_bytes_copy_prefix(entry).ok_or(EINVAL)?;
>  
>              // Check if this token has the requested ID
> -            if image.base.data[entry_offset] == 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],
> -                    ]),
> -                });
> +            if token.id == token_id {
> +                return Ok(token);
>              }
>          }
>  
> 

Re: [PATCH v4 05/20] gpu: nova-core: vbios: use checked ops and accesses in `FwSecBiosImage::ucode`

[John Hubbard]

On 5/18/26 7:54 PM, Eliot Courtney wrote:
> 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 | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 2ff67273fdff..c62d918a3041 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -1110,16 +1110,18 @@ 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;
> +        let size = usize::from_safe_cast(
> +            desc.imem_load_size()
> +                .checked_add(desc.dmem_load_size())
> +                .ok_or(ERANGE)?,
> +        );
>  
>          // 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());
> -
> -        // Get the data slice, checking bounds in a single operation.
>          self.base
>              .data
> -            .get(ucode_data_offset..ucode_data_offset + size)
> +            .get(self.falcon_ucode_offset..)
> +            .and_then(|data| data.get(desc.size()..))
> +            .and_then(|data| data.get(..size))
>              .ok_or(ERANGE)
>              .inspect_err(|_| {
>                  dev_err!(
> 

Re: [PATCH v4 06/20] gpu: nova-core: vbios: use checked access in `FwSecBiosImage::header`

[John Hubbard]

On 5/18/26 7:55 PM, 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")
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index c62d918a3041..48a46684e279 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -1077,17 +1077,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)
> 

Re: [PATCH v4 07/20] gpu: nova-core: vbios: use checked accesses in `setup_falcon_data`

[John Hubbard]

On 5/18/26 7:55 PM, 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")
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 48a46684e279..871f455bb720 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -1036,14 +1036,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 v4 08/20] gpu: nova-core: vbios: drop unused falcon_data_offset from FwSecBiosBuilder

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> This is unused, so we can remove it.
> 
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 5 -----
>  1 file changed, 5 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 871f455bb720..8a0e16e6c9e8 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -338,7 +338,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,
>                      };
> @@ -712,8 +711,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.
> @@ -1015,8 +1012,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 v4 09/20] gpu: nova-core: vbios: keep PmuLookupTable local in setup_falcon_data

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> This does not need to be stored in `FwSecBiosBuilder` so we can remove
> it from there, and just create and use it locally in
> `setup_falcon_data`.
> 
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 8a0e16e6c9e8..cadc6dcffefb 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -338,7 +338,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() {
> @@ -711,8 +710,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>,
>  }
> @@ -1012,24 +1009,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 v4 10/20] gpu: nova-core: vbios: compute FWSEC-relative Falcon data offset

[John Hubbard]

On 5/18/26 7:55 PM, 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(-)
> 

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index cadc6dcffefb..ca101b2b6095 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -846,33 +846,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");
> +            })
>      }
>  }
>  
> @@ -989,15 +985,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 v4 11/20] gpu: nova-core: vbios: simplify setup_falcon_data

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> The code first computes `pmu_in_first_fwsec` or adjusts the offset and
> then uses it in a branch just once to get the correct source for the PMU
> table. This can be simplified to a single branch while also avoiding the
> mutation of `offset`. Also, adjust the code after this to keep the
> success case non-nested.
> 
> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 54 ++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 31 deletions(-)
> 

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index ca101b2b6095..470e0e2a81ab 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -985,48 +985,40 @@ 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();
> +            self.base.data.get(offset - first_fwsec.base.data.len()..)
>          }
> +        .ok_or(EINVAL)?;
>  
> -        let pmu_lookup_data = if pmu_in_first_fwsec {
> -            &first_fwsec.base.data[offset..]
> -        } else {
> -            self.base.data.get(offset..).ok_or(EINVAL)?
> -        };
>          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 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 v4 12/20] gpu: nova-core: vbios: read PMU lookup entries using FromBytes

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> This simplifies the construction of `PmuLookupTableEntry` and is
> allowed now that the driver can assume it is little endian.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 470e0e2a81ab..987eb1948314 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -897,19 +897,8 @@ struct PmuLookupTableEntry {
>      data: u32,
>  }
>  
> -impl PmuLookupTableEntry {
> -    fn new(data: &[u8]) -> Result<Self> {
> -        if data.len() < core::mem::size_of::<Self>() {
> -            return Err(EINVAL);
> -        }
> -
> -        Ok(PmuLookupTableEntry {
> -            application_id: data[0],
> -            target_id: data[1],
> -            data: u32::from_le_bytes(data[2..6].try_into().map_err(|_| EINVAL)?),
> -        })
> -    }
> -}
> +// SAFETY: all bit patterns are valid for `PmuLookupTableEntry`.
> +unsafe impl FromBytes for PmuLookupTableEntry {}
>  
>  #[repr(C)]
>  struct PmuLookupTableHeader {
> @@ -963,7 +952,13 @@ fn lookup_index(&self, idx: u8) -> Result<PmuLookupTableEntry> {
>          }
>  
>          let index = (usize::from(idx)) * usize::from(self.header.entry_len);
> -        PmuLookupTableEntry::new(&self.table_data[index..])
> +        let (entry, _) = self
> +            .table_data
> +            .get(index..)
> +            .and_then(PmuLookupTableEntry::from_bytes_copy_prefix)
> +            .ok_or(EINVAL)?;
> +
> +        Ok(entry)
>      }
>  
>      // find entry by type value
> 

Re: [PATCH v4 13/20] gpu: nova-core: vbios: store PMU lookup entries in a KVVec

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> The current code copies the data into a KVec and parses it on demand. We
> can simplify the code by storing the parsed entries.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 56 ++++++++++++++----------------------------
>  1 file changed, 18 insertions(+), 38 deletions(-)

This is a nice improvement.

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard


> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 987eb1948314..1ffaf0ef56a7 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -917,8 +917,7 @@ unsafe impl FromBytes for PmuLookupTableHeader {}
>  /// The table of entries is pointed to by the falcon data pointer in the BIT table, and is used to
>  /// locate the Falcon Ucode.
>  struct PmuLookupTable {
> -    header: PmuLookupTableHeader,
> -    table_data: KVec<u8>,
> +    entries: KVVec<PmuLookupTableEntry>,
>  }
>  
>  impl PmuLookupTable {
> @@ -929,48 +928,29 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>          let entry_len = usize::from(header.entry_len);
>          let entry_count = usize::from(header.entry_count);
>  
> -        let required_bytes = header_len + (entry_count * entry_len);
> +        let data = data
> +            .get(header_len..header_len + entry_count * entry_len)
> +            .ok_or(EINVAL)
> +            .inspect_err(|_| {
> +                dev_err!(dev, "PmuLookupTable data length less than required\n");
> +            })?;
>  
> -        if data.len() < required_bytes {
> -            dev_err!(dev, "PmuLookupTable data length less than required\n");
> -            return Err(EINVAL);
> +        let mut entries = KVVec::with_capacity(entry_count, GFP_KERNEL)?;
> +        for i in 0..entry_count {
> +            let (entry, _) = PmuLookupTableEntry::from_bytes_copy_prefix(&data[i * entry_len..])
> +                .ok_or(EINVAL)?;
> +            entries.push(entry, GFP_KERNEL)?;
>          }
>  
> -        // Create a copy of only the table data
> -        let table_data = {
> -            let mut ret = KVec::new();
> -            ret.extend_from_slice(&data[header_len..required_bytes], GFP_KERNEL)?;
> -            ret
> -        };
> -
> -        Ok(PmuLookupTable { header, table_data })
> -    }
> -
> -    fn lookup_index(&self, idx: u8) -> Result<PmuLookupTableEntry> {
> -        if idx >= self.header.entry_count {
> -            return Err(EINVAL);
> -        }
> -
> -        let index = (usize::from(idx)) * usize::from(self.header.entry_len);
> -        let (entry, _) = self
> -            .table_data
> -            .get(index..)
> -            .and_then(PmuLookupTableEntry::from_bytes_copy_prefix)
> -            .ok_or(EINVAL)?;
> -
> -        Ok(entry)
> +        Ok(PmuLookupTable { entries })
>      }
>  
>      // find entry by type value
> -    fn find_entry_by_type(&self, entry_type: u8) -> Result<PmuLookupTableEntry> {
> -        for i in 0..self.header.entry_count {
> -            let entry = self.lookup_index(i)?;
> -            if entry.application_id == entry_type {
> -                return Ok(entry);
> -            }
> -        }
> -
> -        Err(EINVAL)
> +    fn find_entry_by_type(&self, entry_type: u8) -> Result<&PmuLookupTableEntry> {
> +        self.entries
> +            .iter()
> +            .find(|entry| entry.application_id == entry_type)
> +            .ok_or(EINVAL)
>      }
>  }
>  
> 

Re: [PATCH v4 16/20] gpu: nova-core: vbios: use let-else in Vbios::new

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> Improve readability by moving the success path outside of a nested
> branch.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index ff21f85fdfb6..64d100b6699b 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -353,30 +353,30 @@ 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(second), Some(first), Some(pci_at)) =
> +        let (Some(second), Some(first), Some(pci_at)) =
>              (second_fwsec_image, first_fwsec_image, pci_at_image)
> -        {
> -            let fwsec_image = FwSecBiosImage::new(pci_at, first, second)
> -                .inspect_err(|e| dev_err!(dev, "Falcon data setup failed: {:?}\n", e))?;
> -
> -            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 {
> +        else {
>              dev_err!(
>                  dev,
>                  "Missing required images for falcon data setup, skipping\n"
>              );
> -            Err(EINVAL)
> +            return Err(EINVAL);
> +        };
> +
> +        let fwsec_image = FwSecBiosImage::new(pci_at, first, second)
> +            .inspect_err(|e| dev_err!(dev, "Falcon data setup failed: {:?}\n", e))?;
> +
> +        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 })
>      }
>  
>      pub(crate) fn fwsec_image(&self) -> &FwSecBiosImage {
> 

Re: [PATCH v4 17/20] gpu: nova-core: vbios: remove unnecessary fields in PciRomHeader

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> Remove unnecessary fields in PciRomHeader. This allows a simplification
> to use `FromBytes` instead of reading fields piecemeal. A lot of these
> checks were redundant as well since it checks the size of the `data`
> first in `BiosImage`.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 68 ++++++++++--------------------------------
>  1 file changed, 16 insertions(+), 52 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 64d100b6699b..1dca7933fac5 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -546,67 +546,38 @@ fn from_id(image: &PciAtBiosImage, token_id: u8) -> Result<Self> {
>  
>  /// PCI ROM Expansion Header as defined in PCI Firmware Specification.
>  ///
> -/// This is header is at the beginning of every image in the set of images in the ROM. It contains
> -/// a pointer to the PCI Data Structure which describes the image. For "NBSI" images (NoteBook
> -/// System Information), the ROM header deviates from the standard and contains an offset to the
> -/// NBSI image however we do not yet parse that in this module and keep it for future reference.
> +/// This header is at the beginning of every image in the set of images in the ROM. It contains a
> +/// pointer to the PCI Data Structure which describes the image.
>  #[derive(Debug, Clone, Copy)]
> -#[expect(dead_code)]
> +#[repr(C)]
>  struct PciRomHeader {
>      /// 00h: Signature (0xAA55)
>      signature: u16,
> -    /// 02h: Reserved bytes for processor architecture unique data (20 bytes)
> -    reserved: [u8; 20],
> -    /// 16h: NBSI Data Offset (NBSI-specific, offset from header to NBSI image)
> -    nbsi_data_offset: Option<u16>,
> +    /// 02h: Reserved bytes for processor architecture unique data (22 bytes)
> +    reserved: [u8; 22],
>      /// 18h: Pointer to PCI Data Structure (offset from start of ROM image)
>      pci_data_struct_offset: u16,
> -    /// 1Ah: Size of block (this is NBSI-specific)
> -    size_of_block: Option<u32>,
>  }
>  
> +// SAFETY: all bit patterns are valid for `PciRomHeader`.
> +unsafe impl FromBytes for PciRomHeader {}
> +
>  impl PciRomHeader {
>      fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
> -        if data.len() < 26 {
> -            // Need at least 26 bytes to read pciDataStrucPtr and sizeOfBlock.
> -            return Err(EINVAL);
> -        }
> -
> -        let signature = u16::from_le_bytes([data[0], data[1]]);
> +        let (rom_header, _) = PciRomHeader::from_bytes_copy_prefix(data)
> +            .ok_or(EINVAL)
> +            .inspect_err(|_| dev_err!(dev, "Not enough data for ROM header\n"))?;
>  
>          // Check for valid ROM signatures.
> -        match signature {
> +        match rom_header.signature {
>              0xAA55 | 0x4E56 => {}
>              _ => {
> -                dev_err!(dev, "ROM signature unknown {:#x}\n", signature);
> +                dev_err!(dev, "ROM signature unknown {:#x}\n", rom_header.signature);
>                  return Err(EINVAL);
>              }
>          }
>  
> -        // Read the pointer to the PCI Data Structure at offset 0x18.
> -        let pci_data_struct_ptr = u16::from_le_bytes([data[24], data[25]]);
> -
> -        // Try to read optional fields if enough data.
> -        let mut size_of_block = None;
> -        let mut nbsi_data_offset = None;
> -
> -        if data.len() >= 30 {
> -            // Read size_of_block at offset 0x1A.
> -            size_of_block = Some(u32::from_le_bytes([data[26], data[27], data[28], data[29]]));
> -        }
> -
> -        // For NBSI images, try to read the nbsiDataOffset at offset 0x16.
> -        if data.len() >= 24 {
> -            nbsi_data_offset = Some(u16::from_le_bytes([data[22], data[23]]));
> -        }
> -
> -        Ok(PciRomHeader {
> -            signature,
> -            reserved: [0u8; 20],
> -            pci_data_struct_offset: pci_data_struct_ptr,
> -            size_of_block,
> -            nbsi_data_offset,
> -        })
> +        Ok(rom_header)
>      }
>  }
>  
> @@ -722,11 +693,11 @@ pub(crate) struct FwSecBiosImage {
>  /// BIOS Image structure containing various headers and reference fields to all BIOS images.
>  ///
>  /// A BiosImage struct is embedded into all image types and implements common operations.
> -#[expect(dead_code)]
>  struct BiosImage {
>      /// Used for logging.
>      dev: ARef<device::Device>,
>      /// PCI ROM Expansion Header
> +    #[expect(dead_code)]
>      rom_header: PciRomHeader,
>      /// PCI Data Structure
>      pcir: PcirStruct,
> @@ -771,15 +742,8 @@ fn is_last(&self) -> bool {
>  
>      /// Creates a new BiosImage from raw byte data.
>      fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
> -        // Ensure we have enough data for the ROM header.
> -        if data.len() < 26 {
> -            dev_err!(dev, "Not enough data for ROM header\n");
> -            return Err(EINVAL);
> -        }
> -
>          // Parse the ROM header.
> -        let rom_header = PciRomHeader::new(dev, &data[0..26])
> -            .inspect_err(|e| dev_err!(dev, "Failed to create PciRomHeader: {:?}\n", e))?;
> +        let rom_header = PciRomHeader::new(dev, data)?;
>  
>          // Get the PCI Data Structure using the pointer from the ROM header.
>          let pcir_offset = usize::from(rom_header.pci_data_struct_offset);
> 

Re: [PATCH v4 18/20] gpu: nova-core: vbios: drop unused image wrappers

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> These are unused currently, and it is probably sufficient to just check
> the type of BIOS image in the future.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 12 ------------
>  1 file changed, 12 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard

> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 1dca7933fac5..ad571b39400d 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -669,18 +669,6 @@ struct PciAtBiosImage {
>      bit_offset: usize,
>  }
>  
> -#[expect(dead_code)]
> -struct EfiBiosImage {
> -    base: BiosImage,
> -    // EFI-specific fields can be added here in the future.
> -}
> -
> -#[expect(dead_code)]
> -struct NbsiBiosImage {
> -    base: BiosImage,
> -    // NBSI-specific fields can be added here in the future.
> -}
> -
>  /// 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.
> 

Re: [PATCH v4 20/20] gpu: nova-core: vbios: move constants and functions to be associated

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> Move constants and functions to be inside the impls of the types they
> are related to. This makes it more obvious what each type and value is
> for.
> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  Documentation/gpu/nova/core/vbios.rst |   2 +-
>  drivers/gpu/nova-core/vbios.rs        | 185 +++++++++++++++++-----------------
>  2 files changed, 96 insertions(+), 91 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>

thanks,
-- 
John Hubbard


> 
> diff --git a/Documentation/gpu/nova/core/vbios.rst b/Documentation/gpu/nova/core/vbios.rst
> index a4fe63422ede..9d3379ccfb30 100644
> --- a/Documentation/gpu/nova/core/vbios.rst
> +++ b/Documentation/gpu/nova/core/vbios.rst
> @@ -232,7 +232,7 @@ Falcon data in the VBIOS which contains the PMU lookup table. This lookup table
>  used to find the required Falcon ucode based on an application ID.
>  
>  The location of the PMU lookup table is found by scanning the BIT (`BIOS Information Table`_)
> -tokens for a token with the id `BIT_TOKEN_ID_FALCON_DATA` (0x70) which indicates the
> +tokens for a token with the Falcon data token id (0x70) which indicates the
>  offset of the same from the start of the VBIOS image. Unfortunately, the offset
>  does not account for the EFI image located between the PciAt and FwSec images.
>  The `vbios.rs` code compensates for this with appropriate arithmetic.
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index 9cc2f008bbfb..07b5235faff8 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -27,16 +27,6 @@
>      num::FromSafeCast,
>  };
>  
> -/// The offset of the VBIOS ROM in the BAR0 space.
> -const ROM_OFFSET: usize = 0x300000;
> -/// The maximum length of the VBIOS ROM to scan into.
> -const BIOS_MAX_SCAN_LEN: usize = 0x100000;
> -/// The size to read ahead when parsing initial BIOS image headers.
> -const BIOS_READ_AHEAD_SIZE: usize = 1024;
> -/// The bit in the last image indicator byte for the PCI Data Structure that
> -/// indicates the last image. Bit 0-6 are reserved, bit 7 is last image bit.
> -const LAST_IMAGE_BIT_MASK: u8 = 0x80;
> -
>  /// BIOS Image Type from PCI Data Structure code_type field.
>  #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>  #[repr(u8)]
> @@ -65,14 +55,6 @@ fn try_from(code: u8) -> Result<Self> {
>      }
>  }
>  
> -// PMU lookup table entry types. Used to locate PMU table entries
> -// in the Fwsec image, corresponding to falcon ucodes.
> -#[expect(dead_code)]
> -const FALCON_UCODE_ENTRY_APPID_FIRMWARE_SEC_LIC: u8 = 0x05;
> -#[expect(dead_code)]
> -const FALCON_UCODE_ENTRY_APPID_FWSEC_DBG: u8 = 0x45;
> -const FALCON_UCODE_ENTRY_APPID_FWSEC_PROD: u8 = 0x85;
> -
>  /// Vbios Reader for constructing the VBIOS data.
>  struct VbiosIterator<'a> {
>      dev: &'a device::Device,
> @@ -110,73 +92,79 @@ struct VbiosIterator<'a> {
>      }
>  }
>  
> -/// Return the byte offset where the PCI Expansion ROM images begin in the GPU's ROM.
> -///
> -/// The GPU's ROM may begin with an Init-from-ROM (IFR) header that precedes
> -/// the PCI Expansion ROM images (VBIOS).  When present, the PROM shadow
> -/// method must parse this header to determine the offset where the PCI ROM
> -/// images actually begin, and adjust all subsequent reads accordingly.
> -///
> -/// On most GPUs this is not needed because the IFR microcode has already
> -/// applied the ROM offset so that PROM reads transparently skip the header.
> -/// On GA100, for some reason, the IFR offset is not applied to PROM
> -/// reads.  Therefore, the search for the PCI expansion must skip the IFR
> -/// header, if found.
> -fn vbios_rom_offset(dev: &device::Device, bar0: &Bar0) -> Result<usize> {
> -    /// IFR signature.
> -    const NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE: u32 = u32::from_le_bytes(*b"NVGI");
> -    /// ROM directory signature.
> -    const NV_ROM_DIRECTORY_IDENTIFIER: u32 = u32::from_le_bytes(*b"RFRD");
> -    /// Offset of the NV_PMGR_ROM_ADDR_OFFSET register in IFR Extended section.
> -    const IFR_SW_EXT_ROM_ADDR_OFFSET: usize = 4;
> -    /// Size of Redundant Firmware Flash Status section.
> -    const RFW_FLASH_STATUS_SIZE: usize = SZ_4K;
> -    /// Offset in the ROM Directory of the PCI Option ROM offset
> -    const PCI_OPTION_ROM_OFFSET: usize = 8;
> -
> -    let signature = bar0.read(NV_PBUS_IFR_FMT_FIXED0).signature();
> -
> -    if signature == NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE {
> -        let fixed1 = bar0.read(NV_PBUS_IFR_FMT_FIXED1);
> -
> -        match fixed1.version() {
> -            1 | 2 => {
> -                let fixed_data_size = usize::from(fixed1.fixed_data_size());
> -                let pmgr_rom_addr_offset = fixed_data_size + IFR_SW_EXT_ROM_ADDR_OFFSET;
> -                bar0.try_read32(ROM_OFFSET + pmgr_rom_addr_offset)
> -                    .map(usize::from_safe_cast)
> -            }
> -            3 => {
> -                let fixed2 = bar0.read(NV_PBUS_IFR_FMT_FIXED2);
> -                let total_data_size = usize::from(fixed2.total_data_size());
> -                let flash_status_offset =
> -                    usize::from_safe_cast(bar0.try_read32(ROM_OFFSET + total_data_size)?);
> -                let dir_offset = flash_status_offset + RFW_FLASH_STATUS_SIZE;
> -                let dir_sig = bar0.try_read32(ROM_OFFSET + dir_offset)?;
> -                if dir_sig != NV_ROM_DIRECTORY_IDENTIFIER {
> -                    dev_err!(dev, "could not find IFR ROM directory\n");
> -                    return Err(EINVAL);
> -                }
> -                bar0.try_read32(ROM_OFFSET + dir_offset + PCI_OPTION_ROM_OFFSET)
> -                    .map(usize::from_safe_cast)
> -            }
> -            _ => {
> -                dev_err!(dev, "unsupported IFR header version {}\n", fixed1.version());
> -                Err(EINVAL)
> -            }
> -        }
> -    } else {
> -        Ok(0)
> -    }
> -}
> -
>  impl<'a> VbiosIterator<'a> {
> +    /// The offset of the VBIOS ROM in the BAR0 space.
> +    const ROM_OFFSET: usize = 0x300000;
> +    /// The maximum length of the VBIOS ROM to scan into.
> +    const BIOS_MAX_SCAN_LEN: usize = 0x100000;
> +    /// The size to read ahead when parsing initial BIOS image headers.
> +    const BIOS_READ_AHEAD_SIZE: usize = 1024;
> +
> +    /// Return the byte offset where the PCI Expansion ROM images begin in the GPU's ROM.
> +    ///
> +    /// The GPU's ROM may begin with an Init-from-ROM (IFR) header that precedes the PCI Expansion
> +    /// ROM images (VBIOS). When present, the PROM shadow method must parse this header to determine
> +    /// the offset where the PCI ROM images actually begin, and adjust all subsequent reads
> +    /// accordingly.
> +    ///
> +    /// On most GPUs this is not needed because the IFR microcode has already applied the ROM offset
> +    /// so that PROM reads transparently skip the header. On GA100, for some reason, the IFR offset
> +    /// is not applied to PROM reads. Therefore, the search for the PCI expansion must skip the IFR
> +    /// header, if found.
> +    fn rom_offset(dev: &device::Device, bar0: &Bar0) -> Result<usize> {
> +        /// IFR signature.
> +        const NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE: u32 = u32::from_le_bytes(*b"NVGI");
> +        /// ROM directory signature.
> +        const NV_ROM_DIRECTORY_IDENTIFIER: u32 = u32::from_le_bytes(*b"RFRD");
> +        /// Offset of the NV_PMGR_ROM_ADDR_OFFSET register in IFR Extended section.
> +        const IFR_SW_EXT_ROM_ADDR_OFFSET: usize = 4;
> +        /// Size of Redundant Firmware Flash Status section.
> +        const RFW_FLASH_STATUS_SIZE: usize = SZ_4K;
> +        /// Offset in the ROM Directory of the PCI Option ROM offset.
> +        const PCI_OPTION_ROM_OFFSET: usize = 8;
> +
> +        let signature = bar0.read(NV_PBUS_IFR_FMT_FIXED0).signature();
> +
> +        if signature == NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE {
> +            let fixed1 = bar0.read(NV_PBUS_IFR_FMT_FIXED1);
> +
> +            match fixed1.version() {
> +                1 | 2 => {
> +                    let fixed_data_size = usize::from(fixed1.fixed_data_size());
> +                    let pmgr_rom_addr_offset = fixed_data_size + IFR_SW_EXT_ROM_ADDR_OFFSET;
> +                    bar0.try_read32(Self::ROM_OFFSET + pmgr_rom_addr_offset)
> +                        .map(usize::from_safe_cast)
> +                }
> +                3 => {
> +                    let fixed2 = bar0.read(NV_PBUS_IFR_FMT_FIXED2);
> +                    let total_data_size = usize::from(fixed2.total_data_size());
> +                    let flash_status_offset =
> +                        usize::from_safe_cast(bar0.try_read32(Self::ROM_OFFSET + total_data_size)?);
> +                    let dir_offset = flash_status_offset + RFW_FLASH_STATUS_SIZE;
> +                    let dir_sig = bar0.try_read32(Self::ROM_OFFSET + dir_offset)?;
> +                    if dir_sig != NV_ROM_DIRECTORY_IDENTIFIER {
> +                        dev_err!(dev, "could not find IFR ROM directory\n");
> +                        return Err(EINVAL);
> +                    }
> +                    bar0.try_read32(Self::ROM_OFFSET + dir_offset + PCI_OPTION_ROM_OFFSET)
> +                        .map(usize::from_safe_cast)
> +                }
> +                _ => {
> +                    dev_err!(dev, "unsupported IFR header version {}\n", fixed1.version());
> +                    Err(EINVAL)
> +                }
> +            }
> +        } else {
> +            Ok(0)
> +        }
> +    }
> +
>      fn new(dev: &'a device::Device, bar0: &'a Bar0) -> Result<Self> {
>          Ok(Self {
>              dev,
>              bar0,
>              data: KVec::new(),
> -            current_offset: vbios_rom_offset(dev, bar0)?,
> +            current_offset: Self::rom_offset(dev, bar0)?,
>              last_found: false,
>          })
>      }
> @@ -186,7 +174,7 @@ fn read_more(&mut self, len: usize) -> Result {
>          let start = self.data.len();
>          let end = start + len;
>  
> -        if end > BIOS_MAX_SCAN_LEN {
> +        if end > Self::BIOS_MAX_SCAN_LEN {
>              dev_err!(self.dev, "Error: exceeded BIOS scan limit.\n");
>              return Err(EINVAL);
>          }
> @@ -205,7 +193,7 @@ fn read_more(&mut self, len: usize) -> Result {
>          // Read ROM data bytes and push directly to `data`.
>          for addr in (start..end).step_by(core::mem::size_of::<u32>()) {
>              // Read 32-bit word from the VBIOS ROM
> -            let word = self.bar0.try_read32(ROM_OFFSET + addr)?;
> +            let word = self.bar0.try_read32(Self::ROM_OFFSET + addr)?;
>  
>              // Convert the `u32` to a 4 byte array and push each byte.
>              word.to_ne_bytes()
> @@ -267,7 +255,7 @@ fn next(&mut self) -> Option<Self::Item> {
>              return None;
>          }
>  
> -        if self.current_offset >= BIOS_MAX_SCAN_LEN {
> +        if self.current_offset >= Self::BIOS_MAX_SCAN_LEN {
>              dev_err!(self.dev, "Error: exceeded BIOS scan limit, stopping scan\n");
>              return None;
>          }
> @@ -275,7 +263,7 @@ fn next(&mut self) -> Option<Self::Item> {
>          // Parse image headers first to get image size.
>          let image_size = match self.read_bios_image_at_offset(
>              self.current_offset,
> -            BIOS_READ_AHEAD_SIZE,
> +            Self::BIOS_READ_AHEAD_SIZE,
>              "parse initial BIOS image headers",
>          ) {
>              Ok(image) => image.image_size_bytes(),
> @@ -416,6 +404,9 @@ struct PcirStruct {
>  unsafe impl FromBytes for PcirStruct {}
>  
>  impl PcirStruct {
> +    /// The bit in `last_image` that indicates the last image.
> +    const LAST_IMAGE_BIT_MASK: u8 = 0x80;
> +
>      fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>          let (pcir, _) = PcirStruct::from_bytes_copy_prefix(data).ok_or(EINVAL)?;
>  
> @@ -439,7 +430,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Result<Self> {
>  
>      /// Check if this is the last image in the ROM.
>      fn is_last(&self) -> bool {
> -        self.last_image & LAST_IMAGE_BIT_MASK != 0
> +        self.last_image & Self::LAST_IMAGE_BIT_MASK != 0
>      }
>  
>      /// Calculate image size in bytes from 512-byte blocks.
> @@ -505,10 +496,10 @@ struct BitToken {
>  // SAFETY: all bit patterns are valid for `BitToken`.
>  unsafe impl FromBytes for BitToken {}
>  
> -// Define the token ID for the Falcon data
> -const BIT_TOKEN_ID_FALCON_DATA: u8 = 0x70;
> -
>  impl BitToken {
> +    /// BIT token ID for Falcon data.
> +    const ID_FALCON_DATA: u8 = 0x70;
> +
>      /// 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;
> @@ -604,6 +595,9 @@ struct NpdeStruct {
>  unsafe impl FromBytes for NpdeStruct {}
>  
>  impl NpdeStruct {
> +    /// The bit in `last_image` that indicates the last image.
> +    const LAST_IMAGE_BIT_MASK: u8 = 0x80;
> +
>      fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
>          let (npde, _) = NpdeStruct::from_bytes_copy_prefix(data)?;
>  
> @@ -627,7 +621,7 @@ fn new(dev: &device::Device, data: &[u8]) -> Option<Self> {
>  
>      /// Check if this is the last image in the ROM.
>      fn is_last(&self) -> bool {
> -        self.last_image & LAST_IMAGE_BIT_MASK != 0
> +        self.last_image & Self::LAST_IMAGE_BIT_MASK != 0
>      }
>  
>      /// Calculate image size in bytes from 512-byte blocks.
> @@ -799,7 +793,7 @@ fn get_bit_token(&self, token_id: u8) -> Result<BitToken> {
>      /// 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)?;
> +        let token = self.get_bit_token(BitToken::ID_FALCON_DATA)?;
>          let offset = usize::from(token.data_offset);
>  
>          // Read the 4-byte falcon data pointer at the offset specified in the token.
> @@ -846,6 +840,17 @@ struct PmuLookupTableEntry {
>  // SAFETY: all bit patterns are valid for `PmuLookupTableEntry`.
>  unsafe impl FromBytes for PmuLookupTableEntry {}
>  
> +impl PmuLookupTableEntry {
> +    /// PMU lookup table application ID for firmware security license ucode.
> +    #[expect(dead_code)]
> +    const APPID_FIRMWARE_SEC_LIC: u8 = 0x05;
> +    /// PMU lookup table application ID for debug FWSEC ucode.
> +    #[expect(dead_code)]
> +    const APPID_FWSEC_DBG: u8 = 0x45;
> +    /// PMU lookup table application ID for production FWSEC ucode.
> +    const APPID_FWSEC_PROD: u8 = 0x85;
> +}
> +
>  #[repr(C)]
>  struct PmuLookupTableHeader {
>      version: u8,
> @@ -923,7 +928,7 @@ fn new(
>          let pmu_lookup_table = PmuLookupTable::new(&second_fwsec.dev, pmu_lookup_data)?;
>  
>          let entry = pmu_lookup_table
> -            .find_entry_by_type(FALCON_UCODE_ENTRY_APPID_FWSEC_PROD)
> +            .find_entry_by_type(PmuLookupTableEntry::APPID_FWSEC_PROD)
>              .inspect_err(|e| {
>                  dev_err!(
>                      second_fwsec.dev,
> 

Re: [PATCH v4 15/20] gpu: nova-core: vbios: use the first PCI-AT and FWSEC images

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> Currently, PCI-AT takes the final image if multiple exist. For FWSEC, it
> takes the first one and the last one. Align both of these to nouveau
> behavior by taking the first ones.

Let's do even better, and use Open RM for guidance instead of
nouveau, for this case.

Open RM tracks one offset where the FWSEC region starts and reads
everything past it as one buffer (see s_locateExpansionRoms() in
kernel_gsp_vbios_tu102.c). Matching that in nova-core drops the
first-vs-second-FWSEC dispatch and the second checked_sub in
FwSecBiosImage::new.

This saves about 40 LOC.

I think it's worth doing.

thanks,
-- 
John Hubbard


> 
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/vbios.rs | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index c5dad9e065da..ff21f85fdfb6 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -333,12 +333,16 @@ 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) => {
> -                    pci_at_image = Some(PciAtBiosImage::try_from(image)?);
> +                    // Silently ignore any extra PCI-AT images.
> +                    if pci_at_image.is_none() {
> +                        pci_at_image = Some(PciAtBiosImage::try_from(image)?);
> +                    }
>                  }
>                  Ok(BiosImageType::FwSec) => {
> +                    // Silently ignore any extra FwSec images beyond the first two.
>                      if first_fwsec_image.is_none() {
>                          first_fwsec_image = Some(image);
> -                    } else {
> +                    } else if second_fwsec_image.is_none() {
>                          second_fwsec_image = Some(image);
>                      }
>                  }
> 

Re: [PATCH v4 14/20] gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images

[Eliot Courtney]

On Sat May 23, 2026 at 9:13 AM JST, John Hubbard wrote:
> On 5/18/26 7:55 PM, 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.
>> 
>> Reviewed-by: Joel Fernandes <joelagnelf@nvidia.com>
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/vbios.rs | 98 +++++++++++++++++-------------------------
>>  1 file changed, 39 insertions(+), 59 deletions(-)
> ...> @@ -353,15 +349,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
>
> Both this patch, and patch 16/20 are doing a tiny bit of work to
> preserve this printing. And that looks good.
>
> However, after careful consideration over several months, I have
> come to believe that this printing is no longer earning its keep,
> even for debug-level printing.
>
> A significant fraction of the dmesg debug level output is consumed
> by this one print, for example:
>
> nova-core 0000:01:00.0: PmuLookupTableEntry desc: V3(
>     FalconUCodeDescV3 {
>         hdr: 78381825,
>         stored_size: 59904,
>         pkc_data_offset: 1444,
>         interface_offset: 28,
>         imem_phys_base: 0,
>         imem_load_size: 57856,
>         imem_virt_base: 0,
>         dmem_phys_base: 0,
>         dmem_load_size: 2048,
>         engine_id_mask: 1024,
>         ucode_id: 9,
>         signature_count: 3,
>         signature_versions: 7,
>         _reserved: 37449,
>     },
> )
>
> ...and yet it is exceedingly rare to make use of that particular
> data, even when debugging.
>
> Let's just delete it. As always, bringup people can add it back in
> temporarily if they need it. But they likely never will, because new
> hardware doesn't hit this path anyway.

Yeah, good idea - I am glad to hear tbh. I wanted to delete these when
working on this but I wasn't completely sure what people find useful
for debugging etc so I left it alone. Thanks!