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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Tue Jul 7, 2026 at 10:34 PM JST, Eliot Courtney wrote:
> 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)?;

After patch 13, `fsp` is borrows mutably from the boot context (which is
also given to the unload bundle), so we end up with a double-borrow
issue. I could solve this by passing a tuple to the ScopeGuard, and
borrowing `fsp` from it:

    let mut unload_guard =
        ScopeGuard::new_with_data((unload_bundle, ctx), |(unload_bundle, ctx)| {
            let _ = unload_bundle.0.run(ctx);
        });

    let fsp = unload_guard.1.fsp.as_mut().ok_or(ENODEV)?;

    fsp.boot_fmc(dev, fb_layout, &args)?;

That's something we will need to do once the falcons become mutable
anyway, so I'll use that approach for v6.