Change in coreboot[master]: mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h 3 files changed, 99 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/1
diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 7f5d389..21bbf67 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -93,8 +93,8 @@
static void fsp_fill_mrc_cache(FSPM_ARCH_UPD *arch_upd, uint32_t fsp_version) { - struct region_device rdev; void *data; + size_t mrc_size;
arch_upd->NvsBufferPtr = NULL;
@@ -113,25 +113,24 @@ return; }
- if (mrc_cache_get_current(MRC_TRAINING_DATA, fsp_version, &rdev) < 0) + data = mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, fsp_version, + &mrc_size); + if (data == NULL) return;
/* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - data = rdev_mmap_full(&rdev);
- if (data == NULL) - return;
if (CONFIG(FSP2_0_USES_TPM_MRC_HASH) && - !mrc_cache_verify_hash(data, region_device_sz(&rdev))) + !mrc_cache_verify_hash(data, mrc_size)) return;
/* MRC cache found */ arch_upd->NvsBufferPtr = data;
printk(BIOS_SPEW, "MRC cache found, size %zx\n", - region_device_sz(&rdev)); + mrc_size); }
static enum cb_err check_region_overlap(const struct memranges *ranges, diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index d567a20..8191a53 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -204,23 +204,16 @@ return 0; }
-static int mrc_data_valid(const struct region_device *rdev, - const struct mrc_metadata *md) +static int mrc_data_valid(const struct mrc_metadata *md, + void *data, size_t data_size) { - void *data; uint16_t checksum; - const size_t md_size = sizeof(*md); - const size_t data_size = md->data_size;
- data = rdev_mmap(rdev, md_size, data_size); - if (data == NULL) { - printk(BIOS_ERR, "MRC: mmap failure on data verification.\n"); + if (md->data_size != data_size) return -1; - }
checksum = compute_ip_checksum(data, data_size);
- rdev_munmap(rdev, data); if (md->data_checksum != checksum) { printk(BIOS_ERR, "MRC: data checksum mismatch: %x vs %x\n", md->data_checksum, checksum); @@ -230,7 +223,7 @@ return 0; }
-static int mrc_cache_latest(const char *name, +static int mrc_cache_get_latest_slot_info(const char *name, const struct region_device *backing_rdev, struct mrc_metadata *md, struct region_file *cache_file, @@ -260,25 +253,19 @@ return fail_bad_data ? -1 : 0; }
- /* Validate Data */ - if (mrc_data_valid(rdev, md) < 0) { - printk(BIOS_ERR, "MRC: invalid data in '%s'\n", name); - return fail_bad_data ? -1 : 0; - } - return 0; }
-int mrc_cache_get_current(int type, uint32_t version, - struct region_device *rdev) +static int mrc_cache_find_current(int type, uint32_t version, + struct region_device *rdev, + struct mrc_metadata *md) { const struct cache_region *cr; struct region region; struct region_device read_rdev; struct region_file cache_file; - struct mrc_metadata md; size_t data_size; - const size_t md_size = sizeof(md); + const size_t md_size = sizeof(*md); const bool fail_bad_data = true;
cr = lookup_region(®ion, type); @@ -289,21 +276,75 @@ if (boot_device_ro_subregion(®ion, &read_rdev) < 0) return -1;
- if (mrc_cache_latest(cr->name, &read_rdev, &md, &cache_file, rdev, - fail_bad_data) < 0) + if (mrc_cache_get_latest_slot_info(cr->name, + &read_rdev, + md, + &cache_file, + rdev, + fail_bad_data) < 0) return -1;
- if (version != md.version) { + if (version != md->version) { printk(BIOS_INFO, "MRC: version mismatch: %x vs %x\n", - md.version, version); + md->version, version); return -1; }
/* Re-size rdev to only contain the data. i.e. remove metadata. */ - data_size = md.data_size; + data_size = md->data_size; return rdev_chain(rdev, rdev, md_size, data_size); }
+int mrc_cache_load_current(int type, uint32_t version, void *buffer, + size_t buffer_size) +{ + struct region_device rdev; + struct mrc_metadata md; + size_t data_size; + + if (mrc_cache_find_current(type, version, &rdev, &md) < 0) + return -1; + + data_size = region_device_sz(&rdev); + if (buffer_size < data_size) + return -1; + + if (rdev_readat(&rdev, buffer, 0, data_size) != data_size) + return -1; + + if (mrc_data_valid(&md, buffer, buffer_size) < 0) + return -1; + + return 0; +} + +void *mrc_cache_current_mmap_leak(int type, uint32_t version, + size_t *data_size) +{ + struct region_device rdev; + void *data; + size_t region_device_size; + struct mrc_metadata md; + + if (mrc_cache_find_current(type, version, &rdev, &md) < 0) + return NULL; + + region_device_size = region_device_sz(&rdev); + if (data_size) + *data_size = region_device_size; + data = rdev_mmap(&rdev, 0, region_device_size); + + if (data == NULL) { + printk(BIOS_ERR, "MRC: mmap failure on data verification.\n"); + return NULL; + } + + if (mrc_data_valid(&md, data, *data_size) < 0) + return NULL; + + return data; +} + static bool mrc_cache_needs_update(const struct region_device *rdev, const struct cbmem_entry *to_be_updated) { @@ -392,8 +433,13 @@ if (backing_rdev == NULL) return;
- if (mrc_cache_latest(cr->name, backing_rdev, &md, &cache_file, - &latest_rdev, fail_bad_data) < 0) + if (mrc_cache_get_latest_slot_info(cr->name, + backing_rdev, + &md, + &cache_file, + &latest_rdev, + fail_bad_data) < 0) + return;
if (!mrc_cache_needs_update(&latest_rdev, to_be_updated)) { diff --git a/src/include/mrc_cache.h b/src/include/mrc_cache.h index 1cefba9..c34e0cc 100644 --- a/src/include/mrc_cache.h +++ b/src/include/mrc_cache.h @@ -23,9 +23,25 @@
/* Get and stash data for saving provided the type passed in. The functions * return < 0 on error, 0 on success. */ -int mrc_cache_get_current(int type, uint32_t version, - struct region_device *rdev); +/** + * mrc_cache_load_current + * + * Fill in the buffer with the latest slot data. This will be a + * common entry point for ARM platforms. + */ +int mrc_cache_load_current(int type, uint32_t version, void *buffer, + size_t buffer_size); +/** + * mrc_cache_mmap_leak + * + * Return a pointer to a buffer with the latest slot data An mmap will + * be executed (without a matching unmmap), so it is up to the caller + * to unmap if necessary. This will be a common entry point for + * platforms where mmap is considered a noop, like x86 + */ +void *mrc_cache_current_mmap_leak(int type, uint32_t version, + size_t *data_size); int mrc_cache_stash_data(int type, uint32_t version, const void *data, - size_t size); + size_t size);
#endif /* _COMMON_MRC_CACHE_H_ */
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 1:
(8 comments)
Jenkins says you forgot src/drivers/intel/fsp1_1/romstage.c
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/intel/fsp2_0/me... PS1, Line 123: nit: double blank line
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/intel/fsp2_0/me... PS1, Line 133: mrc_size); nit: does this fit one line now?
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... PS1, Line 299: This looks like it's trying to align to the parenthesis with spaces, but it's off the mark.
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... PS1, Line 315: buffer_size This must be data_size, not buffer_size
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... PS1, Line 335: data = rdev_mmap(&rdev, 0, region_device_size); nit: rdev_mmap_full(&rdev) is a shorthand for this
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... PS1, Line 342: *data_size This also needs to be region_device_size
https://review.coreboot.org/c/coreboot/+/44006/1/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/1/src/include/mrc_cache.h@37 PS1, Line 37: nit: missing period
https://review.coreboot.org/c/coreboot/+/44006/1/src/include/mrc_cache.h@38 PS1, Line 38: unmmap nit: "unmap" (or, if you want to reference the exact name of the function, munmap)
Hello build bot (Jenkins), Furquan Shaikh, Tim Wawrzynczak, Julius Werner, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#2).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 186 insertions(+), 112 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/2
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 2:
(8 comments)
Patch Set 1:
(8 comments)
Jenkins says you forgot src/drivers/intel/fsp1_1/romstage.c
Jenkins is always right.
I'm temporarily using Jenkins for building all the boards to make sure that I didn't miss anything else, so please hold up on reviewing this change for now...
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/intel/fsp2_0/me... PS1, Line 123:
nit: double blank line
Done
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/intel/fsp2_0/me... PS1, Line 133: mrc_size);
nit: does this fit one line now?
Done
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... PS1, Line 299:
This looks like it's trying to align to the parenthesis with spaces, but it's off the mark.
Done
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... PS1, Line 315: buffer_size
This must be data_size, not buffer_size
Done
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... PS1, Line 335: data = rdev_mmap(&rdev, 0, region_device_size);
nit: rdev_mmap_full(&rdev) is a shorthand for this
Done
https://review.coreboot.org/c/coreboot/+/44006/1/src/drivers/mrc_cache/mrc_c... PS1, Line 342: *data_size
This also needs to be region_device_size
Done
https://review.coreboot.org/c/coreboot/+/44006/1/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/1/src/include/mrc_cache.h@37 PS1, Line 37:
nit: missing period
Done
https://review.coreboot.org/c/coreboot/+/44006/1/src/include/mrc_cache.h@38 PS1, Line 38: unmmap
nit: "unmap" (or, if you want to reference the exact name of the function, munmap)
Done
Hello build bot (Jenkins), Damien Zammit, Furquan Shaikh, Lee Leahy, Tim Wawrzynczak, Angel Pons, Huang Jin, Julius Werner, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#3).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 186 insertions(+), 112 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/3
Hello build bot (Jenkins), Damien Zammit, Furquan Shaikh, Lee Leahy, Tim Wawrzynczak, Angel Pons, Huang Jin, Julius Werner, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#4).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 187 insertions(+), 112 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/4
Hello build bot (Jenkins), Damien Zammit, Furquan Shaikh, Lee Leahy, Tim Wawrzynczak, Angel Pons, Huang Jin, Julius Werner, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#5).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 188 insertions(+), 113 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/5
Hello build bot (Jenkins), Damien Zammit, Furquan Shaikh, Lee Leahy, Tim Wawrzynczak, Angel Pons, Huang Jin, Julius Werner, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#6).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 190 insertions(+), 115 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/6
Hello build bot (Jenkins), Damien Zammit, Furquan Shaikh, Lee Leahy, Tim Wawrzynczak, Angel Pons, Huang Jin, Julius Werner, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#7).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 192 insertions(+), 116 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/7
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 53: (CONFIG(CACHE_MRC_SETTINGS) This check was located earlier. Do the changed semantics of `mrc_cache_current_mmap_leak` prevent keeping the same code as before?
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 53: (CONFIG(CACHE_MRC_SETTINGS)
This check was located earlier. […]
So I tried to keep the assignment and the check in the same line as before, but coreboot has some pre-commit checks that prevent that type of code from going in (for good reason, since it's hard to read). The logic should be the same. We just split it into two lines now (one for the assignment and one for the condition). We need the assignment though, unlike the previous mrc_cache_get_current where we could just check the return value.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 7: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 53: (CONFIG(CACHE_MRC_SETTINGS)
So I tried to keep the assignment and the check in the same line as before, but coreboot has some pr […]
Ack, the only remaining FSP 1.1 platform (Braswell) selects CACHE_MRC_SETTINGS anyway.
I see several line breaks before the 96 characters limit, maybe your git hooks need an update? (`make gitconfig`)
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 53: (CONFIG(CACHE_MRC_SETTINGS)
Ack, the only remaining FSP 1.1 platform (Braswell) selects CACHE_MRC_SETTINGS anyway. […]
But, I mean, we can't just leave it like this, right? That's just wrong. It shouldn't be calling the MRC cache code if it's supposed to be disabled.
You can just use an extra set of parenthesis to make the GCC warning disappear, like this:
} else if (CONFIG(CACHE_MRC_SETTINGS) && (params->saved_data = mrc_cache_current_mmap_leak(...))) {
Alternatively you can do something like
} else { if (CONFIG(...)) params->saved_data = mrc_cache_current_mmap_leak(...); else params->saved_data = NULL;
if (params->saved_data) { ... } else if (s3wake) { ... } else { ... } }
Or, if we're saying that all boards using this code must have that Kconfig enabled, just take the check for it out and describe it via Kconfig depends on.
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 292: __weak void *mrc_cache_current_mmap_leak(int type, uint32_t version, I... am very confused why this is here. The way the old code is written (and the way I think you should write the new version), this shouldn't be needed because it's never calling the function if that Kconfig is disabled. Maybe this is left over from before we had --gc-sections enabled on x86 and can just be removed?
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/ironl... PS7, Line 1626: &mrc_size Are you not allowed to pass NULL here if you don't need it? We should write the function so it's legal to pass NULL here, for convenience in cases like this.
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/sandy... PS7, Line 278: This is missing the size check?
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/amd/common/block/s3... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/amd/common/block/s3... PS7, Line 36: size = mrc_size; Just pass `size` in directly?
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/intel/apollolake/ro... File src/soc/intel/apollolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/intel/apollolake/ro... PS7, Line 319: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); There was never a reason for these assert()s to be guarded, I'm not sure why a lot of this code is ordering it so weirdly (probably all copy&pasted from the same). You can just move this above (or below or wherever).
Hello build bot (Jenkins), Damien Zammit, Furquan Shaikh, Lee Leahy, Tim Wawrzynczak, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#8).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 193 insertions(+), 119 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/8
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 8:
(6 comments)
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 53: (CONFIG(CACHE_MRC_SETTINGS)
But, I mean, we can't just leave it like this, right? That's just wrong. […]
Ok, this is what Angel was trying to tell me earlier but I didn't understand. Sorry about that. Fixed.
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 292: __weak void *mrc_cache_current_mmap_leak(int type, uint32_t version,
I... am very confused why this is here. […]
Are we really not using this? I just changed it to make it consistent with the new definition, but I figure that it was there for a reason...
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/ironl... File src/northbridge/intel/ironlake/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/ironl... PS7, Line 1626: &mrc_size
Are you not allowed to pass NULL here if you don't need it? We should write the function so it's leg […]
Done
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/northbridge/intel/sandy... PS7, Line 278:
This is missing the size check?
Done
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/amd/common/block/s3... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/amd/common/block/s3... PS7, Line 36: size = mrc_size;
Just pass `size` in directly?
Done
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/intel/apollolake/ro... File src/soc/intel/apollolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/soc/intel/apollolake/ro... PS7, Line 319: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
There was never a reason for these assert()s to be guarded, I'm not sure why a lot of this code is o […]
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 8: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 292: __weak void *mrc_cache_current_mmap_leak(int type, uint32_t version,
Are we really not using this? I just changed it to make it consistent with the new definition, but […]
If you remove it and Jenkins doesn't complain, then we aren't using it.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 8:
(13 comments)
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp1_1/ro... PS8, Line 61: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); nit: I think this should be moved above the call to mrc_cache_current_mmap_leak().
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp2_0/me... PS8, Line 121: /* Assume boot device is memory mapped. */ : assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); nit: I think this should be moved above the call to mrc_cache_current_mmap_leak().
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 338: BIOS_ERR Is this really an error? MRC might not have valid data and that is fine e.g. first boot or firmware update with RW_MRC_CACHE erased or a trip to recovery.
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 338: on data verification Why "on data verification"?
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 436: if (mrc_cache_get_latest_slot_info(cr->name, There is a change in logic here i.e. current mrc cache is not checked to be valid. Can you please capture the difference in behavior in the commit message and the reason behind it.
Also, a comment here would be very helpful as to why we don't check the validity of current slot.
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@38 PS8, Line 38: so it is up to the : * caller to unmap if necessary. I don't think the caller can really unmap since it does not have pointer to the rdev structure used for mapping?
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@40 PS8, Line 40: * for platforms where mmap is considered a noop, like x86 nit: Just a note that the comment on line 24 is not really applicable now w.r.t. return value.
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@43 PS8, Line 43: data_size There is at least one path which calls this function with data_size set to NULL. It would be good to add a comment regarding the same.
BTW, in my opinion, it would be better to keep the API consistent for both loading and mmaping i.e. return 0 on success and < 0 on error and accept buffer pointer as input.
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/amd/common/block/s3... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/amd/common/block/s3... PS8, Line 32: if (!base) This is checked again in line 35.
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/apollolake/ro... File src/soc/intel/apollolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/apollolake/ro... PS8, Line 315: &mrc_size This is unused. Can `NULL` be passed instead?
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/baytrail/roms... PS8, Line 140: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); Same here.
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... File src/soc/intel/broadwell/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... PS8, Line 46: !pei_data->saved_data This check doesn't look right. This should be: `if (pei_data->saved_data)` i.e. if there is saved_data then update saved_data_size. Else the other checks are required.
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... PS8, Line 50: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); Same comment as other files.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 8: -Code-Review
(2 comments)
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@38 PS8, Line 38: so it is up to the : * caller to unmap if necessary.
I don't think the caller can really unmap since it does not have pointer to the rdev structure used […]
Technically you could do rdev_unmap(boot_device_ro(), mapping) since rdev ops are always only tied to the root device anyway. We could make an mrc_cache_unmap() to encapsulate that so the caller wouldn't have to guess which root device mrc_cache.c uses. But considering that this is only meant to be used by platforms that know mapping is a no-op for them anyway, I don't think that would be worth it.
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@43 PS8, Line 43: data_size
There is at least one path which calls this function with data_size set to NULL. […]
I think I would prefer keeping this consistent with the underlying rdev API to be honest. rdev_mmap() returns a pointer or NULL on error, and rdev_read() returns < 0 on error. mrc_cache_current_mmap_leak() is the higher level MRC-cache-specific equivalent of rdev_mmap() and mrc_cache_load_current() of rdev_read(), so I think it's good they follow the same return code conventions.
Hello build bot (Jenkins), Furquan Shaikh, Damien Zammit, Lee Leahy, Tim Wawrzynczak, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#9).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
Additionally, we are replacing mrc_cache_latest with mrc_cache_get_latest_slot_info, which does not check the validity of the data when retrieving the current mrc_cache slot. This allows the caller some flexibility in deciding where they want the mrc_cache data stored (either in an mmaped region or at a given address).
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 197 insertions(+), 123 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/9
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 9:
(12 comments)
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp1_1/ro... PS8, Line 61: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
nit: I think this should be moved above the call to mrc_cache_current_mmap_leak().
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp2_0/me... File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/intel/fsp2_0/me... PS8, Line 121: /* Assume boot device is memory mapped. */ : assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
nit: I think this should be moved above the call to mrc_cache_current_mmap_leak().
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 338: BIOS_ERR
Is this really an error? MRC might not have valid data and that is fine e.g. […]
In mrc_data_valid(), we originally pasted this error
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 338: on data verification
Why "on data verification"?
Sorry, copy/pasted the error message from mrc_data_valid() in the old driver.
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 436: if (mrc_cache_get_latest_slot_info(cr->name,
There is a change in logic here i.e. current mrc cache is not checked to be valid. […]
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@38 PS8, Line 38: so it is up to the : * caller to unmap if necessary.
Technically you could do rdev_unmap(boot_device_ro(), mapping) since rdev ops are always only tied t […]
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@43 PS8, Line 43: data_size
I think I would prefer keeping this consistent with the underlying rdev API to be honest. […]
I thought that we had agreed to return the void * in the past? Furquan, do you not agree with this anymore?
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/amd/common/block/s3... File src/soc/amd/common/block/s3/s3_resume.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/amd/common/block/s3... PS8, Line 32: if (!base)
This is checked again in line 35.
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/apollolake/ro... File src/soc/intel/apollolake/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/apollolake/ro... PS8, Line 315: &mrc_size
This is unused. […]
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/baytrail/roms... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/baytrail/roms... PS8, Line 140: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
Same here.
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... File src/soc/intel/broadwell/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... PS8, Line 46: !pei_data->saved_data
This check doesn't look right. This should be: `if (pei_data->saved_data)` i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/44006/8/src/soc/intel/broadwell/rom... PS8, Line 50: assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
Same comment as other files.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 9: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@24 PS8, Line 24: The functions : * return < 0 on error, 0 on success. Instead of dropping this, can we move it to the comment on line 30 since it still applies to mrc_cache_load_current() and line 43 as it applies to mrc_cache_stash_data() as well.
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@43 PS8, Line 43: data_size
I thought that we had agreed to return the void * in the past? Furquan, do you not agree with this […]
I think this is fine. Makes sense to keep it consistent with the underlying calls.
Hello build bot (Jenkins), Furquan Shaikh, Damien Zammit, Lee Leahy, Tim Wawrzynczak, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#10).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
Additionally, we are replacing mrc_cache_latest with mrc_cache_get_latest_slot_info, which does not check the validity of the data when retrieving the current mrc_cache slot. This allows the caller some flexibility in deciding where they want the mrc_cache data stored (either in an mmaped region or at a given address).
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 195 insertions(+), 127 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/10
Hello build bot (Jenkins), Furquan Shaikh, Damien Zammit, Lee Leahy, Tim Wawrzynczak, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#11).
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
Additionally, we are replacing mrc_cache_latest with mrc_cache_get_latest_slot_info, which does not check the validity of the data when retrieving the current mrc_cache slot. This allows the caller some flexibility in deciding where they want the mrc_cache data stored (either in an mmaped region or at a given address).
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 198 insertions(+), 127 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/11
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... File src/drivers/intel/fsp1_1/romstage.c:
https://review.coreboot.org/c/coreboot/+/44006/7/src/drivers/intel/fsp1_1/ro... PS7, Line 292: __weak void *mrc_cache_current_mmap_leak(int type, uint32_t version,
If you remove it and Jenkins doesn't complain, then we aren't using it.
Done
Okay, let's see if Jenkins complains.
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h File src/include/mrc_cache.h:
https://review.coreboot.org/c/coreboot/+/44006/8/src/include/mrc_cache.h@24 PS8, Line 24: The functions : * return < 0 on error, 0 on success.
Instead of dropping this, can we move it to the comment on line 30 since it still applies to mrc_cac […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 11: Code-Review+2
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 338: BIOS_ERR
In mrc_data_valid(), we originally pasted this error
Ack. Changed to BIOS_INFO
https://review.coreboot.org/c/coreboot/+/44006/8/src/drivers/mrc_cache/mrc_c... PS8, Line 338: on data verification
Sorry, copy/pasted the error message from mrc_data_valid() in the old driver.
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44006/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44006/11//COMMIT_MSG@7 PS11, Line 7: mrc_cache: mrc_cache fetch functions to support non-x86 platforms Please make it a statement by adding a verb (in imperative mood).
Hello build bot (Jenkins), Furquan Shaikh, Damien Zammit, Lee Leahy, Tim Wawrzynczak, Julius Werner, Angel Pons, Huang Jin, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#12).
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
Additionally, we are replacing mrc_cache_latest with mrc_cache_get_latest_slot_info, which does not check the validity of the data when retrieving the current mrc_cache slot. This allows the caller some flexibility in deciding where they want the mrc_cache data stored (either in an mmaped region or at a given address).
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 198 insertions(+), 127 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/12
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44006/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44006/11//COMMIT_MSG@7 PS11, Line 7: mrc_cache: mrc_cache fetch functions to support non-x86 platforms
Please make it a statement by adding a verb (in imperative mood).
Done
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 12: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 12: Code-Review+2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 12: Code-Review+1
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 12:
This needs to be rebased with merge conflict resolution
Hello build bot (Jenkins), Furquan Shaikh, Damien Zammit, Lee Leahy, Tim Wawrzynczak, Angel Pons, Julius Werner, Huang Jin, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#13).
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
Additionally, we are replacing mrc_cache_latest with mrc_cache_get_latest_slot_info, which does not check the validity of the data when retrieving the current mrc_cache slot. This allows the caller some flexibility in deciding where they want the mrc_cache data stored (either in an mmaped region or at a given address).
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 198 insertions(+), 126 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/13
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 13:
Patch Set 12:
This needs to be rebased with merge conflict resolution
I rebased and resolved the conflict. Thanks.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44006/13/src/soc/intel/baytrail/rom... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/13/src/soc/intel/baytrail/rom... PS13, Line 125: struct region_device rdev; I think this needs to be dropped as well?
Hello build bot (Jenkins), Furquan Shaikh, Damien Zammit, Lee Leahy, Tim Wawrzynczak, Angel Pons, Julius Werner, Huang Jin, Andrey Petrov, Yu-Ping Wu, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44006
to look at the new patch set (#14).
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
Additionally, we are replacing mrc_cache_latest with mrc_cache_get_latest_slot_info, which does not check the validity of the data when retrieving the current mrc_cache slot. This allows the caller some flexibility in deciding where they want the mrc_cache data stored (either in an mmaped region or at a given address).
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 198 insertions(+), 127 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/44006/14
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 14: Code-Review+2
Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44006/13/src/soc/intel/baytrail/rom... File src/soc/intel/baytrail/romstage/raminit.c:
https://review.coreboot.org/c/coreboot/+/44006/13/src/soc/intel/baytrail/rom... PS13, Line 125: struct region_device rdev;
I think this needs to be dropped as well?
Done. Thanks for catching that.
Shelley Chen has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44006 )
Change subject: mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms ......................................................................
mrc_cache: Add mrc_cache fetch functions to support non-x86 platforms
Create two new functions to fetch mrc_cache data (replacing mrc_cache_get_current):
- mrc_cache_load_current: fetches the mrc_cache data and drops it into the given buffer. This is useful for ARM platforms where the mmap operation is very expensive.
- mrc_cache_mmap_leak: fetch the mrc_cache data and puts it into a given buffer. This is useful for platforms where the mmap operation is a no-op (like x86 platforms). As the name mentions, we are not freeing the memory that we allocated with the mmap, so it is the caller's responsibility to do so.
Additionally, we are replacing mrc_cache_latest with mrc_cache_get_latest_slot_info, which does not check the validity of the data when retrieving the current mrc_cache slot. This allows the caller some flexibility in deciding where they want the mrc_cache data stored (either in an mmaped region or at a given address).
BUG=b:150502246 BRANCH=None TEST=Testing on a nami (x86) device: reboot from ec console. Make sure memory training happens. reboot from ec console. Make sure that we don't do training again.
Signed-off-by: Shelley Chen shchen@google.com Change-Id: I259dd4f550719d821bbafa2d445cbae6ea22e988 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44006 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/fsp1_1/romstage.c M src/drivers/intel/fsp2_0/memory_init.c M src/drivers/mrc_cache/mrc_cache.c M src/include/mrc_cache.h M src/northbridge/intel/haswell/raminit.c M src/northbridge/intel/ironlake/raminit.c M src/northbridge/intel/sandybridge/raminit.c M src/northbridge/intel/sandybridge/raminit_mrc.c M src/northbridge/intel/x4x/raminit.c M src/soc/amd/common/block/s3/s3_resume.c M src/soc/intel/apollolake/romstage.c M src/soc/intel/baytrail/romstage/raminit.c M src/soc/intel/broadwell/romstage/raminit.c 13 files changed, 198 insertions(+), 127 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/intel/fsp1_1/romstage.c b/src/drivers/intel/fsp1_1/romstage.c index 46df1c8..5a59c50 100644 --- a/src/drivers/intel/fsp1_1/romstage.c +++ b/src/drivers/intel/fsp1_1/romstage.c @@ -23,7 +23,7 @@ static void raminit_common(struct romstage_params *params) { bool s3wake; - struct region_device rdev; + size_t mrc_size;
post_code(0x32);
@@ -45,24 +45,31 @@ /* Recovery mode does not use MRC cache */ printk(BIOS_DEBUG, "Recovery mode: not using MRC cache.\n"); - } else if (CONFIG(CACHE_MRC_SETTINGS) - && (!mrc_cache_get_current(MRC_TRAINING_DATA, - params->fsp_version, - &rdev))) { - /* MRC cache found */ - params->saved_data_size = region_device_sz(&rdev); - params->saved_data = rdev_mmap_full(&rdev); + } else { /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - } else if (s3wake) { - /* Waking from S3 and no cache. */ - printk(BIOS_DEBUG, - "No MRC cache found in S3 resume path.\n"); - post_code(POST_RESUME_FAILURE); - /* FIXME: A "system" reset is likely enough: */ - full_reset(); - } else { - printk(BIOS_DEBUG, "No MRC cache found.\n"); + + params->saved_data = NULL; + if (CONFIG(CACHE_MRC_SETTINGS)) + params->saved_data = + mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + params->fsp_version, + &mrc_size); + if (params->saved_data) { + /* MRC cache found */ + params->saved_data_size = mrc_size; + + } else if (s3wake) { + /* Waking from S3 and no cache. */ + printk(BIOS_DEBUG, + "No MRC cache " + "found in S3 resume path.\n"); + post_code(POST_RESUME_FAILURE); + /* FIXME: A "system" reset is likely enough: */ + full_reset(); + } else { + printk(BIOS_DEBUG, "No MRC cache found.\n"); + } } }
@@ -283,13 +290,6 @@ { }
-/* Get the memory configuration data */ -__weak int mrc_cache_get_current(int type, uint32_t version, - struct region_device *rdev) -{ - return -1; -} - /* Save the memory configuration data */ __weak int mrc_cache_stash_data(int type, uint32_t version, const void *data, size_t size) diff --git a/src/drivers/intel/fsp2_0/memory_init.c b/src/drivers/intel/fsp2_0/memory_init.c index 57a0520..07c4463 100644 --- a/src/drivers/intel/fsp2_0/memory_init.c +++ b/src/drivers/intel/fsp2_0/memory_init.c @@ -93,8 +93,8 @@
static void fsp_fill_mrc_cache(FSPM_ARCH_UPD *arch_upd, uint32_t fsp_version) { - struct region_device rdev; void *data; + size_t mrc_size;
arch_upd->NvsBufferPtr = NULL;
@@ -113,25 +113,22 @@ return; }
- if (mrc_cache_get_current(MRC_TRAINING_DATA, fsp_version, &rdev) < 0) - return; - /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - data = rdev_mmap_full(&rdev);
+ data = mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, fsp_version, + &mrc_size); if (data == NULL) return;
if (CONFIG(FSP2_0_USES_TPM_MRC_HASH) && - !mrc_cache_verify_hash(data, region_device_sz(&rdev))) + !mrc_cache_verify_hash(data, mrc_size)) return;
/* MRC cache found */ arch_upd->NvsBufferPtr = data;
- printk(BIOS_SPEW, "MRC cache found, size %zx\n", - region_device_sz(&rdev)); + printk(BIOS_SPEW, "MRC cache found, size %zx\n", mrc_size); }
static enum cb_err check_region_overlap(const struct memranges *ranges, diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c index d567a20..1b1ad63 100644 --- a/src/drivers/mrc_cache/mrc_cache.c +++ b/src/drivers/mrc_cache/mrc_cache.c @@ -204,23 +204,16 @@ return 0; }
-static int mrc_data_valid(const struct region_device *rdev, - const struct mrc_metadata *md) +static int mrc_data_valid(const struct mrc_metadata *md, + void *data, size_t data_size) { - void *data; uint16_t checksum; - const size_t md_size = sizeof(*md); - const size_t data_size = md->data_size;
- data = rdev_mmap(rdev, md_size, data_size); - if (data == NULL) { - printk(BIOS_ERR, "MRC: mmap failure on data verification.\n"); + if (md->data_size != data_size) return -1; - }
checksum = compute_ip_checksum(data, data_size);
- rdev_munmap(rdev, data); if (md->data_checksum != checksum) { printk(BIOS_ERR, "MRC: data checksum mismatch: %x vs %x\n", md->data_checksum, checksum); @@ -230,7 +223,7 @@ return 0; }
-static int mrc_cache_latest(const char *name, +static int mrc_cache_get_latest_slot_info(const char *name, const struct region_device *backing_rdev, struct mrc_metadata *md, struct region_file *cache_file, @@ -260,25 +253,19 @@ return fail_bad_data ? -1 : 0; }
- /* Validate Data */ - if (mrc_data_valid(rdev, md) < 0) { - printk(BIOS_ERR, "MRC: invalid data in '%s'\n", name); - return fail_bad_data ? -1 : 0; - } - return 0; }
-int mrc_cache_get_current(int type, uint32_t version, - struct region_device *rdev) +static int mrc_cache_find_current(int type, uint32_t version, + struct region_device *rdev, + struct mrc_metadata *md) { const struct cache_region *cr; struct region region; struct region_device read_rdev; struct region_file cache_file; - struct mrc_metadata md; size_t data_size; - const size_t md_size = sizeof(md); + const size_t md_size = sizeof(*md); const bool fail_bad_data = true;
cr = lookup_region(®ion, type); @@ -289,21 +276,75 @@ if (boot_device_ro_subregion(®ion, &read_rdev) < 0) return -1;
- if (mrc_cache_latest(cr->name, &read_rdev, &md, &cache_file, rdev, - fail_bad_data) < 0) + if (mrc_cache_get_latest_slot_info(cr->name, + &read_rdev, + md, + &cache_file, + rdev, + fail_bad_data) < 0) return -1;
- if (version != md.version) { + if (version != md->version) { printk(BIOS_INFO, "MRC: version mismatch: %x vs %x\n", - md.version, version); + md->version, version); return -1; }
/* Re-size rdev to only contain the data. i.e. remove metadata. */ - data_size = md.data_size; + data_size = md->data_size; return rdev_chain(rdev, rdev, md_size, data_size); }
+int mrc_cache_load_current(int type, uint32_t version, void *buffer, + size_t buffer_size) +{ + struct region_device rdev; + struct mrc_metadata md; + size_t data_size; + + if (mrc_cache_find_current(type, version, &rdev, &md) < 0) + return -1; + + data_size = region_device_sz(&rdev); + if (buffer_size < data_size) + return -1; + + if (rdev_readat(&rdev, buffer, 0, data_size) != data_size) + return -1; + + if (mrc_data_valid(&md, buffer, data_size) < 0) + return -1; + + return 0; +} + +void *mrc_cache_current_mmap_leak(int type, uint32_t version, + size_t *data_size) +{ + struct region_device rdev; + void *data; + size_t region_device_size; + struct mrc_metadata md; + + if (mrc_cache_find_current(type, version, &rdev, &md) < 0) + return NULL; + + region_device_size = region_device_sz(&rdev); + if (data_size) + *data_size = region_device_size; + data = rdev_mmap_full(&rdev); + + if (data == NULL) { + printk(BIOS_INFO, "MRC: mmap failure.\n"); + return NULL; + } + + if (mrc_data_valid(&md, data, region_device_size) < 0) + return NULL; + + return data; +} + static bool mrc_cache_needs_update(const struct region_device *rdev, const struct cbmem_entry *to_be_updated) { @@ -392,8 +433,17 @@ if (backing_rdev == NULL) return;
- if (mrc_cache_latest(cr->name, backing_rdev, &md, &cache_file, - &latest_rdev, fail_bad_data) < 0) + /* Note that mrc_cache_get_latest_slot_info doesn't check the + * validity of the current slot. If the slot is invalid, + * we'll overwrite it anyway when we update the mrc_cache. + */ + if (mrc_cache_get_latest_slot_info(cr->name, + backing_rdev, + &md, + &cache_file, + &latest_rdev, + fail_bad_data) < 0) + return;
if (!mrc_cache_needs_update(&latest_rdev, to_be_updated)) { diff --git a/src/include/mrc_cache.h b/src/include/mrc_cache.h index 1cefba9..da2bf79 100644 --- a/src/include/mrc_cache.h +++ b/src/include/mrc_cache.h @@ -21,11 +21,30 @@ * policy don't request the data. */
-/* Get and stash data for saving provided the type passed in. The functions - * return < 0 on error, 0 on success. */ -int mrc_cache_get_current(int type, uint32_t version, - struct region_device *rdev); +/* Get and stash data for saving provided the type passed in. */ + +/** + * mrc_cache_load_current + * + * Fill in the buffer with the latest slot data. This will be a + * common entry point for ARM platforms. Returns < 0 on error, 0 on + * success. + */ +int mrc_cache_load_current(int type, uint32_t version, void *buffer, + size_t buffer_size); +/** + * mrc_cache_mmap_leak + * + * Return a pointer to a buffer with the latest slot data. An mmap + * will be executed (without a matching unmap). This will be a common + * entry point for platforms where mmap is considered a noop, like x86 + */ +void *mrc_cache_current_mmap_leak(int type, uint32_t version, + size_t *data_size); +/** + * Returns < 0 on error, 0 on success. + */ int mrc_cache_stash_data(int type, uint32_t version, const void *data, - size_t size); + size_t size);
#endif /* _COMMON_MRC_CACHE_H_ */ diff --git a/src/northbridge/intel/haswell/raminit.c b/src/northbridge/intel/haswell/raminit.c index 0b59692..9c6c00f 100644 --- a/src/northbridge/intel/haswell/raminit.c +++ b/src/northbridge/intel/haswell/raminit.c @@ -31,21 +31,24 @@
static void prepare_mrc_cache(struct pei_data *pei_data) { - struct region_device rdev; + size_t mrc_size;
/* Preset just in case there is an error */ pei_data->mrc_input = NULL; pei_data->mrc_input_len = 0;
- if (mrc_cache_get_current(MRC_TRAINING_DATA, MRC_CACHE_VERSION, &rdev)) + pei_data->mrc_input = + mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + MRC_CACHE_VERSION, + &mrc_size); + if (!pei_data->mrc_input) /* Error message printed in find_current_mrc_cache */ return;
- pei_data->mrc_input = rdev_mmap_full(&rdev); - pei_data->mrc_input_len = region_device_sz(&rdev); + pei_data->mrc_input_len = mrc_size;
- printk(BIOS_DEBUG, "%s: at %p, size %x\n", __func__, pei_data->mrc_input, - pei_data->mrc_input_len); + printk(BIOS_DEBUG, "%s: at %p, size %zx\n", __func__, + pei_data->mrc_input, mrc_size); }
static const char *ecc_decoder[] = { diff --git a/src/northbridge/intel/ironlake/raminit.c b/src/northbridge/intel/ironlake/raminit.c index dd1dbd0..81ba450 100644 --- a/src/northbridge/intel/ironlake/raminit.c +++ b/src/northbridge/intel/ironlake/raminit.c @@ -1618,11 +1618,9 @@
static const struct ram_training *get_cached_training(void) { - struct region_device rdev; - if (mrc_cache_get_current(MRC_TRAINING_DATA, MRC_CACHE_VERSION, - &rdev)) - return 0; - return (void *)rdev_mmap_full(&rdev); + return mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + MRC_CACHE_VERSION, + NULL); }
/* FIXME: add timeout. */ diff --git a/src/northbridge/intel/sandybridge/raminit.c b/src/northbridge/intel/sandybridge/raminit.c index 06b4d1e..6d0e845 100644 --- a/src/northbridge/intel/sandybridge/raminit.c +++ b/src/northbridge/intel/sandybridge/raminit.c @@ -297,7 +297,7 @@ int me_uma_size, cbmem_was_inited, fast_boot, err; ramctr_timing ctrl; spd_raw_data spds[4]; - struct region_device rdev; + size_t mrc_size; ramctr_timing *ctrl_cached = NULL;
MCHBAR32(SAPMCTL) |= 1; @@ -324,10 +324,11 @@ early_thermal_init();
/* Try to find timings in MRC cache */ - err = mrc_cache_get_current(MRC_TRAINING_DATA, MRC_CACHE_VERSION, &rdev); - - if (!err && !(region_device_sz(&rdev) < sizeof(ctrl))) - ctrl_cached = rdev_mmap_full(&rdev); + ctrl_cached = mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + MRC_CACHE_VERSION, + &mrc_size); + if (mrc_size < sizeof(ctrl)) + ctrl_cached = NULL;
/* Before reusing training data, assert that the CPU has not been replaced */ if (ctrl_cached && cpuid != ctrl_cached->cpu) { diff --git a/src/northbridge/intel/sandybridge/raminit_mrc.c b/src/northbridge/intel/sandybridge/raminit_mrc.c index b6b3989..5e5cc63 100644 --- a/src/northbridge/intel/sandybridge/raminit_mrc.c +++ b/src/northbridge/intel/sandybridge/raminit_mrc.c @@ -72,8 +72,8 @@
static void prepare_mrc_cache(struct pei_data *pei_data) { - struct region_device rdev; u16 c1, c2, checksum, seed_checksum; + size_t mrc_size;
/* Preset just in case there is an error */ pei_data->mrc_input = NULL; @@ -103,16 +103,18 @@ return; }
- if (mrc_cache_get_current(MRC_TRAINING_DATA, MRC_CACHE_VERSION, &rdev)) { + pei_data->mrc_input = mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + MRC_CACHE_VERSION, + &mrc_size); + if (pei_data->mrc_input == NULL) { /* Error message printed in find_current_mrc_cache */ return; }
- pei_data->mrc_input = rdev_mmap_full(&rdev); - pei_data->mrc_input_len = region_device_sz(&rdev); + pei_data->mrc_input_len = mrc_size;
- printk(BIOS_DEBUG, "%s: at %p, size %x\n", __func__, pei_data->mrc_input, - pei_data->mrc_input_len); + printk(BIOS_DEBUG, "%s: at %p, size %zx\n", __func__, + pei_data->mrc_input, mrc_size); }
/** diff --git a/src/northbridge/intel/x4x/raminit.c b/src/northbridge/intel/x4x/raminit.c index 9f361b6..a62771d 100644 --- a/src/northbridge/intel/x4x/raminit.c +++ b/src/northbridge/intel/x4x/raminit.c @@ -610,8 +610,8 @@ { struct sysinfo s, *ctrl_cached; u8 reg8; - int fast_boot, cbmem_was_inited, cache_not_found; - struct region_device rdev; + int fast_boot, cbmem_was_inited; + size_t mrc_size;
timestamp_add_now(TS_BEFORE_INITRAM); printk(BIOS_DEBUG, "Setting up RAM controller.\n"); @@ -620,10 +620,11 @@
memset(&s, 0, sizeof(struct sysinfo));
- cache_not_found = mrc_cache_get_current(MRC_TRAINING_DATA, - MRC_CACHE_VERSION, &rdev); + ctrl_cached = mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + MRC_CACHE_VERSION, + &mrc_size);
- if (cache_not_found || (region_device_sz(&rdev) < sizeof(s))) { + if (!ctrl_cached || mrc_size < sizeof(s)) { if (boot_path == BOOT_PATH_RESUME) { /* Failed S3 resume, reset to come up cleanly */ system_reset(); @@ -632,9 +633,6 @@ and therefore requiring valid cached settings */ full_reset(); } - ctrl_cached = NULL; - } else { - ctrl_cached = rdev_mmap_full(&rdev); }
/* verify MRC cache for fast boot */ diff --git a/src/soc/amd/common/block/s3/s3_resume.c b/src/soc/amd/common/block/s3/s3_resume.c index 3752384..2094931 100644 --- a/src/soc/amd/common/block/s3/s3_resume.c +++ b/src/soc/amd/common/block/s3/s3_resume.c @@ -25,14 +25,10 @@ size_t size; int i; uint32_t erased = 0xffffffff; - struct region_device rdev;
- if (mrc_cache_get_current(MRC_TRAINING_DATA, DEFAULT_MRC_VERSION, - &rdev)) - reboot_from_resume("mrc_cache_get_current error, rebooting.\n"); - - base = rdev_mmap_full(&rdev); - size = region_device_sz(&rdev); + base = mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + DEFAULT_MRC_VERSION, + &size); if (!base || !size) reboot_from_resume("Error: S3 NV data not found, rebooting.\n");
diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 54bdd2e..48ae9a7 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -267,8 +267,6 @@
void platform_fsp_memory_init_params_cb(FSPM_UPD *mupd, uint32_t version) { - struct region_device rdev; - check_full_retrain(mupd);
fill_console_params(mupd); @@ -310,11 +308,11 @@ * wrong/missing key renders DRAM contents useless. */
- if (mrc_cache_get_current(MRC_VARIABLE_DATA, version, &rdev) == 0) { - /* Assume leaking is ok. */ - assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - mupd->FspmConfig.VariableNvsBufferPtr = rdev_mmap_full(&rdev); - } + mupd->FspmConfig.VariableNvsBufferPtr = + mrc_cache_current_mmap_leak(MRC_VARIABLE_DATA, version, + NULL); + + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED));
fsp_version = version;
diff --git a/src/soc/intel/baytrail/romstage/raminit.c b/src/soc/intel/baytrail/romstage/raminit.c index 6ff6c03..9a67c22 100644 --- a/src/soc/intel/baytrail/romstage/raminit.c +++ b/src/soc/intel/baytrail/romstage/raminit.c @@ -122,8 +122,8 @@ { int ret; mrc_wrapper_entry_t mrc_entry; - struct region_device rdev; size_t i; + size_t mrc_size;
/* Fill in default entries. */ mp->version = MRC_PARAMS_VER; @@ -135,11 +135,14 @@ if (!mp->io_hole_mb) mp->io_hole_mb = 2048;
- if (!mrc_cache_get_current(MRC_TRAINING_DATA, 0, &rdev)) { - mp->saved_data_size = region_device_sz(&rdev); - mp->saved_data = rdev_mmap_full(&rdev); - /* Assume boot device is memory mapped. */ - assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + /* Assume boot device is memory mapped. */ + assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); + + mp->saved_data = mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, + 0, + &mrc_size); + if (mp->saved_data) { + mp->saved_data_size = mrc_size; } else if (prev_sleep_state == ACPI_S3) { /* If waking from S3 and no cache then. */ printk(BIOS_DEBUG, "No MRC cache found in S3 resume path.\n"); diff --git a/src/soc/intel/broadwell/romstage/raminit.c b/src/soc/intel/broadwell/romstage/raminit.c index 4165c67..0580ca6 100644 --- a/src/soc/intel/broadwell/romstage/raminit.c +++ b/src/soc/intel/broadwell/romstage/raminit.c @@ -26,7 +26,7 @@ */ void raminit(struct pei_data *pei_data) { - struct region_device rdev; + size_t mrc_size; struct memory_info *mem_info; pei_wrapper_entry_t entry; int ret; @@ -39,19 +39,25 @@ vboot_recovery_mode_enabled()) { /* Recovery mode does not use MRC cache */ printk(BIOS_DEBUG, "Recovery mode: not using MRC cache.\n"); - } else if (!mrc_cache_get_current(MRC_TRAINING_DATA, 0, &rdev)) { - /* MRC cache found */ - pei_data->saved_data_size = region_device_sz(&rdev); - pei_data->saved_data = rdev_mmap_full(&rdev); + } else { /* Assume boot device is memory mapped. */ assert(CONFIG(BOOT_DEVICE_MEMORY_MAPPED)); - } else if (pei_data->boot_mode == ACPI_S3) { - /* Waking from S3 and no cache. */ - printk(BIOS_DEBUG, "No MRC cache found in S3 resume path.\n"); - post_code(POST_RESUME_FAILURE); - system_reset(); - } else { - printk(BIOS_DEBUG, "No MRC cache found.\n"); + + pei_data->saved_data = + mrc_cache_current_mmap_leak(MRC_TRAINING_DATA, 0, + &mrc_size); + if (pei_data->saved_data) { + /* MRC cache found */ + pei_data->saved_data_size = mrc_size; + } else if (pei_data->boot_mode == ACPI_S3) { + /* Waking from S3 and no cache. */ + printk(BIOS_DEBUG, + "No MRC cache found in S3 resume path.\n"); + post_code(POST_RESUME_FAILURE); + system_reset(); + } else { + printk(BIOS_DEBUG, "No MRC cache found.\n"); + } }
/*
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Shelley Chen (Code Review)