[PATCH v3 0/8] x86/vm-event: Adjustments & fixes

[PATCH v3 0/8] x86/vm-event: Adjustments & fixes

[Corneliu ZUZU]

This patch-series makes some adjustments and fixes to the X86 vm-events code.

Summary:
Corneliu ZUZU (8):
  1. x86/vmx_update_guest_cr: minor optimization
  2. x86/vm-event/monitor: relocate code-motion more appropriately
  3. x86/vm-event/monitor: don't compromise monitor_write_data on domain
     cleanup
  4. x86/vm_event/monitor/cr: check for vm-event subscriber on domctl
  5. x86/vm_event_resume: surround VM_EVENT_REASON_MOV_TO_MSR w/ CONFIG_X86
        Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
  6. x86/vm-event: minor ASSERT fix, add 'unlikely'
  7. minor fixes (formatting, comments, unused includes etc.)
        Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
        Acked-by: Julien Grall <julien.grall@arm.com>
  8. minor #include change
        Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
        Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

Significant changes since v2:
  0/8: patch 4 added
  2/8: call vmx_vm_event_update_cr3_traps from arch_monitor_cleanup_domain also
  3/8: * introduce vm_event_{vcpu,domain}_initialised inline functions for clarity
       * xfree arch_vm_event in vm_event_cleanup_domain as before if there are no
         uncommitted writes in arch_vm_event.write_data

 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            | 72 ++++++++++++++++++++++++++++++++++-----
 xen/arch/x86/vm_event.c           | 44 ++++++++++++++++++------
 xen/common/monitor.c              |  1 -
 xen/common/vm_event.c             | 20 +++++++++--
 xen/include/asm-arm/vm_event.h    |  9 ++---
 xen/include/asm-x86/domain.h      | 17 +++++----
 xen/include/asm-x86/hvm/monitor.h |  2 --
 xen/include/asm-x86/hvm/vmx/vmx.h |  1 +
 xen/include/asm-x86/monitor.h     | 10 +++---
 xen/include/asm-x86/vm_event.h    | 14 ++++++--
 xen/include/public/vm_event.h     | 38 ++++++++++-----------
 xen/include/xen/vm_event.h        |  1 -
 20 files changed, 255 insertions(+), 111 deletions(-)

-- 
2.5.0


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

[PATCH v3 1/8] 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 v2: <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 v3 2/8] 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
functions write_ctrlreg_adjust_traps and write_ctrlreg_disable_traps (in
x86/monitor.c) which deal with setting/unsetting any traps for cr-write monitor
vm-events.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
Changed since v2:
  * add write_ctrlreg_disable_traps, call from arch_monitor_cleanup_domain
---
 xen/arch/x86/hvm/hvm.c            | 46 ++++++++++-----------------
 xen/arch/x86/hvm/vmx/vmx.c        | 58 +++++++++++++++++++++++++++++++---
 xen/arch/x86/monitor.c            | 66 ++++++++++++++++++++++++++++++++++-----
 xen/include/asm-x86/hvm/vmx/vmx.h |  1 +
 xen/include/asm-x86/monitor.h     |  2 ++
 5 files changed, 131 insertions(+), 42 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..42f31bf 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -19,9 +19,36 @@
  * 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>
 
+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 ) /* other CRs are always trapped */
+        vmx_vm_event_update_cr3_traps(d);
+}
+
+static inline void write_ctrlreg_disable_traps(struct domain *d)
+{
+    unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
+    d->arch.monitor.write_ctrlreg_enabled = 0;
+
+    if ( old )
+    {
+        /* vmx only */
+        ASSERT(cpu_has_vmx);
+
+        /* was CR3 load-exiting enabled due to monitoring? */
+        if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
+            vmx_vm_event_update_cr3_traps(d);
+    }
+}
+
 int arch_monitor_init_domain(struct domain *d)
 {
     if ( !d->arch.monitor.msr_bitmap )
@@ -36,11 +63,40 @@ int arch_monitor_init_domain(struct domain *d)
 void arch_monitor_cleanup_domain(struct domain *d)
 {
     xfree(d->arch.monitor.msr_bitmap);
-
+    write_ctrlreg_disable_traps(d);
     memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
     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);
@@ -159,13 +215,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 v3 3/8] 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
    - in vm_event_cleanup_domain, if there are still uncommitted writes in
      arch_vm_event.write_data:
        - only frees emul_read_data
        - defers xfree of the entire arch_vm_event until vcpu/domain destroyal
    - otherwise arch_vm_event is freed in vm_event_cleanup_domain, as before

For clarity, also introduce inline functions that check initialisation of the
vm_event subsystem for a vcpu/domain (vm_event_{vcpu,domain}_initialised), since
that is now true only when both arch_vm_event and arch_vm_event.emul_read_data
are non-NULL.

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
Changed since v2:
  * introduce vm_event_{vcpu,domain}_initialised inline functions for clarity
  * xfree arch_vm_event in vm_event_cleanup_domain as before if there are no
    uncommitted writes in arch_vm_event.write_data
---
 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        | 35 +++++++++++++++++++++++++++++------
 xen/common/vm_event.c          | 12 ++++++++++++
 xen/include/asm-x86/domain.h   | 17 +++++++++++------
 xen/include/asm-x86/monitor.h  |  3 ++-
 xen/include/asm-x86/vm_event.h | 13 ++++++++++++-
 9 files changed, 79 insertions(+), 20 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..59e2344 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 ( vm_event_vcpu_initialised(curr) )
     {
         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..ff2ba92 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,25 @@ 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;
+
+        /*
+         * Only xfree the entire arch_vm_event if write_data was fully handled.
+         * Otherwise defer entire xfree until domain/vcpu destroyal.
+         */
+        if ( likely(!v->arch.vm_event->write_data.uncommitted_writes) )
+        {
+            xfree(v->arch.vm_event->emul_read_data);
+            xfree(v->arch.vm_event);
+            v->arch.vm_event = NULL;
+            continue;
+        }
+
+        /* write_data not fully handled, only xfree emul_read_data */
+        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..47ae96c 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,16 @@ 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) )
+        {
+            /* vm_event->emul_read_data freed in vm_event_cleanup_domain */
+            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/domain.h b/xen/include/asm-x86/domain.h
index 8f64ae9..0e3e139 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -260,12 +260,17 @@ struct pv_domain
 };
 
 struct monitor_write_data {
-    struct {
-        unsigned int msr : 1;
-        unsigned int cr0 : 1;
-        unsigned int cr3 : 1;
-        unsigned int cr4 : 1;
-    } do_write;
+    union {
+        struct {
+            unsigned int msr : 1;
+            unsigned int cr0 : 1;
+            unsigned int cr3 : 1;
+            unsigned int cr4 : 1;
+        } do_write;
+
+        /* non-zero when at least one of do_write fields is non-zero */
+        unsigned int uncommitted_writes;
+    };
 
     uint32_t msr;
     uint64_t value;
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 0611681..9238ec8 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,7 @@ 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 ( vm_event_domain_initialised(d) )
             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..9bdeccc 100644
--- a/xen/include/asm-x86/vm_event.h
+++ b/xen/include/asm-x86/vm_event.h
@@ -28,12 +28,23 @@
  */
 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;
 };
 
 int vm_event_init_domain(struct domain *d);
 
+static inline bool_t vm_event_vcpu_initialised(struct vcpu *v)
+{
+    return (v->arch.vm_event && v->arch.vm_event->emul_read_data);
+}
+
+static inline bool_t vm_event_domain_initialised(struct domain *d)
+{
+    return (d->max_vcpus && d->vcpu[0] &&
+            vm_event_vcpu_initialised(d->vcpu[0]));
+}
+
 void vm_event_cleanup_domain(struct domain *d);
 
 void vm_event_toggle_singlestep(struct domain *d, struct vcpu *v);
-- 
2.5.0


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

[PATCH v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl

[Corneliu ZUZU]

Enforce presence of a monitor vm-event subscriber when the toolstack user calls
xc_monitor_write_ctrlreg (XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG domctl).
Without this change, ASSERT(v->arch.vm_event) @ hvm_set_cr0 and such would fail
if the toolstack user calls xc_monitor_write_ctrlreg(...) w/ enable = true,
without first calling xc_monitor_enable().

Also adjust returned error code for similar check from -EINVAL to more
descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).

Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
---
 xen/arch/x86/monitor.c        | 7 +++++++
 xen/include/asm-x86/monitor.h | 2 +-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
index 42f31bf..4d4db33 100644
--- a/xen/arch/x86/monitor.c
+++ b/xen/arch/x86/monitor.c
@@ -188,6 +188,13 @@ int arch_monitor_domctl_event(struct domain *d,
         unsigned int ctrlreg_bitmask;
         bool_t old_status;
 
+        /*
+         * Enabling cr-write vm-events without a vm_event subscriber is
+         * meaningless.
+         */
+        if ( !vm_event_domain_initialised(d) )
+            return -ENOSYS;
+
         /* sanity check: avoid left-shift undefined behavior */
         if ( unlikely(mop->u.mov_to_cr.index > 31) )
             return -EINVAL;
diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
index 9238ec8..2213124 100644
--- a/xen/include/asm-x86/monitor.h
+++ b/xen/include/asm-x86/monitor.h
@@ -52,7 +52,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
         if ( vm_event_domain_initialised(d) )
             d->arch.mem_access_emulate_each_rep = !!mop->event;
         else
-            rc = -EINVAL;
+            rc = -ENOSYS;
 
         domain_unpause(d);
         break;
-- 
2.5.0


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

[PATCH v3 5/8] 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 v2: <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 47ae96c..40a7e56 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 v3 6/8] 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>
---
Changed since v2: <nothing>
---
 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 ff2ba92..e37d6d3 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -96,14 +96,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 v3 7/8] 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>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
Changed since v2: <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 4d4db33..8423931 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>
 
 static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index)
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index e37d6d3..2b1abe2 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 40a7e56..93747eb 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -782,8 +782,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 2213124..cf68512 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 9bdeccc..0264686 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 v3 8/8] 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>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
---
Changed since v2: <nothing>
---
 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 v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl

[Jan Beulich]

>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote:
> Also adjust returned error code for similar check from -EINVAL to more
> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).

I'm not sure that's more descriptive, and we really try to not use
ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP
perhaps?

Jan


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

Re: [PATCH v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl

[Corneliu ZUZU]

On 7/6/2016 7:01 PM, Jan Beulich wrote:
>>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote:
>> Also adjust returned error code for similar check from -EINVAL to more
>> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).
> I'm not sure that's more descriptive, and we really try to not use
> ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP
> perhaps?
>
> Jan

Well,  it's not quite an 'unsupported operation' and neither does the 
toolstack user communicate an 'invalid value', he must just be noticed 
that something (the vm-event subsystem) needs to be initialised before 
the operation can be done.
But I agree ENOSYS is not acceptable either (I only now see it signifies 
"Function not implemented", my bad for not peeking at that before using 
it, I expected differently).

What about ENODEV, i.e. "No such device."? We need an error code saying 
"Device not initialised"...

Thanks,
Corneliu.

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

Re: [PATCH v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl

[Corneliu ZUZU]

On 7/6/2016 7:15 PM, Corneliu ZUZU wrote:
> On 7/6/2016 7:01 PM, Jan Beulich wrote:
>>>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote:
>>> Also adjust returned error code for similar check from -EINVAL to more
>>> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).
>> I'm not sure that's more descriptive, and we really try to not use
>> ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP
>> perhaps?
>>
>> Jan
>
> Well,  it's not quite an 'unsupported operation' and neither does the 
> toolstack user communicate an 'invalid value', he must just be noticed 
> that something (the vm-event subsystem) needs to be initialised before 
> the operation can be done.
> But I agree ENOSYS is not acceptable either (I only now see it 
> signifies "Function not implemented", my bad for not peeking at that 
> before using it, I expected differently).
>
> What about ENODEV, i.e. "No such device."? We need an error code 
> saying "Device not initialised"...
>
> Thanks,
> Corneliu.

Or ENOTCONN..?

Corneliu.

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

Re: [PATCH v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl

[Jan Beulich]

>>> On 06.07.16 at 18:15, <czuzu@bitdefender.com> wrote:
> On 7/6/2016 7:01 PM, Jan Beulich wrote:
>>>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote:
>>> Also adjust returned error code for similar check from -EINVAL to more
>>> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).
>> I'm not sure that's more descriptive, and we really try to not use
>> ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP
>> perhaps?
> 
> Well,  it's not quite an 'unsupported operation' and neither does the 
> toolstack user communicate an 'invalid value', he must just be noticed 
> that something (the vm-event subsystem) needs to be initialised before 
> the operation can be done.
> But I agree ENOSYS is not acceptable either (I only now see it signifies 
> "Function not implemented", my bad for not peeking at that before using 
> it, I expected differently).
> 
> What about ENODEV, i.e. "No such device."? We need an error code saying 
> "Device not initialised"...

Fine with me (almost anything other than ENOSYS would be).

Jan


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

Re: [PATCH v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl

[Corneliu ZUZU]

On 7/7/2016 10:30 AM, Jan Beulich wrote:
>>>> On 06.07.16 at 18:15, <czuzu@bitdefender.com> wrote:
>> On 7/6/2016 7:01 PM, Jan Beulich wrote:
>>>>>> On 06.07.16 at 17:52, <czuzu@bitdefender.com> wrote:
>>>> Also adjust returned error code for similar check from -EINVAL to more
>>>> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).
>>> I'm not sure that's more descriptive, and we really try to not use
>>> ENOSYS for other than unimplemented hypercalls. EOPNOTSUPP
>>> perhaps?
>> Well,  it's not quite an 'unsupported operation' and neither does the
>> toolstack user communicate an 'invalid value', he must just be noticed
>> that something (the vm-event subsystem) needs to be initialised before
>> the operation can be done.
>> But I agree ENOSYS is not acceptable either (I only now see it signifies
>> "Function not implemented", my bad for not peeking at that before using
>> it, I expected differently).
>>
>> What about ENODEV, i.e. "No such device."? We need an error code saying
>> "Device not initialised"...
> Fine with me (almost anything other than ENOSYS would be).
>
> Jan
>
>

Ack, thanks.

Corneliu.

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

Re: [PATCH v3 4/8] x86/vm_event/monitor/cr: check for vm-event subscriber on domctl

[Corneliu ZUZU]

On 7/6/2016 6:52 PM, Corneliu ZUZU wrote:
> Enforce presence of a monitor vm-event subscriber when the toolstack user calls
> xc_monitor_write_ctrlreg (XEN_DOMCTL_MONITOR_EVENT_WRITE_CTRLREG domctl).
> Without this change, ASSERT(v->arch.vm_event) @ hvm_set_cr0 and such would fail
> if the toolstack user calls xc_monitor_write_ctrlreg(...) w/ enable = true,
> without first calling xc_monitor_enable().
>
> Also adjust returned error code for similar check from -EINVAL to more
> descriptive -ENOSYS (XEN_DOMCTL_MONITOR_OP_EMULATE_EACH_REP).
>
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
>   xen/arch/x86/monitor.c        | 7 +++++++
>   xen/include/asm-x86/monitor.h | 2 +-
>   2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 42f31bf..4d4db33 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -188,6 +188,13 @@ int arch_monitor_domctl_event(struct domain *d,
>           unsigned int ctrlreg_bitmask;
>           bool_t old_status;
>   
> +        /*
> +         * Enabling cr-write vm-events without a vm_event subscriber is
> +         * meaningless.
> +         */
> +        if ( !vm_event_domain_initialised(d) )
> +            return -ENOSYS;
> +
>           /* sanity check: avoid left-shift undefined behavior */
>           if ( unlikely(mop->u.mov_to_cr.index > 31) )
>               return -EINVAL;
> diff --git a/xen/include/asm-x86/monitor.h b/xen/include/asm-x86/monitor.h
> index 9238ec8..2213124 100644
> --- a/xen/include/asm-x86/monitor.h
> +++ b/xen/include/asm-x86/monitor.h
> @@ -52,7 +52,7 @@ int arch_monitor_domctl_op(struct domain *d, struct xen_domctl_monitor_op *mop)
>           if ( vm_event_domain_initialised(d) )
>               d->arch.mem_access_emulate_each_rep = !!mop->event;
>           else
> -            rc = -EINVAL;
> +            rc = -ENOSYS;
>   
>           domain_unpause(d);
>           break;

But this also doesn't guarantee the ASSERT not failing, as I now 
realize, simply because with this patch v->arch.vm_event can *still be 
NULL* even with a *non-zero d->monitor.write_ctrlreg_enabled*.

Can happen for example if:
1) the toolstack user calls *xc_monitor_enable()* to enable domain 
monitoring (calls *vm_event_init_domain()* along the way)
2) the toolstack user calls *xc_monitor_write_ctrlreg()* to enable *CR0* 
exiting - this will succeed since the user *first called 
xc_monitor_enable()* - which *will result in having a non-zero 
d->monitor.write_ctrlreg_enabled*
3) the toolstack user calls *xc_mem_paging_enable() followed by 
xc_mem_paging_disable()*; this *will call 
vm_event_disable()->vm_event_cleanup_domain()* along the way which *will 
xfree v->arch.vm_event*, but _will not_ call 
*arch_monitor_cleanup_domain()* (see vm_event_domctl) which zeroes out 
d->monitor.write_ctrlreg_enabled
4) a CR0-write is intercepted after these operations which will 
ASSERT(v->arch.vm_event) in hvm_set_cr0(), which will subsequently fail

This issue happens because *vm_event_cleanup()* domain *xfrees 
arch_vm_event along with its write_data field* needed by the monitor 
subsystem, even a vm-event subsystem other than the monitor one is 
uninitialized. I'm wondering why vm_event_{init,cleanup}_domain() 
allocate/free *monitor-specific* resources (arch_vm_event fields) even 
when they're called as a result of 
XEN_DOMCTL_VM_EVENT_OP_PAGING/XEN_DOMCTL_VM_EVENT_OP_SHARING domctl.

Tamas, Razvan, this seems wrong, shouldn't most operations currently 
done in *vm_event_{init,cleanup}_domain* actually be done in 
*arch_monitor_{init,cleanup}_domain* instead?

I propose the following refactoring:

-  define:

         struct arch_vm_event_monitor {
             uint32_t emulate_flags;
             struct vm_event_emul_read_data *emul_read_data;
             struct monitor_write_data write_data;
         }

- change

         struct arch_vm_event {
             struct arch_vm_event_monitor *monitor; // monitor 
subsystem-stuff!
         }

- allocate *arch_vm_event* (but don't touch its *monitor* field) in 
*vm_event_init_domain()*

- allocate/free *arch_vm_event->monitor* in 
*arch_monitor_{init,cleanup}_domain()*, but *only free write_data if 
there are no uncommitted writes*

- call arch_monitor_cleanup_domain() *before* vm_event_cleanup_domain() 
and only free arch_vm_event in the latter if arch_vm_event->monitor was 
freed in the former

- on domain/vcpu destroyal free everything left allocated 
(arch_vm_event->monitor and arch_vm_event)

Sounds good?

Thanks,
Corneliu.

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

Re: [PATCH v3 6/8] x86/vm-event: minor ASSERT fix, add 'unlikely'

[Jan Beulich]

>>> On 06.07.16 at 17:54, <czuzu@bitdefender.com> wrote:
> --- 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;
>  

Acked-by: Jan Beulich <jbeulich@suse.com>

> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -96,14 +96,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:

I'd have preferred for you to leave alone the initializer, but we'll see
what the maintainers are going to say.

Jan


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

Re: [PATCH v3 6/8] x86/vm-event: minor ASSERT fix, add 'unlikely'

[Corneliu ZUZU]

On 7/7/2016 11:27 AM, Jan Beulich wrote:
>>>> On 06.07.16 at 17:54, <czuzu@bitdefender.com> wrote:
>> --- 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;
>>   
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -96,14 +96,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:
> I'd have preferred for you to leave alone the initializer, but we'll see
> what the maintainers are going to say.
>
> Jan

It's 'cleaner' this way, doesn't it? Not assigning a pointer to a 
possibly invalid address...
Anyway, I'm preparing a v4, I'll probably drop this change if you won't 
*subdue* to ack it (kidding).

Thanks,
Corneliu.

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

Re: [PATCH v3 6/8] x86/vm-event: minor ASSERT fix, add 'unlikely'

[Jan Beulich]

>>> On 07.07.16 at 10:35, <czuzu@bitdefender.com> wrote:
> On 7/7/2016 11:27 AM, Jan Beulich wrote:
>>>>> On 06.07.16 at 17:54, <czuzu@bitdefender.com> wrote:
>>> --- a/xen/arch/x86/vm_event.c
>>> +++ b/xen/arch/x86/vm_event.c
>>> @@ -96,14 +96,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:
>> I'd have preferred for you to leave alone the initializer, but we'll see
>> what the maintainers are going to say.
> 
> It's 'cleaner' this way, doesn't it? Not assigning a pointer to a 
> possibly invalid address...

It's a matter of taste, hence subject to the maintainers' opinions.

> Anyway, I'm preparing a v4, I'll probably drop this change if you won't 
> *subdue* to ack it (kidding).

I can't ack it anyway.

Jan


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

Re: [PATCH v3 6/8] x86/vm-event: minor ASSERT fix, add 'unlikely'

[Tamas K Lengyel]

On Wed, Jul 6, 2016 at 9:54 AM, Corneliu ZUZU <czuzu@bitdefender.com> wrote:
> 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>

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 v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Jan Beulich]

>>> On 06.07.16 at 17:50, <czuzu@bitdefender.com> wrote:

The title of this patch keeps confusing me - which code motion is
being relocated here?

> +void vmx_vm_event_update_cr3_traps(struct domain *d)

Is there anything in this function preventing the parameter to be
const?

> +{
> +    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));

Comment style.

> +    /* 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.
> +         */

Same as for the title - what code motion is this referring to? In a
code comment you clearly shouldn't be referring to anything the
patch effects, only to its result.

> +        if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
> +            continue;

The first sentence of the comment should be brought in line with
this condition.

> +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index)

Unless there is a particular reason for this uint8_t, please convert to
unsigned int.

> +{
> +    /* vmx only */
> +    ASSERT(cpu_has_vmx);

Comment style (more below). Should perhaps also get "for now" or
some such added.

> +static inline void write_ctrlreg_disable_traps(struct domain *d)
> +{
> +    unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
> +    d->arch.monitor.write_ctrlreg_enabled = 0;
> +
> +    if ( old )
> +    {
> +        /* vmx only */
> +        ASSERT(cpu_has_vmx);

Wouldn't this better move ahead of the if()?

> +        /* was CR3 load-exiting enabled due to monitoring? */
> +        if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )

And then this if() alone would suffice.

> @@ -36,11 +63,40 @@ int arch_monitor_init_domain(struct domain *d)
>  void arch_monitor_cleanup_domain(struct domain *d)
>  {
>      xfree(d->arch.monitor.msr_bitmap);
> -
> +    write_ctrlreg_disable_traps(d);
>      memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
>      memset(&d->monitor, 0, sizeof(d->monitor));
>  }

Rather than deleting the blank line, perhaps better to insert another
one after your addition?

Jan


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

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

[Jan Beulich]

>>> On 06.07.16 at 17:51, <czuzu@bitdefender.com> wrote:
> @@ -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;
> +    }

Considering the repeat of this pattern ...

> @@ -52,8 +58,25 @@ 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;
> +
> +        /*
> +         * Only xfree the entire arch_vm_event if write_data was fully handled.
> +         * Otherwise defer entire xfree until domain/vcpu destroyal.
> +         */
> +        if ( likely(!v->arch.vm_event->write_data.uncommitted_writes) )
> +        {
> +            xfree(v->arch.vm_event->emul_read_data);
> +            xfree(v->arch.vm_event);
> +            v->arch.vm_event = NULL;
> +            continue;
> +        }

... here, please consider making this another helper (inline?) function.

> +        /* write_data not fully handled, only xfree emul_read_data */

Comment style again (and more below).

> --- 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,16 @@ 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) )
> +        {
> +            /* vm_event->emul_read_data freed in vm_event_cleanup_domain */

Perhaps worthwhile adding a respective ASSERT()?

> +static inline bool_t vm_event_vcpu_initialised(struct vcpu *v)
> +{
> +    return (v->arch.vm_event && v->arch.vm_event->emul_read_data);
> +}
> +
> +static inline bool_t vm_event_domain_initialised(struct domain *d)
> +{
> +    return (d->max_vcpus && d->vcpu[0] &&
> +            vm_event_vcpu_initialised(d->vcpu[0]));
> +}

Both functions' parameters should be const. Pointless parentheses
in both return statements.

Jan


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

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

[Jan Beulich]

>>> On 06.07.16 at 17:55, <czuzu@bitdefender.com> wrote:
> --- 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>

The inclusion of asm/vm_event.h should really be removed in patch 2,
I had to drop it here, to get the patch to apply ahead of the earlier
ones in this series.

> --- 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>
>  
>  static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index)

Similarly I had to drop this hunk, which suggests that one of the
earlier patches needlessly adds that #include (or it gets validly
added and then a later patch makes it unnecessary again).

Jan


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

Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Corneliu ZUZU]

On 7/8/2016 10:21 AM, Jan Beulich wrote:
>>>> On 06.07.16 at 17:50, <czuzu@bitdefender.com> wrote:
> The title of this patch keeps confusing me - which code motion is
> being relocated here?

As the commit message clearly states, the code motions that are being 
relocated are:
1) handling of monitor_write_data @ hvm_do_resume
2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting 
CR3 load-exiting for cr-write monitor vm-events, i.e. the comment:
     /* Trap CR3 updates if CR3 memory events are enabled. */
and what's removed from under it.

By 'relocation' I meant making that code vm-event specific (moving it to 
vm-event specific files).

>
>> +void vmx_vm_event_update_cr3_traps(struct domain *d)
> Is there anything in this function preventing the parameter to be
> const?

Nope, will do.

>> +{
>> +    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));
> Comment style.

As in change to "/* Domain must be paused. */"?

>
>> +    /* 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.
>> +         */
> Same as for the title - what code motion is this referring to? In a
> code comment you clearly shouldn't be referring to anything the
> patch effects, only to its result.

The "vmx_update_guest_cr code motion for cr = 0", that's what's 
referring to.
'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters.
In other words, see what's happening in the function 
'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand 
why CR3 load-exiting must remain enabled when CR0.PE=0.

>
>> +        if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
>> +            continue;
> The first sentence of the comment should be brought in line with
> this condition.

Would this do (aligned with the above observation):

"

         /*
          * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
          * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
          */

"
?

>> +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index)
> Unless there is a particular reason for this uint8_t, please convert to
> unsigned int.

The particular reason is cr-indexes being uint8_t typed (see 
typeof(xen_domctl_monitor_op.mov_to_cr.index)).
But I will change it to unsigned int if you prefer (maybe you could 
explain the preference though).

>> +{
>> +    /* vmx only */
>> +    ASSERT(cpu_has_vmx);
> Comment style (more below). Should perhaps also get "for now" or
> some such added.

As in "/* For now, VMX only. */"?

>
>> +static inline void write_ctrlreg_disable_traps(struct domain *d)
>> +{
>> +    unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
>> +    d->arch.monitor.write_ctrlreg_enabled = 0;
>> +
>> +    if ( old )
>> +    {
>> +        /* vmx only */
>> +        ASSERT(cpu_has_vmx);
> Wouldn't this better move ahead of the if()?
>
>> +        /* was CR3 load-exiting enabled due to monitoring? */
>> +        if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
> And then this if() alone would suffice.

No, it would be wrong because that ASSERT may not hold if "old == 0", 
i.e. we only ASSERT the implication "CR-write vm-events can be enabled 
-> vmx domain", but since the function is called by 
arch_monitor_cleanup_domain, putting the ASSERT before the if() would 
change that implication to "(any) monitor vm-events available -> vmx 
domain", assertion which wouldn't be proper TBD here.

>
>> @@ -36,11 +63,40 @@ int arch_monitor_init_domain(struct domain *d)
>>   void arch_monitor_cleanup_domain(struct domain *d)
>>   {
>>       xfree(d->arch.monitor.msr_bitmap);
>> -
>> +    write_ctrlreg_disable_traps(d);
>>       memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
>>       memset(&d->monitor, 0, sizeof(d->monitor));
>>   }
> Rather than deleting the blank line, perhaps better to insert another
> one after your addition?
>
> Jan

Ack.

Thanks,
Corneliu.

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

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

[Corneliu ZUZU]

On 7/8/2016 10:35 AM, Jan Beulich wrote:
>>>> On 06.07.16 at 17:51, <czuzu@bitdefender.com> wrote:
>> @@ -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;
>> +    }
> Considering the repeat of this pattern ...
>
>> @@ -52,8 +58,25 @@ 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;
>> +
>> +        /*
>> +         * Only xfree the entire arch_vm_event if write_data was fully handled.
>> +         * Otherwise defer entire xfree until domain/vcpu destroyal.
>> +         */
>> +        if ( likely(!v->arch.vm_event->write_data.uncommitted_writes) )
>> +        {
>> +            xfree(v->arch.vm_event->emul_read_data);
>> +            xfree(v->arch.vm_event);
>> +            v->arch.vm_event = NULL;
>> +            continue;
>> +        }
> ... here, please consider making this another helper (inline?) function.

Yeah, I'm sending a separate patch today which will invalidate some of 
these changes (then a v4 above that one).

>
>> +        /* write_data not fully handled, only xfree emul_read_data */
> Comment style again (and more below).

Ack, assuming you mean 'capital letter, end with dot'.

>
>> --- 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,16 @@ 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) )
>> +        {
>> +            /* vm_event->emul_read_data freed in vm_event_cleanup_domain */
> Perhaps worthwhile adding a respective ASSERT()?

Good point, ack.

>
>> +static inline bool_t vm_event_vcpu_initialised(struct vcpu *v)
>> +{
>> +    return (v->arch.vm_event && v->arch.vm_event->emul_read_data);
>> +}
>> +
>> +static inline bool_t vm_event_domain_initialised(struct domain *d)
>> +{
>> +    return (d->max_vcpus && d->vcpu[0] &&
>> +            vm_event_vcpu_initialised(d->vcpu[0]));
>> +}
> Both functions' parameters should be const. Pointless parentheses
> in both return statements.
>
> Jan

Ack (although the parenthesis were there strictly for aesthetics, but 
that's subjective).

Thanks,
Corneliu.

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

Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Jan Beulich]

>>> On 08.07.16 at 12:22, <czuzu@bitdefender.com> wrote:
> On 7/8/2016 10:21 AM, Jan Beulich wrote:
>>>>> On 06.07.16 at 17:50, <czuzu@bitdefender.com> wrote:
>> The title of this patch keeps confusing me - which code motion is
>> being relocated here?
> 
> As the commit message clearly states, the code motions that are being 
> relocated are:

Again this sentence makes no sense to me: I can't see how
"code motions" can be "relocated", just like I don't see how you
could move a move. But maybe it's just me...

> 1) handling of monitor_write_data @ hvm_do_resume
> 2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting 
> CR3 load-exiting for cr-write monitor vm-events, i.e. the comment:
>      /* Trap CR3 updates if CR3 memory events are enabled. */
> and what's removed from under it.
> 
> By 'relocation' I meant making that code vm-event specific (moving it to 
> vm-event specific files).

Yes, that what I've guessed.

>>> +{
>>> +    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));
>> Comment style.
> 
> As in change to "/* Domain must be paused. */"?

Yes, as mandated by ./CODING_STYLE.

>>> +    /* 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.
>>> +         */
>> Same as for the title - what code motion is this referring to? In a
>> code comment you clearly shouldn't be referring to anything the
>> patch effects, only to its result.
> 
> The "vmx_update_guest_cr code motion for cr = 0", that's what's 
> referring to.

So I guess my problem really is that I don't understand what a
"code motion" is (other than the act of moving code from one
place to another).

> 'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters.
> In other words, see what's happening in the function 
> 'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand 
> why CR3 load-exiting must remain enabled when CR0.PE=0.
> 
>>
>>> +        if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
>>> +            continue;
>> The first sentence of the comment should be brought in line with
>> this condition.
> 
> Would this do (aligned with the above observation):
> 
> "
> 
>          /*
>           * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
>           * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
>           */
> 
> "
> ?

Not really: The condition checks whether paging is enabled and
whether it's an unrestricted guest. The comment talks about
protected mode being enabled.

>>> +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t 
> index)
>> Unless there is a particular reason for this uint8_t, please convert to
>> unsigned int.
> 
> The particular reason is cr-indexes being uint8_t typed (see 
> typeof(xen_domctl_monitor_op.mov_to_cr.index)).
> But I will change it to unsigned int if you prefer (maybe you could 
> explain the preference though).

No use of fixed width types when fixed width types aren't really
required. Generally generated code is less efficient when having
to deal with fixed width types.

>>> +{
>>> +    /* vmx only */
>>> +    ASSERT(cpu_has_vmx);
>> Comment style (more below). Should perhaps also get "for now" or
>> some such added.
> 
> As in "/* For now, VMX only. */"?

For example, yes.

>>> +static inline void write_ctrlreg_disable_traps(struct domain *d)
>>> +{
>>> +    unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
>>> +    d->arch.monitor.write_ctrlreg_enabled = 0;
>>> +
>>> +    if ( old )
>>> +    {
>>> +        /* vmx only */
>>> +        ASSERT(cpu_has_vmx);
>> Wouldn't this better move ahead of the if()?
>>
>>> +        /* was CR3 load-exiting enabled due to monitoring? */
>>> +        if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
>> And then this if() alone would suffice.
> 
> No, it would be wrong because that ASSERT may not hold if "old == 0", 
> i.e. we only ASSERT the implication "CR-write vm-events can be enabled 
> -> vmx domain", but since the function is called by 
> arch_monitor_cleanup_domain, putting the ASSERT before the if() would 
> change that implication to "(any) monitor vm-events available -> vmx 
> domain", assertion which wouldn't be proper TBD here.

Ah, okay - I was under the impression that no VM events were
allowed under SVM.

Jan

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

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

[Corneliu ZUZU]

On 7/8/2016 10:56 AM, Jan Beulich wrote:
>>>> On 06.07.16 at 17:55, <czuzu@bitdefender.com> wrote:
>> --- 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>
> The inclusion of asm/vm_event.h should really be removed in patch 2,
> I had to drop it here, to get the patch to apply ahead of the earlier
> ones in this series.
>
>> --- 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>
>>   
>>   static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t index)
> Similarly I had to drop this hunk, which suggests that one of the
> earlier patches needlessly adds that #include (or it gets validly
> added and then a later patch makes it unnecessary again).
>
> Jan

Thanks for applying these ahead, it spares me having to include them in 
future series.
As for the issue, yeah that's probably what happened along the way 
although it was to be expected (applying patches from a series 
'unsequentially') so again, thanks for the effort.
Will keep in mind having to take care of the changes that didn't make it 
in a next series.

Corneliu.

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

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

[Jan Beulich]

>>> On 08.07.16 at 12:28, <czuzu@bitdefender.com> wrote:
> On 7/8/2016 10:35 AM, Jan Beulich wrote:
>>>>> On 06.07.16 at 17:51, <czuzu@bitdefender.com> wrote:
>>> +        /* write_data not fully handled, only xfree emul_read_data */
>> Comment style again (and more below).
> 
> Ack, assuming you mean 'capital letter, end with dot'.

Actually in this particular case I meant only the missing full stop,
as the first word - afaict - refers to a structure field name (and
hence should remain lower case so e.g. a grep would catch it).

Jan


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

Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Corneliu ZUZU]

On 7/8/2016 1:37 PM, Jan Beulich wrote:
>>>> On 08.07.16 at 12:22, <czuzu@bitdefender.com> wrote:
>> On 7/8/2016 10:21 AM, Jan Beulich wrote:
>>>>>> On 06.07.16 at 17:50, <czuzu@bitdefender.com> wrote:
>>> The title of this patch keeps confusing me - which code motion is
>>> being relocated here?
>> As the commit message clearly states, the code motions that are being
>> relocated are:
> Again this sentence makes no sense to me: I can't see how
> "code motions" can be "relocated", just like I don't see how you
> could move a move. But maybe it's just me...

Hah, sorry, I'm not very good expressivity-wise, a weakness I'm aware of 
and which makes me pick up expressions I notice other people use (in 
this case those of maintainers).
I think you were the one I noticed using the expression back in an older 
patch-series I've sent and I thought by "code-motion" you meant simply 
"that which some code does, tries to accomplish and the code itself".

>> 1) handling of monitor_write_data @ hvm_do_resume
>> 2) the code in vmx_update_guest_cr (when cr = 0) that deals with setting
>> CR3 load-exiting for cr-write monitor vm-events, i.e. the comment:
>>       /* Trap CR3 updates if CR3 memory events are enabled. */
>> and what's removed from under it.
>>
>> By 'relocation' I meant making that code vm-event specific (moving it to
>> vm-event specific files).
> Yes, that what I've guessed.
>
>>>> +{
>>>> +    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));
>>> Comment style.
>> As in change to "/* Domain must be paused. */"?
> Yes, as mandated by ./CODING_STYLE.
>
>>>> +    /* 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.
>>>> +         */
>>> Same as for the title - what code motion is this referring to? In a
>>> code comment you clearly shouldn't be referring to anything the
>>> patch effects, only to its result.
>> The "vmx_update_guest_cr code motion for cr = 0", that's what's
>> referring to.
> So I guess my problem really is that I don't understand what a
> "code motion" is (other than the act of moving code from one
> place to another).

Again, sorry, will try to rephrase all of this properly :-).

>
>> 'vmx_update_guest_cr()' is a function, 'cr' is one of its parameters.
>> In other words, see what's happening in the function
>> 'vmx_update_guest_cr() when you pass it cr = 0' and you'll understand
>> why CR3 load-exiting must remain enabled when CR0.PE=0.
>>
>>>> +        if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
>>>> +            continue;
>>> The first sentence of the comment should be brought in line with
>>> this condition.
>> Would this do (aligned with the above observation):
>>
>> "
>>
>>           /*
>>            * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
>>            * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
>>            */
>>
>> "
>> ?
> Not really: The condition checks whether paging is enabled and
> whether it's an unrestricted guest. The comment talks about
> protected mode being enabled.

Hah you're right, I only now notice, that comment has actually been 
adopted (although I don't remember from where, I wonder if it was 
meantime removed and I only now see), I always thought it said "CR0.PG = 
0"...
So...
"

         /*
          * If domain paging is disabled (CR0.PG=0) and
          * the domain is not in real-mode, then CR3 load-exiting
          * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0).
          */
"
?

>>>> +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t
>> index)
>>> Unless there is a particular reason for this uint8_t, please convert to
>>> unsigned int.
>> The particular reason is cr-indexes being uint8_t typed (see
>> typeof(xen_domctl_monitor_op.mov_to_cr.index)).
>> But I will change it to unsigned int if you prefer (maybe you could
>> explain the preference though).
> No use of fixed width types when fixed width types aren't really
> required. Generally generated code is less efficient when having
> to deal with fixed width types.

Strange, I would have thought the compiler would properly (and easily) 
deal with such efficiency issues.

>>>> +{
>>>> +    /* vmx only */
>>>> +    ASSERT(cpu_has_vmx);
>>> Comment style (more below). Should perhaps also get "for now" or
>>> some such added.
>> As in "/* For now, VMX only. */"?
> For example, yes.
>
>>>> +static inline void write_ctrlreg_disable_traps(struct domain *d)
>>>> +{
>>>> +    unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
>>>> +    d->arch.monitor.write_ctrlreg_enabled = 0;
>>>> +
>>>> +    if ( old )
>>>> +    {
>>>> +        /* vmx only */
>>>> +        ASSERT(cpu_has_vmx);
>>> Wouldn't this better move ahead of the if()?
>>>
>>>> +        /* was CR3 load-exiting enabled due to monitoring? */
>>>> +        if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
>>> And then this if() alone would suffice.
>> No, it would be wrong because that ASSERT may not hold if "old == 0",
>> i.e. we only ASSERT the implication "CR-write vm-events can be enabled
>> -> vmx domain", but since the function is called by
>> arch_monitor_cleanup_domain, putting the ASSERT before the if() would
>> change that implication to "(any) monitor vm-events available -> vmx
>> domain", assertion which wouldn't be proper TBD here.
> Ah, okay - I was under the impression that no VM events were
> allowed under SVM.
>
> Jan

Thanks,
Corneliu.

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

Re: [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization

[Corneliu ZUZU]

On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
> 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 v2: <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) )

Jan,

I'm wondering if you could ack this as well (if there's nothing wrong w/ 
it) and push it to be done with it. :-)

Thanks,
Corneliu.

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

Re: [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization

[Jan Beulich]

>>> On 08.07.16 at 13:39, <czuzu@bitdefender.com> wrote:
> On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
>> 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 v2: <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) )
> 
> I'm wondering if you could ack this as well (if there's nothing wrong w/ 
> it) and push it to be done with it. :-)

Well, if you had pinged the patch at least once, I probably would.
I don't, however, recall having seen any such ping (only resends).

Jan


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

Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Jan Beulich]

>>> On 08.07.16 at 13:33, <czuzu@bitdefender.com> wrote:
> On 7/8/2016 1:37 PM, Jan Beulich wrote:
>>>>> On 08.07.16 at 12:22, <czuzu@bitdefender.com> wrote:
>>> On 7/8/2016 10:21 AM, Jan Beulich wrote:
>>>>>>> On 06.07.16 at 17:50, <czuzu@bitdefender.com> wrote:
>>>>> +        /*
>>>>> +         * 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;
>>>> The first sentence of the comment should be brought in line with
>>>> this condition.
>>> Would this do (aligned with the above observation):
>>>
>>> "
>>>
>>>           /*
>>>            * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
>>>            * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
>>>            */
>>>
>>> "
>>> ?
>> Not really: The condition checks whether paging is enabled and
>> whether it's an unrestricted guest. The comment talks about
>> protected mode being enabled.
> 
> Hah you're right, I only now notice, that comment has actually been 
> adopted (although I don't remember from where, I wonder if it was 
> meantime removed and I only now see), I always thought it said "CR0.PG = 
> 0"...
> So...
> "
> 
>          /*
>           * If domain paging is disabled (CR0.PG=0) and
>           * the domain is not in real-mode, then CR3 load-exiting
>           * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0).
>           */
> "
> ?

Looks reasonable.

>>>>> +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t
>>> index)
>>>> Unless there is a particular reason for this uint8_t, please convert to
>>>> unsigned int.
>>> The particular reason is cr-indexes being uint8_t typed (see
>>> typeof(xen_domctl_monitor_op.mov_to_cr.index)).
>>> But I will change it to unsigned int if you prefer (maybe you could
>>> explain the preference though).
>> No use of fixed width types when fixed width types aren't really
>> required. Generally generated code is less efficient when having
>> to deal with fixed width types.
> 
> Strange, I would have thought the compiler would properly (and easily) 
> deal with such efficiency issues.

In this case the compiler may well do (as the function is static
inline), but in other cases it's simply not allowed to. In order to
not misguide people cloning existing code we should thus try to
limit the number of bad examples (which in particular mean not
introducing any new ones).

Jan


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

Re: [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization

[Corneliu ZUZU]

On 7/8/2016 2:48 PM, Jan Beulich wrote:
>>>> On 08.07.16 at 13:39, <czuzu@bitdefender.com> wrote:
>> On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
>>> 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 v2: <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) )
>> I'm wondering if you could ack this as well (if there's nothing wrong w/
>> it) and push it to be done with it. :-)
> Well, if you had pinged the patch at least once, I probably would.
> I don't, however, recall having seen any such ping (only resends).
>
> Jan

So is a 'ping' (haven't done that before) still necessary? Is a ping a 
simple 'reply-to-all' including 'Ping: ' in the subject? :-)

Thanks,
Corneliu.

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

Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Corneliu ZUZU]

On 7/8/2016 2:53 PM, Jan Beulich wrote:
>>>> On 08.07.16 at 13:33, <czuzu@bitdefender.com> wrote:
>> On 7/8/2016 1:37 PM, Jan Beulich wrote:
>>>>>> On 08.07.16 at 12:22, <czuzu@bitdefender.com> wrote:
>>>> On 7/8/2016 10:21 AM, Jan Beulich wrote:
>>>>>>>> On 06.07.16 at 17:50, <czuzu@bitdefender.com> wrote:
>>>>>> +        /*
>>>>>> +         * 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;
>>>>> The first sentence of the comment should be brought in line with
>>>>> this condition.
>>>> Would this do (aligned with the above observation):
>>>>
>>>> "
>>>>
>>>>            /*
>>>>             * If CR3 load-exiting was enabled and CR0.PE=0, then it must remain
>>>>             * enabled (see vmx_update_guest_cr(v, cr) function when cr = 0).
>>>>             */
>>>>
>>>> "
>>>> ?
>>> Not really: The condition checks whether paging is enabled and
>>> whether it's an unrestricted guest. The comment talks about
>>> protected mode being enabled.
>> Hah you're right, I only now notice, that comment has actually been
>> adopted (although I don't remember from where, I wonder if it was
>> meantime removed and I only now see), I always thought it said "CR0.PG =
>> 0"...
>> So...
>> "
>>
>>           /*
>>            * If domain paging is disabled (CR0.PG=0) and
>>            * the domain is not in real-mode, then CR3 load-exiting
>>            * must remain enabled (see vmx_update_guest_cr(v, cr) when cr = 0).
>>            */
>> "
>> ?
> Looks reasonable.
>
>>>>>> +static inline void write_ctrlreg_adjust_traps(struct domain *d, uint8_t
>>>> index)
>>>>> Unless there is a particular reason for this uint8_t, please convert to
>>>>> unsigned int.
>>>> The particular reason is cr-indexes being uint8_t typed (see
>>>> typeof(xen_domctl_monitor_op.mov_to_cr.index)).
>>>> But I will change it to unsigned int if you prefer (maybe you could
>>>> explain the preference though).
>>> No use of fixed width types when fixed width types aren't really
>>> required. Generally generated code is less efficient when having
>>> to deal with fixed width types.
>> Strange, I would have thought the compiler would properly (and easily)
>> deal with such efficiency issues.
> In this case the compiler may well do (as the function is static
> inline), but in other cases it's simply not allowed to. In order to
> not misguide people cloning existing code we should thus try to
> limit the number of bad examples (which in particular mean not
> introducing any new ones).
>
> Jan

Ack.

Corneliu.

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

Re: [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization

[Jan Beulich]

>>> On 08.07.16 at 13:55, <czuzu@bitdefender.com> wrote:
> On 7/8/2016 2:48 PM, Jan Beulich wrote:
>>>>> On 08.07.16 at 13:39, <czuzu@bitdefender.com> wrote:
>>> On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
>>>> 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 v2: <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) )
>>> I'm wondering if you could ack this as well (if there's nothing wrong w/
>>> it) and push it to be done with it. :-)
>> Well, if you had pinged the patch at least once, I probably would.
>> I don't, however, recall having seen any such ping (only resends).
> 
> So is a 'ping' (haven't done that before) still necessary? Is a ping a 
> simple 'reply-to-all' including 'Ping: ' in the subject? :-)

It's not very much formalized. What I do in such a case is, as you
say, prefix the subject with Ping:, but change addressing so that
the people expected to reply end up in To:, while everyone else in
the original To: list (i.e. including xen-devel) would get moved to
Cc:.

And while, with the adjustment to addressees in my earlier reply, I
already tried to do kind of a ping to them, I think it wouldn't hurt if
you did another explicit one.

Jan


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

Ping: [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization

[Corneliu ZUZU]

On 7/6/2016 6:49 PM, Corneliu ZUZU wrote:
> 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 v2: <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) )



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

Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Tamas K Lengyel]

> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
> index 205df41..42f31bf 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c

[snip]

So with this code-portion appropriately being relocated here I think
we should also rename..

> +void arch_monitor_write_data(struct vcpu *v)
> +{
> +    struct monitor_write_data *w = &v->arch.vm_event->write_data;

.. arch.vm_event to arch.monitor to match the subsystem it is used by.

Thanks,
Tamas

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

Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Corneliu ZUZU]

On 7/8/2016 6:50 PM, Tamas K Lengyel wrote:
>> diff --git a/xen/arch/x86/monitor.c b/xen/arch/x86/monitor.c
>> index 205df41..42f31bf 100644
>> --- a/xen/arch/x86/monitor.c
>> +++ b/xen/arch/x86/monitor.c
> [snip]
>
> So with this code-portion appropriately being relocated here I think
> we should also rename..
>
>> +void arch_monitor_write_data(struct vcpu *v)
>> +{
>> +    struct monitor_write_data *w = &v->arch.vm_event->write_data;
> .. arch.vm_event to arch.monitor to match the subsystem it is used by.
>
> Thanks,
> Tamas

Which is (almost) exactly what I've done in (another..) patch-series 
I'll be sending today I hope (and a few more things). ;-)

Thanks,
Corneliu.

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

Re: [PATCH v3 1/8] x86/vmx_update_guest_cr: minor optimization

[Tian, Kevin]

> From: Corneliu ZUZU [mailto:czuzu@bitdefender.com]
> Sent: Wednesday, July 06, 2016 11:50 PM
> 
> 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>

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

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

Re: [PATCH v3 2/8] x86/vm-event/monitor: relocate code-motion more appropriately

[Tian, Kevin]

> From: Corneliu ZUZU [mailto:czuzu@bitdefender.com]
> Sent: Wednesday, July 06, 2016 11:51 PM
> 
> 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
> functions write_ctrlreg_adjust_traps and write_ctrlreg_disable_traps (in
> x86/monitor.c) which deal with setting/unsetting any traps for cr-write monitor
> vm-events.
> 
> Signed-off-by: Corneliu ZUZU <czuzu@bitdefender.com>
> ---
> Changed since v2:
>   * add write_ctrlreg_disable_traps, call from arch_monitor_cleanup_domain
> ---
>  xen/arch/x86/hvm/hvm.c            | 46 ++++++++++-----------------
>  xen/arch/x86/hvm/vmx/vmx.c        | 58
> +++++++++++++++++++++++++++++++---
>  xen/arch/x86/monitor.c            | 66
> ++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-x86/hvm/vmx/vmx.h |  1 +
>  xen/include/asm-x86/monitor.h     |  2 ++
>  5 files changed, 131 insertions(+), 42 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;

better move to inner later loop.

> +    unsigned int cr3_bitmask;
> +    bool_t cr3_vmevent, cr3_ldexit;
> +
> +    /* domain must be paused */
> +    ASSERT(atomic_read(&d->pause_count));

The comment doesn't add more info than the code itself, unless you want to
explain the reason why domain must be paused here.

> +
> +    /* 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.
> +         */

Please give the real informative message here instead of asking people to 
look at other place

> +        if ( cr3_ldexit && !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
> +            continue;

if !cr3_ldexit is not expected to occur in such scenario, is it more strict as below?

if ( !hvm_paging_enabled(v) && !vmx_unrestricted_guest(v) )
{
	ASSERT(cr3_ldexit);
	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..42f31bf 100644
> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -19,9 +19,36 @@
>   * 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>
> 
> +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 ) /* other CRs are always trapped */
> +        vmx_vm_event_update_cr3_traps(d);

Please add above into a hvm function instead of directly calling
vmx in common file. (other arch can have it unimplemented).
Then you don't need change this common code again later when
other archs are added

> +}
> +
> +static inline void write_ctrlreg_disable_traps(struct domain *d)
> +{
> +    unsigned int old = d->arch.monitor.write_ctrlreg_enabled;
> +    d->arch.monitor.write_ctrlreg_enabled = 0;
> +
> +    if ( old )
> +    {
> +        /* vmx only */
> +        ASSERT(cpu_has_vmx);
> +
> +        /* was CR3 load-exiting enabled due to monitoring? */
> +        if ( old & monitor_ctrlreg_bitmask(VM_EVENT_X86_CR3) )
> +            vmx_vm_event_update_cr3_traps(d);
> +    }
> +}
> +
>  int arch_monitor_init_domain(struct domain *d)
>  {
>      if ( !d->arch.monitor.msr_bitmap )
> @@ -36,11 +63,40 @@ int arch_monitor_init_domain(struct domain *d)
>  void arch_monitor_cleanup_domain(struct domain *d)
>  {
>      xfree(d->arch.monitor.msr_bitmap);
> -
> +    write_ctrlreg_disable_traps(d);
>      memset(&d->arch.monitor, 0, sizeof(d->arch.monitor));
>      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);
> @@ -159,13 +215,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