[PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[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).

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>

---
Changes since V8:
 - Removed the additional CR3 test code from xen-access.c.
---
 tools/libxc/include/xenctrl.h   |    9 ++----
 tools/libxc/xc_monitor.c        |   40 ++++---------------------
 xen/arch/x86/hvm/event.c        |   55 ++++++++++------------------------
 xen/arch/x86/hvm/hvm.c          |   11 ++++---
 xen/arch/x86/hvm/vmx/vmx.c      |    6 ++--
 xen/arch/x86/monitor.c          |   63 ++++++++++++++-------------------------
 xen/include/asm-x86/domain.h    |   12 ++------
 xen/include/asm-x86/hvm/event.h |    5 ++--
 xen/include/asm-x86/monitor.h   |    2 ++
 xen/include/public/domctl.h     |   12 ++++----
 xen/include/public/vm_event.h   |   29 ++++++++++--------
 11 files changed, 86 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/xen/arch/x86/hvm/event.c b/xen/arch/x86/hvm/event.c
index 9d5f9f3..fedf617 100644
--- a/xen/arch/x86/hvm/event.c
+++ b/xen/arch/x86/hvm/event.c
@@ -21,6 +21,8 @@
 
 #include <xen/vm_event.h>
 #include <xen/paging.h>
+#include <asm/hvm/event.h>
+#include <asm/monitor.h>
 #include <public/vm_event.h>
 
 static void hvm_event_fill_regs(vm_event_request_t *req)
@@ -88,55 +90,28 @@ 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 )
-        return;
-    else
+    struct arch_domain *currad = &current->domain->arch;
+    unsigned int ctrlreg_bitmask = monitor_ctrlreg_bitmask(index);
+
+    if ( (currad->monitor.write_ctrlreg_enabled & ctrlreg_bitmask) &&
+         (!(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 & ctrlreg_bitmask,
+                        &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..46343a8 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(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(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(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(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..82e04f6 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -57,6 +57,7 @@
 #include <asm/apic.h>
 #include <asm/hvm/nestedhvm.h>
 #include <asm/event.h>
+#include <asm/monitor.h>
 #include <public/arch-x86/cpuid.h>
 
 static bool_t __initdata opt_force_ept;
@@ -1262,7 +1263,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 +2012,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(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..896acf7 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -67,59 +67,42 @@ 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..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..5a75e05 100644
--- a/xen/include/asm-x86/hvm/event.h
+++ b/xen/include/asm-x86/hvm/event.h
@@ -19,9 +19,8 @@
 #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);
+#define hvm_event_cr(what, new, old) hvm_event_cr(VM_EVENT_X86_##what, new, 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/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 21b3e5b..d5815db 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -26,6 +26,8 @@
 struct domain;
 struct xen_domctl_monitor_op;
 
+#define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
+
 int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *op);
 
 #endif /* __ASM_X86_MONITOR_H__ */
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..577e971 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,14 +158,15 @@ struct vm_event_mem_access {
     uint32_t _pad;
 };
 
-struct vm_event_mov_to_cr {
+struct vm_event_write_ctrlreg {
+    uint32_t index;
+    uint32_t _pad;
     uint64_t new_value;
     uint64_t old_value;
 };
 
 struct vm_event_debug {
     uint64_t gfn;
-    uint32_t _pad;
 };
 
 struct vm_event_mov_to_msr {
@@ -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 V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Ian Campbell]

On Wed, 2015-06-03 at 14:35 +0300, 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).
> 
> Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> Acked-by: Kevin Tian <kevin.tian@intel.com>
> 
> ---
> Changes since V8:
>  - Removed the additional CR3 test code from xen-access.c.

=> Acked-by: Ian Campbell <ian.campbell@citrix.com> (for both ARM and
tools)

Re: [PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 1444 bytes --]

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

Just looking at this here again, we will now have a bitmap within a bitmap,
which doesn't seem to be very efficient. IMHO it would be better to just
take the ctrlreg bitmap out as a separate uint8_t within struct {} monitor.


>          uint16_t mov_to_msr_enabled          : 1;
>          uint16_t mov_to_msr_extended         : 1;
>          uint16_t singlestep_enabled          : 1;
>

-- 

[image: www.novetta.com]

Tamas K Lengyel

Senior Security Researcher

7921 Jones Branch Drive

McLean VA 22102

Email  tlengyel@novetta.com

[-- Attachment #1.2: Type: text/html, Size: 4022 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Razvan Cojocaru]

On 06/03/2015 04:29 PM, Lengyel, Tamas wrote:
> 
> 
> 
>     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;
> 
> 
> Just looking at this here again, we will now have a bitmap within a
> bitmap, which doesn't seem to be very efficient. IMHO it would be better
> to just take the ctrlreg bitmap out as a separate uint8_t within struct
> {} monitor.

How is it inefficient? I don't see that at all. And I'm not sure what
you mean about the uint8_t: there are 3 fields there, each 4-bits wide
(write_ctrlreg_enabled, write_ctrlreg_sync, write_ctrlreg_onchangeonly)
and only (at most) two of them would fit into a uint8_t. To put them all
into a new struct would mean wasting an uint16_t for 12-bits.

You might argue that it's not as clean-cut as having one explicitly
specified bit for each control register (like mov_to_cr0_enabled,
mov_to_cr3_enabled, etc. were before), but that's intentional, as it
makes it trivial to add a new control register write event: just go
ahead and "#define VM_EVENT_X86_XCR1  4", for example, and then all you
need to do is make sure that write_ctrlreg_enabled is wide enough to
hold 5 events, and that's it, you can use the new event in the HV and libxc.

Having explicit 1-bit fields for each new event (which I think is what
you're proposing) makes it much more work to add a new ctrlreg event,
and it defeats the purpose of much of the conversations I've had with
Jan about all those shifting and convenience macros.


Thanks,
Razvan

Re: [PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 4010 bytes --]

On Wed, Jun 3, 2015 at 3:48 PM, Razvan Cojocaru <rcojocaru@bitdefender.com>
wrote:

> On 06/03/2015 04:29 PM, Lengyel, Tamas wrote:
> >
> >
> >
> >     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;
> >
> >
> > Just looking at this here again, we will now have a bitmap within a
> > bitmap, which doesn't seem to be very efficient. IMHO it would be better
> > to just take the ctrlreg bitmap out as a separate uint8_t within struct
> > {} monitor.
>
> How is it inefficient? I don't see that at all. And I'm not sure what
> you mean about the uint8_t: there are 3 fields there, each 4-bits wide
> (write_ctrlreg_enabled, write_ctrlreg_sync, write_ctrlreg_onchangeonly)
> and only (at most) two of them would fit into a uint8_t. To put them all
> into a new struct would mean wasting an uint16_t for 12-bits.
>

Right now if you want to access a bit using the index on the ctrlreg
fields, you would for example do (monitor.write_ctrlreg_enabled & index).
This is actually going to perform two bitmask operations. First,
monitor.write_ctrlreg_enabled
does a masking operating to get you only the 4 bits corresponding to this
field, then you do another mask with the index. In general bitmasks are
pretty slow operations, thus IMHO minimizing them would be ideal. What I
meant by doing a uint8_t separately would be like "uint8_t
write_ctrlreg_enabled;" which would eliminate the first bitmask operation.
You would have to do that for the other two bitfields as well,
write_ctrlreg_sync
and write_ctrlreg_onchangeonly. The trade-off with this is that now we will
have 12 bits that are not used for anything (yet). On the other hand this
way we could also reduce the size of the remaining bitfield from uint16_t
to uint8_t, so IMHO it's not that big of an issue.


>
> You might argue that it's not as clean-cut as having one explicitly
> specified bit for each control register (like mov_to_cr0_enabled,
> mov_to_cr3_enabled, etc. were before), but that's intentional, as it
> makes it trivial to add a new control register write event: just go
> ahead and "#define VM_EVENT_X86_XCR1  4", for example, and then all you
> need to do is make sure that write_ctrlreg_enabled is wide enough to
> hold 5 events, and that's it, you can use the new event in the HV and
> libxc.
>

> Having explicit 1-bit fields for each new event (which I think is what
> you're proposing) makes it much more work to add a new ctrlreg event,
> and it defeats the purpose of much of the conversations I've had with
> Jan about all those shifting and convenience macros.
>

I'm fine with having the index-based bitfield approach, don't get me wrong.
This is about how those bitfields are stored.


>
>
> Thanks,
> Razvan
>

Sorry for the last minute addition here, I know you want to close this
patch now that you have the acks on it. If the maintainers don't share my
concern here then feel free to ignore it ;)

Cheers!

-- 

[image: www.novetta.com]

Tamas K Lengyel

Senior Security Researcher

7921 Jones Branch Drive

McLean VA 22102

Email  tlengyel@novetta.com

[-- Attachment #1.2: Type: text/html, Size: 7541 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Razvan Cojocaru]

On 06/03/2015 05:19 PM, Lengyel, Tamas wrote:
> 
> 
> On Wed, Jun 3, 2015 at 3:48 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com <mailto:rcojocaru@bitdefender.com>> wrote:
> 
>     On 06/03/2015 04:29 PM, Lengyel, Tamas wrote:
>     >
>     >
>     >
>     >     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;
>     >
>     >
>     > Just looking at this here again, we will now have a bitmap within a
>     > bitmap, which doesn't seem to be very efficient. IMHO it would be better
>     > to just take the ctrlreg bitmap out as a separate uint8_t within struct
>     > {} monitor.
> 
>     How is it inefficient? I don't see that at all. And I'm not sure what
>     you mean about the uint8_t: there are 3 fields there, each 4-bits wide
>     (write_ctrlreg_enabled, write_ctrlreg_sync, write_ctrlreg_onchangeonly)
>     and only (at most) two of them would fit into a uint8_t. To put them all
>     into a new struct would mean wasting an uint16_t for 12-bits.
> 
> 
> Right now if you want to access a bit using the index on the ctrlreg
> fields, you would for example do (monitor.write_ctrlreg_enabled &
> index). This is actually going to perform two bitmask operations. First,
> monitor.write_ctrlreg_enabled does a masking operating to get you only
> the 4 bits corresponding to this field, then you do another mask with
> the index. In general bitmasks are pretty slow operations, thus IMHO
> minimizing them would be ideal. What I meant by doing a uint8_t
> separately would be like "uint8_t write_ctrlreg_enabled;" which would
> eliminate the first bitmask operation. You would have to do that for the
> other two bitfields as well, write_ctrlreg_sync and
> write_ctrlreg_onchangeonly. The trade-off with this is that now we will
> have 12 bits that are not used for anything (yet). On the other hand
> this way we could also reduce the size of the remaining bitfield from
> uint16_t to uint8_t, so IMHO it's not that big of an issue.

Right, I've assumed that you weren't talking about that, since the
initial patch used more bits than strictly necessary for storing the
current number of events, and you preferred the patch to use the least
possible ammount of bits to store the info, but using dedicated uint8_ts
is indeed a slightly different approach.

> Sorry for the last minute addition here, I know you want to close this
> patch now that you have the acks on it. If the maintainers don't share
> my concern here then feel free to ignore it ;)

No problem, I'd rather have it done right than quick, so if extra
shifting to get to those fields trumps the space economy bitfields
provide, I'll send a new version of the patch. It it after all not that
great of a modification.

Waiting for maintainers' / other input.


Thanks,
Razvan

Re: [PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Tim Deegan]

At 16:19 +0200 on 03 Jun (1433348379), Lengyel, Tamas wrote:
> On Wed, Jun 3, 2015 at 3:48 PM, Razvan Cojocaru <rcojocaru@bitdefender.com>
> wrote:
> > On 06/03/2015 04:29 PM, Lengyel, Tamas wrote:
> > >          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;
> > >
> > >
> > > Just looking at this here again, we will now have a bitmap within a
> > > bitmap, which doesn't seem to be very efficient. IMHO it would be better
> > > to just take the ctrlreg bitmap out as a separate uint8_t within struct
> > > {} monitor.
> >
> > How is it inefficient? I don't see that at all. And I'm not sure what
> > you mean about the uint8_t: there are 3 fields there, each 4-bits wide
> > (write_ctrlreg_enabled, write_ctrlreg_sync, write_ctrlreg_onchangeonly)
> > and only (at most) two of them would fit into a uint8_t. To put them all
> > into a new struct would mean wasting an uint16_t for 12-bits.
> 
> Right now if you want to access a bit using the index on the ctrlreg
> fields, you would for example do (monitor.write_ctrlreg_enabled & index).
> This is actually going to perform two bitmask operations. First,
> monitor.write_ctrlreg_enabled
> does a masking operating to get you only the 4 bits corresponding to this
> field, then you do another mask with the index.

The compiler can optimize away the first mask operation.  E.g., gcc
folds that into a single operation at anything above -O0.

Tim.

Re: [PATCH V9] xen/vm_event: Clean up control-register-write vm_events and add XCR0 event

[Lengyel, Tamas]

[-- Attachment #1.1: Type: text/plain, Size: 2447 bytes --]

On Thu, Jun 4, 2015 at 3:53 PM, Tim Deegan <tim@xen.org> wrote:

> At 16:19 +0200 on 03 Jun (1433348379), Lengyel, Tamas wrote:
> > On Wed, Jun 3, 2015 at 3:48 PM, Razvan Cojocaru <
> rcojocaru@bitdefender.com>
> > wrote:
> > > On 06/03/2015 04:29 PM, Lengyel, Tamas wrote:
> > > >          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;
> > > >
> > > >
> > > > Just looking at this here again, we will now have a bitmap within a
> > > > bitmap, which doesn't seem to be very efficient. IMHO it would be
> better
> > > > to just take the ctrlreg bitmap out as a separate uint8_t within
> struct
> > > > {} monitor.
> > >
> > > How is it inefficient? I don't see that at all. And I'm not sure what
> > > you mean about the uint8_t: there are 3 fields there, each 4-bits wide
> > > (write_ctrlreg_enabled, write_ctrlreg_sync, write_ctrlreg_onchangeonly)
> > > and only (at most) two of them would fit into a uint8_t. To put them
> all
> > > into a new struct would mean wasting an uint16_t for 12-bits.
> >
> > Right now if you want to access a bit using the index on the ctrlreg
> > fields, you would for example do (monitor.write_ctrlreg_enabled & index).
> > This is actually going to perform two bitmask operations. First,
> > monitor.write_ctrlreg_enabled
> > does a masking operating to get you only the 4 bits corresponding to this
> > field, then you do another mask with the index.
>
> The compiler can optimize away the first mask operation.  E.g., gcc
> folds that into a single operation at anything above -O0.
>
> Tim.
>

Good to know, thanks Tim! Sorry for the noise ;)

-- 

[image: www.novetta.com]

Tamas K Lengyel

Senior Security Researcher

7921 Jones Branch Drive

McLean VA 22102

Email  tlengyel@novetta.com

[-- Attachment #1.2: Type: text/html, Size: 5484 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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