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 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.