Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33107 )
Change subject: [RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2 ......................................................................
Patch Set 1:
(5 comments)
I agree with Patrick, just extend (or even rewrite) the existing structure. Payloads are supposed to parse it via libpayload anyway so they can pull in the updated definition from there.
https://review.coreboot.org/#/c/33107/1/src/commonlib/include/commonlib/core... File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/#/c/33107/1/src/commonlib/include/commonlib/core... PS1, Line 458: uint64_t cbfs_legacy_offset; Where does the name "legacy" come from? I think ro_cbfs_offset or boot_cbfs_offset might be more fitting? (Also, if you're thinking about the "RW_LEGACY" CBFS in Chromebooks, that's a different section, not this one.)
https://review.coreboot.org/#/c/33107/1/src/commonlib/include/commonlib/core... PS1, Line 462: uint64_t fmap_size; Can be inferred from reading the header so not sure if this is necessary here.
https://review.coreboot.org/#/c/33107/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/33107/1/src/lib/coreboot_table.c@288 PS1, Line 288: if (find_fmap_directory(&fmrd) == 0) While we're at it we should just use the hardcoded FMAP_OFFSET here (see below), no need to parse the whole thing again.
https://review.coreboot.org/#/c/33107/1/src/lib/coreboot_table.c@307 PS1, Line 307: if (fmap_locate_area("COREBOOT", &ar)) { These are hardcoded as __FMAP__COREBOOT_BASE and __FMAP__COREBOOT_SIZE, you don't need to look them up. (They're from the autogenerated "fmap_config.h" header that needs an extra Makefile rule to be added as a prerequisite for a file, see src/lib/Makefile.inc.)
https://review.coreboot.org/#/c/33107/1/src/lib/coreboot_table.c@316 PS1, Line 316: if (CONFIG(COMMON_CBFS_SPI_WRAPPER)) { Shouldn't this check for BOOT_DEVICE_MEMORY_MAPPED instead?