Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46514 )
Change subject: mrc_cache: Add tpm_hash_index field to cache_region struct ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46514/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46514/2//COMMIT_MSG@9 PS2, Line 9: Pull selection of tpm hash index logic into cache_region struct. Can you please also add the detail that this commit is enabling the support for saving MRC hash in TPM for both recovery and non-recovery cases?
This is a difference in observed behavior on all platforms using TPM2 and MRC cache driver.
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/1/src/drivers/mrc_cache/mrc_c... PS1, Line 58: #if CONFIG(HAS_RECOVERY_MRC_CACHE) && CONFIG(MRC_SAVE_HASH_IN_TPM)
This should be unnecessary, you're already checking MRC_SAVE_HASH_IN_TPM in the code, and if HAS_REC […]
Yes, check for MRC_SAVE_HASH_IN_TPM is not required. But, I think you would still need HAS_RECOVERY_MRC_CACHE.
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... File src/drivers/mrc_cache/mrc_cache.c:
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... PS2, Line 187: struct region region; This is not required.
https://review.coreboot.org/c/coreboot/+/46514/2/src/drivers/mrc_cache/mrc_c... PS2, Line 188: lookup_region(®ion, type); This needs to be `lookup_region_type()`. `lookup_region` also finds the region on SPI flash using `fmap_locate_area()` which is not required. This function only cares about getting a pointer to cache_region entry, which is provided by `lookup_region_type()`.