Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/mp_init: Fix timeout ......................................................................
soc/mp_init: Fix timeout
We ususally don't know how long specific code runs as serial console and it's spinlock might slow down APs.
Increase the timeout of most mp_run_on_all_cpus calls to 1 second, to prevent possible crashing of secondary APs still processing the old job.
Change-Id: I456be647b159f7a2ea7d94986a24424e56dcc8c4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/apollolake/chip.c M src/soc/intel/apollolake/cpu.c M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/skylake/cpu.c 7 files changed, 9 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34587/1
diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index d9f20e7..dcd657a 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -993,7 +993,7 @@ stopwatch_init(&sw);
ret = mp_run_on_aps(park_this_cpu, NULL, MP_RUN_ON_ALL_CPUS, - 250 * USECS_PER_MSEC); + 1000 * USECS_PER_MSEC);
duration_msecs = stopwatch_duration_msecs(&sw);
diff --git a/src/soc/amd/picasso/finalize.c b/src/soc/amd/picasso/finalize.c index 5a4627a..f94856c 100644 --- a/src/soc/amd/picasso/finalize.c +++ b/src/soc/amd/picasso/finalize.c @@ -44,7 +44,7 @@ int r; printk(BIOS_SPEW, "Lock SMM configuration\n");
- r = mp_run_on_all_cpus(per_core_finalize, NULL, 10 * USECS_PER_MSEC); + r = mp_run_on_all_cpus(per_core_finalize, NULL, 1000 * USECS_PER_MSEC); if (r) printk(BIOS_WARNING, "Failed to finalize all cores\n"); } diff --git a/src/soc/amd/stoneyridge/finalize.c b/src/soc/amd/stoneyridge/finalize.c index 5a4627a..f94856c 100644 --- a/src/soc/amd/stoneyridge/finalize.c +++ b/src/soc/amd/stoneyridge/finalize.c @@ -44,7 +44,7 @@ int r; printk(BIOS_SPEW, "Lock SMM configuration\n");
- r = mp_run_on_all_cpus(per_core_finalize, NULL, 10 * USECS_PER_MSEC); + r = mp_run_on_all_cpus(per_core_finalize, NULL, 1000 * USECS_PER_MSEC); if (r) printk(BIOS_WARNING, "Failed to finalize all cores\n"); } diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c index 361e6a4..da72d49 100644 --- a/src/soc/intel/apollolake/chip.c +++ b/src/soc/intel/apollolake/chip.c @@ -764,7 +764,7 @@ static void drop_privilege_all(void) { /* Drop privilege level on all the CPUs */ - if (mp_run_on_all_cpus(&cpu_enable_untrusted_mode, NULL, 1000) < 0) + if (mp_run_on_all_cpus(&cpu_enable_untrusted_mode, NULL, 1000 * USECS_PER_MSEC) < 0) printk(BIOS_ERR, "failed to enable untrusted mode\n"); }
diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c index 625d956..5677497 100644 --- a/src/soc/intel/apollolake/cpu.c +++ b/src/soc/intel/apollolake/cpu.c @@ -255,7 +255,7 @@ smm_southbridge_enable(PWRBTN_EN | GBL_EN);
if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) - mp_run_on_all_cpus(sgx_configure, NULL, 2000); + mp_run_on_all_cpus(sgx_configure, NULL, 100 * USECS_PER_MSEC); }
static const struct mp_ops mp_ops = { diff --git a/src/soc/intel/common/block/cpu/mp_init.c b/src/soc/intel/common/block/cpu/mp_init.c index 1241709..c95131e 100644 --- a/src/soc/intel/common/block/cpu/mp_init.c +++ b/src/soc/intel/common/block/cpu/mp_init.c @@ -158,7 +158,7 @@ if (CONFIG(USE_INTEL_FSP_MP_INIT)) return;
- if (mp_run_on_all_cpus(&wrapper_x86_setup_mtrrs, NULL, 1000) < 0) + if (mp_run_on_all_cpus(&wrapper_x86_setup_mtrrs, NULL, 1000 * USECS_PER_MSEC) < 0) printk(BIOS_ERR, "MTRR programming failure\n");
x86_mtrr_check(); diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c index a558447..b50d0d4 100644 --- a/src/soc/intel/skylake/cpu.c +++ b/src/soc/intel/skylake/cpu.c @@ -491,11 +491,11 @@ if (CONFIG(HAVE_SMI_HANDLER)) smm_lock();
- ret |= mp_run_on_all_cpus(vmx_configure, NULL, 2 * USECS_PER_MSEC); + ret |= mp_run_on_all_cpus(vmx_configure, NULL, 100 * USECS_PER_MSEC);
- ret |= mp_run_on_all_cpus(sgx_configure, NULL, 14 * USECS_PER_MSEC); + ret |= mp_run_on_all_cpus(sgx_configure, NULL, 1000 * USECS_PER_MSEC);
- ret |= mp_run_on_all_cpus(fc_lock_configure, NULL, 2 * USECS_PER_MSEC); + ret |= mp_run_on_all_cpus(fc_lock_configure, NULL, 1000 * USECS_PER_MSEC);
if (ret) printk(BIOS_CRIT, "CRITICAL ERROR: MP post init failed\n");
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34587
to look at the new patch set (#2).
Change subject: soc/mp_init: Fix timeout ......................................................................
soc/mp_init: Fix timeout
We ususally don't know how long specific code runs as serial console and it's spinlock might slow down APs.
Increase the timeout of most mp_run_on_all_cpus calls to 1 second, to prevent possible crashing of secondary APs still processing the old job.
Change-Id: I456be647b159f7a2ea7d94986a24424e56dcc8c4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/apollolake/chip.c M src/soc/intel/apollolake/cpu.c M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/skylake/cpu.c 7 files changed, 10 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34587/2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/mp_init: Fix timeout ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34587/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/2//COMMIT_MSG@7 PS2, Line 7: soc/mp_init: Fix timeout Maybe:
Increase time-out to 1 s
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34587
to look at the new patch set (#3).
Change subject: soc/*/mp_init: Increase timeout to 1 s ......................................................................
soc/*/mp_init: Increase timeout to 1 s
We ususally don't know how long specific code runs as serial console and it's spinlock might slow down APs.
Increase the timeout of most mp_run_on_all_cpus calls to 1 second, to prevent possible crashing of secondary APs still processing the old job.
Change-Id: I456be647b159f7a2ea7d94986a24424e56dcc8c4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/apollolake/chip.c M src/soc/intel/apollolake/cpu.c M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/skylake/cpu.c 7 files changed, 11 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34587/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*/mp_init: Increase timeout to 1 s ......................................................................
Patch Set 3: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*/mp_init: Increase timeout to 1 s ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG@9 PS3, Line 9: s usually
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG@10 PS3, Line 10: it's its
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/apollolake/cp... File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/apollolake/cp... PS3, Line 259: 100 100 or 1000?
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/skylake/cpu.c... PS3, Line 494: 100 same
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*/mp_init: Increase timeout to 1 s ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG@12 PS3, Line 12: most mp_run_on_all_cpus calls How did you decide which calls to increase the timeout for? Would it be better to add a function mp_run_all_default_timeout() which can be used by the callers when they don't care about having a special timeout value? And that can call mp_run_on_all_cpus() with 1 second timeout.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*/mp_init: Increase timeout to 1 s ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG@12 PS3, Line 12: most mp_run_on_all_cpus calls
How did you decide which calls to increase the timeout for? Would it be better to add a function mp_ […]
The first call in post_mp_init should never fail as all APs are idle, due to previous synchronisation in mp init.
Sounds good, but how to know if the caller doesn't care about a special timeout? Do we need a special (non default) timeout?
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/apollolake/cp... File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/apollolake/cp... PS3, Line 259: 100
100 or 1000?
was chosen as APs are idle at start of post_mp_init. That cannot be said for sure after post_mp_init.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*/mp_init: Increase timeout to 1 s ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG@12 PS3, Line 12: most mp_run_on_all_cpus calls
The first call in post_mp_init should never fail as all APs are idle, due to previous synchronisation in mp init.
Right. But that could change if another call is added before the first call.
Sounds good, but how to know if the caller doesn't care about a special timeout? Do we need a special (non default) timeout?
Fair point. 1 second is long enough that I think it should help all callers. How about getting rid of the expire_us parameter for now and just setting it to 1 second always? If there is a case that comes up in the future which requires more than a second, we can add a different API for that.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34587
to look at the new patch set (#4).
Change subject: soc/*: mp_init: Remove configurable timeout ......................................................................
soc/*: mp_init: Remove configurable timeout
Some timeouts given were too small when serial console is enabled due to it's spinlock making code runtime worse with every AP present.
In addition we ususally don't know how long specific code runs and how long ago it was send to the APs.
Remove the timeout argument and instead wait up to 1 second, to prevent possible crashing of secondary APs still processing the old job.
Tested on Supermicro X11SSH-TF.
Change-Id: I456be647b159f7a2ea7d94986a24424e56dcc8c4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/include/cpu/x86/mp.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/apollolake/chip.c M src/soc/intel/apollolake/cpu.c M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/skylake/cpu.c 8 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34587/4
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_init: Remove configurable timeout ......................................................................
Patch Set 4:
Removed the timeouts and used 1 second instead in a common place.
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34587
to look at the new patch set (#5).
Change subject: soc/*: mp_init: Remove configurable timeout ......................................................................
soc/*: mp_init: Remove configurable timeout
Some timeouts given were too small when serial console is enabled due to it's spinlock making code runtime worse with every AP present.
In addition we usually don't know how long specific code runs and how long ago it was send to the APs.
Remove the timeout argument and instead wait up to 1 second, to prevent possible crashing of secondary APs still processing the old job.
Tested on Supermicro X11SSH-TF.
Change-Id: I456be647b159f7a2ea7d94986a24424e56dcc8c4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/include/cpu/x86/mp.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/apollolake/chip.c M src/soc/intel/apollolake/cpu.c M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/skylake/cpu.c 8 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34587/5
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_init: Remove configurable timeout ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34587/5/src/include/cpu/x86/mp.h File src/include/cpu/x86/mp.h:
https://review.coreboot.org/c/coreboot/+/34587/5/src/include/cpu/x86/mp.h@13... PS5, Line 137: long expire_us Is it intentional that mp_run_on_aps still has the timeout but mp_run_on_all_cpus does not?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_init: Remove configurable timeout ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/34587/5/src/include/cpu/x86/mp.h File src/include/cpu/x86/mp.h:
https://review.coreboot.org/c/coreboot/+/34587/5/src/include/cpu/x86/mp.h@13... PS5, Line 137: long expire_us
Is it intentional that mp_run_on_aps still has the timeout but mp_run_on_all_cpus does not?
Yes. It's used by agesa wrapper and fsp2.0 mp init ppi, providing their own custom timeout. I don't want to touch those black-boxes.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_init: Remove configurable timeout ......................................................................
Patch Set 5: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/34587/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/5//COMMIT_MSG@7 PS5, Line 7: mp_init mp_run_on_all_cpus?
https://review.coreboot.org/c/coreboot/+/34587/5//COMMIT_MSG@15 PS5, Line 15: from mp_run_on_all_cpus()
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Duncan Laurie, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34587
to look at the new patch set (#6).
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
soc/*: mp_run_on_all_cpus: Remove configurable timeout
Some timeouts given were too small when serial console is enabled due to it's spinlock making code runtime worse with every AP present.
In addition we usually don't know how long specific code runs and how long ago it was send to the APs.
Remove the timeout argument from mp_run_on_all_cpus and instead wait up to 1 second, to prevent possible crashing of secondary APs still processing the old job.
Tested on Supermicro X11SSH-TF.
Change-Id: I456be647b159f7a2ea7d94986a24424e56dcc8c4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/include/cpu/x86/mp.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/apollolake/chip.c M src/soc/intel/apollolake/cpu.c M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/skylake/cpu.c 8 files changed, 12 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34587/6
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34587/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/5//COMMIT_MSG@7 PS5, Line 7: mp_init
mp_run_on_all_cpus?
Done
https://review.coreboot.org/c/coreboot/+/34587/5//COMMIT_MSG@15 PS5, Line 15:
from mp_run_on_all_cpus()
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34587/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/6//COMMIT_MSG@10 PS6, Line 10: it's its
https://review.coreboot.org/c/coreboot/+/34587/6//COMMIT_MSG@13 PS6, Line 13: send sent
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
Patch Set 6: Code-Review+2
Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Duncan Laurie, Philipp Deppenwiese, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34587
to look at the new patch set (#7).
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
soc/*: mp_run_on_all_cpus: Remove configurable timeout
Some timeouts given were too small when serial console is enabled due to its spinlock making code runtime worse with every AP present.
In addition we usually don't know how long specific code runs and how long ago it was sent to the APs.
Remove the timeout argument from mp_run_on_all_cpus and instead wait up to 1 second, to prevent possible crashing of secondary APs still processing the old job.
Tested on Supermicro X11SSH-TF.
Change-Id: I456be647b159f7a2ea7d94986a24424e56dcc8c4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/cpu/x86/mp_init.c M src/include/cpu/x86/mp.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/apollolake/chip.c M src/soc/intel/apollolake/cpu.c M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/skylake/cpu.c 8 files changed, 13 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/34587/7
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
Patch Set 7:
(2 comments)
https://review.coreboot.org/c/coreboot/+/34587/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/6//COMMIT_MSG@10 PS6, Line 10: it's
its
Done
https://review.coreboot.org/c/coreboot/+/34587/6//COMMIT_MSG@13 PS6, Line 13: send
sent
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
Patch Set 7: Code-Review+1
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
Patch Set 7: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
Patch Set 7:
(7 comments)
https://review.coreboot.org/c/coreboot/+/34587/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/2//COMMIT_MSG@7 PS2, Line 7: soc/mp_init: Fix timeout
Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG@9 PS3, Line 9: s
usually
Done
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG@10 PS3, Line 10: it's
its
Done
https://review.coreboot.org/c/coreboot/+/34587/3//COMMIT_MSG@12 PS3, Line 12: most mp_run_on_all_cpus calls
The first call in post_mp_init should never fail as all APs are idle, due to previous synchronisat […]
Done
https://review.coreboot.org/c/coreboot/+/34587/5/src/include/cpu/x86/mp.h File src/include/cpu/x86/mp.h:
https://review.coreboot.org/c/coreboot/+/34587/5/src/include/cpu/x86/mp.h@13... PS5, Line 137: long expire_us
Yes. It's used by agesa wrapper and fsp2.0 mp init ppi, providing their own custom timeout. […]
Done
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/apollolake/cp... File src/soc/intel/apollolake/cpu.c:
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/apollolake/cp... PS3, Line 259: 100
was chosen as APs are idle at start of post_mp_init. […]
Done
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/skylake/cpu.c File src/soc/intel/skylake/cpu.c:
https://review.coreboot.org/c/coreboot/+/34587/3/src/soc/intel/skylake/cpu.c... PS3, Line 494: 100
same
Done
Patrick Rudolph has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34587 )
Change subject: soc/*: mp_run_on_all_cpus: Remove configurable timeout ......................................................................
soc/*: mp_run_on_all_cpus: Remove configurable timeout
Some timeouts given were too small when serial console is enabled due to its spinlock making code runtime worse with every AP present.
In addition we usually don't know how long specific code runs and how long ago it was sent to the APs.
Remove the timeout argument from mp_run_on_all_cpus and instead wait up to 1 second, to prevent possible crashing of secondary APs still processing the old job.
Tested on Supermicro X11SSH-TF.
Change-Id: I456be647b159f7a2ea7d94986a24424e56dcc8c4 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34587 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com --- M src/cpu/x86/mp_init.c M src/include/cpu/x86/mp.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/apollolake/chip.c M src/soc/intel/apollolake/cpu.c M src/soc/intel/common/block/cpu/mp_init.c M src/soc/intel/skylake/cpu.c 8 files changed, 13 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Paul Menzel: Looks good to me, but someone else must approve Philipp Deppenwiese: Looks good to me, approved
diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 3658a5b..1755a9d 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -964,12 +964,13 @@ return run_ap_work(&lcb, expire_us); }
-int mp_run_on_all_cpus(void (*func)(void *), void *arg, long expire_us) +int mp_run_on_all_cpus(void (*func)(void *), void *arg) { /* Run on BSP first. */ func(arg);
- return mp_run_on_aps(func, arg, MP_RUN_ON_ALL_CPUS, expire_us); + /* For up to 1 second for AP to finish previous work. */ + return mp_run_on_aps(func, arg, MP_RUN_ON_ALL_CPUS, 1000 * USECS_PER_MSEC); }
int mp_park_aps(void) @@ -981,7 +982,7 @@ stopwatch_init(&sw);
ret = mp_run_on_aps(park_this_cpu, NULL, MP_RUN_ON_ALL_CPUS, - 250 * USECS_PER_MSEC); + 1000 * USECS_PER_MSEC);
duration_msecs = stopwatch_duration_msecs(&sw);
diff --git a/src/include/cpu/x86/mp.h b/src/include/cpu/x86/mp.h index c04252e..3ab45cd 100644 --- a/src/include/cpu/x86/mp.h +++ b/src/include/cpu/x86/mp.h @@ -137,7 +137,7 @@ long expire_us);
/* Like mp_run_on_aps() but also runs func on BSP. */ -int mp_run_on_all_cpus(void (*func)(void *), void *arg, long expire_us); +int mp_run_on_all_cpus(void (*func)(void *), void *arg);
/* * Park all APs to prepare for OS boot. This is handled automatically diff --git a/src/soc/amd/picasso/finalize.c b/src/soc/amd/picasso/finalize.c index 5a4627a..0ec7bd9 100644 --- a/src/soc/amd/picasso/finalize.c +++ b/src/soc/amd/picasso/finalize.c @@ -44,7 +44,7 @@ int r; printk(BIOS_SPEW, "Lock SMM configuration\n");
- r = mp_run_on_all_cpus(per_core_finalize, NULL, 10 * USECS_PER_MSEC); + r = mp_run_on_all_cpus(per_core_finalize, NULL); if (r) printk(BIOS_WARNING, "Failed to finalize all cores\n"); } diff --git a/src/soc/amd/stoneyridge/finalize.c b/src/soc/amd/stoneyridge/finalize.c index 5a4627a..0ec7bd9 100644 --- a/src/soc/amd/stoneyridge/finalize.c +++ b/src/soc/amd/stoneyridge/finalize.c @@ -44,7 +44,7 @@ int r; printk(BIOS_SPEW, "Lock SMM configuration\n");
- r = mp_run_on_all_cpus(per_core_finalize, NULL, 10 * USECS_PER_MSEC); + r = mp_run_on_all_cpus(per_core_finalize, NULL); if (r) printk(BIOS_WARNING, "Failed to finalize all cores\n"); } diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c index 361e6a4..b69f9ee 100644 --- a/src/soc/intel/apollolake/chip.c +++ b/src/soc/intel/apollolake/chip.c @@ -764,7 +764,7 @@ static void drop_privilege_all(void) { /* Drop privilege level on all the CPUs */ - if (mp_run_on_all_cpus(&cpu_enable_untrusted_mode, NULL, 1000) < 0) + if (mp_run_on_all_cpus(&cpu_enable_untrusted_mode, NULL) < 0) printk(BIOS_ERR, "failed to enable untrusted mode\n"); }
diff --git a/src/soc/intel/apollolake/cpu.c b/src/soc/intel/apollolake/cpu.c index 9fbcc07..8aa0804 100644 --- a/src/soc/intel/apollolake/cpu.c +++ b/src/soc/intel/apollolake/cpu.c @@ -256,7 +256,7 @@ smm_southbridge_enable(PWRBTN_EN | GBL_EN);
if (CONFIG(SOC_INTEL_COMMON_BLOCK_SGX)) - mp_run_on_all_cpus(sgx_configure, NULL, 2000); + mp_run_on_all_cpus(sgx_configure, NULL); }
static const struct mp_ops mp_ops = { diff --git a/src/soc/intel/common/block/cpu/mp_init.c b/src/soc/intel/common/block/cpu/mp_init.c index d941ab2..e0cee17 100644 --- a/src/soc/intel/common/block/cpu/mp_init.c +++ b/src/soc/intel/common/block/cpu/mp_init.c @@ -159,7 +159,7 @@ if (CONFIG(USE_INTEL_FSP_MP_INIT)) return;
- if (mp_run_on_all_cpus(&wrapper_x86_setup_mtrrs, NULL, 1000) < 0) + if (mp_run_on_all_cpus(&wrapper_x86_setup_mtrrs, NULL) < 0) printk(BIOS_ERR, "MTRR programming failure\n");
x86_mtrr_check(); diff --git a/src/soc/intel/skylake/cpu.c b/src/soc/intel/skylake/cpu.c index eecb004..383a3bd 100644 --- a/src/soc/intel/skylake/cpu.c +++ b/src/soc/intel/skylake/cpu.c @@ -491,11 +491,11 @@ if (CONFIG(HAVE_SMI_HANDLER)) smm_lock();
- ret |= mp_run_on_all_cpus(vmx_configure, NULL, 2 * USECS_PER_MSEC); + ret |= mp_run_on_all_cpus(vmx_configure, NULL);
- ret |= mp_run_on_all_cpus(sgx_configure, NULL, 14 * USECS_PER_MSEC); + ret |= mp_run_on_all_cpus(sgx_configure, NULL);
- ret |= mp_run_on_all_cpus(fc_lock_configure, NULL, 2 * USECS_PER_MSEC); + ret |= mp_run_on_all_cpus(fc_lock_configure, NULL);
if (ret) printk(BIOS_CRIT, "CRITICAL ERROR: MP post init failed\n");
-
Angel Pons (Code Review) -
Furquan Shaikh (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Subrata Banik (Code Review)