[Xen-devel] [PATCH] x86/vm_event: fix race between vmx_vmexit_handler() and vm_event_resume()

[Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>]

The introspection agent can reply to a vm_event faster than
vmx_vmexit_handler() can complete in some cases, where it is then
not safe for vm_event_set_registers() to modify v->arch.user_regs.
This patch ensures that vm_event_resume() code only sets per-VCPU
data to be used for the actual setting of registers only in
hvm_do_resume() (similar to the model used to control setting of CRs
and MSRs).
The patch additionally removes the sync_vcpu_execstate(v) call from
vm_event_resume(), which is no longer necessary, which removes the
associated broadcast TLB flush (read: performance improvement).

Signed-off-by: Razvan Cojocaru <rcojocaru@xxxxxxxxxxxxxxx>
Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
 xen/arch/x86/hvm/hvm.c         | 35 +++++++++++++++++++++++++++++++++++
 xen/arch/x86/vm_event.c        | 22 ++--------------------
 xen/common/vm_event.c          | 14 +++++++-------
 xen/include/asm-x86/vm_event.h |  2 ++
 4 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index a441955..520942a 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -473,6 +473,39 @@ static bool hvm_get_pending_event(struct vcpu *v, struct 
x86_event *info)
     return hvm_funcs.get_pending_event(v, info);
 }
 
+static void hvm_vm_event_set_registers(struct vcpu *v)
+{
+    ASSERT(v == current);
+
+    if ( v->arch.vm_event->set_gprs )
+    {
+        struct cpu_user_regs *regs = guest_cpu_user_regs();
+
+        regs->rax = v->arch.vm_event->gprs.rax;
+        regs->rbx = v->arch.vm_event->gprs.rbx;
+        regs->rcx = v->arch.vm_event->gprs.rcx;
+        regs->rdx = v->arch.vm_event->gprs.rdx;
+        regs->rsp = v->arch.vm_event->gprs.rsp;
+        regs->rbp = v->arch.vm_event->gprs.rbp;
+        regs->rsi = v->arch.vm_event->gprs.rsi;
+        regs->rdi = v->arch.vm_event->gprs.rdi;
+
+        regs->r8 = v->arch.vm_event->gprs.r8;
+        regs->r9 = v->arch.vm_event->gprs.r9;
+        regs->r10 = v->arch.vm_event->gprs.r10;
+        regs->r11 = v->arch.vm_event->gprs.r11;
+        regs->r12 = v->arch.vm_event->gprs.r12;
+        regs->r13 = v->arch.vm_event->gprs.r13;
+        regs->r14 = v->arch.vm_event->gprs.r14;
+        regs->r15 = v->arch.vm_event->gprs.r15;
+
+        regs->rflags = v->arch.vm_event->gprs.rflags;
+        regs->rip = v->arch.vm_event->gprs.rip;
+
+        v->arch.vm_event->set_gprs = 0;
+    }
+}
+
 void hvm_do_resume(struct vcpu *v)
 {
     check_wakeup_from_wait();
@@ -487,6 +520,8 @@ void hvm_do_resume(struct vcpu *v)
     {
         struct monitor_write_data *w = &v->arch.vm_event->write_data;
 
+        hvm_vm_event_set_registers(v);
+
         if ( unlikely(v->arch.vm_event->emulate_flags) )
         {
             enum emul_kind kind = EMUL_KIND_NORMAL;
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 502ccc2..f66780a 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -113,26 +113,8 @@ void vm_event_set_registers(struct vcpu *v, 
vm_event_response_t *rsp)
 {
     ASSERT(atomic_read(&v->vm_event_pause_count));
 
-    v->arch.user_regs.rax = rsp->data.regs.x86.rax;
-    v->arch.user_regs.rbx = rsp->data.regs.x86.rbx;
-    v->arch.user_regs.rcx = rsp->data.regs.x86.rcx;
-    v->arch.user_regs.rdx = rsp->data.regs.x86.rdx;
-    v->arch.user_regs.rsp = rsp->data.regs.x86.rsp;
-    v->arch.user_regs.rbp = rsp->data.regs.x86.rbp;
-    v->arch.user_regs.rsi = rsp->data.regs.x86.rsi;
-    v->arch.user_regs.rdi = rsp->data.regs.x86.rdi;
-
-    v->arch.user_regs.r8 = rsp->data.regs.x86.r8;
-    v->arch.user_regs.r9 = rsp->data.regs.x86.r9;
-    v->arch.user_regs.r10 = rsp->data.regs.x86.r10;
-    v->arch.user_regs.r11 = rsp->data.regs.x86.r11;
-    v->arch.user_regs.r12 = rsp->data.regs.x86.r12;
-    v->arch.user_regs.r13 = rsp->data.regs.x86.r13;
-    v->arch.user_regs.r14 = rsp->data.regs.x86.r14;
-    v->arch.user_regs.r15 = rsp->data.regs.x86.r15;
-
-    v->arch.user_regs.rflags = rsp->data.regs.x86.rflags;
-    v->arch.user_regs.rip = rsp->data.regs.x86.rip;
+    v->arch.vm_event->gprs = rsp->data.regs.x86;
+    v->arch.vm_event->set_gprs = 1;
 }
 
 void vm_event_monitor_next_interrupt(struct vcpu *v)
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 0fe9a53..498749b 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -357,6 +357,13 @@ void vm_event_resume(struct domain *d, struct 
vm_event_domain *ved)
 {
     vm_event_response_t rsp;
 
+    /*
+     * vm_event_resume() runs either from XEN_DOMCTL_VM_EVENT_OP_*, or
+     * EVTCHN_send from the introspection consumer.  Both contexts are
+     * guaranteed not to be the subject of vm_event responses.
+     */
+    ASSERT(d != current->domain);
+
     /* Pull all responses off the ring. */
     while ( vm_event_get_response(d, ved, &rsp) )
     {
@@ -375,13 +382,6 @@ void vm_event_resume(struct domain *d, struct 
vm_event_domain *ved)
         v = d->vcpu[rsp.vcpu_id];
 
         /*
-         * Make sure the vCPU state has been synchronized for the custom
-         * handlers.
-         */
-        if ( atomic_read(&v->vm_event_pause_count) )
-            sync_vcpu_execstate(v);
-
-        /*
          * In some cases the response type needs extra handling, so here
          * we call the appropriate handlers.
          */
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index ca73f99..f703170 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -32,6 +32,8 @@ struct arch_vm_event {
         struct vm_event_emul_insn_data insn;
     } emul;
     struct monitor_write_data write_data;
+    struct vm_event_regs_x86 gprs;
+    bool_t set_gprs;
 };
 
 int vm_event_init_domain(struct domain *d);
-- 
1.9.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel