Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34757 )
Change subject: intel/fsp_baytrail: Use smm_subregion() ......................................................................
intel/fsp_baytrail: Use smm_subregion()
Change-Id: I4e9de9c7f5decd784d881e5a733e995522be5226 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/intel/fsp_baytrail/cpu.c M src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c M src/soc/intel/fsp_baytrail/include/soc/smm.h M src/soc/intel/fsp_baytrail/memmap.c 4 files changed, 74 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/34757/1
diff --git a/src/soc/intel/fsp_baytrail/cpu.c b/src/soc/intel/fsp_baytrail/cpu.c index 15dc851..0339bae 100644 --- a/src/soc/intel/fsp_baytrail/cpu.c +++ b/src/soc/intel/fsp_baytrail/cpu.c @@ -82,13 +82,12 @@ * MP and SMM loading initialization. */
-struct smm_relocation_attrs { - uint32_t smbase; - uint32_t smrr_base; - uint32_t smrr_mask; +struct smm_relocation_params { + msr_t smrr_base; + msr_t smrr_mask; };
-static struct smm_relocation_attrs relo_attrs; +static struct smm_relocation_params smm_reloc_params;
static void pre_mp_init(void) { @@ -105,20 +104,32 @@ return pattrs->num_cpus; }
-static void get_smm_info(uintptr_t *perm_smbase, size_t *perm_smsize, - size_t *smm_save_state_size) +static void fill_in_relocation_params(struct smm_relocation_params *params) { + uintptr_t tseg_base; + size_t tseg_size; + /* All range registers are aligned to 4KiB */ - const uint32_t rmask = ~((1 << 12) - 1); + const u32 rmask = ~(4 * KiB - 1);
- /* Initialize global tracking state. */ - relo_attrs.smbase = (uint32_t)smm_region_start(); - relo_attrs.smrr_base = relo_attrs.smbase | MTRR_TYPE_WRBACK; - relo_attrs.smrr_mask = ~(smm_region_size() - 1) & rmask; - relo_attrs.smrr_mask |= MTRR_PHYS_MASK_VALID; + smm_region(&tseg_base, &tseg_size);
- *perm_smbase = relo_attrs.smbase; - *perm_smsize = smm_region_size() - CONFIG_SMM_RESERVED_SIZE; + /* SMRR has 32-bits of valid address aligned to 4KiB. */ + params->smrr_base.lo = (tseg_base & rmask) | MTRR_TYPE_WRBACK; + params->smrr_base.hi = 0; + params->smrr_mask.lo = (~(tseg_size - 1) & rmask) | MTRR_PHYS_MASK_VALID; + params->smrr_mask.hi = 0; +} + +static void get_smm_info(uintptr_t *perm_smbase, size_t *perm_smsize, + size_t *smm_save_state_size) +{ + printk(BIOS_DEBUG, "Setting up SMI for CPU\n"); + + fill_in_relocation_params(&smm_reloc_params); + + smm_subregion(SMM_SUBREGION_HANDLER, perm_smbase, perm_smsize); + *smm_save_state_size = sizeof(em64t100_smm_state_save_area_t); }
@@ -133,16 +144,12 @@ static void relocation_handler(int cpu, uintptr_t curr_smbase, uintptr_t staggered_smbase) { - msr_t smrr; + struct smm_relocation_params *relo_params = &smm_reloc_params; em64t100_smm_state_save_area_t *smm_state;
/* Set up SMRR. */ - smrr.lo = relo_attrs.smrr_base; - smrr.hi = 0; - wrmsr(IA32_SMRR_PHYS_BASE, smrr); - smrr.lo = relo_attrs.smrr_mask; - smrr.hi = 0; - wrmsr(IA32_SMRR_PHYS_MASK, smrr); + wrmsr(IA32_SMRR_PHYS_BASE, relo_params->smrr_base); + wrmsr(IA32_SMRR_PHYS_MASK, relo_params->smrr_mask);
smm_state = (void *)(SMM_EM64T100_SAVE_STATE_OFFSET + curr_smbase); smm_state->smbase = staggered_smbase; diff --git a/src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c b/src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c index f11b206..65f1705 100644 --- a/src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c +++ b/src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c @@ -121,7 +121,7 @@ else if ((config->PcdeMMCBootMode != EMMC_USE_DEFAULT)) UpdData->PcdeMMCBootMode = config->PcdeMMCBootMode - EMMC_DISABLED;
- UpdData->PcdMrcInitTsegSize = smm_region_size() >> 20; + UpdData->PcdMrcInitTsegSize = CONFIG_SMM_TSEG_SIZE >> 20;
printk(FSP_INFO_LEVEL, "GTT Size:\t\t%d MB\n", UpdData->PcdGttSize); printk(FSP_INFO_LEVEL, "Tseg Size:\t\t%d MB\n", UpdData->PcdMrcInitTsegSize); diff --git a/src/soc/intel/fsp_baytrail/include/soc/smm.h b/src/soc/intel/fsp_baytrail/include/soc/smm.h index c929572..eb3158e 100644 --- a/src/soc/intel/fsp_baytrail/include/soc/smm.h +++ b/src/soc/intel/fsp_baytrail/include/soc/smm.h @@ -17,19 +17,6 @@ #ifndef _BAYTRAIL_SMM_H_ #define _BAYTRAIL_SMM_H_
-/* There is a bug in the order of Kconfig includes in that arch/x86/Kconfig - * is included after chipset code. This causes the chipset's Kconfig to be - * clobbered by the arch/x86/Kconfig if they have the same name. */ -static inline int smm_region_size(void) -{ - /* Make it 8MiB by default. */ - if (CONFIG_SMM_TSEG_SIZE == 0) - return (8 << 20); - return CONFIG_SMM_TSEG_SIZE; -} - -uintptr_t smm_region_start(void); - #if !defined(__PRE_RAM__) && !defined(__SMM___) #include <stdint.h> void southcluster_smm_clear_state(void); diff --git a/src/soc/intel/fsp_baytrail/memmap.c b/src/soc/intel/fsp_baytrail/memmap.c index 77ba406..5e70e4e 100644 --- a/src/soc/intel/fsp_baytrail/memmap.c +++ b/src/soc/intel/fsp_baytrail/memmap.c @@ -15,13 +15,20 @@ */
#include <cbmem.h> +#include <cpu/x86/smm.h> #include <soc/iosf.h> #include <soc/smm.h> #include <drivers/intel/fsp1_0/fsp_util.h> +#include <types.h>
-uintptr_t smm_region_start(void) +static uintptr_t smm_region_start(void) { - return (iosf_bunit_read(BUNIT_SMRRL) << 20); + return (iosf_bunit_read(BUNIT_SMRRL) & 0xFFFF) << 20; +} + +static size_t smm_region_size(void) +{ + return CONFIG_SMM_TSEG_SIZE; }
/** @brief get the top of usable low memory from the FSP's HOB list @@ -38,3 +45,38 @@ { return find_fsp_reserved_mem(*(void **)CBMEM_FSP_HOB_PTR); } + +void smm_region(uintptr_t *start, size_t *size) +{ + *start = smm_region_start(); + *size = smm_region_size(); +} + +int smm_subregion(int sub, uintptr_t *start, size_t *size) +{ + uintptr_t sub_base; + size_t sub_size; + const size_t cache_size = CONFIG_SMM_RESERVED_SIZE; + + smm_region(&sub_base, &sub_size); + + switch (sub) { + case SMM_SUBREGION_HANDLER: + /* Handler starts at the base of TSEG. */ + sub_size -= cache_size; + break; + case SMM_SUBREGION_CACHE: + /* External cache is in the middle of TSEG. */ + sub_base += sub_size - cache_size; + sub_size = cache_size; + break; + default: + *start = 0; + *size = 0; + return -1; + } + + *start = sub_base; + *size = sub_size; + return 0; +}
Hello Patrick Rudolph, Huang Jin, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34757
to look at the new patch set (#2).
Change subject: intel/fsp_baytrail: Use smm_subregion() ......................................................................
intel/fsp_baytrail: Use smm_subregion()
Change-Id: I4e9de9c7f5decd784d881e5a733e995522be5226 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/intel/fsp_baytrail/cpu.c M src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c M src/soc/intel/fsp_baytrail/include/soc/smm.h M src/soc/intel/fsp_baytrail/memmap.c 4 files changed, 45 insertions(+), 38 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/34757/2
Hello Werner Zeh, Patrick Rudolph, Huang Jin, Philipp Deppenwiese, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/34757
to look at the new patch set (#8).
Change subject: intel/fsp_baytrail: Use smm_subregion() ......................................................................
intel/fsp_baytrail: Use smm_subregion()
Change-Id: I4e9de9c7f5decd784d881e5a733e995522be5226 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/soc/intel/fsp_baytrail/cpu.c M src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c M src/soc/intel/fsp_baytrail/include/soc/smm.h M src/soc/intel/fsp_baytrail/memmap.c 4 files changed, 46 insertions(+), 39 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/34757/8
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34757 )
Change subject: intel/fsp_baytrail: Use smm_subregion() ......................................................................
Patch Set 12: Code-Review+2
Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34757 )
Change subject: intel/fsp_baytrail: Use smm_subregion() ......................................................................
intel/fsp_baytrail: Use smm_subregion()
Change-Id: I4e9de9c7f5decd784d881e5a733e995522be5226 Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/34757 Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/fsp_baytrail/cpu.c M src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c M src/soc/intel/fsp_baytrail/include/soc/smm.h M src/soc/intel/fsp_baytrail/memmap.c 4 files changed, 46 insertions(+), 39 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/fsp_baytrail/cpu.c b/src/soc/intel/fsp_baytrail/cpu.c index 0c86ce3..787a410 100644 --- a/src/soc/intel/fsp_baytrail/cpu.c +++ b/src/soc/intel/fsp_baytrail/cpu.c @@ -21,6 +21,7 @@ #include <cpu/intel/microcode.h> #include <cpu/intel/smm_reloc.h> #include <cpu/intel/turbo.h> +#include <cpu/intel/smm_reloc.h> #include <cpu/x86/cache.h> #include <cpu/x86/lapic.h> #include <cpu/x86/mp.h> @@ -85,13 +86,12 @@ * MP and SMM loading initialization. */
-struct smm_relocation_attrs { - uint32_t smbase; - uint32_t smrr_base; - uint32_t smrr_mask; +struct smm_relocation_params { + msr_t smrr_base; + msr_t smrr_mask; };
-static struct smm_relocation_attrs relo_attrs; +static struct smm_relocation_params smm_reloc_params;
static void pre_mp_init(void) { @@ -108,20 +108,32 @@ return pattrs->num_cpus; }
-static void get_smm_info(uintptr_t *perm_smbase, size_t *perm_smsize, - size_t *smm_save_state_size) +static void fill_in_relocation_params(struct smm_relocation_params *params) { + uintptr_t tseg_base; + size_t tseg_size; + /* All range registers are aligned to 4KiB */ - const uint32_t rmask = ~((1 << 12) - 1); + const u32 rmask = ~(4 * KiB - 1);
- /* Initialize global tracking state. */ - relo_attrs.smbase = (uint32_t)smm_region_start(); - relo_attrs.smrr_base = relo_attrs.smbase | MTRR_TYPE_WRBACK; - relo_attrs.smrr_mask = ~(smm_region_size() - 1) & rmask; - relo_attrs.smrr_mask |= MTRR_PHYS_MASK_VALID; + smm_region(&tseg_base, &tseg_size);
- *perm_smbase = relo_attrs.smbase; - *perm_smsize = smm_region_size() - CONFIG_SMM_RESERVED_SIZE; + /* SMRR has 32-bits of valid address aligned to 4KiB. */ + params->smrr_base.lo = (tseg_base & rmask) | MTRR_TYPE_WRBACK; + params->smrr_base.hi = 0; + params->smrr_mask.lo = (~(tseg_size - 1) & rmask) | MTRR_PHYS_MASK_VALID; + params->smrr_mask.hi = 0; +} + +static void get_smm_info(uintptr_t *perm_smbase, size_t *perm_smsize, + size_t *smm_save_state_size) +{ + printk(BIOS_DEBUG, "Setting up SMI for CPU\n"); + + fill_in_relocation_params(&smm_reloc_params); + + smm_subregion(SMM_SUBREGION_HANDLER, perm_smbase, perm_smsize); + *smm_save_state_size = sizeof(em64t100_smm_state_save_area_t); }
@@ -136,16 +148,12 @@ static void relocation_handler(int cpu, uintptr_t curr_smbase, uintptr_t staggered_smbase) { - msr_t smrr; + struct smm_relocation_params *relo_params = &smm_reloc_params; em64t100_smm_state_save_area_t *smm_state;
/* Set up SMRR. */ - smrr.lo = relo_attrs.smrr_base; - smrr.hi = 0; - wrmsr(IA32_SMRR_PHYS_BASE, smrr); - smrr.lo = relo_attrs.smrr_mask; - smrr.hi = 0; - wrmsr(IA32_SMRR_PHYS_MASK, smrr); + wrmsr(IA32_SMRR_PHYS_BASE, relo_params->smrr_base); + wrmsr(IA32_SMRR_PHYS_MASK, relo_params->smrr_mask);
smm_state = (void *)(SMM_EM64T100_SAVE_STATE_OFFSET + curr_smbase); smm_state->smbase = staggered_smbase; diff --git a/src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c b/src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c index 48f0cdd..c0b0998 100644 --- a/src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c +++ b/src/soc/intel/fsp_baytrail/fsp/chipset_fsp_util.c @@ -29,7 +29,6 @@ #include <soc/pmc.h> #include <soc/acpi.h> #include <soc/iomap.h> -#include <soc/smm.h>
/* Copy the default UPD region and settings to a buffer for modification */ static void GetUpdDefaultFromFsp (FSP_INFO_HEADER *FspInfo, UPD_DATA_REGION *UpdData) @@ -115,7 +114,7 @@ else if ((config->PcdeMMCBootMode != EMMC_USE_DEFAULT)) UpdData->PcdeMMCBootMode = config->PcdeMMCBootMode - EMMC_DISABLED;
- UpdData->PcdMrcInitTsegSize = smm_region_size() >> 20; + UpdData->PcdMrcInitTsegSize = CONFIG_SMM_TSEG_SIZE >> 20;
printk(FSP_INFO_LEVEL, "GTT Size:\t\t%d MB\n", UpdData->PcdGttSize); printk(FSP_INFO_LEVEL, "Tseg Size:\t\t%d MB\n", UpdData->PcdMrcInitTsegSize); diff --git a/src/soc/intel/fsp_baytrail/include/soc/smm.h b/src/soc/intel/fsp_baytrail/include/soc/smm.h index 2ae7f5b..b6e24a8 100644 --- a/src/soc/intel/fsp_baytrail/include/soc/smm.h +++ b/src/soc/intel/fsp_baytrail/include/soc/smm.h @@ -17,17 +17,5 @@ #ifndef _BAYTRAIL_SMM_H_ #define _BAYTRAIL_SMM_H_
-/* There is a bug in the order of Kconfig includes in that arch/x86/Kconfig - * is included after chipset code. This causes the chipset's Kconfig to be - * clobbered by the arch/x86/Kconfig if they have the same name. */ -static inline int smm_region_size(void) -{ - /* Make it 8MiB by default. */ - if (CONFIG_SMM_TSEG_SIZE == 0) - return (8 << 20); - return CONFIG_SMM_TSEG_SIZE; -} - -uintptr_t smm_region_start(void);
#endif /* _BAYTRAIL_SMM_H_ */ diff --git a/src/soc/intel/fsp_baytrail/memmap.c b/src/soc/intel/fsp_baytrail/memmap.c index 77ba406..7fec7f9 100644 --- a/src/soc/intel/fsp_baytrail/memmap.c +++ b/src/soc/intel/fsp_baytrail/memmap.c @@ -15,13 +15,19 @@ */
#include <cbmem.h> +#include <cpu/x86/smm.h> #include <soc/iosf.h> -#include <soc/smm.h> #include <drivers/intel/fsp1_0/fsp_util.h> +#include <types.h>
-uintptr_t smm_region_start(void) +static uintptr_t smm_region_start(void) { - return (iosf_bunit_read(BUNIT_SMRRL) << 20); + return (iosf_bunit_read(BUNIT_SMRRL) & 0xFFFF) << 20; +} + +static size_t smm_region_size(void) +{ + return CONFIG_SMM_TSEG_SIZE; }
/** @brief get the top of usable low memory from the FSP's HOB list @@ -38,3 +44,9 @@ { return find_fsp_reserved_mem(*(void **)CBMEM_FSP_HOB_PTR); } + +void smm_region(uintptr_t *start, size_t *size) +{ + *start = smm_region_start(); + *size = smm_region_size(); +}
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)