Re: [Xen-devel] [PATCH] x86: log XPTI enabled status

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/02/18 08:52, Jan Beulich wrote:
> At the same time also report the state of the two defined
> ARCH_CAPABILITIES MSR bits (but don't expose the MSR to guests yet).
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Should we disable XPTI right here when we find RDCL_NO?

I was planning to wait for a working piece of hardware before trying to
implement ARCH_CAPS, given how often the spec changed.  Then again, at
the point Linux is doing this, we might as well consider it safe.

>
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -21,7 +21,7 @@
>  #include <xen/lib.h>
>  
>  #include <asm/microcode.h>
> -#include <asm/msr-index.h>
> +#include <asm/msr.h>
>  #include <asm/processor.h>
>  #include <asm/spec_ctrl.h>
>  #include <asm/spec_ctrl_asm.h>
> @@ -84,12 +84,15 @@ custom_param("bti", parse_bti);
>  static void __init print_details(enum ind_thunk thunk)
>  {
>      unsigned int _7d0 = 0, e8b = 0, tmp;
> +    uint64_t caps = 0;
>  
>      /* Collect diagnostics about available mitigations. */
>      if ( boot_cpu_data.cpuid_level >= 7 )
>          cpuid_count(7, 0, &tmp, &tmp, &tmp, &_7d0);
>      if ( boot_cpu_data.extended_cpuid_level >= 0x80000008 )
>          cpuid(0x80000008, &tmp, &e8b, &tmp, &tmp);
> +    if ( _7d0 & cpufeat_mask(X86_FEATURE_ARCH_CAPS) )
> +        rdmsrl(MSR_ARCH_CAPABILITIES, caps);

It was going to ask if I could talk you into using the raw msr policy,
but it appears that those patches haven't landed yet.  I have another
patch pending to drop these CPUIDs and use the raw cpuid policy.

>  
>      printk(XENLOG_DEBUG "Speculative mitigation facilities:\n");
>  
> @@ -97,10 +100,12 @@ static void __init print_details(enum in
>      if ( (_7d0 & (cpufeat_mask(X86_FEATURE_IBRSB) |
>                    cpufeat_mask(X86_FEATURE_STIBP))) ||
>           (e8b & cpufeat_mask(X86_FEATURE_IBPB)) )

We either need to update the caps check in this conditional, or drop it
entirely.  It there to avoid printing the line when it would be
otherwise empty, but given that it's now at debug level anyway, it's
perhaps not worth the effort of trying to skip.

> -        printk(XENLOG_DEBUG "  Hardware features:%s%s%s\n",
> +        printk(XENLOG_DEBUG "  Hardware features:%s%s%s%s%s\n",
>                 (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
>                 (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP"     : "",
> -               (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "");
> +               (e8b  & cpufeat_mask(X86_FEATURE_IBPB))  ? " IBPB"      : "",
> +               (caps & MSR_ARCH_CAPABILITIES_IBRS_ALL)  ? " IBRS_ALL"  : "",
> +               (caps & MSR_ARCH_CAPABILITIES_RDCL_NO)   ? " RDCL_NO"   : "");
>  
>      /* Compiled-in support which pertains to BTI mitigations. */
>      if ( IS_ENABLED(CONFIG_INDIRECT_THUNK) )
> @@ -117,6 +122,9 @@ static void __init print_details(enum in
>             opt_ibpb                                  ? " IBPB"       : "",
>             boot_cpu_has(X86_FEATURE_RSB_NATIVE)      ? " RSB_NATIVE" : "",
>             boot_cpu_has(X86_FEATURE_RSB_VMEXIT)      ? " RSB_VMEXIT" : "");
> +
> +    printk(XENLOG_INFO "XPTI: %s\n",
> +           boot_cpu_has(X86_FEATURE_NO_XPTI) ? "disabled" : "enabled");
>  }
>  
>  /* Calculate whether Retpoline is known-safe on this CPU. */
> --- a/xen/include/asm-x86/msr-index.h
> +++ b/xen/include/asm-x86/msr-index.h
> @@ -40,6 +40,8 @@
>  #define PRED_CMD_IBPB                        (_AC(1, ULL) << 0)
>  
>  #define MSR_ARCH_CAPABILITIES                0x0000010a
> +#define MSR_ARCH_CAPABILITIES_RDCL_NO   (_AC(1, ULL) << 0)
> +#define MSR_ARCH_CAPABILITIES_IBRS_ALL  (_AC(1, ULL) << 1)

Can we drop the MSR_ for the bits?  It doesn't reduce code clarity when
used, and shortens the constants a little.

>  
>  /* Intel MSRs. Some also available on other CPUs */
>  #define MSR_IA32_PERFCTR0            0x000000c1
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -244,6 +244,7 @@ XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /
>  XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A  AVX512 Multiply Accumulation 
> Single Precision */
>  XEN_CPUFEATURE(IBRSB,         9*32+26) /*A  IBRS and IBPB support (used by 
> Intel) */
>  XEN_CPUFEATURE(STIBP,         9*32+27) /*A! STIBP */
> +XEN_CPUFEATURE(ARCH_CAPS,     9*32+29) /*   IA32_ARCH_CAPABILITIES MSR */

This doesn't appear to be used?

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel