|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [xen staging] x86/spec-ctrl: Fix NMI race condition with VT-x MSR_SPEC_CTRL handling
commit 81f0eaadf84d273a6ff8df3660b874a02d0e7677
Author: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Wed Jan 12 15:47:27 2022 +0000
Commit: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Thu Jan 20 16:32:11 2022 +0000
x86/spec-ctrl: Fix NMI race condition with VT-x MSR_SPEC_CTRL handling
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.
Both of vmx_{add,del}_guest_msr() can fail. The -ESRCH delete case is fine,
but all others are fatal to the running of the VM, so handle them using
domain_crash() - this path is only used during domain construction anyway.
Second, update vmx_{get,set}_reg() to use the MSR load/save lists rather
than
vcpu_msrs, and update the vcpu_msrs comment to describe the new state
location.
Finally, adjust the entry/exit asm.
Because the guest value is saved and loaded atomically, we do not need to
manually load the guest value, nor do we need to enable SCF_use_shadow.
This
lets us remove the use of DO_SPEC_CTRL_EXIT_TO_GUEST. Additionally,
SPEC_CTRL_ENTRY_FROM_PV gets removed too, because on an early entry failure,
we're no longer in the guest MSR_SPEC_CTRL context needing to switch back to
Xen's context.
The only action remaining is to load Xen's MSR_SPEC_CTRL value on vmexit.
We
could in principle use the host msr list, but is expected to complicated
future work. Delete 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.
Adjust the comment at the top of spec_ctrl_asm.h in light of this bugfix.
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
xen/arch/x86/hvm/vmx/entry.S | 22 ++++++++++------
xen/arch/x86/hvm/vmx/vmx.c | 44 +++++++++++++++++++++++++++++---
xen/arch/x86/include/asm/msr.h | 10 +++++++-
xen/arch/x86/include/asm/spec_ctrl_asm.h | 32 +++--------------------
4 files changed, 68 insertions(+), 40 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index 30139ae58e..7ee3382fd0 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
/* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
/* Hardware clears MSR_DEBUGCTL on VMExit. Reinstate it if debugging
Xen. */
@@ -82,8 +89,7 @@ UNLIKELY_END(realmode)
mov VCPUMSR_spec_ctrl_raw(%rax), %eax
/* 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
+ /* SPEC_CTRL_EXIT_TO_VMX Req: %rsp=regs/cpuinfo Clob:
*/
ALTERNATIVE "", __stringify(verw CPUINFO_verw_sel(%rsp)),
X86_FEATURE_SC_VERW_HVM
mov VCPU_hvm_guest_cr2(%rbx),%rax
@@ -119,12 +125,12 @@ 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. The MSR lists have
+ * not been processed, so we still have Xen's choice of MSR_SPEC_CTRL
+ * in context, and the RSB is unchanged.
*/
- SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
- /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
call vmx_vmentry_failure
jmp .Lvmx_process_softirqs
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 833a4af6f0..bcd0ec572c 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -592,6 +592,7 @@ void vmx_update_exception_bitmap(struct vcpu *v)
static void vmx_cpuid_policy_changed(struct vcpu *v)
{
const struct cpuid_policy *cp = v->domain->arch.cpuid;
+ int rc = 0;
if ( opt_hvm_fep ||
(v->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor) )
@@ -601,17 +602,29 @@ 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 out;
+ }
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 */
+ }
+
/* MSR_PRED_CMD is safe to pass through if the guest knows about it. */
if ( cp->feat.ibrsb || cp->extd.ibpb )
vmx_clear_msr_intercept(v, MSR_PRED_CMD, VMX_MSR_RW);
@@ -623,6 +636,15 @@ static void vmx_cpuid_policy_changed(struct vcpu *v)
vmx_clear_msr_intercept(v, MSR_FLUSH_CMD, VMX_MSR_RW);
else
vmx_set_msr_intercept(v, MSR_FLUSH_CMD, VMX_MSR_RW);
+
+ out:
+ vmx_vmcs_exit(v);
+
+ if ( rc )
+ {
+ printk(XENLOG_G_ERR "%pv MSR list error: %d", v, rc);
+ domain_crash(v->domain);
+ }
}
int vmx_guest_x86_mode(struct vcpu *v)
@@ -2402,11 +2424,20 @@ static int vmtrace_reset(struct vcpu *v)
static uint64_t vmx_get_reg(struct vcpu *v, unsigned int reg)
{
struct domain *d = v->domain;
+ uint64_t val = 0;
+ int rc;
switch ( reg )
{
case MSR_SPEC_CTRL:
- return v->arch.msrs->spec_ctrl.raw;
+ rc = vmx_read_guest_msr(v, reg, &val);
+ if ( rc )
+ {
+ printk(XENLOG_G_ERR "%s(%pv, 0x%08x) MSR list error: %d\n",
+ __func__, v, reg, rc);
+ domain_crash(d);
+ }
+ return val;
default:
printk(XENLOG_G_ERR "%s(%pv, 0x%08x) Bad register\n",
@@ -2419,11 +2450,18 @@ static uint64_t vmx_get_reg(struct vcpu *v, unsigned
int reg)
static void vmx_set_reg(struct vcpu *v, unsigned int reg, uint64_t val)
{
struct domain *d = v->domain;
+ int rc;
switch ( reg )
{
case MSR_SPEC_CTRL:
- v->arch.msrs->spec_ctrl.raw = val;
+ rc = vmx_write_guest_msr(v, reg, val);
+ if ( rc )
+ {
+ printk(XENLOG_G_ERR "%s(%pv, 0x%08x) MSR list error: %d\n",
+ __func__, v, reg, rc);
+ domain_crash(d);
+ }
break;
default:
diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
index 1d3eca9063..10039c2d22 100644
--- a/xen/arch/x86/include/asm/msr.h
+++ b/xen/arch/x86/include/asm/msr.h
@@ -287,7 +287,15 @@ extern struct msr_policy raw_msr_policy,
/* Container object for per-vCPU MSRs */
struct vcpu_msrs
{
- /* 0x00000048 - MSR_SPEC_CTRL */
+ /*
+ * 0x00000048 - MSR_SPEC_CTRL
+ *
+ * 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.
+ */
struct {
uint32_t raw;
} spec_ctrl;
diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h
b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index 2b3f123cb5..bf82528a12 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -42,9 +42,10 @@
* path, or late in the exit path after restoring the guest value. This
* will corrupt the guest value.
*
- * Factor 1 is dealt with by relying on NMIs/MCEs being blocked immediately
- * after VMEXIT. The VMEXIT-specific code reads MSR_SPEC_CTRL and updates
- * current before loading Xen's MSR_SPEC_CTRL setting.
+ * Factor 1 is dealt with:
+ * - On VMX by using MSR load/save lists to have vmentry/exit atomically
+ * load/save the guest value. Xen's value is loaded in regular code, and
+ * there is no need to use the shadow logic (below).
*
* Factor 2 is harder. We maintain a shadow_spec_ctrl value, and a use_shadow
* boolean in the per cpu spec_ctrl_flags. The synchronous use is:
@@ -128,31 +129,6 @@
#endif
.endm
-.macro DO_SPEC_CTRL_ENTRY_FROM_HVM
-/*
- * Requires %rbx=current, %rsp=regs/cpuinfo
- * Clobbers %rax, %rcx, %rdx
- *
- * The common case is that a guest has direct access to MSR_SPEC_CTRL, at
- * which point we need to save the guest value before setting IBRS for Xen.
- * Unilaterally saving the guest value is shorter and faster than checking.
- */
- mov $MSR_SPEC_CTRL, %ecx
- rdmsr
-
- /* Stash the value from hardware. */
- mov VCPU_arch_msrs(%rbx), %rdx
- mov %eax, VCPUMSR_spec_ctrl_raw(%rdx)
- xor %edx, %edx
-
- /* Clear SPEC_CTRL shadowing *before* loading Xen's value. */
- andb $~SCF_use_shadow, CPUINFO_spec_ctrl_flags(%rsp)
-
- /* Load Xen's intended value. */
- movzbl CPUINFO_xen_spec_ctrl(%rsp), %eax
- wrmsr
-.endm
-
.macro DO_SPEC_CTRL_ENTRY maybexen:req
/*
* Requires %rsp=regs (also cpuinfo if !maybexen)
--
generated by git-patchbot for /home/xen/git/xen.git#staging
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |