Nouveau - Jun 2025 - [PATCH v4 16/20] nova-core: Add support for VBIOS ucode extraction for boot

On Wed, May 21, 2025 at 03:45:11PM +0900, Alexandre Courbot
wrote:
> +impl Vbios {
<snip>
> +    pub(crate) fn fwsec_header(&self, pdev: &device::Device) ->
Result<&FalconUCodeDescV3> {
> +        self.fwsec_image.fwsec_header(pdev)
> +    }
> +
> +    pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) ->
Result<&[u8]> {
> +        self.fwsec_image.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
> +    }
> +
> +    pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) ->
Result<&[u8]> {
> +        self.fwsec_image.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
> +    }
Can't we just implement Deref here? Why do we need this indirection?
> +impl PcirStruct {
> +    fn new(pdev: &pci::Device, data: &[u8]) ->
Result<Self> {
> +        if data.len() < core::mem::size_of::<PcirStruct>() {
> +            dev_err!(pdev.as_ref(), "Not enough data for
PcirStruct\n");
> +            return Err(EINVAL);
> +        }
> +
> +        let mut signature = [0u8; 4];
> +        signature.copy_from_slice(&data[0..4]);
> +
> +        // Signature should be "PCIR" (0x52494350) or
"NPDS" (0x5344504e)
> +        if &signature != b"PCIR" && &signature
!= b"NPDS" {
> +            dev_err!(
> +                pdev.as_ref(),
> +                "Invalid signature for PcirStruct: {:?}\n",
> +                signature
> +            );
> +            return Err(EINVAL);
> +        }
> +
> +        let mut class_code = [0u8; 3];
> +        class_code.copy_from_slice(&data[13..16]);
> +
> +        Ok(PcirStruct {
> +            signature,
> +            vendor_id: u16::from_le_bytes([data[4], data[5]]),
> +            device_id: u16::from_le_bytes([data[6], data[7]]),
> +            device_list_ptr: u16::from_le_bytes([data[8], data[9]]),
> +            pci_data_struct_len: u16::from_le_bytes([data[10], data[11]]),
> +            pci_data_struct_rev: data[12],
> +            class_code,
> +            image_len: u16::from_le_bytes([data[16], data[17]]),
> +            vendor_rom_rev: u16::from_le_bytes([data[18], data[19]]),
> +            code_type: data[20],
> +            last_image: data[21],
> +            max_runtime_image_len: u16::from_le_bytes([data[22],
data[23]]),
> +        })
> +    }
> +
> +    /// Check if this is the last image in the ROM
> +    fn is_last(&self) -> bool {
> +        self.last_image & LAST_IMAGE_BIT_MASK != 0
> +    }
> +
> +    /// Calculate image size in bytes
> +    fn image_size_bytes(&self) -> Result<usize> {
> +        if self.image_len > 0 {
Please make this check when creating the structure...
> +            // Image size is in 512-byte blocks
...and make this a type invariant.
> +            Ok(self.image_len as usize * 512)
It should also be a type invariant that this does not overflow.

The same applies to NpdeStruct.
> +        } else {
> +            Err(EINVAL)
> +        }
> +    }
> +}
<snip>
> +    /// Try to find NPDE in the data, the NPDE is right after the PCIR.
> +    fn find_in_data(
> +        pdev: &pci::Device,
> +        data: &[u8],
> +        rom_header: &PciRomHeader,
> +        pcir: &PcirStruct,
> +    ) -> Option<Self> {
> +        // Calculate the offset where NPDE might be located
> +        // NPDE should be right after the PCIR structure, aligned to 16
bytes
> +        let pcir_offset = rom_header.pci_data_struct_offset as usize;
> +        let npde_start = (pcir_offset + pcir.pci_data_struct_len as usize
+ 0x0F) & !0x0F;
What's this magic offset and mask?
> +
> +        // Check if we have enough data
> +        if npde_start + 11 > data.len() {
'+ 11'?
> +            dev_err!(pdev.as_ref(), "Not enough data for
NPDE\n");
BiosImageBase declares this as "NVIDIA PCI Data Extension (optional)".
If it's
really optional, why is this an error?
> +            return None;
> +        }
> +
> +        // Try to create NPDE from the data
> +        NpdeStruct::new(pdev, &data[npde_start..])
> +            .inspect_err(|e| {
> +                dev_err!(pdev.as_ref(), "Error creating NpdeStruct:
{:?}\n", e);
> +            })
> +            .ok()
So, this returns None if it's a real error. This indicates that the return
type
should just be Result<Option<Self>>.
> +struct FwSecBiosPartial {
Since this structure follows the builder pattern, can we please call it
FwSecBiosBuilder?
> +    base: BiosImageBase,
> +    // FWSEC-specific fields
> +    // These are temporary fields that are used during the construction of
> +    // the FwSecBiosPartial. Once FwSecBiosPartial 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
> +    falcon_ucode_offset: Option<usize>,
> +}
> +
> +struct FwSecBiosImage {
> +    base: BiosImageBase,
> +    // The offset of the Falcon ucode
> +    falcon_ucode_offset: usize,
> +}
> +
> +// Convert from BiosImageBase to BiosImage
> +impl TryFrom<BiosImageBase> for BiosImage {
Why is this a TryFrom impl, instead of a regular constructor, i.e.
BiosImage::new()?

I don't think this is a canonical conversion.
> +    type Error = Error;
> +
> +    fn try_from(base: BiosImageBase) -> Result<Self> {
> +        match base.pcir.code_type {
> +            0x00 => Ok(BiosImage::PciAt(base.try_into()?)),
> +            0x03 => Ok(BiosImage::Efi(EfiBiosImage { base })),
> +            0x70 => Ok(BiosImage::Nbsi(NbsiBiosImage { base })),
> +            0xE0 => Ok(BiosImage::FwSecPartial(FwSecBiosPartial {
> +                base,
> +                falcon_data_offset: None,
> +                pmu_lookup_table: None,
> +                falcon_ucode_offset: None,
> +            })),
> +            _ => Err(EINVAL),
> +        }
> +    }
> +}
<snip>
> +impl TryFrom<BiosImageBase> for PciAtBiosImage {
Same here.
> +    type Error = Error;
> +
> +    fn try_from(base: BiosImageBase) -> Result<Self> {
> +        let data_slice = &base.data;
> +        let (bit_header, bit_offset) =
PciAtBiosImage::find_bit_header(data_slice)?;
> +
> +        Ok(PciAtBiosImage {
> +            base,
> +            bit_header,
> +            bit_offset,
> +        })
> +    }
> +}
<snip>
> +impl FwSecBiosImage {
> +    fn new(pdev: &pci::Device, data: FwSecBiosPartial) ->
Result<Self> {
Please add a method FwSecBiosBuilder::build() that returns an instance of
FwSecBiosImage instead.

On Mon, Jun 02, 2025 at 03:33:56PM +0200, Danilo Krummrich
wrote:
> On Wed, May 21, 2025 at 03:45:11PM +0900, Alexandre Courbot wrote:
> > +impl Vbios {
> 
> <snip>
> 
> > +    pub(crate) fn fwsec_header(&self, pdev: &device::Device)
-> Result<&FalconUCodeDescV3> {
> > +        self.fwsec_image.fwsec_header(pdev)
> > +    }
> > +
> > +    pub(crate) fn fwsec_ucode(&self, pdev: &device::Device)
-> Result<&[u8]> {
> > +        self.fwsec_image.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
> > +    }
> > +
> > +    pub(crate) fn fwsec_sigs(&self, pdev: &device::Device)
-> Result<&[u8]> {
> > +        self.fwsec_image.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
> > +    }
> 
> Can't we just implement Deref here? Why do we need this indirection?
We could, but it seems weird to deref a Vbios struct to an FwsecBiosImage
struct. Conceptually a Vbios is a collection of things and it could have
future extensions to its struct.

The win with using Deref is also not that much, just 2 lines fewer since the
deleted functions are replaced by the the impl Deref block. But I am Ok with
it either way, here is the diff on top of this patch.
Or did I miss something about the suggestion? Will respond to the other
comments, soon, Thanks.
---8<-----------------------
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index 346d48c4820c..ccf83b206758 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -6,6 +6,7 @@
 use crate::firmware::fwsec::Bcrt30Rsa3kSignature;
 use crate::firmware::FalconUCodeDescV3;
 use core::convert::TryFrom;
+use core::ops::Deref;
 use kernel::device;
 use kernel::error::Result;
 use kernel::num::NumExt;
@@ -247,17 +248,13 @@ pub(crate) fn new(pdev: &pci::Device, bar0: &Bar0)
-> Result<Vbios> {
             Err(EINVAL)
         }
     }
+}
 
-    pub(crate) fn fwsec_header(&self, pdev: &device::Device) ->
Result<&FalconUCodeDescV3> {
-        self.fwsec_image.fwsec_header(pdev)
-    }
-
-    pub(crate) fn fwsec_ucode(&self, pdev: &device::Device) ->
Result<&[u8]> {
-        self.fwsec_image.fwsec_ucode(pdev, self.fwsec_header(pdev)?)
-    }
+impl Deref for Vbios {
+    type Target = FwSecBiosImage;
 
-    pub(crate) fn fwsec_sigs(&self, pdev: &device::Device) ->
Result<&[Bcrt30Rsa3kSignature]> {
-        self.fwsec_image.fwsec_sigs(pdev, self.fwsec_header(pdev)?)
+    fn deref(&self) -> &Self::Target {
+        &self.fwsec_image
     }
 }
 
@@ -735,7 +732,7 @@ struct FwSecBiosPartial {
     falcon_ucode_offset: Option<usize>,
 }
 
-struct FwSecBiosImage {
+pub(crate) struct FwSecBiosImage {
     base: BiosImageBase,
     // The offset of the Falcon ucode
     falcon_ucode_offset: usize,
@@ -1091,7 +1088,7 @@ fn new(pdev: &pci::Device, data: FwSecBiosPartial)
-> Result<Self> {
     }
 
     /// Get the FwSec header (FalconUCodeDescV3)
-    fn fwsec_header(&self, dev: &device::Device) ->
Result<&FalconUCodeDescV3> {
+    pub(crate) fn fwsec_header(&self, dev: &device::Device) ->
Result<&FalconUCodeDescV3> {
         // Get the falcon ucode offset that was found in setup_falcon_data
         let falcon_ucode_offset = self.falcon_ucode_offset;
 
@@ -1119,9 +1116,11 @@ fn fwsec_header(&self, dev: &device::Device)
-> Result<&FalconUCodeDescV3> {
             &*(self.base.data.as_ptr().add(falcon_ucode_offset) as *const
FalconUCodeDescV3)
         })
     }
+
     /// Get the ucode data as a byte slice
-    fn fwsec_ucode(&self, dev: &device::Device, desc:
&FalconUCodeDescV3) -> Result<&[u8]> {
+    pub(crate) fn fwsec_ucode(&self, dev: &device::Device) ->
Result<&[u8]> {
         let falcon_ucode_offset = self.falcon_ucode_offset;
+        let desc = self.fwsec_header(dev)?;
 
         // The ucode data follows the descriptor
         let ucode_data_offset = falcon_ucode_offset + desc.size();
@@ -1136,17 +1135,17 @@ fn fwsec_ucode(&self, dev: &device::Device,
desc: &FalconUCodeDescV3) -> Result<
     }
 
     /// Get the FWSEC signatures.
-    fn fwsec_sigs(
+    pub(crate) fn fwsec_sigs(
         &self,
         dev: &device::Device,
-        v3_desc: &FalconUCodeDescV3,
     ) -> Result<&[Bcrt30Rsa3kSignature]> {
         let falcon_ucode_offset = self.falcon_ucode_offset;
+        let desc = self.fwsec_header(dev)?;
 
         // The signatures data follows the descriptor
         let sigs_data_offset = falcon_ucode_offset +
core::mem::size_of::<FalconUCodeDescV3>();
         let sigs_size -            v3_desc.signature_count as usize *
core::mem::size_of::<Bcrt30Rsa3kSignature>();
+            desc.signature_count as usize *
core::mem::size_of::<Bcrt30Rsa3kSignature>();
 
         // Make sure the data is within bounds
         if sigs_data_offset + sigs_size > self.base.data.len() {
@@ -1166,9 +1165,8 @@ fn fwsec_sigs(
                     .as_ptr()
                     .add(sigs_data_offset)
                     .cast::<Bcrt30Rsa3kSignature>(),
-                v3_desc.signature_count as usize,
+                desc.signature_count as usize,
             )
         })
     }
-}
-
+}
\ No newline at end of file

On Tue Jun 3, 2025 at 12:15 AM JST, Joel Fernandes
wrote:
> On Mon, Jun 02, 2025 at 03:33:56PM +0200, Danilo Krummrich wrote:
>> On Wed, May 21, 2025 at 03:45:11PM +0900, Alexandre Courbot wrote:
>> > +impl Vbios {
>> 
>> <snip>
>> 
>> > +    pub(crate) fn fwsec_header(&self, pdev:
&device::Device) -> Result<&FalconUCodeDescV3> {
>> > +        self.fwsec_image.fwsec_header(pdev)
>> > +    }
>> > +
>> > +    pub(crate) fn fwsec_ucode(&self, pdev:
&device::Device) -> Result<&[u8]> {
>> > +        self.fwsec_image.fwsec_ucode(pdev,
self.fwsec_header(pdev)?)
>> > +    }
>> > +
>> > +    pub(crate) fn fwsec_sigs(&self, pdev:
&device::Device) -> Result<&[u8]> {
>> > +        self.fwsec_image.fwsec_sigs(pdev,
self.fwsec_header(pdev)?)
>> > +    }
>> 
>> Can't we just implement Deref here? Why do we need this
indirection?
>
> We could, but it seems weird to deref a Vbios struct to an FwsecBiosImage
> struct. Conceptually a Vbios is a collection of things and it could have
> future extensions to its struct.
Would it then make sense to make `FwSecBiosImage` public, add an `fn
fwsec_image(&self) -> &FwSecBiosImage` method and have the caller
call
its methods directly (maybe renamed to `header`, `ucode` and `sigs`)?

On 6/2/2025 9:33 AM, Danilo Krummrich wrote:
[...]
>> +impl PcirStruct {
>> +    fn new(pdev: &pci::Device, data: &[u8]) ->
Result<Self> {
>> +        if data.len() < core::mem::size_of::<PcirStruct>() {
>> +            dev_err!(pdev.as_ref(), "Not enough data for
PcirStruct\n");
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let mut signature = [0u8; 4];
>> +        signature.copy_from_slice(&data[0..4]);
>> +
>> +        // Signature should be "PCIR" (0x52494350) or
"NPDS" (0x5344504e)
>> +        if &signature != b"PCIR" &&
&signature != b"NPDS" {
>> +            dev_err!(
>> +                pdev.as_ref(),
>> +                "Invalid signature for PcirStruct: {:?}\n",
>> +                signature
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let mut class_code = [0u8; 3];
>> +        class_code.copy_from_slice(&data[13..16]);
>> +
>> +        Ok(PcirStruct {
>> +            signature,
>> +            vendor_id: u16::from_le_bytes([data[4], data[5]]),
>> +            device_id: u16::from_le_bytes([data[6], data[7]]),
>> +            device_list_ptr: u16::from_le_bytes([data[8], data[9]]),
>> +            pci_data_struct_len: u16::from_le_bytes([data[10],
data[11]]),
>> +            pci_data_struct_rev: data[12],
>> +            class_code,
>> +            image_len: u16::from_le_bytes([data[16], data[17]]),
>> +            vendor_rom_rev: u16::from_le_bytes([data[18], data[19]]),
>> +            code_type: data[20],
>> +            last_image: data[21],
>> +            max_runtime_image_len: u16::from_le_bytes([data[22],
data[23]]),
>> +        })
>> +    }
>> +
>> +    /// Check if this is the last image in the ROM
>> +    fn is_last(&self) -> bool {
>> +        self.last_image & LAST_IMAGE_BIT_MASK != 0
>> +    }
>> +
>> +    /// Calculate image size in bytes
>> +    fn image_size_bytes(&self) -> Result<usize> {
>> +        if self.image_len > 0 {
> 
> Please make this check when creating the structure...
> 
>> +            // Image size is in 512-byte blocks
> 
> ...and make this a type invariant.
> 
>> +            Ok(self.image_len as usize * 512)
> 
> It should also be a type invariant that this does not overflow.
> 
> The same applies to NpdeStruct.
> 
Done, that's a lot cleaner, thanks!
> 
>> +    /// Try to find NPDE in the data, the NPDE is right after the
PCIR.
>> +    fn find_in_data(
>> +        pdev: &pci::Device,
>> +        data: &[u8],
>> +        rom_header: &PciRomHeader,
>> +        pcir: &PcirStruct,
>> +    ) -> Option<Self> {
>> +        // Calculate the offset where NPDE might be located
>> +        // NPDE should be right after the PCIR structure, aligned to
16 bytes
>> +        let pcir_offset = rom_header.pci_data_struct_offset as usize;
>> +        let npde_start = (pcir_offset + pcir.pci_data_struct_len as
usize + 0x0F) & !0x0F;
> 
> What's this magic offset and mask?
> 
>> +
>> +        // Check if we have enough data
>> +        if npde_start + 11 > data.len() {
> 
> '+ 11'?
> 
>> +            dev_err!(pdev.as_ref(), "Not enough data for
NPDE\n");
> 
> BiosImageBase declares this as "NVIDIA PCI Data Extension
(optional)". If it's
> really optional, why is this an error?
> 
Good catch, me and Timur were just coincidentally talking about this as well!
Indeed it should not be an error since NPDE on some GPUs doesn't exist.

Will address the other NPDE comments separately since I have to do some research
first. Thanks for double checking.
>> +            return None;
>> +        }
>> +
>> +        // Try to create NPDE from the data
>> +        NpdeStruct::new(pdev, &data[npde_start..])
>> +            .inspect_err(|e| {
>> +                dev_err!(pdev.as_ref(), "Error creating
NpdeStruct: {:?}\n", e);
>> +            })
>> +            .ok()
> 
> So, this returns None if it's a real error. This indicates that the
return type
> should just be Result<Option<Self>>.
> 
>> +struct FwSecBiosPartial {
> 
> Since this structure follows the builder pattern, can we please call it
> FwSecBiosBuilder?
Yes, done.
>> +    base: BiosImageBase,
>> +    // FWSEC-specific fields
>> +    // These are temporary fields that are used during the
construction of
>> +    // the FwSecBiosPartial. Once FwSecBiosPartial 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
>> +    falcon_ucode_offset: Option<usize>,
>> +}
>> +
>> +struct FwSecBiosImage {
>> +    base: BiosImageBase,
>> +    // The offset of the Falcon ucode
>> +    falcon_ucode_offset: usize,
>> +}
>> +
>> +// Convert from BiosImageBase to BiosImage
>> +impl TryFrom<BiosImageBase> for BiosImage {
> 
> Why is this a TryFrom impl, instead of a regular constructor, i.e.
> BiosImage::new()?
> 
> I don't think this is a canonical conversion.
The main advantage is to use .try_into(). It also documents we're
implementing a
conversion from one type to another. I am not sure where the boundary is, but
when you requested me to get rid the other TryFrom(s), I did that but I left
these ones alone because I'd like to use .try_into() for these. I think it
makes
sense to convert a BiosImageBase to BiosImage since they're both essentially
similar. Alex, do you have any thoughts on it as you had suggested this for
other usecases during the initial nova-core stub series as well?

Btw, .try_into() does hurt readability a bit even though its more of a
short-hand, since one has to work harder to know what type converts to what, so
I'm really Ok either way here.
> 
>> +    type Error = Error;
>> +
>> +    fn try_from(base: BiosImageBase) -> Result<Self> {
>> +        match base.pcir.code_type {
>> +            0x00 => Ok(BiosImage::PciAt(base.try_into()?)),
>> +            0x03 => Ok(BiosImage::Efi(EfiBiosImage { base })),
>> +            0x70 => Ok(BiosImage::Nbsi(NbsiBiosImage { base })),
>> +            0xE0 => Ok(BiosImage::FwSecPartial(FwSecBiosPartial {
>> +                base,
>> +                falcon_data_offset: None,
>> +                pmu_lookup_table: None,
>> +                falcon_ucode_offset: None,
>> +            })),
>> +            _ => Err(EINVAL),
>> +        }
>> +    }
>> +}
> 
> <snip>
> 
>> +impl TryFrom<BiosImageBase> for PciAtBiosImage {
> 
> Same here.
Same comment as above.
>> +    type Error = Error;
>> +
>> +    fn try_from(base: BiosImageBase) -> Result<Self> {
>> +        let data_slice = &base.data;
>> +        let (bit_header, bit_offset) =
PciAtBiosImage::find_bit_header(data_slice)?;
>> +
>> +        Ok(PciAtBiosImage {
>> +            base,
>> +            bit_header,
>> +            bit_offset,
>> +        })
>> +    }
>> +}
> 
> <snip>
> 
>> +impl FwSecBiosImage {
>> +    fn new(pdev: &pci::Device, data: FwSecBiosPartial) ->
Result<Self> {
> 
> Please add a method FwSecBiosBuilder::build() that returns an instance of
> FwSecBiosImage instead.
Done, I made this change and it is cleaner. Thanks!

 - Joel

On 6/2/2025 9:33 AM, Danilo Krummrich wrote:
>> +    /// Try to find NPDE in the data, the NPDE is right after the
PCIR.
>> +    fn find_in_data(
>> +        pdev: &pci::Device,
>> +        data: &[u8],
>> +        rom_header: &PciRomHeader,
>> +        pcir: &PcirStruct,
>> +    ) -> Option<Self> {
>> +        // Calculate the offset where NPDE might be located
>> +        // NPDE should be right after the PCIR structure, aligned to
16 bytes
>> +        let pcir_offset = rom_header.pci_data_struct_offset as usize;
>> +        let npde_start = (pcir_offset + pcir.pci_data_struct_len as
usize + 0x0F) & !0x0F;
>
> What's this magic offset and mask?
> 
Oh, hmm. I had a comment on that above though ("NPDE should be right after
the
PCIR structure, aligned to 16 bytes"), does that suffice? I could move the
comment further down.
>> +
>> +        // Check if we have enough data
>> +        if npde_start + 11 > data.len() {
>
> '+ 11'?
Good point, I replaced this and the above with
core::mem::size_of::<Self>().
> 
>> +            dev_err!(pdev.as_ref(), "Not enough data for
NPDE\n");
> BiosImageBase declares this as "NVIDIA PCI Data Extension
(optional)". If it's
> really optional, why is this an error?
> 
>> +            return None;
>> +        }
>> +
>> +        // Try to create NPDE from the data
>> +        NpdeStruct::new(pdev, &data[npde_start..])
>> +            .inspect_err(|e| {
>> +                dev_err!(pdev.as_ref(), "Error creating
NpdeStruct: {:?}\n", e);
>> +            })
>> +            .ok()
>
> So, this returns None if it's a real error. This indicates that the
return type
> should just be Result<Option<Self>>.
I made NpdeStruct::new() return Option only for next revision since NPDE is
optional.

thanks,

 - Joel