Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33397 )
Change subject: util/amdfwtool: Align PSP NVRAM ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c File util/amdfwtool/amdfwtool.c:
https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@479 PS1, Line 479: (uint32_t)bytes;
Should this be rounded up to fill the entire 4k flash sector?
Yes, done
https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@484 PS1, Line 484: ctx->current = ALIGN(ctx->current, ERASE_ALIGNMENT);
Why the intermediate value with BLOB_ALIGNMENT?
I didn't want this code to need to know which was the larger granularity. Will change it to use a max() value instead.
https://review.coreboot.org/#/c/33397/1/util/amdfwtool/amdfwtool.c@508 PS1, Line 508: if (count > MAX_PSP_ENTRIES) {
Unrelated, but you really should test for invalid indexing of arrays before the access happens.
Maybe I'll consider that for a follow-on.
Since the utility allocates the size of the rom, I don't think it's an emergency, and in fact count should never be greater than the number of items in the fw_table. We allocate the amount of memory equal to the flash's size. The intent here was to not write a higher count into the header than the header was meant to describe.