Re: [PATCH v4 09/13] gpu: nova-core: introduce GspBootMethod

On Thu Jul 2, 2026 at 11:28 AM JST, Eliot Courtney wrote:
> On Mon Jun 29, 2026 at 11:09 PM JST, Alexandre Courbot wrote:
>> The GSP boot method is currently determined by two ad-hoc methods of
>> `Chipset`: `uses_fsp` (a boolean telling whether to use the FSP boot
>> path or the Sec2 Booter one) and `needs_fwsec_bootloader` (another
>> boolean valid only for the Sec2 Booter method that tells whether the
>> FWSEC bootloader must be used).
>>
>> This is neither extensible nor sound: the combination `uses_fsp &&
>> needs_fwsec_bootloader` is invalid, but can still be expressed.
>>
>> Thus, unify these two predicates into a single `gsp_boot_method` method
>> that returns an enum type unambiguously describing the boot method to
>> use. This ensures that no invalid combination can be expressed, which
>> makes matching sounder.
>>
>> Signed-off-by: Alexandre Courbot <[email protected]>
>> ---
>
> Ideally we would not need to expose bootloader details (needs fwsec
> bootloader, mapping of architecture to GspBootMethod) outside of the
> HAL, and also not have to duplicate the logic for determining
> `needs_fwsec_bootloader` in both the HAL and Chipset. One thing
> preventing keeping all this in the HALs is that we need to know the set
> of firmware files in a const context, which can't go through the &dyn
> for HALs. I also find it a bit odd to go Chipset->GspBootMethod->HAL
> which relies on the GSP HAL being entirely determined by the boot method
> which doesn't feel generally correct to me.

So far it is a good mapping though - the only thing abstracted by the
GSP HAL is how we boot and stop the GSP, which is entirely determined by
the boot method used.

While I agree that not exposing these details outside of the HAL would
be nice, at least for the time being we need to - the creation of the
`Fsp` happens at the `Gpu` level, and is now conditional on the boot
method. Thus the introduction of `GspBootMethod`. I would be happy to
remove this in the future if we can, but that doesn't look possible
without more refactoring, and this series is already doing quite a bit
of it just to move the `Fsp` instance up. I think going further should
be a separate effort.

>
> What if we added a const fn n gsp/hal.rs called like boot_firmware_files
> ( or whatever) which returns the extra firmware files needed to boot the
> GSP based on the architecture. Then we don't need to expose details like
> `needs_fwsec_bootloader` or `GspBootMethod` outside of the HALs. We
> still need another set of matches but it's unavoidable AFAICT without
> being able to know the HAL type in a const context, at least it's
> consolidated and in the HAL this way. In the future we might consider
> type parameterising `Gpu` with the chipset and types that it uses with
> the minimal set of strategies that it needs (e.g. sec2 vs fsp boot) then
> we could even make the HALs not use dyn and push a bunch of stuff into
> associated types.

I like this idea of a function that provides the required firmware files,
I imagine we could have the relevant modules provide the files they need
in a const function and the `ModInfoBuilder` going through that.

For the moment though, as shown in the patch you provided this requires
another match on `Architecture`, with consideration for whether we are
using the FWSEC bootloader or not, and handling the special case for
GA100 - so it's not really abstacting more things away.

Conversely, the boot method is again a perfect 1:1 mapping for the files
we need, and removes the need to special-case GA100, so I think it is a
better fit for the current state of the code.