Change in coreboot[master]: intel/smm/gen1: Split alternative SMRR register function

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

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

Change subject: intel/smm/gen1: Split alternative SMRR register function
......................................................................

intel/smm/gen1: Split alternative SMRR register function

The non-alternative one will have inlined version available
with the new header.

Change-Id: I208ac84fdf5d8041a1901cc2331769cd3a8d6bea
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/smm/gen1/smmrelocate.c
1 file changed, 27 insertions(+), 17 deletions(-)

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

diff --git a/src/cpu/intel/smm/gen1/smmrelocate.c b/src/cpu/intel/smm/gen1/smmrelocate.c
index c572b4a..d200805 100644
--- a/src/cpu/intel/smm/gen1/smmrelocate.c
+++ b/src/cpu/intel/smm/gen1/smmrelocate.c
@@ -76,27 +76,32 @@
    }
 }

+static void write_smrr_alt(struct smm_relocation_params *relo_params)
+{
+	msr_t msr;
+	msr = rdmsr(IA32_FEATURE_CONTROL);
+	/* SMRR enabled and feature locked */
+	if (!((msr.lo & SMRR_ENABLE)
+			&& (msr.lo & FEATURE_CONTROL_LOCK_BIT))) {
+		printk(BIOS_WARNING,
+			"SMRR not enabled, skip writing SMRR...\n");
+		return;
+	}
+
+	printk(BIOS_DEBUG, "Writing SMRR. base = 0x%08x, mask=0x%08x\n",
+	       relo_params->smrr_base.lo, relo_params->smrr_mask.lo);
+
+	wrmsr(MSR_SMRR_PHYS_BASE, relo_params->smrr_base);
+	wrmsr(MSR_SMRR_PHYS_MASK, relo_params->smrr_mask);
+}
+
 static void write_smrr(struct smm_relocation_params *relo_params)
 {
    printk(BIOS_DEBUG, "Writing SMRR. base = 0x%08x, mask=0x%08x\n",
           relo_params->smrr_base.lo, relo_params->smrr_mask.lo);

-	if (cpu_has_alternative_smrr()) {
-		msr_t msr;
-		msr = rdmsr(IA32_FEATURE_CONTROL);
-		/* SMRR enabled and feature locked */
-		if (!((msr.lo & SMRR_ENABLE)
-				&& (msr.lo & FEATURE_CONTROL_LOCK_BIT))) {
-			printk(BIOS_WARNING,
-				"SMRR not enabled, skip writing SMRR...\n");
-			return;
-		}
-		wrmsr(MSR_SMRR_PHYS_BASE, relo_params->smrr_base);
-		wrmsr(MSR_SMRR_PHYS_MASK, relo_params->smrr_mask);
-	} else {
-		wrmsr(IA32_SMRR_PHYS_BASE, relo_params->smrr_base);
-		wrmsr(IA32_SMRR_PHYS_MASK, relo_params->smrr_mask);
-	}
+	wrmsr(IA32_SMRR_PHYS_BASE, relo_params->smrr_base);
+	wrmsr(IA32_SMRR_PHYS_MASK, relo_params->smrr_mask);
 }

static void fill_in_relocation_params(struct smm_relocation_params *params)
@@ -208,7 +213,12 @@

/* Write EMRR and SMRR MSRs based on indicated support. */
    mtrr_cap = rdmsr(MTRR_CAP_MSR);
-	if (mtrr_cap.lo & SMRR_SUPPORTED)
+	if (!(mtrr_cap.lo & SMRR_SUPPORTED))
+		return;
+
+	if (cpu_has_alternative_smrr())
+		write_smrr_alt(relo_params);
+	else
    	write_smrr(relo_params);
 }

[Arthur Heymans (Code Review)]

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

Change subject: intel/smm/gen1: Split alternative SMRR register function
......................................................................

Patch Set 1: Code-Review+2

[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/+/34839

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

Change subject: intel/smm/gen1: Split alternative SMRR register function
......................................................................

intel/smm/gen1: Split alternative SMRR register function

The non-alternative one will have inlined version available
with the new header.

Change-Id: I208ac84fdf5d8041a1901cc2331769cd3a8d6bea
Signed-off-by: Kyösti Mälkki 
---
M src/cpu/intel/smm/gen1/smmrelocate.c
1 file changed, 27 insertions(+), 17 deletions(-)

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

[Angel Pons (Code Review)]

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

Change subject: intel/smm/gen1: Split alternative SMRR register function
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/34839/2/src/cpu/intel/smm/gen1/smmr... 
File src/cpu/intel/smm/gen1/smmrelocate.c:

https://review.coreboot.org/c/coreboot/+/34839/2/src/cpu/intel/smm/gen1/smmr... 
PS2, Line 86: if (!((msr.lo & SMRR_ENABLE)
            : 			&& (msr.lo & FEATURE_CONTROL_LOCK_BIT))) {
            : 		printk(BIOS_WARNING,
            : 			"SMRR not enabled, skip writing SMRR...\n");
Very minor: Can fit in 96 chars

[Angel Pons (Code Review)]

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

Change subject: intel/smm/gen1: Split alternative SMRR register function
......................................................................

Patch Set 2:

I noticed that most Intel big core chips since Haswell have their own write_smrr(), which happens to be the same...

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

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

Change subject: intel/smm/gen1: Split alternative SMRR register function
......................................................................

Patch Set 2:

(1 comment)

CB:34840

https://review.coreboot.org/c/coreboot/+/34839/2/src/cpu/intel/smm/gen1/smmr... 
File src/cpu/intel/smm/gen1/smmrelocate.c:

https://review.coreboot.org/c/coreboot/+/34839/2/src/cpu/intel/smm/gen1/smmr... 
PS2, Line 86: if (!((msr.lo & SMRR_ENABLE)
            : 			&& (msr.lo & FEATURE_CONTROL_LOCK_BIT))) {
            : 		printk(BIOS_WARNING,
            : 			"SMRR not enabled, skip writing SMRR...\n");
> Very minor: Can fit in 96 chars

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/+/34839 )

Change subject: intel/smm/gen1: Split alternative SMRR register function
......................................................................

intel/smm/gen1: Split alternative SMRR register function

The non-alternative one will have inlined version available
with the new header.

Change-Id: I208ac84fdf5d8041a1901cc2331769cd3a8d6bea
Signed-off-by: Kyösti Mälkki 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/34839
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
---
M src/cpu/intel/smm/gen1/smmrelocate.c
1 file changed, 27 insertions(+), 17 deletions(-)

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

diff --git a/src/cpu/intel/smm/gen1/smmrelocate.c b/src/cpu/intel/smm/gen1/smmrelocate.c
index fd98c30..f196706 100644
--- a/src/cpu/intel/smm/gen1/smmrelocate.c
+++ b/src/cpu/intel/smm/gen1/smmrelocate.c
@@ -78,27 +78,32 @@
    }
 }

+static void write_smrr_alt(struct smm_relocation_params *relo_params)
+{
+	msr_t msr;
+	msr = rdmsr(IA32_FEATURE_CONTROL);
+	/* SMRR enabled and feature locked */
+	if (!((msr.lo & SMRR_ENABLE)
+			&& (msr.lo & FEATURE_CONTROL_LOCK_BIT))) {
+		printk(BIOS_WARNING,
+			"SMRR not enabled, skip writing SMRR...\n");
+		return;
+	}
+
+	printk(BIOS_DEBUG, "Writing SMRR. base = 0x%08x, mask=0x%08x\n",
+	       relo_params->smrr_base.lo, relo_params->smrr_mask.lo);
+
+	wrmsr(MSR_SMRR_PHYS_BASE, relo_params->smrr_base);
+	wrmsr(MSR_SMRR_PHYS_MASK, relo_params->smrr_mask);
+}
+
 static void write_smrr(struct smm_relocation_params *relo_params)
 {
    printk(BIOS_DEBUG, "Writing SMRR. base = 0x%08x, mask=0x%08x\n",
           relo_params->smrr_base.lo, relo_params->smrr_mask.lo);

-	if (cpu_has_alternative_smrr()) {
-		msr_t msr;
-		msr = rdmsr(IA32_FEATURE_CONTROL);
-		/* SMRR enabled and feature locked */
-		if (!((msr.lo & SMRR_ENABLE)
-				&& (msr.lo & FEATURE_CONTROL_LOCK_BIT))) {
-			printk(BIOS_WARNING,
-				"SMRR not enabled, skip writing SMRR...\n");
-			return;
-		}
-		wrmsr(MSR_SMRR_PHYS_BASE, relo_params->smrr_base);
-		wrmsr(MSR_SMRR_PHYS_MASK, relo_params->smrr_mask);
-	} else {
-		wrmsr(IA32_SMRR_PHYS_BASE, relo_params->smrr_base);
-		wrmsr(IA32_SMRR_PHYS_MASK, relo_params->smrr_mask);
-	}
+	wrmsr(IA32_SMRR_PHYS_BASE, relo_params->smrr_base);
+	wrmsr(IA32_SMRR_PHYS_MASK, relo_params->smrr_mask);
 }

static void fill_in_relocation_params(struct smm_relocation_params *params)
@@ -235,7 +240,12 @@

/* Write EMRR and SMRR MSRs based on indicated support. */
    mtrr_cap = rdmsr(MTRR_CAP_MSR);
-	if (mtrr_cap.lo & SMRR_SUPPORTED && relo_params->smrr_mask.lo != 0)
+	if (!(mtrr_cap.lo & SMRR_SUPPORTED && relo_params->smrr_mask.lo != 0))
+		return;
+
+	if (cpu_has_alternative_smrr())
+		write_smrr_alt(relo_params);
+	else
    	write_smrr(relo_params);
 }