GNUBoot
/
coreboot-kgpe-d16
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

drivers/mrc_cache: Compare hashes instead of full data 

The current MRC cache update process is slow (28 ms on nissa), because
cbmem is not cached in romstage. Specifically, the new MRC data returned
by the FSP is stored in the FSP reserved memory in cbmem, so operations
on the new data (computing the checksum, comparing to the old data) are
slow.

Replace the data checksum in the MRC header with a hash, and compare
hashes instead of comparing the full data. This has two benefits:
1. The xxhash function is faster than computing an IP checksum (4 ms vs
   14 ms on uncached data on nissa).
2. There's no need to memcmp() the full MRC data, which takes 14 ms on
   nissa.

Before:
550:starting to load ChromeOS VPD                     867,930 (4,664)
  3:after RAM initialization                          896,020 (28,090)
  4:end of romstage                                   906,274 (10,254)

After:
550:starting to load ChromeOS VPD                     864,820 (4,649)
  3:after RAM initialization                          869,652 (4,831)
  4:end of romstage                                   879,909 (10,257)

BUG=b:242667207
TEST=Check that MRC caching still works as expected on nissa. Corrupt
the MRC cache and check that memory is retrained.

Change-Id: I1b7848d1d05e555b61e0f1cb605550dfe3449c6d
Signed-off-by: Reka Norman <rekanorman@chromium.org>
Reviewed-on: https://review.coreboot.org/c/coreboot/+/67670
Reviewed-by: Julius Werner <jwerner@chromium.org>
Reviewed-by: Tim Wawrzynczak <twawrzynczak@chromium.org>
Reviewed-by: Eric Lai <eric_lai@quanta.corp-partner.google.com>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
This commit is contained in:
Reka Norman 2022-09-16 10:39:49 +10:00 committed by Felix Held
parent 1df1cf994a
commit ee63b44c47
1 changed files with 27 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

59
src/drivers/mrc_cache/mrc_cache.c
View File

@ -8,12 +8,12 @@
#include <cbmem.h> #include <cbmem.h>
#include <elog.h> #include <elog.h>
#include <fmap.h> #include <fmap.h>
#include <ip_checksum.h>
#include <region_file.h> #include <region_file.h>
#include <security/vboot/antirollback.h> #include <security/vboot/antirollback.h>
#include <security/vboot/mrc_cache_hash_tpm.h> #include <security/vboot/mrc_cache_hash_tpm.h>
#include <security/vboot/vboot_common.h> #include <security/vboot/vboot_common.h>
#include <spi_flash.h> #include <spi_flash.h>
#include <xxhash.h>
#include "mrc_cache.h" #include "mrc_cache.h"
@ -27,8 +27,8 @@
struct mrc_metadata { struct mrc_metadata {
uint32_t signature; uint32_t signature;
uint32_t data_size; uint32_t data_size;
uint16_t data_checksum; uint32_t data_hash;
uint16_t header_checksum; uint32_t header_hash;
uint32_t version; uint32_t version;
} __packed; } __packed;
@ -166,8 +166,8 @@ static const struct cache_region *lookup_region(struct region *r, int type)
static int mrc_header_valid(struct region_device *rdev, struct mrc_metadata *md) static int mrc_header_valid(struct region_device *rdev, struct mrc_metadata *md)
{ {
uint16_t checksum; uint32_t hash;
uint16_t checksum_result; uint32_t hash_result;
size_t size; size_t size;
if (rdev_readat(rdev, md, 0, sizeof(*md)) < 0) { if (rdev_readat(rdev, md, 0, sizeof(*md)) < 0) {
@ -180,19 +180,19 @@ static int mrc_header_valid(struct region_device *rdev, struct mrc_metadata *md)
return -1; return -1;
} }
/* Compute checksum over header with 0 as the value. */ /* Compute hash over header with 0 as the value. */
checksum = md->header_checksum; hash = md->header_hash;
md->header_checksum = 0; md->header_hash = 0;
checksum_result = compute_ip_checksum(md, sizeof(*md)); hash_result = xxh32(md, sizeof(*md), 0);
if (checksum != checksum_result) { if (hash != hash_result) {
printk(BIOS_ERR, "MRC: header checksum mismatch: %x vs %x\n", printk(BIOS_ERR, "MRC: header hash mismatch: %x vs %x\n",
checksum, checksum_result); hash, hash_result);
return -1; return -1;
} }
/* Put back original. */ /* Put back original. */
md->header_checksum = checksum; md->header_hash = hash;
/* Re-size the region device according to the metadata as a region_file /* Re-size the region device according to the metadata as a region_file
* does block allocation. */ * does block allocation. */
@ -209,7 +209,7 @@ static int mrc_header_valid(struct region_device *rdev, struct mrc_metadata *md)
static int mrc_data_valid(int type, const struct mrc_metadata *md, static int mrc_data_valid(int type, const struct mrc_metadata *md,
void *data, size_t data_size) void *data, size_t data_size)
{ {
uint16_t checksum; uint32_t hash;
const struct cache_region *cr = lookup_region_type(type); const struct cache_region *cr = lookup_region_type(type);
uint32_t hash_idx; uint32_t hash_idx;
@ -224,11 +224,11 @@ static int mrc_data_valid(int type, const struct mrc_metadata *md,
if (!mrc_cache_verify_hash(hash_idx, data, data_size)) if (!mrc_cache_verify_hash(hash_idx, data, data_size))
return -1; return -1;
} else { } else {
checksum = compute_ip_checksum(data, data_size); hash = xxh32(data, data_size, 0);
if (md->data_checksum != checksum) { if (md->data_hash != hash) {
printk(BIOS_ERR, "MRC: data checksum mismatch: %x vs %x\n", printk(BIOS_ERR, "MRC: data hash mismatch: %x vs %x\n",
md->data_checksum, checksum); md->data_hash, hash);
return -1; return -1;
} }
} }
@ -368,9 +368,9 @@ void *mrc_cache_current_mmap_leak(int type, uint32_t version,
static bool mrc_cache_needs_update(const struct region_device *rdev, static bool mrc_cache_needs_update(const struct region_device *rdev,
const struct mrc_metadata *new_md, const struct mrc_metadata *new_md,
const void *new_data, size_t new_data_size) size_t new_data_size)
{ {
void *mapping, *data_mapping; void *mapping;
size_t old_data_size = region_device_sz(rdev) - sizeof(struct mrc_metadata); size_t old_data_size = region_device_sz(rdev) - sizeof(struct mrc_metadata);
bool need_update = false; bool need_update = false;
@ -382,17 +382,14 @@ static bool mrc_cache_needs_update(const struct region_device *rdev,
printk(BIOS_ERR, "MRC: cannot mmap existing cache.\n"); printk(BIOS_ERR, "MRC: cannot mmap existing cache.\n");
return true; return true;
} }
data_mapping = mapping + sizeof(struct mrc_metadata);
/* we need to compare the md and the data separately */ /*
/* check the mrc_metadata */ * Compare the old and new metadata only. If the data hashes don't
* match, the comparison will fail.
*/
if (memcmp(new_md, mapping, sizeof(struct mrc_metadata))) if (memcmp(new_md, mapping, sizeof(struct mrc_metadata)))
need_update = true; need_update = true;
/* check the data */
if (!need_update && memcmp(new_data, data_mapping, new_data_size))
need_update = true;
rdev_munmap(rdev, mapping); rdev_munmap(rdev, mapping);
return need_update; return need_update;
@ -475,8 +472,7 @@ static void update_mrc_cache_by_type(int type,
return; return;
if (!mrc_cache_needs_update(&latest_rdev, if (!mrc_cache_needs_update(&latest_rdev, new_md, new_data_size)) {
new_md, new_data, new_data_size)) {
printk(BIOS_DEBUG, "MRC: '%s' does not need update.\n", cr->name); printk(BIOS_DEBUG, "MRC: '%s' does not need update.\n", cr->name);
log_event_cache_update(cr->elog_slot, ALREADY_UPTODATE); log_event_cache_update(cr->elog_slot, ALREADY_UPTODATE);
return; return;
@ -693,10 +689,9 @@ int mrc_cache_stash_data(int type, uint32_t version, const void *data,
.signature = MRC_DATA_SIGNATURE, .signature = MRC_DATA_SIGNATURE,
.data_size = size, .data_size = size,
.version = version, .version = version,
.data_checksum = compute_ip_checksum(data, size), .data_hash = xxh32(data, size, 0),
}; };
md.header_checksum = md.header_hash = xxh32(&md, sizeof(md), 0);
compute_ip_checksum(&md, sizeof(md));
if (CONFIG(MRC_STASH_TO_CBMEM)) { if (CONFIG(MRC_STASH_TO_CBMEM)) {
/* Store data in cbmem for use in ramstage */ /* Store data in cbmem for use in ramstage */