Change in coreboot[master]: soc/amd/picasso: Add console & timestamp buffers to psp_verstage
Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42830 )
Change subject: soc/amd/picasso: Add console & timestamp buffers to psp_verstage ......................................................................
soc/amd/picasso: Add console & timestamp buffers to psp_verstage
Create areas for console & timestamp data in psp_verstage and pass it to the x86 to save for use later.
BUG=b:159220781 TEST=Build & Boot trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I41c8d7a1565e761187e941d7d6021805a9744d06 --- M src/soc/amd/picasso/memlayout_psp_verstage.ld M src/soc/amd/picasso/psp_verstage/psp_verstage.c 2 files changed, 15 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/42830/1
diff --git a/src/soc/amd/picasso/memlayout_psp_verstage.ld b/src/soc/amd/picasso/memlayout_psp_verstage.ld index fe70f08..764c3e3 100644 --- a/src/soc/amd/picasso/memlayout_psp_verstage.ld +++ b/src/soc/amd/picasso/memlayout_psp_verstage.ld @@ -15,6 +15,8 @@ #define VBOOT_WORK_START 0x24000 #define VBOOT_WORK_SIZE 12K #define FMAP_CACHE_SIZE 2K +#define PSP_CONSOLE_SIZE CONFIG_PRERAM_CBMEM_CONSOLE_SIZE +#define PSP_TIMESTAMP_SIZE 0x100
/* * The temp stack can be made much smaller if needed - even 256 bytes @@ -53,6 +55,8 @@ _everstage = _verstage + VERSTAGE_SIZE;
REGION(vboot2_work, VERSTAGE_START + VERSTAGE_SIZE, VBOOT_WORK_SIZE, 64) + PRERAM_CBMEM_CONSOLE(., PSP_CONSOLE_SIZE) + TIMESTAMP(., PSP_TIMESTAMP_SIZE)
FMAP_CACHE(., FMAP_SIZE)
diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.c b/src/soc/amd/picasso/psp_verstage/psp_verstage.c index bac0548..b13edb7 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.c +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.c @@ -108,15 +108,20 @@ } printk(BIOS_DEBUG, "\nMaximum buffer size: %d bytes\n", max_buffer_size);
- retval = vb2api_relocate(_vboot2_work, _vboot2_work, buffer_size, ctx); - if (retval != VB2_SUCCESS) { - printk(BIOS_ERR, "Error shrinking workbuf. Error code %#x\n", retval); - buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE; - post_code(POSTCODE_WORKBUF_RESIZE_WARNING); + /* Shrink workbuf if MP2 is in use and cannot be used to save buffer */ + if (max_buffer_size < VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE) { + retval = vb2api_relocate(_vboot2_work, _vboot2_work, buffer_size, ctx); + if (retval != VB2_SUCCESS) { + printk(BIOS_ERR, "Error shrinking workbuf. Error code %#x\n", retval); + buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE; + post_code(POSTCODE_WORKBUF_RESIZE_WARNING); + } + } else { + buffer_size = (uint32_t)((uintptr_t)_etimestamp - (uintptr_t)_vboot2_work) }
if (buffer_size > max_buffer_size) { - printk(BIOS_ERR, "Error: Workbuf is larger than max buffer size.\n"); + printk(BIOS_ERR, "Error: Buffer is larger than max buffer size.\n"); post_code(POSTCODE_WORKBUF_BUFFER_SIZE_ERROR); return POSTCODE_WORKBUF_BUFFER_SIZE_ERROR; }
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 3:
This change is ready for review.
Aaron Durbin 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 3: Code-Review+2
Kangheui Won 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 3: Code-Review+1
Eric Peers 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 3:
(1 comment)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 150: printk(BIOS_ERR, "Error: Buffer is larger than max buffer size.\n"); but this is still workbuf. Why change the error? Not a blocker.
Kangheui Won 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 3: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 146: buffer_size = (uint32_t)((uintptr_t)_etimestamp - (uintptr_t)_vboot2_work); After this path ctx will be NULL in Main() below.
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 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 146: buffer_size = (uint32_t)((uintptr_t)_etimestamp - (uintptr_t)_vboot2_work);
After this path ctx will be NULL in Main() below.
Sorry, I think I'm missing something. Why would setting the buffer size change ctx to null? We're not touching ctx at all on this path. We only use it if we need to shrink the buffer in the other half of this if/else, right?
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 150: printk(BIOS_ERR, "Error: Buffer is larger than max buffer size.\n");
but this is still workbuf. Why change the error? Not a blocker.
Because the buffer may not be just the workbuf. It now might include the console and timestamp areas as well. I'll change it back if you want.
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 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 146: buffer_size = (uint32_t)((uintptr_t)_etimestamp - (uintptr_t)_vboot2_work);
Sorry, I think I'm missing something. […]
Ok, I see - it will *STILL* be null. I'll update the code to make sure it's not null when we need it.
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 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_psp_verstage.ld:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/memlayo... PS3, Line 57: vboot2_work Do the console and timestamp regions have some kind of magic value we can use in bootblock to check if the regions were written by the PSP?
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 155: _vboot2_work Can you define a new region in the linker script: _workbuf and _eworkbuf. This will make it clearer that we are copying more than just vboot.
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 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_psp_verstage.ld:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/memlayo... PS3, Line 57: vboot2_work
Do the console and timestamp regions have some kind of magic value we can use in bootblock to check […]
I'm not sure. I'll take a look, but even if there isn't we can add something. That'll be in a later patch when we actually start using the regions though.
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 155: _vboot2_work
Can you define a new region in the linker script: _workbuf and _eworkbuf. […]
how about _transfer_buf? naming it _workbuf but including things in it that aren't the workbuf are going to make it even more confusing. _vboot2_work *IS* just the workbuf, but it's the start of the whole transfer buffer.
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 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_psp_verstage.ld:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/memlayo... PS3, Line 57: vboot2_work
I'm not sure. I'll take a look, but even if there isn't we can add something. […]
I guess you could add a struct at the beginning of the transfer buf that describes what got transferred so we don't have to change the console and timestamp structs to include a magic value.
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 155: _vboot2_work
how about _transfer_buf? naming it _workbuf but including things in it that aren't the workbuf are […]
I like that.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Kangheui Won, Julius Werner, Eric Peers, Rob Barnes, Aaron Durbin, Kangheui Won, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42830
to look at the new patch set (#4).
Change subject: soc/amd/picasso: Add console & timestamp buffers to psp_verstage ......................................................................
soc/amd/picasso: Add console & timestamp buffers to psp_verstage
Create areas for console & timestamp data in psp_verstage and pass it to the x86 to save for use later.
BUG=b:159220781 TEST=Build & Boot trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I41c8d7a1565e761187e941d7d6021805a9744d06 --- M src/include/symbols.h A src/soc/amd/picasso/include/soc/psp_transfer.h M src/soc/amd/picasso/memlayout_psp_verstage.ld M src/soc/amd/picasso/memlayout_x86.ld M src/soc/amd/picasso/psp_verstage/psp_verstage.c M src/soc/amd/picasso/psp_verstage/psp_verstage.h 6 files changed, 86 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/42830/4
build bot (Jenkins) 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 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42830/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/4/src/soc/amd/picasso/psp_ver... PS4, Line 140: retval = vb2api_relocate(_vboot2_work, _vboot2_work, MIN_WORKBUF_TRANSFER_SIZE, ctx); line over 96 characters
https://review.coreboot.org/c/coreboot/+/42830/4/src/soc/amd/picasso/psp_ver... PS4, Line 150: buffer_info.console_offset=(uint32_t)((uintptr_t)_preram_cbmem_console - spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42830/4/src/soc/amd/picasso/psp_ver... PS4, Line 152: buffer_info.timestamp_offset=(uint32_t)((uintptr_t)_timestamp - spaces required around that '=' (ctx:VxV)
https://review.coreboot.org/c/coreboot/+/42830/4/src/soc/amd/picasso/psp_ver... PS4, Line 154: buffer_info.fmap_offset=(uint32_t)((uintptr_t)_fmap_cache - spaces required around that '=' (ctx:VxV)
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Kangheui Won, Julius Werner, Eric Peers, Rob Barnes, Aaron Durbin, Kangheui Won, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42830
to look at the new patch set (#5).
Change subject: soc/amd/picasso: Add console & timestamp buffers to psp_verstage ......................................................................
soc/amd/picasso: Add console & timestamp buffers to psp_verstage
Create areas for console & timestamp data in psp_verstage and pass it to the x86 to save for use later.
BUG=b:159220781 TEST=Build & Boot trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I41c8d7a1565e761187e941d7d6021805a9744d06 --- M src/include/symbols.h A src/soc/amd/picasso/include/soc/psp_transfer.h M src/soc/amd/picasso/memlayout_psp_verstage.ld M src/soc/amd/picasso/memlayout_x86.ld M src/soc/amd/picasso/psp_verstage/psp_verstage.c M src/soc/amd/picasso/psp_verstage/psp_verstage.h 6 files changed, 87 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/42830/5
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:
(2 comments)
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 146: buffer_size = (uint32_t)((uintptr_t)_etimestamp - (uintptr_t)_vboot2_work);
Ok, I see - it will *STILL* be null. […]
Done
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/psp_ver... PS3, Line 155: _vboot2_work
I like that.
Done
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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/memlayo... File src/soc/amd/picasso/memlayout_psp_verstage.ld:
https://review.coreboot.org/c/coreboot/+/42830/3/src/soc/amd/picasso/memlayo... PS3, Line 57: vboot2_work
I guess you could add a struct at the beginning of the transfer buf that describes what got transfer […]
Done
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.
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.
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:
(2 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
Because that's a cache line size and a reasonable boundary. […]
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 = .;
I didn't see that having the extra symbol is a problem, but I've updated it to surround it with CONF […]
Done
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Kangheui Won, Julius Werner, Eric Peers, Rob Barnes, Aaron Durbin, Kangheui Won, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42830
to look at the new patch set (#6).
Change subject: soc/amd/picasso: Add console & timestamp buffers to psp_verstage ......................................................................
soc/amd/picasso: Add console & timestamp buffers to psp_verstage
Create areas for console & timestamp data in psp_verstage and pass it to the x86 to save for use later.
BUG=b:159220781 TEST=Build & Boot trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I41c8d7a1565e761187e941d7d6021805a9744d06 --- M src/include/symbols.h A src/soc/amd/picasso/include/soc/psp_transfer.h M src/soc/amd/picasso/memlayout_psp_verstage.ld M src/soc/amd/picasso/memlayout_x86.ld M src/soc/amd/picasso/psp_verstage/psp_verstage.c M src/soc/amd/picasso/psp_verstage/psp_verstage.h 6 files changed, 94 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/30/42830/6
build bot (Jenkins) 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42830/6/src/soc/amd/picasso/include... File src/soc/amd/picasso/include/soc/psp_transfer.h:
https://review.coreboot.org/c/coreboot/+/42830/6/src/soc/amd/picasso/include... PS6, Line 28: _Static_assert(sizeof(struct transfer_info_struct) == TRANSFER_INFO_SIZE, \ Avoid unnecessary line continuations
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 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42830 )
Change subject: soc/amd/picasso: Add console & timestamp buffers to psp_verstage ......................................................................
soc/amd/picasso: Add console & timestamp buffers to psp_verstage
Create areas for console & timestamp data in psp_verstage and pass it to the x86 to save for use later.
BUG=b:159220781 TEST=Build & Boot trembyle
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: I41c8d7a1565e761187e941d7d6021805a9744d06 Reviewed-on: https://review.coreboot.org/c/coreboot/+/42830 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/include/symbols.h A src/soc/amd/picasso/include/soc/psp_transfer.h M src/soc/amd/picasso/memlayout_psp_verstage.ld M src/soc/amd/picasso/memlayout_x86.ld M src/soc/amd/picasso/psp_verstage/psp_verstage.c M src/soc/amd/picasso/psp_verstage/psp_verstage.h 6 files changed, 94 insertions(+), 22 deletions(-)
Approvals: build bot (Jenkins): Verified Raul Rangel: Looks good to me, approved
diff --git a/src/include/symbols.h b/src/include/symbols.h index 57c52ee..fe3f46a 100644 --- a/src/include/symbols.h +++ b/src/include/symbols.h @@ -60,6 +60,7 @@ DECLARE_REGION(pdpt) DECLARE_REGION(opensbi) DECLARE_REGION(bl31) +DECLARE_REGION(transfer_buffer)
/* * Put this into a .c file accessing a linker script region to mark that region diff --git a/src/soc/amd/picasso/include/soc/psp_transfer.h b/src/soc/amd/picasso/include/soc/psp_transfer.h new file mode 100644 index 0000000..6a43b55 --- /dev/null +++ b/src/soc/amd/picasso/include/soc/psp_transfer.h @@ -0,0 +1,32 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef PSP_VERSTAGE_PSP_TRANSFER_H +#define PSP_VERSTAGE_PSP_TRANSFER_H + +#define TRANSFER_INFO_SIZE 64 +#define TIMESTAMP_BUFFER_SIZE 0x200 + +#define TRANSFER_MAGIC_VAL 0x50544953 + +/* Area for things that would cause errors in a linker script */ +#if !defined(__ASSEMBLER__) +#include <stdint.h> + +struct transfer_info_struct { + uint32_t magic_val; /* Identifier */ + uint32_t struct_bytes; /* Size of this structure */ + uint32_t buffer_size; /* Size of the transfer buffer area */ + + /* Offsets from start of transfer buffer */ + uint32_t workbuf_offset; + uint32_t console_offset; + uint32_t timestamp_offset; + uint32_t fmap_offset; + uint32_t unused[9]; /* Pad to 64 bytes */ +}; + +_Static_assert(sizeof(struct transfer_info_struct) == TRANSFER_INFO_SIZE, \ + "TRANSFER_INFO_SIZE is incorrect"); +#endif + +#endif /* PSP_VERSTAGE_PSP_TRANSFER_H */ diff --git a/src/soc/amd/picasso/memlayout_psp_verstage.ld b/src/soc/amd/picasso/memlayout_psp_verstage.ld index 0fed7b0..4ad88b1 100644 --- a/src/soc/amd/picasso/memlayout_psp_verstage.ld +++ b/src/soc/amd/picasso/memlayout_psp_verstage.ld @@ -1,6 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0-only */
#include <memlayout.h> +#include <soc/psp_transfer.h> +#include <fmap_config.h>
/* * Start of available space is 0x15000 and this is where the @@ -11,8 +13,6 @@ #define PSP_SRAM_SIZE 160K
#define VERSTAGE_START 0x15000 -#define VBOOT_WORK_SIZE 12K -#define FMAP_CACHE_SIZE 2K
/* * The temp stack can be made much smaller if needed - even 256 bytes @@ -52,9 +52,15 @@ _everstage = .;
ALIGN_COUNTER(64) - REGION(vboot2_work, ., VBOOT_WORK_SIZE, 64) - + _transfer_buffer = .; + REGION(transfer_info, ., TRANSFER_INFO_SIZE, 4) + ALIGN_COUNTER(64) + REGION(vboot2_work, ., VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE, 64) + ALIGN_COUNTER(64) + PRERAM_CBMEM_CONSOLE(., CONFIG_PRERAM_CBMEM_CONSOLE_SIZE) + TIMESTAMP(., TIMESTAMP_BUFFER_SIZE) FMAP_CACHE(., FMAP_SIZE) + _etransfer_buffer = .;
PSP_VERSTAGE_TEMP_STACK_END = (PSP_VERSTAGE_TEMP_STACK_START + PSP_VERSTAGE_TEMP_STACK_SIZE );
diff --git a/src/soc/amd/picasso/memlayout_x86.ld b/src/soc/amd/picasso/memlayout_x86.ld index 6f43ba1..7930793 100644 --- a/src/soc/amd/picasso/memlayout_x86.ld +++ b/src/soc/amd/picasso/memlayout_x86.ld @@ -2,6 +2,7 @@
#include <memlayout.h> #include <arch/header.ld> +#include <soc/psp_transfer.h>
#define EARLY_RESERVED_DRAM_START(addr) SYMBOL(early_reserved_dram, addr) #define EARLY_RESERVED_DRAM_END(addr) SYMBOL(eearly_reserved_dram, addr) @@ -38,14 +39,16 @@ * | Unused hole | * +--------------------------------+ * | FMAP cache (FMAP_SIZE) | - * +--------------------------------+ PSP_SHAREDMEM_BASE + PSP_SHAREDMEM_SIZE + PRERAM_CBMEM_CONSOLE_SIZE + 0x200 + * +--------------------------------+ PSP_SHAREDMEM_BASE + 0x40 + PSP_SHAREDMEM_SIZE + PRERAM_CBMEM_CONSOLE_SIZE + 0x200 * | Early Timestamp region (512B) | - * +--------------------------------+ PSP_SHAREDMEM_BASE + PSP_SHAREDMEM_SIZE + PRERAM_CBMEM_CONSOLE_SIZE + * +--------------------------------+ PSP_SHAREDMEM_BASE + 0x40 + PSP_SHAREDMEM_SIZE + PRERAM_CBMEM_CONSOLE_SIZE * | Preram CBMEM console | * | (PRERAM_CBMEM_CONSOLE_SIZE) | - * +--------------------------------+ PSP_SHAREDMEM_BASE + PSP_SHAREDMEM_SIZE + * +--------------------------------+ PSP_SHAREDMEM_BASE + 0x40 + PSP_SHAREDMEM_SIZE * | PSP shared (vboot workbuf) | * | (PSP_SHAREDMEM_SIZE) | + * +--------------------------------+ PSP_SHAREDMEM_BASE + 0x40 + * | Transfer Info Structure | * +--------------------------------+ PSP_SHAREDMEM_BASE * | APOB (64KiB) | * +--------------------------------+ PSP_APOB_DRAM_ADDRESS @@ -72,14 +75,18 @@
#if CONFIG(VBOOT) PSP_SHAREDMEM_DRAM_START(CONFIG_PSP_SHAREDMEM_BASE) + _transfer_buffer = .; + REGION(transfer_info, ., TRANSFER_INFO_SIZE, 4) VBOOT2_WORK(., VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE) PSP_SHAREDMEM_DRAM_END(CONFIG_PSP_SHAREDMEM_BASE + CONFIG_PSP_SHAREDMEM_SIZE) #endif
PRERAM_CBMEM_CONSOLE(., CONFIG_PRERAM_CBMEM_CONSOLE_SIZE) - TIMESTAMP(., 0x200) + TIMESTAMP(., TIMESTAMP_BUFFER_SIZE) FMAP_CACHE(., FMAP_SIZE) - +#if CONFIG(VBOOT) + _etransfer_buffer = .; +#endif _ = ASSERT((CONFIG_BOOTBLOCK_ADDR + CONFIG_C_ENV_BOOTBLOCK_SIZE - 0x10) == CONFIG_X86_RESET_VECTOR, "Reset vector should be -0x10 from end of bootblock"); _ = ASSERT(CONFIG_BOOTBLOCK_ADDR == ((CONFIG_BOOTBLOCK_ADDR + 0xFFFF) & 0xFFFF0000), "Bootblock must be 16 bit aligned"); BOOTBLOCK(CONFIG_BOOTBLOCK_ADDR, CONFIG_C_ENV_BOOTBLOCK_SIZE) diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.c b/src/soc/amd/picasso/psp_verstage/psp_verstage.c index 2524651..5568797 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.c +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.c @@ -8,6 +8,7 @@ #include <commonlib/region.h> #include <console/console.h> #include <fmap.h> +#include <soc/psp_transfer.h> #include <security/vboot/misc.h> #include <security/vboot/symbols.h> #include <security/vboot/vboot_common.h> @@ -120,8 +121,9 @@ static uint32_t save_buffers(struct vb2_context **ctx) { uint32_t retval; - uint32_t buffer_size = DEFAULT_WORKBUF_TRANSFER_SIZE; + uint32_t buffer_size = MIN_TRANSFER_BUFFER_SIZE; uint32_t max_buffer_size; + struct transfer_info_struct buffer_info = {0};
/* * This should never fail, but if it does, we should still try to @@ -130,28 +132,47 @@ if (svc_get_max_workbuf_size(&max_buffer_size)) { post_code(POSTCODE_DEFAULT_BUFFER_SIZE_NOTICE); printk(BIOS_NOTICE, "Notice: using default transfer buffer size.\n"); - max_buffer_size = DEFAULT_WORKBUF_TRANSFER_SIZE; + max_buffer_size = MIN_TRANSFER_BUFFER_SIZE; } printk(BIOS_DEBUG, "\nMaximum buffer size: %d bytes\n", max_buffer_size);
- retval = vb2api_relocate(_vboot2_work, _vboot2_work, buffer_size, ctx); - if (retval != VB2_SUCCESS) { - printk(BIOS_ERR, "Error shrinking workbuf. Error code %#x\n", retval); - buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE; - post_code(POSTCODE_WORKBUF_RESIZE_WARNING); + /* Shrink workbuf if MP2 is in use and cannot be used to save buffer */ + if (max_buffer_size < VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE) { + retval = vb2api_relocate(_vboot2_work, _vboot2_work, MIN_WORKBUF_TRANSFER_SIZE, + ctx); + if (retval != VB2_SUCCESS) { + printk(BIOS_ERR, "Error shrinking workbuf. Error code %#x\n", retval); + buffer_size = VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE; + post_code(POSTCODE_WORKBUF_RESIZE_WARNING); + } + } else { + buffer_size = + (uint32_t)((uintptr_t)_etransfer_buffer - (uintptr_t)_transfer_buffer); + + buffer_info.console_offset = (uint32_t)((uintptr_t)_preram_cbmem_console - + (uintptr_t)_transfer_buffer); + buffer_info.timestamp_offset = (uint32_t)((uintptr_t)_timestamp - + (uintptr_t)_transfer_buffer); + buffer_info.fmap_offset = (uint32_t)((uintptr_t)_fmap_cache - + (uintptr_t)_transfer_buffer); }
if (buffer_size > max_buffer_size) { - printk(BIOS_ERR, "Error: Workbuf is larger than max buffer size.\n"); + printk(BIOS_ERR, "Error: Buffer is larger than max buffer size.\n"); post_code(POSTCODE_WORKBUF_BUFFER_SIZE_ERROR); return POSTCODE_WORKBUF_BUFFER_SIZE_ERROR; }
- retval = svc_save_uapp_data(UAPP_COPYBUF_CHROME_WORKBUF, (void *)_vboot2_work, - buffer_size); + buffer_info.magic_val = TRANSFER_MAGIC_VAL; + buffer_info.struct_bytes = sizeof(buffer_info); + buffer_info.buffer_size = buffer_size; + buffer_info.workbuf_offset = (uint32_t)((uintptr_t)_fmap_cache - + (uintptr_t)_vboot2_work); + + retval = svc_save_uapp_data(UAPP_COPYBUF_CHROME_WORKBUF, (void *)_transfer_buffer, + buffer_size); if (retval) { - printk(BIOS_ERR, "Error: Could not save workbuf. Error code 0x%08x\n", - retval); + printk(BIOS_ERR, "Error: Could not save workbuf. Error code 0x%08x\n", retval); return POSTCODE_WORKBUF_SAVE_ERROR; }
@@ -193,6 +214,9 @@
verstage_main();
+ vb2api_relocate(_vboot2_work, _vboot2_work, VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE, + &ctx); + post_code(POSTCODE_SAVE_BUFFERS); retval = save_buffers(&ctx); if (retval) diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.h b/src/soc/amd/picasso/psp_verstage/psp_verstage.h index ad422fc..3c7574d 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.h +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.h @@ -4,6 +4,7 @@ #define PSP_VERSTAGE_H
#include <stdint.h> +#include <soc/psp_transfer.h>
#define EMBEDDED_FW_SIGNATURE 0x55aa55aa #define PSP_COOKIE 0x50535024 /* 'PSP$' */ @@ -36,7 +37,8 @@ #define POSTCODE_LEAVING_VERSTAGE 0xF2
#define SPI_ADDR_MASK 0x00ffffff -#define DEFAULT_WORKBUF_TRANSFER_SIZE (8 * KiB) +#define MIN_TRANSFER_BUFFER_SIZE (8 * KiB) +#define MIN_WORKBUF_TRANSFER_SIZE (MIN_TRANSFER_BUFFER_SIZE - TRANSFER_INFO_SIZE)
struct psp_ef_table { uint32_t signature; /* 0x55aa55aa */
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Eric Peers (Code Review)
-
Kangheui Won (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)