Attention is currently required from: Jason Glenesk, Raul Rangel, Matt DeVillier, Martin Roth, Felix Held.
Fred Reitberger has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/67301 )
Change subject: soc/amd/common/block/apob: Add hashed APOB support ......................................................................
Patch Set 2:
(3 comments)
File src/soc/amd/common/block/apob/apob_cache.c:
https://review.coreboot.org/c/coreboot/+/67301/comment/28bb344f_ac3f394e PS2, Line 22: RW_MRC_HASH
Curious if we can just place the hash at the end of the APCB so we can reuse the `RW_MRC_CACHE`? Or […]
There's plenty of space (this hash is only 8 bytes), but doesn't that require it to read the entire block from flash first? Or can it be randomly accessed without loading/memory mapping the rest of the block?
https://review.coreboot.org/c/coreboot/+/67301/comment/a2dc9b69_5f591ee8 PS2, Line 162: sizeof(hash)
I would add an assert somewhere that verifies `sizeof(has) <= DEFAULT_MRC_HASH_SIZE`
Can't use assert due to cases where it's not enabled, but updated the if statement to check the size. This shouldn't get called unless hashing is enabled, and if the fmap doesn't have the correct region, the locate above should fail first.
https://review.coreboot.org/c/coreboot/+/67301/comment/237caa66_bff29212 PS2, Line 233: memcmp
Add a `!update_needed && `. […]
Done
-
Fred Reitberger (Code Review)