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

[Timur Tabi <ttabi@nvidia.com>]

On Mon, 2026-04-13 at 13:53 +0900, Eliot Courtney wrote:
> On Sat Apr 11, 2026 at 5:37 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.
> 
> I like this commit message a lot because it made it really easy for me
> to understand why we are doing this. Could we concisely put a comment in
> the code as well explaining why it's necessary?

Sure, I'll add one for v2.

> > 
> > Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> > ---
> >  drivers/gpu/nova-core/vbios.rs | 39 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
> > index e726594eb130..a7ec68448a4a 100644
> > --- a/drivers/gpu/nova-core/vbios.rs
> > +++ b/drivers/gpu/nova-core/vbios.rs
> > @@ -89,13 +89,50 @@ struct VbiosIterator<'a> {
> >      last_found: bool,
> >  }
> >  
> > +/// IFR signature: ASCII "NVGI" as a little-endian u32.
> > +const IFR_SIGNATURE: u32 = 0x4947564E;
> > +/// ROM directory signature: ASCII "RFRD" as a little-endian u32.
> > +const ROM_DIRECTORY_SIGNATURE: u32 = 0x44524652;
> 
> I wonder if we can use the bindings for some of these constants,
> including the fixed value offsets below. I found these values in OpenRM
> in dev_bus.h, for example (but it was in tu102 so even though the values
> are the same not sure if it's semantically correct):
> 
> ```
> #define NV_PBUS_IFR_FMT_FIXED0                        0x00000000 /*       */
> #define NV_PBUS_IFR_FMT_FIXED0_SIGNATURE                    31:0 /*       */
> #define NV_PBUS_IFR_FMT_FIXED0_SIGNATURE_VALUE        0x4947564E /*       */
> #define NV_PBUS_IFR_FMT_FIXED1                        0x00000004 /*       */
> #define NV_PBUS_IFR_FMT_FIXED1_VERSIONSW                    15:8 /*       */
> #define NV_PBUS_IFR_FMT_FIXED1_FIXED_DATA_SIZE             30:16 /*       */
> #define NV_PBUS_IFR_FMT_FIXED2                        0x00000008 /*       */
> #define NV_PBUS_IFR_FMT_FIXED2_TOTAL_DATA_SIZE              19:0 /*       */
> ```
> 
> Unfortunately IIUC we can't use the bitfield range specifies via
> bindgen, but the FIXEDX offsets and some of the signature values might
> be doable via bindings. WDYT?

I can rename the two macros and add the rest as new constants, but adding them to bindgen.rs seems
overkill.

> >  impl<'a> VbiosIterator<'a> {
> >      fn new(dev: &'a device::Device, bar0: &'a Bar0) -> Result<Self> {
> > +        let sig = bar0.try_read32(ROM_OFFSET)?;
> > +
> > +        let current_offset = if sig == IFR_SIGNATURE {
> > +            let fixed1 = bar0.try_read32(ROM_OFFSET + 4)?;
> > +            let version = ((fixed1 >> 8) & 0xff) as u8;
> > +
> > +            match version {
> > +                // Note: We do not actually expect to see v1 or v2 on these GPUs
> > +                1 | 2 => {
> > +                    let fixed_data_size = ((fixed1 >> 16) & 0x7fff) as usize;
> > +                    bar0.try_read32(ROM_OFFSET + fixed_data_size + 4)? as usize
> 
> Should this be an error instead if we don't expect to see it?

Well, I can't be certain that we'll never actually see it.  For example, my corresponding patch for
Nouveau does not have this comment because it's possible that Nouveau might encounter it since it
supports much older GPUs that actually have an IFR header with those versions. 

> > +                }
> > +                3 => {
> > +                    let fixed2 = bar0.try_read32(ROM_OFFSET + 8)?;
> > +                    let total_data_size = (fixed2 & 0x000f_ffff) as usize;
> 
> What about using the bitfield! macro to define bitfields for these
> things? e.g.
> 
> ```
> bitfield! {
>     struct IfrFixed1(u32) {
>         15:8 version as u8;
>         30:16 fixed_data_size as u16;
>     }
> }
> 
> bitfield! {
>     struct IfrFixed2(u32) {
>         19:0 total_data_size as u32;
>     }
> }
> ```

Yes, that's a good idea.


> > +                    let dir_offset = bar0.try_read32(ROM_OFFSET + total_data_size)? as usize +
> > 4096;
> 
> What about making an inline constant for 4096?

Sure.  I'd have to invent one since RM just hard-codes 4096.

>