[PATCH v2 0/2] x86/Intel: PPIN recognition adjustments

[PATCH v2 0/2] x86/Intel: PPIN recognition adjustments

[Jan Beulich]

1: Sapphire Rapids Xeons also support PPIN
2: use CPUID bit to determine PPIN availability

Jan

[PATCH v2 1/2] x86/Intel: Sapphire Rapids Xeons also support PPIN

[Jan Beulich]

This is as per Linux commit a331f5fdd36d ("x86/mce: Add Xeon Sapphire
Rapids to list of CPUs that support PPIN") just in case a subsequent
change making use of the respective new CPUID bit doesn't cover this
model.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
It is unclear to me whether this change is actually made obsolete by the
subsequent one adding support for the respective new CPUID bit.

It also continues to be unclear for which CPU models, if any, the
PPIN_CAP bit in PLATFORM_INFO could be used in favor of a model check.
---
v2: New.

--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -873,6 +873,7 @@ static void intel_init_ppin(const struct
     case 0x57: /* Knights Landing */
     case 0x6a: /* Icelake X */
     case 0x85: /* Knights Mill */
+    case 0x8f: /* Sapphire Rapids X */
 
         if ( (c != &boot_cpu_data && !ppin_msr) ||
              rdmsr_safe(MSR_PPIN_CTL, val) )

[PATCH v2 2/2] 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>
---
It continues to be unclear for which CPU models, if any, the PPIN_CAP
bit in PLATFORM_INFO could be used in favor of a model check.
---
v2: Don't rename AMD's identifier in xen-cpuid.c. Name Intel's just
    "ppin" as well. Move str_7b1[]. Update a comment.

--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -195,6 +195,11 @@ static const char *const str_e21a[32] =
     [ 6] = "nscb",
 };
 
+static const char *const str_7b1[32] =
+{
+    [ 0] = "ppin",
+};
+
 static const struct {
     const char *name;
     const char *abbr;
@@ -213,6 +218,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
@@ -859,12 +859,20 @@ static void intel_init_ppin(const struct
     /*
      * Even if testing the presence of the MSR would be enough, we don't
      * want to risk the situation where other models reuse this MSR for
-     * other purposes.
+     * other purposes.  Despite the late addition of a CPUID bit (rendering
+     * the MSR architectural), keep using the same detection logic there.
      */
     switch ( c->x86_model )
     {
         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 v2 1/2] x86/Intel: Sapphire Rapids Xeons also support PPIN

[Andrew Cooper]

On 20/01/2022 14:16, Jan Beulich wrote:
> This is as per Linux commit a331f5fdd36d ("x86/mce: Add Xeon Sapphire
> Rapids to list of CPUs that support PPIN") just in case a subsequent
> change making use of the respective new CPUID bit doesn't cover this
> model.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Sadly,
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=e464121f2d40eabc7d11823fb26db807ce945df4


IceLake-D too.

Preferably with this fixed, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com> (to save a trivial repost), but ...

> ---
> It is unclear to me whether this change is actually made obsolete by the
> subsequent one adding support for the respective new CPUID bit.

... Sapphire Rapids doesn't enumerate PPIN.  Hopefully Granite Rapids
will, but everything SPR and older will have to rely on model checks only.

Probably best to drop the second half of the commit message to remove
the uncertainty.

> It also continues to be unclear for which CPU models, if any, the
> PPIN_CAP bit in PLATFORM_INFO could be used in favor of a model check.

Presumably none, because you need the same set of model checks to
interpret the PPIN bit in PLATFORM_INFO.  It does beg the question what
the point of the bit is...

~Andrew

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

[Andrew Cooper]

On 20/01/2022 14:17, Jan Beulich wrote:
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -195,6 +195,11 @@ static const char *const str_e21a[32] =
>      [ 6] = "nscb",
>  };
>  
> +static const char *const str_7b1[32] =
> +{
> +    [ 0] = "ppin",
> +};

I hadn't realised what a mess we had with the prefixes.

We have AMD_PPIN rendered as simply "ppin", while we also have
AMD_{STIBP,SSBD} which are rendered with an amd- prefix.  This patch is
the first introduction of an INTEL_ prefixed feature.

We should figure out a consistency rule and fix the logic, before adding
more confusion.

Given the AMD MSR_SPEC_CTRL series just posted, use of CPUID bits will
often be symmetrical and it's awkward to have one or with a prefix and
the other without.

> --- 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;
>  
>

Looking at a related patch I've got, at a minimum, you also need:
* collect the leaf in generic_identify()
* extend cpuid_policy_to_featureset() and cpuid_featureset_to_policy()

However I've got an idea to help us split "add new leaf" from "first bit
in new leaf" which I'd like to experiment with first.  It is rather
awkward having the two mostly-unrelated changes forced together in a
single patch.

~Andrew

Re: [PATCH v2 1/2] x86/Intel: Sapphire Rapids Xeons also support PPIN

[Jan Beulich]

On 27.01.2022 00:01, Andrew Cooper wrote:
> On 20/01/2022 14:16, Jan Beulich wrote:
>> This is as per Linux commit a331f5fdd36d ("x86/mce: Add Xeon Sapphire
>> Rapids to list of CPUs that support PPIN") just in case a subsequent
>> change making use of the respective new CPUID bit doesn't cover this
>> model.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Sadly,
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/urgent&id=e464121f2d40eabc7d11823fb26db807ce945df4
> 
> 
> IceLake-D too.
> 
> Preferably with this fixed, Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com> (to save a trivial repost),

Sure, added. And thanks.

> but ...
> 
>> ---
>> It is unclear to me whether this change is actually made obsolete by the
>> subsequent one adding support for the respective new CPUID bit.
> 
> ... Sapphire Rapids doesn't enumerate PPIN.  Hopefully Granite Rapids
> will, but everything SPR and older will have to rely on model checks only.

At least in theory I suppose they could address this by as simple as
a microcode update?

>> It also continues to be unclear for which CPU models, if any, the
>> PPIN_CAP bit in PLATFORM_INFO could be used in favor of a model check.
> 
> Presumably none, because you need the same set of model checks to
> interpret the PPIN bit in PLATFORM_INFO.  It does beg the question what
> the point of the bit is...

Well, if the bit never had a different meaning, then a model check
wouldn't be necessary. Just like e.g. probe_cpuid_faulting() doesn't
have one.

Jan

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

[Jan Beulich]

On 27.01.2022 00:30, Andrew Cooper wrote:
> On 20/01/2022 14:17, Jan Beulich wrote:
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -195,6 +195,11 @@ static const char *const str_e21a[32] =
>>      [ 6] = "nscb",
>>  };
>>  
>> +static const char *const str_7b1[32] =
>> +{
>> +    [ 0] = "ppin",
>> +};
> 
> I hadn't realised what a mess we had with the prefixes.
> 
> We have AMD_PPIN rendered as simply "ppin", while we also have
> AMD_{STIBP,SSBD} which are rendered with an amd- prefix.  This patch is
> the first introduction of an INTEL_ prefixed feature.
> 
> We should figure out a consistency rule and fix the logic, before adding
> more confusion.
> 
> Given the AMD MSR_SPEC_CTRL series just posted, use of CPUID bits will
> often be symmetrical and it's awkward to have one or with a prefix and
> the other without.

IOW you suggest I add a kind-of-prereq patch to drop the prefixes?

>> --- 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;
>>  
>>
> 
> Looking at a related patch I've got, at a minimum, you also need:
> * collect the leaf in generic_identify()

I'll need to make a patch first to collect 7a1, as it seems. It was
actually 7a1 that I used as a reference, iirc.

> * extend cpuid_policy_to_featureset() and cpuid_featureset_to_policy()

Yeah, I missed those. Presumably by looking for instances only under
arch/x86/. Especially with per-arch include/ now living under
arch/<arch>/, having separate x86 bits elsewhere is a little unhelpful.

> However I've got an idea to help us split "add new leaf" from "first bit
> in new leaf" which I'd like to experiment with first.  It is rather
> awkward having the two mostly-unrelated changes forced together in a
> single patch.

I'll make the necessary adjustments here anyway. I can always re-base
on top of what you may come up with.

Jan

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

[Jan Beulich]

On 27.01.2022 08:56, Jan Beulich wrote:
> On 27.01.2022 00:30, Andrew Cooper wrote:
>> On 20/01/2022 14:17, Jan Beulich wrote:
>>> @@ -188,6 +189,10 @@ struct cpuid_policy
>>>                  uint32_t _7a1;
>>>                  struct { DECL_BITFIELD(7a1); };
>>>              };
>>> +            union {
>>> +                uint32_t _7b1;
>>> +                struct { DECL_BITFIELD(7b1); };
>>> +            };
>>>          };
>>>      } feat;
>>>  
>>>
>>
>> Looking at a related patch I've got, at a minimum, you also need:
>> * collect the leaf in generic_identify()
> 
> I'll need to make a patch first to collect 7a1, as it seems. It was
> actually 7a1 that I used as a reference, iirc.

Actually that's there, just that I didn't spot it when looking for
the "(7, " pattern in cpu/common.c. This form is used only in
early_cpu_init(), while generic_identify() uses
cpuid_count(0x00000007, ...). All quite inconsistent ...

Jan

[PATCH v3] 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>
---
It continues to be unclear for which CPU models, if any, the PPIN_CAP
bit in PLATFORM_INFO could be used in favor of a model check.
---
v3: Actually record CPUID[7:1].EBX. Adjust policy <-> feature conversion
    functions.
v2: Don't rename AMD's identifier in xen-cpuid.c. Name Intel's just
    "ppin" as well. Move str_7b1[]. Update a comment.

--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -195,6 +195,11 @@ static const char *const str_e21a[32] =
     [ 6] = "nscb",
 };
 
+static const char *const str_7b1[32] =
+{
+    [ 0] = "ppin",
+};
+
 static const struct {
     const char *name;
     const char *abbr;
@@ -213,6 +218,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/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -445,7 +445,8 @@ static void generic_identify(struct cpui
 		if (eax > 0)
 			cpuid_count(0x00000007, 1,
 				    &c->x86_capability[cpufeat_word(X86_FEATURE_AVX512_BF16)],
-				    &tmp, &tmp, &tmp);
+				    &c->x86_capability[cpufeat_word(X86_FEATURE_INTEL_PPIN)],
+				    &tmp, &tmp);
 	}
 
 	if (c->cpuid_level >= 0xd)
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -859,12 +859,20 @@ static void intel_init_ppin(const struct
     /*
      * Even if testing the presence of the MSR would be enough, we don't
      * want to risk the situation where other models reuse this MSR for
-     * other purposes.
+     * other purposes.  Despite the late addition of a CPUID bit (rendering
+     * the MSR architectural), keep using the same detection logic there.
      */
     switch ( c->x86_model )
     {
         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;
 
@@ -327,6 +332,7 @@ static inline void cpuid_policy_to_featu
     fs[FEATURESET_7d0] = p->feat._7d0;
     fs[FEATURESET_7a1] = p->feat._7a1;
     fs[FEATURESET_e21a] = p->extd.e21a;
+    fs[FEATURESET_7b1] = p->feat._7b1;
 }
 
 /* Fill in a CPUID policy from a featureset bitmap. */
@@ -345,6 +351,7 @@ static inline void cpuid_featureset_to_p
     p->feat._7d0  = fs[FEATURESET_7d0];
     p->feat._7a1  = fs[FEATURESET_7a1];
     p->extd.e21a  = fs[FEATURESET_e21a];
+    p->feat._7b1  = fs[FEATURESET_7b1];
 }
 
 static inline uint64_t cpuid_policy_xcr0_max(const struct cpuid_policy *p)