Re: [PATCH v5 03/13] gpu: nova-core: gsp: replace BootUnloadGuard with local handlers

["Eliot Courtney" <ecourtney@nvidia.com>]

On Tue Jul 7, 2026 at 9:56 PM JST, Alexandre Courbot wrote:
> On Tue Jul 7, 2026 at 5:04 PM JST, Eliot Courtney wrote:
>> On Tue Jul 7, 2026 at 4:21 PM JST, Alexandre Courbot wrote:
>>> When adding the GSP unload capability, we introduced `BootUnloadGuard`
>>> to automatically call `Gsp::unload` whenever an error occurred during
>>> the boot process, in order to try to reset the GSP to a valid state.
>>>
>>> This approach is not well-suited to the errors that may occur in HALs:
>>> by definition, an error occurring in the HAL means that the GSP is not
>>> booted; yet the first thing that `Gsp::unload` does is queue a shutdown
>>> message to the GSP, which will inevitably result in a timeout when done
>>> from a HAL.
>>>
>>> Furthermore, `BootUnloadGuard` is problematic because it holds
>>> additional references to the boot context, notably the `Falcon`s. These
>>> extra references stand in the way of making some of the `Falcon`'s
>>> methods mutable, since those methods would require exclusive access. As
>>> this behavior is only needed in one place, introducing dedicated types
>>> for it is distracting and unnecessary.
>>>
>>> Thus, remove `BootUnloadGuard` and adopt a two-level error handling
>>> strategy:
>>>
>>> - HALs are free to handle their errors as they see fit (most likely, by
>>>   running their unload bundle if it is ready by the time of the error),
>>> - `Gsp::boot` uses a `ScopeGuard` that runs `Gsp::unload`, since the
>>>   GSP should be up and running by the time `GspHal::boot` has returned.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>>> ---
>>>  drivers/gpu/nova-core/gsp/boot.rs      | 67 +++-------------------------------
>>>  drivers/gpu/nova-core/gsp/hal.rs       | 13 +++----
>>>  drivers/gpu/nova-core/gsp/hal/gh100.rs | 31 ++++++++++------
>>>  drivers/gpu/nova-core/gsp/hal/tu102.rs | 23 +++++++-----
>>>  4 files changed, 44 insertions(+), 90 deletions(-)
>>>
>>> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
>>> index ab0491b57944..536f2e341c01 100644
>>> --- a/drivers/gpu/nova-core/gsp/boot.rs
>>> +++ b/drivers/gpu/nova-core/gsp/boot.rs
>>> @@ -30,66 +30,6 @@
>>>      },
>>>  };
>>>  
>>> -/// Arguments required to call [`Gsp::unload`](super::Gsp::unload).
>>> -///
>>> -/// Stored as their own type to avoid repeating a long and tedious list in [`BootUnloadGuard`].
>>> -pub(super) struct BootUnloadArgs<'a> {
>>> -    gsp: &'a super::Gsp,
>>> -    dev: &'a device::Device<device::Bound>,
>>> -    bar: Bar0<'a>,
>>> -    gsp_falcon: &'a Falcon<'a, Gsp>,
>>> -    sec2_falcon: &'a Falcon<'a, Sec2>,
>>> -    unload_bundle: Option<super::UnloadBundle>,
>>> -}
>>> -
>>> -/// Guard that calls [`Gsp::unload`](super::Gsp::unload) with a
>>> -/// [`UnloadBundle`](super::UnloadBundle) when dropped.
>>> -///
>>> -/// Used to ensure the `UnloadBundle` is run during failure paths.
>>> -pub(super) struct BootUnloadGuard<'a> {
>>> -    guard: ScopeGuard<BootUnloadArgs<'a>, fn(BootUnloadArgs<'a>)>,
>>> -}
>>> -
>>> -impl<'a> BootUnloadGuard<'a> {
>>> -    /// Wraps `unload_bundle` into a guard that executes it when dropped.
>>> -    pub(super) fn new(
>>> -        gsp: &'a super::Gsp,
>>> -        dev: &'a device::Device<device::Bound>,
>>> -        bar: Bar0<'a>,
>>> -        gsp_falcon: &'a Falcon<'a, Gsp>,
>>> -        sec2_falcon: &'a Falcon<'a, Sec2>,
>>> -        unload_bundle: Option<super::UnloadBundle>,
>>> -    ) -> Self {
>>> -        Self {
>>> -            guard: ScopeGuard::new_with_data(
>>> -                BootUnloadArgs {
>>> -                    gsp,
>>> -                    dev,
>>> -                    bar,
>>> -                    gsp_falcon,
>>> -                    sec2_falcon,
>>> -                    unload_bundle,
>>> -                },
>>> -                |args| {
>>> -                    let _ = super::Gsp::unload(
>>> -                        args.gsp,
>>> -                        args.dev,
>>> -                        args.bar,
>>> -                        args.gsp_falcon,
>>> -                        args.sec2_falcon,
>>> -                        args.unload_bundle,
>>> -                    );
>>> -                },
>>> -            ),
>>> -        }
>>> -    }
>>> -
>>> -    /// Disarms the guard and returns the [`UnloadBundle`](super::UnloadBundle) it contains.
>>> -    pub(super) fn dismiss(self) -> Option<super::UnloadBundle> {
>>> -        self.guard.dismiss().unload_bundle
>>> -    }
>>> -}
>>> -
>>>  impl super::Gsp {
>>>      /// Attempt to boot the GSP.
>>>      ///
>>> @@ -107,6 +47,7 @@ pub(crate) fn boot(
>>>          let bar = ctx.bar;
>>>          let chipset = ctx.chipset;
>>>          let gsp_falcon = ctx.gsp_falcon;
>>> +        let sec2_falcon = ctx.sec2_falcon;
>>>          let dev = pdev.as_ref();
>>>          let hal = super::hal::gsp_hal(chipset);
>>>  
>>> @@ -118,7 +59,11 @@ pub(crate) fn boot(
>>>          let wpr_meta = Coherent::init(dev, GFP_KERNEL, GspFwWprMeta::new(&gsp_fw, &fb_layout))?;
>>>  
>>>          // Perform the chipset-specific boot sequence, and retrieve the unload bundle.
>>> -        let unload_guard = hal.boot(&self, &ctx, &fb_layout, &wpr_meta)?;
>>> +        let unload_bundle = hal.boot(&self, &ctx, &fb_layout, &wpr_meta)?;
>>> +
>>> +        let unload_guard = ScopeGuard::new_with_data(unload_bundle, |unload_bundle| {
>>> +            let _ = self.unload(dev, bar, gsp_falcon, sec2_falcon, unload_bundle);
>>> +        });
>>>  
>>>          gsp_falcon.write_os_version(gsp_fw.bootloader.app_version);
>>>  
>>> diff --git a/drivers/gpu/nova-core/gsp/hal.rs b/drivers/gpu/nova-core/gsp/hal.rs
>>> index d3e47ef206de..851d1f24c137 100644
>>> --- a/drivers/gpu/nova-core/gsp/hal.rs
>>> +++ b/drivers/gpu/nova-core/gsp/hal.rs
>>> @@ -24,7 +24,6 @@
>>>          Chipset, //
>>>      },
>>>      gsp::{
>>> -        boot::BootUnloadGuard,
>>>          Gsp,
>>>          GspBootContext,
>>>          GspFwWprMeta, //
>>> @@ -51,15 +50,15 @@ fn run(
>>>  pub(super) trait GspHal: Send {
>>>      /// Performs the GSP boot process, loading and running the required firmwares as needed.
>>>      ///
>>> -    /// Upon success, returns a guard that runs the GSP unload sequence if GSP boot does not
>>> -    /// complete.
>>> -    fn boot<'a>(
>>> +    /// Upon success, returns the [`crate::gsp::UnloadBundle`] to use with [`Gsp::unload`], if one
>>> +    /// could be created.
>>> +    fn boot(
>>>          &self,
>>> -        gsp: &'a Gsp,
>>> -        ctx: &GspBootContext<'a>,
>>> +        gsp: &Gsp,
>>> +        ctx: &GspBootContext<'_>,
>>>          fb_layout: &FbLayout,
>>>          wpr_meta: &Coherent<GspFwWprMeta>,
>>> -    ) -> Result<BootUnloadGuard<'a>>;
>>> +    ) -> Result<Option<crate::gsp::UnloadBundle>>;
>>>  
>>>      /// Performs HAL-specific post-GSP boot tasks.
>>>      ///
>>> diff --git a/drivers/gpu/nova-core/gsp/hal/gh100.rs b/drivers/gpu/nova-core/gsp/hal/gh100.rs
>>> index 1d06405a32f6..18c889f9f413 100644
>>> --- a/drivers/gpu/nova-core/gsp/hal/gh100.rs
>>> +++ b/drivers/gpu/nova-core/gsp/hal/gh100.rs
>>> @@ -23,7 +23,6 @@
>>>          Fsp, //
>>>      },
>>>      gsp::{
>>> -        boot::BootUnloadGuard,
>>>          hal::{
>>>              GspHal,
>>>              UnloadBundle, //
>>> @@ -143,13 +142,13 @@ impl GspHal for Gh100 {
>>>      ///
>>>      /// This path uses FSP to establish a chain of trust and boot GSP-FMC. FSP handles
>>>      /// the GSP boot internally - no manual GSP reset/boot is needed.
>>> -    fn boot<'a>(
>>> +    fn boot(
>>>          &self,
>>> -        gsp: &'a Gsp,
>>> -        ctx: &GspBootContext<'a>,
>>> +        gsp: &Gsp,
>>> +        ctx: &GspBootContext<'_>,
>>>          fb_layout: &FbLayout,
>>>          wpr_meta: &Coherent<GspFwWprMeta>,
>>> -    ) -> Result<BootUnloadGuard<'a>> {
>>> +    ) -> Result<Option<crate::gsp::UnloadBundle>> {
>>>          let dev = ctx.dev();
>>>          let bar = ctx.bar;
>>>          let chipset = ctx.chipset;
>>> @@ -160,10 +159,6 @@ fn boot<'a>(
>>>              KBox::new(FspUnloadBundle, GFP_KERNEL)? as KBox<dyn UnloadBundle>
>>>          );
>>>  
>>> -        // Wrap the unload bundle into a drop guard so it is automatically run upon failure.
>>> -        let unload_guard =
>>> -            BootUnloadGuard::new(gsp, dev, bar, gsp_falcon, sec2_falcon, Some(unload_bundle));
>>> -
>>>          let mut fsp = Fsp::wait_secure_boot(dev, bar, chipset)?;
>>>  
>>>          let args = FmcBootArgs::new(
>>> @@ -174,11 +169,23 @@ fn boot<'a>(
>>>              false,
>>>          )?;
>>>  
>>> -        fsp.boot_fmc(dev, fb_layout, &args)?;
>>> +        // Keep the result as we want to wait for lockdown release even in case of error, to make
>>> +        // sure `args` is not accessed by the GSP anymore.
>>> +        let fsp_res = fsp.boot_fmc(dev, fb_layout, &args);
>>>  
>>> -        wait_for_gsp_lockdown_release(dev, gsp_falcon, args.boot_params_dma_handle())?;
>>> +        // Wait for GSP-FMC to release the GSP lockdown, indicating that `args` is not accessed
>>> +        // anymore.
>>> +        let lockdown_res =
>>> +            wait_for_gsp_lockdown_release(dev, gsp_falcon, args.boot_params_dma_handle());
>>>  
>>> -        Ok(unload_guard)
>>> +        match fsp_res.and(lockdown_res) {
>>> +            Ok(()) => Ok(Some(unload_bundle)),
>>> +            Err(e) => {
>>> +                // Wait for the GSP RISC-V core to halt in case of error.
>>> +                let _ = unload_bundle.0.run(dev, bar, gsp_falcon, sec2_falcon);
>>> +                Err(e)
>>> +            }
>>> +        }
>>
>> IMO we should just ScopeGuard here too (after args creation) to run the
>> unload bundle. It will avoid manual manipulation of results which is
>> error prone (since a ? added later will break things) and also halt
>> happens-after gsp lockdown, so it's a natural sequence point. Also I am
>
> ScopedGuard here is reasonable - do you mean only for running the unload
> bundle, or would you also duplicate the lockdown release there so we can
> avoid using `and`?

I would skip the lockdown release path on the error path since AFAICT in
every case halt happens, we would have also gotten
`wait_for_gsp_lockdown_release` to return if we had executed it. So it
doesn't buy us any additional waiting/sequencing, and we still need to
wait for halt anyway. So concretely I think something like this is
simpler and as robust (conceptually to me it also seems odd to wait for
"gsp lockdown release" when boot has failed):

```
diff --git a/drivers/gpu/nova-core/gsp/hal/gh100.rs b/drivers/gpu/nova-core/gsp/hal/gh100.rs
--- a/drivers/gpu/nova-core/gsp/hal/gh100.rs
+++ b/drivers/gpu/nova-core/gsp/hal/gh100.rs
@@
 use kernel::{
     device,
     dma::Coherent,
     io::poll::read_poll_timeout,
-    time::Delta, //
+    time::Delta,
+    types::ScopeGuard, //
 };
@@
         let args = FmcBootArgs::new(
             dev,
             chipset,
             wpr_meta.dma_handle(),
             gsp.libos.dma_handle(),
             false,
         )?;

-        // Keep the result as we want to wait for lockdown release even in case of error, to make
-        // sure `args` is not accessed by the GSP anymore.
-        let fsp_res = fsp.boot_fmc(dev, fb_layout, &args);
+        // Wait for the GSP RISC-V core to halt in case of error. We create this guard after `args`
+        // to make sure that boot args are kept alive until halt, in case they are still being
+        // accessed.
+        let unload_guard = ScopeGuard::new_with_data(unload_bundle, |unload_bundle| {
+            let _ = unload_bundle.0.run(dev, bar, gsp_falcon, sec2_falcon);
+        });
+
+        fsp.boot_fmc(dev, fb_layout, &args)?;

         // Wait for GSP-FMC to release the GSP lockdown, indicating that `args` is not accessed
         // anymore.
-        let lockdown_res =
-            wait_for_gsp_lockdown_release(dev, gsp_falcon, args.boot_params_dma_handle());
+        wait_for_gsp_lockdown_release(dev, gsp_falcon, args.boot_params_dma_handle())?;

-        match fsp_res.and(lockdown_res) {
-            Ok(()) => Ok(Some(unload_bundle)),
-            Err(e) => {
-                // Wait for the GSP RISC-V core to halt in case of error.
-                let _ = unload_bundle.0.run(dev, bar, gsp_falcon, sec2_falcon);
-                Err(e)
-            }
-        }
+        Ok(Some(unload_guard.dismiss()))
     }
 }
```

WDYT?

>
>> not 100% sure but I am not fully convinced that
>> wait_for_gsp_lockdown_release will correctly detect all errors if they
>> occur.
>
> The failure path is a best effort. I don't think we can guarantee
> anything as it doesn't appear to have been designed with recovery in mind.

Yeah I agree, what I meant is that I think that I think that the wait
until halt is a better signal that we are definitely done than the
lockdown release one, and it's easier to just only wait for halt in the
error case than to also manipulate results to wait for the lockdown
release path too.

>
>>
>> With ^ change,
>> Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>
>
> Thanks!