Re: [PATCH] x86/vmx: add support for virtualize SPEC_CTRL

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 08, 2024 at 02:40:53PM +0100, Jan Beulich wrote:
> On 06.02.2024 15:25, Roger Pau Monne wrote:
> > @@ -2086,6 +2091,9 @@ void vmcs_dump_vcpu(struct vcpu *v)
> >      if ( v->arch.hvm.vmx.secondary_exec_control &
> >           SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY )
> >          printk("InterruptStatus = %04x\n", vmr16(GUEST_INTR_STATUS));
> > +    if ( cpu_has_vmx_virt_spec_ctrl )
> > +        printk("SPEC_CTRL mask = %#016lx  shadow = %#016lx\n",
> > +               vmr(SPEC_CTRL_MASK), vmr(SPEC_CTRL_SHADOW));
> 
> #0... doesn't make a lot of sense; only e.g. %#lx does. Seeing context
> there's no 0x prefix there anyway. Having looked at the function the
> other day, I know though that there's a fair mix of 0x-prefixed and
> unprefixed hex numbers that are output.

For consistency with how other MSRs are printed I should use the '0x'
prefix.

> Personally I'd prefer if all
> 0x prefixes were omitted here. If you and Andrew think otherwise, I can
> live with that, so long as we're at least striving towards consistent
> output (I may be able to get to doing a conversion patch, once I know
> which way the conversion should be).

I usually prefer the '0x' to avoid ambiguity.  However this being all
hardware registers, I might be fine with dropping the '0x' on the
grounds that all registers are always printed as hex.

> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -823,18 +823,28 @@ static void cf_check vmx_cpuid_policy_changed(struct 
> > vcpu *v)
> >      {
> >          vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
> >  
> > -        rc = vmx_add_guest_msr(v, MSR_SPEC_CTRL, 0);
> > -        if ( rc )
> > -            goto out;
> > +        if ( !cpu_has_vmx_virt_spec_ctrl )
> > +        {
> > +            rc = vmx_add_guest_msr(v, MSR_SPEC_CTRL, 0);
> > +            if ( rc )
> > +                goto out;
> > +        }
> 
> I'm certainly okay with you doing it this way, but generally I'd prefer
> if code churn was limited whjere possible. Here leveraging that rc is 0
> on entry, a smaller change would be to
> 
>         if ( !cpu_has_vmx_virt_spec_ctrl )
>             rc = vmx_add_guest_msr(v, MSR_SPEC_CTRL, 0);
>         if ( rc )
>             goto out;
> 
> (similarly below then).

That looks odd to me, and is not how I would write that code.  I can
however adjust if you insist.  Given it's just a two line difference I
think it was worth having the more usual form.

> >      else
> >      {
> >          vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
> >  
> > -        rc = vmx_del_msr(v, MSR_SPEC_CTRL, VMX_MSR_GUEST);
> > -        if ( rc && rc != -ESRCH )
> > -            goto out;
> > -        rc = 0; /* Tolerate -ESRCH */
> > +        /*
> > +         * NB: there's no need to clear the virtualize SPEC_CTRL control, 
> > as
> > +         * the MSR intercept takes precedence.
> > +         */
> 
> The two VMCS values are, aiui, unused during guest entry/exit. Maybe
> worth mentioning here as well, as that not being the case would also
> raise correctness questions?

Hm, yes indeed, I've double checked and the value is not loaded, so
will expand the message.

> > --- a/xen/arch/x86/include/asm/msr.h
> > +++ b/xen/arch/x86/include/asm/msr.h
> > @@ -302,8 +302,13 @@ struct vcpu_msrs
> >       * For PV guests, this holds the guest kernel value.  It is accessed on
> >       * every entry/exit path.
> >       *
> > -     * For VT-x guests, the guest value is held in the MSR guest load/save
> > -     * list.
> > +     * For VT-x guests, the guest value is held in the MSR guest load/save 
> > list
> > +     * if there's no support for virtualized SPEC_CTRL. If virtualized
> > +     * SPEC_CTRL is enabled the value here signals which bits in SPEC_CTRL 
> > the
> > +     * guest is not able to modify.  Note that the value for those bits 
> > used in
> > +     * Xen context is also used in the guest context.  Setting a bit here
> > +     * doesn't force such bit to set in the guest context unless also set 
> > in
> > +     * Xen selection of SPEC_CTRL.
> 
> Hmm, this mask value is unlikely to be in need of being vCPU-specific.
> I'd not even expect it to be per-domain, but simply global.

This is mostly to keep the logic in-sync with the one used on AMD.

> I also can't spot where you set that field; do we really mean to give
> guests full control now that we have it (rather than e.g. running in
> IBRS-always-on mode at least under certain conditions)? If intended to
> be like this for now, this (to me at least) surprising aspect could
> likely do with mentioning in the description.

Yes, so far I didn't set any bit before the guest back, that should be
done in a separate patch.

Thanks, Roger.