Change in coreboot[master]: mrc_cache: Update mrc_cache data in romstage - coreboot-gerrit

5 Aug 2020

Shelley Chen has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44196 )
Change subject: mrc_cache: Update mrc_cache data in romstage
......................................................................
mrc_cache: Update mrc_cache data in romstage
Previously, we were writing to cbmem after memory training and then
writing the training data from cbmem to mrc_cache in ramstage.  We
were doing this because we were unable to read/write to SPI
simultaneously on older SIP chips.  Now that newer chips allow for
simultaneously reads and writes, we can move the mrc_cache update into
romstage.
BUG=b:150502246
BRANCH=None
TEST=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: I3430bda45484cb8c2b01ab9614508039dfaac9a3
---
M src/drivers/elog/Makefile.inc
M src/drivers/mrc_cache/mrc_cache.c
M src/include/region_file.h
M src/lib/region_file.c
4 files changed, 137 insertions(+), 53 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/96/44196/1

diff --git a/src/drivers/elog/Makefile.inc b/src/drivers/elog/Makefile.inc
index cce1c3d..a1c14ff 100644
--- a/src/drivers/elog/Makefile.inc
+++ b/src/drivers/elog/Makefile.inc
@@ -1,6 +1,7 @@
 bootblock-$(CONFIG_ELOG_PRERAM) += elog.c
 verstage-$(CONFIG_ELOG_PRERAM) += elog.c
 romstage-$(CONFIG_ELOG_PRERAM) += elog.c
+romstage-$(CONFIG_ELOG) += elog.c
 postcar-$(CONFIG_ELOG_PRERAM) += elog.c
 ramstage-$(CONFIG_ELOG) += elog.c
diff --git a/src/drivers/mrc_cache/mrc_cache.c b/src/drivers/mrc_cache/mrc_cache.c
index 0e42120..2dc6ee8 100644
--- a/src/drivers/mrc_cache/mrc_cache.c
+++ b/src/drivers/mrc_cache/mrc_cache.c
@@ -311,18 +311,26 @@
 }
static bool mrc_cache_needs_update(const struct region_device *rdev,
-				const struct cbmem_entry *to_be_updated)
+				   struct mrc_metadata *new_md,
+				   const void *new_data, size_t new_data_size)
 {
-	void *mapping;
+	void *mapping, *data_mapping;
    size_t size = region_device_sz(rdev);
    bool need_update = false;
-	if (cbmem_entry_size(to_be_updated) != size)
+	if (new_data_size != size)
    	return true;
mapping = rdev_mmap_full(rdev);
+	data_mapping = mapping + sizeof(struct mrc_metadata);
-	if (memcmp(cbmem_entry_start(to_be_updated), mapping, size))
+	// we need to compare the md and the data separately
+	// check the mrc_metadata
+	if (memcmp(new_md, mapping, sizeof(struct mrc_metadata)))
+		need_update = true;
+
+	// check the data
+	if (memcmp(new_data, data_mapping, new_data_size))
    	need_update = true;
rdev_munmap(rdev, mapping);
@@ -357,7 +365,10 @@
  * read and write. The read assumes a memory-mapped boot device that can be used
  * to quickly locate and compare the up-to-date data. However, when an update
  * is required it uses the writeable region access to perform the update. */
-static void update_mrc_cache_by_type(int type)
+static void update_mrc_cache_by_type(int type,
+				     struct mrc_metadata *new_md,
+				     const void *new_data,
+				     size_t new_data_size)
 {
    const struct cache_region *cr;
    struct region region;
@@ -365,7 +376,6 @@
    struct region_device write_rdev;
    struct region_file cache_file;
    struct mrc_metadata md;
-	const struct cbmem_entry *to_be_updated;
    struct incoherent_rdev backing_irdev;
    const struct region_device *backing_rdev;
    struct region_device latest_rdev;
@@ -376,13 +386,6 @@
    if (cr == NULL)
    	return;
-	to_be_updated = cbmem_entry_find(cr->cbmem_id);
-	if (to_be_updated == NULL) {
-		printk(BIOS_ERR, "MRC: No data in cbmem for '%s'.\n",
-			cr->name);
-		return;
-	}
-
    printk(BIOS_DEBUG, "MRC: Checking cached data update for '%s'.\n",
    	cr->name);
@@ -411,8 +414,9 @@
return;
-	if (!mrc_cache_needs_update(&latest_rdev, to_be_updated)) {
-		printk(BIOS_DEBUG, "MRC: '%s' does not need update.\n", cr->name);
+	if (!mrc_cache_needs_update(&latest_rdev,
+				    new_md,
+				    new_data, new_data_size)) {
    	log_event_cache_update(cr->elog_slot, ALREADY_UPTODATE);
    	return;
    }
@@ -420,9 +424,10 @@
    printk(BIOS_DEBUG, "MRC: cache data '%s' needs update.\n", cr->name);
if (region_file_update_data(&cache_file,
-				cbmem_entry_start(to_be_updated),
-				cbmem_entry_size(to_be_updated)) < 0) {
-		printk(BIOS_DEBUG, "MRC: failed to update '%s'.\n", cr->name);
+				    new_md,
+				    sizeof(struct mrc_metadata),
+				    new_data,
+				    new_data_size) < 0) {
    	log_event_cache_update(cr->elog_slot, UPDATE_FAILURE);
    } else {
    	printk(BIOS_DEBUG, "MRC: updated '%s'.\n", cr->name);
@@ -544,16 +549,59 @@
/* Push an update that consists of 4 bytes that is smaller than the
     * MRC metadata as well as an invalid signature. */
-	if (region_file_update_data(&cache_file, &invalid, sizeof(invalid)) < 0)
+	if (region_file_update_data(&cache_file, NULL, 0,
+				    &invalid, sizeof(invalid)) < 0)
    	printk(BIOS_ERR, "MRC: invalidation failed for '%s'.\n", name);
 }
-static void update_mrc_cache(void *unused)
+static void update_mrc_cache(struct mrc_metadata *md,
+			     const void *data,
+			     size_t size)
 {
-	update_mrc_cache_by_type(MRC_TRAINING_DATA);
+	update_mrc_cache_by_type(MRC_TRAINING_DATA, md, data, size);
if (CONFIG(MRC_SETTINGS_VARIABLE_DATA))
-		update_mrc_cache_by_type(MRC_VARIABLE_DATA);
+		update_mrc_cache_by_type(MRC_VARIABLE_DATA,
+					 md, data, size);
+
+	if (CONFIG(MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN))
+		invalidate_normal_cache();
+
+	protect_mrc_region();
+}
+
+static void update_mrc_cache_from_cbmem(int type)
+{
+	const struct cache_region *cr;
+	struct region region;
+	struct mrc_metadata md;
+	const struct cbmem_entry *to_be_updated;
+
+	cr = lookup_region(&region, type);
+
+	if (cr == NULL)
+		return;
+
+	to_be_updated = cbmem_entry_find(cr->cbmem_id);
+
+	if (to_be_updated == NULL) {
+		printk(BIOS_ERR, "MRC: No data in cbmem for '%s'.\n",
+		       cr->name);
+		return;
+	}
+
+	update_mrc_cache(&md,
+			 cbmem_entry_start(to_be_updated),
+			 cbmem_entry_size(to_be_updated));
+}
+
+static void finalize_mrc_cache(void *unused)
+{
+	if (CONFIG(MRC_WRITE_NV_LATE))
+		update_mrc_cache_from_cbmem(MRC_TRAINING_DATA);
+
+	if (CONFIG(MRC_SETTINGS_VARIABLE_DATA))
+		update_mrc_cache_from_cbmem(MRC_VARIABLE_DATA);
if (CONFIG(MRC_CLEAR_NORMAL_CACHE_ON_RECOVERY_RETRAIN))
    	invalidate_normal_cache();
@@ -562,11 +610,9 @@
 }
int mrc_cache_stash_data(int type, uint32_t version, const void *data,
-			size_t size)
+			 size_t size)
 {
    const struct cache_region *cr;
-	size_t cbmem_size;
-	struct mrc_metadata *md;
cr = lookup_region_type(type);
    if (cr == NULL) {
@@ -575,24 +621,41 @@
    	return -1;
    }
-	cbmem_size = sizeof(*md) + size;
+	if (CONFIG(MRC_WRITE_NV_LATE)) {
+		/* Store data in cbmem for use in ramstage */
+		struct mrc_metadata *md;
+		size_t cbmem_size;
+		cbmem_size = sizeof(*md) + size;
-	md = cbmem_add(cr->cbmem_id, cbmem_size);
+		md = cbmem_add(cr->cbmem_id, cbmem_size);
-	if (md == NULL) {
-		printk(BIOS_ERR, "MRC: failed to add '%s' to cbmem.\n",
-			cr->name);
-		return -1;
+		if (md == NULL) {
+			printk(BIOS_ERR, "MRC: failed to add '%s' to cbmem.\n",
+			       cr->name);
+			return -1;
+		}
+
+		memset(md, 0, sizeof(*md));
+		md->signature = MRC_DATA_SIGNATURE;
+		md->data_size = size;
+		md->version = version;
+		md->data_checksum = compute_ip_checksum(data, size);
+		md->header_checksum = compute_ip_checksum(md, sizeof(*md));
+		memcpy(&md[1], data, size);
+	} else {
+		/* Otherwise store to mrc_cache right away */
+		struct mrc_metadata md;
+
+		memset(&md, 0, sizeof(struct mrc_metadata));
+		md.signature = MRC_DATA_SIGNATURE;
+		md.data_size = size;
+		md.version = version;
+		md.data_checksum = compute_ip_checksum(data, size);
+		md.header_checksum =
+			compute_ip_checksum(&md, sizeof(struct mrc_metadata));
+
+		update_mrc_cache(&md, data, size);
    }
-
-	memset(md, 0, sizeof(*md));
-	md->signature = MRC_DATA_SIGNATURE;
-	md->data_size = size;
-	md->version = version;
-	md->data_checksum = compute_ip_checksum(data, size);
-	md->header_checksum = compute_ip_checksum(md, sizeof(*md));
-	memcpy(&md[1], data, size);
-
    return 0;
 }
@@ -602,7 +665,7 @@
  */
#if CONFIG(MRC_WRITE_NV_LATE)
-BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_ENTRY, update_mrc_cache, NULL);
+BOOT_STATE_INIT_ENTRY(BS_OS_RESUME_CHECK, BS_ON_ENTRY, finalize_mrc_cache, NULL);
 #else
-BOOT_STATE_INIT_ENTRY(BS_DEV_ENUMERATE, BS_ON_EXIT, update_mrc_cache, NULL);
+BOOT_STATE_INIT_ENTRY(BS_DEV_ENUMERATE, BS_ON_EXIT, finalize_mrc_cache, NULL);
 #endif
diff --git a/src/include/region_file.h b/src/include/region_file.h
index 063e0e0..068e147 100644
--- a/src/include/region_file.h
+++ b/src/include/region_file.h
@@ -32,8 +32,11 @@
 int region_file_data(const struct region_file *f, struct region_device *rdev);
/* Update region file with latest data. Returns < 0 on error, 0 on success. */
-int region_file_update_data(struct region_file *f, const void *buf,
-				size_t size);
+int region_file_update_data(struct region_file *f,
+			    const void *metadata,
+			    size_t metadata_size,
+			    const void *data,
+			    size_t data_size);
/* Declared here for easy object allocation. */
 struct region_file {
diff --git a/src/lib/region_file.c b/src/lib/region_file.c
index ce2ed30..3f47b28 100644
--- a/src/lib/region_file.c
+++ b/src/lib/region_file.c
@@ -365,11 +365,17 @@
    return 0;
 }
-static int commit_data(const struct region_file *f, const void *buf,
-			size_t size)
+static int commit_data(const struct region_file *f,
+		       const void *metadata,
+		       size_t metadata_size,
+		       const void *data_buf,
+		       size_t data_size)
 {
    size_t offset = block_to_bytes(region_file_data_begin(f));
-	if (rdev_writeat(&f->rdev, buf, offset, size) < 0)
+	if (rdev_writeat(&f->rdev, metadata, offset, metadata_size) < 0)
+		return -1;
+	offset += metadata_size;
+	if (rdev_writeat(&f->rdev, data_buf, offset, data_size) < 0)
    	return -1;
    return 0;
 }
@@ -399,8 +405,12 @@
    return 0;
 }
-static int handle_update(struct region_file *f, size_t blocks, const void *buf,
-				size_t size)
+static int handle_update(struct region_file *f,
+			 size_t blocks,
+			 const void *metadata,
+			 size_t metadata_size,
+			 const void *data,
+			 size_t data_size)
 {
    if (!update_can_fit(f, blocks)) {
    	printk(BIOS_INFO, "REGF update can't fit. Will empty.\n");
@@ -413,7 +423,7 @@
    	return -1;
    }
-	if (commit_data(f, buf, size)) {
+	if (commit_data(f, metadata, metadata_size, data, data_size)) {
    	printk(BIOS_ERR, "REGF failed to commit data.\n");
    	return -1;
    }
@@ -421,12 +431,18 @@
    return 0;
 }
-int region_file_update_data(struct region_file *f, const void *buf, size_t size)
+int region_file_update_data(struct region_file *f,
+			    const void *metadata,
+			    size_t metadata_size,
+			    const void *data,
+			    size_t data_size)
 {
    int ret;
    size_t blocks;
-	blocks = bytes_to_block(ALIGN_UP(size, REGF_BLOCK_GRANULARITY));
+	blocks = bytes_to_block(ALIGN_UP(data_size +
+					 metadata_size,
+					 REGF_BLOCK_GRANULARITY));
while (1) {
    	int prev_slot = f->slot;
@@ -442,7 +458,8 @@
    		ret = -1;
    		break;
    	default:
-			ret = handle_update(f, blocks, buf, size);
+			ret = handle_update(f, blocks, metadata,
+					    metadata_size, data, data_size);
    		break;
    	}
-- 
To view, visit https://review.coreboot.org/c/coreboot/+/44196
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: I3430bda45484cb8c2b01ab9614508039dfaac9a3
Gerrit-Change-Number: 44196
Gerrit-PatchSet: 1
Gerrit-Owner: Shelley Chen shchen@google.com
Gerrit-Reviewer: Martin Roth martinroth@google.com
Gerrit-Reviewer: Patrick Georgi pgeorgi@google.com
Gerrit-MessageType: newchange



    