linux-kernel - Re: [PATCH v4 20/20] gpu: nova-core: load and run FWSEC-FRTS

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <DADD57F0Z5IX.2EQ7AIDLERVCO@nvidia.com>
Date: Wed, 04 Jun 2025 10:38:16 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Lyude Paul" <lyude@...hat.com>, "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>, "Danilo
 Krummrich" <dakr@...nel.org>, "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>
Cc: "John Hubbard" <jhubbard@...dia.com>, "Ben Skeggs" <bskeggs@...dia.com>,
 "Joel Fernandes" <joelagnelf@...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 v4 20/20] gpu: nova-core: load and run FWSEC-FRTS

On Wed Jun 4, 2025 at 6:45 AM JST, Lyude Paul wrote:
> On Wed, 2025-05-21 at 15:45 +0900, Alexandre Courbot wrote:
>> With all the required pieces in place, load FWSEC-FRTS onto the GSP
>> falcon, run it, and check that it successfully carved out the WPR2
>> region out of framebuffer memory.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
>> ---
>>  drivers/gpu/nova-core/falcon.rs |  3 ---
>>  drivers/gpu/nova-core/gpu.rs    | 57 ++++++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/nova-core/regs.rs   | 15 +++++++++++
>>  3 files changed, 71 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>> index f224ca881b72954d17fee87278ecc7a0ffac5322..91f0451a04e7b4d0631fbcf9b1e76e59d5dfb7e8 100644
>> --- a/drivers/gpu/nova-core/falcon.rs
>> +++ b/drivers/gpu/nova-core/falcon.rs
>> @@ -2,9 +2,6 @@
>>  
>>  //! Falcon microprocessor base support
>>  
>> -// To be removed when all code is used.
>> -#![expect(dead_code)]
>> -
>>  use core::ops::Deref;
>>  use core::time::Duration;
>>  use hal::FalconHal;
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 5a4c23a7a6c22abc1f6e72a307fa3336d731a396..280929203189fba6ad8e37709927597bb9c7d545 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -246,7 +246,7 @@ pub(crate) fn new(
>>  
>>          let bios = Vbios::new(pdev, bar)?;
>>  
>> -        let _fwsec_frts = FwsecFirmware::new(
>> +        let fwsec_frts = FwsecFirmware::new(
>>              &gsp_falcon,
>>              pdev.as_ref(),
>>              bar,
>> @@ -257,6 +257,61 @@ pub(crate) fn new(
>>              },
>>          )?;
>>  
>> +        // Check that the WPR2 region does not already exists - if it does, the GPU needs to be
>> +        // reset.
>> +        if regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI::read(bar).hi_val() != 0 {
>> +            dev_err!(
>> +                pdev.as_ref(),
>> +                "WPR2 region already exists - GPU needs to be reset to proceed\n"
>> +            );
>> +            return Err(EBUSY);
>> +        }
>> +
>> +        // Reset falcon, load FWSEC-FRTS, and run it.
>> +        gsp_falcon.reset(bar)?;
>> +        gsp_falcon.dma_load(bar, &fwsec_frts)?;
>> +        let (mbox0, _) = gsp_falcon.boot(bar, Some(0), None)?;
>> +        if mbox0 != 0 {
>> +            dev_err!(pdev.as_ref(), "FWSEC firmware returned error {}\n", mbox0);
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // SCRATCH_E contains FWSEC-FRTS' error code, if any.
>> +        let frts_status = regs::NV_PBUS_SW_SCRATCH_0E::read(bar).frts_err_code();
>> +        if frts_status != 0 {
>> +            dev_err!(
>> +                pdev.as_ref(),
>> +                "FWSEC-FRTS returned with error code {:#x}",
>> +                frts_status
>> +            );
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // Check the WPR2 has been created as we requested.
>> +        let (wpr2_lo, wpr2_hi) = (
>> +            (regs::NV_PFB_PRI_MMU_WPR2_ADDR_LO::read(bar).lo_val() as u64) << 12,
>> +            (regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI::read(bar).hi_val() as u64) << 12,
>> +        );
>> +        if wpr2_hi == 0 {
>> +            dev_err!(
>> +                pdev.as_ref(),
>> +                "WPR2 region not created after running FWSEC-FRTS\n"
>> +            );
>> +
>> +            return Err(ENOTTY);
>
> ENOTTY? Is this correct?

Probably not - I guess `EIO` would be better to express a firmware
failure? (and for the errors around this one as well).

>
>> +        } else if wpr2_lo != fb_layout.frts.start {
>> +            dev_err!(
>> +                pdev.as_ref(),
>> +                "WPR2 region created at unexpected address {:#x} ; expected {:#x}\n",
>
> Extra space (but if that's intentional, feel free to leave it)

Oops, French typography habits. ;)

>
> Besides those two nits: Reviewed-by: Lyude Paul <lyude@...hat.com>

Thanks!