Re: [PATCH 1/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.02.2022 17:46, Roger Pau Monne wrote:
> Use the logic to set shadow SPEC_CTRL values in order to implement
> support for VIRT_SPEC_CTRL (signaled by VIRT_SSBD CPUID flag) for HVM
> guests. This includes using the spec_ctrl vCPU MSR variable to store
> the guest set value of VIRT_SPEC_CTRL.SSBD.

This leverages the guest running on the OR of host and guest values,
aiui. If so, this could do with spelling out.

> Note that VIRT_SSBD is only set in the HVM max CPUID policy, as the
> default should be to expose SPEC_CTRL only and support VIRT_SPEC_CTRL
> for migration compatibility.

I'm afraid I don't understand this last statement: How would this be
about migration compatibility? No guest so far can use VIRT_SPEC_CTRL,
and a future guest using it is unlikely to be able to cope with the
MSR "disappearing" during migration.

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2273,8 +2273,9 @@ to use.
>  * `pv=` and `hvm=` offer control over all suboptions for PV and HVM guests
>    respectively.
>  * `msr-sc=` offers control over Xen's support for manipulating 
> `MSR_SPEC_CTRL`
> -  on entry and exit.  These blocks are necessary to virtualise support for
> -  guests and if disabled, guests will be unable to use IBRS/STIBP/SSBD/etc.
> +  and/or `MSR_VIRT_SPEC_CTRL` on entry and exit.  These blocks are necessary 
> to

Why would Xen be manipulating an MSR it only brings into existence for its
guests?

> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -543,6 +543,13 @@ static void __init calculate_hvm_max_policy(void)
>          __clear_bit(X86_FEATURE_IBRSB, hvm_featureset);
>          __clear_bit(X86_FEATURE_IBRS, hvm_featureset);
>      }
> +    else
> +        /*
> +         * If SPEC_CTRL is available VIRT_SPEC_CTRL can also be implemented 
> as
> +         * it's a subset of the controls exposed in SPEC_CTRL (SSBD only).
> +         * Expose in the max policy for compatibility migration.
> +         */
> +        __set_bit(X86_FEATURE_VIRT_SSBD, hvm_featureset);

This means even Intel guests can use the feature then? I thought it was
meanwhile deemed bad to offer such cross-vendor features?

Additionally, is SPEC_CTRL (i.e. IBRS) availability enough? Don't you
need AMD_SSBD as a prereq (which may want expressing in gen-cpuid.py)?

> --- a/xen/arch/x86/include/asm/msr.h
> +++ b/xen/arch/x86/include/asm/msr.h
> @@ -291,6 +291,7 @@ struct vcpu_msrs
>  {
>      /*
>       * 0x00000048 - MSR_SPEC_CTRL
> +     * 0xc001011f - MSR_VIRT_SPEC_CTRL
>       *
>       * For PV guests, this holds the guest kernel value.  It is accessed on
>       * every entry/exit path.
> @@ -301,7 +302,10 @@ struct vcpu_msrs
>       * For SVM, the guest value lives in the VMCB, and hardware 
> saves/restores
>       * the host value automatically.  However, guests run with the OR of the
>       * host and guest value, which allows Xen to set protections behind the
> -     * guest's back.
> +     * guest's back.  Use such functionality in order to implement support 
> for
> +     * VIRT_SPEC_CTRL as a shadow value of SPEC_CTRL and thus store the value
> +     * of VIRT_SPEC_CTRL in this field, taking advantage of both MSRs having
> +     * compatible layouts.

I guess "shadow value" means more like an alternative value, but
(see above) this is about setting for now just one bit behind the
guest's back.

> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -395,12 +395,13 @@ static void __init print_details(enum ind_thunk thunk, 
> uint64_t caps)
>       * mitigation support for guests.
>       */
>  #ifdef CONFIG_HVM
> -    printk("  Support for HVM VMs:%s%s%s%s%s\n",
> +    printk("  Support for HVM VMs:%s%s%s%s%s%s\n",
>             (boot_cpu_has(X86_FEATURE_SC_MSR_HVM) ||
>              boot_cpu_has(X86_FEATURE_SC_RSB_HVM) ||
>              boot_cpu_has(X86_FEATURE_MD_CLEAR)   ||
>              opt_eager_fpu)                           ? ""               : " 
> None",
>             boot_cpu_has(X86_FEATURE_SC_MSR_HVM)      ? " MSR_SPEC_CTRL" : "",
> +           boot_cpu_has(X86_FEATURE_SC_MSR_HVM)      ? " MSR_VIRT_SPEC_CTRL" 
> : "",
>             boot_cpu_has(X86_FEATURE_SC_RSB_HVM)      ? " RSB"           : "",
>             opt_eager_fpu                             ? " EAGER_FPU"     : "",
>             boot_cpu_has(X86_FEATURE_MD_CLEAR)        ? " MD_CLEAR"      : 
> "");

The output getting longish, can the two SC_MSR_HVM dependent items
perhaps be folded, e.g. by making it "MSR_{,VIRT_}SPEC_CTRL"?

> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -265,7 +265,7 @@ XEN_CPUFEATURE(IBRS_SAME_MODE, 8*32+19) /*S  IBRS 
> provides same-mode protection
>  XEN_CPUFEATURE(NO_LMSL,       8*32+20) /*S  EFER.LMSLE no longer supported. 
> */
>  XEN_CPUFEATURE(AMD_PPIN,      8*32+23) /*   Protected Processor Inventory 
> Number */
>  XEN_CPUFEATURE(AMD_SSBD,      8*32+24) /*S  MSR_SPEC_CTRL.SSBD available */
> -XEN_CPUFEATURE(VIRT_SSBD,     8*32+25) /*   MSR_VIRT_SPEC_CTRL.SSBD */
> +XEN_CPUFEATURE(VIRT_SSBD,     8*32+25) /*!s MSR_VIRT_SPEC_CTRL.SSBD */

What is the ! intended to cover here? From guest perspective the
MSR acts entirely normally afaict.

Jan