[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 1/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL
On Wed, Mar 09, 2022 at 04:40:24PM +0100, Jan Beulich wrote: > On 09.03.2022 16:03, Roger Pau Monné wrote: > > On Mon, Feb 14, 2022 at 04:07:09PM +0100, Jan Beulich wrote: > >> 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. > > > > Maybe I didn't express this correctly. What I meant to explain is that > > on hardware having SPEC_CTRL VIRT_SPEC_CTRL will not be offered by > > default to guests. VIRT_SPEC_CTRL will only be part of the max CPUID > > policy so it can be enabled for compatibility purposes. Does this make > > sense? > > Yes. Can you re-word along these lines? Sure. > >>> --- 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? > > > > Well, that's not exactly true. Xen does use VIRT_SPEC_CTRL (see > > amd_init_ssbd). > > > > I'm unsure how to express support for VIRT_SPEC_CTRL, as it does rely > > on SPEC_CTRL when available. > > I wonder whether the command line doc needs to go into this level of > detail. Right, so you would be fine with leaving the command line option description alone. > >>> --- 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? > > > > No, we shouldn't expose to Intel. > > > >> 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)? > > > > We need AMD_SSBD if we implement VIRT_SPEC_CTRL on top of SPEC_CTRL, > > but we could also implement it on top of VIRT_SPEC_CTRL (if Xen runs > > virtualized) or even using the legacy SSBD setting mechanisms found in > > amd_init_ssbd, so I don't think VIRT_SSBD should explicitly depend on > > AMD_SSBD in gen-cpuid.py. > > Hmm, yes, good point. But when the prereqs cannot be expressed in > gen-cpuid.py, I guess they need to be encoded here. Yes, I've added a dependency on AMD_SSBD here, which was missing. > >>> --- 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. > > > > Well, the guest sets the bit in VIRT_SPEC_CTRL and Xen sets it on > > SPEC_CTRL in order for it to have effect. I can use 'alternative > > value' if that's clearer. > > Well, as I tried to express in my earlier reply, I view "shadow value" > to mean "alternative value", so replacing wouldn't help. The question > whether it acts like the shadow values we know elsewhere (VMX'es CR0 > and CR4, for example). If it does, using the same term is of course > fine. But it didn't look to me as if it would, hence I'd prefer to > avoid ambiguity. But please realize that I may have misunderstood > things ... No, you are OK to ask. When developing the series I went back and forth myself deciding whether 'hijacking' the spec_ctrl field to implement VIRT_SPEC_CTRL was OK. If host has AMD_SSBD: VIRT_SPEC_CTRL.SSBD will use the SPEC_CTRL.SSBD bit in the spec_ctrl field, but it will be set behind the guests back. If guests sets VIRT_SPEC_CTRL.SSBD but not SPEC_CTRL.SSBD, reads of SPEC_CTRL.SSBD from guest context will return 0, but the bit will be set. I called it 'shadow' because the underlying SPEC_CTRL.SSBD bit will get set, but reading SPEC_CTRL.SSBD could return 0 if the bit has been set from VIRT_SPEC_CTRL. Do you think that's a suitable use of 'shadow'? > >>> --- 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. > > > > I've used the ! to note that VIRT_SSBD might be exposed on hardware > > whether it's not available as part of the host featureset. It did seem > > to me that using just 's' didn't reflect this properly. > > I wouldn't have assigned such meaning to !. In fact if we emulated > a feature completely, I think it could legitimately show up here > without !. But then again I may also not fully be aware of all of > Andrew's intentions ... Not sure either. I've assumed '!' to mean that such feature could appear on guest policies even when not present on the host one, but I might be wrong. I'm happy to use a different annotation here. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |