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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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