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
