[coreboot-gerrit] [M] Change in coreboot[master]: soc/intel/xeon_sp: lock MSR_PPIN_CTL at BS_PAYLOAD_LOAD

[Martin L Roth (Code Review)]

Martin L Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/71144 )

Change subject: soc/intel/xeon_sp: lock MSR_PPIN_CTL at BS_PAYLOAD_LOAD
......................................................................

soc/intel/xeon_sp: lock MSR_PPIN_CTL at BS_PAYLOAD_LOAD

MSR_PPIN_CTL may need to be read more than once, so lock PPIN CTL
MSR at a late BS_PAYLOAD_LOAD boot state.

This MSR is in platform scope and must only be locked once on each
socket. Add a spinlock to do so.

Tested=On OCP Craterlake single socket, rdmsr -a 0x04e shows 1.

Signed-off-by: Johnny Lin 
Signed-off-by: Patrick Rudolph 
Change-Id: I8deb086339267cf36e41e16f189e1378f20b82f1
Reviewed-on: https://review.coreboot.org/c/coreboot/+/71144
Tested-by: build bot (Jenkins) 
Reviewed-by: David Hendricks 
---
M src/soc/intel/xeon_sp/finalize.c
M src/soc/intel/xeon_sp/util.c
2 files changed, 59 insertions(+), 3 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  David Hendricks: Looks good to me, approved

diff --git a/src/soc/intel/xeon_sp/finalize.c b/src/soc/intel/xeon_sp/finalize.c
index 76e3ef1..33a4d43 100644
--- a/src/soc/intel/xeon_sp/finalize.c
+++ b/src/soc/intel/xeon_sp/finalize.c
@@ -3,12 +3,15 @@
 #include <bootstate.h>
 #include <console/console.h>
 #include <console/debug.h>
+#include <cpu/x86/mp.h>
 #include <cpu/x86/smm.h>
 #include <device/pci.h>
 #include <intelpch/lockdown.h>
+#include <soc/msr.h>
 #include <soc/pci_devs.h>
 #include <soc/pm.h>
 #include <soc/util.h>
+#include <smp/spinlock.h>

#include "chip.h"

@@ -23,6 +26,32 @@
    pci_or_config32(dev, SAD_ALL_PAM0123_CSR, PAM_LOCK);
 }

+DECLARE_SPIN_LOCK(msr_ppin_lock);
+
+static void lock_msr_ppin_ctl(void *unused)
+{
+	msr_t msr;
+
+	msr = rdmsr(MSR_PLATFORM_INFO);
+	if ((msr.lo & MSR_PPIN_CAP) == 0)
+		return;
+
+	spin_lock(&msr_ppin_lock);
+
+	msr = rdmsr(MSR_PPIN_CTL);
+	if (msr.lo & MSR_PPIN_CTL_LOCK) {
+		spin_unlock(&msr_ppin_lock);
+		return;
+	}
+
+	/* Clear enable and lock it */
+	msr.lo &= ~MSR_PPIN_CTL_ENABLE;
+	msr.lo |= MSR_PPIN_CTL_LOCK;
+	wrmsr(MSR_PPIN_CTL, msr);
+
+	spin_unlock(&msr_ppin_lock);
+}
+
 static void soc_finalize(void *unused)
 {
    printk(BIOS_DEBUG, "Finalizing chipset.\n");
@@ -43,6 +72,14 @@
    apm_control(APM_CNT_FINALIZE);
    lock_pam0123();

+	if (CONFIG_MAX_SOCKET > 1) {
+		/* This MSR is package scope but run for all cpus for code simplicity */
+		if (mp_run_on_all_cpus(&lock_msr_ppin_ctl, NULL) != CB_SUCCESS)
+			printk(BIOS_ERR, "Lock PPIN CTL MSR failed\n");
+	} else {
+		lock_msr_ppin_ctl(NULL);
+	}
+
    post_code(POST_OS_BOOT);
 }

diff --git a/src/soc/intel/xeon_sp/util.c b/src/soc/intel/xeon_sp/util.c
index 0c8e63a..525efc5 100644
--- a/src/soc/intel/xeon_sp/util.c
+++ b/src/soc/intel/xeon_sp/util.c
@@ -68,9 +68,6 @@
    	wrmsr(MSR_PPIN_CTL, msr);
    }
    ppin = rdmsr(MSR_PPIN);
-	/* Set enable to 0 after reading MSR_PPIN */
-	msr.lo &= ~MSR_PPIN_CTL_ENABLE;
-	wrmsr(MSR_PPIN_CTL, msr);
    return ppin;
 }