| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aa540122-6a6a-4fd0-9005-5a4061f8eb6f@nvidia.com>
Date: Thu, 24 Apr 2025 16:22:44 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>,
Alexandre Courbot <acourbot@...dia.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
Trevor Gross <tmgross@...ch.edu>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
Jonathan Corbet <corbet@....net>, John Hubbard <jhubbard@...dia.com>,
Ben Skeggs <bskeggs@...dia.com>, Timur Tabi <ttabi@...dia.com>,
Alistair Popple <apopple@...dia.com>, linux-kernel@...r.kernel.org,
rust-for-linux@...r.kernel.org, nouveau@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 13/16] gpu: nova-core: Add support for VBIOS ucode
extraction for boot
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@...dia.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@...dia.com>
>> Signed-off-by: Alexandre Courbot <acourbot@...dia.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
Powered by blists - more mailing lists