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
