Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44196 )
Change subject: mrc_cache: Update mrc_cache data in romstage ......................................................................
Patch Set 7:
(10 comments)
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/elog/Makefile.i... File src/drivers/elog/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/elog/Makefile.i... PS7, Line 4: romstage-$(CONFIG_ELOG) += elog.c No, if we do this there's no point in having an ELOG_PRERAM option. Either we choose to get rid of ELOG_PRERAM completely or the accesses done by the MRC code need to be guarded with that option.
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... PS7, Line 384: cr = lookup_region(®ion, type); nit: might be easier to just pass in *cr instead of type because all the callers already have that.
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... PS7, Line 562: update_mrc_cache_by_type(MRC_TRAINING_DATA, md, data, size); I don't think you're treating `type` quite right throughout this. In mrc_cache_stash_data(), you already have a `type` parameter, so that function already handles only a single type (and if there are multiple types, platform code will call that function more than once). That means the function called from that should also just handle a single type. In the same way, update_mrc_cache_from_cbmem() is already only dealing with a single type.
So, long story short, I think both of those functions should probably just call update_mrc_cache_by_type() directly and you should get rid of this function.
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... PS7, Line 569: invalidate_normal_cache(); This and the protect_mrc_region() should probably just go in finalize, then.
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... PS7, Line 589: printk(BIOS_ERR, "MRC: No data in cbmem for '%s'.\n", Should this really be an ERR... isn't this the most common case, where we just didn't need to retrain and thus are also not updating anything? (I know the existing code already does this but it feels like INFO would be more appropriate... maybe Furquan understands what happens here better?)
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... PS7, Line 605: update_mrc_cache_from_cbmem(MRC_VARIABLE_DATA); Pretty sure this should also only happen for NV_LATE.
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... PS7, Line 644: md->header_checksum = compute_ip_checksum(md, sizeof(*md)); Let's try to factor out some of the code duplication here. For example:
struct mrc_metadata md;
...fill `md` like in the else-path...
if (CONFIG(MRC_WRITE_NV_LATE)) { struct mrc_metadata *cbmem = cbmem_add(...); if (!cbmem) ... memcpy(cbmem, &md, sizeof(*cbmem)); memcpy(&cbmem[1], data, size); } else { update_mrc_cache(&md, data, size); }
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... PS7, Line 668: #if CONFIG(MRC_WRITE_NV_LATE) Is there still a reason to differentiate this or can we just always call finalize in RESUME_CHECK? (Sorry, I feel like this may have been discussed before but I forgot the result.)
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/spi/Kconfig File src/drivers/spi/Kconfig:
https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/spi/Kconfig@47 PS7, Line 47: default y if BOOT_DEVICE_SPI_FLASH_RW_NOMMAP && BOOT_DEVICE_SPI_FLASH If this is required to use the MRC cache code you probably also need to add a depends on to some MRC cache option. This is just a default, users are free to change it (and it shouldn't be possible for them to change options through menuconfig into a configuration that doesn't build).
https://review.coreboot.org/c/coreboot/+/44196/7/src/lib/region_file.c File src/lib/region_file.c:
https://review.coreboot.org/c/coreboot/+/44196/7/src/lib/region_file.c@438 PS7, Line 438: size_t data_size) This API seems to be designed to handle arbitrary files, so encoding details of the MRC use case here doesn't seem great. Although I'm not sure how to do it better either, and since this is only used for MRC maybe we also just don't care.
I guess the more "correct" and generic way of doing this would be to support arbitrary scatter-gather writes (basically make the function take an array of pointers and an array of sizes). Not sure how much we care.