| Message ID | 20220201164651.6369-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | amd/msr: implement MSR_VIRT_SPEC_CTRL for HVM guests | expand |
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
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? > > --- 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. > > --- 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. > > --- 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. > > --- 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"? OK, but further patches will add MSR_VIRT_SPEC_CTRL to hardware that doesn't expose MSR_SPEC_CTRL to guests, at which point it could be confusing? > > --- 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. According to my reading of the comment at the top '!' is not used to signal that the feature might act differently, but just that it's presence cannot be properly expressed with just the A, S, H flags, which would be the case for VIRT_SSBD I think. Thanks, Roger.
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? >>> --- 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. >>> --- 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. >>> --- 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 ... >>> --- 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"? > > OK, but further patches will add MSR_VIRT_SPEC_CTRL to hardware that > doesn't expose MSR_SPEC_CTRL to guests, at which point it could be > confusing? Yeah, I obviously hadn't seen adjustments done here by later patches. When I saw those, I think I understood why you things do this way. >>> --- 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 ... Jan > According to my reading of the comment at the top '!' is not used to > signal that the feature might act differently, but just that it's > presence cannot be properly expressed with just the A, S, H flags, > which would be the case for VIRT_SSBD I think. > > Thanks, Roger. >
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.
On 09.03.2022 17:31, Roger Pau Monné wrote: > 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: >>>>> --- 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. Yes. >>>>> --- 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'? Not sure, but since I don't have a good alternative suggestion, please keep using "shadow". Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 6b3da6ddc1..081e10f80b 100644 --- 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 + virtualise support for guests and if disabled, guests will be unable to use + IBRS/STIBP/SSBD/etc. * `rsb=` offers control over whether to overwrite the Return Stack Buffer / Return Address Stack on entry to Xen. * `md-clear=` offers control over whether to use VERW to flush diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c index e24dd283e7..29b4cfc9e6 100644 --- 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); /* * With VT-x, some features are only supported by Xen if dedicated diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index c4ddb8607d..3400c9299c 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1332,6 +1332,7 @@ static const uint32_t msrs_to_send[] = { MSR_INTEL_MISC_FEATURES_ENABLES, MSR_IA32_BNDCFGS, MSR_IA32_XSS, + MSR_VIRT_SPEC_CTRL, MSR_AMD64_DR0_ADDRESS_MASK, MSR_AMD64_DR1_ADDRESS_MASK, MSR_AMD64_DR2_ADDRESS_MASK, diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h index ce4fe51afe..98f6b79e09 100644 --- 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. * * We must clear/restore Xen's value before/after VMRUN to avoid unduly * influencing the guest. In order to support "behind the guest's back" diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index 4ac5b5a048..aa74cfde6c 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -381,6 +381,13 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val) ? K8_HWCR_TSC_FREQ_SEL : 0; break; + case MSR_VIRT_SPEC_CTRL: + if ( !cp->extd.virt_ssbd ) + goto gp_fault; + + *val = msrs->spec_ctrl.raw & SPEC_CTRL_SSBD; + break; + case MSR_AMD64_DE_CFG: if ( !(cp->x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) ) goto gp_fault; @@ -666,6 +673,14 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val) wrmsr_tsc_aux(val); break; + case MSR_VIRT_SPEC_CTRL: + if ( !cp->extd.virt_ssbd ) + goto gp_fault; + + /* Only supports SSBD bit, the rest are ignored. */ + msrs->spec_ctrl.raw = val & SPEC_CTRL_SSBD; + break; + case MSR_AMD64_DE_CFG: /* * OpenBSD 6.7 will panic if writing to DE_CFG triggers a #GP: diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index ee862089b7..64b154b2d3 100644 --- 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" : ""); diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h index 957df23b65..b9ab878ec1 100644 --- 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 */ XEN_CPUFEATURE(SSB_NO, 8*32+26) /*A Hardware not vulnerable to SSB */ XEN_CPUFEATURE(PSFD, 8*32+28) /*S MSR_SPEC_CTRL.PSFD */
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. 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. Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- docs/misc/xen-command-line.pandoc | 5 +++-- xen/arch/x86/cpuid.c | 7 +++++++ xen/arch/x86/hvm/hvm.c | 1 + xen/arch/x86/include/asm/msr.h | 6 +++++- xen/arch/x86/msr.c | 15 +++++++++++++++ xen/arch/x86/spec_ctrl.c | 3 ++- xen/include/public/arch-x86/cpufeatureset.h | 2 +- 7 files changed, 34 insertions(+), 5 deletions(-)