Kangheui Won has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
soc/amd/picasso: copy local info to transfer buf
buffer_info is local variable, we need to copy it to _transfer_buffer before we hand it over to PSP.
BUG=b:159220781 TEST=check transfer_info_struct is correctly populated on romstage
Signed-off-by: Kangheui Won khwon@chromium.org Change-Id: I14bc34e6af501240a6f633db3999a7759e88d60b --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/44751/1
diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.c b/src/soc/amd/picasso/psp_verstage/psp_verstage.c index 005c8b0..ef4884f 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.c +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.c @@ -186,6 +186,8 @@ buffer_info.workbuf_offset = (uint32_t)((uintptr_t)_fmap_cache - (uintptr_t)_vboot2_work);
+ memcpy(_transfer_buffer, &buffer_info, sizeof(buffer_info)); + retval = svc_save_uapp_data(UAPP_COPYBUF_CHROME_WORKBUF, (void *)_transfer_buffer, buffer_size); if (retval) {
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44751/1//COMMIT_MSG@8 PS1, Line 8: Please start by describing the problem.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 1: Code-Review+1
As Paul noted, please work on summary and fuller description so others have better context.
Hello build bot (Jenkins), Raul Rangel, Martin Roth, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44751
to look at the new patch set (#2).
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
soc/amd/picasso: copy local info to transfer buf
We added transfer_info_struct to contain various information about memory region we pass from PSP to x86 in 0c12abe.
This should be at the start of transfer region, but we only manipulated it as local varaible and didn't put data into the region.
Copy the content of local variable to beginning of _transfer_buffer before requesting transfer to PSP so coreboot on x86 can access it.
BUG=b:159220781 TEST=check transfer_info_struct is correctly populated on romstage
Signed-off-by: Kangheui Won khwon@chromium.org Change-Id: I14bc34e6af501240a6f633db3999a7759e88d60b --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/44751/2
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44751/1//COMMIT_MSG@8 PS1, Line 8:
Please start by describing the problem.
Added more description. Let me know if there is still any unclear thing.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44751/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44751/1//COMMIT_MSG@8 PS1, Line 8:
Added more description. Let me know if there is still any unclear thing.
Thank you, I added some more notes.
https://review.coreboot.org/c/coreboot/+/44751/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44751/2//COMMIT_MSG@10 PS2, Line 10: 0c12abe Please prepend it with *commit*, and maybe use an eight or ten digit hash, so Gerrit marks it up as a link.
https://review.coreboot.org/c/coreboot/+/44751/2//COMMIT_MSG@13 PS2, Line 13: varaible variable
https://review.coreboot.org/c/coreboot/+/44751/2//COMMIT_MSG@13 PS2, Line 13: it as local varaible and didn't put data into the region. What problem did it cause?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/44751/2/src/soc/amd/picasso/psp_ver... PS2, Line 143: struct transfer_info_struct buffer_info = {0}; How about:
struct transfer_info_struct *buffer_info = &_transfer_buffer;
Hello build bot (Jenkins), Raul Rangel, Martin Roth, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44751
to look at the new patch set (#3).
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
soc/amd/picasso: copy local info to transfer buf
We added transfer_info_struct to contain various information about memory region we pass from PSP to x86 in commit 0c12abe462.
This should be at the start of transfer region but we only manipulated it as local variable and didn't put data into the region, resulting garbage data for transfer_info when x86 tries to read it.
Copy the content of local variable to beginning of _transfer_buffer before requesting transfer to PSP so coreboot on x86 can access it.
BUG=b:159220781 TEST=check transfer_info_struct is correctly populated on romstage
Signed-off-by: Kangheui Won khwon@chromium.org Change-Id: I14bc34e6af501240a6f633db3999a7759e88d60b --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/44751/3
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/44751/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44751/2//COMMIT_MSG@10 PS2, Line 10: 0c12abe
Please prepend it with *commit*, and maybe use an eight or ten digit hash, so Gerrit marks it up as […]
Thanks, I was wondering why gerrit doesn't make a link.
https://review.coreboot.org/c/coreboot/+/44751/2//COMMIT_MSG@13 PS2, Line 13: varaible
variable
Done
https://review.coreboot.org/c/coreboot/+/44751/2//COMMIT_MSG@13 PS2, Line 13: it as local varaible and didn't put data into the region.
What problem did it cause?
Done
https://review.coreboot.org/c/coreboot/+/44751/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/44751/2/src/soc/amd/picasso/psp_ver... PS2, Line 143: struct transfer_info_struct buffer_info = {0};
How about: […]
That and memset for zeroing will also work but I guess it's basically same. Is there any additional benefits?
Hello build bot (Jenkins), Raul Rangel, Martin Roth, Edward O'Callaghan, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44751
to look at the new patch set (#4).
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
soc/amd/picasso: copy local info to transfer buf
We added transfer_info_struct to contain various information about memory region we pass from PSP to x86 in commit 0c12abe462.
This should be at the start of transfer region but we only manipulated it as local variable and didn't put data into the region, resulting garbage data for transfer_info when x86 tries to read it.
Copy the content of local variable to beginning of _transfer_buffer before requesting transfer to PSP so coreboot on x86 can access it.
BUG=b:159220781 BRANCH=zork TEST=check transfer_info_struct is correctly populated on romstage
Signed-off-by: Kangheui Won khwon@chromium.org Change-Id: I14bc34e6af501240a6f633db3999a7759e88d60b --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 2 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/51/44751/4
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... PS4, Line 189: sizeof(buffer_info) Optional but I think a good idea - While both src and dst happen to be the same type at the moment it is best to be defensive here and do the sizeof the dst or directly the size of the struct type and not the source.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 4: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/44751/2/src/soc/amd/picasso/psp_ver... PS2, Line 143: struct transfer_info_struct buffer_info = {0};
That and memset for zeroing will also work but I guess it's basically same. […]
Probably not. My intention was to do as Raul suggested, but as you say, it's basically the same.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... PS4, Line 189: sizeof(buffer_info)
Optional but I think a good idea - While both src and dst happen to be the same type at the moment i […]
Agreed. Plus, if the type of the destination is ever changed, this will break. Not necessarily resulting in a build failure, btw.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... PS4, Line 189: sizeof(buffer_info)
Agreed. Plus, if the type of the destination is ever changed, this will break. […]
dst is a bare symbol with the size allocated by the linker script. There is no type but the buffer_info type used for src. Please see src/soc/amd/picasso/memlayout_psp_verstage.ld
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... PS4, Line 189: sizeof(buffer_info)
dst is a bare symbol with the size allocated by the linker script. […]
Oh, it's a linker symbol. Nevermind then.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/44751/4/src/soc/amd/picasso/psp_ver... PS4, Line 189: sizeof(buffer_info)
Oh, it's a linker symbol. Nevermind then.
Ack
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
soc/amd/picasso: copy local info to transfer buf
We added transfer_info_struct to contain various information about memory region we pass from PSP to x86 in commit 0c12abe462.
This should be at the start of transfer region but we only manipulated it as local variable and didn't put data into the region, resulting garbage data for transfer_info when x86 tries to read it.
Copy the content of local variable to beginning of _transfer_buffer before requesting transfer to PSP so coreboot on x86 can access it.
BUG=b:159220781 BRANCH=zork TEST=check transfer_info_struct is correctly populated on romstage
Signed-off-by: Kangheui Won khwon@chromium.org Change-Id: I14bc34e6af501240a6f633db3999a7759e88d60b Reviewed-on: https://review.coreboot.org/c/coreboot/+/44751 Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Edward O'Callaghan quasisec@chromium.org Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 2 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Aaron Durbin: Looks good to me, approved Edward O'Callaghan: Looks good to me, approved
diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.c b/src/soc/amd/picasso/psp_verstage/psp_verstage.c index d071aa6..c2178a3 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.c +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.c @@ -196,6 +196,8 @@ buffer_info.workbuf_offset = (uint32_t)((uintptr_t)_fmap_cache - (uintptr_t)_vboot2_work);
+ memcpy(_transfer_buffer, &buffer_info, sizeof(buffer_info)); + retval = svc_save_uapp_data(UAPP_COPYBUF_CHROME_WORKBUF, (void *)_transfer_buffer, buffer_size); if (retval) {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44751 )
Change subject: soc/amd/picasso: copy local info to transfer buf ......................................................................
Patch Set 5:
Automatic boot test returned (PASS/FAIL/TOTAL): 8/1/9 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19175 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19174 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/19173 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19172 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/19171 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19179 "HP Z220 SFF Workstation" (x86_32) using payload LinuxBoot_BusyBox_kexec : SUCCESS : https://lava.9esec.io/r/19178 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/19177 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/19176
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Edward O'Callaghan (Code Review)
-
Kangheui Won (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)