Julien Viard de Galbert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25437 )
Change subject: FSP 2.0: Add fsp_relax_security ......................................................................
Patch Set 21:
(5 comments)
I can understand that this change is very specific to a use case (BMC able to enable/disable the security flag) and a platform (denverton FSP does not provide and UPD to handle that).
So maybe that change should just not go in and I should rewrite my other patches to not depend on it. What do you think ?
https://review.coreboot.org/c/coreboot/+/25437/6/src/drivers/intel/fsp2_0/no... File src/drivers/intel/fsp2_0/notify.c:
https://review.coreboot.org/c/coreboot/+/25437/6/src/drivers/intel/fsp2_0/no... PS6, Line 79: if ((phase == READY_TO_BOOT) && fsp_relax_security()) {
Is it possible to move this after soc_display_mtrrs()?
yes
https://review.coreboot.org/c/coreboot/+/25437/9/src/drivers/intel/fsp2_0/no... File src/drivers/intel/fsp2_0/notify.c:
https://review.coreboot.org/c/coreboot/+/25437/9/src/drivers/intel/fsp2_0/no... PS9, Line 69: __attribute__((weak))
There is __weak macro to use.
Done
https://review.coreboot.org/c/coreboot/+/25437/9/src/drivers/intel/fsp2_0/no... PS9, Line 78: fsp_relax_security()
How were you envisioning this working? Recompile with a stronger symbol that returns true? At that p […]
The stronger symbol will only return true if the BMC told it to do so... without reflashing, while a Kconfig require rebuild and reflash...
https://review.coreboot.org/c/coreboot/+/25437/18/src/drivers/intel/fsp2_0/n... File src/drivers/intel/fsp2_0/notify.c:
https://review.coreboot.org/c/coreboot/+/25437/18/src/drivers/intel/fsp2_0/n... PS18, Line 70: fsp_relax_security
Right I missed the introduction of CHIPSET_LOCKDOWN_COREBOOT, I can certainly rewrite it using that. […]
Except I can't... on other platform CHIPSET_LOCKDOWN_COREBOOT/FSP is used to fill the parameters to FSP but on denverton we don't have any parameters to configure lockdown. Reusing it here would break other platforms.
So next option rename it.
https://review.coreboot.org/c/coreboot/+/25437/18/src/drivers/intel/fsp2_0/n... PS18, Line 70: __attribute__((weak)) bool fsp_relax_security(void)
Ah, I missed the bmcInfo patch and I don't know bmcInfo, yet \o/ Soo... just that I get that right.. […]
Exaclty, the 'tagada' platform is using denverton microservers as 'blade' but without hotplug so when one has issue we prefer to be able to diagnose it remotely. Our BMC is not very powerful but can still set several settings in flash in the bmcInfo struct that coreboot will then pick.
-
Julien Viard de Galbert (Code Review)