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 <[email protected]>
>>>> ---
>>>> 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.
