On Wed Jul 1, 2026 at 11:54 AM JST, Eliot Courtney wrote:
> On Mon Jun 29, 2026 at 11:09 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 | 20 ++++------
>> drivers/gpu/nova-core/gsp/hal/tu102.rs | 23 +++++++-----
>> 4 files changed, 33 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..5fe445d73599 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,27 +142,22 @@ 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;
>> let gsp_falcon = ctx.gsp_falcon;
>> - let sec2_falcon = ctx.sec2_falcon;
>>
>> let unload_bundle = crate::gsp::UnloadBundle(
>> 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 +168,13 @@ 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 res = fsp.boot_fmc(dev, fb_layout, &args);
>
> In the error case this no longer runs the unload bundle (which waits for
> GSP halt). But, we need to wait for GSP halt to ensure stuff is properly
> torn down (e.g. in [1], not needing to wait on FSP queue is predicated
> on GSP riscv actually halting, to avoid an issue on reprobe, IIUC).
>
> Separately, are you sure that GSP lockdown will release in an error
> case? I'm worried that we'll just timeout here, which while it probably
> works in that it waits long enough, is semantically a bit weird.
>
> What about using a ScopeGuard that runs the unload_bundle? That way we
> will have waited for GSP halt. Later, we could add additional HAL
> specific common code for trying to reset GSP in case it doesn't halt.
> WDYT?
>
> [1]: https://lore.kernel.org/all/[email protected]/
The problem is that we don't know that either procedure (wait for GSP
halt vs. wait for GSP lockdown release) are correct in the error case. I
settled for GSP lockdown because GSP halt is typically triggered by the
shutdown message being received and acknowledged - but if we fail here,
the GSP is by definition unable to process that message.
I've noticed only now that `wait_for_gsp_lockdown_release` actually
returns when either the lockdown is released, OR an error has been
signaled by GSP-FMC (see `lockdown_released_or_error`). So, we can be
confident that unless something has crashed catastrophically this
should not timeout, and also give us the bound after which the
`FmcBootArgs` is not accessed anymore.
By then, we can also probably expect the GSP to halt cleanly; so we
might indeed wait for the RISC-V core to halt - not to avoid a
use-after-free of `FmcBootArgs` (it's clearly unused by then), but as a
general indicator that the GPU can be reprobed.
So I guess the answer is, we should try and do both? Something like this maybe:
let fsp_res = 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());
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(ctx);
Err(e)
}
}
