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

[PATCH V3] 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 CR0, CR3 and CR4 events are
now pre-write vm_events. 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>

---
Changes since V2:
 - The CR0, CR3 and CR4 events are now pre-write vm_events.
 - Struct monitor's .write_ctrlreg_enabled, .write_ctrlreg_sync
   and .write_ctrlreg_onchangeonly are now 4 bits wide instead
   of 8 (previously wider than necessary to reserve them for
   future events).
 - Consistent use of "curr" in hvm_handle_xsetbv().
---
 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            |   50 +++++++----------------------
 xen/arch/x86/hvm/hvm.c              |   16 ++++++----
 xen/arch/x86/hvm/vmx/vmx.c          |    4 +--
 xen/arch/x86/monitor.c              |   60 ++++++++++++-----------------------
 xen/include/asm-x86/domain.h        |   12 ++-----
 xen/include/asm-x86/hvm/event.h     |    4 +--
 xen/include/public/domctl.h         |   12 +++----
 xen/include/public/vm_event.h       |   27 +++++++++-------
 11 files changed, 105 insertions(+), 159 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..cff855a 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 short index, unsigned long value, unsigned long old)
 {
-    if ( onchangeonly && value == old )
+    struct arch_domain *currad = &current->domain->arch;
+
+    if ( !(currad->monitor.write_ctrlreg_enabled & index) )
+        return;
+
+    if ( (currad->monitor.write_ctrlreg_onchangeonly & index) && value == old )
         return;
     else
     {
         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..85c8822 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;
 
@@ -3201,6 +3204,8 @@ int hvm_set_cr0(unsigned long value)
         goto gpf;
     }
 
+    hvm_event_cr(VM_EVENT_X86_CR0, value, old_value);
+
     if ( (value & X86_CR0_PG) && !(old_value & X86_CR0_PG) )
     {
         if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
@@ -3267,7 +3272,6 @@ 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);
 
     if ( (value ^ old_value) & X86_CR0_PG ) {
         if ( !nestedhvm_vmswitch_in_progress(v) && nestedhvm_vcpu_in_guestmode(v) )
@@ -3287,7 +3291,9 @@ int hvm_set_cr3(unsigned long value)
 {
     struct vcpu *v = current;
     struct page_info *page;
-    unsigned long old;
+    unsigned long old = v->arch.hvm_vcpu.guest_cr[3];
+
+    hvm_event_cr(VM_EVENT_X86_CR3, value, old);
 
     if ( hvm_paging_enabled(v) && !paging_mode_hap(v->domain) &&
          (value != v->arch.hvm_vcpu.guest_cr[3]) )
@@ -3305,10 +3311,8 @@ int hvm_set_cr3(unsigned long value)
         HVM_DBG_LOG(DBG_LEVEL_VMMU, "Update CR3 value = %lx", 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);
     return X86EMUL_OKAY;
 
  bad_cr3:
@@ -3348,8 +3352,8 @@ int hvm_set_cr4(unsigned long value)
         goto gpf;
     }
 
+    hvm_event_cr(VM_EVENT_X86_CR4, value, old_cr);
     hvm_update_cr(v, 4, value);
-    hvm_event_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..9a7b924 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1262,7 +1262,7 @@ 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 & VM_EVENT_X86_CR3 )
                 v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
 
             vmx_update_cpu_exec_control(v);
@@ -2010,7 +2010,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..6fdacbd 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -67,59 +67,39 @@ 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;
+        bool_t status = ad->monitor.write_ctrlreg_enabled & mop->u.mov_to_cr.index;
         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 |= mop->u.mov_to_cr.index;
+        else
+            ad->monitor.write_ctrlreg_sync &= ~mop->u.mov_to_cr.index;
 
-        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 |= mop->u.mov_to_cr.index;
+        else
+            ad->monitor.write_ctrlreg_onchangeonly &= mop->u.mov_to_cr.index;
 
-        /* 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 |= mop->u.mov_to_cr.index;
+        else
+            ad->monitor.write_ctrlreg_enabled &= ~mop->u.mov_to_cr.index;
 
-        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..a3c117f 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -341,15 +341,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..7d327f4 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 short 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..54560b4 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     (1 << 0)
+#define VM_EVENT_X86_CR3     (1 << 1)
+#define VM_EVENT_X86_CR4     (1 << 2)
+#define VM_EVENT_X86_XCR0    (1 << 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 V3] xen/vm_event: Clean up control-register-write vm_events

[Razvan Cojocaru]

On 05/19/2015 11:31 AM, Razvan Cojocaru wrote:
> 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 CR0, CR3 and CR4 events are
> now pre-write vm_events. 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>
> 
> ---
> Changes since V2:
>  - The CR0, CR3 and CR4 events are now pre-write vm_events.
>  - Struct monitor's .write_ctrlreg_enabled, .write_ctrlreg_sync
>    and .write_ctrlreg_onchangeonly are now 4 bits wide instead
>    of 8 (previously wider than necessary to reserve them for
>    future events).
>  - Consistent use of "curr" in hvm_handle_xsetbv().

I've intentionally lost "Reviewed-by: Tim Deegan <tim@xen.org>" as I
thought moving the CR0, CR3 and CR3 events to be pre-write is a serious
enough change to warrant a new review. Hopefully it wasn't the wrong
thing to do.


Thanks,
Razvan

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

[Jan Beulich]

>>> On 19.05.15 at 10:31, <rcojocaru@bitdefender.com> wrote:
> The CR0, CR3 and CR4 events are
> now pre-write vm_events.

I didn't get the impression you and Tamas had already settled on
whether this is the way to go forward.

> --- 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 short index, unsigned long value, unsigned long old)

What's the point of using "unsigned short" here?

>  {
> -    if ( onchangeonly && value == old )
> +    struct arch_domain *currad = &current->domain->arch;
> +
> +    if ( !(currad->monitor.write_ctrlreg_enabled & index) )
> +        return;
> +
> +    if ( (currad->monitor.write_ctrlreg_onchangeonly & index) && value == old )
>          return;
>      else
>      {

Considering that nothing follows this else block, please invert the
condition and avoid both return and else.

> @@ -3201,6 +3204,8 @@ int hvm_set_cr0(unsigned long value)
>          goto gpf;
>      }
>  
> +    hvm_event_cr(VM_EVENT_X86_CR0, value, old_value);
> +
>      if ( (value & X86_CR0_PG) && !(old_value & X86_CR0_PG) )
>      {
>          if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )

If the monitor's response isn't being checked (i.e. "deny" not [yet]
being honored), what's the point of moving the generation of the
event in this patch (which would be involved enough without these
extra adjustments)?

> @@ -3287,7 +3291,9 @@ int hvm_set_cr3(unsigned long value)
>  {
>      struct vcpu *v = current;
>      struct page_info *page;
> -    unsigned long old;
> +    unsigned long old = v->arch.hvm_vcpu.guest_cr[3];
> +
> +    hvm_event_cr(VM_EVENT_X86_CR3, value, old);

I don't think the local variable is warranted anymore with the moved
event generation point.

> @@ -2010,7 +2010,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);

Why is this not becoming a pre event?

> +/* Supported values for the vm_event_write_ctrlreg index. */
> +#define VM_EVENT_X86_CR0     (1 << 0)
> +#define VM_EVENT_X86_CR3     (1 << 1)
> +#define VM_EVENT_X86_CR4     (1 << 2)
> +#define VM_EVENT_X86_XCR0    (1 << 3)

Why bit masks rather than an enumeration like thing?

> @@ -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;

In particular here - what meaning would it have if there was more
than one bit set?

Jan

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

[Razvan Cojocaru]

On 05/20/2015 05:53 PM, Jan Beulich wrote:
>>>> On 19.05.15 at 10:31, <rcojocaru@bitdefender.com> wrote:
>> The CR0, CR3 and CR4 events are
>> now pre-write vm_events.
> 
> I didn't get the impression you and Tamas had already settled on
> whether this is the way to go forward.

I took Tamas' previous comment ("IMHO there is not much point in having
two distinct event types (PRE/POST).") to mean that he agrees that if we
aim for consistency, we need to have the CR and MSR events behave the
same way.

Tamas did indeed question whether or not post-write (vs. pre-write) CR
events matter from the guest perspective, and I've explained why this
does matter to the monitoring application, from both an architectural
and efficiency perspective.

Of course, if there is more to discuss, by all means. We should all
agree on how to go forward. Tamas, did I misunderstand your position?

>> --- 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 short index, unsigned long value, unsigned long old)
> 
> What's the point of using "unsigned short" here?

As opposed to an uint8_t? This would allow the hvm_event_cr() signature
to remain unmodified longer if more than 4 control register events are
added in the future, and it is the least wasteful of the
bigger-than-char-sized integers.

>>  {
>> -    if ( onchangeonly && value == old )
>> +    struct arch_domain *currad = &current->domain->arch;
>> +
>> +    if ( !(currad->monitor.write_ctrlreg_enabled & index) )
>> +        return;
>> +
>> +    if ( (currad->monitor.write_ctrlreg_onchangeonly & index) && value == old )
>>          return;
>>      else
>>      {
> 
> Considering that nothing follows this else block, please invert the
> condition and avoid both return and else.

Ack.

>> @@ -3201,6 +3204,8 @@ int hvm_set_cr0(unsigned long value)
>>          goto gpf;
>>      }
>>  
>> +    hvm_event_cr(VM_EVENT_X86_CR0, value, old_value);
>> +
>>      if ( (value & X86_CR0_PG) && !(old_value & X86_CR0_PG) )
>>      {
>>          if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
> 
> If the monitor's response isn't being checked (i.e. "deny" not [yet]
> being honored), what's the point of moving the generation of the
> event in this patch (which would be involved enough without these
> extra adjustments)?

Consistency. Since the patch concerns itself with cleaning up the
control register events a bit, it didn't seem too far-fetched to assume
that having the control register write vm_events sent at the same place
as the MSR events fits the concept.

But I am happy to do this in the DENY patch in the next iteration of the
series if so requested.

>> @@ -3287,7 +3291,9 @@ int hvm_set_cr3(unsigned long value)
>>  {
>>      struct vcpu *v = current;
>>      struct page_info *page;
>> -    unsigned long old;
>> +    unsigned long old = v->arch.hvm_vcpu.guest_cr[3];
>> +
>> +    hvm_event_cr(VM_EVENT_X86_CR3, value, old);
> 
> I don't think the local variable is warranted anymore with the moved
> event generation point.

Ack.

>> @@ -2010,7 +2010,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);
> 
> Why is this not becoming a pre event?

It should, I've missed that. Thank you for noticing!
Will fix it.

>> +/* Supported values for the vm_event_write_ctrlreg index. */
>> +#define VM_EVENT_X86_CR0     (1 << 0)
>> +#define VM_EVENT_X86_CR3     (1 << 1)
>> +#define VM_EVENT_X86_CR4     (1 << 2)
>> +#define VM_EVENT_X86_XCR0    (1 << 3)
> 
> Why bit masks rather than an enumeration like thing?

Ack, will change it to an enum. That would have been my first preference
too, but the header just seemed to be more #define-oriented and I tried
to follow suit.

>> @@ -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;
> 
> In particular here - what meaning would it have if there was more
> than one bit set?

Ack, will change the constants to members of an enum.


Thanks,
Razvan

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

[Jan Beulich]

>>> On 20.05.15 at 17:24, <rcojocaru@bitdefender.com> wrote:
> On 05/20/2015 05:53 PM, Jan Beulich wrote:
>>>>> On 19.05.15 at 10:31, <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 short index, unsigned long value, unsigned long old)
>> 
>> What's the point of using "unsigned short" here?
> 
> As opposed to an uint8_t? This would allow the hvm_event_cr() signature
> to remain unmodified longer if more than 4 control register events are
> added in the future, and it is the least wasteful of the
> bigger-than-char-sized integers.

No, opposed to "unsigned int".

>>> @@ -3201,6 +3204,8 @@ int hvm_set_cr0(unsigned long value)
>>>          goto gpf;
>>>      }
>>>  
>>> +    hvm_event_cr(VM_EVENT_X86_CR0, value, old_value);
>>> +
>>>      if ( (value & X86_CR0_PG) && !(old_value & X86_CR0_PG) )
>>>      {
>>>          if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
>> 
>> If the monitor's response isn't being checked (i.e. "deny" not [yet]
>> being honored), what's the point of moving the generation of the
>> event in this patch (which would be involved enough without these
>> extra adjustments)?
> 
> Consistency. Since the patch concerns itself with cleaning up the
> control register events a bit, it didn't seem too far-fetched to assume
> that having the control register write vm_events sent at the same place
> as the MSR events fits the concept.
> 
> But I am happy to do this in the DENY patch in the next iteration of the
> series if so requested.

In small patches I don't mind multiple things to be done together.
But large patches should focus on one thing at a time.

>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>> +#define VM_EVENT_X86_CR0     (1 << 0)
>>> +#define VM_EVENT_X86_CR3     (1 << 1)
>>> +#define VM_EVENT_X86_CR4     (1 << 2)
>>> +#define VM_EVENT_X86_XCR0    (1 << 3)
>> 
>> Why bit masks rather than an enumeration like thing?
> 
> Ack, will change it to an enum. That would have been my first preference
> too, but the header just seemed to be more #define-oriented and I tried
> to follow suit.

And I didn't say use an enum, I intentionally said enum like.

Jan

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

[Tamas K Lengyel]

On Wed, May 20, 2015 at 5:24 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 05/20/2015 05:53 PM, Jan Beulich wrote:
>>>>> On 19.05.15 at 10:31, <rcojocaru@bitdefender.com> wrote:
>>> The CR0, CR3 and CR4 events are
>>> now pre-write vm_events.
>>
>> I didn't get the impression you and Tamas had already settled on
>> whether this is the way to go forward.
>
> I took Tamas' previous comment ("IMHO there is not much point in having
> two distinct event types (PRE/POST).") to mean that he agrees that if we
> aim for consistency, we need to have the CR and MSR events behave the
> same way.
>
> Tamas did indeed question whether or not post-write (vs. pre-write) CR
> events matter from the guest perspective, and I've explained why this
> does matter to the monitoring application, from both an architectural
> and efficiency perspective.
>
> Of course, if there is more to discuss, by all means. We should all
> agree on how to go forward. Tamas, did I misunderstand your position?

Ack, I see your point and it make sense to do it this way!

Cheers,
Tamas

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

[Razvan Cojocaru]

On 05/20/2015 06:48 PM, Jan Beulich wrote:
>>>> On 20.05.15 at 17:24, <rcojocaru@bitdefender.com> wrote:
>> On 05/20/2015 05:53 PM, Jan Beulich wrote:
>>>>>> On 19.05.15 at 10:31, <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 short index, unsigned long value, unsigned long old)
>>>
>>> What's the point of using "unsigned short" here?
>>
>> As opposed to an uint8_t? This would allow the hvm_event_cr() signature
>> to remain unmodified longer if more than 4 control register events are
>> added in the future, and it is the least wasteful of the
>> bigger-than-char-sized integers.
> 
> No, opposed to "unsigned int".

Ack, will change it to unsigned int.

>>>> @@ -3201,6 +3204,8 @@ int hvm_set_cr0(unsigned long value)
>>>>          goto gpf;
>>>>      }
>>>>  
>>>> +    hvm_event_cr(VM_EVENT_X86_CR0, value, old_value);
>>>> +
>>>>      if ( (value & X86_CR0_PG) && !(old_value & X86_CR0_PG) )
>>>>      {
>>>>          if ( v->arch.hvm_vcpu.guest_efer & EFER_LME )
>>>
>>> If the monitor's response isn't being checked (i.e. "deny" not [yet]
>>> being honored), what's the point of moving the generation of the
>>> event in this patch (which would be involved enough without these
>>> extra adjustments)?
>>
>> Consistency. Since the patch concerns itself with cleaning up the
>> control register events a bit, it didn't seem too far-fetched to assume
>> that having the control register write vm_events sent at the same place
>> as the MSR events fits the concept.
>>
>> But I am happy to do this in the DENY patch in the next iteration of the
>> series if so requested.
> 
> In small patches I don't mind multiple things to be done together.
> But large patches should focus on one thing at a time.

Understood, will withhold the reordering of control register vm_events
in V4.

>>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>>> +#define VM_EVENT_X86_CR0     (1 << 0)
>>>> +#define VM_EVENT_X86_CR3     (1 << 1)
>>>> +#define VM_EVENT_X86_CR4     (1 << 2)
>>>> +#define VM_EVENT_X86_XCR0    (1 << 3)
>>>
>>> Why bit masks rather than an enumeration like thing?
>>
>> Ack, will change it to an enum. That would have been my first preference
>> too, but the header just seemed to be more #define-oriented and I tried
>> to follow suit.
> 
> And I didn't say use an enum, I intentionally said enum like.

I see. They're bitmasks because it makes it easy to use them with the
X-bit wide (where X is the number of control register event indices)
flags write_ctrlreg_enabled, write_ctrlreg_sync and
write_ctrlreg_onchangeonly. It makes no difference for the .index field
of the actual event, you are right indeed that it would not make sense
for .index to consist of OR-ed bit masks.

I can change them to 0, 1, 2, and so on, and use individual single-bit
bitfields for write_ctrlreg_enabled & friends, but this way makes it
trivial to add a new control register vm_event: just use 1 << next
available position and widen the proper domain bitfields by one, and the
new vm_event is ready to use.


Thanks,
Razvan

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

[Jan Beulich]

>>> On 20.05.15 at 18:05, <rcojocaru@bitdefender.com> wrote:
> On 05/20/2015 06:48 PM, Jan Beulich wrote:
>>>>> On 20.05.15 at 17:24, <rcojocaru@bitdefender.com> wrote:
>>> On 05/20/2015 05:53 PM, Jan Beulich wrote:
>>>>>>> On 19.05.15 at 10:31, <rcojocaru@bitdefender.com> wrote:
>>>>> +/* Supported values for the vm_event_write_ctrlreg index. */
>>>>> +#define VM_EVENT_X86_CR0     (1 << 0)
>>>>> +#define VM_EVENT_X86_CR3     (1 << 1)
>>>>> +#define VM_EVENT_X86_CR4     (1 << 2)
>>>>> +#define VM_EVENT_X86_XCR0    (1 << 3)
>>>>
>>>> Why bit masks rather than an enumeration like thing?
>>>
>>> Ack, will change it to an enum. That would have been my first preference
>>> too, but the header just seemed to be more #define-oriented and I tried
>>> to follow suit.
>> 
>> And I didn't say use an enum, I intentionally said enum like.
> 
> I see. They're bitmasks because it makes it easy to use them with the
> X-bit wide (where X is the number of control register event indices)
> flags write_ctrlreg_enabled, write_ctrlreg_sync and
> write_ctrlreg_onchangeonly. It makes no difference for the .index field
> of the actual event, you are right indeed that it would not make sense
> for .index to consist of OR-ed bit masks.
> 
> I can change them to 0, 1, 2, and so on, and use individual single-bit
> bitfields for write_ctrlreg_enabled & friends, but this way makes it
> trivial to add a new control register vm_event: just use 1 << next
> available position and widen the proper domain bitfields by one, and the
> new vm_event is ready to use.

And I don't mind that - have an index where an index is needed,
and use (1 << index) when you want a bit mask.

Jan