Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33107
Change subject: [RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2 ......................................................................
[RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2
BOOT_MEDIA_PARAMS2 exposes the boot media MMIO address if it's memory mapped in addition to various regions inside the bootmedia.
That information can be used by payloads to: * Support VBOOT on SeaBIOS, as it otherwise uses the RO CBFS only * Support Intel Apollolake and platforms that don't map the end of the BIOS region to the end of the address space * Support fwupd and flashrom finding the FMAP in memory
Change-Id: Ia0b1ac927b8782cc99cd7f34d8bf5c4ef60b5570 Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c 2 files changed, 118 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/33107/1
diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h index 198ad27..a854e44 100644 --- a/src/commonlib/include/commonlib/coreboot_tables.h +++ b/src/commonlib/include/commonlib/coreboot_tables.h @@ -392,6 +392,81 @@ struct mac_address mac_addrs[0]; };
+/** + * coreboot boot media params2 + * + * The coreboot 'boot media params2' contain information about the + * bootmedia layout, allowing a payload to read the boot media + * without the need to parse (platform specific) layout files. + * + * It extends 'boot media params' by MMIO addresses and a second CBFS + * pointer. + * + * If the boot media is memory mapped, as it's usually done on x86 platforms, + * the FMAP and CBFS can be easily accessed by any software without the need + * for platform specific drivers. + * + * If the boot media is not memory mapped, the `mmap_mmio_address` is set + * to ~0ULL. In that case the software must use platform specific drivers + * to access the boot media (like flashrom or Linux's MTD). + * + * The memory mapped area is `mmap_size` bytes in size, starting `mmap_offset` + * bytes from `mmap_mmio_address`. + * The memory mapped area might be smaller than `boot_media_size` bytes, + * which gives the total size in bytes as seen by an external programmer. + * + * Software utilizing the coreboot boot media params2 shall check if the + * region to be access falls completely within the memory mapped region, + * before trying to access them. + * In addition it should not assume that the whole boot media is memory mapped. + * + * Example on Intel Apollolake: + * + * physical memory boot media + * +-----------+ + * | | + * | | + * +-----------+ MMAP MMIO address +-----------------+ -------- + * | ~UNAVAIL~ | | | IFD | | | | | + * | | MMAP offset | | | | | | + * | | | | | | | | | + * +-----------+ --- <<< +-----------------+ | | | | + * | BIOS REG | | | BIOS | | | | | + * | | | | | | | | | + * | | | |+---------------+| --+-+-+-+CBFS offset + * | | | || CBFS Active || | | | + * | | MMAP size |+---------------+| --+-+-+FMAP offset + * | | | || FMAP || | | + * | | | |+---------------+| --+-+CBFS legacy offset + * | | | || COREBOOT(CBFS)|| | + * +-----------+ --- <<< |+---------------+| | + * | TXE SRAM | || BIOS UNUSABLE || | + * | | |+---------------+| | + * +-----------+ +-----------------+ | + * |DEVICE EXTENSION | | + * | | | + * +-----------------+ -+Boot media size + */ + +#define LB_TAG_BOOT_MEDIA_PARAMS2 0x0036 +struct lb_boot_media_params2 { + uint32_t tag; + uint32_t size; + /* offsets are relative to start of boot media */ + uint64_t fmap_offset; + uint64_t cbfs_offset; + uint64_t cbfs_legacy_offset; + uint64_t mmap_offset; + /* Size is in bytes */ + uint64_t cbfs_size; + uint64_t fmap_size; + uint64_t boot_media_size; + uint64_t cbfs_legacy_size; + uint64_t mmap_size; + /* MMIO address of MMAPed boot media, ~0ULL if not MMAPed.*/ + uint64_t mmap_mmio_address; +}; + #define LB_TAG_SERIALNO 0x002a #define MAX_SERIALNO_LENGTH 32
diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 6e44f5d..c5807fd 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -261,8 +261,10 @@ static void lb_boot_media_params(struct lb_header *header) { struct lb_boot_media_params *bmp; + struct lb_boot_media_params2 *bmp2; struct cbfs_props props; const struct region_device *boot_dev; + struct region ar; struct region_device fmrd;
boot_device_init(); @@ -285,6 +287,47 @@ bmp->fmap_offset = ~(uint64_t)0; if (find_fmap_directory(&fmrd) == 0) bmp->fmap_offset = region_device_offset(&fmrd); + + /* LB_TAG_BOOT_MEDIA_PARAMS2 exposes additional parameters: + * - MMAPed boot media address + * - legacy CBFS position (RO partition in case of VBOOT) + */ + bmp2 = (struct lb_boot_media_params2 *)lb_new_record(header); + bmp2->tag = LB_TAG_BOOT_MEDIA_PARAMS2; + bmp2->size = sizeof(*bmp2); + + bmp2->cbfs_offset = bmp->cbfs_offset; + bmp2->cbfs_size = bmp->cbfs_size; + bmp2->boot_media_size = bmp->boot_media_size; + bmp2->fmap_offset = bmp->fmap_offset; + + if (find_fmap_directory(&fmrd) == 0) + bmp2->fmap_size = region_device_size(&fmrd); + + if (fmap_locate_area("COREBOOT", &ar)) { + printk(BIOS_INFO, "Can't find 'COREBOOT' area in FMAP\n"); + bmp2->cbfs_legacy_offset = ~(uint64_t)0; + bmp2->cbfs_legacy_size = 0; + } else { + bmp2->cbfs_legacy_offset = ar.offset; + bmp2->cbfs_legacy_size = ar.size; + } + + if (CONFIG(COMMON_CBFS_SPI_WRAPPER)) { + /* rdev_mmap will return a pointer to _postram_cbfs_cache */ + bmp2->mmap_offset = ~(uint64_t)0; + bmp2->mmap_size = 0; + bmp2->mmap_mmio_address = ~(uint64_t)0; + } else { + /* FIXME: Introduce API to set correct values here */ + bmp2->mmap_offset = 0; + bmp2->mmap_size = bmp2->boot_media_size; + /* Get address to MMAP boot media */ + uintptr_t off = (uintptr_t)rdev_mmap(boot_dev, 0, + bmp2->mmap_size); + bmp2->mmap_mmio_address = off; + rdev_munmap(boot_dev, (void *)off); + } }
static void lb_ram_code(struct lb_header *header)
Patrick Georgi 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:
why a new data structure? we generally allow extending structures with new fields, and maintain their size somewhere which keeps them compatible in both directions
Aaron Durbin 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:
(1 comment)
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@328 PS1, Line 328: off This is assuming the address returned by rdev_mmap() is a physical address. That may be true now, but nothing guarantees that in the API. I see you have a FIXME above, but it's a big assumption on implementation in various places.
Aaron Durbin 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:
(1 comment)
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@305 PS1, Line 305: size sz
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?
Werner Zeh 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:
(1 comment)
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 419: access accessed?
Patrick Rudolph 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:
(3 comments)
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 th […]
true, but I don't know the size. It's useful to mmap enough memory without parsing the fmap header first and to validate that the whole region falls within the mmap region as set by mmap_offset and mmap_size.
https://review.coreboot.org/#/c/33107/1/src/lib/coreboot_table.c@305 PS1, Line 305: size
sz
Done
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 […]
Done
Hello Werner Zeh, Kyösti Mälkki, Aaron Durbin, Julius Werner, build bot (Jenkins), Christian Gmeiner, Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33107
to look at the new patch set (#2).
Change subject: [RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2 ......................................................................
[RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2
BOOT_MEDIA_PARAMS2 exposes the boot media MMIO address if it's memory mapped in addition to various regions inside the bootmedia.
That information can be used by payloads to: * Support VBOOT on SeaBIOS, as it otherwise uses the RO CBFS only * Support Intel Apollolake and platforms that don't map the end of the BIOS region to the end of the address space * Support fwupd and flashrom finding the FMAP in memory
Change-Id: Ia0b1ac927b8782cc99cd7f34d8bf5c4ef60b5570 Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/commonlib/include/commonlib/coreboot_tables.h M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/lib/coreboot_table.c 4 files changed, 170 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/33107/2
build bot (Jenkins) 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 2:
(2 comments)
https://review.coreboot.org/#/c/33107/2/src/commonlib/include/commonlib/regi... File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/#/c/33107/2/src/commonlib/include/commonlib/regi... PS2, Line 89: uint64_t (*mmio_addr)(const struct region_device *, struct region *); function definition argument 'const struct region_device *' should also have an identifier name
https://review.coreboot.org/#/c/33107/2/src/commonlib/include/commonlib/regi... PS2, Line 89: uint64_t (*mmio_addr)(const struct region_device *, struct region *); function definition argument 'struct region *' should also have an identifier name
Hello Werner Zeh, Kyösti Mälkki, Aaron Durbin, Julius Werner, Arthur Heymans, build bot (Jenkins), Christian Gmeiner, Nico Huber, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33107
to look at the new patch set (#3).
Change subject: [RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2 ......................................................................
[RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2
BOOT_MEDIA_PARAMS2 exposes the boot media MMIO address if it's memory mapped in addition to various regions inside the bootmedia.
That information can be used by payloads to: * Support VBOOT on SeaBIOS, as it otherwise uses the RO CBFS only * Support Intel Apollolake and platforms that don't map the end of the BIOS region to the end of the address space * Support fwupd and flashrom finding the FMAP in memory
Change-Id: Ia0b1ac927b8782cc99cd7f34d8bf5c4ef60b5570 Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/commonlib/include/commonlib/coreboot_tables.h M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/lib/coreboot_table.c 4 files changed, 200 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/33107/3
build bot (Jenkins) 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 3:
(2 comments)
https://review.coreboot.org/#/c/33107/3/src/commonlib/include/commonlib/regi... File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/#/c/33107/3/src/commonlib/include/commonlib/regi... PS3, Line 89: uint64_t (*mmio_addr)(const struct region_device *, struct region *); function definition argument 'const struct region_device *' should also have an identifier name
https://review.coreboot.org/#/c/33107/3/src/commonlib/include/commonlib/regi... PS3, Line 89: uint64_t (*mmio_addr)(const struct region_device *, struct region *); function definition argument 'struct region *' should also have an identifier name
Aaron Durbin 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 3:
(2 comments)
https://review.coreboot.org/#/c/33107/2/src/commonlib/include/commonlib/regi... File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/#/c/33107/2/src/commonlib/include/commonlib/regi... PS2, Line 39: region device It's just a region -- not a region device. Ditto for the wording one line up as well.
https://review.coreboot.org/#/c/33107/2/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/#/c/33107/2/src/lib/coreboot_table.c@298 PS2, Line 298: rdev_mmio_addr I see what you are trying to do here, but I think it really confuses the point of the region/region device api.
Also, with your current implementation you will not get a mmap_mmio_address for anything except the translation device platforms. Is that intentional? It doesn't seem very helpful.
I think it's best to add an API to boot_device.h that provides the information you need. It would work for both non-users and users of xlate device since the default x86 one (src/arch/x86/mmap_boot.c) could implement it as well as the special ones (src/soc/intel/apollolake/mmap_boot.c).
What do you think?
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 3:
(1 comment)
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)
true, but I don't know the size. […]
I'm confused... where are you mmaping the FMAP? Not in this code, right? Or am I missing something?
If the payload wants to use this to read the FMAP, it should follow the same approach (first find out size, then read all entries). I mean, that's what calling find_fmap_directory() here does anyway. It doesn't make sense to spend more time re-reading the FMAP here just so that a payload might potentially spend less time reading it later.
Paul Menzel 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 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33107/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/33107/1/src/lib/coreboot_table.c@29... PS1, Line 291: /* LB_TAG_BOOT_MEDIA_PARAMS2 exposes additional parameters: : * - MMAPed boot media address : * - legacy CBFS position (RO partition in case of VBOOT) : */ Please use one of the allowed comment styles in the style guide.
Patrick Rudolph has uploaded a new patch set (#4) to the change originally created by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/33107 )
Change subject: [RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2 ......................................................................
[RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2
BOOT_MEDIA_PARAMS2 exposes the boot media MMIO address if it's memory mapped in addition to various regions inside the bootmedia.
That information can be used by payloads to: * Support VBOOT on SeaBIOS, as it otherwise uses the RO CBFS only * Support Intel Apollolake and platforms that don't map the end of the BIOS region to the end of the address space * Support fwupd and flashrom finding the FMAP in memory
Linux kernel module for testing: https://github.com/9elements/linux/tree/google_firmware_fmap
Tested on qemu: Both the FMAP and active VBOOT slot can be exported into sysfs.
Change-Id: Ia0b1ac927b8782cc99cd7f34d8bf5c4ef60b5570 Signed-off-by: Patrick Rudolph siro@das-labor.org --- M src/commonlib/include/commonlib/coreboot_tables.h M src/commonlib/include/commonlib/region.h M src/commonlib/region.c M src/lib/coreboot_table.c 4 files changed, 201 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/07/33107/4
build bot (Jenkins) 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 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33107/4/src/commonlib/include/commo... File src/commonlib/include/commonlib/region.h:
https://review.coreboot.org/c/coreboot/+/33107/4/src/commonlib/include/commo... PS4, Line 87: uint64_t (*mmio_addr)(const struct region_device *, struct region *); function definition argument 'const struct region_device *' should also have an identifier name
https://review.coreboot.org/c/coreboot/+/33107/4/src/commonlib/include/commo... PS4, Line 87: uint64_t (*mmio_addr)(const struct region_device *, struct region *); function definition argument 'struct region *' should also have an identifier name
Patrick Rudolph 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 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/33107/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/33107/1/src/lib/coreboot_table.c@28... PS1, Line 288: if (find_fmap_directory(&fmrd) == 0)
I'm confused... where are you mmaping the FMAP? Not in this code, right? Or am I missing something? […]
using FMAP_OFFSET instead
https://review.coreboot.org/c/coreboot/+/33107/1/src/lib/coreboot_table.c@31... PS1, Line 316: if (CONFIG(COMMON_CBFS_SPI_WRAPPER)) {
Shouldn't this check for BOOT_DEVICE_MEMORY_MAPPED instead?
Done
https://review.coreboot.org/c/coreboot/+/33107/1/src/lib/coreboot_table.c@32... PS1, Line 328: off
This is assuming the address returned by rdev_mmap() is a physical address. […]
introduced a new function rdev_mmio_addr()
https://review.coreboot.org/c/coreboot/+/33107/2/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/33107/2/src/lib/coreboot_table.c@29... PS2, Line 298: rdev_mmio_addr
I see what you are trying to do here, but I think it really confuses the point of the region/region […]
I tested on qemu and it works fine. I'll do tests on APL, but my understanding is that it should work on any memory mapped bootmedia.
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 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33107/4/src/commonlib/include/commo... File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/33107/4/src/commonlib/include/commo... PS4, Line 408: struct lb_boot_media_params { Still not clear why you can't just extend existing structure (see earlier CL-level comments about this).
https://review.coreboot.org/c/coreboot/+/33107/4/src/commonlib/include/commo... PS4, Line 494: uint64_t mmap_mmio_address; How does this relate to the FMAP-in-CBMEM thing you just uploaded? Most of this stuff is duplicated between the two structures. Do we need both? How about just creating a new separate struct for the MMAP stuff (that's only added for Apollolake-like devices) and getting the rest out of the FMAP copy?
Patrick Rudolph 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33107/4/src/commonlib/include/commo... File src/commonlib/include/commonlib/coreboot_tables.h:
https://review.coreboot.org/c/coreboot/+/33107/4/src/commonlib/include/commo... PS4, Line 494: uint64_t mmap_mmio_address;
How does this relate to the FMAP-in-CBMEM thing you just uploaded? Most of this stuff is duplicated […]
It's not just about APL. Other platforms like QEMU and SiFive Unleashed have memory mapped boot media, too.
I agree that most stuff isn't needed any more if FMAP is also present in memory.
Kyösti Mälkki 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 4:
Maybe I am mistaken, this got superseded by an alternative approach?
Patrick Rudolph 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 4:
The FMAP is now cached in cbmem, but accessing the memory mapped bootmedia (if any) in payloads is still a pain. I don't have a good solution yet, not time to investigate further.
Alexander Couzens 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 4:
Patch Set 4:
The FMAP is now cached in cbmem, but accessing the memory mapped bootmedia (if any) in payloads is still a pain. I don't have a good solution yet, not time to investigate further.
I think this patch can be abandoned.
Superseeded by fmap in cbmem: https://review.coreboot.org/c/coreboot/+/35377
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33107?usp=email )
Change subject: [RFC]lib/coreboot_tables: Introduce BOOT_MEDIA_PARAMS2 ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
Aaron Durbin (Code Review) -
Alexander Couzens (Code Review)
-
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Kyösti Mälkki (Code Review)
-
Martin L Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Werner Zeh (Code Review)