[PATCH v2 0/7] x86/vm-event: Adjustments & fixes

[PATCH v2 0/7] x86/vm-event: Adjustments & fixes

[Corneliu ZUZU]

This patch-series makes some adjustments and fixes to the X86 vm-events code.
Summary:
    1. minor optimization
    2. relocate some code into added vm-event functions
        Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
    3. fix monitor_write_data behavior on domain cleanup
        Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
    4. surround VM_EVENT_REASON_MOV_TO_MSR w/ #ifdef CONFIG_X86
        Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
    5+6+7. minor fixes (cleanup stuff)

Significant changes since v1:
  0/7: removed patch 4/8 of v1 series (restored monitor_write_data.do_write)
  2/7: separated vmx-specific code motion in vmx_vm_event_update_cr3_traps
  3/7: arch_vcpu.vm_event made pointer again to avoid eating memory from
       arch_vcpu structure

Corneliu ZUZU (7):
  x86/vmx_update_guest_cr: minor optimization
  x86/vm-event/monitor: relocate code-motion more appropriately
  x86/vm-event/monitor: don't compromise monitor_write_data on domain
    cleanup
  x86/vm_event_resume: surround VM_EVENT_REASON_MOV_TO_MSR w/ CONFIG_X86
  x86/vm-event: minor ASSERT fix, add 'unlikely'
  minor fixes (formatting, comments, unused includes etc.)
  minor #include change

 xen/arch/arm/domain.c             |  1 -
 xen/arch/arm/traps.c              |  1 -
 xen/arch/x86/domain.c             |  9 ++++--
 xen/arch/x86/hvm/emulate.c        |  6 ++--
 xen/arch/x86/hvm/hvm.c            | 53 ++++++++++++--------------------
 xen/arch/x86/hvm/monitor.c        |  1 +
 xen/arch/x86/hvm/vmx/vmx.c        | 64 ++++++++++++++++++++++++++++++++++-----
 xen/arch/x86/mm/p2m.c             |  2 +-
 xen/arch/x86/monitor.c            | 47 +++++++++++++++++++++++-----
 xen/arch/x86/vm_event.c           | 31 ++++++++++++-------
 xen/common/monitor.c              |  1 -
 xen/common/vm_event.c             | 19 ++++++++++--
 xen/include/asm-arm/vm_event.h    |  9 ++----
 xen/include/asm-x86/hvm/monitor.h |  2 --
 xen/include/asm-x86/hvm/vmx/vmx.h |  1 +
 xen/include/asm-x86/monitor.h     |  9 +++---
 xen/include/asm-x86/vm_event.h    |  3 +-
 xen/include/public/vm_event.h     | 38 +++++++++++------------
 xen/include/xen/vm_event.h        |  1 -
 19 files changed, 195 insertions(+), 103 deletions(-)

-- 
2.5.0


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

[PATCH v2 1/7] x86/vmx_update_guest_cr: minor optimization

[Corneliu ZUZU]

Minor optimization @ vmx_update_guest_cr: checks if v->arch.hvm_vmx.exec_control
was modified before actually calling vmx_update_cpu_exec_control(v).

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
Changed since v1: <nothing>
---
 xen/arch/x86/hvm/vmx/vmx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index df19579..8ab074f 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1434,8 +1434,10 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
         if ( paging_mode_hap(v->domain) )
         {
             /* Manage GUEST_CR3 when CR0.PE=0. */
+            uint32_t old_ctls = v->arch.hvm_vmx.exec_control;
             uint32_t cr3_ctls = (CPU_BASED_CR3_LOAD_EXITING |
                                  CPU_BASED_CR3_STORE_EXITING);
+
             v->arch.hvm_vmx.exec_control &= ~cr3_ctls;
             if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
                 v->arch.hvm_vmx.exec_control |= cr3_ctls;
@@ -1445,7 +1447,8 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
                  monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
                 v->arch.hvm_vmx.exec_control |= CPU_BASED_CR3_LOAD_EXITING;
 
-            vmx_update_cpu_exec_control(v);
+            if ( old_ctls != v->arch.hvm_vmx.exec_control )
+                vmx_update_cpu_exec_control(v);
         }
 
         if ( !nestedhvm_vcpu_in_guestmode(v) )
-- 
2.5.0


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

[PATCH v2 2/7] x86/vm-event/monitor: relocate code-motion more appropriately

[Corneliu ZUZU]

For readability:

* Add function arch_monitor_write_data (in x86/monitor.c) and separate handling
of monitor_write_data there (previously done directly in hvm_do_resume).

* Separate enabling/disabling of CPU_BASED_CR3_LOAD_EXITING for CR3 monitor
vm-events from CR0 node @ vmx_update_guest_cr(v, 0) into newly added function
vmx_vm_event_update_cr3_traps(d); also, for a better interface, create generic
function write_ctrlreg_adjust_traps (in x86/monitor.c) which deals with setting
up any traps for cr-write monitor vm-events.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Changed since v1:
  * separated vmx-specific code motion in vmx_vm_event_update_cr3_traps
  * minor: reorder added include alphabetically
  * minor: move arch_monitor_write_data call in if @ hvm_do_resume
---
 xen/arch/x86/hvm/hvm.c            | 46 ++++++++++++-------------------
 xen/arch/x86/hvm/vmx/vmx.c        | 58 +++++++++++++++++++++++++++++++++++----
 xen/arch/x86/monitor.c            | 48 +++++++++++++++++++++++++++-----
 xen/include/asm-x86/hvm/vmx/vmx.h |  1 +
 xen/include/asm-x86/monitor.h     |  2 ++
 5 files changed, 114 insertions(+), 41 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c89ab6e..e3829d2 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -475,8 +475,6 @@ void hvm_do_resume(struct vcpu *v)
 
     if ( unlikely(v->arch.vm_event) )
     {
-        struct monitor_write_data *w = &v->arch.vm_event->write_data;
-
         if ( v->arch.vm_event->emulate_flags )
         {
             enum emul_kind kind = EMUL_KIND_NORMAL;
@@ -494,29 +492,7 @@ void hvm_do_resume(struct vcpu *v)
             v->arch.vm_event->emulate_flags = 0;
         }
 
-        if ( w->do_write.msr )
-        {
-            hvm_msr_write_intercept(w->msr, w->value, 0);
-            w->do_write.msr = 0;
-        }
-
-        if ( w->do_write.cr0 )
-        {
-            hvm_set_cr0(w->cr0, 0);
-            w->do_write.cr0 = 0;
-        }
-
-        if ( w->do_write.cr4 )
-        {
-            hvm_set_cr4(w->cr4, 0);
-            w->do_write.cr4 = 0;
-        }
-
-        if ( w->do_write.cr3 )
-        {
-            hvm_set_cr3(w->cr3, 0);
-            w->do_write.cr3 = 0;
-        }
+        arch_monitor_write_data(v);
     }
 
     /* Inject pending hw/sw trap */
@@ -2206,7 +2182,10 @@ int hvm_set_cr0(unsigned long value, bool_t may_defer)
 
         if ( hvm_monitor_crX(CR0, value, old_value) )
         {
-            /* The actual write will occur in hvm_do_resume(), if permitted. */
+            /*
+             * The actual write will occur in arch_monitor_write_data(), if
+             * permitted.
+             */
             v->arch.vm_event->write_data.do_write.cr0 = 1;
             v->arch.vm_event->write_data.cr0 = value;
 
@@ -2308,7 +2287,10 @@ int hvm_set_cr3(unsigned long value, bool_t may_defer)
 
         if ( hvm_monitor_crX(CR3, value, old) )
         {
-            /* The actual write will occur in hvm_do_resume(), if permitted. */
+            /*
+             * The actual write will occur in arch_monitor_write_data(), if
+             * permitted.
+             */
             v->arch.vm_event->write_data.do_write.cr3 = 1;
             v->arch.vm_event->write_data.cr3 = value;
 
@@ -2388,7 +2370,10 @@ int hvm_set_cr4(unsigned long value, bool_t may_defer)
 
         if ( hvm_monitor_crX(CR4, value, old_cr) )
         {
-            /* The actual write will occur in hvm_do_resume(), if permitted. */
+            /*
+             * The actual write will occur in arch_monitor_write_data(), if
+             * permitted.
+             */
             v->arch.vm_event->write_data.do_write.cr4 = 1;
             v->arch.vm_event->write_data.cr4 = value;
 
@@ -3767,7 +3752,10 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content,
     {
         ASSERT(v->arch.vm_event);
 
-        /* The actual write will occur in hvm_do_resume() (if permitted). */
+        /*
+         * The actual write will occur in arch_monitor_write_data(), if
+         * permitted.
+         */
         v->arch.vm_event->write_data.do_write.msr = 1;
         v->arch.vm_event->write_data.msr = msr;
         v->arch.vm_event->write_data.value = msr_content;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 8ab074f..0fa3fea 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1442,11 +1442,6 @@ static void vmx_update_guest_cr(struct vcpu *v, unsigned int cr)
             if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
                 v->arch.hvm_vmx.exec_control |= cr3_ctls;
 
-            /* Trap CR3 updates if CR3 memory events are 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;
-
             if ( old_ctls != v->arch.hvm_vmx.exec_control )
                 vmx_update_cpu_exec_control(v);
         }
@@ -3899,6 +3894,59 @@ void vmx_vmenter_helper(const struct cpu_user_regs *regs)
 }
 
 /*
+ * Adjusts domain CR3 load-exiting execution control for CR3-write monitor
+ * vm-event.
+ */
+void vmx_vm_event_update_cr3_traps(struct domain *d)
+{
+    struct vcpu *v;
+    struct arch_vmx_struct *avmx;
+    unsigned int cr3_bitmask;
+    bool_t cr3_vmevent, cr3_ldexit;
+
+    /* domain must be paused */
+    ASSERT(atomic_read(&d->pause_count));
+
+    /* non-hap domains trap CR3 writes unconditionally */
+    if ( !paging_mode_hap(d) )
+    {
+#ifndef NDEBUG
+        for_each_vcpu ( d, v )
+            ASSERT(v->arch.hvm_vmx.exec_control & CPU_BASED_CR3_LOAD_EXITING);
+#endif
+        return;
+    }
+
+    cr3_bitmask = monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3);
+    cr3_vmevent = !!(d->arch.monitor.write_ctrlreg_enabled & cr3_bitmask);
+
+    for_each_vcpu ( d, v )
+    {
+        avmx = &v->arch.hvm_vmx;
+        cr3_ldexit = !!(avmx->exec_control & CPU_BASED_CR3_LOAD_EXITING);
+
+        if ( cr3_vmevent == cr3_ldexit )
+            continue;
+
+        /*
+         * If CR0.PE=0, CR3 load exiting must remain enabled.
+         * See vmx_update_guest_cr code motion for cr = 0.
+         */
+        if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
+            continue;
+
+        if ( cr3_vmevent )
+            avmx->exec_control |= CPU_BASED_CR3_LOAD_EXITING;
+        else
+            avmx->exec_control &= ~CPU_BASED_CR3_LOAD_EXITING;
+
+        vmx_vmcs_enter(v);
+        vmx_update_cpu_exec_control(v);
+        vmx_vmcs_exit(v);
+    }
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 205df41..2bb7d86 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -19,7 +19,9 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <asm/hvm/vmx/vmx.h>
 #include <asm/monitor.h>
+#include <asm/vm_event.h>
 #include <public/vm_event.h>
 
 int arch_monitor_init_domain(struct domain *d)
@@ -41,6 +43,35 @@ void arch_monitor_cleanup_domain(struct domain *d)
     memset(&d->monitor, 0, sizeof(d->monitor));
 }
 
+void arch_monitor_write_data(struct vcpu *v)
+{
+    struct monitor_write_data *w = &v->arch.vm_event->write_data;
+
+    if ( w->do_write.msr )
+    {
+        hvm_msr_write_intercept(w->msr, w->value, 0);
+        w->do_write.msr = 0;
+    }
+
+    if ( w->do_write.cr0 )
+    {
+        hvm_set_cr0(w->cr0, 0);
+        w->do_write.cr0 = 0;
+    }
+
+    if ( w->do_write.cr4 )
+    {
+        hvm_set_cr4(w->cr4, 0);
+        w->do_write.cr4 = 0;
+    }
+
+    if ( w->do_write.cr3 )
+    {
+        hvm_set_cr3(w->cr3, 0);
+        w->do_write.cr3 = 0;
+    }
+}
+
 static unsigned long *monitor_bitmap_for_msr(const struct domain *d, u32 *msr)
 {
     ASSERT(d->arch.monitor.msr_bitmap && msr);
@@ -119,6 +150,15 @@ bool_t monitored_msr(const struct domain *d, u32 msr)
     return test_bit(msr, bitmap);
 }
 
+static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index)
+{
+    /* vmx only */
+    ASSERT(cpu_has_vmx);
+
+    if ( VM_EVENT_X86_CR3 == index )
+        vmx_vm_event_update_cr3_traps(d);
+}
+
 int arch_monitor_domctl_event(struct domain *d,
                               struct xen_domctl_monitor_op *mop)
 {
@@ -159,13 +199,7 @@ int arch_monitor_domctl_event(struct domain *d,
         else
             ad->monitor.write_ctrlreg_enabled &= ~ctrlreg_bitmask;
 
-        if ( VM_EVENT_X86_CR3 == mop->u.mov_to_cr.index )
-        {
-            struct vcpu *v;
-            /* Latches new CR3 mask through CR0 code. */
-            for_each_vcpu ( d, v )
-                hvm_update_guest_cr(v, 0);
-        }
+        write_ctrlreg_adjust_traps(d, mop->u.mov_to_cr.index);
 
         domain_unpause(d);
 
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 359b2a9..15bb592 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -101,6 +101,7 @@ void vmx_update_debug_state(struct vcpu *v);
 void vmx_update_exception_bitmap(struct vcpu *v);
 void vmx_update_cpu_exec_control(struct vcpu *v);
 void vmx_update_secondary_exec_control(struct vcpu *v);
+void vmx_vm_event_update_cr3_traps(struct domain *d);
 
 #define POSTED_INTR_ON  0
 #define POSTED_INTR_SN  1
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index a9db3c0..0611681 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -94,6 +94,8 @@ int arch_monitor_init_domain(struct domain *d);
 
 void arch_monitor_cleanup_domain(struct domain *d);
 
+void arch_monitor_write_data(struct vcpu *v);
+
 bool_t monitored_msr(const struct domain *d, u32 msr);
 
 #endif /* __ASM_X86_MONITOR_H__ */
-- 
2.5.0


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

[PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

[Corneliu ZUZU]

The arch_vm_event structure is dynamically allocated and freed @
vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack user
disables domain monitoring (xc_monitor_disable), which in turn effectively
discards any information that was in arch_vm_event.write_data.

But this can yield unexpected behavior since if a CR-write was awaiting to be
committed on the scheduling tail (hvm_do_resume->arch_monitor_write_data)
before xc_monitor_disable is called, then the domain CR write is wrongfully
ignored, which of course, in these cases, can easily render a domain crash.

To fix the issue, this patch makes arch_vm_event.emul_read_data dynamically
allocated and only frees that in vm_event_cleanup_domain, instead of the whole
arch_vcpu.vm_event structure, which with this patch will only be freed on
vcpu/domain destroyal.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Changed since v1:
  * arch_vcpu.vm_event made pointer again to avoid eating memory from arch_vcpu
    structure
---
 xen/arch/x86/domain.c          |  9 +++++++--
 xen/arch/x86/hvm/emulate.c     |  6 +++---
 xen/arch/x86/hvm/hvm.c         |  2 ++
 xen/arch/x86/mm/p2m.c          |  2 +-
 xen/arch/x86/vm_event.c        | 22 ++++++++++++++++------
 xen/common/vm_event.c          | 11 +++++++++++
 xen/include/asm-x86/monitor.h  |  4 +++-
 xen/include/asm-x86/vm_event.h |  2 +-
 8 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index bb59247..0313208 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -56,6 +56,7 @@
 #include <asm/debugreg.h>
 #include <asm/msr.h>
 #include <asm/traps.h>
+#include <asm/vm_event.h>
 #include <asm/nmi.h>
 #include <asm/mce.h>
 #include <asm/amd.h>
@@ -492,8 +493,12 @@ int vcpu_initialise(struct vcpu *v)
 
 void vcpu_destroy(struct vcpu *v)
 {
-    xfree(v->arch.vm_event);
-    v->arch.vm_event = NULL;
+    if ( unlikely(v->arch.vm_event) )
+    {
+        xfree(v->arch.vm_event->emul_read_data);
+        xfree(v->arch.vm_event);
+        v->arch.vm_event = NULL;
+    }
 
     if ( is_pv_32bit_vcpu(v) )
     {
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 855af4d..3880df1 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -73,12 +73,12 @@ static int set_context_data(void *buffer, unsigned int size)
 {
     struct vcpu *curr = current;
 
-    if ( curr->arch.vm_event )
+    if ( curr->arch.vm_event && curr->arch.vm_event->emul_read_data )
     {
         unsigned int safe_size =
-            min(size, curr->arch.vm_event->emul_read_data.size);
+            min(size, curr->arch.vm_event->emul_read_data->size);
 
-        memcpy(buffer, curr->arch.vm_event->emul_read_data.data, safe_size);
+        memcpy(buffer, curr->arch.vm_event->emul_read_data->data, safe_size);
         memset(buffer + safe_size, 0, size - safe_size);
         return X86EMUL_OKAY;
     }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index e3829d2..ac6d9eb 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -479,6 +479,8 @@ void hvm_do_resume(struct vcpu *v)
         {
             enum emul_kind kind = EMUL_KIND_NORMAL;
 
+            ASSERT(v->arch.vm_event->emul_read_data);
+
             if ( v->arch.vm_event->emulate_flags &
                  VM_EVENT_FLAG_SET_EMUL_READ_DATA )
                 kind = EMUL_KIND_SET_CONTEXT;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 16733a4..6616626 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1642,7 +1642,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
         v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
 
         if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
-            v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
+            *v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
     }
 }
 
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 80f84d6..0e25e4d 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -30,12 +30,18 @@ int vm_event_init_domain(struct domain *d)
 
     for_each_vcpu ( d, v )
     {
-        if ( v->arch.vm_event )
+        if ( likely(!v->arch.vm_event) )
+        {
+            v->arch.vm_event = xzalloc(struct arch_vm_event);
+            if ( !v->arch.vm_event )
+                return -ENOMEM;
+        }
+        else if ( unlikely(v->arch.vm_event->emul_read_data) )
             continue;
 
-        v->arch.vm_event = xzalloc(struct arch_vm_event);
-
-        if ( !v->arch.vm_event )
+        v->arch.vm_event->emul_read_data =
+                xzalloc(struct vm_event_emul_read_data);
+        if ( !v->arch.vm_event->emul_read_data )
             return -ENOMEM;
     }
 
@@ -52,8 +58,12 @@ void vm_event_cleanup_domain(struct domain *d)
 
     for_each_vcpu ( d, v )
     {
-        xfree(v->arch.vm_event);
-        v->arch.vm_event = NULL;
+        if ( likely(!v->arch.vm_event) )
+            continue;
+
+        v->arch.vm_event->emulate_flags = 0;
+        xfree(v->arch.vm_event->emul_read_data);
+        v->arch.vm_event->emul_read_data = NULL;
     }
 
     d->arch.mem_access_emulate_each_rep = 0;
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 17d2716..75bbbab 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v, unsigned int port)
 /* Clean up on domain destruction */
 void vm_event_cleanup(struct domain *d)
 {
+    struct vcpu *v;
+
 #ifdef CONFIG_HAS_MEM_PAGING
     if ( d->vm_event->paging.ring_page )
     {
@@ -560,6 +562,15 @@ void vm_event_cleanup(struct domain *d)
         (void)vm_event_disable(d, &d->vm_event->share);
     }
 #endif
+
+    for_each_vcpu ( d, v )
+    {
+        if ( unlikely(v->arch.vm_event) )
+        {
+            xfree(v->arch.vm_event);
+            v->arch.vm_event = NULL;
+        }
+    }
 }
 
 int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 0611681..a094c0d 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -26,6 +26,7 @@
 #include <public/domctl.h>
 #include <asm/cpufeature.h>
 #include <asm/hvm/hvm.h>
+#include <asm/vm_event.h>
 
 #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
 
@@ -48,7 +49,8 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
          * Enabling mem_access_emulate_each_rep without a vm_event subscriber
          * is meaningless.
          */
-        if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event )
+        if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event &&
+             d->vcpu[0]->arch.vm_event->emul_read_data )
             d->arch.mem_access_emulate_each_rep = !!mop->event;
         else
             rc = -EINVAL;
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 026f42e..557f353 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -28,7 +28,7 @@
  */
 struct arch_vm_event {
     uint32_t emulate_flags;
-    struct vm_event_emul_read_data emul_read_data;
+    struct vm_event_emul_read_data *emul_read_data;
     struct monitor_write_data write_data;
 };
 
-- 
2.5.0


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

[PATCH v2 4/7] x86/vm_event_resume: surround VM_EVENT_REASON_MOV_TO_MSR w/ CONFIG_X86

[Corneliu ZUZU]

VM_EVENT_REASON_MOV_TO_MSR is X86-specific, surround w/ #ifdef accordingly.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Changed since v1: <nothing>
---
 xen/common/vm_event.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 75bbbab..78126cf 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -394,7 +394,9 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
          */
         switch ( rsp.reason )
         {
+#ifdef CONFIG_X86
         case VM_EVENT_REASON_MOV_TO_MSR:
+#endif
         case VM_EVENT_REASON_WRITE_CTRLREG:
             vm_event_register_write_resume(v, &rsp);
             break;
-- 
2.5.0


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

[PATCH v2 5/7] x86/vm-event: minor ASSERT fix, add 'unlikely'

[Corneliu ZUZU]

Minor fixes:

* vm_event_register_write_resume: ASSERT on non-NULL v->arch.vm_event instead of
  &v->arch.vm_event->write_data.
* add 'unlikely' in if

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/hvm/hvm.c  | 2 +-
 xen/arch/x86/vm_event.c | 6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ac6d9eb..091cabe 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -475,7 +475,7 @@ void hvm_do_resume(struct vcpu *v)
 
     if ( unlikely(v->arch.vm_event) )
     {
-        if ( v->arch.vm_event->emulate_flags )
+        if ( unlikely(v->arch.vm_event->emulate_flags) )
         {
             enum emul_kind kind = EMUL_KIND_NORMAL;
 
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 0e25e4d..2fe7808 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -83,14 +83,16 @@ void vm_event_register_write_resume(struct vcpu *v, vm_event_response_t *rsp)
 {
     if ( rsp->flags & VM_EVENT_FLAG_DENY )
     {
-        struct monitor_write_data *w = &v->arch.vm_event->write_data;
+        struct monitor_write_data *w;
 
-        ASSERT(w);
+        ASSERT(v->arch.vm_event);
 
         /* deny flag requires the vCPU to be paused */
         if ( !atomic_read(&v->vm_event_pause_count) )
             return;
 
+        w = &v->arch.vm_event->write_data;
+
         switch ( rsp->reason )
         {
         case VM_EVENT_REASON_MOV_TO_MSR:
-- 
2.5.0


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

[PATCH v2 6/7] minor fixes (formatting, comments, unused includes etc.)

[Corneliu ZUZU]

Minor fixes:
 - remove some empty lines
 - remove some unused includes
 - multi-line comment fixes
 - 80-columns formatting fixes

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
Changed since v1: <nothing>
---
 xen/arch/arm/domain.c             |  1 -
 xen/arch/arm/traps.c              |  1 -
 xen/arch/x86/hvm/hvm.c            |  3 ---
 xen/arch/x86/hvm/vmx/vmx.c        |  1 -
 xen/arch/x86/monitor.c            |  1 -
 xen/arch/x86/vm_event.c           |  3 ---
 xen/common/monitor.c              |  1 -
 xen/common/vm_event.c             |  6 ++++--
 xen/include/asm-arm/vm_event.h    |  9 +++------
 xen/include/asm-x86/hvm/monitor.h |  1 -
 xen/include/asm-x86/monitor.h     |  3 ---
 xen/include/asm-x86/vm_event.h    |  1 -
 xen/include/public/vm_event.h     | 38 +++++++++++++++++++-------------------
 xen/include/xen/vm_event.h        |  1 -
 14 files changed, 26 insertions(+), 44 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 6ce4645..61fc08e 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -294,7 +294,6 @@ static void continue_new_vcpu(struct vcpu *prev)
     else
         /* check_wakeup_from_wait(); */
         reset_stack_and_jump(return_to_new_vcpu64);
-
 }
 
 void context_switch(struct vcpu *prev, struct vcpu *next)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 44926ca..fb01703 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -439,7 +439,6 @@ static void inject_abt32_exception(struct cpu_user_regs *regs,
         far |= addr << 32;
         WRITE_SYSREG(far, FAR_EL1);
         WRITE_SYSREG(fsr, IFSR32_EL2);
-
 #endif
     }
     else
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 091cabe..32d2d5e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -31,7 +31,6 @@
 #include <xen/hypercall.h>
 #include <xen/guest_access.h>
 #include <xen/event.h>
-#include <xen/paging.h>
 #include <xen/cpu.h>
 #include <xen/wait.h>
 #include <xen/mem_access.h>
@@ -63,11 +62,9 @@
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/monitor.h>
 #include <asm/hvm/ioreq.h>
-#include <asm/hvm/vmx/vmx.h>
 #include <asm/altp2m.h>
 #include <asm/mtrr.h>
 #include <asm/apic.h>
-#include <asm/vm_event.h>
 #include <public/sched.h>
 #include <public/hvm/ioreq.h>
 #include <public/version.h>
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 0fa3fea..ee108b5 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -34,7 +34,6 @@
 #include <asm/guest_access.h>
 #include <asm/debugreg.h>
 #include <asm/msr.h>
-#include <asm/paging.h>
 #include <asm/p2m.h>
 #include <asm/mem_sharing.h>
 #include <asm/hvm/emulate.h>
diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 2bb7d86..e3f57eb 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -21,7 +21,6 @@
 
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/monitor.h>
-#include <asm/vm_event.h>
 #include <public/vm_event.h>
 
 int arch_monitor_init_domain(struct domain *d)
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 2fe7808..eed82e5 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -18,9 +18,6 @@
  * License along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <xen/sched.h>
-#include <asm/hvm/hvm.h>
-#include <asm/monitor.h>
 #include <asm/vm_event.h>
 
 /* Implicitly serialized by the domctl lock. */
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 436214a..08d1d98 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -23,7 +23,6 @@
 #include <xen/sched.h>
 #include <xen/vm_event.h>
 #include <xsm/xsm.h>
-#include <public/domctl.h>
 #include <asm/monitor.h>
 
 int monitor_domctl(struct domain *d, struct xen_domctl_monitor_op *mop)
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 78126cf..8c80901 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -781,8 +781,10 @@ void vm_event_vcpu_unpause(struct vcpu *v)
 {
     int old, new, prev = v->vm_event_pause_count.counter;
 
-    /* All unpause requests as a result of toolstack responses.  Prevent
-     * underflow of the vcpu pause count. */
+    /*
+     * All unpause requests as a result of toolstack responses.
+     * Prevent underflow of the vcpu pause count.
+     */
     do
     {
         old = prev;
diff --git a/xen/include/asm-arm/vm_event.h b/xen/include/asm-arm/vm_event.h
index a3fc4ce..ccc4b60 100644
--- a/xen/include/asm-arm/vm_event.h
+++ b/xen/include/asm-arm/vm_event.h
@@ -23,21 +23,18 @@
 #include <xen/vm_event.h>
 #include <public/domctl.h>
 
-static inline
-int vm_event_init_domain(struct domain *d)
+static inline int vm_event_init_domain(struct domain *d)
 {
     /* Nothing to do. */
     return 0;
 }
 
-static inline
-void vm_event_cleanup_domain(struct domain *d)
+static inline void vm_event_cleanup_domain(struct domain *d)
 {
     memset(&d->monitor, 0, sizeof(d->monitor));
 }
 
-static inline
-void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
+static inline void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v)
 {
     /* Not supported on ARM. */
 }
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index 8b0f119..8d4ef19 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -19,7 +19,6 @@
 #ifndef __ASM_X86_HVM_MONITOR_H__
 #define __ASM_X86_HVM_MONITOR_H__
 
-#include <xen/sched.h>
 #include <xen/paging.h>
 #include <public/vm_event.h>
 
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index a094c0d..aa17be1 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -23,9 +23,6 @@
 #define __ASM_X86_MONITOR_H__
 
 #include <xen/sched.h>
-#include <public/domctl.h>
-#include <asm/cpufeature.h>
-#include <asm/hvm/hvm.h>
 #include <asm/vm_event.h>
 
 #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
index 557f353..10c2e08 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -20,7 +20,6 @@
 #define __ASM_X86_VM_EVENT_H__
 
 #include <xen/sched.h>
-#include <xen/vm_event.h>
 
 /*
  * Should we emulate the next matching instruction on VCPU resume
diff --git a/xen/include/public/vm_event.h b/xen/include/public/vm_event.h
index 7bfe6cc..8c29968 100644
--- a/xen/include/public/vm_event.h
+++ b/xen/include/public/vm_event.h
@@ -74,20 +74,20 @@
  * VM_EVENT_FLAG_SET_EMUL_READ_DATA are set, only the latter will be honored).
  */
 #define VM_EVENT_FLAG_SET_EMUL_READ_DATA (1 << 5)
- /*
-  * Deny completion of the operation that triggered the event.
-  * Currently only useful for MSR, CR0, CR3 and CR4 write events.
-  * Requires the vCPU to be paused already (synchronous events only).
-  */
+/*
+ * Deny completion of the operation that triggered the event.
+ * Currently only useful for MSR and control-register write events.
+ * Requires the vCPU to be paused already (synchronous events only).
+ */
 #define VM_EVENT_FLAG_DENY               (1 << 6)
 /*
  * This flag can be set in a request or a response
  *
- * On a request, indicates that the event occurred in the alternate p2m specified by
- * the altp2m_idx request field.
+ * On a request, indicates that the event occurred in the alternate p2m
+ * specified by the altp2m_idx request field.
  *
- * On a response, indicates that the VCPU should resume in the alternate p2m specified
- * by the altp2m_idx response field if possible.
+ * On a response, indicates that the VCPU should resume in the alternate p2m
+ * specified by the altp2m_idx response field if possible.
  */
 #define VM_EVENT_FLAG_ALTERNATE_P2M      (1 << 7)
 /*
@@ -180,16 +180,16 @@ struct vm_event_regs_x86 {
  * FAULT_WITH_GLA: If the violation was triggered by accessing gla
  * FAULT_IN_GPT: If the violation was triggered during translating gla
  */
-#define MEM_ACCESS_R                    (1 << 0)
-#define MEM_ACCESS_W                    (1 << 1)
-#define MEM_ACCESS_X                    (1 << 2)
-#define MEM_ACCESS_RWX                  (MEM_ACCESS_R | MEM_ACCESS_W | MEM_ACCESS_X)
-#define MEM_ACCESS_RW                   (MEM_ACCESS_R | MEM_ACCESS_W)
-#define MEM_ACCESS_RX                   (MEM_ACCESS_R | MEM_ACCESS_X)
-#define MEM_ACCESS_WX                   (MEM_ACCESS_W | MEM_ACCESS_X)
-#define MEM_ACCESS_GLA_VALID            (1 << 3)
-#define MEM_ACCESS_FAULT_WITH_GLA       (1 << 4)
-#define MEM_ACCESS_FAULT_IN_GPT         (1 << 5)
+#define MEM_ACCESS_R                (1 << 0)
+#define MEM_ACCESS_W                (1 << 1)
+#define MEM_ACCESS_X                (1 << 2)
+#define MEM_ACCESS_RWX              (MEM_ACCESS_R | MEM_ACCESS_W | MEM_ACCESS_X)
+#define MEM_ACCESS_RW               (MEM_ACCESS_R | MEM_ACCESS_W)
+#define MEM_ACCESS_RX               (MEM_ACCESS_R | MEM_ACCESS_X)
+#define MEM_ACCESS_WX               (MEM_ACCESS_W | MEM_ACCESS_X)
+#define MEM_ACCESS_GLA_VALID        (1 << 3)
+#define MEM_ACCESS_FAULT_WITH_GLA   (1 << 4)
+#define MEM_ACCESS_FAULT_IN_GPT     (1 << 5)
 
 struct vm_event_mem_access {
     uint64_t gfn;
diff --git a/xen/include/xen/vm_event.h b/xen/include/xen/vm_event.h
index 89e6243..2074090 100644
--- a/xen/include/xen/vm_event.h
+++ b/xen/include/xen/vm_event.h
@@ -83,7 +83,6 @@ int vm_event_monitor_traps(struct vcpu *v, uint8_t sync,
 
 #endif /* __VM_EVENT_H__ */
 
-
 /*
  * Local variables:
  * mode: C
-- 
2.5.0


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

[PATCH v2 7/7] minor #include change

[Corneliu ZUZU]

Move xen/paging.h #include from hvm/monitor.h to hvm/monitor.c (include strictly
where needed) and also change to asm/paging.h (include strictly what's needed).

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
Changed since v1:
  * preserve alphabetical ordering
---
 xen/arch/x86/hvm/monitor.c        | 1 +
 xen/include/asm-x86/hvm/monitor.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
index bbe5952..db6db6f 100644
--- a/xen/arch/x86/hvm/monitor.c
+++ b/xen/arch/x86/hvm/monitor.c
@@ -25,6 +25,7 @@
 #include <xen/vm_event.h>
 #include <asm/hvm/monitor.h>
 #include <asm/monitor.h>
+#include <asm/paging.h>
 #include <asm/vm_event.h>
 #include <public/vm_event.h>
 
diff --git a/xen/include/asm-x86/hvm/monitor.h b/xen/include/asm-x86/hvm/monitor.h
index 8d4ef19..1c8ec6c 100644
--- a/xen/include/asm-x86/hvm/monitor.h
+++ b/xen/include/asm-x86/hvm/monitor.h
@@ -19,7 +19,6 @@
 #ifndef __ASM_X86_HVM_MONITOR_H__
 #define __ASM_X86_HVM_MONITOR_H__
 
-#include <xen/paging.h>
 #include <public/vm_event.h>
 
 enum hvm_monitor_debug_type
-- 
2.5.0


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

Re: [PATCH v2 7/7] minor #include change

[Tamas K Lengyel]

On Tue, Jul 5, 2016 at 8:31 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
> Move xen/paging.h #include from hvm/monitor.h to hvm/monitor.c (include strictly
> where needed) and also change to asm/paging.h (include strictly what's needed).
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

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

Re: [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

[George Dunlap]

On 05/07/16 15:28, Corneliu ZUZU wrote:
> The arch_vm_event structure is dynamically allocated and freed @
> vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack user
> disables domain monitoring (xc_monitor_disable), which in turn effectively
> discards any information that was in arch_vm_event.write_data.
> 
> But this can yield unexpected behavior since if a CR-write was awaiting to be
> committed on the scheduling tail (hvm_do_resume->arch_monitor_write_data)
> before xc_monitor_disable is called, then the domain CR write is wrongfully
> ignored, which of course, in these cases, can easily render a domain crash.
> 
> To fix the issue, this patch makes arch_vm_event.emul_read_data dynamically
> allocated and only frees that in vm_event_cleanup_domain, instead of the whole
> arch_vcpu.vm_event structure, which with this patch will only be freed on
> vcpu/domain destroyal.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

[snip]

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 16733a4..6616626 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1642,7 +1642,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>          v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
>  
>          if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
> -            v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> +            *v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
>      }
>  }
>  

On the basis of Razvan's ack:

Acked-by: George Dunlap <george.dunlap@citrix.com>

> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 80f84d6..0e25e4d 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -30,12 +30,18 @@ int vm_event_init_domain(struct domain *d)
>  
>      for_each_vcpu ( d, v )
>      {
> -        if ( v->arch.vm_event )
> +        if ( likely(!v->arch.vm_event) )
> +        {
> +            v->arch.vm_event = xzalloc(struct arch_vm_event);
> +            if ( !v->arch.vm_event )
> +                return -ENOMEM;
> +        }
> +        else if ( unlikely(v->arch.vm_event->emul_read_data) )
>              continue;
>  
> -        v->arch.vm_event = xzalloc(struct arch_vm_event);
> -
> -        if ( !v->arch.vm_event )
> +        v->arch.vm_event->emul_read_data =
> +                xzalloc(struct vm_event_emul_read_data);
> +        if ( !v->arch.vm_event->emul_read_data )
>              return -ENOMEM;
>      }
>  
> @@ -52,8 +58,12 @@ void vm_event_cleanup_domain(struct domain *d)
>  
>      for_each_vcpu ( d, v )
>      {
> -        xfree(v->arch.vm_event);
> -        v->arch.vm_event = NULL;
> +        if ( likely(!v->arch.vm_event) )
> +            continue;
> +
> +        v->arch.vm_event->emulate_flags = 0;
> +        xfree(v->arch.vm_event->emul_read_data);
> +        v->arch.vm_event->emul_read_data = NULL;
>      }
>  
>      d->arch.mem_access_emulate_each_rep = 0;
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 17d2716..75bbbab 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -534,6 +534,8 @@ static void mem_sharing_notification(struct vcpu *v, unsigned int port)
>  /* Clean up on domain destruction */
>  void vm_event_cleanup(struct domain *d)
>  {
> +    struct vcpu *v;
> +
>  #ifdef CONFIG_HAS_MEM_PAGING
>      if ( d->vm_event->paging.ring_page )
>      {
> @@ -560,6 +562,15 @@ void vm_event_cleanup(struct domain *d)
>          (void)vm_event_disable(d, &d->vm_event->share);
>      }
>  #endif
> +
> +    for_each_vcpu ( d, v )
> +    {
> +        if ( unlikely(v->arch.vm_event) )
> +        {
> +            xfree(v->arch.vm_event);
> +            v->arch.vm_event = NULL;
> +        }
> +    }
>  }
>  
>  int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 0611681..a094c0d 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -26,6 +26,7 @@
>  #include <public/domctl.h>
>  #include <asm/cpufeature.h>
>  #include <asm/hvm/hvm.h>
> +#include <asm/vm_event.h>
>  
>  #define monitor_ctrlreg_bitmask(ctrlreg_index) (1U << (ctrlreg_index))
>  
> @@ -48,7 +49,8 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>           * Enabling mem_access_emulate_each_rep without a vm_event subscriber
>           * is meaningless.
>           */
> -        if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event )
> +        if ( d->max_vcpus && d->vcpu[0] && d->vcpu[0]->arch.vm_event &&
> +             d->vcpu[0]->arch.vm_event->emul_read_data )
>              d->arch.mem_access_emulate_each_rep = !!mop->event;
>          else
>              rc = -EINVAL;
> diff --git a/xen/include/asm-x86/vm_event.h b/xen/include/asm-x86/vm_event.h
> index 026f42e..557f353 100644
> --- a/xen/include/asm-x86/vm_event.h
> +++ b/xen/include/asm-x86/vm_event.h
> @@ -28,7 +28,7 @@
>   */
>  struct arch_vm_event {
>      uint32_t emulate_flags;
> -    struct vm_event_emul_read_data emul_read_data;
> +    struct vm_event_emul_read_data *emul_read_data;
>      struct monitor_write_data write_data;
>  };
>  
> 


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

Re: [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

[George Dunlap]

On 05/07/16 15:28, Corneliu ZUZU wrote:
> The arch_vm_event structure is dynamically allocated and freed @
> vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack user
> disables domain monitoring (xc_monitor_disable), which in turn effectively
> discards any information that was in arch_vm_event.write_data.
> 
> But this can yield unexpected behavior since if a CR-write was awaiting to be
> committed on the scheduling tail (hvm_do_resume->arch_monitor_write_data)
> before xc_monitor_disable is called, then the domain CR write is wrongfully
> ignored, which of course, in these cases, can easily render a domain crash.
> 
> To fix the issue, this patch makes arch_vm_event.emul_read_data dynamically
> allocated and only frees that in vm_event_cleanup_domain, instead of the whole
> arch_vcpu.vm_event structure, which with this patch will only be freed on
> vcpu/domain destroyal.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

On the basis of one of the vm_event maintainer's acks...

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 16733a4..6616626 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1642,7 +1642,7 @@ void p2m_mem_access_emulate_check(struct vcpu *v,
>          v->arch.vm_event->emulate_flags = violation ? rsp->flags : 0;
>  
>          if ( (rsp->flags & VM_EVENT_FLAG_SET_EMUL_READ_DATA) )
> -            v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
> +            *v->arch.vm_event->emul_read_data = rsp->data.emul_read_data;
>      }
>  }
>  

p2m bits:

Acked-by: George Dunlap <george.dunlap@citrix.com>


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

Re: [PATCH v2 7/7] minor #include change

[Razvan Cojocaru]

On 07/05/16 17:31, Corneliu ZUZU wrote:
> Move xen/paging.h #include from hvm/monitor.h to hvm/monitor.c (include strictly
> where needed) and also change to asm/paging.h (include strictly what's needed).
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
> Changed since v1:
>   * preserve alphabetical ordering
> ---
>  xen/arch/x86/hvm/monitor.c        | 1 +
>  xen/include/asm-x86/hvm/monitor.h | 1 -
>  2 files changed, 1 insertion(+), 1 deletion(-)


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


Thanks,
Razvan

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

Re: [PATCH v2 6/7] minor fixes (formatting, comments, unused includes etc.)

[Razvan Cojocaru]

On 07/05/16 17:30, Corneliu ZUZU wrote:
> Minor fixes:
>  - remove some empty lines
>  - remove some unused includes
>  - multi-line comment fixes
>  - 80-columns formatting fixes
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
> Changed since v1: <nothing>
> ---
>  xen/arch/arm/domain.c             |  1 -
>  xen/arch/arm/traps.c              |  1 -
>  xen/arch/x86/hvm/hvm.c            |  3 ---
>  xen/arch/x86/hvm/vmx/vmx.c        |  1 -
>  xen/arch/x86/monitor.c            |  1 -
>  xen/arch/x86/vm_event.c           |  3 ---
>  xen/common/monitor.c              |  1 -
>  xen/common/vm_event.c             |  6 ++++--
>  xen/include/asm-arm/vm_event.h    |  9 +++------
>  xen/include/asm-x86/hvm/monitor.h |  1 -
>  xen/include/asm-x86/monitor.h     |  3 ---
>  xen/include/asm-x86/vm_event.h    |  1 -
>  xen/include/public/vm_event.h     | 38 +++++++++++++++++++-------------------
>  xen/include/xen/vm_event.h        |  1 -
>  14 files changed, 26 insertions(+), 44 deletions(-)

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


Thanks,
Razvan

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

Re: [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

[Razvan Cojocaru]

On 07/05/16 17:28, Corneliu ZUZU wrote:
> The arch_vm_event structure is dynamically allocated and freed @
> vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack user
> disables domain monitoring (xc_monitor_disable), which in turn effectively
> discards any information that was in arch_vm_event.write_data.
> 
> But this can yield unexpected behavior since if a CR-write was awaiting to be
> committed on the scheduling tail (hvm_do_resume->arch_monitor_write_data)
> before xc_monitor_disable is called, then the domain CR write is wrongfully
> ignored, which of course, in these cases, can easily render a domain crash.
> 
> To fix the issue, this patch makes arch_vm_event.emul_read_data dynamically
> allocated and only frees that in vm_event_cleanup_domain, instead of the whole
> arch_vcpu.vm_event structure, which with this patch will only be freed on
> vcpu/domain destroyal.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
> ---
> Changed since v1:
>   * arch_vcpu.vm_event made pointer again to avoid eating memory from arch_vcpu
>     structure

I believe that all acks should be presumed lost on non-trivial changes
in a new version (which I believe this qualifies as being, with all the
new logic of allocating / deallocating only part of vm_event).

Unfortunately I'm out of office until early next week so I can't
properly test / thoroughly parse this until then, but we should be extra
careful that there are several places in the code where it is assumed
that v->arch.vm_event != NULL is the same thing as monitoring being
enabled. I'm not saying that they're not treated in this patch (the
proper change has certainly been made in emulate.c), I'm just saying
that we should be careful that they are.

Having said that, I propose a special macro to make this all clearer,
something like:

#define is_monitor_enabled_for_vcpu(v) \
    ( v->arch.vm_event && v->arch.vm_event->emul_read_data )

or equivalent inline functions returning a bool_t. Just a thought.


Thanks,
Razvan

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

Re: [PATCH v2 3/7] x86/vm-event/monitor: don't compromise monitor_write_data on domain cleanup

[Corneliu ZUZU]

On 7/5/2016 8:16 PM, Razvan Cojocaru wrote:
> On 07/05/16 17:28, Corneliu ZUZU wrote:
>> The arch_vm_event structure is dynamically allocated and freed @
>> vm_event_cleanup_domain. This cleanup is triggered e.g. when the toolstack user
>> disables domain monitoring (xc_monitor_disable), which in turn effectively
>> discards any information that was in arch_vm_event.write_data.
>>
>> But this can yield unexpected behavior since if a CR-write was awaiting to be
>> committed on the scheduling tail (hvm_do_resume->arch_monitor_write_data)
>> before xc_monitor_disable is called, then the domain CR write is wrongfully
>> ignored, which of course, in these cases, can easily render a domain crash.
>>
>> To fix the issue, this patch makes arch_vm_event.emul_read_data dynamically
>> allocated and only frees that in vm_event_cleanup_domain, instead of the whole
>> arch_vcpu.vm_event structure, which with this patch will only be freed on
>> vcpu/domain destroyal.
>>
>> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
>> Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
>> ---
>> Changed since v1:
>>    * arch_vcpu.vm_event made pointer again to avoid eating memory from arch_vcpu
>>      structure
> I believe that all acks should be presumed lost on non-trivial changes
> in a new version (which I believe this qualifies as being, with all the
> new logic of allocating / deallocating only part of vm_event).
>
> Unfortunately I'm out of office until early next week so I can't
> properly test / thoroughly parse this until then, but we should be extra
> careful that there are several places in the code where it is assumed
> that v->arch.vm_event != NULL is the same thing as monitoring being
> enabled. I'm not saying that they're not treated in this patch (the
> proper change has certainly been made in emulate.c), I'm just saying
> that we should be careful that they are.
>
> Having said that, I propose a special macro to make this all clearer,
> something like:
>
> #define is_monitor_enabled_for_vcpu(v) \
>      ( v->arch.vm_event && v->arch.vm_event->emul_read_data )
>
> or equivalent inline functions returning a bool_t. Just a thought.
>
>
> Thanks,
> Razvan

At some point I actually defined that exact macro while constructing 
this patch, but I decided to delete it afterwards because I thought the 
code would still be clear without it (i.e. only check emul_read_data 
when we actually need _that_ to be non-NULL) and because it seemed a bit 
strange, being possible to have _arch_vm_event allocated_ but having the 
monitor vm-events subsystem _uninitialized_ (because of .write_data 
being treated specially). Since the write_data field is also part of the 
monitor subsystem, conceptually one could say the monitor subsystem is 
at least _partially_ initialized when that's non-NULL, not uninitialized 
at all. I'll think of a resolution around this, but in the meantime I 
there's something more important to settle:

I only now notice, it seems to me that the ASSERT(v->arch.vm_event) @ 
hvm_set_cr0 & such (the one just before calling hvm_monitor_crX) might 
fail. That's because from what I see in the code the following can happen:
- v.arch.vm_event = NULL (vm-event _not_ initialized)
- toolstack user calls e.g. xc_moLRnitor_write_ctrlreg(xch, domid, 
VM_EVENT_X86_CR0, true, true, false) -> do_domctl(XEN_DOMCTL_monitor_op) 
-> monitor_domctl(XEN_DOMCTL_MONITOR_OP_ENABLE) -> 
arch_monitor_domctl_event(XEN_DOMCTL_MONITOR_EVENT_WRITE_CTREG) which in 
turn sets the CR0 bit of d.arch.monitor.write_ctrlreg_enabled _without 
ever checking if v.arch.vm_event is non-NULL_
- afterwards a CR0 write happens on one of the domain vCPUs and we 
arrive at that assert without having v.arch.vm_event allocated!

I can't test this @ the moment, am I missing something here? I remember 
taking a look on this code path at some point and idk how I didn't 
notice something like this, it seems obviously wrong..has something 
changed recently?
I think we need to take a second look over this code-path, I'm also 
seeing that the proper check _is done_ @ 
arch_monitor_domctl_op(XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP) (though I 
think a more proper error return value there would be ENODEV instead of 
EINVAL).

Corneliu.

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

Re: [PATCH v2 6/7] minor fixes (formatting, comments, unused includes etc.)

[Julien Grall]

Hi Corneliu,

On 05/07/16 15:30, Corneliu ZUZU wrote:
> Minor fixes:
>   - remove some empty lines
>   - remove some unused includes
>   - multi-line comment fixes
>   - 80-columns formatting fixes
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Regards,

-- 
Julien Grall

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