Re: [PATCH 13/16] gpu: nova-core: Add support for VBIOS ucode extraction for boot

Joel Fernandes
2025-04-24T20:22:50Z

On 4/23/2025 10:06 AM, Danilo Krummrich wrote:
> On Sun, Apr 20, 2025 at 09:19:45PM +0900, Alexandre Courbot wrote:
>> From: Joel Fernandes <joelagnelf@nvidia.com>
>>
>> Add support for navigating and setting up vBIOS ucode data required for
>> GSP to boot. The main data extracted from the vBIOS is the FWSEC-FRTS
>> firmware which runs on the GSP processor. This firmware runs in high
>> secure mode, and sets up the WPR2 (Write protected region) before the
>> Booter runs on the SEC2 processor.
>>
>> Also add log messages to show the BIOS images.
>>
>> [102141.013287] NovaCore: Found BIOS image at offset 0x0, size: 0xfe00, type: PciAt
>> [102141.080692] NovaCore: Found BIOS image at offset 0xfe00, size: 0x14800, type: Efi
>> [102141.098443] NovaCore: Found BIOS image at offset 0x24600, size: 0x5600, type: FwSec
>> [102141.415095] NovaCore: Found BIOS image at offset 0x29c00, size: 0x60800, type: FwSec
>>
>> Tested on my Ampere GA102 and boot is successful.
>>
>> [applied changes by Alex Courbot for fwsec signatures]
>> [applied feedback from Alex Courbot and Timur Tabi]
>>
>> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> drivers/gpu/nova-core/firmware.rs | 2 -
>> drivers/gpu/nova-core/gpu.rs | 5 +
>> drivers/gpu/nova-core/nova_core.rs | 1 +
>> drivers/gpu/nova-core/vbios.rs | 1103 ++++++++++++++++++++++++++++++++++++
>> 4 files changed, 1109 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
>> index 4ef5ba934b9d255635aa9a902e1d3a732d6e5568..58c0513d49e9a0cef36917c8e2b25c414f6fc596 100644
>> --- a/drivers/gpu/nova-core/firmware.rs
>> +++ b/drivers/gpu/nova-core/firmware.rs
>> @@ -44,7 +44,6 @@ pub(crate) fn new(
>> }
>>
>> /// Structure used to describe some firmwares, notable fwsec-frts.
>> -#[allow(dead_code)]
>> #[repr(C)]
>> #[derive(Debug, Clone)]
>> pub(crate) struct FalconUCodeDescV3 {
>> @@ -64,7 +63,6 @@ pub(crate) struct FalconUCodeDescV3 {
>> _reserved: u16,
>> }
>>
>> -#[allow(dead_code)]
>> impl FalconUCodeDescV3 {
>> pub(crate) fn size(&self) -> usize {
>> ((self.hdr & 0xffff0000) >> 16) as usize
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index ec4c648c6e8b4aa7d06c627ed59c0e66a08c679e..2344dfc69fe4246644437d70572680a4450b5bd7 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -11,6 +11,7 @@
>> use crate::regs;
>> use crate::timer::Timer;
>> use crate::util;
>> +use crate::vbios::Vbios;
>> use core::fmt;
>>
>> macro_rules! define_chipset {
>> @@ -157,6 +158,7 @@ pub(crate) struct Gpu {
>> fw: Firmware,
>> sysmem_flush: DmaObject,
>> timer: Timer,
>> + bios: Vbios,
>> }
>>
>> #[pinned_drop]
>> @@ -237,12 +239,15 @@ pub(crate) fn new(
>>
>> let _sec2_falcon = Sec2Falcon::new(pdev, spec.chipset, &bar, true)?;
>>
>> + let bios = Vbios::probe(&bar)?;
>> +
>> Ok(pin_init!(Self {
>> spec,
>> bar,
>> fw,
>> sysmem_flush,
>> timer,
>> + bios,
>> }))
>> }
>> }
>> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
>> index 4dde8004d24882c60669b5acd6af9d6988c66a9c..2858f4a0dc35eb9d6547d5cbd81de44c8fc47bae 100644
>> --- a/drivers/gpu/nova-core/nova_core.rs
>> +++ b/drivers/gpu/nova-core/nova_core.rs
>> @@ -29,6 +29,7 @@ macro_rules! with_bar {
>> mod regs;
>> mod timer;
>> mod util;
>> +mod vbios;
>>
>> kernel::module_pci_driver! {
>> type: driver::NovaCore,
>> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..534107b708cab0eb8d0accf7daa5718edf030358
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/vbios.rs
>> @@ -0,0 +1,1103 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +// To be removed when all code is used.
>> +#![allow(dead_code)]
>
> Please not, use 'expect' and and only where needed. If it would be too much,
> it's probably a good indicator that we want to reduce the size of the patch for
> now.

Done.

[..]

>> +
>> + // loop till break
>
> This comment seems unnecessary, better explain what we loop over and why.

Done.

>> + loop {
>> + // Try to parse a BIOS image at the current offset
>> + // This will now check for all valid ROM signatures (0xAA55, 0xBB77, 0x4E56)
>> + let image_size =
>> + Self::read_bios_image_at_offset(bar0, &mut data, cur_offset, BIOS_READ_AHEAD_SIZE)
>> + .and_then(|image| image.image_size_bytes())
>> + .inspect_err(|e| {
>> + pr_err!(
>> + "Failed to parse initial BIOS image headers at offset {:#x}: {:?}\n",
>> + cur_offset,
>> + e
>> + );
>> + })?;
>> +
>> + // Create a new BiosImage with the full image data
>> + let full_image =
>> + Self::read_bios_image_at_offset(bar0, &mut data, cur_offset, image_size)
>> + .inspect_err(|e| {
>> + pr_err!(
>> + "Failed to parse full BIOS image at offset {:#x}: {:?}\n",
>> + cur_offset,
>> + e
>> + );
>> + })?;
>> +
>> + // Determine the image type
>> + let image_type = full_image.image_type_str();
>> +
>> + pr_info!(
>
> I think this should be a debug print.

Done.

Will continue looking into the feedback on the rest of the items and reply. Thanks!

- Joel