Re: [PATCH 3/6] x86/cpu-policy: Infrastructure for MSR_ARCH_CAPS

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 3/6] x86/cpu-policy: Infrastructure for MSR_ARCH_CAPS
Date: Tue, 16 May 2023 14:02:06 +0200	[thread overview]
Message-ID: <347219e4-6c3a-a0ad-b010-4dbd7282c7ad@suse.com> (raw)
In-Reply-To: <20230515144259.1009245-4-andrew.cooper3@citrix.com>

On 15.05.2023 16:42, Andrew Cooper wrote:
> Bits through 24 are already defined, meaning that we're not far off needing
> the second word.  Put both in right away.
> 
> The bool bitfield names in the arch_caps union are unused, and somewhat out of
> date.  They'll shortly be automatically generated.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'm largely okay, but I'd like to raise a couple of naming / presentation
questions:

> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -226,6 +226,14 @@ static const char *const str_7d2[32] =
>      [ 4] = "bhi-ctrl",      [ 5] = "mcdt-no",
>  };
>  
> +static const char *const str_10Al[32] =
> +{
> +};
> +
> +static const char *const str_10Ah[32] =
> +{
> +};
> +
>  static const struct {
>      const char *name;
>      const char *abbr;
> @@ -248,6 +256,8 @@ static const struct {
>      { "0x00000007:2.edx", "7d2", str_7d2 },
>      { "0x00000007:1.ecx", "7c1", str_7c1 },
>      { "0x00000007:1.edx", "7d1", str_7d1 },
> +    { "0x0000010a.lo",   "10Al", str_10Al },
> +    { "0x0000010a.hi",   "10Ah", str_10Ah },

The MSR-ness can certainly be inferred from the .lo / .hi and l/h
suffixes of the strings, but I wonder whether having it e.g. like

    { "MSR0000010a.lo",   "m10Al", str_10Al },
    { "MSR0000010a.hi",   "m10Ah", str_10Ah },

or

    { "MSR[010a].lo",   "m10Al", str_10Al },
    { "MSR[010a].hi",   "m10Ah", str_10Ah },

or even

    { "ARCH_CAPS.lo",   "m10Al", str_10Al },
    { "ARCH_CAPS.hi",   "m10Ah", str_10Ah },

wouldn't make it more obvious. For the two str_*, see below.

> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -307,6 +307,10 @@ XEN_CPUFEATURE(AVX_VNNI_INT8,      15*32+ 4) /*A  AVX-VNNI-INT8 Instructions */
>  XEN_CPUFEATURE(AVX_NE_CONVERT,     15*32+ 5) /*A  AVX-NE-CONVERT Instructions */
>  XEN_CPUFEATURE(CET_SSS,            15*32+18) /*   CET Supervisor Shadow Stacks safe to use */
>  
> +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.eax, word 16 */
> +
> +/* Intel-defined CPU features, MSR_ARCH_CAPS 0x10a.edx, word 17 */

Right here I'd be inclined to omit the MSR index; the name ought to
be sufficient.

> --- a/xen/include/xen/lib/x86/cpu-policy.h
> +++ b/xen/include/xen/lib/x86/cpu-policy.h
> @@ -20,6 +20,8 @@
>  #define FEATURESET_7d2   13 /* 0x00000007:2.edx    */
>  #define FEATURESET_7c1   14 /* 0x00000007:1.ecx    */
>  #define FEATURESET_7d1   15 /* 0x00000007:1.edx    */
> +#define FEATURESET_10Al  16 /* 0x0000010a.eax      */
> +#define FEATURESET_10Ah  17 /* 0x0000010a.edx      */

Just like we use an "e" prefix for extended CPUID leaves, perhaps
use an "m" prefix for MSRs (then also affecting e.g. the str_*
above)?

Jan