[PATCH] x86/Intel: use CPUID bit to determine PPIN availability

[PATCH] x86/Intel: use CPUID bit to determine PPIN availability

[Jan Beulich]

As of SDM revision 076 there is a CPUID bit for this functionality. Use
it to amend the existing model-based logic.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
In xen-cpuid.c I wasn't sure whether it's better to put the 7b1 table
next to the 7a1 one, or whether tables should continue to be placed by
feature set ABI identifier.

--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -156,7 +156,7 @@ static const char *const str_e8b[32] =
     [18] = "ibrs-fast",        [19] = "ibrs-same-mode",
 
     [20] = "no-lmsl",
-    /* [22] */                 [23] = "ppin",
+    /* [22] */                 [23] = "amd-ppin",
     [24] = "amd-ssbd",         [25] = "virt-ssbd",
     [26] = "ssb-no",
     [28] = "psfd",
@@ -188,6 +188,11 @@ static const char *const str_7a1[32] =
     [12] = "fsrcs",
 };
 
+static const char *const str_7b1[32] =
+{
+    [ 0] = "intel-ppin",
+};
+
 static const char *const str_e21a[32] =
 {
     [ 2] = "lfence+",
@@ -212,6 +217,7 @@ static const struct {
     { "0x00000007:0.edx", "7d0", str_7d0 },
     { "0x00000007:1.eax", "7a1", str_7a1 },
     { "0x80000021.eax",  "e21a", str_e21a },
+    { "0x00000007:1.ebx", "7b1", str_7b1 },
 };
 
 #define COL_ALIGN "18"
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -865,6 +865,13 @@ static void intel_init_ppin(const struct
     {
         uint64_t val;
 
+    default:
+        if ( !cpu_has(c, X86_FEATURE_INTEL_PPIN) )
+        {
+            ppin_msr = 0;
+            return;
+        }
+        fallthrough;
     case 0x3e: /* IvyBridge X */
     case 0x3f: /* Haswell X */
     case 0x4f: /* Broadwell X */
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -299,6 +299,9 @@ XEN_CPUFEATURE(FSRCS,        10*32+12) /
 XEN_CPUFEATURE(LFENCE_DISPATCH,    11*32+ 2) /*A  LFENCE always serializing */
 XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null Selector Clears Base (and limit too) */
 
+/* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */
+XEN_CPUFEATURE(INTEL_PPIN,         12*32+ 0) /*   Protected Processor Inventory Number */
+
 #endif /* XEN_CPUFEATURE */
 
 /* Clean up from a default include.  Close the enum (for C). */
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -16,6 +16,7 @@
 #define FEATURESET_7d0    9 /* 0x00000007:0.edx    */
 #define FEATURESET_7a1   10 /* 0x00000007:1.eax    */
 #define FEATURESET_e21a  11 /* 0x80000021.eax      */
+#define FEATURESET_7b1   12 /* 0x00000007:1.ebx    */
 
 struct cpuid_leaf
 {
@@ -188,6 +189,10 @@ struct cpuid_policy
                 uint32_t _7a1;
                 struct { DECL_BITFIELD(7a1); };
             };
+            union {
+                uint32_t _7b1;
+                struct { DECL_BITFIELD(7b1); };
+            };
         };
     } feat;
 

Re: [PATCH] x86/Intel: use CPUID bit to determine PPIN availability

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]

On 17/01/2022 15:30, Jan Beulich wrote:
> As of SDM revision 076 there is a CPUID bit for this functionality. Use
> it to amend the existing model-based logic.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> In xen-cpuid.c I wasn't sure whether it's better to put the 7b1 table
> next to the 7a1 one, or whether tables should continue to be placed by
> feature set ABI identifier.

They're in FEATURESET_* order, which is also chronological order. 
str_7b1 wants to be after str_e21a.

> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -156,7 +156,7 @@ static const char *const str_e8b[32] =
>      [18] = "ibrs-fast",        [19] = "ibrs-same-mode",
>  
>      [20] = "no-lmsl",
> -    /* [22] */                 [23] = "ppin",
> +    /* [22] */                 [23] = "amd-ppin",

We don't retrofit names like this.  If we did, loads of the speculation
bits would need to change.

The Intel vs AMD split is clear from the leaf index, and the only reason
we have the distinction is to fit the two bits into the same namespace.

Please leave this as was, to match its name in the source code.

> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
> @@ -865,6 +865,13 @@ static void intel_init_ppin(const struct
>      {
>          uint64_t val;
>  
> +    default:

Considering the comment above this switch statement, which you haven't
edited at all, this wants a note saying that a CPUID bit was added in
Sapphire Rapids, but older CPUs still require model-specific enumeration.

~Andrew

[-- Attachment #2: Type: text/html, Size: 2397 bytes --]

Re: [PATCH] x86/Intel: use CPUID bit to determine PPIN availability

[Jan Beulich]

On 18.01.2022 13:18, Andrew Cooper wrote:
> On 17/01/2022 15:30, Jan Beulich wrote:
>> As of SDM revision 076 there is a CPUID bit for this functionality. Use
>> it to amend the existing model-based logic.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> In xen-cpuid.c I wasn't sure whether it's better to put the 7b1 table
>> next to the 7a1 one, or whether tables should continue to be placed by
>> feature set ABI identifier.
> 
> They're in FEATURESET_* order, which is also chronological order. 
> str_7b1 wants to be after str_e21a.

I can see the ordering being necessary for decodes[], but I have a
hard time seeing why there would be any strict need for a particular
ordering model for the str_...[] objects; there it would seem
slightly more natural to me to have adjacent registers / leaves
next to each other. But since I don't care that much, I'll switch.

>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -156,7 +156,7 @@ static const char *const str_e8b[32] =
>>      [18] = "ibrs-fast",        [19] = "ibrs-same-mode",
>>  
>>      [20] = "no-lmsl",
>> -    /* [22] */                 [23] = "ppin",
>> +    /* [22] */                 [23] = "amd-ppin",
> 
> We don't retrofit names like this.  If we did, loads of the speculation
> bits would need to change.
> 
> The Intel vs AMD split is clear from the leaf index, and the only reason
> we have the distinction is to fit the two bits into the same namespace.

In which case are you also asking to make the new leave simply say
"ppin"?

> Please leave this as was, to match its name in the source code.

By match you mean what? Hardly

XEN_CPUFEATURE(AMD_PPIN,      8*32+23) /*   Protected Processor Inventory Number */

?

>> --- a/xen/arch/x86/cpu/mcheck/mce_intel.c
>> +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
>> @@ -865,6 +865,13 @@ static void intel_init_ppin(const struct
>>      {
>>          uint64_t val;
>>  
>> +    default:
> 
> Considering the comment above this switch statement, which you haven't
> edited at all, this wants a note saying that a CPUID bit was added in
> Sapphire Rapids, but older CPUs still require model-specific enumeration.

Oh, yes, I should have paid attention to that comment. Will edit.

Jan

Re: [PATCH] x86/Intel: use CPUID bit to determine PPIN availability

[Andrew Cooper]

On 17/01/2022 15:30, Jan Beulich wrote:
> As of SDM revision 076 there is a CPUID bit for this functionality. Use
> it to amend the existing model-based logic.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

https://lore.kernel.org/lkml/20220107225442.1690165-1-tony.luck@intel.com/T/#u
suggests that Sapphire Rapids also needs the model specific treatment.

I agree with the "only-expose-on-error" observation, so perhaps we ought
to make these details available to the hardware domain in a suitable form.

~Andrew

Re: [PATCH] x86/Intel: use CPUID bit to determine PPIN availability

[Jan Beulich]

On 18.01.2022 21:28, Andrew Cooper wrote:
> On 17/01/2022 15:30, Jan Beulich wrote:
>> As of SDM revision 076 there is a CPUID bit for this functionality. Use
>> it to amend the existing model-based logic.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> https://lore.kernel.org/lkml/20220107225442.1690165-1-tony.luck@intel.com/T/#u
> suggests that Sapphire Rapids also needs the model specific treatment.

Well, I can go and pull in their a331f5fdd36 just to be on the safe side.
I have to admit that it's not clear to me whether that older commit is
made obsolete by the CPUID bit check about to be added.

> I agree with the "only-expose-on-error" observation, so perhaps we ought
> to make these details available to the hardware domain in a suitable form.

hypfs?

Jan