Re: [PATCH v4 5/6] gpu: nova-core: skip the IFR header if present

["Alexandre Courbot" <acourbot@nvidia.com>]

On Fri Apr 17, 2026 at 8:31 AM JST, Timur Tabi wrote:
> 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 whatever reason, the IFR offset is not applied to PROM
> reads.  Therefore, the search for the PCI expansion must skip the IFR
> header, if found.
>
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  drivers/gpu/nova-core/regs.rs  | 21 ++++++++++++
>  drivers/gpu/nova-core/vbios.rs | 60 +++++++++++++++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> index 2f171a4ff9ba..0880d52e4527 100644
> --- a/drivers/gpu/nova-core/regs.rs
> +++ b/drivers/gpu/nova-core/regs.rs

Can you add the definitions in `vbios.rs`? They are not used outside of
that module, and are not exactly registers like the other definitions of
`regs.rs`.

Placing them under the `ga100` module also sounds like they are
exclusive to GA100, which is not the case as the vbios code reads them
unconditionally.

> @@ -537,4 +537,25 @@ pub(crate) mod ga100 {
>              0:0     display_disabled => bool;
>          }
>      }
> +
> +    // IFR Header in VBIOS
> +
> +    register! {
> +        pub(crate) NV_PBUS_IFR_FMT_FIXED0(u32) @ 0x300000 {
> +            31:0    signature;
> +        }
> +    }
> +
> +    register! {
> +        pub(crate) NV_PBUS_IFR_FMT_FIXED1(u32) @ 0x300004 {
> +            30:16   fixed_data_size;
> +            15:8    version => u8;
> +        }
> +    }
> +
> +    register! {
> +        pub(crate) NV_PBUS_IFR_FMT_FIXED2(u32) @ 0x300008 {
> +            19:0 total_data_size;
> +        }
> +    }
>  }
> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> index e726594eb130..7ebe58020533 100644
> --- a/drivers/gpu/nova-core/vbios.rs
> +++ b/drivers/gpu/nova-core/vbios.rs
> @@ -25,6 +25,7 @@
>          FalconUCodeDescV3, //
>      },
>      num::FromSafeCast,
> +    regs::ga100, //
>  };
>  
>  /// The offset of the VBIOS ROM in the BAR0 space.
> @@ -89,13 +90,70 @@ struct VbiosIterator<'a> {
>      last_found: bool,
>  }
>  
> +/// IFR signature: ASCII "NVGI" as a little-endian u32.
> +const NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE: u32 = 0x4947564E;

You can use 

    const NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE: u32 = u32::from_le_bytes(*b"NVGI");

To make the signature explicit in the code (and simplify the doccomment
a bit).

> +/// ROM directory signature: ASCII "RFRD" as a little-endian u32.
> +const NV_ROM_DIRECTORY_IDENTIFIER: u32 = 0x44524652;

Same here.

> +/// 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 = 4096;

`SZ_4K`?

Also can you move these constants into the `current_offset` method, as
they are not used outside of it?

> +
> +/// 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 whatever 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 current_offset(dev: &device::Device, bar0: &Bar0) -> Result<usize> {

The doccomment is very informative. The function name... not so much. :)
Something like `vbios_rom_offset` maybe?

Also style nit: s/whatever/some

> +    let signature = bar0.read(ga100::NV_PBUS_IFR_FMT_FIXED0).signature();
> +
> +    if signature == NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE {
> +        let fixed1 = bar0.read(ga100::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(|v| v as usize)

.map(usize::from_safe_cast)

Each `as` should normally come with its own justifying comment, so
better use the infallible converters we introduced.

> +            }
> +            3 => {
> +                let fixed2 = bar0.read(ga100::NV_PBUS_IFR_FMT_FIXED2);
> +                let total_data_size = usize::from(fixed2.total_data_size());
> +                let dir_offset =
> +                    bar0.try_read32(ROM_OFFSET + total_data_size)? as usize + RFW_FLASH_STATUS_SIZE;

usize::from_safe_cast(bar0.try_read32(ROM_OFFSET + total_data_size)?) + RFW_FLASH_STATUS_SIZE;

(or feel free to use an intermediate value if that doesn't read well)

> +                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 + 8)

Where does this `8` come from? This should be a constant at the very least.

> +                    .map(|v| v as usize)

.map(usize::from_safe_cast)

With these fixed I expect v5 to be in a mergeable state.