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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/Kconfig
M src/cpu/intel/haswell/haswell.h
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/haswell.h
M src/northbridge/intel/haswell/memmap.c
5 files changed, 61 insertions(+), 55 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34737/1

diff --git a/src/cpu/intel/haswell/Kconfig b/src/cpu/intel/haswell/Kconfig
index 8f91b60..90ac679 100644
--- a/src/cpu/intel/haswell/Kconfig
+++ b/src/cpu/intel/haswell/Kconfig
@@ -28,10 +28,6 @@
    hex
    default 0x800000

-config IED_REGION_SIZE
-	hex
-	default 0x400000
-
 config SMM_RESERVED_SIZE
    hex
    default 0x100000
diff --git a/src/cpu/intel/haswell/haswell.h b/src/cpu/intel/haswell/haswell.h
index cfd9d45..bdfce8d 100644
--- a/src/cpu/intel/haswell/haswell.h
+++ b/src/cpu/intel/haswell/haswell.h
@@ -118,19 +118,12 @@
 /* Data is passed through bits 31:0 of the data register. */
 #define BIOS_MAILBOX_DATA			0x5da0

-/* Sanity check config options. */
-#if (CONFIG_SMM_TSEG_SIZE <= (CONFIG_IED_REGION_SIZE + CONFIG_SMM_RESERVED_SIZE))
-# error "CONFIG_SMM_TSEG_SIZE <= (CONFIG_IED_REGION_SIZE + CONFIG_SMM_RESERVED_SIZE)"
-#endif
 #if (CONFIG_SMM_TSEG_SIZE < 0x800000)
 # error "CONFIG_SMM_TSEG_SIZE must at least be 8MiB"
 #endif
 #if ((CONFIG_SMM_TSEG_SIZE & (CONFIG_SMM_TSEG_SIZE - 1)) != 0)
 # error "CONFIG_SMM_TSEG_SIZE is not a power of 2"
 #endif
-#if ((CONFIG_IED_REGION_SIZE & (CONFIG_IED_REGION_SIZE - 1)) != 0)
-# error "CONFIG_IED_REGION_SIZE is not a power of 2"
-#endif

#if !defined(__ROMCC__) // FIXME romcc should handle below constructs

diff --git a/src/cpu/intel/haswell/smmrelocate.c b/src/cpu/intel/haswell/smmrelocate.c
index 3a4a0a7..ca2c68f 100644
--- a/src/cpu/intel/haswell/smmrelocate.c
+++ b/src/cpu/intel/haswell/smmrelocate.c
@@ -46,10 +46,10 @@
 #define EMRR_SUPPORTED (1 << 12)

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 emrr_base;
@@ -210,22 +210,12 @@
    }
 }

-static u32 northbridge_get_base_reg(struct device *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(struct device *dev,
    			      struct smm_relocation_params *params)
 {
-	u32 tseg_size;
-	u32 tsegmb;
-	u32 bgsm;
+	uintptr_t tseg_base;
+	size_t tseg_size;
+
    u32 emrr_base;
    u32 emrr_size;
    int phys_bits;
@@ -240,25 +230,17 @@
     * SMRAM range as well as the IED range. However, the SMRAM available
     * to the handler is 4MiB since the IEDRAM lives TSEGMB + 4MiB.
     */
-	tsegmb = northbridge_get_base_reg(dev, TSEG);
-	bgsm = northbridge_get_base_reg(dev, BGSM);
-	tseg_size = bgsm - tsegmb;
-
-	params->smram_base = tsegmb;
-	params->smram_size = 4 << 20;
-	params->ied_base = tsegmb + 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 EMRR and UNCORE_EMRR are at IEDBASE + 2MiB */
    emrr_base = (params->ied_base + (2 << 20)) & rmask;
    emrr_size = params->ied_size - (2 << 20);
diff --git a/src/northbridge/intel/haswell/haswell.h b/src/northbridge/intel/haswell/haswell.h
index dc0e5db..a4c6934 100644
--- a/src/northbridge/intel/haswell/haswell.h
+++ b/src/northbridge/intel/haswell/haswell.h
@@ -22,9 +22,6 @@
 #define HASWELL_DESKTOP	1
 #define HASWELL_SERVER	2

-/* Intel Enhanced Debug region */
-#define IED_SIZE	CONFIG_IED_REGION_SIZE
-
 /* Northbridge BARs */
 #define DEFAULT_MCHBAR		0xfed10000	/* 16 KB */
 #ifndef __ACPI__
@@ -225,6 +222,14 @@
 void report_platform_info(void);
 #endif /* !__SMM__ */

+/* Intel Enhanced Debug region must be 4MB */
+#define FIXED_IED_REGION_SIZE	(4 << 20)
+
+/* Sanity check config options. */
+#if (CONFIG_SMM_TSEG_SIZE <= (FIXED_IED_REGION_SIZE + CONFIG_SMM_RESERVED_SIZE))
+# error "CONFIG_SMM_TSEG_SIZE <= (FIXED_IED_REGION_SIZE + CONFIG_SMM_RESERVED_SIZE)"
+#endif
+
 #if ENV_RAMSTAGE && !defined(__SIMPLE_DEVICE__)
 #include <device/device.h>

diff --git a/src/northbridge/intel/haswell/memmap.c b/src/northbridge/intel/haswell/memmap.c
index 3a63afc..02b9572 100644
--- a/src/northbridge/intel/haswell/memmap.c
+++ b/src/northbridge/intel/haswell/memmap.c
@@ -18,6 +18,7 @@

#include <device/pci_ops.h>
 #include <cbmem.h>
+#include <cpu/x86/smm.h>
 #include <stage_cache.h>
 #include "haswell.h"

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

-/* Region of SMM space is reserved for multipurpose use. It falls below
- * the IED region and above the SMM handler. */
-#define RESERVED_SMM_OFFSET \
-	(CONFIG_SMM_TSEG_SIZE - CONFIG_IED_REGION_SIZE - CONFIG_SMM_RESERVED_SIZE)
-
-void stage_cache_external_region(void **base, size_t *size)
+void smm_region(uintptr_t *start, size_t *size)
 {
-	/* The ramstage cache lives in the TSEG region at RESERVED_SMM_OFFSET.
-	 * The top of RAM is defined to be the TSEG base address. */
-	*size = CONFIG_SMM_RESERVED_SIZE;
-	*base = (void *)((uint32_t)cbmem_top() + RESERVED_SMM_OFFSET);
+	*start = ALIGN_DOWN(smm_region_start(), 1*MiB);
+	*size = CONFIG_SMM_TSEG_SIZE;
+}
+
+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;
 }

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34737 )

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

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/34737/1/src/northbridge/intel/haswe... 
File src/northbridge/intel/haswell/memmap.c:

https://review.coreboot.org/c/coreboot/+/34737/1/src/northbridge/intel/haswe... 
PS1, Line 42: ALIGN_DOWN
Isn't this already done above in smm_region_start()?

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

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

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

Patch Set 1: Code-Review-2

Flagging -2 until tested.

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

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/memmap.c
2 files changed, 18 insertions(+), 46 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34737/2

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

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

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

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/34737/1/src/northbridge/intel/haswe... 
File src/northbridge/intel/haswell/memmap.c:

https://review.coreboot.org/c/coreboot/+/34737/1/src/northbridge/intel/haswe... 
PS1, Line 42: ALIGN_DOWN
> Isn't this already done above in smm_region_start()?

Done

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

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/memmap.c
2 files changed, 17 insertions(+), 48 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34737/3

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

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/memmap.c
2 files changed, 17 insertions(+), 48 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34737/4

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

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/memmap.c
2 files changed, 19 insertions(+), 49 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34737/6

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

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/memmap.c
2 files changed, 60 insertions(+), 90 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34737/8

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

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/memmap.c
2 files changed, 19 insertions(+), 49 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34737/9

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

Hello Patrick Rudolph, Arthur Heymans, build bot (Jenkins),

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

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

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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/memmap.c
2 files changed, 17 insertions(+), 48 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/34737/10

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

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

Change subject: intel/haswell: 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/+/34737 )

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

Patch Set 12: Code-Review+2

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

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

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

intel/haswell: Use smm_subregion()

Change-Id: Idfb13ab03d4d4ae764bdda62a29848db9d8dcd81
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34737
Reviewed-by: Angel Pons 
Tested-by: build bot (Jenkins) 
---
M src/cpu/intel/haswell/smmrelocate.c
M src/northbridge/intel/haswell/memmap.c
2 files changed, 17 insertions(+), 48 deletions(-)

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

diff --git a/src/cpu/intel/haswell/smmrelocate.c b/src/cpu/intel/haswell/smmrelocate.c
index e3b9c38..1eb6f68 100644
--- a/src/cpu/intel/haswell/smmrelocate.c
+++ b/src/cpu/intel/haswell/smmrelocate.c
@@ -48,10 +48,8 @@
 #define PRMRR_SUPPORTED (1 << 12)

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;
@@ -212,22 +210,11 @@
    }
 }

-static u32 northbridge_get_base_reg(struct device *dev, int reg)
+static void fill_in_relocation_params(struct smm_relocation_params *params)
 {
-	u32 value;
+	uintptr_t tseg_base;
+	size_t tseg_size;

-	value = pci_read_config32(dev, reg);
-	/* Base registers are at 1MiB granularity. */
-	value &= ~((1 << 20) - 1);
-	return value;
-}
-
-static void fill_in_relocation_params(struct device *dev,
-				      struct smm_relocation_params *params)
-{
-	u32 tseg_size;
-	u32 tsegmb;
-	u32 bgsm;
    u32 prmrr_base;
    u32 prmrr_size;
    int phys_bits;
@@ -242,25 +229,16 @@
     * SMRAM range as well as the IED range. However, the SMRAM available
     * to the handler is 4MiB since the IEDRAM lives TSEGMB + 4MiB.
     */
-	tsegmb = northbridge_get_base_reg(dev, TSEG);
-	bgsm = northbridge_get_base_reg(dev, BGSM);
-	tseg_size = bgsm - tsegmb;
-
-	params->smram_base = tsegmb;
-	params->smram_size = 4 << 20;
-	params->ied_base = tsegmb + 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 and UNCORE_PRMRR are at IEDBASE + 2MiB */
    prmrr_base = (params->ied_base + (2 << 20)) & rmask;
    prmrr_size = params->ied_size - (2 << 20);
@@ -310,16 +288,14 @@
 void smm_info(uintptr_t *perm_smbase, size_t *perm_smsize,
    			size_t *smm_save_state_size)
 {
-	struct device *dev = pcidev_on_root(0, 0);
-
    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);
 }

diff --git a/src/northbridge/intel/haswell/memmap.c b/src/northbridge/intel/haswell/memmap.c
index 5bc74f8..b1eb770 100644
--- a/src/northbridge/intel/haswell/memmap.c
+++ b/src/northbridge/intel/haswell/memmap.c
@@ -20,9 +20,9 @@
 #include <console/console.h>
 #include <commonlib/helpers.h>
 #include <cpu/x86/mtrr.h>
+#include <cpu/x86/smm.h>
 #include <device/pci_ops.h>
 #include <cbmem.h>
-#include <stage_cache.h>
 #include "haswell.h"

static uintptr_t smm_region_start(void)
@@ -40,17 +40,10 @@
    return (void *)smm_region_start();
 }

-/* Region of SMM space is reserved for multipurpose use. It falls below
- * the IED region and above the SMM handler. */
-#define RESERVED_SMM_OFFSET \
-	(CONFIG_SMM_TSEG_SIZE - CONFIG_IED_REGION_SIZE - CONFIG_SMM_RESERVED_SIZE)
-
-void stage_cache_external_region(void **base, size_t *size)
+void smm_region(uintptr_t *start, size_t *size)
 {
-	/* The ramstage cache lives in the TSEG region at RESERVED_SMM_OFFSET.
-	 * The top of RAM is defined to be the TSEG base address. */
-	*size = CONFIG_SMM_RESERVED_SIZE;
-	*base = (void *)((uint32_t)cbmem_top() + RESERVED_SMM_OFFSET);
+	*start = smm_region_start();
+	*size = CONFIG_SMM_TSEG_SIZE;
 }

void fill_postcar_frame(struct postcar_frame *pcf)