Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33484 )
Change subject: mainboard/facebook/fbg1701: Add FMAP file ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2: Code-Review-1
(1 comment)
It's not needed and prevents the use of features that automatically update the generated a fmap.
Should renaming fmap.fmd to e.g. secure.fmd be better solution? (and not adding to Kconfig)
Why secure?
I don't see a reason to add a handcrafted FMD, either. The default `CBFS_SIZE` currently set is just wrong, which likely prevents the default FMAP from working. Please note, that the name `CBFS_SIZE` is historical, today it includes all default coreboot regions, e.g. `FMAP` and `RW_MRC_CACHE`, too.
A handcrafted FMD should only be added if it adds features that the default FMAP can't provide.
Clean. Size should be 0x60000.
IMO there should be fmap.fmd to describe the layout of the SPI including the descriptor and ME location.
Previous comment was that fmap.fmd must not be supplied. My idea was to have a fmd file available, but rename it to some 'none default' name. Since main features this project are verified boot and measured boot my suggestion is using secure.fmp
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... File src/mainboard/facebook/fbg1701/Kconfig:
https://review.coreboot.org/#/c/33484/2/src/mainboard/facebook/fbg1701/Kconf... PS2, Line 57:
This should be the size of the BIOS region at most.
My assumption is tht the CBFS_SIZE is 0x5F700 which is available for coreboot. Should this be the BIOS region (0x600000 in fmap.fmd) for BIOS or size?