Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36827 )
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
arch/x86: Declare lock_smm_on_exit()
The condition to set SMRAM lock may not be a simple and single Kconfig after followup changes. Wrap it such that all platforms will have equal conditions.
Change-Id: Ic1878120256e968a2d2f58225d1cf7c5aaf10961 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/amd/agesa/family15tn/model_15_init.c M src/cpu/amd/agesa/family16kb/model_16_init.c M src/include/cpu/intel/smm_reloc.h M src/include/cpu/x86/smm.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/fsp_broadwell_de/cpu.c 7 files changed, 30 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36827/1
diff --git a/src/cpu/amd/agesa/family15tn/model_15_init.c b/src/cpu/amd/agesa/family15tn/model_15_init.c index be3d58b..25b8b83 100644 --- a/src/cpu/amd/agesa/family15tn/model_15_init.c +++ b/src/cpu/amd/agesa/family15tn/model_15_init.c @@ -115,9 +115,11 @@ }
/* Write protect SMM space with SMMLOCK. */ - msr = rdmsr(HWCR_MSR); - msr.lo |= (1 << 0); - wrmsr(HWCR_MSR, msr); + if (lock_smm_on_exit()) { + msr = rdmsr(HWCR_MSR); + msr.lo |= SMM_LOCK; + wrmsr(HWCR_MSR, msr); + } }
static struct device_operations cpu_dev_ops = { diff --git a/src/cpu/amd/agesa/family16kb/model_16_init.c b/src/cpu/amd/agesa/family16kb/model_16_init.c index 3d53b51..c9c0882 100644 --- a/src/cpu/amd/agesa/family16kb/model_16_init.c +++ b/src/cpu/amd/agesa/family16kb/model_16_init.c @@ -98,9 +98,11 @@ wrmsr(NB_CFG_MSR, msr);
/* Write protect SMM space with SMMLOCK. */ - msr = rdmsr(HWCR_MSR); - msr.lo |= (1 << 0); - wrmsr(HWCR_MSR, msr); + if (lock_smm_on_exit()) { + msr = rdmsr(HWCR_MSR); + msr.lo |= SMM_LOCK; + wrmsr(HWCR_MSR, msr); + } }
static struct device_operations cpu_dev_ops = { diff --git a/src/include/cpu/intel/smm_reloc.h b/src/include/cpu/intel/smm_reloc.h index 7b5e82e..4a64576 100644 --- a/src/include/cpu/intel/smm_reloc.h +++ b/src/include/cpu/intel/smm_reloc.h @@ -18,6 +18,7 @@ #include <types.h> #include <cpu/x86/msr.h> #include <cpu/x86/mtrr.h> +#include <cpu/x86/smm.h>
struct smm_relocation_params { uintptr_t ied_base; @@ -59,6 +60,9 @@
static inline void smm_lock(void) { + if (!lock_smm_on_exit()) + return; + /* LOCK the SMM memory window and enable normal SMM. * After running this function, only a full reset can * make the SMM registers writable again. diff --git a/src/include/cpu/x86/smm.h b/src/include/cpu/x86/smm.h index cf107b1..c3e1b9c 100644 --- a/src/include/cpu/x86/smm.h +++ b/src/include/cpu/x86/smm.h @@ -57,6 +57,11 @@ int mainboard_smi_apmc(u8 data); void mainboard_smi_sleep(u8 slp_typ);
+static inline int lock_smm_on_exit(void) +{ + return CONFIG(HAVE_SMI_HANDLER); +} + /* This is the SMM handler. */ extern unsigned char _binary_smm_start[]; extern unsigned char _binary_smm_end[]; diff --git a/src/soc/amd/picasso/finalize.c b/src/soc/amd/picasso/finalize.c index 5ea52c6..947337d 100644 --- a/src/soc/amd/picasso/finalize.c +++ b/src/soc/amd/picasso/finalize.c @@ -37,8 +37,10 @@ wrmsr(SMM_MASK_MSR, mask); }
- hwcr.lo |= SMM_LOCK; - wrmsr(HWCR_MSR, hwcr); + if (lock_smm_on_exit()) { + hwcr.lo |= SMM_LOCK; + wrmsr(HWCR_MSR, hwcr); + } }
static void finalize_cores(void) diff --git a/src/soc/amd/stoneyridge/finalize.c b/src/soc/amd/stoneyridge/finalize.c index 5ea52c6..947337d 100644 --- a/src/soc/amd/stoneyridge/finalize.c +++ b/src/soc/amd/stoneyridge/finalize.c @@ -37,8 +37,10 @@ wrmsr(SMM_MASK_MSR, mask); }
- hwcr.lo |= SMM_LOCK; - wrmsr(HWCR_MSR, hwcr); + if (lock_smm_on_exit()) { + hwcr.lo |= SMM_LOCK; + wrmsr(HWCR_MSR, hwcr); + } }
static void finalize_cores(void) diff --git a/src/soc/intel/fsp_broadwell_de/cpu.c b/src/soc/intel/fsp_broadwell_de/cpu.c index 1b69b1f..d0d2e1b 100644 --- a/src/soc/intel/fsp_broadwell_de/cpu.c +++ b/src/soc/intel/fsp_broadwell_de/cpu.c @@ -124,6 +124,9 @@ struct device *dev = pcidev_on_root(LPC_DEV, LPC_FUNC); uint16_t smi_lock;
+ if (!lock_smm_on_exit()) + return; + /* There is no register to lock SMRAM region on Broadwell-DE. Use this function to lock the SMI control bits. */ printk(BIOS_DEBUG, "Locking SMM.\n");
Hello Patrick Rudolph, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36827
to look at the new patch set (#2).
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
arch/x86: Declare lock_smm_on_exit()
The condition to set SMRAM lock may not be a simple and single Kconfig after followup changes. Wrap it such that all platforms will have equal conditions.
Change-Id: Ic1878120256e968a2d2f58225d1cf7c5aaf10961 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/amd/agesa/family15tn/model_15_init.c M src/cpu/amd/agesa/family16kb/model_16_init.c M src/include/cpu/intel/smm_reloc.h M src/include/cpu/x86/smm.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/fsp_broadwell_de/cpu.c 7 files changed, 31 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36827/2
Hello Patrick Rudolph, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36827
to look at the new patch set (#4).
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
arch/x86: Declare lock_smm_on_exit()
The condition to set SMRAM lock may not be a simple and single Kconfig after followup changes. Wrap it such that all platforms will have equal conditions.
Change-Id: Ic1878120256e968a2d2f58225d1cf7c5aaf10961 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/amd/agesa/family15tn/model_15_init.c M src/cpu/amd/agesa/family16kb/model_16_init.c M src/include/cpu/intel/smm_reloc.h M src/include/cpu/x86/smm.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c M src/soc/intel/fsp_broadwell_de/cpu.c 7 files changed, 31 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36827/4
Hello Patrick Rudolph, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36827
to look at the new patch set (#5).
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
arch/x86: Declare lock_smm_on_exit()
The condition to set SMRAM lock may not be a simple and single Kconfig after followup changes. Wrap it such that all platforms will have equal conditions.
Change-Id: Ic1878120256e968a2d2f58225d1cf7c5aaf10961 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/amd/agesa/family15tn/model_15_init.c M src/cpu/amd/agesa/family16kb/model_16_init.c M src/include/cpu/intel/smm_reloc.h M src/include/cpu/x86/smm.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c 6 files changed, 28 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36827/5
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36827 )
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
Patch Set 6: Code-Review+2
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36827 )
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
Abandoned
Lack of vendor feedback for 2 months on APL.
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/36827 )
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
Restored
Hello Philipp Deppenwiese, build bot (Jenkins), Marshall Dawson, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36827
to look at the new patch set (#9).
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
arch/x86: Declare lock_smm_on_exit()
The condition to set SMRAM lock may not be a simple and single Kconfig after followup changes. Wrap it such that all platforms will have equal conditions.
Change-Id: Ic1878120256e968a2d2f58225d1cf7c5aaf10961 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/amd/agesa/family15tn/model_15_init.c M src/cpu/amd/agesa/family16kb/model_16_init.c M src/include/cpu/intel/smm_reloc.h M src/include/cpu/x86/smm.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c 6 files changed, 28 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36827/9
Hello Philipp Deppenwiese, build bot (Jenkins), Marshall Dawson, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36827
to look at the new patch set (#11).
Change subject: arch/x86: Declare lock_smm_on_exit() ......................................................................
arch/x86: Declare lock_smm_on_exit()
The condition to set SMRAM lock may not be a simple and single Kconfig after followup changes. Wrap it such that all platforms will have equal conditions.
Change-Id: Ic1878120256e968a2d2f58225d1cf7c5aaf10961 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/amd/agesa/family15tn/model_15_init.c M src/cpu/amd/agesa/family16kb/model_16_init.c M src/include/cpu/intel/smm_reloc.h M src/include/cpu/x86/smm.h M src/soc/amd/picasso/finalize.c M src/soc/amd/stoneyridge/finalize.c 6 files changed, 28 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/27/36827/11
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36827 )
Change subject: cpu/x86/smm: Improve SMRAM locking ......................................................................
Patch Set 16:
This change is ready for review.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36827 )
Change subject: cpu/x86/smm: Improve SMRAM locking ......................................................................
Patch Set 16:
(6 comments)
https://review.coreboot.org/c/coreboot/+/36827/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36827/16//COMMIT_MSG@13 PS16, Line 13: SMM_LOCK_ON_1ST suggestion: what about something like SMM_LOCK_EARLY?
https://review.coreboot.org/c/coreboot/+/36827/16//COMMIT_MSG@18 PS16, Line 18: SMM_LOCK_ON_2ND suggestion: what about SMM_LOCK_ON_MP_INIT
https://review.coreboot.org/c/coreboot/+/36827/16/src/cpu/x86/mp_init.c File src/cpu/x86/mp_init.c:
https://review.coreboot.org/c/coreboot/+/36827/16/src/cpu/x86/mp_init.c@1099 PS16, Line 1099: smm_close_2nd(); Just checking, is it OK to do this before `restore_default_smm_area()` ? The default function there memcpy()'s into SMM_DEFAULT_BASE
https://review.coreboot.org/c/coreboot/+/36827/16/src/cpu/x86/smm/smm_util.c File src/cpu/x86/smm/smm_util.c:
https://review.coreboot.org/c/coreboot/+/36827/16/src/cpu/x86/smm/smm_util.c... PS16, Line 7: smram_lock smm_lock
https://review.coreboot.org/c/coreboot/+/36827/16/src/cpu/x86/smm/smm_util.c... PS16, Line 66: if (lock_smm_on_exit()) `CONFIG(SMM_LOCK_ON_2ND) && lock_smm_on_exit()` ?
https://review.coreboot.org/c/coreboot/+/36827/16/src/cpu/x86/smm/smm_util.c... PS16, Line 76: if (lock_smm_on_exit()) `CONFIG(SMM_LOCK_ON_FINAL) && lock_smm_on_exit()` ?
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36827 )
Change subject: cpu/x86/smm: Improve SMRAM locking ......................................................................
Abandoned
-
Kyösti Mälkki (Code Review) -
Marshall Dawson (Code Review)
-
Tim Wawrzynczak (Code Review)