Change in coreboot[master]: intel/fsp_broadwell_de: 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/+/34745 )

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

intel/fsp_broadwell_de: Use smm_subregion()

Change-Id: I986bbe978d3f68693b2d4538ccbcc11cdbd23c6a
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/fsp_broadwell_de/include/soc/smm.h
M src/soc/intel/fsp_broadwell_de/memmap.c
M src/soc/intel/fsp_broadwell_de/smmrelocate.c
3 files changed, 64 insertions(+), 30 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34745/1

diff --git a/src/soc/intel/fsp_broadwell_de/include/soc/smm.h b/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
index 72aa7fa..42cadb3 100644
--- a/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
+++ b/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
@@ -27,10 +27,10 @@
 } __packed;

struct smm_relocation_params {
-	u32 smram_base;
-	u32 smram_size;
-	u32 ied_base;
-	u32 ied_size;
+	uintptr_t smram_base;
+	size_t smram_size;
+	uintptr_t ied_base;
+	size_t ied_size;
    msr_t smrr_base;
    msr_t smrr_mask;
    msr_t prmrr_base;
diff --git a/src/soc/intel/fsp_broadwell_de/memmap.c b/src/soc/intel/fsp_broadwell_de/memmap.c
index 7510094..c1a685b 100644
--- a/src/soc/intel/fsp_broadwell_de/memmap.c
+++ b/src/soc/intel/fsp_broadwell_de/memmap.c
@@ -14,10 +14,62 @@
  * GNU General Public License for more details.
  */

+#define __SIMPLE_DEVICE__
+
 #include <cbmem.h>
+#include <cpu/x86/smm.h>
+#include <device/pci_ops.h>
 #include <drivers/intel/fsp1_0/fsp_util.h>
+#include <soc/broadwell_de.h>

void *cbmem_top(void)
 {
    return find_fsp_reserved_mem(*(void **)CBMEM_FSP_HOB_PTR);
 }
+
+void smm_region(uintptr_t *start, size_t *size)
+{
+	u32 tseg_base = pci_read_config32(PCI_DEV(0, 0, 0), TSEG_BASE);
+	u32 tseg_limit = pci_read_config32(PCI_DEV(0, 0, 0), TSEG_LIMIT);
+	*start = tseg_base;
+	*size = tseg_limit - tseg_base;
+}
+
+/* Intel Enhanced Debug region must be 4MB */
+#define FIXED_IED_REGION_SIZE	(4 << 20)
+
+int smm_subregion(int sub, uintptr_t *start, size_t *size)
+{
+	uintptr_t sub_base;
+	size_t sub_size;
+	const size_t ied_size = FIXED_IED_REGION_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 -= ied_size;
+		sub_size -= cache_size;
+		break;
+	case SMM_SUBREGION_CACHE:
+		/* External cache is in the middle of TSEG. */
+		sub_base += sub_size - (ied_size + cache_size);
+		sub_size = cache_size;
+		break;
+	case SMM_SUBREGION_IED:
+		/* IED is at the top. */
+		sub_base += sub_size - ied_size;
+		sub_size = ied_size;
+		break;
+	default:
+		*start = 0;
+		*size = 0;
+		return -1;
+	}
+
+	*start = sub_base;
+	*size = sub_size;
+	return 0;
+}
diff --git a/src/soc/intel/fsp_broadwell_de/smmrelocate.c b/src/soc/intel/fsp_broadwell_de/smmrelocate.c
index 57ba432..3086acb 100644
--- a/src/soc/intel/fsp_broadwell_de/smmrelocate.c
+++ b/src/soc/intel/fsp_broadwell_de/smmrelocate.c
@@ -190,22 +190,11 @@
    	write_prmrr(relo_params);
 }

-static u32 northbridge_get_base_reg(pci_devfn_t dev, int reg)
-{
-	u32 value;
-
-	value = pci_read_config32(dev, reg);
-	/* Base registers are at 1MiB granularity. */
-	value &= ~((1 << 20) - 1);
-	return value;
-}
-
 static void fill_in_relocation_params(pci_devfn_t dev,
    			      struct smm_relocation_params *params)
 {
-	u32 tseg_size;
-	u32 tseg_base;
-	u32 tseg_limit;
+	uintptr_t tseg_base;
+	size_t tseg_size;
    u32 prmrr_base;
    u32 prmrr_size;
    int phys_bits;
@@ -225,25 +214,18 @@
     * The result is that BASE[19:0] is effectively 00000h and LIMIT is
     * effectively FFFFFh.
     */
-	tseg_base = northbridge_get_base_reg(dev, TSEG_BASE);
-	tseg_limit = northbridge_get_base_reg(dev, TSEG_LIMIT) + 1 * MiB;
-	tseg_size = tseg_limit - tseg_base;

-	params->smram_base = tseg_base;
-	params->smram_size = 4 << 20;
-	params->ied_base = tseg_base + params->smram_size;
-	params->ied_size = tseg_size - params->smram_size;
-
-	/* Adjust available SMM handler memory size. */
-	params->smram_size -= CONFIG_SMM_RESERVED_SIZE;
+	smm_region(&tseg_base, &tseg_size);

/* SMRR has 32-bits of valid address aligned to 4KiB. */
-	params->smrr_base.lo = (params->smram_base & rmask) | MTRR_TYPE_WRBACK;
+	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.lo = (~(tseg_size - 1) & rmask) | MTRR_PHYS_MASK_VALID;
    params->smrr_mask.hi = 0;

+	smm_subregion(SMM_SUBREGION_HANDLER, &params->smram_base, &params->smram_size);
+	smm_subregion(SMM_SUBREGION_IED, &params->ied_base, &params->ied_size);
+
    /* The PRMRR is at IEDBASE + 2MiB */
    prmrr_base = (params->ied_base + (2 << 20)) & rmask;
    prmrr_size = params->ied_size - (2 << 20);

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

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

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

Patch Set 1: Code-Review-2

Flagging -2 until tested.

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

Hello Patrick Rudolph, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

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

intel/fsp_broadwell_de: Use smm_subregion()

Change-Id: I986bbe978d3f68693b2d4538ccbcc11cdbd23c6a
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/fsp_broadwell_de/include/soc/smm.h
M src/soc/intel/fsp_broadwell_de/memmap.c
M src/soc/intel/fsp_broadwell_de/smmrelocate.c
3 files changed, 27 insertions(+), 35 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34745/2

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

Hello Werner Zeh, Patrick Rudolph, Philipp Deppenwiese, build bot (Jenkins),

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

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

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

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

intel/fsp_broadwell_de: Use smm_subregion()

Change-Id: I986bbe978d3f68693b2d4538ccbcc11cdbd23c6a
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/fsp_broadwell_de/include/soc/smm.h
M src/soc/intel/fsp_broadwell_de/memmap.c
M src/soc/intel/fsp_broadwell_de/smmrelocate.c
3 files changed, 29 insertions(+), 49 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34745/8

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

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

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

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

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34745 )

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

Patch Set 11: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34745/11/src/soc/intel/fsp_broadwel... 
File src/soc/intel/fsp_broadwell_de/memmap.c:

https://review.coreboot.org/c/coreboot/+/34745/11/src/soc/intel/fsp_broadwel... 
PS11, Line 32: 	uintptr_t tseg = pci_read_config32(PCI_DEV(0, 0, 0), TSEG_BASE);
tseg is on the VTD devices

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

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

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

Patch Set 11:

Patrick, you should be able to squash this together with sa_get_tseg_base() commit if you wan to.

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded a new patch set (#12) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/34745 )

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

intel/fsp_broadwell_de: Use smm_subregion()

Tested on OCP/Wedge100s:
No error is visible in console output, still boots to OS.

Change-Id: I986bbe978d3f68693b2d4538ccbcc11cdbd23c6a
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/fsp_broadwell_de/include/soc/smm.h
M src/soc/intel/fsp_broadwell_de/memmap.c
M src/soc/intel/fsp_broadwell_de/smmrelocate.c
3 files changed, 23 insertions(+), 49 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34745/12

[Patrick Rudolph (Code Review)]

Patrick Rudolph has uploaded a new patch set (#14) to the change originally created by Kyösti Mälkki. ( https://review.coreboot.org/c/coreboot/+/34745 )

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

intel/fsp_broadwell_de: Use smm_subregion()

Tested on OCP/Wedge100s:
No error is visible in console output, still boots to OS.

Change-Id: I986bbe978d3f68693b2d4538ccbcc11cdbd23c6a
Signed-off-by: Kyösti Mälkki 
---
M src/soc/intel/fsp_broadwell_de/include/soc/smm.h
M src/soc/intel/fsp_broadwell_de/memmap.c
M src/soc/intel/fsp_broadwell_de/smmrelocate.c
3 files changed, 22 insertions(+), 49 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/34745/14

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

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

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

Patch Set 15: Code-Review+1

Thanks for testing and rebasing. I'll +2 once the includes from parent commit are fine.

[Angel Pons (Code Review)]

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

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

Patch Set 15: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34745/15/src/soc/intel/fsp_broadwel... 
File src/soc/intel/fsp_broadwell_de/memmap.c:

https://review.coreboot.org/c/coreboot/+/34745/15/src/soc/intel/fsp_broadwel... 
PS15, Line 53: sa_get_tseg_base
Is calling this twice (once here, once on smm_region) needed? I'd prefer to avoid that by e.g.: smm_get_tseg_size taking the base as argument, or returning the TSEG limit instead

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

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

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

Patch Set 15: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/34745/15/src/soc/intel/fsp_broadwel... 
File src/soc/intel/fsp_broadwell_de/memmap.c:

https://review.coreboot.org/c/coreboot/+/34745/15/src/soc/intel/fsp_broadwel... 
PS15, Line 53: sa_get_tseg_base
> Is calling this twice (once here, once on smm_region) needed? I'd prefer to avoid that by e.g. […]

I think it is intentional that sa_get_tseg_size() signature matches with some other intel SoC. Besides, with sa_get_tseg_base() so simple, I would not be surprised if compiler inlines it.

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

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

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

Patch Set 15:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34745/11/src/soc/intel/fsp_broadwel... 
File src/soc/intel/fsp_broadwell_de/memmap.c:

https://review.coreboot.org/c/coreboot/+/34745/11/src/soc/intel/fsp_broadwel... 
PS11, Line 32: 	uintptr_t tseg = pci_read_config32(PCI_DEV(0, 0, 0), TSEG_BASE);
> tseg is on the VTD devices

Done

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

Kyösti Mälkki has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/34745 )

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

intel/fsp_broadwell_de: Use smm_subregion()

Tested on OCP/Wedge100s:
No error is visible in console output, still boots to OS.

Change-Id: I986bbe978d3f68693b2d4538ccbcc11cdbd23c6a
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34745
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/fsp_broadwell_de/include/soc/smm.h
M src/soc/intel/fsp_broadwell_de/memmap.c
M src/soc/intel/fsp_broadwell_de/smmrelocate.c
3 files changed, 20 insertions(+), 49 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Kyösti Mälkki: Looks good to me, approved
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/src/soc/intel/fsp_broadwell_de/include/soc/smm.h b/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
index ae3b839..867bf60 100644
--- a/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
+++ b/src/soc/intel/fsp_broadwell_de/include/soc/smm.h
@@ -22,10 +22,8 @@

struct smm_relocation_params {
-	u32 smram_base;
-	u32 smram_size;
-	u32 ied_base;
-	u32 ied_size;
+	uintptr_t ied_base;
+	size_t ied_size;
    msr_t smrr_base;
    msr_t smrr_mask;
    msr_t prmrr_base;
@@ -36,17 +34,5 @@
    int smm_save_state_in_msrs;
 };

-/*
- * 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;
-}

#endif
diff --git a/src/soc/intel/fsp_broadwell_de/memmap.c b/src/soc/intel/fsp_broadwell_de/memmap.c
index 36f400c..cbd3cf7 100644
--- a/src/soc/intel/fsp_broadwell_de/memmap.c
+++ b/src/soc/intel/fsp_broadwell_de/memmap.c
@@ -17,6 +17,7 @@
 #define __SIMPLE_DEVICE__

#include <cbmem.h>
+#include <cpu/x86/smm.h>
 #include <drivers/intel/fsp1_0/fsp_util.h>
 #include <soc/broadwell_de.h>
 #include <soc/pci_devs.h>
@@ -51,3 +52,9 @@
    /* Subtract base to get the size */
    return ret - sa_get_tseg_base();
 }
+
+void smm_region(uintptr_t *start, size_t *size)
+{
+	*start = sa_get_tseg_base();
+	*size = sa_get_tseg_size();
+}
diff --git a/src/soc/intel/fsp_broadwell_de/smmrelocate.c b/src/soc/intel/fsp_broadwell_de/smmrelocate.c
index bd491c7..b0d3f14 100644
--- a/src/soc/intel/fsp_broadwell_de/smmrelocate.c
+++ b/src/soc/intel/fsp_broadwell_de/smmrelocate.c
@@ -192,22 +192,10 @@
    	write_prmrr(relo_params);
 }

-static u32 northbridge_get_base_reg(pci_devfn_t dev, int reg)
+static void fill_in_relocation_params(struct smm_relocation_params *params)
 {
-	u32 value;
-
-	value = pci_read_config32(dev, reg);
-	/* Base registers are at 1MiB granularity. */
-	value &= ~((1 << 20) - 1);
-	return value;
-}
-
-static void fill_in_relocation_params(pci_devfn_t dev,
-				      struct smm_relocation_params *params)
-{
-	u32 tseg_size;
-	u32 tseg_base;
-	u32 tseg_limit;
+	uintptr_t tseg_base;
+	size_t tseg_size;
    u32 prmrr_base;
    u32 prmrr_size;
    int phys_bits;
@@ -227,25 +215,17 @@
     * The result is that BASE[19:0] is effectively 00000h and LIMIT is
     * effectively FFFFFh.
     */
-	tseg_base = northbridge_get_base_reg(dev, TSEG_BASE);
-	tseg_limit = northbridge_get_base_reg(dev, TSEG_LIMIT) + 1 * MiB;
-	tseg_size = tseg_limit - tseg_base;

-	params->smram_base = tseg_base;
-	params->smram_size = 4 << 20;
-	params->ied_base = tseg_base + params->smram_size;
-	params->ied_size = tseg_size - params->smram_size;
-
-	/* Adjust available SMM handler memory size. */
-	params->smram_size -= CONFIG_SMM_RESERVED_SIZE;
+	smm_region(&tseg_base, &tseg_size);

/* SMRR has 32-bits of valid address aligned to 4KiB. */
-	params->smrr_base.lo = (params->smram_base & rmask) | MTRR_TYPE_WRBACK;
+	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.lo = (~(tseg_size - 1) & rmask) | MTRR_PHYS_MASK_VALID;
    params->smrr_mask.hi = 0;

+	smm_subregion(SMM_SUBREGION_CHIPSET, &params->ied_base, &params->ied_size);
+
    /* The PRMRR is at IEDBASE + 2MiB */
    prmrr_base = (params->ied_base + (2 << 20)) & rmask;
    prmrr_size = params->ied_size - (2 << 20);
@@ -281,16 +261,14 @@
 void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
    			size_t *smm_save_state_size)
 {
-	pci_devfn_t dev = PCI_DEV(BUS0, VTD_DEV, VTD_FUNC);
-
    printk(BIOS_DEBUG, "Setting up SMI for CPU\n");

-	fill_in_relocation_params(dev, &smm_reloc_params);
+	fill_in_relocation_params(&smm_reloc_params);
+
+	smm_subregion(SMM_SUBREGION_HANDLER, perm_smbase, perm_smsize);

setup_ied_area(&smm_reloc_params);

-	*perm_smbase = smm_reloc_params.smram_base;
-	*perm_smsize = smm_reloc_params.smram_size;
    *smm_save_state_size = sizeof(em64t101_smm_state_save_area_t);
 }