Change in coreboot[master]: soc/intel/tigerlake: Add SMRR Locking support

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45013 )

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

soc/intel/tigerlake: Add SMRR Locking support

The SMRR MSRs can be locked, so that a further write to them will cause
a #GP.  This patch adds that functionality, but since the MSR is a
core-level register, it must only be done once per core; if the SoC has
hyperthreading enabled, then attempting to write the SMRR Lock bit on
the primary thread will cause a #GP when the secondary (sibling) thread
attempts to also write to this MSR.

BUG=b:164489598
TEST=Boot into OS, verify using `iotools rdmsr` that all threads have
the Lock bit set.

Signed-off-by: Tim Wawrzynczak 
Change-Id: I4ae7c7f703bdf090144637d071eb810617d9e309
---
M src/soc/intel/tigerlake/Kconfig
M src/soc/intel/tigerlake/smmrelocate.c
2 files changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/45013/1

diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig
index 8718f97..eb0cb77 100644
--- a/src/soc/intel/tigerlake/Kconfig
+++ b/src/soc/intel/tigerlake/Kconfig
@@ -16,6 +16,7 @@
    select BOOT_DEVICE_SUPPORTS_WRITES
    select CACHE_MRC_SETTINGS
    select CPU_INTEL_COMMON
+	select CPU_INTEL_COMMON_HYPERTHREADING
    select CPU_INTEL_FIRMWARE_INTERFACE_TABLE
    select FSP_COMPRESS_FSP_S_LZ4
    select FSP_M_XIP
diff --git a/src/soc/intel/tigerlake/smmrelocate.c b/src/soc/intel/tigerlake/smmrelocate.c
index bbdcb68..3dada3e 100644
--- a/src/soc/intel/tigerlake/smmrelocate.c
+++ b/src/soc/intel/tigerlake/smmrelocate.c
@@ -9,6 +9,7 @@
 #include <cpu/x86/msr.h>
 #include <cpu/x86/mtrr.h>
 #include <cpu/x86/smm.h>
+#include <cpu/intel/common/common.h>
 #include <cpu/intel/em64t101_save_state.h>
 #include <cpu/intel/smm_reloc.h>
 #include <console/console.h>
@@ -139,7 +140,13 @@

/* Write SMRR MSRs based on indicated support. */
    mtrr_cap = rdmsr(MTRR_CAP_MSR);
-	if (mtrr_cap.lo & SMRR_SUPPORTED)
+
+	/* Set Lock bit if supported */
+	if (mtrr_cap.lo & SMRR_LOCK_SUPPORTED)
+		relo_params->mask.lo |= SMRR_PHYS_MASK_LOCK;
+
+	/* Write SMRRs (if supported) on each *core* only */
+	if ((mtrr_cap.lo & SMRR_SUPPORTED) && !intel_ht_sibling())
    	write_smrr(relo_params);
 }

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Subrata Banik, Kane Chen, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

soc/intel/tigerlake: Add SMRR Locking support

The SMRR MSRs can be locked, so that a further write to them will cause
a #GP.  This patch adds that functionality, but since the MSR is a
core-level register, it must only be done once per core; if the SoC has
hyperthreading enabled, then attempting to write the SMRR Lock bit on
the primary thread will cause a #GP when the secondary (sibling) thread
attempts to also write to this MSR.

BUG=b:164489598
TEST=Boot into OS, verify using `iotools rdmsr` that all threads have
the Lock bit set.

Signed-off-by: Tim Wawrzynczak 
Change-Id: I4ae7c7f703bdf090144637d071eb810617d9e309
---
M src/soc/intel/tigerlake/Kconfig
M src/soc/intel/tigerlake/smmrelocate.c
2 files changed, 9 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/45013/2

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45013 )

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

Patch Set 3: Code-Review+1

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45013 )

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45013/4/src/soc/intel/tigerlake/smm... 
File src/soc/intel/tigerlake/smmrelocate.c:

https://review.coreboot.org/c/coreboot/+/45013/4/src/soc/intel/tigerlake/smm... 
PS4, Line 144: 	/* Set Lock bit if supported */
             : 	if (mtrr_cap.lo & SMRR_LOCK_SUPPORTED)
             : 		relo_params->smrr_mask.lo |= SMRR_PHYS_MASK_LOCK;
             : 
             : 	/* Write SMRRs (if supported) on each *core* only */
             : 	if ((mtrr_cap.lo & SMRR_SUPPORTED) && !intel_ht_sibling())
how about this? then the sibling doesn't need to execute other code which is not necessary.
thanks.

if (intel_ht_sibling())
    return;
/* Write SMRR MSRs based on indicated support. */
    mtrr_cap = rdmsr(MTRR_CAP_MSR);
/* Set Lock bit if supported */
if (mtrr_cap.lo & SMRR_LOCK_SUPPORTED)
    relo_params->smrr_mask.lo |= SMRR_PHYS_MASK_LOCK;

/* Write SMRRs (if supported) on each *core* only */
if ((mtrr_cap.lo & SMRR_SUPPORTED))
...
Or

if (!intel_ht_sibling()) {
    //* Write SMRR MSRs based on indicated support. */
    mtrr_cap = rdmsr(MTRR_CAP_MSR);
    /* Set Lock bit if supported */
    if (mtrr_cap.lo & SMRR_LOCK_SUPPORTED)
    	relo_params->smrr_mask.lo |= SMRR_PHYS_MASK_LOCK;

/* Write SMRRs (if supported) on each *core* only */
    if ((mtrr_cap.lo & SMRR_SUPPORTED))
....
}

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Subrata Banik, Kane Chen, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

soc/intel/tigerlake: Add SMRR Locking support

The SMRR MSRs can be locked, so that a further write to them will cause
a #GP.  This patch adds that functionality, but since the MSR is a
core-level register, it must only be done once per core; if the SoC has
hyperthreading enabled, then attempting to write the SMRR Lock bit on
the primary thread will cause a #GP when the secondary (sibling) thread
attempts to also write to this MSR.

BUG=b:164489598
TEST=Boot into OS, verify using `iotools rdmsr` that all threads have
the Lock bit set.

Signed-off-by: Tim Wawrzynczak 
Change-Id: I4ae7c7f703bdf090144637d071eb810617d9e309
---
M src/soc/intel/tigerlake/Kconfig
M src/soc/intel/tigerlake/smmrelocate.c
2 files changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/45013/5

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45013 )

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/45013/4/src/soc/intel/tigerlake/smm... 
File src/soc/intel/tigerlake/smmrelocate.c:

https://review.coreboot.org/c/coreboot/+/45013/4/src/soc/intel/tigerlake/smm... 
PS4, Line 144: 	/* Set Lock bit if supported */
             : 	if (mtrr_cap.lo & SMRR_LOCK_SUPPORTED)
             : 		relo_params->smrr_mask.lo |= SMRR_PHYS_MASK_LOCK;
             : 
             : 	/* Write SMRRs (if supported) on each *core* only */
             : 	if ((mtrr_cap.lo & SMRR_SUPPORTED) && !intel_ht_sibling())
> how about this? then the sibling doesn't need to execute other code which is not necessary. […]

SGTM

[Tim Wawrzynczak (Code Review)]

Hello build bot (Jenkins), Furquan Shaikh, Caveh Jalali, Subrata Banik, Kane Chen, Patrick Rudolph,

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

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

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

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

soc/intel/tigerlake: Add SMRR Locking support

The SMRR MSRs can be locked, so that a further write to them will cause
a #GP.  This patch adds that functionality, but since the MSR is a
core-level register, it must only be done once per core; if the SoC has
hyperthreading enabled, then attempting to write the SMRR Lock bit on
the primary thread will cause a #GP when the secondary (sibling) thread
attempts to also write to this MSR.

BUG=b:164489598
TEST=Boot into OS, verify using `iotools rdmsr` that all threads have
the Lock bit set.

Signed-off-by: Tim Wawrzynczak 
Change-Id: I4ae7c7f703bdf090144637d071eb810617d9e309
---
M src/soc/intel/tigerlake/Kconfig
M src/soc/intel/tigerlake/smmrelocate.c
2 files changed, 18 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/45013/6

[Kane Chen (Code Review)]

Kane Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45013 )

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

Patch Set 6: Code-Review+2

[Caveh Jalali (Code Review)]

Caveh Jalali has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45013 )

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

Patch Set 6: Code-Review+1

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45013 )

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

Patch Set 6: Code-Review+2

[Angel Pons (Code Review)]

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

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

Patch Set 6: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45013 )

Change subject: soc/intel/tigerlake: Add SMRR Locking support
......................................................................

soc/intel/tigerlake: Add SMRR Locking support

The SMRR MSRs can be locked, so that a further write to them will cause
a #GP.  This patch adds that functionality, but since the MSR is a
core-level register, it must only be done once per core; if the SoC has
hyperthreading enabled, then attempting to write the SMRR Lock bit on
the primary thread will cause a #GP when the secondary (sibling) thread
attempts to also write to this MSR.

BUG=b:164489598
TEST=Boot into OS, verify using `iotools rdmsr` that all threads have
the Lock bit set.

Signed-off-by: Tim Wawrzynczak 
Change-Id: I4ae7c7f703bdf090144637d071eb810617d9e309
Reviewed-on: https://review.coreboot.org/c/coreboot/+/45013
Tested-by: build bot (Jenkins) 
Reviewed-by: Kane Chen 
Reviewed-by: Caveh Jalali 
Reviewed-by: Subrata Banik 
Reviewed-by: Angel Pons 
---
M src/soc/intel/tigerlake/Kconfig
M src/soc/intel/tigerlake/smmrelocate.c
2 files changed, 18 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Kane Chen: Looks good to me, approved
  Subrata Banik: Looks good to me, approved
  Caveh Jalali: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, approved

diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig
index 182c5ad..cdec3ef 100644
--- a/src/soc/intel/tigerlake/Kconfig
+++ b/src/soc/intel/tigerlake/Kconfig
@@ -16,6 +16,7 @@
    select BOOT_DEVICE_SUPPORTS_WRITES
    select CACHE_MRC_SETTINGS
    select CPU_INTEL_COMMON
+	select CPU_INTEL_COMMON_HYPERTHREADING
    select CPU_INTEL_FIRMWARE_INTERFACE_TABLE
    select FSP_COMPRESS_FSP_S_LZ4
    select FSP_M_XIP
diff --git a/src/soc/intel/tigerlake/smmrelocate.c b/src/soc/intel/tigerlake/smmrelocate.c
index bbdcb68..e75e884 100644
--- a/src/soc/intel/tigerlake/smmrelocate.c
+++ b/src/soc/intel/tigerlake/smmrelocate.c
@@ -9,6 +9,7 @@
 #include <cpu/x86/msr.h>
 #include <cpu/x86/mtrr.h>
 #include <cpu/x86/smm.h>
+#include <cpu/intel/common/common.h>
 #include <cpu/intel/em64t101_save_state.h>
 #include <cpu/intel/smm_reloc.h>
 #include <console/console.h>
@@ -137,8 +138,24 @@
    /* Make appropriate changes to the save state map. */
    update_save_state(cpu, curr_smbase, staggered_smbase, relo_params);

+	/*
+	 * The SMRR MSRs are core-level registers, so if two threads that share
+	 * a core try to both set the lock bit (in the same physical register),
+	 * a #GP will be raised on the second write to that register (which is
+	 * exactly what the lock is supposed to do), therefore secondary threads
+	 * should exit here.
+	 */
+	if (intel_ht_sibling())
+		return;
+
    /* Write SMRR MSRs based on indicated support. */
    mtrr_cap = rdmsr(MTRR_CAP_MSR);
+
+	/* Set Lock bit if supported */
+	if (mtrr_cap.lo & SMRR_LOCK_SUPPORTED)
+		relo_params->smrr_mask.lo |= SMRR_PHYS_MASK_LOCK;
+
+	/* Write SMRRs if supported */
    if (mtrr_cap.lo & SMRR_SUPPORTED)
    	write_smrr(relo_params);
 }