Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
[WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE
At least google/stout has APM_CNT_FINALIZE handler in mainboard, so mainboard_smi_apmc() should be an unconditional call.
Change-Id: I7b6450e0c8436e1c6113d54b7987b6b486908ed9 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/lpc.c M src/southbridge/intel/lynxpoint/smihandler.c 6 files changed, 21 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41992/1
diff --git a/src/southbridge/intel/bd82x6x/lpc.c b/src/southbridge/intel/bd82x6x/lpc.c index 7bfde3f..170f4b4 100644 --- a/src/southbridge/intel/bd82x6x/lpc.c +++ b/src/southbridge/intel/bd82x6x/lpc.c @@ -827,10 +827,7 @@ spi_finalize_ops();
/* Call SMM finalize() handlers before resume */ - if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || - acpi_is_wakeup_s3()) { - apm_control(APM_CNT_FINALIZE); - } + apm_control(APM_CNT_FINALIZE); }
void intel_southbridge_override_spi( diff --git a/src/southbridge/intel/bd82x6x/smihandler.c b/src/southbridge/intel/bd82x6x/smihandler.c index f778f7a..721cfe4 100644 --- a/src/southbridge/intel/bd82x6x/smihandler.c +++ b/src/southbridge/intel/bd82x6x/smihandler.c @@ -210,6 +210,9 @@
void southbridge_finalize_all(void) { + if (!CONFIG(INTEL_CHIPSET_LOCKDOWN) && !acpi_is_wakeup_s3()) + return; + intel_me_finalize_smm(); intel_pch_finalize_smm(); intel_sandybridge_finalize_smm(); diff --git a/src/southbridge/intel/ibexpeak/lpc.c b/src/southbridge/intel/ibexpeak/lpc.c index 9a115f7..bdfd8e0 100644 --- a/src/southbridge/intel/ibexpeak/lpc.c +++ b/src/southbridge/intel/ibexpeak/lpc.c @@ -720,10 +720,7 @@ spi_finalize_ops();
/* Call SMM finalize() handlers before resume */ - if (CONFIG(INTEL_CHIPSET_LOCKDOWN) || - acpi_is_wakeup_s3()) { - apm_control(APM_CNT_FINALIZE); - } + apm_control(APM_CNT_FINALIZE); }
static struct pci_operations pci_ops = { diff --git a/src/southbridge/intel/ibexpeak/smihandler.c b/src/southbridge/intel/ibexpeak/smihandler.c index a881f33..b8ff520 100644 --- a/src/southbridge/intel/ibexpeak/smihandler.c +++ b/src/southbridge/intel/ibexpeak/smihandler.c @@ -168,6 +168,9 @@
void southbridge_finalize_all(void) { + if (!CONFIG(INTEL_CHIPSET_LOCKDOWN) && !acpi_is_wakeup_s3()) + return; + intel_me_finalize_smm(); intel_pch_finalize_smm(); intel_ironlake_finalize_smm(); diff --git a/src/southbridge/intel/lynxpoint/lpc.c b/src/southbridge/intel/lynxpoint/lpc.c index d3a066e..782ecb5 100644 --- a/src/southbridge/intel/lynxpoint/lpc.c +++ b/src/southbridge/intel/lynxpoint/lpc.c @@ -913,8 +913,7 @@ { spi_finalize_ops();
- if (acpi_is_wakeup_s3() || CONFIG(INTEL_CHIPSET_LOCKDOWN)) - apm_control(APM_CNT_FINALIZE); + apm_control(APM_CNT_FINALIZE); }
static struct pci_operations pci_ops = { diff --git a/src/southbridge/intel/lynxpoint/smihandler.c b/src/southbridge/intel/lynxpoint/smihandler.c index 8bbe3fe..74e0802 100644 --- a/src/southbridge/intel/lynxpoint/smihandler.c +++ b/src/southbridge/intel/lynxpoint/smihandler.c @@ -287,11 +287,7 @@ return; }
- intel_me_finalize_smm(); - intel_pch_finalize_smm(); - intel_northbridge_haswell_finalize_smm(); - intel_cpu_haswell_finalize_smm(); - + southbridge_finalize_all(); chipset_finalized = 1; break; case APM_CNT_CST_CONTROL: @@ -556,3 +552,14 @@ } } } + +void southbridge_finalize_all(void) +{ + if (!CONFIG(INTEL_CHIPSET_LOCKDOWN) && !acpi_is_wakeup_s3()) + return; + + intel_me_finalize_smm(); + intel_pch_finalize_smm(); + intel_northbridge_haswell_finalize_smm(); + intel_cpu_haswell_finalize_smm(); +}
Hello build bot (Jenkins), Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41992
to look at the new patch set (#2).
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
[WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE
At least google/stout has APM_CNT_FINALIZE handler in mainboard, so mainboard_smi_apmc() should be an unconditional call.
TBD: ENV_SMM needs acpi_is_wakeup_s3()
Change-Id: I7b6450e0c8436e1c6113d54b7987b6b486908ed9 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/include/acpi/acpi.h M src/southbridge/intel/bd82x6x/lpc.c M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/ibexpeak/lpc.c M src/southbridge/intel/ibexpeak/smihandler.c M src/southbridge/intel/lynxpoint/lpc.c M src/southbridge/intel/lynxpoint/smihandler.c 7 files changed, 25 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/92/41992/2
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41992/2/src/include/acpi/acpi.h File src/include/acpi/acpi.h:
https://review.coreboot.org/c/coreboot/+/41992/2/src/include/acpi/acpi.h@103... PS2, Line 1035: static inline int acpi_is_wakeup_s3(void) { return 0; } We don't access CBMEM from ENV_SMM. So we don't have a usable romstage_handoff_is_resume() either.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
Patch Set 2:
(1 comment)
Sorry, the name `INTEL_CHIPSET_LOCKDOWN` is probably not descriptive enough. But it's prompt and help text should make it clear.
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... PS2, Line 214: return; This makes it impossible to call it from the payload. Which was the original reason to put it into SMM, AFAICT.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... PS2, Line 214: return;
This makes it impossible to call it from the payload. Which was the original […]
How so? With INTEL_CHIPSET_LOCKDOWN=n it returns for normal boot path.
AFAICS this would not change.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... PS2, Line 214: return;
How so? With INTEL_CHIPSET_LOCKDOWN=n it returns for normal boot path. […]
Sorry, wrong words. It's impossible for the payload to trigger execution of the code below (with INTEL_CHIPSET_LOCKDOWN=n).
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... PS2, Line 214: return;
Sorry, wrong words. It's impossible for the payload to trigger execution […]
Ahh.. I did not realize payload has to do this via SMI. I still think it is wrong that raising APM_CNT_FINALIZE is not consistent across platforms. But it would only be an issue if we needed mainboard_smi_apmc() do something at finalize.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... PS2, Line 214: return;
Ahh.. I did not realize payload has to do this via SMI. […]
Ok.. Can we do something like this instead?
static int once;
if (!once) { once++; if (!CONFIG(INTEL_CHIPSET_LOCKDOWN)) return; }
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... File src/southbridge/intel/bd82x6x/smihandler.c:
https://review.coreboot.org/c/coreboot/+/41992/2/src/southbridge/intel/bd82x... PS2, Line 214: return;
Ok.. Can we do something like this instead? […]
Should work. I also wonder if we shouldn't just drop INTEL_CHIPSET_LOCKDOWN (i.e. always lock). We (secunet) used it to defer flash lockdown into the payload. Which could also be handled more fine grained. But originally the finalize handler was called by depthcharge, I guess. Don't know what the requirements are there (or if anybody still cares).
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41992 )
Change subject: [WIP] sb/intel: Always raise SMI with APM_CNT_FINALIZE ......................................................................
Abandoned
-
Kyösti Mälkki (Code Review) -
Nico Huber (Code Review)