Change in coreboot[master]: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU

[Johnny Lin (Code Review)]

Johnny Lin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU
......................................................................

soc/xeon_sp: Read PPIN MSR and save to an array for each CPU

PPIN (Protected Processor Inventory Number) MSR is read and saved to an
array during xeon_sp_core_init() for each CPU for later use, such as
SMBIOS type 11 OEM string population.

Tested on OCP Tioga Pass.
Signed-off-by: Johnny Lin

Change-Id: I5e1de8bcb651fb8ae8b106db1978235b0dd84c47
---
M src/soc/intel/xeon_sp/skx/cpu.c
M src/soc/intel/xeon_sp/skx/include/soc/msr.h
2 files changed, 22 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/40523/1

diff --git a/src/soc/intel/xeon_sp/skx/cpu.c b/src/soc/intel/xeon_sp/skx/cpu.c
index fcee02f..7072285 100644
--- a/src/soc/intel/xeon_sp/skx/cpu.c
+++ b/src/soc/intel/xeon_sp/skx/cpu.c
@@ -25,8 +25,21 @@
 #include <assert.h>
 #include "chip.h"

+msr_t xeon_sp_ppin[MAX_SOCKET] = {0};
+static uint32_t core_bits, thread_bits;
+static uint32_t socket_index = (1 << MAX_SOCKET) - 1;
 static const config_t *chip_config = NULL;

+static void save_ppin(msr_t ppin, unsigned int apic_id)
+{
+	uint8_t package;
+
+	get_cpu_info_from_apicid(apic_id, core_bits, thread_bits, &package, NULL, NULL);
+	/* Save to the corresponding CPU PPIN. */
+	xeon_sp_ppin[package] = ppin;
+	socket_index &= ~(1UL << package);
+}
+
 static void xeon_configure_mca(void)
 {
    msr_t msr;
@@ -59,6 +72,10 @@
    	__func__, dev_path(cpu), cpu_index(), cpu->path.apic.apic_id);
    assert(chip_config != NULL);

+	/* If socket_index is 0 then all PPIN have been saved. */
+	if (socket_index)
+		save_ppin(rdmsr(MSR_PPIN), cpu->path.apic.apic_id);
+
    /* set MSR_PKG_CST_CONFIG_CONTROL - scope per core*/
    msr.hi = 0;
    msr.lo = (PKG_CSTATE_NO_LIMIT | IO_MWAIT_REDIRECTION_ENABLE | CFG_LOCK_ENABLE);
@@ -246,7 +263,7 @@
    chip_config = dev->chip_info;

config_reset_cpl3_csrs();
-
+	get_core_thread_bits(&core_bits, &thread_bits);
    /* calls src/cpu/x86/mp_init.c */
    if (mp_init_with_smm(dev->link_list, &mp_ops) < 0)
    	printk(BIOS_ERR, "MP initialization failure.\n");
diff --git a/src/soc/intel/xeon_sp/skx/include/soc/msr.h b/src/soc/intel/xeon_sp/skx/include/soc/msr.h
index 9505776..4f569e1 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/msr.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/msr.h
@@ -17,6 +17,7 @@
 #define _SOC_MSR_H_

#include <intelblocks/msr.h>
+#include <cpu/x86/msr.h>

#define IA32_MCG_CAP                    0x179
 #define IA32_MCG_CAP_COUNT_MASK         0xff
@@ -109,4 +110,7 @@
 #define EPB_ENERGY_POLICY_SHIFT      3
 #define EPB_ENERGY_POLICY_MASK       (0xf << EPB_ENERGY_POLICY_SHIFT)

+/* MSR Protected Processor Inventory Number */
+#define MSR_PPIN		     0x04F
+extern msr_t xeon_sp_ppin[];
 #endif /* _SOC_MSR_H_ */

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU
......................................................................

Patch Set 1:

Reading MSR for a remote CPU socket seems not available in coreboot, so piggyback on xeon_sp_core_init() that gets to run on each CPU core and read their MSRs. May not be the best way
to do it, maybe rely on FSP to report PPIN via HOB is another option.

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Andrey Petrov, Jonathan Zhang, David Hendricks, Jingle Hsu, Morgan Jang, Patrick Rudolph,

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

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

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

Change subject: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU
......................................................................

soc/xeon_sp: Read PPIN MSR and save to an array for each CPU

PPIN (Protected Processor Inventory Number) MSR is read and saved to an
array during xeon_sp_core_init() for each CPU for later use, such as
SMBIOS type 11 OEM string population.

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I5e1de8bcb651fb8ae8b106db1978235b0dd84c47
---
M src/soc/intel/xeon_sp/skx/cpu.c
M src/soc/intel/xeon_sp/skx/include/soc/msr.h
2 files changed, 22 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/40523/5

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU
......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
File src/soc/intel/xeon_sp/skx/cpu.c:

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
PS5, Line 75: 	/* If socket_index is 0 then all PPIN have been saved. */
I wonder if this logic is over complicated. We want to read PPIN MSR from one core/thread of each socket. So we can just read PPIN MSR of core 0, thread 0 of each socket (or PBSP, SBSP).

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Andrey Petrov, Jonathan Zhang, David Hendricks, Jingle Hsu, Morgan Jang, Patrick Rudolph,

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

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

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

Change subject: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU
......................................................................

soc/xeon_sp: Read PPIN MSR and save to an array for each CPU

PPIN (Protected Processor Inventory Number) MSR is read and saved to an
array during xeon_sp_core_init() for each CPU for later use, such as
SMBIOS type 11 OEM string population.

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I5e1de8bcb651fb8ae8b106db1978235b0dd84c47
---
M src/soc/intel/xeon_sp/skx/cpu.c
M src/soc/intel/xeon_sp/skx/include/soc/msr.h
2 files changed, 22 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/40523/6

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
File src/soc/intel/xeon_sp/skx/cpu.c:

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
PS5, Line 75: 	/* If socket_index is 0 then all PPIN have been saved. */
> I wonder if this logic is over complicated. […]

You are right, but I can't find a way to only read the MSR on a core in the 2nd socket without running on all cores. mp_run_on_aps() is similar to this need but it still runs on all APs. I think FSP should be able to provide the PPIN for each socket, or maybe until there's a more ideal way we only read socket0 PPIN?

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
File src/soc/intel/xeon_sp/skx/cpu.c:

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
PS5, Line 75: 	/* If socket_index is 0 then all PPIN have been saved. */
> You are right, but I can't find a way to only read the MSR on a core in the 2nd socket without runni […]

I found mp_run_on_aps() can run on the assigned CPU core only, after enabling config PARALLEL_MP_AP_WORK,
mp_run_on_aps(function, NULL, x , 100 * USECS_PER_MSEC)
can run function() on logical CPU #x only, so it should be more efficient and simpler.
With CPU number being get_platform_thread_count() - 1, this core should always be in the last socket? I tried this method works.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp: Read PPIN MSR and save to an array for each CPU
......................................................................

Patch Set 8:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
File src/soc/intel/xeon_sp/skx/cpu.c:

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
PS5, Line 75: 	/* If socket_index is 0 then all PPIN have been saved. */
> I found mp_run_on_aps() can run on the assigned CPU core only, after enabling config PARALLEL_MP_AP_ […]

Makes sense, calling mp_run_on_aps() is more efficient. How are the threads numbered in coreboot? There are several different ways of numbering (supposing two socket, 26 cores/socket, 2 threads/core):
a. S0C0T0, S0C0T1, S0C1T0, ... , S1C26T0, S1C26T1.
b. S0C0T0, S0C1T0, S0C2T0, ... , S1C25T1, S1C26T1.
c. S0C0T0, S1C0T0, S0C1T0, ... , S0C26T1, S1C26T1.
In any case, the last one is a thread on socket 1. How this is numbered is also important in ACPI xSDT table. ACPI spec recommends option C.

Change in coreboot[master]: soc/xeon_sp/skx: Define PPIN MSR

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Andrey Petrov, Jonathan Zhang, David Hendricks, Jingle Hsu, Morgan Jang, Patrick Rudolph,

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

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

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

Change subject: soc/xeon_sp/skx: Define PPIN MSR
......................................................................

soc/xeon_sp/skx: Define PPIN MSR

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I5e1de8bcb651fb8ae8b106db1978235b0dd84c47
---
M src/soc/intel/xeon_sp/skx/include/soc/msr.h
1 file changed, 2 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/40523/9

Change in coreboot[master]: soc/xeon_sp/skx: Define PPIN MSR

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define PPIN MSR
......................................................................

Patch Set 9:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
File src/soc/intel/xeon_sp/skx/cpu.c:

https://review.coreboot.org/c/coreboot/+/40523/5/src/soc/intel/xeon_sp/skx/c... 
PS5, Line 75: 	/* If socket_index is 0 then all PPIN have been saved. */
> Makes sense, calling mp_run_on_aps() is more efficient. […]

Thanks for the info, I move the PPIN implementation to CB:40308

Change in coreboot[master]: soc/xeon_sp/skx: Define PPIN MSR

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define PPIN MSR
......................................................................

Patch Set 10: Code-Review+1

Change in coreboot[master]: soc/xeon_sp/skx: Define PPIN MSR

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define PPIN MSR
......................................................................

Patch Set 11: Code-Review+1

Change in coreboot[master]: soc/xeon_sp/skx: Define PPIN MSR

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define PPIN MSR
......................................................................

Patch Set 11: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40523/11//COMMIT_MSG@9 
PS11, Line 9: Tested on OCP Tioga Pass.
tested here: #40308 ?

Change in coreboot[master]: soc/xeon_sp/skx: Define PPIN MSR

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define PPIN MSR
......................................................................

Patch Set 11:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40523/11//COMMIT_MSG@9 
PS11, Line 9: Tested on OCP Tioga Pass.
> tested here: #40308 ?

Yes.

Change in coreboot[master]: soc/xeon_sp/skx: Define PPIN MSR

[Maxim Polyakov (Code Review)]

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define PPIN MSR
......................................................................

Patch Set 11: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40523/11//COMMIT_MSG@10 
PS11, Line 10: 
Please add the document number
(that contains information about this register)
For example:

These changes are in accordance with the documentation:
[*] page 2-297,
    Intel(R) 64 and IA-32 Architectures, Developer’s Manual,
    Volume 4: Model-Specific Registers. May 2019.
    Order Number: 335592-070US

Change in coreboot[master]: soc/xeon_sp/skx: Define MSR PPIN related registers

[Johnny Lin (Code Review)]

Hello build bot (Jenkins), Andrey Petrov, Frans Hendriks, Maxim Polyakov, Jonathan Zhang, David Hendricks, Jingle Hsu, Morgan Jang, Patrick Rudolph, HAOUAS Elyes,

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

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

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

Change subject: soc/xeon_sp/skx: Define MSR PPIN related registers
......................................................................

soc/xeon_sp/skx: Define MSR PPIN related registers

These changes are in accordance with the documentation:
[*] page 208-209
    Intel(R) 64 and IA-32 Architectures, Software Developer’s Manual,
    Volume 4: Model-Specific Registers. May 2019.
    Order Number: 335592-070US

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I5e1de8bcb651fb8ae8b106db1978235b0dd84c47
---
M src/soc/intel/xeon_sp/skx/include/soc/msr.h
1 file changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/40523/12

Change in coreboot[master]: soc/xeon_sp/skx: Define MSR PPIN related registers

[Johnny Lin (Code Review)]

Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define MSR PPIN related registers
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/40523/11//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/40523/11//COMMIT_MSG@10 
PS11, Line 10:
> Please add the document number […]

Done

Change in coreboot[master]: soc/xeon_sp/skx: Define MSR PPIN related registers

[Maxim Polyakov (Code Review)]

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define MSR PPIN related registers
......................................................................

Patch Set 13: Code-Review+2

Change in coreboot[master]: soc/xeon_sp/skx: Define MSR PPIN related registers

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40523 )

Change subject: soc/xeon_sp/skx: Define MSR PPIN related registers
......................................................................

Patch Set 16: Code-Review+1

Change in coreboot[master]: soc/xeon_sp/skx: Define MSR PPIN related registers

[Patrick Georgi (Code Review)]

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

Change subject: soc/xeon_sp/skx: Define MSR PPIN related registers
......................................................................

soc/xeon_sp/skx: Define MSR PPIN related registers

These changes are in accordance with the documentation:
[*] page 208-209
    Intel(R) 64 and IA-32 Architectures, Software Developer’s Manual,
    Volume 4: Model-Specific Registers. May 2019.
    Order Number: 335592-070US

Tested on OCP Tioga Pass.

Signed-off-by: Johnny Lin 
Change-Id: I5e1de8bcb651fb8ae8b106db1978235b0dd84c47
Reviewed-on: https://review.coreboot.org/c/coreboot/+/40523
Reviewed-by: Frans Hendriks 
Reviewed-by: Maxim Polyakov 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/xeon_sp/skx/include/soc/msr.h
1 file changed, 9 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Frans Hendriks: Looks good to me, but someone else must approve
  Maxim Polyakov: Looks good to me, approved

diff --git a/src/soc/intel/xeon_sp/skx/include/soc/msr.h b/src/soc/intel/xeon_sp/skx/include/soc/msr.h
index 2b1597b..dd05adc 100644
--- a/src/soc/intel/xeon_sp/skx/include/soc/msr.h
+++ b/src/soc/intel/xeon_sp/skx/include/soc/msr.h
@@ -96,4 +96,13 @@
 #define EPB_ENERGY_POLICY_SHIFT      3
 #define EPB_ENERGY_POLICY_MASK       (0xf << EPB_ENERGY_POLICY_SHIFT)

+/* MSR Protected Processor Inventory Number */
+#define MSR_PPIN_CTL		     0x04e
+#define MSR_PPIN_CTL_LOCK	     0x1
+#define MSR_PPIN_CTL_ENABLE_SHIFT    1
+#define MSR_PPIN_CTL_ENABLE	     (0x1 << MSR_PPIN_CTL_ENABLE_SHIFT)
+#define MSR_PPIN		     0x04f
+#define MSR_PPIN_CAP_SHIFT	     23
+#define MSR_PPIN_CAP		     (0x1 << MSR_PPIN_CAP_SHIFT)
+
 #endif /* _SOC_MSR_H_ */