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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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

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

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.

Jan