[PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Razvan Cojocaru]

As suggested by Andrew Cooper, this patch attempts to remove
some redundancy and allow for an easier time when adding vm_events
for new control registers in the future, by having a single
VM_EVENT_REASON_WRITE_CTRLREG vm_event type, meant to serve CR0,
CR3, CR4 and (newly introduced) XCR0. The actual control register
will be deduced by the new .index field in vm_event_write_ctrlreg
(renamed from vm_event_mov_to_cr). The patch has also modified
the xen-access.c test - it is now able to log CR3 events.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Reviewed-by: Tim Deegan <tim@xen.org>

---
Changes since V3:
 - Reverted the move of CR0, CR3 and CR4 to pre-write events,
   since it is outside the scope of this patch (and reintroduced
   "Reviewed-by: Tim Deegan <tim@xen.org>" as a result, since
   now there are only minor, mostly cosmetic changes since his
   review of V2).
 - Control register vm_event indices are no longer bitmasks.
 - Changed parameter type from unsigned short to unsigned int
   in hvm_event_cr().
 - Simplified hvm_event_cr() by inverting the onchangeonly
   condition and removing the superfluous return.
 - Fixed onchangeonly case in monitor_domctl().
---
 tools/libxc/include/xenctrl.h       |    9 ++---
 tools/libxc/xc_monitor.c            |   40 +++-------------------
 tools/tests/xen-access/xen-access.c |   30 +++++++++++++++--
 xen/arch/x86/hvm/event.c            |   52 ++++++++---------------------
 xen/arch/x86/hvm/hvm.c              |   11 ++++---
 xen/arch/x86/hvm/vmx/vmx.c          |    5 +--
 xen/arch/x86/monitor.c              |   62 +++++++++++++----------------------
 xen/include/asm-x86/domain.h        |   14 +++-----
 xen/include/asm-x86/hvm/event.h     |    4 +--
 xen/include/public/domctl.h         |   12 +++----
 xen/include/public/vm_event.h       |   27 ++++++++-------
 11 files changed, 108 insertions(+), 158 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 09a7450..83fd289 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2375,12 +2375,9 @@ int xc_mem_access_disable_emulate(xc_interface *xch, domid_t domain_id);
 void *xc_monitor_enable(xc_interface *xch, domid_t domain_id, uint32_t *port);
 int xc_monitor_disable(xc_interface *xch, domid_t domain_id);
 int xc_monitor_resume(xc_interface *xch, domid_t domain_id);
-int xc_monitor_mov_to_cr0(xc_interface *xch, domid_t domain_id, bool enable,
-                          bool sync, bool onchangeonly);
-int xc_monitor_mov_to_cr3(xc_interface *xch, domid_t domain_id, bool enable,
-                          bool sync, bool onchangeonly);
-int xc_monitor_mov_to_cr4(xc_interface *xch, domid_t domain_id, bool enable,
-                          bool sync, bool onchangeonly);
+int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
+                             uint16_t index, bool enable, bool sync,
+                             bool onchangeonly);
 int xc_monitor_mov_to_msr(xc_interface *xch, domid_t domain_id, bool enable,
                           bool extended_capture);
 int xc_monitor_singlestep(xc_interface *xch, domid_t domain_id, bool enable);
diff --git a/tools/libxc/xc_monitor.c b/tools/libxc/xc_monitor.c
index 87ad968..63013de 100644
--- a/tools/libxc/xc_monitor.c
+++ b/tools/libxc/xc_monitor.c
@@ -45,8 +45,9 @@ int xc_monitor_resume(xc_interface *xch, domid_t domain_id)
                                NULL);
 }
 
-int xc_monitor_mov_to_cr0(xc_interface *xch, domid_t domain_id, bool enable,
-                          bool sync, bool onchangeonly)
+int xc_monitor_write_ctrlreg(xc_interface *xch, domid_t domain_id,
+                             uint16_t index, bool enable, bool sync,
+                             bool onchangeonly)
 {
     DECLARE_DOMCTL;
 
@@ -54,39 +55,8 @@ int xc_monitor_mov_to_cr0(xc_interface *xch, domid_t domain_id, bool enable,
     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_MOV_TO_CR0;
-    domctl.u.monitor_op.u.mov_to_cr.sync = sync;
-    domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly;
-
-    return do_domctl(xch, &domctl);
-}
-
-int xc_monitor_mov_to_cr3(xc_interface *xch, domid_t domain_id, bool enable,
-                          bool sync, bool onchangeonly)
-{
-    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_MOV_TO_CR3;
-    domctl.u.monitor_op.u.mov_to_cr.sync = sync;
-    domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly;
-
-    return do_domctl(xch, &domctl);
-}
-
-int xc_monitor_mov_to_cr4(xc_interface *xch, domid_t domain_id, bool enable,
-                          bool sync, bool onchangeonly)
-{
-    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_MOV_TO_CR4;
+    domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG;
+    domctl.u.monitor_op.u.mov_to_cr.index = index;
     domctl.u.monitor_op.u.mov_to_cr.sync = sync;
     domctl.u.monitor_op.u.mov_to_cr.onchangeonly = onchangeonly;
 
diff --git a/tools/tests/xen-access/xen-access.c b/tools/tests/xen-access/xen-access.c
index 12ab921..909aba6 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -318,9 +318,9 @@ static void put_response(vm_event_t *vm_event, vm_event_response_t *rsp)
 void usage(char* progname)
 {
     fprintf(stderr,
-            "Usage: %s [-m] <domain_id> write|exec|breakpoint\n"
+            "Usage: %s [-m] <domain_id> write|exec|breakpoint|cr3\n"
             "\n"
-            "Logs first page writes, execs, or breakpoint traps that occur on the domain.\n"
+            "Logs first page writes, execs, CR3 writes or breakpoint traps that occur on the domain.\n"
             "\n"
             "-m requires this program to run, or else the domain may pause\n",
             progname);
@@ -341,6 +341,7 @@ int main(int argc, char *argv[])
     int required = 0;
     int breakpoint = 0;
     int shutting_down = 0;
+    int cr3 = 0;
 
     char* progname = argv[0];
     argv++;
@@ -383,6 +384,10 @@ int main(int argc, char *argv[])
     {
         breakpoint = 1;
     }
+    else if ( !strcmp(argv[0], "cr3") )
+    {
+        cr3 = 1;
+    }
     else
     {
         usage(argv[0]);
@@ -443,6 +448,16 @@ int main(int argc, char *argv[])
         }
     }
 
+    if ( cr3 )
+    {
+        rc = xc_monitor_write_ctrlreg(xch, domain_id, VM_EVENT_X86_CR3, 1, 1, 1);
+        if ( rc < 0 )
+        {
+            ERROR("Error %d requesting CR3 monitoring with vm_event\n", rc);
+            goto exit;
+        }
+    }
+
     /* Wait for access */
     for (;;)
     {
@@ -455,6 +470,7 @@ int main(int argc, char *argv[])
             rc = xc_set_mem_access(xch, domain_id, XENMEM_access_rwx, START_PFN,
                                    (xenaccess->max_gpfn - START_PFN) );
             rc = xc_monitor_software_breakpoint(xch, domain_id, 0);
+            rc = xc_monitor_write_ctrlreg(xch, domain_id, VM_EVENT_X86_CR3, 0, 1, 1);
 
             shutting_down = 1;
         }
@@ -546,6 +562,16 @@ int main(int argc, char *argv[])
                 }
 
                 break;
+            case VM_EVENT_REASON_WRITE_CTRLREG:
+                if ( req.u.write_ctrlreg.index == VM_EVENT_X86_CR3 )
+                    printf("CR3: old 0x%016lx new 0x%016lx\n",
+                        req.u.write_ctrlreg.new_value,
+                        req.u.write_ctrlreg.old_value);
+                else
+                    printf("Non-CR3 control register write event: 0x%08lx\n",
+                        req.u.write_ctrlreg.index);
+
+                break;
             default:
                 fprintf(stderr, "UNKNOWN REASON CODE %d\n", req.reason);
             }
diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 9d5f9f3..310f0e8 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -88,55 +88,29 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
     return 1;
 }
 
-static inline
-void hvm_event_cr(uint32_t reason, unsigned long value,
-                         unsigned long old, bool_t onchangeonly, bool_t sync)
+void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
 {
-    if ( onchangeonly && value == old )
+    struct arch_domain *currad = &current->domain->arch;
+    unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
+
+    if ( !(currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) )
         return;
-    else
+
+    if ( !(currad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
+         value != old )
     {
         vm_event_request_t req = {
-            .reason = reason,
+            .reason = VM_EVENT_REASON_WRITE_CTRLREG,
             .vcpu_id = current->vcpu_id,
-            .u.mov_to_cr.new_value = value,
-            .u.mov_to_cr.old_value = old
+            .u.write_ctrlreg.index = index,
+            .u.write_ctrlreg.new_value = value,
+            .u.write_ctrlreg.old_value = old
         };
 
-        hvm_event_traps(sync, &req);
+        hvm_event_traps(currad->monitor.write_ctrlreg_sync & index, &req);
     }
 }
 
-void hvm_event_cr0(unsigned long value, unsigned long old)
-{
-    struct arch_domain *currad = &current->domain->arch;
-
-    if ( currad->monitor.mov_to_cr0_enabled )
-        hvm_event_cr(VM_EVENT_REASON_MOV_TO_CR0, value, old,
-                     currad->monitor.mov_to_cr0_onchangeonly,
-                     currad->monitor.mov_to_cr0_sync);
-}
-
-void hvm_event_cr3(unsigned long value, unsigned long old)
-{
-    struct arch_domain *currad = &current->domain->arch;
-
-    if ( currad->monitor.mov_to_cr3_enabled )
-        hvm_event_cr(VM_EVENT_REASON_MOV_TO_CR3, value, old,
-                     currad->monitor.mov_to_cr3_onchangeonly,
-                     currad->monitor.mov_to_cr3_sync);
-}
-
-void hvm_event_cr4(unsigned long value, unsigned long old)
-{
-    struct arch_domain *currad = &current->domain->arch;
-
-    if ( currad->monitor.mov_to_cr4_enabled )
-        hvm_event_cr(VM_EVENT_REASON_MOV_TO_CR4, value, old,
-                     currad->monitor.mov_to_cr4_onchangeonly,
-                     currad->monitor.mov_to_cr4_sync);
-}
-
 void hvm_event_msr(unsigned int msr, uint64_t value)
 {
     struct vcpu *curr = current;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 689e402..4d2e4e9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2965,11 +2965,14 @@ out:
 int hvm_handle_xsetbv(u32 index, u64 new_bv)
 {
     struct segment_register sreg;
+    struct vcpu *curr = current;
 
-    hvm_get_segment_register(current, x86_seg_ss, &sreg);
+    hvm_get_segment_register(curr, x86_seg_ss, &sreg);
     if ( sreg.attr.fields.dpl != 0 )
         goto err;
 
+    hvm_event_cr(VM_EVENT_X86_XCR0, new_bv, curr->arch.xcr0);
+
     if ( handle_xsetbv(index, new_bv) )
         goto err;
 
@@ -3267,7 +3270,7 @@ int hvm_set_cr0(unsigned long value)
         hvm_funcs.handle_cd(v, value);
 
     hvm_update_cr(v, 0, value);
-    hvm_event_cr0(value, old_value);
+    hvm_event_cr(VM_EVENT_X86_CR0, value, old_value);
 
     if ( (value ^ old_value) & X86_CR0_PG ) {
         if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
@@ -3308,7 +3311,7 @@ int hvm_set_cr3(unsigned long value)
     old=v->arch.hvm_vcpu.guest_cr[3];
     v->arch.hvm_vcpu.guest_cr[3] = value;
     paging_update_cr3(v);
-    hvm_event_cr3(value, old);
+    hvm_event_cr(VM_EVENT_X86_CR3, value, old);
     return X86EMUL_OKAY;
 
  bad_cr3:
@@ -3349,7 +3352,7 @@ int hvm_set_cr4(unsigned long value)
     }
 
     hvm_update_cr(v, 4, value);
-    hvm_event_cr4(value, old_cr);
+    hvm_event_cr(VM_EVENT_X86_CR4, value, old_cr);
 
     /*
      * Modifying CR4.{PSE,PAE,PGE,SMEP}, or clearing CR4.PCIDE
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 74f563f..72c1350 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1262,7 +1262,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
                 v->arch.hvm_vmx.exec_control |= cr3_ctls;
 
             /* Trap CR3 updates if CR3 memory events are enabled. */
-            if ( v->domain->arch.monitor.mov_to_cr3_enabled )
+            if ( v->domain->arch.monitor.write_ctrlreg_enabled &
+                 monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
                 v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
 
             vmx_update_cpu_exec_control(v);
@@ -2010,7 +2011,7 @@ static int vmx_cr_access(unsigned long exit_qualification)
         unsigned long old = curr->arch.hvm_vcpu.guest_cr[0];
         curr->arch.hvm_vcpu.guest_cr[0] &= ~X86_CR0_TS;
         vmx_update_guest_cr(curr, 0);
-        hvm_event_cr0(curr->arch.hvm_vcpu.guest_cr[0], old);
+        hvm_event_cr(VM_EVENT_X86_CR0, curr->arch.hvm_vcpu.guest_cr[0], old);
         HVMTRACE_0D(CLTS);
         break;
     }
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index d7b1c18..810e563 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -67,59 +67,41 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
 
     switch ( mop->event )
     {
-    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR0:
+    case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
     {
-        bool_t status = ad->monitor.mov_to_cr0_enabled;
-
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
-
-        ad->monitor.mov_to_cr0_sync = mop->u.mov_to_cr.sync;
-        ad->monitor.mov_to_cr0_onchangeonly = mop->u.mov_to_cr.onchangeonly;
-
-        domain_pause(d);
-        ad->monitor.mov_to_cr0_enabled = !status;
-        domain_unpause(d);
-        break;
-    }
-
-    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR3:
-    {
-        bool_t status = ad->monitor.mov_to_cr3_enabled;
+        unsigned int ctrlreg_bitmask =
+            monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
+        bool_t status = ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask;
         struct vcpu *v;
 
         rc = status_check(mop, status);
         if ( rc )
             return rc;
 
-        ad->monitor.mov_to_cr3_sync = mop->u.mov_to_cr.sync;
-        ad->monitor.mov_to_cr3_onchangeonly = mop->u.mov_to_cr.onchangeonly;
+        if ( mop->u.mov_to_cr.sync )
+            ad->monitor.write_ctrlreg_sync |= ctrlreg_bitmask;
+        else
+            ad->monitor.write_ctrlreg_sync &= ~ctrlreg_bitmask;
 
-        domain_pause(d);
-        ad->monitor.mov_to_cr3_enabled = !status;
-        domain_unpause(d);
+        if ( mop->u.mov_to_cr.onchangeonly )
+            ad->monitor.write_ctrlreg_onchangeonly |= ctrlreg_bitmask;
+        else
+            ad->monitor.write_ctrlreg_onchangeonly &= ~ctrlreg_bitmask;
 
-        /* Latches new CR3 mask through CR0 code */
-        for_each_vcpu ( d, v )
-            hvm_funcs.update_guest_cr(v, 0);
-        break;
-    }
+        domain_pause(d);
 
-    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR4:
-    {
-        bool_t status = ad->monitor.mov_to_cr4_enabled;
+        if ( !status )
+            ad->monitor.write_ctrlreg_enabled |= ctrlreg_bitmask;
+        else
+            ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
 
-        rc = status_check(mop, status);
-        if ( rc )
-            return rc;
+        domain_unpause(d);
 
-        ad->monitor.mov_to_cr4_sync = mop->u.mov_to_cr.sync;
-        ad->monitor.mov_to_cr4_onchangeonly = mop->u.mov_to_cr.onchangeonly;
+        if ( mop->u.mov_to_cr.index == VM_EVENT_X86_CR3 )
+            /* Latches new CR3 mask through CR0 code */
+            for_each_vcpu ( d, v )
+                hvm_funcs.update_guest_cr(v, 0);
 
-        domain_pause(d);
-        ad->monitor.mov_to_cr4_enabled = !status;
-        domain_unpause(d);
         break;
     }
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 45b5283..6748366 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -249,6 +249,8 @@ struct pv_domain
     struct mapcache_domain mapcache;
 };
 
+#define monitor_ctrlreg_bitmask(ctrlreg_index) (1 << ctrlreg_index)
+
 struct arch_domain
 {
     struct page_info *perdomain_l3_pg;
@@ -341,15 +343,9 @@ struct arch_domain
 
     /* Monitor options */
     struct {
-        uint16_t mov_to_cr0_enabled          : 1;
-        uint16_t mov_to_cr0_sync             : 1;
-        uint16_t mov_to_cr0_onchangeonly     : 1;
-        uint16_t mov_to_cr3_enabled          : 1;
-        uint16_t mov_to_cr3_sync             : 1;
-        uint16_t mov_to_cr3_onchangeonly     : 1;
-        uint16_t mov_to_cr4_enabled          : 1;
-        uint16_t mov_to_cr4_sync             : 1;
-        uint16_t mov_to_cr4_onchangeonly     : 1;
+        uint16_t write_ctrlreg_enabled       : 4;
+        uint16_t write_ctrlreg_sync          : 4;
+        uint16_t write_ctrlreg_onchangeonly  : 4;
         uint16_t mov_to_msr_enabled          : 1;
         uint16_t mov_to_msr_extended         : 1;
         uint16_t singlestep_enabled          : 1;
diff --git a/xen/include/asm-x86/hvm/event.h b/xen/include/asm-x86/hvm/event.h
index bb757a1..3088f3d 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -19,9 +19,7 @@
 #define __ASM_X86_HVM_EVENT_H__
 
 /* Called for current VCPU on crX/MSR changes by guest */
-void hvm_event_cr0(unsigned long value, unsigned long old);
-void hvm_event_cr3(unsigned long value, unsigned long old);
-void hvm_event_cr4(unsigned long value, unsigned long old);
+void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old);
 void hvm_event_msr(unsigned int msr, uint64_t value);
 /* Called for current VCPU: returns -1 if no listener */
 int hvm_event_int3(unsigned long gla);
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 0c0ea4a..32629f9 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1034,12 +1034,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_psr_cmt_op_t);
 #define XEN_DOMCTL_MONITOR_OP_ENABLE   0
 #define XEN_DOMCTL_MONITOR_OP_DISABLE  1
 
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR0            0
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR3            1
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR4            2
-#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            3
-#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            4
-#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   5
+#define XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG         0
+#define XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR            1
+#define XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP            2
+#define XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT   3
 
 struct xen_domctl_monitor_op {
     uint32_t op; /* XEN_DOMCTL_MONITOR_OP_* */
@@ -1050,6 +1048,8 @@ struct xen_domctl_monitor_op {
      */
     union {
         struct {
+            /* Which control register */
+            uint8_t index;
             /* Pause vCPU until response */
             uint8_t sync;
             /* Send event only on a change of value */
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index c7426de..31c76f1 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -60,22 +60,24 @@
 #define VM_EVENT_REASON_MEM_SHARING             2
 /* Memory paging event */
 #define VM_EVENT_REASON_MEM_PAGING              3
-/* CR0 was updated */
-#define VM_EVENT_REASON_MOV_TO_CR0              4
-/* CR3 was updated */
-#define VM_EVENT_REASON_MOV_TO_CR3              5
-/* CR4 was updated */
-#define VM_EVENT_REASON_MOV_TO_CR4              6
+/* A control register was updated */
+#define VM_EVENT_REASON_WRITE_CTRLREG           4
 /* An MSR was updated. */
-#define VM_EVENT_REASON_MOV_TO_MSR              7
+#define VM_EVENT_REASON_MOV_TO_MSR              5
 /* Debug operation executed (e.g. int3) */
-#define VM_EVENT_REASON_SOFTWARE_BREAKPOINT     8
+#define VM_EVENT_REASON_SOFTWARE_BREAKPOINT     6
 /* Single-step (e.g. MTF) */
-#define VM_EVENT_REASON_SINGLESTEP              9
+#define VM_EVENT_REASON_SINGLESTEP              7
+
+/* Supported values for the vm_event_write_ctrlreg index. */
+#define VM_EVENT_X86_CR0    0
+#define VM_EVENT_X86_CR3    1
+#define VM_EVENT_X86_CR4    2
+#define VM_EVENT_X86_XCR0   3
 
 /*
  * Using a custom struct (not hvm_hw_cpu) so as to not fill
- * the mem_event ring buffer too quickly.
+ * the vm_event ring buffer too quickly.
  */
 struct vm_event_regs_x86 {
     uint64_t rax;
@@ -156,7 +158,8 @@ struct vm_event_mem_access {
     uint32_t _pad;
 };
 
-struct vm_event_mov_to_cr {
+struct vm_event_write_ctrlreg {
+    uint64_t index;
     uint64_t new_value;
     uint64_t old_value;
 };
@@ -196,7 +199,7 @@ typedef struct vm_event_st {
         struct vm_event_paging                mem_paging;
         struct vm_event_sharing               mem_sharing;
         struct vm_event_mem_access            mem_access;
-        struct vm_event_mov_to_cr             mov_to_cr;
+        struct vm_event_write_ctrlreg         write_ctrlreg;
         struct vm_event_mov_to_msr            mov_to_msr;
         struct vm_event_debug                 software_breakpoint;
         struct vm_event_debug                 singlestep;
-- 
1.7.9.5

Re: [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Razvan Cojocaru]

> diff --git a/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
> index 9d5f9f3..310f0e8 100644
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -88,55 +88,29 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
>      return 1;
>  }
>  
> -static inline
> -void hvm_event_cr(uint32_t reason, unsigned long value,
> -                         unsigned long old, bool_t onchangeonly, bool_t sync)
> +void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
>  {
> -    if ( onchangeonly && value == old )
> +    struct arch_domain *currad = &current->domain->arch;
> +    unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
> +
> +    if ( !(currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) )
>          return;
> -    else
> +
> +    if ( !(currad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
> +         value != old )
>      {
>          vm_event_request_t req = {
> -            .reason = reason,
> +            .reason = VM_EVENT_REASON_WRITE_CTRLREG,
>              .vcpu_id = current->vcpu_id,
> -            .u.mov_to_cr.new_value = value,
> -            .u.mov_to_cr.old_value = old
> +            .u.write_ctrlreg.index = index,
> +            .u.write_ctrlreg.new_value = value,
> +            .u.write_ctrlreg.old_value = old
>          };
>  
> -        hvm_event_traps(sync, &req);
> +        hvm_event_traps(currad->monitor.write_ctrlreg_sync & index, &req);

Sorry, I've missed a required change here, the last line should now read
"currad->monitor.write_ctrlreg_sync & ctrlreg_bitmask", of course. I'll
fix this in V5, as well as address other remarks made on this patch.


Thanks,
Razvan

Re: [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Jan Beulich]

>>> On 21.05.15 at 15:00, <rcojocaru@bitdefender.com> wrote:
> --- a/xen/arch/x86/hvm/event.c
> +++ b/xen/arch/x86/hvm/event.c
> @@ -88,55 +88,29 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
>      return 1;
>  }
>  
> -static inline
> -void hvm_event_cr(uint32_t reason, unsigned long value,
> -                         unsigned long old, bool_t onchangeonly, bool_t sync)
> +void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
>  {
> -    if ( onchangeonly && value == old )
> +    struct arch_domain *currad = &current->domain->arch;
> +    unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
> +
> +    if ( !(currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) )
>          return;
> -    else
> +
> +    if ( !(currad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
> +         value != old )
>      {

While this is now better than the previous if/else pair, I still don't
see why this can't be just a single if().

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2965,11 +2965,14 @@ out:
>  int hvm_handle_xsetbv(u32 index, u64 new_bv)
>  {
>      struct segment_register sreg;
> +    struct vcpu *curr = current;
>  
> -    hvm_get_segment_register(current, x86_seg_ss, &sreg);
> +    hvm_get_segment_register(curr, x86_seg_ss, &sreg);
>      if ( sreg.attr.fields.dpl != 0 )
>          goto err;
>  
> +    hvm_event_cr(VM_EVENT_X86_XCR0, new_bv, curr->arch.xcr0);
> +
>      if ( handle_xsetbv(index, new_bv) )
>          goto err;

So this is a pre event now, contrary to all others. Is that really a
good idea? And is it a good idea in the first place to introduce new
events in a patch titled "cleanup"?

Also, for readability I wonder whether an identically named macro
wrapper around hvm_event_cr() wouldn't be a good idea, eliminating
the need to spell out VM_EVENT_X86_ at each use site.

> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -67,59 +67,41 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>  
>      switch ( mop->event )
>      {
> -    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR0:
> +    case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
>      {
> -        bool_t status = ad->monitor.mov_to_cr0_enabled;
> -
> -        rc = status_check(mop, status);
> -        if ( rc )
> -            return rc;
> -
> -        ad->monitor.mov_to_cr0_sync = mop->u.mov_to_cr.sync;
> -        ad->monitor.mov_to_cr0_onchangeonly = mop->u.mov_to_cr.onchangeonly;
> -
> -        domain_pause(d);
> -        ad->monitor.mov_to_cr0_enabled = !status;
> -        domain_unpause(d);
> -        break;
> -    }
> -
> -    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR3:
> -    {
> -        bool_t status = ad->monitor.mov_to_cr3_enabled;
> +        unsigned int ctrlreg_bitmask =
> +            monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
> +        bool_t status = ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask;

!!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask)

> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -249,6 +249,8 @@ struct pv_domain
>      struct mapcache_domain mapcache;
>  };
>  
> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1 << ctrlreg_index)

(1U << (ctrlreg_index)) would seem better to me. Also - does this
need to be in this header (which gets included basically everywhere)?

> @@ -1050,6 +1048,8 @@ struct xen_domctl_monitor_op {
>       */
>      union {
>          struct {
> +            /* Which control register */
> +            uint8_t index;

Okay, 8 bits here (which is reasonable), but ...

> @@ -156,7 +158,8 @@ struct vm_event_mem_access {
>      uint32_t _pad;
>  };
>  
> -struct vm_event_mov_to_cr {
> +struct vm_event_write_ctrlreg {
> +    uint64_t index;
>      uint64_t new_value;
>      uint64_t old_value;
>  };

... a full 64 bits here? 32 bits surely would do (with 32 bits of padding),
allowing slightly better accessing code on both the consumer and
producer sides.

Jan

Re: [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Razvan Cojocaru]

On 05/22/2015 10:36 AM, Jan Beulich wrote:
>>>> On 21.05.15 at 15:00, <rcojocaru@bitdefender.com> wrote:
>> --- a/xen/arch/x86/hvm/event.c
>> +++ b/xen/arch/x86/hvm/event.c
>> @@ -88,55 +88,29 @@ static int hvm_event_traps(uint8_t sync, vm_event_request_t *req)
>>      return 1;
>>  }
>>  
>> -static inline
>> -void hvm_event_cr(uint32_t reason, unsigned long value,
>> -                         unsigned long old, bool_t onchangeonly, bool_t sync)
>> +void hvm_event_cr(unsigned int index, unsigned long value, unsigned long old)
>>  {
>> -    if ( onchangeonly && value == old )
>> +    struct arch_domain *currad = &current->domain->arch;
>> +    unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
>> +
>> +    if ( !(currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) )
>>          return;
>> -    else
>> +
>> +    if ( !(currad->monitor.write_ctrlreg_onchangeonly & ctrlreg_bitmask) ||
>> +         value != old )
>>      {
> 
> While this is now better than the previous if/else pair, I still don't
> see why this can't be just a single if().

Ack, will change it to a single if.

>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -2965,11 +2965,14 @@ out:
>>  int hvm_handle_xsetbv(u32 index, u64 new_bv)
>>  {
>>      struct segment_register sreg;
>> +    struct vcpu *curr = current;
>>  
>> -    hvm_get_segment_register(current, x86_seg_ss, &sreg);
>> +    hvm_get_segment_register(curr, x86_seg_ss, &sreg);
>>      if ( sreg.attr.fields.dpl != 0 )
>>          goto err;
>>  
>> +    hvm_event_cr(VM_EVENT_X86_XCR0, new_bv, curr->arch.xcr0);
>> +
>>      if ( handle_xsetbv(index, new_bv) )
>>          goto err;
> 
> So this is a pre event now, contrary to all others. Is that really a
> good idea? And is it a good idea in the first place to introduce new
> events in a patch titled "cleanup"?

While it is indeed different from the rest of the CR events in that it
is a pre-write, it is not however different from the MSR event, which is
currently pre-write.

As for introducing the new event, it's a trivial one-line change if we
don't count the VM_EVENT_X86_XCR0 constant and the extra bit in the
enabled, sync and onchangeonly fields.

Should I drop the introduction of the XR0 event from this patch and come
back to it in the following iteration of the series?

> Also, for readability I wonder whether an identically named macro
> wrapper around hvm_event_cr() wouldn't be a good idea, eliminating
> the need to spell out VM_EVENT_X86_ at each use site.

#define hvm_event_cr0(new, old) hvm_event_cr(VM_EVENT_X86_XCR0, new,
old)? Sure.

>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
>> @@ -67,59 +67,41 @@ int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
>>  
>>      switch ( mop->event )
>>      {
>> -    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR0:
>> +    case XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG:
>>      {
>> -        bool_t status = ad->monitor.mov_to_cr0_enabled;
>> -
>> -        rc = status_check(mop, status);
>> -        if ( rc )
>> -            return rc;
>> -
>> -        ad->monitor.mov_to_cr0_sync = mop->u.mov_to_cr.sync;
>> -        ad->monitor.mov_to_cr0_onchangeonly = mop->u.mov_to_cr.onchangeonly;
>> -
>> -        domain_pause(d);
>> -        ad->monitor.mov_to_cr0_enabled = !status;
>> -        domain_unpause(d);
>> -        break;
>> -    }
>> -
>> -    case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_CR3:
>> -    {
>> -        bool_t status = ad->monitor.mov_to_cr3_enabled;
>> +        unsigned int ctrlreg_bitmask =
>> +            monitor_ctrlreg_bitmask(mop->u.mov_to_cr.index);
>> +        bool_t status = ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask;
> 
> !!(ad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask)

Ack.

>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -249,6 +249,8 @@ struct pv_domain
>>      struct mapcache_domain mapcache;
>>  };
>>  
>> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1 << ctrlreg_index)
> 
> (1U << (ctrlreg_index)) would seem better to me. Also - does this
> need to be in this header (which gets included basically everywhere)?

Ack. As for the header, that's where the fields are
(write_ctrlreg_enabled, sync and onchangeonly), and since several .c
files use the fields it seemed the natural place to put the macros.

>> @@ -1050,6 +1048,8 @@ struct xen_domctl_monitor_op {
>>       */
>>      union {
>>          struct {
>> +            /* Which control register */
>> +            uint8_t index;
> 
> Okay, 8 bits here (which is reasonable), but ...
> 
>> @@ -156,7 +158,8 @@ struct vm_event_mem_access {
>>      uint32_t _pad;
>>  };
>>  
>> -struct vm_event_mov_to_cr {
>> +struct vm_event_write_ctrlreg {
>> +    uint64_t index;
>>      uint64_t new_value;
>>      uint64_t old_value;
>>  };
> 
> ... a full 64 bits here? 32 bits surely would do (with 32 bits of padding),
> allowing slightly better accessing code on both the consumer and
> producer sides.

Ack, will modify it. I set it to 64 bits there because I thought I'd get
free padding, but you're right, it's better to be explicit.


Thanks,
Razvan

Re: [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Jan Beulich]

>>> On 22.05.15 at 09:50, <rcojocaru@bitdefender.com> wrote:
> On 05/22/2015 10:36 AM, Jan Beulich wrote:
>>>>> On 21.05.15 at 15:00, <rcojocaru@bitdefender.com> wrote:
> Should I drop the introduction of the XR0 event from this patch and come
> back to it in the following iteration of the series?

Or simply adjust the title of the patch.

>> Also, for readability I wonder whether an identically named macro
>> wrapper around hvm_event_cr() wouldn't be a good idea, eliminating
>> the need to spell out VM_EVENT_X86_ at each use site.
> 
> #define hvm_event_cr0(new, old) hvm_event_cr(VM_EVENT_X86_XCR0, new,
> old)? Sure.

Maybe this way. But specifically said "identically named", i.e.
envisioned

#define hvm_event_cr(what, new, old) hvm_event_cr(VM_EVENT_X86_##what, new, old)

>>> --- a/xen/include/asm-x86/domain.h
>>> +++ b/xen/include/asm-x86/domain.h
>>> @@ -249,6 +249,8 @@ struct pv_domain
>>>      struct mapcache_domain mapcache;
>>>  };
>>>  
>>> +#define monitor_ctrlreg_bitmask(ctrlreg_index) (1 << ctrlreg_index)
>> 
>> (1U << (ctrlreg_index)) would seem better to me. Also - does this
>> need to be in this header (which gets included basically everywhere)?
> 
> Ack. As for the header, that's where the fields are
> (write_ctrlreg_enabled, sync and onchangeonly), and since several .c
> files use the fields it seemed the natural place to put the macros.

A monitor/event specific header would seem more natural to me.
If everything relating to field in struct domain and struct vcpu got
put in this one header, we'd end up with a mess.

Jan

Re: [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Razvan Cojocaru]

On 05/22/2015 10:36 AM, Jan Beulich wrote:
>> @@ -1050,6 +1048,8 @@ struct xen_domctl_monitor_op {
>>       */
>>      union {
>>          struct {
>> +            /* Which control register */
>> +            uint8_t index;
> 
> Okay, 8 bits here (which is reasonable), but ...
> 
>> @@ -156,7 +158,8 @@ struct vm_event_mem_access {
>>      uint32_t _pad;
>>  };
>>  
>> -struct vm_event_mov_to_cr {
>> +struct vm_event_write_ctrlreg {
>> +    uint64_t index;
>>      uint64_t new_value;
>>      uint64_t old_value;
>>  };
> 
> ... a full 64 bits here? 32 bits surely would do (with 32 bits of padding),
> allowing slightly better accessing code on both the consumer and
> producer sides.

While working on this I found the following in the vm_event.h header:

168 struct vm_event_debug {
169     uint64_t gfn;
170     uint32_t _pad;
171 };

Is this supposed to be 64 + 32 bits padding?


Thanks,
Razvan

Re: [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Jan Beulich]

>>> On 22.05.15 at 14:15, <rcojocaru@bitdefender.com> wrote:
> While working on this I found the following in the vm_event.h header:
> 
> 168 struct vm_event_debug {
> 169     uint64_t gfn;
> 170     uint32_t _pad;
> 171 };
> 
> Is this supposed to be 64 + 32 bits padding?

I think I need to direct the question back to you.

Jan

Re: [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Razvan Cojocaru]

On 05/22/2015 03:22 PM, Jan Beulich wrote:
>>>> On 22.05.15 at 14:15, <rcojocaru@bitdefender.com> wrote:
>> While working on this I found the following in the vm_event.h header:
>>
>> 168 struct vm_event_debug {
>> 169     uint64_t gfn;
>> 170     uint32_t _pad;
>> 171 };
>>
>> Is this supposed to be 64 + 32 bits padding?
> 
> I think I need to direct the question back to you.

Fair enough, let me rephrase that: I don't see how that padding would be
useful, and I propose to remove _pad as part of the next version of this
patch (or a different smaller patch).


Thanks,
Razvan

Re: [PATCH V4] xen/vm_event: Clean up control-register-write vm_events

[Jan Beulich]

>>> On 22.05.15 at 14:45, <rcojocaru@bitdefender.com> wrote:
> On 05/22/2015 03:22 PM, Jan Beulich wrote:
>>>>> On 22.05.15 at 14:15, <rcojocaru@bitdefender.com> wrote:
>>> While working on this I found the following in the vm_event.h header:
>>>
>>> 168 struct vm_event_debug {
>>> 169     uint64_t gfn;
>>> 170     uint32_t _pad;
>>> 171 };
>>>
>>> Is this supposed to be 64 + 32 bits padding?
>> 
>> I think I need to direct the question back to you.
> 
> Fair enough, let me rephrase that: I don't see how that padding would be
> useful,

Neither do I.

> and I propose to remove _pad as part of the next version of this
> patch (or a different smaller patch).

Fine with me.

Jan