[PATCH v7 1/3] vm_event: clear up return value of vm_event_monitor_traps

[PATCH v7 1/3] vm_event: clear up return value of vm_event_monitor_traps

[Tamas K Lengyel]

The return value has not been clearly defined, with the function
never returning 0 which seemingly indicated a condition where the
guest should crash.

In this patch we define -rc as error condition where a subscriber is
present but an error prevented the notification from being sent;
0 where there is no subscriber or the notification was sent and the vCPU
is not paused (i.e. safe to continue execution as normal); and 1 where the
notification was sent with the vCPU paused and we are waiting for a
response.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v6: Remove unnecessary else clause in vmx
---
 xen/arch/x86/hvm/monitor.c | 4 ++--
 xen/arch/x86/hvm/vmx/vmx.c | 6 +++---
 xen/common/vm_event.c      | 5 +++--
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index f0ab33a..472926c 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -48,8 +48,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value, unsigned long old
             .u.write_ctrlreg.old_value = old
         };
 
-        vm_event_monitor_traps(curr, sync, &req);
-        return 1;
+        if ( vm_event_monitor_traps(curr, sync, &req) >= 0 )
+            return 1;
     }
 
     return 0;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 3850602..9d4121e 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3391,11 +3391,11 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                 break;
             }
             else {
-                int handled =
+                int rc =
                       hvm_monitor_breakpoint(regs->eip,
                                              HVM_MONITOR_SOFTWARE_BREAKPOINT);
 
-                if ( handled < 0 ) 
+                if ( !rc )
                 {
                     struct hvm_trap trap = {
                         .vector = TRAP_int3,
@@ -3409,7 +3409,7 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                     hvm_inject_trap(&trap);
                     break;
                 }
-                else if ( handled )
+                if ( rc > 0 )
                     break;
             }
 
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index ca1eced..b303180 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -806,7 +806,7 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
          * If there was no ring to handle the event, then
          * simply continue executing normally.
          */
-        return 1;
+        return 0;
     default:
         return rc;
     };
@@ -815,6 +815,7 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
     {
         req->flags |= VM_EVENT_FLAG_VCPU_PAUSED;
         vm_event_vcpu_pause(v);
+        rc = 1;
     }
 
     if ( altp2m_active(d) )
@@ -826,7 +827,7 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
     vm_event_fill_regs(req);
     vm_event_put_request(d, &d->vm_event->monitor, req);
 
-    return 1;
+    return rc;
 }
 
 /*
-- 
2.8.1


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

[PATCH v7 2/3] x86/vm_event: Add HVM debug exception vm_events

[Tamas K Lengyel]

Since in-guest debug exceptions are now unconditionally trapped to Xen, adding
a hook for vm_event subscribers to tap into this new always-on guest event. We
rename along the way hvm_event_breakpoint_type to hvm_event_type to better
match the various events that can be passed with it. We also introduce the
necessary monitor_op domctl's to enable subscribing to the events.

This patch also provides monitor subscribers to int3 events proper access
to the instruction length necessary for accurate event-reinjection. Without
this subscribers manually have to evaluate if the int3 instruction has any
prefix attached which would change the instruction length.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Razvan Cojocaru <rcojocaru@bitdefender.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>

v7: Add rc < 0 clause for crash case in vmx and cosmetic fixes
---
 tools/libxc/include/xenctrl.h       |  3 +-
 tools/libxc/xc_monitor.c            | 25 +++++++++++++++
 tools/tests/xen-access/xen-access.c | 61 ++++++++++++++++++++++++++++++++-----
 xen/arch/x86/hvm/monitor.c          | 21 +++++++++++--
 xen/arch/x86/hvm/vmx/vmx.c          | 46 ++++++++++++++++++++++------
 xen/arch/x86/monitor.c              | 16 ++++++++++
 xen/include/asm-x86/domain.h        |  2 ++
 xen/include/asm-x86/hvm/monitor.h   |  7 +++--
 xen/include/asm-x86/monitor.h       |  3 +-
 xen/include/public/domctl.h         |  6 ++++
 xen/include/public/vm_event.h       | 14 +++++++--
 11 files changed, 177 insertions(+), 27 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 4f5d954..4a85b4a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2165,7 +2165,8 @@ int xc_monitor_software_breakpoint(xc_interface *xch, domid_t domain_id,
                                    bool enable);
 int xc_monitor_guest_request(xc_interface *xch, domid_t domain_id,
                              bool enable, bool sync);
-
+int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
+                                bool enable, bool sync);
 /**
  * This function enables / disables emulation for each REP for a
  * REP-compatible instruction.
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 78131b2..264992c 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -156,3 +156,28 @@ int xc_monitor_emulate_each_rep(xc_interface *xch, domid_t domain_id,
 
     return do_domctl(xch, &domctl);
 }
+
+int xc_monitor_debug_exceptions(xc_interface *xch, domid_t domain_id,
+                                bool enable, bool sync)
+{
+    DECLARE_DOMCTL;
+
+    domctl.cmd = XEN_DOMCTL_monitor_op;
+    domctl.domain = domain_id;
+    domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
+                                    : XEN_DOMCTL_MONITOR_OP_DISABLE;
+    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION;
+    domctl.u.monitor_op.u.debug_exception.sync = sync;
+
+    return do_domctl(xch, &domctl);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index f26e723..02655d5 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -53,6 +53,10 @@
 #define ERROR(a, b...) fprintf(stderr, a "\n", ## b)
 #define PERROR(a, b...) fprintf(stderr, a ": %s\n", ## b, strerror(errno))
 
+/* From xen/include/asm-x86/processor.h */
+#define X86_TRAP_DEBUG  1
+#define X86_TRAP_INT3   3
+
 typedef struct vm_event {
     domid_t domain_id;
     xenevtchn_handle *xce_handle;
@@ -333,7 +337,7 @@ void usage(char* progname)
 {
     fprintf(stderr, "Usage: %s [-m] <domain_id> write|exec", progname);
 #if defined(__i386__) || defined(__x86_64__)
-            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec");
+            fprintf(stderr, "|breakpoint|altp2m_write|altp2m_exec|debug");
 #endif
             fprintf(stderr,
             "\n"
@@ -354,10 +358,12 @@ int main(int argc, char *argv[])
     xc_interface *xch;
     xenmem_access_t default_access = XENMEM_access_rwx;
     xenmem_access_t after_first_access = XENMEM_access_rwx;
+    int memaccess = 0;
     int required = 0;
     int breakpoint = 0;
     int shutting_down = 0;
     int altp2m = 0;
+    int debug = 0;
     uint16_t altp2m_view_id = 0;
 
     char* progname = argv[0];
@@ -391,11 +397,13 @@ int main(int argc, char *argv[])
     {
         default_access = XENMEM_access_rx;
         after_first_access = XENMEM_access_rwx;
+        memaccess = 1;
     }
     else if ( !strcmp(argv[0], "exec") )
     {
         default_access = XENMEM_access_rw;
         after_first_access = XENMEM_access_rwx;
+        memaccess = 1;
     }
 #if defined(__i386__) || defined(__x86_64__)
     else if ( !strcmp(argv[0], "breakpoint") )
@@ -406,11 +414,17 @@ int main(int argc, char *argv[])
     {
         default_access = XENMEM_access_rx;
         altp2m = 1;
+        memaccess = 1;
     }
     else if ( !strcmp(argv[0], "altp2m_exec") )
     {
         default_access = XENMEM_access_rw;
         altp2m = 1;
+        memaccess = 1;
+    }
+    else if ( !strcmp(argv[0], "debug") )
+    {
+        debug = 1;
     }
 #endif
     else
@@ -446,7 +460,7 @@ int main(int argc, char *argv[])
     }
 
     /* With altp2m we just create a new, restricted view of the memory */
-    if ( altp2m )
+    if ( memaccess && altp2m )
     {
         xen_pfn_t gfn = 0;
         unsigned long perm_set = 0;
@@ -493,7 +507,7 @@ int main(int argc, char *argv[])
         }
     }
 
-    if ( !altp2m )
+    if ( memaccess && !altp2m )
     {
         /* Set the default access type and convert all pages to it */
         rc = xc_set_mem_access(xch, domain_id, default_access, ~0ull, 0);
@@ -524,6 +538,16 @@ int main(int argc, char *argv[])
         }
     }
 
+    if ( debug )
+    {
+        rc = xc_monitor_debug_exceptions(xch, domain_id, 1, 1);
+        if ( rc < 0 )
+        {
+            ERROR("Error %d setting debug exception listener with vm_event\n", rc);
+            goto exit;
+        }
+    }
+
     /* Wait for access */
     for (;;)
     {
@@ -534,6 +558,8 @@ int main(int argc, char *argv[])
 
             if ( breakpoint )
                 rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
+            if ( debug )
+                rc = xc_monitor_debug_exceptions(xch, domain_id, 0, 0);
 
             if ( altp2m )
             {
@@ -641,16 +667,16 @@ int main(int argc, char *argv[])
                        req.vcpu_id);
 
                 /* Reinject */
-                rc = xc_hvm_inject_trap(
-                    xch, domain_id, req.vcpu_id, 3,
-                    HVMOP_TRAP_sw_exc, -1, 0, 0);
+                rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id,
+                                        X86_TRAP_INT3,
+                                        req.u.software_breakpoint.type, -1,
+                                        req.u.software_breakpoint.insn_length, 0);
                 if (rc < 0)
                 {
                     ERROR("Error %d injecting breakpoint\n", rc);
                     interrupted = -1;
                     continue;
                 }
-
                 break;
             case VM_EVENT_REASON_SINGLESTEP:
                 printf("Singlestep: rip=%016"PRIx64", vcpu %d, altp2m %u\n",
@@ -669,6 +695,27 @@ int main(int argc, char *argv[])
                 rsp.flags |= VM_EVENT_FLAG_TOGGLE_SINGLESTEP;
 
                 break;
+            case VM_EVENT_REASON_DEBUG_EXCEPTION:
+                printf("Debug exception: rip=%016"PRIx64", vcpu %d. Type: %u. Length: %u\n",
+                       req.data.regs.x86.rip,
+                       req.vcpu_id,
+                       req.u.debug_exception.type,
+                       req.u.debug_exception.insn_length);
+
+                /* Reinject */
+                rc = xc_hvm_inject_trap(xch, domain_id, req.vcpu_id,
+                                        X86_TRAP_DEBUG,
+                                        req.u.debug_exception.type, -1,
+                                        req.u.debug_exception.insn_length,
+                                        req.data.regs.x86.cr2);
+                if (rc < 0)
+                {
+                    ERROR("Error %d injecting breakpoint\n", rc);
+                    interrupted = -1;
+                    continue;
+                }
+
+                break;
             default:
                 fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
             }
diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index 472926c..bbe5952 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -87,12 +87,13 @@ static inline unsigned long gfn_of_rip(unsigned long rip)
     return paging_gva_to_gfn(curr, sreg.base + rip, &pfec);
 }
 
-int hvm_monitor_breakpoint(unsigned long rip,
-                           enum hvm_monitor_breakpoint_type type)
+int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
+                      unsigned long trap_type, unsigned long insn_length)
 {
     struct vcpu *curr = current;
     struct arch_domain *ad = &curr->domain->arch;
     vm_event_request_t req = {};
+    bool_t sync;
 
     switch ( type )
     {
@@ -101,6 +102,9 @@ int hvm_monitor_breakpoint(unsigned long rip,
             return 0;
         req.reason = VM_EVENT_REASON_SOFTWARE_BREAKPOINT;
         req.u.software_breakpoint.gfn = gfn_of_rip(rip);
+        req.u.software_breakpoint.type = trap_type;
+        req.u.software_breakpoint.insn_length = insn_length;
+        sync = 1;
         break;
 
     case HVM_MONITOR_SINGLESTEP_BREAKPOINT:
@@ -108,6 +112,17 @@ int hvm_monitor_breakpoint(unsigned long rip,
             return 0;
         req.reason = VM_EVENT_REASON_SINGLESTEP;
         req.u.singlestep.gfn = gfn_of_rip(rip);
+        sync = 1;
+        break;
+
+    case HVM_MONITOR_DEBUG_EXCEPTION:
+        if ( !ad->monitor.debug_exception_enabled )
+            return 0;
+        req.reason = VM_EVENT_REASON_DEBUG_EXCEPTION;
+        req.u.debug_exception.gfn = gfn_of_rip(rip);
+        req.u.debug_exception.type = trap_type;
+        req.u.debug_exception.insn_length = insn_length;
+        sync = !!ad->monitor.debug_exception_sync;
         break;
 
     default:
@@ -116,7 +131,7 @@ int hvm_monitor_breakpoint(unsigned long rip,
 
     req.vcpu_id = curr->vcpu_id;
 
-    return vm_event_monitor_traps(curr, 1, &req);
+    return vm_event_monitor_traps(curr, sync, &req);
 }
 
 /*
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 9d4121e..5b22d87 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3376,7 +3376,29 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
             write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
             if ( !v->domain->debugger_attached )
-                vmx_propagate_intr(intr_info);
+            {
+                unsigned long insn_len = 0;
+                int rc;
+                unsigned long trap_type = MASK_EXTR(intr_info,
+                                                    INTR_INFO_INTR_TYPE_MASK);
+
+                if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
+                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len);
+
+                rc = hvm_monitor_debug(regs->eip,
+                                       HVM_MONITOR_DEBUG_EXCEPTION,
+                                       trap_type, insn_len);
+
+                /*
+                 * !rc    continue normally
+                 * rc > 0 paused waiting for response, work here is done
+                 * rc < 0 error in monitor/vm_event, crash
+                 */
+                if ( !rc )
+                    vmx_propagate_intr(intr_info);
+                if ( rc < 0 )
+                    goto exit_and_crash;
+            }
             else
                 domain_pause_for_debugger();
             break;
@@ -3391,9 +3413,14 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                 break;
             }
             else {
-                int rc =
-                      hvm_monitor_breakpoint(regs->eip,
-                                             HVM_MONITOR_SOFTWARE_BREAKPOINT);
+                unsigned long insn_len;
+                int rc;
+
+                __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len);
+                rc = hvm_monitor_debug(regs->eip,
+                                       HVM_MONITOR_SOFTWARE_BREAKPOINT,
+                                       X86_EVENTTYPE_SW_EXCEPTION,
+                                       insn_len);
 
                 if ( !rc )
                 {
@@ -3401,11 +3428,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                         .vector = TRAP_int3,
                         .type = X86_EVENTTYPE_SW_EXCEPTION,
                         .error_code = HVM_DELIVER_NO_ERROR_CODE,
+                        .insn_len = insn_len
                     };
-                    unsigned long insn_len;
-
-                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len);
-                    trap.insn_len = insn_len;
                     hvm_inject_trap(&trap);
                     break;
                 }
@@ -3720,8 +3744,10 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
         vmx_update_cpu_exec_control(v);
         if ( v->arch.hvm_vcpu.single_step )
         {
-            hvm_monitor_breakpoint(regs->eip,
-                                   HVM_MONITOR_SINGLESTEP_BREAKPOINT);
+            hvm_monitor_debug(regs->eip,
+                              HVM_MONITOR_SINGLESTEP_BREAKPOINT,
+                              0, 0);
+
             if ( v->domain->debugger_attached )
                 domain_pause_for_debugger();
         }
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index a271161..205df41 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -224,6 +224,22 @@ int arch_monitor_domctl_event(struct domain *d,
         break;
     }
 
+    case XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION:
+    {
+        bool_t old_status = ad->monitor.debug_exception_enabled;
+
+        if ( unlikely(old_status == requested_status) )
+            return -EEXIST;
+
+        domain_pause(d);
+        ad->monitor.debug_exception_enabled = requested_status;
+        ad->monitor.debug_exception_sync = requested_status ?
+                                            mop->u.debug_exception.sync :
+                                            0;
+        domain_unpause(d);
+        break;
+    }
+
     default:
         /*
          * Should not be reached unless arch_monitor_get_capabilities() is
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 7c27f9e..8f64ae9 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -403,6 +403,8 @@ struct arch_domain
         unsigned int write_ctrlreg_onchangeonly  : 4;
         unsigned int singlestep_enabled          : 1;
         unsigned int software_breakpoint_enabled : 1;
+        unsigned int debug_exception_enabled     : 1;
+        unsigned int debug_exception_sync        : 1;
         struct monitor_msr_bitmap *msr_bitmap;
     } monitor;
 
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index 55d435e..8b0f119 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -23,10 +23,11 @@
 #include <xen/paging.h>
 #include <public/vm_event.h>
 
-enum hvm_monitor_breakpoint_type
+enum hvm_monitor_debug_type
 {
     HVM_MONITOR_SOFTWARE_BREAKPOINT,
     HVM_MONITOR_SINGLESTEP_BREAKPOINT,
+    HVM_MONITOR_DEBUG_EXCEPTION,
 };
 
 /*
@@ -39,8 +40,8 @@ bool_t hvm_monitor_cr(unsigned int index, unsigned long value,
 #define hvm_monitor_crX(cr, new, old) \
                         hvm_monitor_cr(VM_EVENT_X86_##cr, new, old)
 void hvm_monitor_msr(unsigned int msr, uint64_t value);
-int hvm_monitor_breakpoint(unsigned long rip,
-                           enum hvm_monitor_breakpoint_type type);
+int hvm_monitor_debug(unsigned long rip, enum hvm_monitor_debug_type type,
+                      unsigned long trap_type, unsigned long insn_length);
 
 #endif /* __ASM_X86_HVM_MONITOR_H__ */
 
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 94b6768..a9db3c0 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -77,7 +77,8 @@ static inline uint32_t arch_monitor_get_capabilities(struct domain *d)
     capabilities = (1U << XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG) |
                    (1U << XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR) |
                    (1U << XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT) |
-                   (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST);
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST) |
+                   (1U << XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION);
 
     /* Since we know this is on VMX, we can just call the hvm func */
     if ( hvm_is_singlestep_supported() )
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 7be3924..30020ba 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1080,6 +1080,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            2
 #define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   3
 #define XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST         4
+#define XEN_DOMCTL_MONITOR_EVENT_DEBUG_EXCEPTION       5
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
@@ -1114,6 +1115,11 @@ struct xen_domctl_monitor_op {
             /* Pause vCPU until response */
             uint8_t sync;
         } guest_request;
+
+        struct {
+            /* Pause vCPU until response */
+            uint8_t sync;
+        } debug_exception;
     } u;
 };
 typedef struct xen_domctl_monitor_op xen_domctl_monitor_op_t;
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 9270d52..68bddfb 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -29,7 +29,7 @@
 
 #include "xen.h"
 
-#define VM_EVENT_INTERFACE_VERSION 0x00000001
+#define VM_EVENT_INTERFACE_VERSION 0x00000002
 
 #if defined(__XEN__) || defined(__XEN_TOOLS__)
 
@@ -119,6 +119,8 @@
 #define VM_EVENT_REASON_SINGLESTEP              7
 /* An event has been requested via HVMOP_guest_request_vm_event. */
 #define VM_EVENT_REASON_GUEST_REQUEST           8
+/* A debug exception was caught */
+#define VM_EVENT_REASON_DEBUG_EXCEPTION         9
 
 /* Supported values for the vm_event_write_ctrlreg index. */
 #define VM_EVENT_X86_CR0    0
@@ -203,8 +205,15 @@ struct vm_event_write_ctrlreg {
     uint64_t old_value;
 };
 
+struct vm_event_singlestep {
+    uint64_t gfn;
+};
+
 struct vm_event_debug {
     uint64_t gfn;
+    uint32_t insn_length;
+    uint8_t type;        /* HVMOP_TRAP_* */
+    uint8_t _pad[3];
 };
 
 struct vm_event_mov_to_msr {
@@ -247,8 +256,9 @@ typedef struct vm_event_st {
         struct vm_event_mem_access            mem_access;
         struct vm_event_write_ctrlreg         write_ctrlreg;
         struct vm_event_mov_to_msr            mov_to_msr;
+        struct vm_event_singlestep            singlestep;
         struct vm_event_debug                 software_breakpoint;
-        struct vm_event_debug                 singlestep;
+        struct vm_event_debug                 debug_exception;
     } u;
 
     union {
-- 
2.8.1


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

[PATCH v7 3/3] x86/vmx: Clean up TRAP_int3 handling

[Tamas K Lengyel]

Clean up the handling of TRAP_int3 VMEXITs to conform to the handling
of TRAP_debug.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/vmx/vmx.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 5b22d87..c981450 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3402,17 +3402,10 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
             else
                 domain_pause_for_debugger();
             break;
-        case TRAP_int3: 
-        {
+        case TRAP_int3:
             HVMTRACE_1D(TRAP, vector);
-            if ( v->domain->debugger_attached )
+            if ( !v->domain->debugger_attached )
             {
-                update_guest_eip(); /* Safe: INT3 */            
-                v->arch.gdbsx_vcpu_event = TRAP_int3;
-                domain_pause_for_debugger();
-                break;
-            }
-            else {
                 unsigned long insn_len;
                 int rc;
 
@@ -3423,22 +3416,17 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
                                        insn_len);
 
                 if ( !rc )
-                {
-                    struct hvm_trap trap = {
-                        .vector = TRAP_int3,
-                        .type = X86_EVENTTYPE_SW_EXCEPTION,
-                        .error_code = HVM_DELIVER_NO_ERROR_CODE,
-                        .insn_len = insn_len
-                    };
-                    hvm_inject_trap(&trap);
-                    break;
-                }
-                if ( rc > 0 )
-                    break;
+                    vmx_propagate_intr(intr_info);
+                if ( rc < 0 )
+                    goto exit_and_crash;
             }
-
-            goto exit_and_crash;
-        }
+            else
+            {
+                update_guest_eip(); /* Safe: INT3 */
+                v->arch.gdbsx_vcpu_event = TRAP_int3;
+                domain_pause_for_debugger();
+            }
+            break;
         case TRAP_no_device:
             HVMTRACE_1D(TRAP, vector);
             vmx_fpu_dirty_intercept();
-- 
2.8.1


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

Re: [PATCH v7 2/3] x86/vm_event: Add HVM debug exception vm_events

[Razvan Cojocaru]

On 06/27/16 21:08, Tamas K Lengyel wrote:
> Since in-guest debug exceptions are now unconditionally trapped to Xen, adding
> a hook for vm_event subscribers to tap into this new always-on guest event. We
> rename along the way hvm_event_breakpoint_type to hvm_event_type to better
> match the various events that can be passed with it. We also introduce the
> necessary monitor_op domctl's to enable subscribing to the events.
> 
> This patch also provides monitor subscribers to int3 events proper access
> to the instruction length necessary for accurate event-reinjection. Without
> this subscribers manually have to evaluate if the int3 instruction has any
> prefix attached which would change the instruction length.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.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>
> 
> v7: Add rc < 0 clause for crash case in vmx and cosmetic fixes
> ---
>  tools/libxc/include/xenctrl.h       |  3 +-
>  tools/libxc/xc_monitor.c            | 25 +++++++++++++++
>  tools/tests/xen-access/xen-access.c | 61 ++++++++++++++++++++++++++++++++-----
>  xen/arch/x86/hvm/monitor.c          | 21 +++++++++++--
>  xen/arch/x86/hvm/vmx/vmx.c          | 46 ++++++++++++++++++++++------
>  xen/arch/x86/monitor.c              | 16 ++++++++++
>  xen/include/asm-x86/domain.h        |  2 ++
>  xen/include/asm-x86/hvm/monitor.h   |  7 +++--
>  xen/include/asm-x86/monitor.h       |  3 +-
>  xen/include/public/domctl.h         |  6 ++++
>  xen/include/public/vm_event.h       | 14 +++++++--
>  11 files changed, 177 insertions(+), 27 deletions(-)

Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>


Thanks,
Razvan

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

Re: [PATCH v7 2/3] x86/vm_event: Add HVM debug exception vm_events

[Jan Beulich]

>>> On 27.06.16 at 20:08, <tamas@tklengyel.com> wrote:
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3376,7 +3376,29 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>              HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>              write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>              if ( !v->domain->debugger_attached )
> -                vmx_propagate_intr(intr_info);
> +            {
> +                unsigned long insn_len = 0;
> +                int rc;
> +                unsigned long trap_type = MASK_EXTR(intr_info,
> +                                                    INTR_INFO_INTR_TYPE_MASK);
> +
> +                if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
> +                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len);
> +
> +                rc = hvm_monitor_debug(regs->eip,
> +                                       HVM_MONITOR_DEBUG_EXCEPTION,
> +                                       trap_type, insn_len);
> +
> +                /*
> +                 * !rc    continue normally
> +                 * rc > 0 paused waiting for response, work here is done
> +                 * rc < 0 error in monitor/vm_event, crash
> +                 */
> +                if ( !rc )
> +                    vmx_propagate_intr(intr_info);
> +                if ( rc < 0 )
> +                    goto exit_and_crash;
> +            }

As opposed to earlier versions, here omitting the "else" seems
undesirable. Or, perhaps better, simply re-order the two if()-s.
This is to make clear that what is now the second if() does in no
way depend on what the body of the current first if() does.

The same would then apply to patch 3, and I'd be fine doing the
adjustment while committing (provided all necessary acks trickle
in). Feel free to add my ack here for the few changes for which
that's actually relevant.

Jan


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

Re: [PATCH v7 2/3] x86/vm_event: Add HVM debug exception vm_events

[Tian, Kevin]

> From: Tamas K Lengyel [mailto:tamas@tklengyel.com]
> Sent: Tuesday, June 28, 2016 2:08 AM
> 
> Since in-guest debug exceptions are now unconditionally trapped to Xen, adding
> a hook for vm_event subscribers to tap into this new always-on guest event. We
> rename along the way hvm_event_breakpoint_type to hvm_event_type to better
> match the various events that can be passed with it. We also introduce the
> necessary monitor_op domctl's to enable subscribing to the events.
> 
> This patch also provides monitor subscribers to int3 events proper access
> to the instruction length necessary for accurate event-reinjection. Without
> this subscribers manually have to evaluate if the int3 instruction has any
> prefix attached which would change the instruction length.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v7 3/3] x86/vmx: Clean up TRAP_int3 handling

[Tian, Kevin]

> From: Tamas K Lengyel [mailto:tamas@tklengyel.com]
> Sent: Tuesday, June 28, 2016 2:08 AM
> 
> Clean up the handling of TRAP_int3 VMEXITs to conform to the handling
> of TRAP_debug.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

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

Re: [PATCH v7 2/3] x86/vm_event: Add HVM debug exception vm_events

[Tamas K Lengyel]

On Tue, Jun 28, 2016 at 1:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 27.06.16 at 20:08, <tamas@tklengyel.com> wrote:
>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>> @@ -3376,7 +3376,29 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>              HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>>              write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>>              if ( !v->domain->debugger_attached )
>> -                vmx_propagate_intr(intr_info);
>> +            {
>> +                unsigned long insn_len = 0;
>> +                int rc;
>> +                unsigned long trap_type = MASK_EXTR(intr_info,
>> +                                                    INTR_INFO_INTR_TYPE_MASK);
>> +
>> +                if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
>> +                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len);
>> +
>> +                rc = hvm_monitor_debug(regs->eip,
>> +                                       HVM_MONITOR_DEBUG_EXCEPTION,
>> +                                       trap_type, insn_len);
>> +
>> +                /*
>> +                 * !rc    continue normally
>> +                 * rc > 0 paused waiting for response, work here is done
>> +                 * rc < 0 error in monitor/vm_event, crash
>> +                 */
>> +                if ( !rc )
>> +                    vmx_propagate_intr(intr_info);
>> +                if ( rc < 0 )
>> +                    goto exit_and_crash;
>> +            }
>
> As opposed to earlier versions, here omitting the "else" seems
> undesirable. Or, perhaps better, simply re-order the two if()-s.
> This is to make clear that what is now the second if() does in no
> way depend on what the body of the current first if() does.
>
> The same would then apply to patch 3, and I'd be fine doing the
> adjustment while committing (provided all necessary acks trickle
> in). Feel free to add my ack here for the few changes for which
> that's actually relevant.
>
> Jan

That sounds fine to me. I think this patch only needs Wei's or Ian's
ack now for the libxc changes.

Tamas

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

Re: [PATCH v7 2/3] x86/vm_event: Add HVM debug exception vm_events

[Jan Beulich]

>>> On 29.06.16 at 19:26, <tamas@tklengyel.com> wrote:
> On Tue, Jun 28, 2016 at 1:37 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 27.06.16 at 20:08, <tamas@tklengyel.com> wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -3376,7 +3376,29 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>>>              HVMTRACE_1D(TRAP_DEBUG, exit_qualification);
>>>              write_debugreg(6, exit_qualification | DR_STATUS_RESERVED_ONE);
>>>              if ( !v->domain->debugger_attached )
>>> -                vmx_propagate_intr(intr_info);
>>> +            {
>>> +                unsigned long insn_len = 0;
>>> +                int rc;
>>> +                unsigned long trap_type = MASK_EXTR(intr_info,
>>> +                                                    
> INTR_INFO_INTR_TYPE_MASK);
>>> +
>>> +                if ( trap_type >= X86_EVENTTYPE_SW_INTERRUPT )
>>> +                    __vmread(VM_EXIT_INSTRUCTION_LEN, &insn_len);
>>> +
>>> +                rc = hvm_monitor_debug(regs->eip,
>>> +                                       HVM_MONITOR_DEBUG_EXCEPTION,
>>> +                                       trap_type, insn_len);
>>> +
>>> +                /*
>>> +                 * !rc    continue normally
>>> +                 * rc > 0 paused waiting for response, work here is done
>>> +                 * rc < 0 error in monitor/vm_event, crash
>>> +                 */
>>> +                if ( !rc )
>>> +                    vmx_propagate_intr(intr_info);
>>> +                if ( rc < 0 )
>>> +                    goto exit_and_crash;
>>> +            }
>>
>> As opposed to earlier versions, here omitting the "else" seems
>> undesirable. Or, perhaps better, simply re-order the two if()-s.
>> This is to make clear that what is now the second if() does in no
>> way depend on what the body of the current first if() does.
>>
>> The same would then apply to patch 3, and I'd be fine doing the
>> adjustment while committing (provided all necessary acks trickle
>> in). Feel free to add my ack here for the few changes for which
>> that's actually relevant.
> 
> That sounds fine to me. I think this patch only needs Wei's or Ian's
> ack now for the libxc changes.

Indeed.

Jan


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

Re: [PATCH v7 2/3] x86/vm_event: Add HVM debug exception vm_events

[Wei Liu]

On Mon, Jun 27, 2016 at 12:08:06PM -0600, Tamas K Lengyel wrote:
> Since in-guest debug exceptions are now unconditionally trapped to Xen, adding
> a hook for vm_event subscribers to tap into this new always-on guest event. We
> rename along the way hvm_event_breakpoint_type to hvm_event_type to better
> match the various events that can be passed with it. We also introduce the
> necessary monitor_op domctl's to enable subscribing to the events.
> 
> This patch also provides monitor subscribers to int3 events proper access
> to the instruction length necessary for accurate event-reinjection. Without
> this subscribers manually have to evaluate if the int3 instruction has any
> prefix attached which would change the instruction length.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Razvan Cojocaru <rcojocaru@bitdefender.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>
> 
> v7: Add rc < 0 clause for crash case in vmx and cosmetic fixes
> ---
>  tools/libxc/include/xenctrl.h       |  3 +-
>  tools/libxc/xc_monitor.c            | 25 +++++++++++++++
>  tools/tests/xen-access/xen-access.c | 61 ++++++++++++++++++++++++++++++++-----

Acked-by: Wei Liu <wei.liu2@citrix.com>

FAOD I expect HV side committer to take care of this patch.

Wei.

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