On Fri, Jan 14, 2022 at 01:08:43PM +0000, Andrew Cooper wrote:
> On 14/01/2022 12:50, Roger Pau Monné wrote:
> > On Thu, Jan 13, 2022 at 04:38:33PM +0000, Andrew Cooper wrote:
> >> The logic was based on a mistaken understanding of how NMI blocking on
> >> vmexit
> >> works. NMIs are only blocked for EXIT_REASON_NMI, and not for general
> >> exits.
> >> Therefore, an NMI can in general hit early in the vmx_asm_vmexit_handler
> >> path,
> >> and the guest's value will be clobbered before it is saved.
> >>
> >> Switch to using MSR load/save lists. This causes the guest value to be
> >> saved
> >> atomically with respect to NMIs/MCEs/etc.
> >>
> >> First, update vmx_cpuid_policy_changed() to configure the load/save lists
> >> at
> >> the same time as configuring the intercepts. This function is always used
> >> in
> >> remote context, so extend the vmx_vmcs_{enter,exit}() block to cover the
> >> whole
> >> function, rather than having multiple remote acquisitions of the same VMCS.
> >>
> >> vmx_add_guest_msr() can fail, but only in ways which are entirely fatal to
> >> the
> >> guest, so handle failures using domain_crash(). vmx_del_msr() can fail
> >> with
> >> -ESRCH but we don't matter, and this path will be taken during domain
> >> create
> >> on a system lacking IBRSB.
> >>
> >> 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.
> >>
> >> Finally, adjust the entry/exit asm. Drop DO_SPEC_CTRL_ENTRY_FROM_HVM
> >> entirely, and use a shorter code sequence to simply reload Xen's setting
> >> from
> >> the top-of-stack block.
> >>
> >> Because the guest values are loaded/saved atomically, we do not need to use
> >> the shadowing logic to cope with late NMIs/etc, so we can omit
> >> DO_SPEC_CTRL_EXIT_TO_GUEST entirely and VMRESUME/VMLAUNCH with Xen's value
> >> in
> >> context. Furthermore, we can drop the SPEC_CTRL_ENTRY_FROM_PV too, as
> >> there's
> >> no need to switch back to Xen's context on an early failure.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> >> ---
> >> CC: Jan Beulich <jbeul...@suse.com>
> >> CC: Roger Pau Monné <roger....@citrix.com>
> >> CC: Wei Liu <w...@xen.org>
> >> CC: Jun Nakajima <jun.nakaj...@intel.com>
> >> CC: Kevin Tian <kevin.t...@intel.com>
> >>
> >> Needs backporting as far as people can tolerate.
> >>
> >> If the entry/exit logic were in C, I'd ASSERT() that shadow tracking is
> >> off,
> >> but this is awkard to arrange in asm.
> >> ---
> >> xen/arch/x86/hvm/vmx/entry.S | 19 ++++++++++-------
> >> xen/arch/x86/hvm/vmx/vmx.c | 36
> >> +++++++++++++++++++++++++++-----
> >> xen/arch/x86/include/asm/msr.h | 10 ++++++++-
> >> xen/arch/x86/include/asm/spec_ctrl_asm.h | 32 ++++------------------------
> >> 4 files changed, 56 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
> >> index 30139ae58e9d..297ed8685126 100644
> >> --- 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
> > I assume loading the host value into SPEC_CTRL strictly needs to
> > happen after the RSB overwrite, or else you could use a VM exit host
> > load MSR entry to handle MSR_SPEC_CTRL.
>
> We could use the host load list, but Intel warned that the lists aren't
> as efficient as writing it like this, hence why I didn't use them
> originally when we thought the NMI behaviour was different. Obviously,
> we're using them now for correctness reasons, which is more important
> than avoiding them for (unquantified) perf reasons.
>
> I've got some plans for changes to how Xen handles MSR_SPEC_CTRL for its
> own protection, which I hope will bring a substantial efficiency
> improvements, and those would require us not to use a host load list here.
Might be worth mentioning that further work will prevent us from using
host load list for SPEC_CTRL has a comment here or in the commit
message.
Using host load lists would also allow us to get rid of
X86_FEATURE_SC_MSR_HVM.
Reviewed-by: Roger Pau Monné <roger....@citrix.com>
Thanks, Roger.
