Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34257 )
Change subject: cpu/x86: Define INSTALL_SMM option ......................................................................
cpu/x86: Define INSTALL_SMM option
Change-Id: Ibe827193527b7e16a3360b92bc0be25ceec8d1e6 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/cpu/amd/smm/smm_init.c M src/cpu/x86/Kconfig M src/cpu/x86/mp_init.c M src/cpu/x86/smm/Makefile.inc M src/drivers/elog/Kconfig M src/drivers/spi/Kconfig M src/southbridge/intel/i82801dx/smi.c M src/southbridge/intel/i82801ix/smi.c 8 files changed, 27 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/34257/1
diff --git a/src/cpu/amd/smm/smm_init.c b/src/cpu/amd/smm/smm_init.c index 60e506c..612557e 100644 --- a/src/cpu/amd/smm/smm_init.c +++ b/src/cpu/amd/smm/smm_init.c @@ -23,6 +23,16 @@ #include <cpu/x86/smm.h> #include <string.h>
+static void smm_install(void) +{ + enable_cache(); + /* copy the real SMM handler */ + memcpy((void *)SMM_BASE, _binary_smm_start, + _binary_smm_end - _binary_smm_start); + wbinvd(); + disable_cache(); +} + void smm_init(void) { msr_t msr, syscfg_orig, mtrr_aseg_orig; @@ -55,12 +65,8 @@ msr.lo |= SYSCFG_MSR_MtrrFixDramEn; wrmsr(SYSCFG_MSR, msr);
- enable_cache(); - /* copy the real SMM handler */ - memcpy((void *)SMM_BASE, _binary_smm_start, - _binary_smm_end - _binary_smm_start); - wbinvd(); - disable_cache(); + if (CONFIG(INSTALL_SMM)) + smm_install();
/* Restore SYSCFG and MTRR */ wrmsr(SYSCFG_MSR, syscfg_orig); diff --git a/src/cpu/x86/Kconfig b/src/cpu/x86/Kconfig index caee5db..25eee72 100644 --- a/src/cpu/x86/Kconfig +++ b/src/cpu/x86/Kconfig @@ -86,6 +86,11 @@ bool default y
+config INSTALL_SMM + bool + default y + depends on HAVE_SMI_HANDLER + config HAVE_SMI_HANDLER bool default n diff --git a/src/cpu/x86/mp_init.c b/src/cpu/x86/mp_init.c index 9528149..7855ebf 100644 --- a/src/cpu/x86/mp_init.c +++ b/src/cpu/x86/mp_init.c @@ -789,6 +789,9 @@ if (mp_state.ops.adjust_smm_params != NULL) mp_state.ops.adjust_smm_params(&smm_params, 1);
+ if (!CONFIG(INSTALL_SMM)) + return -1; + printk(BIOS_DEBUG, "Installing SMM handler to 0x%08lx\n", smbase);
if (smm_load_module((void *)smbase, smsize, &smm_params)) diff --git a/src/cpu/x86/smm/Makefile.inc b/src/cpu/x86/smm/Makefile.inc index 5c7aab3..57013da 100644 --- a/src/cpu/x86/smm/Makefile.inc +++ b/src/cpu/x86/smm/Makefile.inc @@ -36,7 +36,7 @@ @printf " OBJCOPY $(subst $(obj)/,,$(@))\n" cd $(dir $<); $(OBJCOPY_smm) -I binary $(notdir $<) $(target-objcopy) $(abspath $@)
-ifeq ($(CONFIG_HAVE_SMI_HANDLER),y) +ifeq ($(CONFIG_INSTALL_SMM),y) ramstage-srcs += $(obj)/cpu/x86/smm/smm.manual endif
diff --git a/src/drivers/elog/Kconfig b/src/drivers/elog/Kconfig index 4a66e78..7848f7b 100644 --- a/src/drivers/elog/Kconfig +++ b/src/drivers/elog/Kconfig @@ -43,7 +43,7 @@ This option will enable event logging from the preram stage.
config ELOG_GSMI - depends on HAVE_SMI_HANDLER + depends on INSTALL_SMM bool "SMI interface to write and clear event log" select SPI_FLASH_SMM if BOOT_DEVICE_SPI_FLASH_RW_NOMMAP default n diff --git a/src/drivers/spi/Kconfig b/src/drivers/spi/Kconfig index b15a502..3013087 100644 --- a/src/drivers/spi/Kconfig +++ b/src/drivers/spi/Kconfig @@ -63,7 +63,7 @@
config SPI_FLASH_SMM bool - depends on HAVE_SMI_HANDLER + depends on INSTALL_SMM help Select this option if you want SPI flash support in SMM.
diff --git a/src/southbridge/intel/i82801dx/smi.c b/src/southbridge/intel/i82801dx/smi.c index 7dfed9d..86e75d0 100644 --- a/src/southbridge/intel/i82801dx/smi.c +++ b/src/southbridge/intel/i82801dx/smi.c @@ -328,7 +328,8 @@ void smm_init(void) { /* Put SMM code to 0xa0000 */ - smm_install(); + if (CONFIG(INSTALL_SMM)) + smm_install();
/* Put relocation code to 0x38000 and relocate SMBASE */ smm_relocate(); diff --git a/src/southbridge/intel/i82801ix/smi.c b/src/southbridge/intel/i82801ix/smi.c index 5d898cc..d50432b 100644 --- a/src/southbridge/intel/i82801ix/smi.c +++ b/src/southbridge/intel/i82801ix/smi.c @@ -157,7 +157,8 @@ void smm_init(void) { /* Put SMM code to 0xa0000 */ - smm_install(); + if (CONFIG(INSTALL_SMM)) + smm_install();
/* Put relocation code to 0x38000 and relocate SMBASE */ smm_relocate();
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34257 )
Change subject: cpu/x86: Define INSTALL_SMM option ......................................................................
Patch Set 1:
Three scenarios to consider:
a) INSTALL_SMM=y, HAVE_SMI_HANDLER=y
Current (unfortunate) situation. Build and install smm -class. Let MP init raise SMIs and do its job.
b) INSTALL_SMM=n, HAVE_SMI_HANDLER=y
Don't build smm -class. Absolute minimal resident SMM may be installed, like asm inline "rsm"? Let MP init raise SMIs to relocate SMM stacks? On entry to payload, SMRAM can be locked or left unlocked.
c) INSTALL_SMM=n, HAVE_SMI_HANDLER=n
Not possible for MP init to raise SMIs. See CB:34152 and CB:34158 what is required on source level. Little benefits in comparison to b) above?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34257 )
Change subject: cpu/x86: Define INSTALL_SMM option ......................................................................
Patch Set 2:
(1 comment)
Patch Set 1:
Three scenarios to consider:
a) INSTALL_SMM=y, HAVE_SMI_HANDLER=y
Current (unfortunate) situation. Build and install smm -class. Let MP init raise SMIs and do its job.
b) INSTALL_SMM=n, HAVE_SMI_HANDLER=y
Don't build smm -class. Absolute minimal resident SMM may be installed, like asm inline "rsm"? Let MP init raise SMIs to relocate SMM stacks? On entry to payload, SMRAM can be locked or left unlocked.
Is this behavior a long term goal?
What definition do we have HAVE_SMI_HANDLER to be going forward? That it can support SMM? And we want to turn off the permanent handler installation with INSTALL_SMM?
I think the combinations seem legit, but I think the naming could be more descriptive in what we're aiming for.
c) INSTALL_SMM=n, HAVE_SMI_HANDLER=n
Not possible for MP init to raise SMIs. See CB:34152 and CB:34158 what is required on source level. Little benefits in comparison to b) above?
https://review.coreboot.org/c/coreboot/+/34257/2/src/cpu/x86/Kconfig File src/cpu/x86/Kconfig:
https://review.coreboot.org/c/coreboot/+/34257/2/src/cpu/x86/Kconfig@89 PS2, Line 89: INSTALL_SMM I think we probably need a more descriptive name. RICH_PERMANENT_HANDLER ? FULL_PERMANENT_HANDLER ?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34257 )
Change subject: cpu/x86: Define INSTALL_SMM option ......................................................................
Patch Set 2:
Patch Set 2:
Don't build smm -class. Absolute minimal resident SMM may be installed, like asm inline "rsm"? Let MP init raise SMIs to relocate SMM stacks? On entry to payload, SMRAM can be locked or left unlocked.
Is this behavior a long term goal?
I just noticed that it was pretty hard to try evaluate what is built into SMM currently with all the guards in place. I think we would want to separate SMM to rmodule file in CBFS. Things should be stable enough such that one may use audited SMM build from previous build, yet update ramstage as needed?
Also with the experiments/initiatives to leave SMM loading to OS, I'd like to see that platforms at least pass the build in such a use case. Currently HAVE_SMI_HANDLER=n mostly will not build.
What definition do we have HAVE_SMI_HANDLER to be going forward? That it can support SMM? And we want to turn off the permanent handler installation with INSTALL_SMM?
I think HAVE_SMI_HANDLER would indicate platform code has support to raise and configure SMIs. Essentially, satisfy the requirements of PARALLEL_MP code.
I think the combinations seem legit, but I think the naming could be more descriptive in what we're aiming for.
Sure.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34257 )
Change subject: cpu/x86: Define INSTALL_SMM option ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2:
Don't build smm -class. Absolute minimal resident SMM may be installed, like asm inline "rsm"? Let MP init raise SMIs to relocate SMM stacks? On entry to payload, SMRAM can be locked or left unlocked.
Is this behavior a long term goal?
I just noticed that it was pretty hard to try evaluate what is built into SMM currently with all the guards in place. I think we would want to separate SMM to rmodule file in CBFS. Things should be stable enough such that one may use audited SMM build from previous build, yet update ramstage as needed?
Seems fine to me. smm being built into ramstage proper is mainly legacy.
Here's what's in there now:
$ git grep _binary -- src/ src/cpu/amd/smm/smm_init.c: memcpy((void *)SMM_BASE, _binary_smm_start, src/cpu/amd/smm/smm_init.c: _binary_smm_end - _binary_smm_start); src/cpu/x86/mp_init.c:extern char _binary_sipi_vector_start[]; src/cpu/x86/mp_init.c: if (rmodule_parse(&_binary_sipi_vector_start, &sipi_mod)) { src/cpu/x86/smm/smm_module_loader.c:extern unsigned char _binary_smmstub_start[]; src/cpu/x86/smm/smm_module_loader.c: if (rmodule_parse(&_binary_smmstub_start, &smm_stub)) src/cpu/x86/smm/smm_module_loader.c: if (rmodule_parse(&_binary_smm_start, &smm_mod)) src/drivers/vpd/vpd_tables.h:struct vpd_table_binary_blob_pointer { src/include/cpu/x86/smm.h:extern unsigned char _binary_smm_start[]; src/include/cpu/x86/smm.h:extern unsigned char _binary_smm_end[]; src/include/gpio.h:static inline uint32_t gpio_binary_first_base3_value(const gpio_t gpio[], src/mainboard/google/gale/boardid.c: bid = gpio_binary_first_base3_value(hw_rev_gpios, src/mainboard/google/veyron/boardid.c: code = gpio_binary_first_base3_value(pins, ARRAY_SIZE(pins)); src/mainboard/google/veyron_mickey/boardid.c: code = gpio_binary_first_base3_value(pins, ARRAY_SIZE(pins)); src/mainboard/google/veyron_rialto/boardid.c: code = gpio_binary_first_base3_value(pins, ARRAY_SIZE(pins)); src/southbridge/intel/i82801dx/smi.c: memcpy((void *)0xa0000, _binary_smm_start, src/southbridge/intel/i82801dx/smi.c: _binary_smm_end - _binary_smm_start); src/southbridge/intel/i82801ix/smi.c: memcpy((void *)0xa0000, _binary_smm_start, src/southbridge/intel/i82801ix/smi.c: _binary_smm_end - _binary_smm_start); [adurbin@adurbin:~/devel/coreboot]
Also with the experiments/initiatives to leave SMM loading to OS, I'd like to see that platforms at least pass the build in such a use case. Currently HAVE_SMI_HANDLER=n mostly will not build.
What definition do we have HAVE_SMI_HANDLER to be going forward? That it can support SMM? And we want to turn off the permanent handler installation with INSTALL_SMM?
I think HAVE_SMI_HANDLER would indicate platform code has support to raise and configure SMIs. Essentially, satisfy the requirements of PARALLEL_MP code.
I think the combinations seem legit, but I think the naming could be more descriptive in what we're aiming for.
Sure.
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34257 )
Change subject: cpu/x86: Define INSTALL_SMM option ......................................................................
Abandoned
Kyösti Mälkki has restored this change. ( https://review.coreboot.org/c/coreboot/+/34257 )
Change subject: cpu/x86: Define INSTALL_SMM option ......................................................................
Restored
Kyösti Mälkki has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/34257 )
Change subject: cpu/x86: Define INSTALL_SMM option ......................................................................
Abandoned
-
Aaron Durbin (Code Review) -
Kyösti Mälkki (Code Review)