Re: [PATCH 1/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL
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
> {
> /*
> * 0x0048 - 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
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
> >>> {
> >>> /*
> >>> * 0x0048 - 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
Re: [PATCH 1/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL
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
>>> {
>>> /*
>>> * 0x0048 - 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
Re: [PATCH 1/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL
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
> > {
> > /*
> > * 0x0048 - 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)
Re: [PATCH 1/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL
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
> {
> /*
> * 0x0048 - 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
[PATCH 1/3] amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL
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
Signed-off-by: Roger Pau Monné
---
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(-)
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
{
/*
* 0x0048 - 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
