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.
