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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.