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.