Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42830 )
Change subject: soc/amd/picasso: Add console & timestamp buffers to psp_verstage ......................................................................
Patch Set 5:
(7 comments)
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/psp_transfer.h:
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/include... PS5, Line 6: 64 Why is this 64? It's only 28 bytes? Can you maybe add a static assert to verify this number == sizeof(transfer_info_struct)
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_psp_verstage.ld:
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/memlayo... PS5, Line 56: TRANSFER_INFO_SIZE does sizeof(struct transfer_info_struct) work? Dunno if the CPP can handle it.
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/memlayo... PS5, Line 57: REGION If you add ALIGN_COUNTER(64) before this line, I think you can correctly set TRANSFER_INFO_SIZE.
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_x86.ld:
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/memlayo... PS5, Line 87: _etransfer_buffer = .; This is outside of if CONFIG(VBOOT)
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/psp_ver... PS5, Line 126: buffer_info You need to zero out the buffer_info so you don't get random offsets.
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/psp_ver... PS5, Line 146: } Do we still have room for either the console or timestamps? Part of me feels like we waste a lot of space sharing the memory layout between the PSP, transfer buffer, and x86.
https://review.coreboot.org/c/coreboot/+/42830/5/src/soc/amd/picasso/psp_ver... PS5, Line 155: buffer_info Don't you need to check the max_buffer_size? I don't think you want offsets that are > then the size in the transfer buf.