Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Replace vboot_reboot() in psp_verstage ......................................................................
soc/amd/picasso: Replace vboot_reboot() in psp_verstage
The vboot_reboot function calls through board_reset() which flushes the cache before rebooting. On the PSP, because we're running in userspace, this causes the system to hang before it can reboot.
Instead, just call the relevant functions of vboot_reboot() directly.
BUG=b:161554141 TEST=Run board through a bunch of recovery cycles.
Change-Id: I6775b2e049d6cfd9f78f2e07a941f6cc5be254b8 --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/43678/1
diff --git a/src/soc/amd/picasso/psp_verstage/psp_verstage.c b/src/soc/amd/picasso/psp_verstage/psp_verstage.c index bac0548..b349be4 100644 --- a/src/soc/amd/picasso/psp_verstage/psp_verstage.c +++ b/src/soc/amd/picasso/psp_verstage/psp_verstage.c @@ -11,6 +11,7 @@ #include <arch/stages.h> #include <stdarg.h> #include <stdio.h> +#include <reset.h>
#define RUN_PSP_SVC_TESTS 0
@@ -30,7 +31,9 @@ vboot_save_data(ctx);
svc_debug_print("Rebooting into recovery\n"); - vboot_reboot(); + + vboot_platform_prepare_reboot(); + do_board_reset(); }
/*
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, 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/+/43678
to look at the new patch set (#2).
Change subject: soc/amd/picasso: Replace vboot_reboot() in psp_verstage ......................................................................
soc/amd/picasso: Replace vboot_reboot() in psp_verstage
The vboot_reboot function calls through board_reset() which flushes the cache before rebooting. On the PSP, because we're running in userspace, this causes the system to hang before it can reboot.
Instead, just call the relevant functions of vboot_reboot() directly.
BUG=b:161554141 TEST=Run board through a bunch of recovery cycles.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I6775b2e049d6cfd9f78f2e07a941f6cc5be254b8 --- M src/soc/amd/picasso/psp_verstage/psp_verstage.c 1 file changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/43678/2
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Replace vboot_reboot() in psp_verstage ......................................................................
Patch Set 2: Code-Review+1
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Replace vboot_reboot() in psp_verstage ......................................................................
Patch Set 2: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/43678/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/43678/2/src/soc/amd/picasso/psp_ver... PS2, Line 36: do_board_reset(); vboot_reboot is also called a few times in verstage_main. How about adding a flag to disable the cache flush?
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Rob Barnes, Eric Peers, Aaron Durbin, Kangheui Won, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43678
to look at the new patch set (#3).
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage
Because psp_verstage runs in userspace, running the dcache_clean_all function causes the system to hang. Skip the call.
I've thought about a number of ways to do this, and I'm really not super happy with any of the options. - Kconfig symbol - SKIP_DCACHE_CLEAN_IN_VERSTAGE. This just doesn't seem like a good Kconfig symbol candidate to me. This does at least have the advantage of just being compile time code - #ifdef SKIP_DCACHE_CLEAN, then define it in the psp_verstage makefile. We don't really do this in other places, so it doesn't seem like a good way to do it. - The standard method for psp_verstage also doesn't seem appropriate in this instance because it's just because this implementation runs in user-space. It's not about the timing. if (CONFIG(RUN_VBOOT_BEFORE_BOOTBLOCK) && ENV_VERSTAGE) - Register in devicetree - All the disadvantages of the Kconfig symbol, while being harder to implement and using runtime code to do it.
So while I'm not happy with the weak function implementation, it seemed the cleanest way to do it.
BUG=b:161554141 TEST=Run board through a bunch of recovery cycles.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I6775b2e049d6cfd9f78f2e07a941f6cc5be254b8 --- M src/include/reset.h M src/lib/reset.c M src/soc/amd/picasso/psp_verstage/reset.c 3 files changed, 22 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/43678/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/43678/3/src/lib/reset.c File src/lib/reset.c:
https://review.coreboot.org/c/coreboot/+/43678/3/src/lib/reset.c@9 PS3, Line 9: __weak int run_dcache_clean_in_board_reset() { Bad function definition - __weak int run_dcache_clean_in_board_reset() should probably be __weak int run_dcache_clean_in_board_reset(void)
https://review.coreboot.org/c/coreboot/+/43678/3/src/lib/reset.c@9 PS3, Line 9: __weak int run_dcache_clean_in_board_reset() { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/43678/3/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/reset.c:
https://review.coreboot.org/c/coreboot/+/43678/3/src/soc/amd/picasso/psp_ver... PS3, Line 11: int run_dcache_clean_in_board_reset() { Bad function definition - int run_dcache_clean_in_board_reset() should probably be int run_dcache_clean_in_board_reset(void)
https://review.coreboot.org/c/coreboot/+/43678/3/src/soc/amd/picasso/psp_ver... PS3, Line 11: int run_dcache_clean_in_board_reset() { open brace '{' following function definitions go on the next line
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Rob Barnes, Eric Peers, Aaron Durbin, Kangheui Won, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43678
to look at the new patch set (#4).
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage
Because psp_verstage runs in userspace, running the dcache_clean_all function causes the system to hang. Skip the call.
I've thought about a number of ways to do this, and I'm really not super happy with any of the options. - Kconfig symbol - SKIP_DCACHE_CLEAN_IN_VERSTAGE. This just doesn't seem like a good Kconfig symbol candidate to me. This does at least have the advantage of just being compile time code - #ifdef SKIP_DCACHE_CLEAN, then define it in the psp_verstage makefile. We don't really do this in other places, so it doesn't seem like a good way to do it. - The standard method for psp_verstage also doesn't seem appropriate in this instance because it's just because this implementation runs in user-space. It's not about the timing. if (CONFIG(RUN_VBOOT_BEFORE_BOOTBLOCK) && ENV_VERSTAGE) - Register in devicetree - All the disadvantages of the Kconfig symbol, while being harder to implement and using runtime code to do it.
So while I'm not happy with the weak function implementation, it seemed the cleanest way to do it.
BUG=b:161554141 TEST=Run board through a bunch of recovery cycles.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I6775b2e049d6cfd9f78f2e07a941f6cc5be254b8 --- M src/include/reset.h M src/lib/reset.c M src/soc/amd/picasso/psp_verstage/reset.c 3 files changed, 24 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/43678/4
Eric Peers has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 4: Code-Review+1
I considered another option over chat with Martin. Why not make the dcache_clean_all the weak function?
The benefit: One less layer The cost: PSP has it permanently defined to do nothing. We might actually want such a function to invalidate a PSP cache. Name of function also doesn't follow actual implementation which is confusing.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 4:
Patch Set 4: Code-Review+1
I considered another option over chat with Martin. Why not make the dcache_clean_all the weak function?
The benefit: One less layer The cost: PSP has it permanently defined to do nothing. We might actually want such a function to invalidate a PSP cache. Name of function also doesn't follow actual implementation which is confusing.
The biggest issue I see with making dcache_clean_all the weak function is that it's the main function for each architecture that would have to be made weak. There's a definite preference for weak functions to be empty or minimal as we have here with just the return of a value. While it would be one less layer, it would also have to touch every other implementation of dcache_clean_all, so would end up touching more files.
As far as wanting to invalidate the PSP cache, we can't actually do that because the psp_verstage code is running in userspace. If we needed to do it, we'd have to get AMD to add an SVC call, but I don't think it's even an issue for the PSP. The only reason for wanting to clear the cache before a reboot that I can think of is for security, so there's no data with privileged information left in the cache that could be retrieved after a reboot, and since nothing can run in the PSP before the PSP bootloader which must be signed by AMD, I don't think that's a problem.
Kangheui Won has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 4: Code-Review+1
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 4: Code-Review+1
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43678/4/src/include/reset.h File src/include/reset.h:
https://review.coreboot.org/c/coreboot/+/43678/4/src/include/reset.h@6 PS4, Line 6: e e?
https://review.coreboot.org/c/coreboot/+/43678/4/src/lib/reset.c File src/lib/reset.c:
https://review.coreboot.org/c/coreboot/+/43678/4/src/lib/reset.c@14 PS4, Line 14: board_reset Part of me feels like this whole function should just be made weak or we should replace the compilation unit. This function doesn't really buy us anything other than some logging and a method that needs to be implemented.
We already have a MISSING_BOARD_RESET option, how about we add a new option CUSTOM_BOARD_RESET. When it's set we omit this reset.c file from the build.
For picasso/reset.c we copy this board_reset function over. For psp_verstage/reset.c we copy this function and remove the dcache.
This also means we could move the do_board_reset functionality into the board_reset since we aren't using the lib/reset.c framework anymore.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 4: Code-Review+1
I don't like adding weak functions very much, but a relatively generic Kconfig option can't be just only set in the verstage on PSP case and not for the rest of corebooot for picasso, linking cache_m.c instead of cpu.S in the verstage on psp case is semantically incorrect and having an oddly specific Kconfig option isn't great either.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43678/4/src/lib/reset.c File src/lib/reset.c:
https://review.coreboot.org/c/coreboot/+/43678/4/src/lib/reset.c@14 PS4, Line 14: board_reset
Part of me feels like this whole function should just be made weak or we should replace the compilat […]
I'm not in favor of making this weak for the reason I already mentioned in the top level response to eric, but I like the CUSTOM_BOARD_RESET idea. I'll switch to that.
Thanks.
Hello build bot (Jenkins), Raul Rangel, Furquan Shaikh, Kangheui Won, Rob Barnes, Eric Peers, Aaron Durbin, Kangheui Won, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/43678
to look at the new patch set (#5).
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage
Because psp_verstage runs in userspace, running the dcache_clean_all function causes the system to hang.
Adding a soc-specific version of board_reset() allows us to have one version on the X86 that mirrors the generic version and another copy for the PSP which skips the dcache_clean_all() function.
BUG=b:161554141 TEST=Run board through a bunch of recovery cycles.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I6775b2e049d6cfd9f78f2e07a941f6cc5be254b8 --- M src/soc/amd/picasso/Kconfig M src/soc/amd/picasso/psp_verstage/reset.c M src/soc/amd/picasso/reset.c 3 files changed, 10 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/78/43678/5
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/coreboot/+/43678/4/src/include/reset.h File src/include/reset.h:
https://review.coreboot.org/c/coreboot/+/43678/4/src/include/reset.h@6 PS4, Line 6: e
e?
Done
https://review.coreboot.org/c/coreboot/+/43678/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/psp_verstage.c:
https://review.coreboot.org/c/coreboot/+/43678/2/src/soc/amd/picasso/psp_ver... PS2, Line 36: do_board_reset();
vboot_reboot is also called a few times in verstage_main. […]
Done
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 5:
I don't understand the arguments against overriding dcache_clean_all(). If a weak function isn't the way to go (I also see it like that), one can still use other mechanisms (weak functions mostly just spare us the addition of a Kconfig symbol).
Alternatively, we could just guard the call to it. Either way, we have `rules.h` do advertise properties of the environment. Why not add an ENV_USER_SPACE and then don't try to clear caches if it's set?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 5:
Patch Set 5:
I don't understand the arguments against overriding dcache_clean_all(). If a weak function isn't the way to go (I also see it like that), one can still use other mechanisms (weak functions mostly just spare us the addition of a Kconfig symbol).
Alternatively, we could just guard the call to it. Either way, we have `rules.h` do advertise properties of the environment. Why not add an ENV_USER_SPACE and then don't try to clear caches if it's set?
I was going to follow up with this notion so it's clearer what semantics we're trying to convey.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 5:
Patch Set 5:
Patch Set 5:
I don't understand the arguments against overriding dcache_clean_all(). If a weak function isn't the way to go (I also see it like that), one can still use other mechanisms (weak functions mostly just spare us the addition of a Kconfig symbol).
Alternatively, we could just guard the call to it. Either way, we have `rules.h` do advertise properties of the environment. Why not add an ENV_USER_SPACE and then don't try to clear caches if it's set?
I was going to follow up with this notion so it's clearer what semantics we're trying to convey.
Sorry Aaron, follow up with which notion? Nico mentions two different ideas here.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 6:
Patch Set 5:
Patch Set 5:
Patch Set 5:
I don't understand the arguments against overriding dcache_clean_all(). If a weak function isn't the way to go (I also see it like that), one can still use other mechanisms (weak functions mostly just spare us the addition of a Kconfig symbol).
Alternatively, we could just guard the call to it. Either way, we have `rules.h` do advertise properties of the environment. Why not add an ENV_USER_SPACE and then don't try to clear caches if it's set?
I was going to follow up with this notion so it's clearer what semantics we're trying to convey.
Sorry Aaron, follow up with which notion? Nico mentions two different ideas here.
In the hope that I can get something merged this week, I've pushed a competing patch train to https://review.coreboot.org/c/coreboot/+/43784
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 5:
The dcache_xxx functions are meant to be callable at any point in common code and do the platform-appropriate thing, so I guess it's true that we can't just have Picasso sidestep every point where it happens to be called. Otherwise we'll keep chasing that whenever we add more cache flushes to common code later (and I assume it's a runtime exception so it wouldn't even be easy to tell that you broke it).
Please don't put a guard around every dcache_xxx call in coreboot either, though, it should be handled inside the function. I guess we need something like CONFIG(ARM_IN_USR_MODE)?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 6:
Patch Set 6:
Patch Set 5:
Patch Set 5:
Patch Set 5:
I don't understand the arguments against overriding dcache_clean_all(). If a weak function isn't the way to go (I also see it like that), one can still use other mechanisms (weak functions mostly just spare us the addition of a Kconfig symbol).
Alternatively, we could just guard the call to it. Either way, we have `rules.h` do advertise properties of the environment. Why not add an ENV_USER_SPACE and then don't try to clear caches if it's set?
I was going to follow up with this notion so it's clearer what semantics we're trying to convey.
Sorry Aaron, follow up with which notion? Nico mentions two different ideas here.
In the hope that I can get something merged this week, I've pushed a competing patch train to https://review.coreboot.org/c/coreboot/+/43784
I was meaning the concept of ENV_USER_SPACE for a particular stage. I see you pushed something to that effect already.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Patch Set 6: Code-Review+2
Martin Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/43678 )
Change subject: soc/amd/picasso: Don't run dcache_clean_all() in psp_verstage ......................................................................
Abandoned
No longer needed.
-
Aaron Durbin (Code Review) -
build bot (Jenkins) (Code Review)
-
Eric Peers (Code Review)
-
Felix Held (Code Review)
-
Julius Werner (Code Review)
-
Kangheui Won (Code Review)
-
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Raul Rangel (Code Review)
-
Rob Barnes (Code Review)