Change in coreboot[master]: intel/baytrail: Use smm_subregion()

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/34739 )

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

intel/baytrail: Use smm_subregion()

Change-Id: Ic2677bcf9f2f79c4db725ebcf342a8575ee7bc38
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/baytrail/cpu.c
M src/soc/intel/baytrail/memmap.c
2 files changed, 61 insertions(+), 32 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34739/1

diff --git a/src/soc/intel/baytrail/cpu.c b/src/soc/intel/baytrail/cpu.c
index 9526932..2e14270 100644
--- a/src/soc/intel/baytrail/cpu.c
+++ b/src/soc/intel/baytrail/cpu.c
@@ -86,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;

/* Package level MSRs */
 static const struct reg_script package_msr_script[] = {
@@ -138,21 +137,33 @@
    return pattrs->num_cpus;
 }

+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 u32 rmask = ~((1 << 12) - 1);
+
+	smm_region(&tseg_base, &tseg_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)
 {
-	/* All range registers are aligned to 4KiB */
-	const uint32_t rmask = ~((1 << 12) - 1);
+	printk(BIOS_DEBUG, "Setting up SMI for CPU\n");

-	/* 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;
+	fill_in_relocation_params(&smm_reloc_params);

-	*perm_smbase = relo_attrs.smbase;
-	*perm_smsize = smm_region_size() - CONFIG_SMM_RESERVED_SIZE;
-	*smm_save_state_size = sizeof(em64t100_smm_state_save_area_t);
+	smm_subregion(SMM_SUBREGION_HANDLER, perm_smbase, perm_smsize);
+
+	*smm_save_state_size = sizeof(em64t101_smm_state_save_area_t);
 }

static void get_microcode_info(const void **microcode, int *parallel)
@@ -177,16 +188,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/baytrail/memmap.c b/src/soc/intel/baytrail/memmap.c
index 94e91ca..c502270 100644
--- a/src/soc/intel/baytrail/memmap.c
+++ b/src/soc/intel/baytrail/memmap.c
@@ -14,7 +14,7 @@
  */

#include <cbmem.h>
-#include <stage_cache.h>
+#include <cpu/x86/smm.h>
 #include <soc/iosf.h>
 #include <soc/smm.h>

@@ -28,15 +28,37 @@
    return (void *) smm_region_start();
 }

-void stage_cache_external_region(void **base, size_t *size)
+void smm_region(uintptr_t *start, size_t *size)
 {
-	char *smm_base;
-	/* 1MiB cache size */
-	const long cache_size = CONFIG_SMM_RESERVED_SIZE;
+	*start = (iosf_bunit_read(BUNIT_SMRRL) & 0xFFFF) << 20;
+	*size = smm_region_size();
+}

-	/* Ramstage cache lives in TSEG region which is the definition of
-	 * cbmem_top(). */
-	smm_base = cbmem_top();
-	*size = cache_size;
-	*base = &smm_base[smm_region_size() - cache_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;
 }

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34739 )

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

Patch Set 1: Code-Review-2

Flagging -2 until tested.

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34739

to look at the new patch set (#2).

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

intel/baytrail: Use smm_subregion()

Change-Id: Ic2677bcf9f2f79c4db725ebcf342a8575ee7bc38
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/baytrail/cpu.c
M src/soc/intel/baytrail/include/soc/smm.h
M src/soc/intel/baytrail/memmap.c
3 files changed, 39 insertions(+), 47 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34739/2

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34739

to look at the new patch set (#6).

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

intel/baytrail: Use smm_subregion()

Change-Id: Ic2677bcf9f2f79c4db725ebcf342a8575ee7bc38
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/baytrail/cpu.c
M src/soc/intel/baytrail/include/soc/smm.h
M src/soc/intel/baytrail/memmap.c
3 files changed, 38 insertions(+), 46 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34739/6

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34739

to look at the new patch set (#10).

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

intel/baytrail: Use smm_subregion()

Change-Id: Ic2677bcf9f2f79c4db725ebcf342a8575ee7bc38
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/baytrail/cpu.c
M src/soc/intel/baytrail/include/soc/smm.h
M src/soc/intel/baytrail/memmap.c
M src/soc/intel/baytrail/romstage/romstage.c
4 files changed, 38 insertions(+), 48 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34739/10

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34739

to look at the new patch set (#11).

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

intel/baytrail: Use smm_subregion()

Change-Id: Ic2677bcf9f2f79c4db725ebcf342a8575ee7bc38
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/baytrail/cpu.c
M src/soc/intel/baytrail/include/soc/smm.h
M src/soc/intel/baytrail/memmap.c
M src/soc/intel/baytrail/romstage/romstage.c
4 files changed, 38 insertions(+), 48 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34739/11

[Kyösti Mälkki (Code Review)]

Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/34739

to look at the new patch set (#12).

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

intel/baytrail: Use smm_subregion()

Change-Id: Ic2677bcf9f2f79c4db725ebcf342a8575ee7bc38
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/baytrail/cpu.c
M src/soc/intel/baytrail/include/soc/smm.h
M src/soc/intel/baytrail/memmap.c
M src/soc/intel/baytrail/romstage/romstage.c
4 files changed, 38 insertions(+), 49 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/34739/12

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/34739 )

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

Removed Code-Review-2 by Kyösti Mälkki

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34739 )

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

Patch Set 18: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34739 )

Change subject: intel/baytrail: Use smm_subregion()
......................................................................

intel/baytrail: Use smm_subregion()

Change-Id: Ic2677bcf9f2f79c4db725ebcf342a8575ee7bc38
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34739
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/baytrail/cpu.c
M src/soc/intel/baytrail/include/soc/smm.h
M src/soc/intel/baytrail/memmap.c
M src/soc/intel/baytrail/romstage/romstage.c
4 files changed, 38 insertions(+), 49 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/baytrail/cpu.c b/src/soc/intel/baytrail/cpu.c
index 4cb0a06..edc4e83 100644
--- a/src/soc/intel/baytrail/cpu.c
+++ b/src/soc/intel/baytrail/cpu.c
@@ -88,13 +88,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;

/* Package level MSRs */
 static const struct reg_script package_msr_script[] = {
@@ -140,20 +139,32 @@
    return pattrs->num_cpus;
 }

+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 u32 rmask = ~((1 << 12) - 1);
+
+	smm_region(&tseg_base, &tseg_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)
 {
-	/* All range registers are aligned to 4KiB */
-	const uint32_t rmask = ~((1 << 12) - 1);
+	printk(BIOS_DEBUG, "Setting up SMI for CPU\n");

-	/* 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;
+	fill_in_relocation_params(&smm_reloc_params);

-	*perm_smbase = relo_attrs.smbase;
-	*perm_smsize = smm_region_size() - CONFIG_SMM_RESERVED_SIZE;
+	smm_subregion(SMM_SUBREGION_HANDLER, perm_smbase, perm_smsize);
+
    *smm_save_state_size = sizeof(em64t100_smm_state_save_area_t);
 }

@@ -179,16 +190,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/baytrail/include/soc/smm.h b/src/soc/intel/baytrail/include/soc/smm.h
index 29b7946..a2b7ec0 100644
--- a/src/soc/intel/baytrail/include/soc/smm.h
+++ b/src/soc/intel/baytrail/include/soc/smm.h
@@ -18,19 +18,6 @@

#include <types.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);
-
 enum {
    SMM_SAVE_PARAM_GPIO_ROUTE = 0,
    SMM_SAVE_PARAM_PCIE_WAKE_ENABLE,
diff --git a/src/soc/intel/baytrail/memmap.c b/src/soc/intel/baytrail/memmap.c
index 94e91ca..015f13c 100644
--- a/src/soc/intel/baytrail/memmap.c
+++ b/src/soc/intel/baytrail/memmap.c
@@ -14,29 +14,26 @@
  */

#include <cbmem.h>
-#include <stage_cache.h>
+#include <cpu/x86/smm.h>
 #include <soc/iosf.h>
-#include <soc/smm.h>

-uintptr_t smm_region_start(void)
+static uintptr_t smm_region_start(void)
 {
    return (iosf_bunit_read(BUNIT_SMRRL) << 20);
 }

+static size_t smm_region_size(void)
+{
+	return CONFIG_SMM_TSEG_SIZE;
+}
+
 void *cbmem_top(void)
 {
    return (void *) smm_region_start();
 }

-void stage_cache_external_region(void **base, size_t *size)
+void smm_region(uintptr_t *start, size_t *size)
 {
-	char *smm_base;
-	/* 1MiB cache size */
-	const long cache_size = CONFIG_SMM_RESERVED_SIZE;
-
-	/* Ramstage cache lives in TSEG region which is the definition of
-	 * cbmem_top(). */
-	smm_base = cbmem_top();
-	*size = cache_size;
-	*base = &smm_base[smm_region_size() - cache_size];
+	*start = (iosf_bunit_read(BUNIT_SMRRL) & 0xFFFF) << 20;
+	*size = smm_region_size();
 }
diff --git a/src/soc/intel/baytrail/romstage/romstage.c b/src/soc/intel/baytrail/romstage/romstage.c
index 3607e6d..2b92c32 100644
--- a/src/soc/intel/baytrail/romstage/romstage.c
+++ b/src/soc/intel/baytrail/romstage/romstage.c
@@ -30,7 +30,6 @@
 #include <elog.h>
 #include <program_loading.h>
 #include <romstage_handoff.h>
-#include <stage_cache.h>
 #include <string.h>
 #include <timestamp.h>
 #include <vendorcode/google/chromeos/chromeos.h>
@@ -41,7 +40,6 @@
 #include <soc/pci_devs.h>
 #include <soc/pmc.h>
 #include <soc/romstage.h>
-#include <soc/smm.h>
 #include <soc/spi.h>

static void program_base_addresses(void)