[PATCH v4] x86/vm_event: Allow overwriting Xen's i-cache used for emulation

* [PATCH v4] x86/vm_event: Allow overwriting Xen's i-cache used for emulation
@ 2016-09-22 18:54 Tamas K Lengyel
  2016-09-26 10:33 ` Jan Beulich
  2016-09-26 15:54 ` Paul Durrant
  0 siblings, 2 replies; 6+ messages in thread
From: Tamas K Lengyel @ 2016-09-22 18:54 UTC (permalink / raw)
  To: xen-devel
  Cc: Kevin Tian, Stefano Stabellini, Jan Beulich, George Dunlap,
	Tamas K Lengyel, Julien Grall, Paul Durrant, Jun Nakajima,
	Andrew Cooper

When emulating instructions Xen's emulator maintains a small i-cache fetched
from the guest memory. This patch extends the vm_event interface to allow
overwriting this i-cache via a buffer returned in the vm_event response.

When responding to a SOFTWARE_BREAKPOINT event (INT3) the monitor subscriber
normally has to remove the INT3 from memory - singlestep - place back INT3
to allow the guest to continue execution. This routine however is susceptible
to a race-condition on multi-vCPU guests. By allowing the subscriber to return
the i-cache to be used for emulation it can side-step the problem by returning
a clean buffer without the INT3 present.

As part of this patch we rename hvm_mem_access_emulate_one to
hvm_emulate_one_vm_event to better reflect that it is used in various vm_event
scenarios now, not just in response to mem_access events.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

v4: Copy insn buffer into mmio buffer to avoid more login in hvm_emulate_one
    Add comment in hvm_do_resume to preserve order as described in vm_event.h
---
 xen/arch/x86/hvm/emulate.c        | 27 +++++++++++++++++++++------
 xen/arch/x86/hvm/hvm.c            | 13 ++++++++++---
 xen/arch/x86/vm_event.c           | 11 ++++++++++-
 xen/include/asm-x86/hvm/emulate.h |  5 +++--
 xen/include/asm-x86/vm_event.h    |  5 ++++-
 xen/include/public/vm_event.h     | 17 ++++++++++++++++-
 6 files changed, 64 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index cc25676..17f7f0d 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -76,9 +76,9 @@ static int set_context_data(void *buffer, unsigned int size)
     if ( curr->arch.vm_event )
     {
         unsigned int safe_size =
-            min(size, curr->arch.vm_event->emul_read_data.size);
+            min(size, curr->arch.vm_event->emul.read.size);
 
-        memcpy(buffer, curr->arch.vm_event->emul_read_data.data, safe_size);
+        memcpy(buffer, curr->arch.vm_event->emul.read.data, safe_size);
         memset(buffer + safe_size, 0, size - safe_size);
         return X86EMUL_OKAY;
     }
@@ -1931,7 +1931,7 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
     return rc;
 }
 
-void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
+void hvm_emulate_one_vm_event(enum emul_kind kind, unsigned int trapnr,
     unsigned int errcode)
 {
     struct hvm_emulate_ctxt ctx = {{ 0 }};
@@ -1944,10 +1944,25 @@ void hvm_mem_access_emulate_one(enum emul_kind kind, unsigned int trapnr,
     case EMUL_KIND_NOWRITE:
         rc = hvm_emulate_one_no_write(&ctx);
         break;
-    case EMUL_KIND_SET_CONTEXT:
-        ctx.set_context = 1;
-        /* Intentional fall-through. */
+    case EMUL_KIND_SET_CONTEXT_INSN: {
+        struct vcpu *curr = current;
+        struct hvm_vcpu_io *vio = &curr->arch.hvm_vcpu.hvm_io;
+
+        BUILD_BUG_ON(sizeof(vio->mmio_insn) !=
+                     sizeof(curr->arch.vm_event->emul.insn.data));
+        ASSERT(!vio->mmio_insn_bytes);
+
+        /*
+         * Stash insn buffer into mmio buffer here instead of ctx
+         * to avoid having to add more logic to hvm_emulate_one.
+         */
+        vio->mmio_insn_bytes = sizeof(vio->mmio_insn);
+        memcpy(vio->mmio_insn, curr->arch.vm_event->emul.insn.data,
+               vio->mmio_insn_bytes);
+    }
+    /* Fall-through */
     default:
+        ctx.set_context = (kind == EMUL_KIND_SET_CONTEXT_DATA);
         rc = hvm_emulate_one(&ctx);
     }
 
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 7bad845..b06e4d5 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -487,15 +487,22 @@ void hvm_do_resume(struct vcpu *v)
         {
             enum emul_kind kind = EMUL_KIND_NORMAL;
 
+            /*
+             * Please observ the order here to match the flag descriptions
+             * provided in public/vm_event.h
+             */
             if ( v->arch.vm_event->emulate_flags &
                  VM_EVENT_FLAG_SET_EMUL_READ_DATA )
-                kind = EMUL_KIND_SET_CONTEXT;
+                kind = EMUL_KIND_SET_CONTEXT_DATA;
             else if ( v->arch.vm_event->emulate_flags &
                       VM_EVENT_FLAG_EMULATE_NOWRITE )
                 kind = EMUL_KIND_NOWRITE;
+            else if ( v->arch.vm_event->emulate_flags &
+                      VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
+                kind = EMUL_KIND_SET_CONTEXT_INSN;
 
-            hvm_mem_access_emulate_one(kind, TRAP_invalid_op,
-                                       HVM_DELIVER_NO_ERROR_CODE);
+            hvm_emulate_one_vm_event(kind, TRAP_invalid_op,
+                                     HVM_DELIVER_NO_ERROR_CODE);
 
             v->arch.vm_event->emulate_flags = 0;
         }
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 343b9c8..1e88d67 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -209,11 +209,20 @@ void vm_event_emulate_check(struct vcpu *v, vm_event_response_t *rsp)
         if ( p2m_mem_access_emulate_check(v, rsp) )
         {
             if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA )
-                v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
+                v->arch.vm_event->emul.read = rsp->data.emul.read;
 
             v->arch.vm_event->emulate_flags = rsp->flags;
         }
         break;
+
+    case VM_EVENT_REASON_SOFTWARE_BREAKPOINT:
+        if ( rsp->flags & VM_EVENT_FLAG_SET_EMUL_INSN_DATA )
+        {
+            v->arch.vm_event->emul.insn = rsp->data.emul.insn;
+            v->arch.vm_event->emulate_flags = rsp->flags;
+        }
+        break;
+
     default:
         break;
     };
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index 3aabcbe..96d8f0b 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -40,14 +40,15 @@ struct hvm_emulate_ctxt {
 enum emul_kind {
     EMUL_KIND_NORMAL,
     EMUL_KIND_NOWRITE,
-    EMUL_KIND_SET_CONTEXT
+    EMUL_KIND_SET_CONTEXT_DATA,
+    EMUL_KIND_SET_CONTEXT_INSN
 };
 
 int hvm_emulate_one(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
 int hvm_emulate_one_no_write(
     struct hvm_emulate_ctxt *hvmemul_ctxt);
-void hvm_mem_access_emulate_one(enum emul_kind kind,
+void hvm_emulate_one_vm_event(enum emul_kind kind,
     unsigned int trapnr,
     unsigned int errcode);
 void hvm_emulate_prepare(
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index ebb5d88..ca73f99 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -27,7 +27,10 @@
  */
 struct arch_vm_event {
     uint32_t emulate_flags;
-    struct vm_event_emul_read_data emul_read_data;
+    union {
+        struct vm_event_emul_read_data read;
+        struct vm_event_emul_insn_data insn;
+    } emul;
     struct monitor_write_data write_data;
 };
 
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index f756126..ba8e387 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -97,6 +97,14 @@
  * Requires the vCPU to be paused already (synchronous events only).
  */
 #define VM_EVENT_FLAG_SET_REGISTERS      (1 << 8)
+/*
+ * Instruction cache is being sent back to the hypervisor in the event response
+ * to be used by the emulator. This flag is only useful when combined with
+ * VM_EVENT_FLAG_EMULATE and does not take presedence if combined with
+ * VM_EVENT_FLAG_EMULATE_NOWRITE or VM_EVENT_FLAG_SET_EMUL_READ_DATA, (i.e.
+ * if any of those flags are set, only those will be honored).
+ */
+#define VM_EVENT_FLAG_SET_EMUL_INSN_DATA (1 << 9)
 
 /*
  * Reasons for the vm event request
@@ -265,6 +273,10 @@ struct vm_event_emul_read_data {
     uint8_t  data[sizeof(struct vm_event_regs_x86) - sizeof(uint32_t)];
 };
 
+struct vm_event_emul_insn_data {
+    uint8_t data[16]; /* Has to be completely filled */
+};
+
 typedef struct vm_event_st {
     uint32_t version;   /* VM_EVENT_INTERFACE_VERSION */
     uint32_t flags;     /* VM_EVENT_FLAG_* */
@@ -291,7 +303,10 @@ typedef struct vm_event_st {
             struct vm_event_regs_arm arm;
         } regs;
 
-        struct vm_event_emul_read_data emul_read_data;
+        union {
+            struct vm_event_emul_read_data read;
+            struct vm_event_emul_insn_data insn;
+        } emul;
     } data;
 } vm_event_request_t, vm_event_response_t;
 
-- 
2.9.3


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread