Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35783 )
Change subject: [WIP]sb/intel/common: Handle legacy save states properly ......................................................................
[WIP]sb/intel/common: Handle legacy save states properly
This moves southbridge_update_gnvs to a common place.
Older 32bit only CPU's have a different SMM save state, requiring a different handling of those.
Change-Id: I08391d4ee361ce75507c9de508cd75cc07249fd7 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/cpu/intel/em64t101_save_state.h M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/common/pmutil.h M src/southbridge/intel/common/smihandler.c M src/southbridge/intel/i82801gx/smihandler.c M src/southbridge/intel/i82801ix/smihandler.c M src/southbridge/intel/i82801jx/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c 9 files changed, 140 insertions(+), 75 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/35783/1
diff --git a/src/cpu/intel/smm/gen1/smmrelocate.c b/src/cpu/intel/smm/gen1/smmrelocate.c index 3eb869a..5350d1c 100644 --- a/src/cpu/intel/smm/gen1/smmrelocate.c +++ b/src/cpu/intel/smm/gen1/smmrelocate.c @@ -168,6 +168,9 @@ if (smm_reloc_params.ied_size) setup_ied_area(&smm_reloc_params);
+ /* This may not be be correct for older CPU's supported by this code, + but given that em64t101_smm_state_save_area_t is larger than the + save_state of these CPU's it works. */ *smm_save_state_size = sizeof(em64t101_smm_state_save_area_t); }
@@ -191,6 +194,8 @@ { msr_t mtrr_cap; struct smm_relocation_params *relo_params = &smm_reloc_params; + /* The em64t101 save state is sufficiently compatible with older + save states with regards of smbase, smm_revision. */ em64t101_smm_state_save_area_t *save_state; u32 smbase = staggered_smbase; u32 iedbase = relo_params->ied_base; @@ -208,7 +213,10 @@ save_state = (void *)(curr_smbase + SMM_DEFAULT_SIZE - sizeof(*save_state)); save_state->smbase = smbase; - save_state->iedbase = iedbase; + + printk(BIOS_SPEW, "SMM revision: 0x%08x\n", save_state->smm_revision); + if (save_state->smm_revision == 0x00030101) + save_state->iedbase = iedbase;
/* Write EMRR and SMRR MSRs based on indicated support. */ mtrr_cap = rdmsr(MTRR_CAP_MSR); diff --git a/src/include/cpu/intel/em64t101_save_state.h b/src/include/cpu/intel/em64t101_save_state.h index b8bb2db..7493c85 100644 --- a/src/include/cpu/intel/em64t101_save_state.h +++ b/src/include/cpu/intel/em64t101_save_state.h @@ -20,6 +20,7 @@
/* Intel Revision 30101 SMM State-Save Area * The following processor architectures use this: + * - Nehalem * - SandyBridge * - IvyBridge * - Haswell diff --git a/src/southbridge/intel/bd82x6x/smihandler.c b/src/southbridge/intel/bd82x6x/smihandler.c index ceac598..fe4f80e 100644 --- a/src/southbridge/intel/bd82x6x/smihandler.c +++ b/src/southbridge/intel/bd82x6x/smihandler.c @@ -21,7 +21,6 @@ #include <cpu/x86/cache.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> -#include <cpu/intel/em64t101_save_state.h> #include <northbridge/intel/sandybridge/sandybridge.h> #include <southbridge/intel/bd82x6x/me.h> #include <southbridge/intel/common/gpio.h> @@ -38,6 +37,11 @@ return gnvs; }
+void smm_set_gnvs(uintptr_t gnvs_ptr) +{ + gnvs = (global_nvs_t *)gnvs_ptr; +} + int southbridge_io_trap_handler(int smif) { switch (smif) { @@ -211,18 +215,6 @@ xhci_sleep(slp_type); }
-void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) -{ - em64t101_smm_state_save_area_t *state = - smi_apmc_find_state_save(apm_cnt); - if (state) { - /* EBX in the state save contains the GNVS pointer */ - gnvs = (global_nvs_t *)((u32)state->rbx); - *smm_done = 1; - printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); - } -} - void southbridge_finalize_all(void) { intel_me_finalize_smm(); diff --git a/src/southbridge/intel/common/pmutil.h b/src/southbridge/intel/common/pmutil.h index cea5c82..b2f1dc4 100644 --- a/src/southbridge/intel/common/pmutil.h +++ b/src/southbridge/intel/common/pmutil.h @@ -18,7 +18,9 @@ #define INTEL_COMMON_PMUTIL_H
#include <cpu/x86/smm.h> +#include <cpu/x86/legacy_save_state.h> #include <cpu/intel/em64t101_save_state.h> +#include <cpu/intel/em64t_save_state.h>
#define D31F0_PMBASE 0x40 #define D31F0_GEN_PMCON_1 0xa0 @@ -142,10 +144,11 @@ void southbridge_smm_xhci_sleep(u8 slp_type); void gpi_route_interrupt(u8 gpi, u8 mode); void southbridge_gate_memory_reset(void); -void southbridge_update_gnvs(u8 apm_cnt, int *smm_done); +void smm_set_gnvs_ptr(uintptr_t gnvs); void southbridge_finalize_all(void); void southbridge_smi_monitor(void); -em64t101_smm_state_save_area_t *smi_apmc_find_state_save(u8 cmd); + +void *smi_apmc_find_state_save(u8 cmd); void pch_log_state(void);
#endif /*INTEL_COMMON_PMUTIL_H */ diff --git a/src/southbridge/intel/common/smihandler.c b/src/southbridge/intel/common/smihandler.c index d61238c..731856a 100644 --- a/src/southbridge/intel/common/smihandler.c +++ b/src/southbridge/intel/common/smihandler.c @@ -210,37 +210,77 @@ } }
+enum save_state_type { + EM64T, + EM64T101, + LEGACY +}; + +static enum save_state_type get_save_state_type(void) +{ + /* Is geting this on node 0 fine??? */ + void *save_state = smm_get_save_state(0); + u32 smm_revision = ((em64t101_smm_state_save_area_t *)state)->smm_revision; + switch (smm_revision) { + case 0x00030002: + case 0x00030007: + return LEGACY; + case 0x00030100: + return EM64T; + default: + printk(BIOS_WARNING, "Unknown smm_revision...\n"); + /* Fallthrough */ + case 0x00030101: /* Nahalem, SandyBridge, IvyBridge, and Haswell */ + return EM64T101; + } +} + /* * Look for Synchronous IO SMI and use save state from that * core in case we are not running on the same core that * initiated the IO transaction. */ -em64t101_smm_state_save_area_t *smi_apmc_find_state_save(u8 cmd) +void *smi_apmc_find_state_save(u8 cmd) { - em64t101_smm_state_save_area_t *state; int node; + enum save_state_type save_state_type = get_save_state_type();
- /* Check all nodes looking for the one that issued the IO */ - for (node = 0; node < CONFIG_MAX_CPUS; node++) { - state = smm_get_save_state(node); + if (save_state_type == EM64T || save_state_type == EM64T101) { + /* Check all nodes looking for the one that issued the IO. + TODO: Check against actual amount of CPUs!! */ + for (node = 0; node < CONFIG_MAX_CPUS; node++) { + em64t101_smm_state_save_area_t *state = + (em64t101_smm_state_save_area_t *)smm_get_save_state(node);
- /* Check for Synchronous IO (bit0 == 1) */ - if (!(state->io_misc_info & (1 << 0))) - continue; + /* Check for Synchronous IO (bit0 == 1) */ + if (!(state->io_misc_info & (1 << 0))) + continue;
- /* Make sure it was a write (bit4 == 0) */ - if (state->io_misc_info & (1 << 4)) - continue; + /* Make sure it was a write (bit4 == 0) */ + if (state->io_misc_info & (1 << 4)) + continue;
- /* Check for APMC IO port */ - if (((state->io_misc_info >> 16) & 0xff) != APM_CNT) - continue; + /* Check for APMC IO port */ + if (((state->io_misc_info >> 16) & 0xff) != APM_CNT) + continue;
- /* Check AX against the requested command */ - if ((state->rax & 0xff) != cmd) - continue; + /* Check AX against the requested command */ + if ((state->rax & 0xff) != cmd) + continue;
- return state; + return state; + } + } + + if (save_state_type == LEGACY) { + for (node = 0; node < CONFIG_MAX_CPUS; node++) { + legacy_smm_state_save_area_t *state = + (legacy_smm_state_save_area_t *)smm_get_save_state(node); + + /* Check AX against the requested command */ + if ((state->eax & 0xff) == cmd) + return state; + } }
return NULL; @@ -272,23 +312,62 @@ static void southbridge_smi_store(void) { u8 sub_command, ret; - em64t101_smm_state_save_area_t *io_smi = - smi_apmc_find_state_save(APM_CNT_SMMSTORE); + void *state = smi_apmc_find_state_save(APM_CNT_SMMSTORE); uint32_t reg_ebx;
- if (!io_smi) + if (!state) return; /* Command and return value in EAX */ - sub_command = (io_smi->rax >> 8) & 0xff; + switch (get_save_state_type()) { + case EM64T: + case EM64T101: + sub_command = ((u32)((em64t101_smm_state_save_area_t *)state)->rax >> 8) & 0xff;
- /* Parameter buffer in EBX */ - reg_ebx = io_smi->rbx; + reg_ebx = (u32)((em64t101_smm_state_save_area_t *)state)->rbx; + break; + case LEGACY: + sub_command = (((legacy_smm_state_save_area_t *)state)->eax >> 8) & 0xff; + reg_ebx = ((legacy_smm_state_save_area_t *)state)->ebx; + break; + }
- /* drivers/smmstore/smi.c */ + /* drivers/smmstore/smi.c, parameter buffer in EBX */ ret = smmstore_exec(sub_command, (uintptr_t *)reg_ebx); - io_smi->rax = ret; + + switch (get_save_state_type()) { + case EM64T: + case EM64T101: + ((em64t101_smm_state_save_area_t *)state)->rax = ret; + break; + case LEGACY: + ((legacy_smm_state_save_area_t *)state)->eax = ret; + break; + } }
+static void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) +{ + uintptr_t gnvs = 0; + void *state = smi_apmc_find_state_save(apm_cnt); + if (state) { + switch (get_save_state_type()) { + case EM64T: + case EM64T101: + /* EBX in the state save contains the GNVS pointer */ + gnvs = (u32)((em64t101_smm_state_save_area_t *)state)->rbx; + break; + case LEGACY: + gnvs = ((legacy_smm_state_save_area_t *)state)->ebx; + break; + } + *smm_done = 1; + printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); + } + if (gnvs) + smm_set_gnvs_ptr(gnvs); +} + + static int mainboard_finalized = 0;
static void southbridge_smi_apmc(void) diff --git a/src/southbridge/intel/i82801gx/smihandler.c b/src/southbridge/intel/i82801gx/smihandler.c index 16ceb13..fccc67d 100644 --- a/src/southbridge/intel/i82801gx/smihandler.c +++ b/src/southbridge/intel/i82801gx/smihandler.c @@ -41,12 +41,11 @@ /* GNVS needs to be updated by an 0xEA PM Trap (B2) after it has been located * by coreboot. */ -global_nvs_t *gnvs = (global_nvs_t *)0x0; +static global_nvs_t *gnvs;
-void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) +void smm_set_gnvs_ptr(uintptr_t gnvs_ptr) { - gnvs = *(global_nvs_t **)0x500; - *smm_done = 1; + gnvs = (global_nvs_t *)gnvs; }
int southbridge_io_trap_handler(int smif) diff --git a/src/southbridge/intel/i82801ix/smihandler.c b/src/southbridge/intel/i82801ix/smihandler.c index 8090a09..7135425 100644 --- a/src/southbridge/intel/i82801ix/smihandler.c +++ b/src/southbridge/intel/i82801ix/smihandler.c @@ -28,9 +28,7 @@ /* GNVS needs to be updated by an 0xEA PM Trap (B2) after it has been located * by coreboot. */ -global_nvs_t *gnvs = (global_nvs_t *)0x0; -void *tcg = (void *)0x0; -void *smi1 = (void *)0x0; +static global_nvs_t *gnvs;
int southbridge_io_trap_handler(int smif) { @@ -49,12 +47,9 @@ return 0; }
-void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) +void smm_set_gnvs_ptr(uintptr_t gnvs_ptr) { - gnvs = *(global_nvs_t **)0x500; - tcg = *(void **)0x504; - smi1 = *(void **)0x508; - *smm_done = 1; + gnvs = (global_nvs_t *)gnvs; }
void southbridge_smi_monitor(void) diff --git a/src/southbridge/intel/i82801jx/smihandler.c b/src/southbridge/intel/i82801jx/smihandler.c index 667a853..d9e944d 100644 --- a/src/southbridge/intel/i82801jx/smihandler.c +++ b/src/southbridge/intel/i82801jx/smihandler.c @@ -35,9 +35,7 @@ /* GNVS needs to be updated by an 0xEA PM Trap (B2) after it has been located * by coreboot. */ -global_nvs_t *gnvs = (global_nvs_t *)0x0; -void *tcg = (void *)0x0; -void *smi1 = (void *)0x0; +static global_nvs_t *gnvs;
int southbridge_io_trap_handler(int smif) { @@ -56,12 +54,9 @@ return 0; }
-void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) +void smm_set_gnvs_ptr(uintptr_t gnvs_ptr) { - gnvs = *(global_nvs_t **)0x500; - tcg = *(void **)0x504; - smi1 = *(void **)0x508; - *smm_done = 1; + gnvs = (global_nvs_t *)gnvs; }
void southbridge_smi_monitor(void) diff --git a/src/southbridge/intel/ibexpeak/smihandler.c b/src/southbridge/intel/ibexpeak/smihandler.c index 3668842..cc32127 100644 --- a/src/southbridge/intel/ibexpeak/smihandler.c +++ b/src/southbridge/intel/ibexpeak/smihandler.c @@ -21,7 +21,6 @@ #include <cpu/x86/cache.h> #include <device/pci_def.h> #include <cpu/x86/smm.h> -#include <cpu/intel/em64t101_save_state.h> #include <halt.h> #include <pc80/mc146818rtc.h> #include <cpu/intel/model_2065x/model_2065x.h> @@ -44,11 +43,17 @@ * by coreboot. */ static global_nvs_t *gnvs; + global_nvs_t *smm_get_gnvs(void) { return gnvs; }
+void smm_set_gnvs_ptr(uintptr_t gnvs_ptr) +{ + gnvs = (global_nvs_t *)gnvs; +} + int southbridge_io_trap_handler(int smif) { switch (smif) { @@ -171,18 +176,6 @@ #undef IOTRAP }
-void southbridge_update_gnvs(u8 apm_cnt, int *smm_done) -{ - em64t101_smm_state_save_area_t *state = - smi_apmc_find_state_save(apm_cnt); - if (state) { - /* EBX in the state save contains the GNVS pointer */ - gnvs = (global_nvs_t *)((u32)state->rbx); - *smm_done = 1; - printk(BIOS_DEBUG, "SMI#: Setting GNVS to %p\n", gnvs); - } -} - void southbridge_finalize_all(void) { intel_me_finalize_smm();
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35783 )
Change subject: [WIP]sb/intel/common: Handle legacy save states properly ......................................................................
Patch Set 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35783/1/src/southbridge/intel/commo... File src/southbridge/intel/common/smihandler.c:
https://review.coreboot.org/c/coreboot/+/35783/1/src/southbridge/intel/commo... PS1, Line 229: return EM64T; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35783/1/src/southbridge/intel/commo... PS1, Line 351: void *state = smi_apmc_find_state_save(apm_cnt); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35783/1/src/southbridge/intel/commo... PS1, Line 351: void *state = smi_apmc_find_state_save(apm_cnt); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35783/1/src/southbridge/intel/commo... PS1, Line 353: switch (get_save_state_type()) { code indent should use tabs where possible
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35783
to look at the new patch set (#2).
Change subject: [WIP]sb/intel/common/smihandler: Handle legacy save states properly ......................................................................
[WIP]sb/intel/common/smihandler: Handle legacy save states properly
This moves southbridge_update_gnvs to a common place.
Older 32bit only CPU's have a different SMM save state, requiring a different handling of those.
Change-Id: I08391d4ee361ce75507c9de508cd75cc07249fd7 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/cpu/intel/em64t101_save_state.h M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/common/pmutil.h M src/southbridge/intel/common/smihandler.c M src/southbridge/intel/i82801gx/smihandler.c M src/southbridge/intel/i82801ix/smihandler.c M src/southbridge/intel/i82801jx/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c 9 files changed, 140 insertions(+), 75 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/35783/2
Hello Aaron Durbin, Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35783
to look at the new patch set (#3).
Change subject: [WIP]sb/intel/common/smihandler: Handle legacy save states properly ......................................................................
[WIP]sb/intel/common/smihandler: Handle legacy save states properly
This moves southbridge_update_gnvs to a common place.
Older 32bit only CPU's have a different SMM save state, requiring a different handling of those.
Change-Id: I08391d4ee361ce75507c9de508cd75cc07249fd7 Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/cpu/intel/smm/gen1/smmrelocate.c M src/include/cpu/intel/em64t101_save_state.h M src/southbridge/intel/bd82x6x/smihandler.c M src/southbridge/intel/common/pmutil.h M src/southbridge/intel/common/smihandler.c M src/southbridge/intel/i82801gx/smihandler.c M src/southbridge/intel/i82801ix/smihandler.c M src/southbridge/intel/i82801jx/smihandler.c M src/southbridge/intel/ibexpeak/smihandler.c 9 files changed, 140 insertions(+), 75 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/35783/3
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35783 )
Change subject: [WIP]sb/intel/common/smihandler: Handle legacy save states properly ......................................................................
Patch Set 3:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35783/3/src/southbridge/intel/commo... File src/southbridge/intel/common/smihandler.c:
https://review.coreboot.org/c/coreboot/+/35783/3/src/southbridge/intel/commo... PS3, Line 229: return EM64T; code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35783/3/src/southbridge/intel/commo... PS3, Line 351: void *state = smi_apmc_find_state_save(apm_cnt); code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/35783/3/src/southbridge/intel/commo... PS3, Line 351: void *state = smi_apmc_find_state_save(apm_cnt); please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/35783/3/src/southbridge/intel/commo... PS3, Line 353: switch (get_save_state_type()) { code indent should use tabs where possible
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35783 )
Change subject: [WIP]sb/intel/common/smihandler: Handle legacy save states properly ......................................................................
Patch Set 3:
Arthur, I've got some fairly large refactor pending on SMM. Let me rebase that and see if this is already addressed.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35783 )
Change subject: [WIP]sb/intel/common/smihandler: Handle legacy save states properly ......................................................................
Patch Set 3:
Patch Set 3:
Arthur, I've got some fairly large refactor pending on SMM. Let me rebase that and see if this is already addressed.
Ok thx. This code felt pretty hacky.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35783 )
Change subject: [WIP]sb/intel/common/smihandler: Handle legacy save states properly ......................................................................
Patch Set 3:
Patch Set 3:
Arthur, I've got some fairly large refactor pending on SMM. Let me rebase that and see if this is already addressed.
Any updates on this?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35783 )
Change subject: [WIP]sb/intel/common/smihandler: Handle legacy save states properly ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 3:
Arthur, I've got some fairly large refactor pending on SMM. Let me rebase that and see if this is already addressed.
Any updates on this?
Not really. This is probably orthogonal work so go ahead with it, I'll rebase on your work when I get back to SMM branch.
Arthur Heymans has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/35783 )
Change subject: [WIP]sb/intel/common/smihandler: Handle legacy save states properly ......................................................................
Abandoned
CB:36590 attempts to abstract the different save states.
-
Arthur Heymans (Code Review) -
build bot (Jenkins) (Code Review)
-
Kyösti Mälkki (Code Review)