On 13.01.2022 17:38, Andrew Cooper wrote:
> Second, update vmx_msr_{read,write}_intercept() to use the load/save lists
> rather than vcpu_msrs, and update the comment to describe the new state
> location.
Nit: Assuming "the comment" refers to something in the named function,
I'm afraid I can't spot any comment being updated there. Perhaps you
mean the comment in the declaration of struct vcpu_msrs?
> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -35,7 +35,14 @@ ENTRY(vmx_asm_vmexit_handler)
>
> /* SPEC_CTRL_ENTRY_FROM_VMX Req: b=curr %rsp=regs/cpuinfo, Clob:
> acd */
> ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
> - ALTERNATIVE "", DO_SPEC_CTRL_ENTRY_FROM_HVM, X86_FEATURE_SC_MSR_HVM
> +
> + .macro restore_spec_ctrl
> + mov $MSR_SPEC_CTRL, %ecx
> + movzbl CPUINFO_xen_spec_ctrl(%rsp), %eax
> + xor %edx, %edx
> + wrmsr
> + .endm
> + ALTERNATIVE "", restore_spec_ctrl, X86_FEATURE_SC_MSR_HVM
> /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>
> /* Hardware clears MSR_DEBUGCTL on VMExit. Reinstate it if
> debugging Xen. */
> @@ -83,7 +90,6 @@ UNLIKELY_END(realmode)
>
> /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
> /* SPEC_CTRL_EXIT_TO_VMX Req: a=spec_ctrl %rsp=regs/cpuinfo, Clob:
> cd */
> - ALTERNATIVE "", DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_SC_MSR_HVM
> ALTERNATIVE "", __stringify(verw CPUINFO_verw_sel(%rsp)),
> X86_FEATURE_SC_VERW_HVM
Shouldn't you update the "Clob:" remarks here as well?
> @@ -119,12 +125,11 @@ UNLIKELY_END(realmode)
> SAVE_ALL
>
> /*
> - * PV variant needed here as no guest code has executed (so
> - * MSR_SPEC_CTRL can't have changed value), and NMIs/MCEs are liable
> - * to hit (in which case the HVM variant might corrupt things).
> + * SPEC_CTRL_ENTRY notes
> + *
> + * If we end up here, no guest code has executed. We still have
> Xen's
> + * choice of MSR_SPEC_CTRL in context, and the RSB is safe.
> */
> - SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
> - /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
Is "no guest code has executed" actually enough here? If VM entry failed
due to a later MSR-load-list entry, SPEC_CTRL could have changed value
already, couldn't it?
> @@ -601,17 +602,27 @@ static void vmx_cpuid_policy_changed(struct vcpu *v)
>
> vmx_vmcs_enter(v);
> vmx_update_exception_bitmap(v);
> - vmx_vmcs_exit(v);
>
> /*
> * We can safely pass MSR_SPEC_CTRL through to the guest, even if STIBP
> * isn't enumerated in hardware, as SPEC_CTRL_STIBP is ignored.
> */
> if ( cp->feat.ibrsb )
> + {
> vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
> +
> + rc = vmx_add_guest_msr(v, MSR_SPEC_CTRL, 0);
> + if ( rc )
> + goto err;
> + }
> else
> + {
> vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
>
> + /* Can only fail with -ESRCH, and we don't care. */
> + vmx_del_msr(v, MSR_SPEC_CTRL, VMX_MSR_GUEST);
To be forward-compatible with possible future changes to the
function (or load list handling as a whole), how about having an
assertion proving what the comment says:
rc = vmx_del_msr(v, MSR_SPEC_CTRL, VMX_MSR_GUEST);
if ( rc )
{
ASSERT(rc == -ESRCH);
rc = 0;
}
?
Jan
