Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: coreboot_table: Add copy of FMAP ......................................................................
coreboot_table: Add copy of FMAP
For platform independend exposure of FMAP through a kernel module expose the FMAP through coreboot tables.
Create a copy of FMAP and place it in CBMEM.
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c 3 files changed, 36 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/1
diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index 8d0ca46..30bf88a 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -76,6 +76,7 @@ #define CBMEM_ID_ROM1 0x524f4d31 #define CBMEM_ID_ROM2 0x524f4d32 #define CBMEM_ID_ROM3 0x524f4d33 +#define CBMEM_ID_FMAP 0x464d4150
#define CBMEM_ID_TO_NAME_TABLE \ { CBMEM_ID_ACPI, "ACPI " }, \ @@ -134,5 +135,6 @@ { CBMEM_ID_ROM0, "VGA ROM #0 "}, \ { CBMEM_ID_ROM1, "VGA ROM #1 "}, \ { CBMEM_ID_ROM2, "VGA ROM #2 "}, \ - { CBMEM_ID_ROM3, "VGA ROM #3 "}, + { CBMEM_ID_ROM3, "VGA ROM #3 "}, \ + { CBMEM_ID_FMAP, "FMAP "}, #endif /* _CBMEM_ID_H_ */ diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h index 7bded2a..6816670 100644 --- a/src/commonlib/include/commonlib/coreboot_tables.h +++ b/src/commonlib/include/commonlib/coreboot_tables.h @@ -89,6 +89,7 @@ LB_TAG_VBOOT_WORKBUF = 0x0034, LB_TAG_MMC_INFO = 0x0035, LB_TAG_TCPA_LOG = 0x0036, + LB_TAG_FMAP = 0x0037, LB_TAG_CMOS_OPTION_TABLE = 0x00c8, LB_TAG_OPTION = 0x00c9, LB_TAG_OPTION_ENUM = 0x00ca, diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 95c2ae6..a72150c 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -336,7 +336,8 @@ {CBMEM_ID_ACPI_GNVS, LB_TAG_ACPI_GNVS}, {CBMEM_ID_VPD, LB_TAG_VPD}, {CBMEM_ID_WIFI_CALIBRATION, LB_TAG_WIFI_CALIBRATION}, - {CBMEM_ID_TCPA_LOG, LB_TAG_TCPA_LOG} + {CBMEM_ID_TCPA_LOG, LB_TAG_TCPA_LOG}, + {CBMEM_ID_FMAP, LB_TAG_FMAP}, }; int i;
@@ -476,6 +477,34 @@ return (unsigned long)rec + rec->size; }
+static void add_fmap_copy_cbmem(void) +{ + struct region_device fmrd; + + if (find_fmap_directory(&fmrd)) { + printk(BIOS_ERR, "Failed to find FMAP\n"); + return; + } + + void *fmap_cbmem = cbmem_add(CBMEM_ID_FMAP, region_device_sz(&fmrd)); + if (!fmap_cbmem) { + printk(BIOS_ERR, "Failed to allocate CBMEM\n"); + return; + } + + void *fmap_reg = rdev_mmap_full(&fmrd); + if (!fmap_reg) { + printk(BIOS_ERR, "Failed to mmap FMAP device\n"); + return; + } + + memcpy(fmap_cbmem, fmap_reg, region_device_sz(&fmrd)); + printk(BIOS_ERR, "fmap_cbmem %x\n", *((uint32_t*)(fmap_cbmem))); + printk(BIOS_ERR, "fmap_cbmem %x\n", *((uint32_t*)(fmap_cbmem+4))); + + rdev_munmap(&fmrd, fmap_reg); +} + size_t write_coreboot_forwarding_table(uintptr_t entry, uintptr_t target) { struct lb_header *head; @@ -563,6 +592,8 @@ if (CONFIG(BOOT_DEVICE_SPI_FLASH)) lb_spi_flash(head);
+ /* Add copy of FMAP to CBMEM */ + add_fmap_copy_cbmem(); add_cbmem_pointers(head);
/* Add board-specific table entries, if any. */
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: coreboot_table: Add copy of FMAP ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35377/1/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/35377/1/src/lib/coreboot_table.c@50... PS1, Line 502: printk(BIOS_ERR, "fmap_cbmem %x\n", *((uint32_t*)(fmap_cbmem))); "(foo*)" should be "(foo *)"
https://review.coreboot.org/c/coreboot/+/35377/1/src/lib/coreboot_table.c@50... PS1, Line 503: printk(BIOS_ERR, "fmap_cbmem %x\n", *((uint32_t*)(fmap_cbmem+4))); "(foo*)" should be "(foo *)"
Patrick Rudolph has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: coreboot_table: Add copy of FMAP ......................................................................
coreboot_table: Add copy of FMAP
For platform independend exposure of FMAP through a kernel module expose the FMAP through coreboot tables.
Create a copy of FMAP and place it in CBMEM.
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c 3 files changed, 34 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: coreboot_table: Add copy of FMAP ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35377/2/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/35377/2/src/lib/coreboot_table.c@48... PS2, Line 489: void *fmap_cbmem = cbmem_add(CBMEM_ID_FMAP, region_device_sz(&fmrd)); Yay, more reserved memory that the OS will never get back...
Any chance I could persuade you to integrate this into the FMAP code as a cache, so that after CBMEM is up all the FMAP functions won't need to read from flash again every time they're called? That might help convince me that this is worth the cost... ;)
https://review.coreboot.org/c/coreboot/+/35377/2/src/lib/coreboot_table.c@50... PS2, Line 501: memcpy(fmap_cbmem, fmap_reg, region_device_sz(&fmrd)); Please use rdev_read() instead of mmap so it isn't copied twice on non-x86 devices.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: coreboot_table: Add copy of FMAP ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35377/2/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/35377/2/src/lib/coreboot_table.c@48... PS2, Line 489: void *fmap_cbmem = cbmem_add(CBMEM_ID_FMAP, region_device_sz(&fmrd));
Yay, more reserved memory that the OS will never get back... […]
I'll look into that
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#3).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
TODO: Is it secure to store FMAP in DRAM?
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 43 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/3
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#4).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
TODO: Is it secure to store FMAP in DRAM?
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 43 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/4
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG@14 PS4, Line 14: TODO: Is it secure to store FMAP in DRAM? It should be at boot time because there's nothing else that could access DRAM. I guess it may be an issue for x86 SMM if you're worried that the OS would use this as an attack vector into SMM. Maybe ask the x86 guys if they think this is a concern (or if there's any platform-specific way they could force this memory to be read-only to the OS), or maybe just skip the caching path for ENV_SMM for now.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@32 PS4, Line 32: static struct fmap *fmap_c CAR_GLOBAL; I'm not a CAR_GLOBAL expert but I think(?) these days you don't need it for cases like this anymore.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@108 PS4, Line 108: /* Try to cache FMAP. Might fail if CBMEM isn't up yet. */ Why handle this here instead of inside find_fmap_directory()? That way the fmap_find_region_name() below can't share it. Why not just make find_fmap_directory() do a mem_region_device_ro_init() on the CBMEM area if it exists?
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@111 PS4, Line 111: car_set_var(fmap_c, cbmem_add(CBMEM_ID_FMAP, s)); Probably cleaner to create the cache in a ROMSTAGE_CBMEM_INIT_HOOK().
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 4:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG@14 PS4, Line 14: TODO: Is it secure to store FMAP in DRAM?
It should be at boot time because there's nothing else that could access DRAM. […]
It really depends on when one wants to access it. If it's only during the boot sequence then one should be explicitly writing the code to handle that. Right now it's all interleaved in the common which I don't think is correct.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@104 PS4, Line 104: if (!car_get_var(fmap_c)) { This is all quite messy. Please use a local variable or just remove the CAR_GLOBAL decorations. It'd be cleaner to also break up this logic into some helper functions which would also contain the policies (along w/ comments) you are wanting to do.
e.g. I think you want to use the cache except in SMM? And then when you aren't in SMM you want this storage to be in cbmem. Why aren't you using cbmem's callbacks to see this at the correct time?
https://github.com/coreboot/coreboot/blob/master/src/include/cbmem.h#L119
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@141 PS4, Line 141: rdev_munmap(&fmrd, area); This is not correct now since you conditionalized where area is being set.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#5).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
TODO: Is it secure to store FMAP in DRAM?
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 57 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/5
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 5:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG@14 PS4, Line 14: TODO: Is it secure to store FMAP in DRAM?
It really depends on when one wants to access it. […]
SMM doesn't link in cbmem functions, so it would never use the cache. I was thinking about S3 resume attacks, too.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@32 PS4, Line 32: static struct fmap *fmap_c CAR_GLOBAL;
I'm not a CAR_GLOBAL expert but I think(?) these days you don't need it for cases like this anymore.
Leaving this in as there are still FSP1.0 platforms and AMD? that needs CAR_GLOBAL.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@104 PS4, Line 104: if (!car_get_var(fmap_c)) {
This is all quite messy. Please use a local variable or just remove the CAR_GLOBAL decorations. […]
I can't cache in SMM as the cbmem functions aren't linked in.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@108 PS4, Line 108: /* Try to cache FMAP. Might fail if CBMEM isn't up yet. */
Why handle this here instead of inside find_fmap_directory()? That way the fmap_find_region_name() b […]
I tried to but with CAR_GLOBAL it's difficult to implement. Also does the SPI code for writing MRC cache depend on the device type? rdev vs mdev?
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@111 PS4, Line 111: car_set_var(fmap_c, cbmem_add(CBMEM_ID_FMAP, s));
Probably cleaner to create the cache in a ROMSTAGE_CBMEM_INIT_HOOK().
Done
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@141 PS4, Line 141: rdev_munmap(&fmrd, area);
This is not correct now since you conditionalized where area is being set.
Done
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#6).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
TODO: Is it secure to store FMAP in DRAM?
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 57 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/6
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG@14 PS4, Line 14: TODO: Is it secure to store FMAP in DRAM?
SMM doesn't link in cbmem functions, so it would never use the cache. I was thinking about S3 resume attacks, too.
On resume one should re-seed the cache from its source if one believes DRAM can be compromised.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@104 PS4, Line 104: if (!car_get_var(fmap_c)) {
I can't cache in SMM as the cbmem functions aren't linked in.
OK. It's certainly possible to cache things in SMM as well if one wanted. However, that is separate from my suggestion about using the cbmem callbacks for seeding the cache.
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@108 PS4, Line 108: /* Try to cache FMAP. Might fail if CBMEM isn't up yet. */
I tried to but with CAR_GLOBAL it's difficult to implement. Also does the SPI code for writing MRC cache depend on the device type? rdev vs mdev?
What do the MRC cache dependencies have to do with this code? I'm confused as to why you are asking.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@32 PS6, Line 32: static const struct cbmem_entry *fmap_cache CAR_GLOBAL; I think it's cleaner to have struct mem_region_device as your cache. One can check if it's initialized by region_device_sz(&cache.rdev) != 0.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@159 PS6, Line 159: } Why are we duplicating this sequence? A helper function can easily initialize the fmrd object that is passed into it instead of duplicating the logic. Similarly, one can apply the desired policy -- like not using cache in SMM, e.g.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@221 PS6, Line 221: Can't you check fmap_cache value and return early here?
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@223 PS6, Line 223: if (!e) { I think we should always re-seed the cache in cbmem. But one needs to ensure sizes match of the cbmem_entry and size of region directory.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG@14 PS4, Line 14: TODO: Is it secure to store FMAP in DRAM?
SMM doesn't link in cbmem functions, so it would never use the cache. […]
Don't we resume into DRAM (relocatable ramstage) anyway? Or am I confusing things there?
Generally, for the OS and lower, there's no point in worrying about a DRAM compromise between suspend/resume. I don't know if there is for coreboot.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@159 PS6, Line 159: }
Why are we duplicating this sequence? A helper function can easily initialize the fmrd object that i […]
If the global to store the cache is a struct mem_region_device, we shouldn't even need a helper... we could just put this into the top of find_fmap_directory().
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@223 PS6, Line 223: if (!e) {
I think we should always re-seed the cache in cbmem. […]
...but then we're loading it from flash 3 times each boot (in addition to all the times we need it in pre-RAM code)? Why?
If you're worried about S3 resume, we could check for ENV_ROMSTAGE here, but for the other two stages I see no reason to reread it?
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@236 PS6, Line 236: printk(BIOS_ERR, "Failed to read FMAP into CBMEM\n"); This should probably die() because otherwise you'd be running around with a cache full of invalid data (or you'd have to cbmem_entry_remove() it again).
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#7).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Needs tests on other platforms.
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 69 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/7
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@32 PS6, Line 32: static const struct cbmem_entry *fmap_cache CAR_GLOBAL;
I think it's cleaner to have struct mem_region_device as your cache. […]
Done
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@159 PS6, Line 159: }
If the global to store the cache is a struct mem_region_device, we shouldn't even need a helper... […]
Done
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@221 PS6, Line 221:
Can't you check fmap_cache value and return early here?
Done
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@223 PS6, Line 223: if (!e) {
... […]
Done
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@236 PS6, Line 236: printk(BIOS_ERR, "Failed to read FMAP into CBMEM\n");
This should probably die() because otherwise you'd be running around with a cache full of invalid da […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG@14 PS4, Line 14: TODO: Is it secure to store FMAP in DRAM?
Don't we resume into DRAM (relocatable ramstage) anyway? Or am I confusing things there?
Generally, for the OS and lower, there's no point in worrying about a DRAM compromise between suspend/resume. I don't know if there is for coreboot.
Chrome OS devices that employed S3 resume would put ramstage in an area of memory off limits from the OS. It would then be copied to execution place. That said, cbmem is not protected from the OS so there's an assumption there about its integrity being maintained.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@159 PS6, Line 159: }
Done
Well, there is logic around when to utilize and/or seed the cache.
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@223 PS6, Line 223: if (!e) {
Done
I don't understand the loading from flash 3 times each boot. Julius were you thinking I was suggesting to unconditionally read it in from flash on every cbmem recovery? I wasn't, and I expected we'd have the necessary guards.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@42 PS7, Line 42: if (cbmem_possibly_online()) { I don't think this check is necessary as the size check below should always work.
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@45 PS7, Line 45: fmap_cache just use car_get_var(&fmap_cache);
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@223 PS7, Line 223: mdev Why can't we use the global directly? I don't understand why we'd be copying objects from local to global.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/6/src/lib/fmap.c@223 PS6, Line 223: if (!e) {
I don't understand the loading from flash 3 times each boot. […]
Yeah, I thought that's what you mean by "always re-seed" here. Maybe I misunderstood.
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@42 PS7, Line 42: if (cbmem_possibly_online()) {
I don't think this check is necessary as the size check below should always work.
Saves 5 more instructions for pre-RAM stages, I guess?
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@47 PS7, Line 47: if (!rdev_chain(fmrd, &cache->rdev, 0, region_device_sz(&cache->rdev))) Is there a point in checking this rather than just 'return rdev_chain(...)'? I don't see how it could fail.
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@212 PS7, Line 212: fmap_cache_setup_late nit: I think register_cache() or find_cache() or something would be more clear than setup_late().
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#8).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Needs tests on other platforms.
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 68 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/8
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 8:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@42 PS7, Line 42: if (cbmem_possibly_online()) {
Saves 5 more instructions for pre-RAM stages, I guess?
Yes, that will remove the caching code in bootblock, verstage and SMM, as it doesn't work anyway.
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@45 PS7, Line 45: fmap_cache
just use car_get_var(&fmap_cache);
Done
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@47 PS7, Line 47: if (!rdev_chain(fmrd, &cache->rdev, 0, region_device_sz(&cache->rdev)))
Is there a point in checking this rather than just 'return rdev_chain(... […]
Done
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@212 PS7, Line 212: fmap_cache_setup_late
nit: I think register_cache() or find_cache() or something would be more clear than setup_late().
Done
https://review.coreboot.org/c/coreboot/+/35377/7/src/lib/fmap.c@223 PS7, Line 223: mdev
Why can't we use the global directly? I don't understand why we'd be copying objects from local to g […]
Done
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#9).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Needs tests on other platforms.
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 68 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/9
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 9: Code-Review+1
(2 comments)
LGTM to me now (name-bikeshedding isn't blocking), let's see what Aaron says.
https://review.coreboot.org/c/coreboot/+/35377/9/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/9/src/lib/fmap.c@221 PS9, Line 221: /* Don't set fmap_cache so that find_fmap_directory will use regular path */ nit: maybe that's just me but I'd avoid multiple lines without braces around them, even if the other line is a comment. I'd put the comment either on the line with the if() or above it.
https://review.coreboot.org/c/coreboot/+/35377/9/src/lib/fmap.c@231 PS9, Line 231: fmap_register_cache nit: Sorry, I meant "register" for the other one. To me, "register" says "find this thing that already exists and hook it up". For this function here I think setup_cache() was a better fit.
Hello Julius Werner, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#10).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Needs tests on other platforms.
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 68 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/10
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35377/9/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/9/src/lib/fmap.c@221 PS9, Line 221: /* Don't set fmap_cache so that find_fmap_directory will use regular path */
nit: maybe that's just me but I'd avoid multiple lines without braces around them, even if the other […]
Done
https://review.coreboot.org/c/coreboot/+/35377/9/src/lib/fmap.c@231 PS9, Line 231: fmap_register_cache
nit: Sorry, I meant "register" for the other one. […]
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 10: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 10:
Tested on Supermicro X11SSH-TF using VBOOT, still boots to OS.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35377/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/10//COMMIT_MSG@9 PS10, Line 9: For platform independend exposure of FMAP through a kernel module : cache the FMAP in CBMEM. maybe state that you add a pointer in coreboot tables?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35377/10/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/10/src/lib/fmap.c@46 PS10, Line 46: if (region_device_sz(&cache->rdev)) this accesses cache->rdev (and inside, several more level of indirection) without checking if any of that is valid
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 10: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/35377/10/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/10/src/lib/fmap.c@46 PS10, Line 46: if (region_device_sz(&cache->rdev))
this accesses cache->rdev (and inside, several more level of indirection) without checking if any of […]
That should be fine? There should be no way to dereference a NULL pointer if that's what you're worried about, the result of car_get_var_ptr() should always be valid and the struct region that contains the size field is directly embedded in struct mem_region_device. I know it's a bit of pointer soup to unwrap all these APIs, but in the end it should only reference stuff that's part of the global variable itself.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/4//COMMIT_MSG@14 PS4, Line 14: TODO: Is it secure to store FMAP in DRAM?
Don't we resume into DRAM (relocatable ramstage) anyway? Or am I confusing things there? […]
Done
https://review.coreboot.org/c/coreboot/+/35377/2/src/lib/coreboot_table.c File src/lib/coreboot_table.c:
https://review.coreboot.org/c/coreboot/+/35377/2/src/lib/coreboot_table.c@48... PS2, Line 489: void *fmap_cbmem = cbmem_add(CBMEM_ID_FMAP, region_device_sz(&fmrd));
I'll look into that
Done
https://review.coreboot.org/c/coreboot/+/35377/2/src/lib/coreboot_table.c@50... PS2, Line 501: memcpy(fmap_cbmem, fmap_reg, region_device_sz(&fmrd));
Please use rdev_read() instead of mmap so it isn't copied twice on non-x86 devices.
Done
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c File src/lib/fmap.c:
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@32 PS4, Line 32: static struct fmap *fmap_c CAR_GLOBAL;
Leaving this in as there are still FSP1.0 platforms and AMD? that needs CAR_GLOBAL.
Done
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@104 PS4, Line 104: if (!car_get_var(fmap_c)) {
OK. It's certainly possible to cache things in SMM as well if one wanted. […]
Done
https://review.coreboot.org/c/coreboot/+/35377/4/src/lib/fmap.c@108 PS4, Line 108: /* Try to cache FMAP. Might fail if CBMEM isn't up yet. */
I tried to but with CAR_GLOBAL it's difficult to implement. […]
Done
Hello Aaron Durbin, Julius Werner, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#11).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM. In addition add a pointer in coreboot tables pointing to the introduced CBMEM area.
Use not waste the allocated DRAM, use the cached CBMEM if possible.
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Tested on QEMU and Supermicro X11SSH-TF.
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 68 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/11
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35377/10//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35377/10//COMMIT_MSG@9 PS10, Line 9: For platform independend exposure of FMAP through a kernel module : cache the FMAP in CBMEM.
maybe state that you add a pointer in coreboot tables?
Done
Hello Aaron Durbin, Julius Werner, Arthur Heymans, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35377
to look at the new patch set (#12).
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM. In addition add a pointer in coreboot tables pointing to the introduced CBMEM area.
To not waste the allocated DRAM, use the cached CBMEM in RAM enabled stages if possible.
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Tested on QEMU and Supermicro X11SSH-TF.
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 68 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/35377/12
Philipp Deppenwiese has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
lib/fmap: Cache FMAP in cbmem
For platform independend exposure of FMAP through a kernel module cache the FMAP in CBMEM. In addition add a pointer in coreboot tables pointing to the introduced CBMEM area.
To not waste the allocated DRAM, use the cached CBMEM in RAM enabled stages if possible.
Tested on qemu using https://github.com/9elements/linux/commits/google_firmware_fmap2
Tested on QEMU and Supermicro X11SSH-TF.
Change-Id: I4e01c573c3edfa34dbba5fe7604d4f6e18b584d5 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35377 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz Reviewed-by: Julius Werner jwerner@chromium.org --- M src/commonlib/include/commonlib/cbmem_id.h M src/commonlib/include/commonlib/coreboot_tables.h M src/lib/coreboot_table.c M src/lib/fmap.c 4 files changed, 68 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Julius Werner: Looks good to me, approved Arthur Heymans: Looks good to me, approved
diff --git a/src/commonlib/include/commonlib/cbmem_id.h b/src/commonlib/include/commonlib/cbmem_id.h index 8d0ca46..30bf88a 100644 --- a/src/commonlib/include/commonlib/cbmem_id.h +++ b/src/commonlib/include/commonlib/cbmem_id.h @@ -76,6 +76,7 @@ #define CBMEM_ID_ROM1 0x524f4d31 #define CBMEM_ID_ROM2 0x524f4d32 #define CBMEM_ID_ROM3 0x524f4d33 +#define CBMEM_ID_FMAP 0x464d4150
#define CBMEM_ID_TO_NAME_TABLE \ { CBMEM_ID_ACPI, "ACPI " }, \ @@ -134,5 +135,6 @@ { CBMEM_ID_ROM0, "VGA ROM #0 "}, \ { CBMEM_ID_ROM1, "VGA ROM #1 "}, \ { CBMEM_ID_ROM2, "VGA ROM #2 "}, \ - { CBMEM_ID_ROM3, "VGA ROM #3 "}, + { CBMEM_ID_ROM3, "VGA ROM #3 "}, \ + { CBMEM_ID_FMAP, "FMAP "}, #endif /* _CBMEM_ID_H_ */ diff --git a/src/commonlib/include/commonlib/coreboot_tables.h b/src/commonlib/include/commonlib/coreboot_tables.h index 7bded2a..6816670 100644 --- a/src/commonlib/include/commonlib/coreboot_tables.h +++ b/src/commonlib/include/commonlib/coreboot_tables.h @@ -89,6 +89,7 @@ LB_TAG_VBOOT_WORKBUF = 0x0034, LB_TAG_MMC_INFO = 0x0035, LB_TAG_TCPA_LOG = 0x0036, + LB_TAG_FMAP = 0x0037, LB_TAG_CMOS_OPTION_TABLE = 0x00c8, LB_TAG_OPTION = 0x00c9, LB_TAG_OPTION_ENUM = 0x00ca, diff --git a/src/lib/coreboot_table.c b/src/lib/coreboot_table.c index 66afcf3..9c5942f 100644 --- a/src/lib/coreboot_table.c +++ b/src/lib/coreboot_table.c @@ -345,7 +345,8 @@ {CBMEM_ID_ACPI_GNVS, LB_TAG_ACPI_GNVS}, {CBMEM_ID_VPD, LB_TAG_VPD}, {CBMEM_ID_WIFI_CALIBRATION, LB_TAG_WIFI_CALIBRATION}, - {CBMEM_ID_TCPA_LOG, LB_TAG_TCPA_LOG} + {CBMEM_ID_TCPA_LOG, LB_TAG_TCPA_LOG}, + {CBMEM_ID_FMAP, LB_TAG_FMAP}, }; int i;
diff --git a/src/lib/fmap.c b/src/lib/fmap.c index 2b4e3ba..f007418 100644 --- a/src/lib/fmap.c +++ b/src/lib/fmap.c @@ -20,6 +20,7 @@ #include <commonlib/fmap_serialized.h> #include <stddef.h> #include <string.h> +#include <cbmem.h>
#include "fmap_config.h"
@@ -28,6 +29,7 @@ */
static int fmap_print_once CAR_GLOBAL; +static struct mem_region_device fmap_cache CAR_GLOBAL;
int find_fmap_directory(struct region_device *fmrd) { @@ -36,6 +38,16 @@ size_t fmap_size; size_t offset = FMAP_OFFSET;
+ if (cbmem_possibly_online() && !ENV_SMM) { + /* Try FMAP cache first */ + const struct mem_region_device *cache; + + cache = car_get_var_ptr(&fmap_cache); + if (region_device_sz(&cache->rdev)) + return rdev_chain(fmrd, &cache->rdev, 0, + region_device_sz(&cache->rdev)); + } + boot_device_init(); boot = boot_device_ro();
@@ -195,3 +207,53 @@ return -1; return rdev_writeat(&rdev, buffer, 0, size); } + +static void fmap_register_cache(int unused) +{ + const struct cbmem_entry *e; + struct mem_region_device *mdev; + + mdev = car_get_var_ptr(&fmap_cache); + + /* Find the FMAP cache installed by previous stage */ + e = cbmem_entry_find(CBMEM_ID_FMAP); + /* Don't set fmap_cache so that find_fmap_directory will use regular path */ + if (!e) + return; + + mem_region_device_ro_init(mdev, cbmem_entry_start(e), cbmem_entry_size(e)); +} + +/* + * The main reason to copy the FMAP into CBMEM is to make it available to the + * OS on every architecture. As side effect use the CBMEM copy as cache. + */ +static void fmap_setup_cache(int unused) +{ + struct region_device fmrd; + + if (find_fmap_directory(&fmrd)) + return; + + /* Reloads the FMAP even on ACPI S3 resume */ + const size_t s = region_device_sz(&fmrd); + struct fmap *fmap = cbmem_add(CBMEM_ID_FMAP, s); + if (!fmap) { + printk(BIOS_ERR, "ERROR: Failed to allocate CBMEM\n"); + return; + } + + const ssize_t ret = rdev_readat(&fmrd, fmap, 0, s); + if (ret != s) { + printk(BIOS_ERR, "ERROR: Failed to read FMAP into CBMEM\n"); + cbmem_entry_remove(cbmem_entry_find(CBMEM_ID_FMAP)); + return; + } + + /* Finally advertise the cache for the current stage */ + fmap_register_cache(unused); +} + +ROMSTAGE_CBMEM_INIT_HOOK(fmap_setup_cache) +RAMSTAGE_CBMEM_INIT_HOOK(fmap_register_cache) +POSTCAR_CBMEM_INIT_HOOK(fmap_register_cache)
AndreX Andraos has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 13:
This change is very likely causing an issue. Testing drallion FW R79-12542 .. R79-12546 on moffett arcada_cml, dut is not booting up to dev screen:
Wipe memory regions: [0x00000000001000, 0x000000000a0000) [0x00000000100000, 0x00000030000000) [0x00000032639b60, 0x00000099a3e000) [0x00000100000000, 0x0000025c800000) EC: disable power button Calling VbSelectAndLoadKernel(). cr50 TPM 2.0 (i2c 0x50 id 0x28) tpm_get_response: command 0x14e, return code 0x0 ReadSpaceKernel: TPM: read secdata_kernel: 02 4c 57 52 47 01 00 01 00 00 00 00 55 RollbackKernelRead: TPM: RollbackKernelRead 0x10001 tpm_get_response: command 0x14e, return code 0x28b RollbackFwmpRead: TPM: no FWMP space VbSelectAndLoadKernel: GBB flags are 0x2b9 vb2_developer_ui: Entering vboot_draw_screen: screen=0x101 locale=0 Bad signature on the FMAP.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35377 )
Change subject: lib/fmap: Cache FMAP in cbmem ......................................................................
Patch Set 13:
Patch Set 13:
This change is very likely causing an issue. Testing drallion FW R79-12542 .. R79-12546 on moffett arcada_cml, dut is not booting up to dev screen:
Wipe memory regions: [0x00000000001000, 0x000000000a0000) [0x00000000100000, 0x00000030000000) [0x00000032639b60, 0x00000099a3e000) [0x00000100000000, 0x0000025c800000) EC: disable power button Calling VbSelectAndLoadKernel(). cr50 TPM 2.0 (i2c 0x50 id 0x28) tpm_get_response: command 0x14e, return code 0x0 ReadSpaceKernel: TPM: read secdata_kernel: 02 4c 57 52 47 01 00 01 00 00 00 00 55 RollbackKernelRead: TPM: RollbackKernelRead 0x10001 tpm_get_response: command 0x14e, return code 0x28b RollbackFwmpRead: TPM: no FWMP space VbSelectAndLoadKernel: GBB flags are 0x2b9 vb2_developer_ui: Entering vboot_draw_screen: screen=0x101 locale=0 Bad signature on the FMAP.
There's a fix here: https://review.coreboot.org/c/coreboot/+/35639
-
Aaron Durbin (Code Review) -
AndreX Andraos (Code Review)
-
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Philipp Deppenwiese (Code Review)