Martin Roth 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 == sizeo […]
Because that's a cache line size and a reasonable boundary. Additionally, we want to give extra padding in case anything changes in the future we don't have to change the size here, and everything will still work.
Added padding to 64 bytes and static assert to verify that this value is correct.
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.
I added a static assert to make sure that the size of TRANSFER_INFO_SIZE matches the transfer_info_struct size.
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.
Done
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)
I didn't see that having the extra symbol is a problem, but I've updated it to surround it with 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.
Done
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 […]
No, if the transfer buffer gets stored in PSP memory instead of in the MP2, we ONLY have room for the workbuf.
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 […]
If the MP2 is available for storage, we are guaranteed to have space for all the buffers, so we don't need to check for each additional buffer (we were given 32k and use a good bit less than that). If the MP2 is NOT available, we only have room for the workbuf. We handled the size check above for that case. I could add additional checks for each buffer, but they're not actually valuable.
I'm going to mark this as resolved, but if you want to check for space before assigning the buffers anyway, let me know.