[PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Alexandru Isaila]

The patch splits the vm_event into three structures:vm_event_share,
vm_event_paging, vm_event_monitor. The allocation for the
structure is moved to vm_event_enable so that it can be
allocated/init when needed and freed in vm_event_disable.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/arm/mem_access.c     |   2 +-
 xen/arch/x86/mm/mem_access.c  |   2 +-
 xen/arch/x86/mm/mem_paging.c  |   2 +-
 xen/arch/x86/mm/mem_sharing.c |   4 +-
 xen/arch/x86/mm/p2m.c         |  10 +--
 xen/common/domain.c           |  13 ++--
 xen/common/mem_access.c       |   3 +-
 xen/common/monitor.c          |   4 +-
 xen/common/vm_event.c         | 153 +++++++++++++++++++++++++-----------------
 xen/drivers/passthrough/pci.c |   4 +-
 xen/include/xen/sched.h       |  18 ++---
 11 files changed, 123 insertions(+), 92 deletions(-)

diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index e0888bb..a7f0cae 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -256,7 +256,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
     }
 
     /* Otherwise, check if there is a vm_event monitor subscriber */
-    if ( !vm_event_check_ring(&v->domain->vm_event->monitor) )
+    if ( !vm_event_check_ring(v->domain->vm_event_monitor) )
     {
         /* No listener */
         if ( p2m->access_required )
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 5adaf6d..414e38f 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -179,7 +179,7 @@ bool_t p2m_mem_access_check(paddr_t gpa, unsigned long gla,
     gfn_unlock(p2m, gfn, 0);
 
     /* Otherwise, check if there is a memory event listener, and send the message along */
-    if ( !vm_event_check_ring(&d->vm_event->monitor) || !req_ptr )
+    if ( !vm_event_check_ring(d->vm_event_monitor) || !req_ptr )
     {
         /* No listener */
         if ( p2m->access_required )
diff --git a/xen/arch/x86/mm/mem_paging.c b/xen/arch/x86/mm/mem_paging.c
index a049e0d..20214ac 100644
--- a/xen/arch/x86/mm/mem_paging.c
+++ b/xen/arch/x86/mm/mem_paging.c
@@ -43,7 +43,7 @@ int mem_paging_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg)
         goto out;
 
     rc = -ENODEV;
-    if ( unlikely(!d->vm_event->paging.ring_page) )
+    if ( !d->vm_event_paging || unlikely(!d->vm_event_paging->ring_page) )
         goto out;
 
     switch( mpo.op )
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 1f20ce7..12fb9cc 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -563,7 +563,7 @@ int mem_sharing_notify_enomem(struct domain *d, unsigned long gfn,
     };
 
     if ( (rc = __vm_event_claim_slot(d, 
-                        &d->vm_event->share, allow_sleep)) < 0 )
+                        d->vm_event_share, allow_sleep)) < 0 )
         return rc;
 
     if ( v->domain == d )
@@ -572,7 +572,7 @@ int mem_sharing_notify_enomem(struct domain *d, unsigned long gfn,
         vm_event_vcpu_pause(v);
     }
 
-    vm_event_put_request(d, &d->vm_event->share, &req);
+    vm_event_put_request(d, d->vm_event_share, &req);
 
     return 0;
 }
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index e8a57d1..6ae23be 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1454,7 +1454,7 @@ void p2m_mem_paging_drop_page(struct domain *d, unsigned long gfn,
      * correctness of the guest execution at this point.  If this is the only
      * page that happens to be paged-out, we'll be okay..  but it's likely the
      * guest will crash shortly anyways. */
-    int rc = vm_event_claim_slot(d, &d->vm_event->paging);
+    int rc = vm_event_claim_slot(d, d->vm_event_paging);
     if ( rc < 0 )
         return;
 
@@ -1468,7 +1468,7 @@ void p2m_mem_paging_drop_page(struct domain *d, unsigned long gfn,
         /* Evict will fail now, tag this request for pager */
         req.u.mem_paging.flags |= MEM_PAGING_EVICT_FAIL;
 
-    vm_event_put_request(d, &d->vm_event->paging, &req);
+    vm_event_put_request(d, d->vm_event_paging, &req);
 }
 
 /**
@@ -1505,7 +1505,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn)
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
 
     /* We're paging. There should be a ring */
-    int rc = vm_event_claim_slot(d, &d->vm_event->paging);
+    int rc = vm_event_claim_slot(d, d->vm_event_paging);
     if ( rc == -ENOSYS )
     {
         gdprintk(XENLOG_ERR, "Domain %hu paging gfn %lx yet no ring "
@@ -1543,7 +1543,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn)
     else if ( p2mt != p2m_ram_paging_out && p2mt != p2m_ram_paged )
     {
         /* gfn is already on its way back and vcpu is not paused */
-        vm_event_cancel_slot(d, &d->vm_event->paging);
+        vm_event_cancel_slot(d, d->vm_event_paging);
         return;
     }
 
@@ -1551,7 +1551,7 @@ void p2m_mem_paging_populate(struct domain *d, unsigned long gfn)
     req.u.mem_paging.p2mt = p2mt;
     req.vcpu_id = v->vcpu_id;
 
-    vm_event_put_request(d, &d->vm_event->paging, &req);
+    vm_event_put_request(d, d->vm_event_paging, &req);
 }
 
 /**
diff --git a/xen/common/domain.c b/xen/common/domain.c
index b22aacc..30f507b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -363,9 +363,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
         poolid = 0;
 
         err = -ENOMEM;
-        d->vm_event = xzalloc(struct vm_event_per_domain);
-        if ( !d->vm_event )
-            goto fail;
 
         d->pbuf = xzalloc_array(char, DOMAIN_PBUF_SIZE);
         if ( !d->pbuf )
@@ -403,7 +400,6 @@ struct domain *domain_create(domid_t domid, unsigned int domcr_flags,
     if ( hardware_domain == d )
         hardware_domain = old_hwdom;
     atomic_set(&d->refcnt, DOMAIN_DESTROYED);
-    xfree(d->vm_event);
     xfree(d->pbuf);
     if ( init_status & INIT_arch )
         arch_domain_destroy(d);
@@ -820,7 +816,14 @@ static void complete_domain_destroy(struct rcu_head *head)
     free_xenoprof_pages(d);
 #endif
 
-    xfree(d->vm_event);
+#ifdef CONFIG_HAS_MEM_PAGING
+    xfree(d->vm_event_paging);
+#endif
+    xfree(d->vm_event_monitor);
+#ifdef CONFIG_HAS_MEM_SHARING
+    xfree(d->vm_event_share);
+#endif
+
     xfree(d->pbuf);
 
     for ( i = d->max_vcpus - 1; i >= 0; i-- )
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 19f63bb..2712b2b 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -51,8 +51,7 @@ int mem_access_memop(unsigned long cmd,
     if ( rc )
         goto out;
 
-    rc = -ENODEV;
-    if ( unlikely(!d->vm_event->monitor.ring_page) )
+    if ( !d->vm_event_monitor || unlikely(!d->vm_event_monitor->ring_page) )
         goto out;
 
     switch ( mao.op )
diff --git a/xen/common/monitor.c b/xen/common/monitor.c
index 451f42f..70d38d4 100644
--- a/xen/common/monitor.c
+++ b/xen/common/monitor.c
@@ -92,7 +92,7 @@ int monitor_traps(struct vcpu *v, bool_t sync, vm_event_request_t *req)
     int rc;
     struct domain *d = v->domain;
 
-    rc = vm_event_claim_slot(d, &d->vm_event->monitor);
+    rc = vm_event_claim_slot(d, d->vm_event_monitor);
     switch ( rc )
     {
     case 0:
@@ -123,7 +123,7 @@ int monitor_traps(struct vcpu *v, bool_t sync, vm_event_request_t *req)
     }
 
     vm_event_fill_regs(req);
-    vm_event_put_request(d, &d->vm_event->monitor, req);
+    vm_event_put_request(d, d->vm_event_monitor, req);
 
     return rc;
 }
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 9291db6..c06fd5f 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -42,7 +42,7 @@
 static int vm_event_enable(
     struct domain *d,
     xen_domctl_vm_event_op_t *vec,
-    struct vm_event_domain *ved,
+    struct vm_event_domain **ved,
     int pause_flag,
     int param,
     xen_event_channel_notification_t notification_fn)
@@ -50,32 +50,37 @@ static int vm_event_enable(
     int rc;
     unsigned long ring_gfn = d->arch.hvm_domain.params[param];
 
+    if ( !(*ved) )
+        (*ved) = xzalloc(struct vm_event_domain);
+    if ( !(*ved) )
+        return -ENOMEM;
+
     /* Only one helper at a time. If the helper crashed,
      * the ring is in an undefined state and so is the guest.
      */
-    if ( ved->ring_page )
-        return -EBUSY;
+    if ( (*ved)->ring_page )
+        return -EBUSY;;
 
     /* The parameter defaults to zero, and it should be
      * set to something */
     if ( ring_gfn == 0 )
         return -ENOSYS;
 
-    vm_event_ring_lock_init(ved);
-    vm_event_ring_lock(ved);
+    vm_event_ring_lock_init(*ved);
+    vm_event_ring_lock(*ved);
 
     rc = vm_event_init_domain(d);
 
     if ( rc < 0 )
         goto err;
 
-    rc = prepare_ring_for_helper(d, ring_gfn, &ved->ring_pg_struct,
-                                    &ved->ring_page);
+    rc = prepare_ring_for_helper(d, ring_gfn, &(*ved)->ring_pg_struct,
+                                    &(*ved)->ring_page);
     if ( rc < 0 )
         goto err;
 
     /* Set the number of currently blocked vCPUs to 0. */
-    ved->blocked = 0;
+    (*ved)->blocked = 0;
 
     /* Allocate event channel */
     rc = alloc_unbound_xen_event_channel(d, 0, current->domain->domain_id,
@@ -83,26 +88,28 @@ static int vm_event_enable(
     if ( rc < 0 )
         goto err;
 
-    ved->xen_port = vec->port = rc;
+    (*ved)->xen_port = vec->port = rc;
 
     /* Prepare ring buffer */
-    FRONT_RING_INIT(&ved->front_ring,
-                    (vm_event_sring_t *)ved->ring_page,
+    FRONT_RING_INIT(&(*ved)->front_ring,
+                    (vm_event_sring_t *)(*ved)->ring_page,
                     PAGE_SIZE);
 
     /* Save the pause flag for this particular ring. */
-    ved->pause_flag = pause_flag;
+    (*ved)->pause_flag = pause_flag;
 
     /* Initialize the last-chance wait queue. */
-    init_waitqueue_head(&ved->wq);
+    init_waitqueue_head(&(*ved)->wq);
 
-    vm_event_ring_unlock(ved);
+    vm_event_ring_unlock((*ved));
     return 0;
 
  err:
-    destroy_ring_for_helper(&ved->ring_page,
-                            ved->ring_pg_struct);
-    vm_event_ring_unlock(ved);
+    destroy_ring_for_helper(&(*ved)->ring_page,
+                            (*ved)->ring_pg_struct);
+    vm_event_ring_unlock((*ved));
+    xfree(*ved);
+    *ved = NULL;
 
     return rc;
 }
@@ -187,39 +194,45 @@ void vm_event_wake(struct domain *d, struct vm_event_domain *ved)
         vm_event_wake_blocked(d, ved);
 }
 
-static int vm_event_disable(struct domain *d, struct vm_event_domain *ved)
+static int vm_event_disable(struct domain *d, struct vm_event_domain **ved)
 {
-    if ( ved->ring_page )
+    if ( !*ved )
+        return 0;
+
+    if ( (*ved)->ring_page )
     {
         struct vcpu *v;
 
-        vm_event_ring_lock(ved);
+        vm_event_ring_lock(*ved);
 
-        if ( !list_empty(&ved->wq.list) )
+        if ( !list_empty(&(*ved)->wq.list) )
         {
-            vm_event_ring_unlock(ved);
+            vm_event_ring_unlock(*ved);
             return -EBUSY;
         }
 
         /* Free domU's event channel and leave the other one unbound */
-        free_xen_event_channel(d, ved->xen_port);
+        free_xen_event_channel(d, (*ved)->xen_port);
 
         /* Unblock all vCPUs */
         for_each_vcpu ( d, v )
         {
-            if ( test_and_clear_bit(ved->pause_flag, &v->pause_flags) )
+            if ( test_and_clear_bit((*ved)->pause_flag, &v->pause_flags) )
             {
                 vcpu_unpause(v);
-                ved->blocked--;
+                (*ved)->blocked--;
             }
         }
 
-        destroy_ring_for_helper(&ved->ring_page,
-                                ved->ring_pg_struct);
+        destroy_ring_for_helper(&(*ved)->ring_page,
+                                (*ved)->ring_pg_struct);
 
         vm_event_cleanup_domain(d);
 
-        vm_event_ring_unlock(ved);
+        vm_event_ring_unlock(*ved);
+
+        xfree(*ved);
+        *ved = NULL;
     }
 
     return 0;
@@ -267,6 +280,9 @@ void vm_event_put_request(struct domain *d,
     RING_IDX req_prod;
     struct vcpu *curr = current;
 
+    if( !ved )
+        return;
+
     if ( curr->domain != d )
     {
         req->flags |= VM_EVENT_FLAG_FOREIGN;
@@ -434,6 +450,9 @@ void vm_event_resume(struct domain *d, struct vm_event_domain *ved)
 
 void vm_event_cancel_slot(struct domain *d, struct vm_event_domain *ved)
 {
+    if( !ved )
+        return;
+
     vm_event_ring_lock(ved);
     vm_event_release_slot(d, ved);
     vm_event_ring_unlock(ved);
@@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct vm_event_domain *ved)
 int __vm_event_claim_slot(struct domain *d, struct vm_event_domain *ved,
                           bool_t allow_sleep)
 {
+    if ( !ved )
+        return -ENOSYS;
+
     if ( (current->domain == d) && allow_sleep )
         return vm_event_wait_slot(ved);
     else
@@ -510,24 +532,24 @@ int __vm_event_claim_slot(struct domain *d, struct vm_event_domain *ved,
 /* Registered with Xen-bound event channel for incoming notifications. */
 static void mem_paging_notification(struct vcpu *v, unsigned int port)
 {
-    if ( likely(v->domain->vm_event->paging.ring_page != NULL) )
-        vm_event_resume(v->domain, &v->domain->vm_event->paging);
+    if ( likely(v->domain->vm_event_paging->ring_page != NULL) )
+        vm_event_resume(v->domain, v->domain->vm_event_paging);
 }
 #endif
 
 /* Registered with Xen-bound event channel for incoming notifications. */
 static void monitor_notification(struct vcpu *v, unsigned int port)
 {
-    if ( likely(v->domain->vm_event->monitor.ring_page != NULL) )
-        vm_event_resume(v->domain, &v->domain->vm_event->monitor);
+    if ( likely(v->domain->vm_event_monitor->ring_page != NULL) )
+        vm_event_resume(v->domain, v->domain->vm_event_monitor);
 }
 
 #ifdef CONFIG_HAS_MEM_SHARING
 /* Registered with Xen-bound event channel for incoming notifications. */
 static void mem_sharing_notification(struct vcpu *v, unsigned int port)
 {
-    if ( likely(v->domain->vm_event->share.ring_page != NULL) )
-        vm_event_resume(v->domain, &v->domain->vm_event->share);
+    if ( likely(v->domain->vm_event_share->ring_page != NULL) )
+        vm_event_resume(v->domain, v->domain->vm_event_share);
 }
 #endif
 
@@ -535,7 +557,7 @@ static void mem_sharing_notification(struct vcpu *v, unsigned int port)
 void vm_event_cleanup(struct domain *d)
 {
 #ifdef CONFIG_HAS_MEM_PAGING
-    if ( d->vm_event->paging.ring_page )
+    if ( d->vm_event_paging && d->vm_event_paging->ring_page )
     {
         /* Destroying the wait queue head means waking up all
          * queued vcpus. This will drain the list, allowing
@@ -544,20 +566,20 @@ void vm_event_cleanup(struct domain *d)
          * Finally, because this code path involves previously
          * pausing the domain (domain_kill), unpausing the
          * vcpus causes no harm. */
-        destroy_waitqueue_head(&d->vm_event->paging.wq);
-        (void)vm_event_disable(d, &d->vm_event->paging);
+        destroy_waitqueue_head(&d->vm_event_paging->wq);
+        (void)vm_event_disable(d, &d->vm_event_paging);
     }
 #endif
-    if ( d->vm_event->monitor.ring_page )
+    if ( d->vm_event_monitor && d->vm_event_monitor->ring_page )
     {
-        destroy_waitqueue_head(&d->vm_event->monitor.wq);
-        (void)vm_event_disable(d, &d->vm_event->monitor);
+        destroy_waitqueue_head(&d->vm_event_monitor->wq);
+        (void)vm_event_disable(d, &d->vm_event_monitor);
     }
 #ifdef CONFIG_HAS_MEM_SHARING
-    if ( d->vm_event->share.ring_page )
+    if ( d->vm_event_share && d->vm_event_share->ring_page )
     {
-        destroy_waitqueue_head(&d->vm_event->share.wq);
-        (void)vm_event_disable(d, &d->vm_event->share);
+        destroy_waitqueue_head(&d->vm_event_share->wq);
+        (void)vm_event_disable(d, &d->vm_event_share);
     }
 #endif
 }
@@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
 #ifdef CONFIG_HAS_MEM_PAGING
     case XEN_DOMCTL_VM_EVENT_OP_PAGING:
     {
-        struct vm_event_domain *ved = &d->vm_event->paging;
         rc = -EINVAL;
 
         switch( vec->op )
@@ -629,24 +650,28 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
                 break;
 
             /* domain_pause() not required here, see XSA-99 */
-            rc = vm_event_enable(d, vec, ved, _VPF_mem_paging,
+            rc = vm_event_enable(d, vec, &d->vm_event_paging, _VPF_mem_paging,
                                  HVM_PARAM_PAGING_RING_PFN,
                                  mem_paging_notification);
         }
         break;
 
         case XEN_VM_EVENT_DISABLE:
-            if ( ved->ring_page )
+            if ( !d->vm_event_paging )
+                break;
+            if ( d->vm_event_paging->ring_page )
             {
                 domain_pause(d);
-                rc = vm_event_disable(d, ved);
+                rc = vm_event_disable(d, &d->vm_event_paging);
                 domain_unpause(d);
             }
             break;
 
         case XEN_VM_EVENT_RESUME:
-            if ( ved->ring_page )
-                vm_event_resume(d, ved);
+            if ( !d->vm_event_paging )
+                break;
+            if ( d->vm_event_paging->ring_page )
+                vm_event_resume(d, d->vm_event_paging);
             else
                 rc = -ENODEV;
             break;
@@ -661,7 +686,6 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
 
     case XEN_DOMCTL_VM_EVENT_OP_MONITOR:
     {
-        struct vm_event_domain *ved = &d->vm_event->monitor;
         rc = -EINVAL;
 
         switch( vec->op )
@@ -671,24 +695,28 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
             rc = arch_monitor_init_domain(d);
             if ( rc )
                 break;
-            rc = vm_event_enable(d, vec, ved, _VPF_mem_access,
+            rc = vm_event_enable(d, vec, &d->vm_event_monitor, _VPF_mem_access,
                                  HVM_PARAM_MONITOR_RING_PFN,
                                  monitor_notification);
             break;
 
         case XEN_VM_EVENT_DISABLE:
-            if ( ved->ring_page )
+            if ( !d->vm_event_monitor )
+                break;
+            if ( d->vm_event_monitor->ring_page )
             {
                 domain_pause(d);
-                rc = vm_event_disable(d, ved);
+                rc = vm_event_disable(d, &d->vm_event_monitor);
                 arch_monitor_cleanup_domain(d);
                 domain_unpause(d);
             }
             break;
 
         case XEN_VM_EVENT_RESUME:
-            if ( ved->ring_page )
-                vm_event_resume(d, ved);
+            if ( !d->vm_event_monitor )
+                break;
+            if ( d->vm_event_monitor->ring_page )
+                vm_event_resume(d, d->vm_event_monitor);
             else
                 rc = -ENODEV;
             break;
@@ -703,7 +731,6 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
 #ifdef CONFIG_HAS_MEM_SHARING
     case XEN_DOMCTL_VM_EVENT_OP_SHARING:
     {
-        struct vm_event_domain *ved = &d->vm_event->share;
         rc = -EINVAL;
 
         switch( vec->op )
@@ -720,23 +747,27 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
                 break;
 
             /* domain_pause() not required here, see XSA-99 */
-            rc = vm_event_enable(d, vec, ved, _VPF_mem_sharing,
+            rc = vm_event_enable(d, vec, &d->vm_event_share, _VPF_mem_sharing,
                                  HVM_PARAM_SHARING_RING_PFN,
                                  mem_sharing_notification);
             break;
 
         case XEN_VM_EVENT_DISABLE:
-            if ( ved->ring_page )
+            if ( !d->vm_event_share )
+                break;
+            if ( d->vm_event_share->ring_page )
             {
                 domain_pause(d);
-                rc = vm_event_disable(d, ved);
+                rc = vm_event_disable(d, &d->vm_event_share);
                 domain_unpause(d);
             }
             break;
 
         case XEN_VM_EVENT_RESUME:
-            if ( ved->ring_page )
-                vm_event_resume(d, ved);
+            if ( !d->vm_event_share )
+                break;
+            if ( d->vm_event_share->ring_page )
+                vm_event_resume(d, d->vm_event_share);
             else
                 rc = -ENODEV;
             break;
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 27bdb71..2899dd1 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1363,9 +1363,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
 
     /* Prevent device assign if mem paging or mem sharing have been 
      * enabled for this domain */
+    if( !d->vm_event_paging )
+        return -EXDEV;
     if ( unlikely(!need_iommu(d) &&
             (d->arch.hvm_domain.mem_sharing_enabled ||
-             d->vm_event->paging.ring_page ||
+             d->vm_event_paging->ring_page ||
              p2m_get_hostp2m(d)->global_logdirty)) )
         return -EXDEV;
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 6673b27..e48487c 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -295,16 +295,6 @@ struct vm_event_domain
     unsigned int last_vcpu_wake_up;
 };
 
-struct vm_event_per_domain
-{
-    /* Memory sharing support */
-    struct vm_event_domain share;
-    /* Memory paging support */
-    struct vm_event_domain paging;
-    /* VM event monitor support */
-    struct vm_event_domain monitor;
-};
-
 struct evtchn_port_ops;
 
 enum guest_type {
@@ -464,7 +454,13 @@ struct domain
     struct lock_profile_qhead profile_head;
 
     /* Various vm_events */
-    struct vm_event_per_domain *vm_event;
+
+    /* Memory sharing support */
+    struct vm_event_domain *vm_event_share;
+    /* Memory paging support */
+    struct vm_event_domain *vm_event_paging;
+    /* VM event monitor support */
+    struct vm_event_domain *vm_event_monitor;
 
     /*
      * Can be specified by the user. If that is not the case, it is
-- 
2.7.4


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

Re: [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Jan Beulich]

>>> On 24.08.17 at 13:48, <aisaila@bitdefender.com> wrote:
> The patch splits the vm_event into three structures:vm_event_share,
> vm_event_paging, vm_event_monitor. The allocation for the
> structure is moved to vm_event_enable so that it can be
> allocated/init when needed and freed in vm_event_disable.
> 
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

Missing brief description of changes from v1 here.

> @@ -50,32 +50,37 @@ static int vm_event_enable(
>      int rc;
>      unsigned long ring_gfn = d->arch.hvm_domain.params[param];
>  
> +    if ( !(*ved) )
> +        (*ved) = xzalloc(struct vm_event_domain);
> +    if ( !(*ved) )

In none of the three cases you really need the parentheses around
*ved.

> @@ -187,39 +194,45 @@ void vm_event_wake(struct domain *d, struct 
> vm_event_domain *ved)
>          vm_event_wake_blocked(d, ved);
>  }
>  
> -static int vm_event_disable(struct domain *d, struct vm_event_domain *ved)
> +static int vm_event_disable(struct domain *d, struct vm_event_domain **ved)
>  {
> -    if ( ved->ring_page )
> +    if ( !*ved )
> +        return 0;
> +
> +    if ( (*ved)->ring_page )
>      {
>[...]
> +        xfree(*ved);
> +        *ved = NULL;
>      }

If both if()-s above are really useful, you are leaking *ved when it
is non-NULL but ->ring_page is NULL.

> @@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct vm_event_domain *ved)
>  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain *ved,
>                            bool_t allow_sleep)
>  {
> +    if ( !ved )
> +        return -ENOSYS;

I don't think ENOSYS is correct here.

> @@ -510,24 +532,24 @@ int __vm_event_claim_slot(struct domain *d, struct vm_event_domain *ved,
>  /* Registered with Xen-bound event channel for incoming notifications. */
>  static void mem_paging_notification(struct vcpu *v, unsigned int port)
>  {
> -    if ( likely(v->domain->vm_event->paging.ring_page != NULL) )
> -        vm_event_resume(v->domain, &v->domain->vm_event->paging);
> +    if ( likely(v->domain->vm_event_paging->ring_page != NULL) )
> +        vm_event_resume(v->domain, v->domain->vm_event_paging);
>  }
>  #endif
>  
>  /* Registered with Xen-bound event channel for incoming notifications. */
>  static void monitor_notification(struct vcpu *v, unsigned int port)
>  {
> -    if ( likely(v->domain->vm_event->monitor.ring_page != NULL) )
> -        vm_event_resume(v->domain, &v->domain->vm_event->monitor);
> +    if ( likely(v->domain->vm_event_monitor->ring_page != NULL) )
> +        vm_event_resume(v->domain, v->domain->vm_event_monitor);
>  }
>  
>  #ifdef CONFIG_HAS_MEM_SHARING
>  /* Registered with Xen-bound event channel for incoming notifications. */
>  static void mem_sharing_notification(struct vcpu *v, unsigned int port)
>  {
> -    if ( likely(v->domain->vm_event->share.ring_page != NULL) )
> -        vm_event_resume(v->domain, &v->domain->vm_event->share);
> +    if ( likely(v->domain->vm_event_share->ring_page != NULL) )
> +        vm_event_resume(v->domain, v->domain->vm_event_share);
>  }
>  #endif

For all three a local variable holding v->domain would certain help;
eventually the functions should even be passed struct domain *
instead of struct vcpu *, I think.

> @@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d, xen_domctl_vm_event_op_t *vec,
>  #ifdef CONFIG_HAS_MEM_PAGING
>      case XEN_DOMCTL_VM_EVENT_OP_PAGING:
>      {
> -        struct vm_event_domain *ved = &d->vm_event->paging;

Dropping this local variable (and similar ones below) pointlessly
increases the size of this patch.

> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1363,9 +1363,11 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>  
>      /* Prevent device assign if mem paging or mem sharing have been 
>       * enabled for this domain */
> +    if( !d->vm_event_paging )
> +        return -EXDEV;

Is this check the wrong way round? And why can't it be combined
with ...

>      if ( unlikely(!need_iommu(d) &&
>              (d->arch.hvm_domain.mem_sharing_enabled ||
> -             d->vm_event->paging.ring_page ||
> +             d->vm_event_paging->ring_page ||
>               p2m_get_hostp2m(d)->global_logdirty)) )
>          return -EXDEV;

... this set?

Jan

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

Re: [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Alexandru Stefan ISAILA]

On Jo, 2017-08-24 at 07:24 -0600, Jan Beulich wrote:
> > 
> > > 
> > > > 
> > > > On 24.08.17 at 13:48, <aisaila@bitdefender.com> wrote:
> > The patch splits the vm_event into three structures:vm_event_share,
> > vm_event_paging, vm_event_monitor. The allocation for the
> > structure is moved to vm_event_enable so that it can be
> > allocated/init when needed and freed in vm_event_disable.
> > 
> > Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> Missing brief description of changes from v1 here.
> 
> > 
> > @@ -50,32 +50,37 @@ static int vm_event_enable(
> >      int rc;
> >      unsigned long ring_gfn = d->arch.hvm_domain.params[param];
> >  
> > +    if ( !(*ved) )
> > +        (*ved) = xzalloc(struct vm_event_domain);
> > +    if ( !(*ved) )
> In none of the three cases you really need the parentheses around
> *ved.
> 
> > 
> > @@ -187,39 +194,45 @@ void vm_event_wake(struct domain *d, struct 
> > vm_event_domain *ved)
> >          vm_event_wake_blocked(d, ved);
> >  }
> >  
> > -static int vm_event_disable(struct domain *d, struct
> > vm_event_domain *ved)
> > +static int vm_event_disable(struct domain *d, struct
> > vm_event_domain **ved)
> >  {
> > -    if ( ved->ring_page )
> > +    if ( !*ved )
> > +        return 0;
> > +
> > +    if ( (*ved)->ring_page )
> >      {
> > [...]
> > +        xfree(*ved);
> > +        *ved = NULL;
> >      }
> If both if()-s above are really useful, you are leaking *ved when it
> is non-NULL but ->ring_page is NULL.
> 
> > 
> > @@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct
> > vm_event_domain *ved)
> >  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain
> > *ved,
> >                            bool_t allow_sleep)
> >  {
> > +    if ( !ved )
> > +        return -ENOSYS;
> I don't think ENOSYS is correct here.
Can you tell me what is the preferred return value here?
> 
> > 
> > @@ -510,24 +532,24 @@ int __vm_event_claim_slot(struct domain *d,
> > struct vm_event_domain *ved,
> >  /* Registered with Xen-bound event channel for incoming
> > notifications. */
> >  static void mem_paging_notification(struct vcpu *v, unsigned int
> > port)
> >  {
> > -    if ( likely(v->domain->vm_event->paging.ring_page != NULL) )
> > -        vm_event_resume(v->domain, &v->domain->vm_event->paging);
> > +    if ( likely(v->domain->vm_event_paging->ring_page != NULL) )
> > +        vm_event_resume(v->domain, v->domain->vm_event_paging);
> >  }
> >  #endif
> >  
> >  /* Registered with Xen-bound event channel for incoming
> > notifications. */
> >  static void monitor_notification(struct vcpu *v, unsigned int
> > port)
> >  {
> > -    if ( likely(v->domain->vm_event->monitor.ring_page != NULL) )
> > -        vm_event_resume(v->domain, &v->domain->vm_event->monitor);
> > +    if ( likely(v->domain->vm_event_monitor->ring_page != NULL) )
> > +        vm_event_resume(v->domain, v->domain->vm_event_monitor);
> >  }
> >  
> >  #ifdef CONFIG_HAS_MEM_SHARING
> >  /* Registered with Xen-bound event channel for incoming
> > notifications. */
> >  static void mem_sharing_notification(struct vcpu *v, unsigned int
> > port)
> >  {
> > -    if ( likely(v->domain->vm_event->share.ring_page != NULL) )
> > -        vm_event_resume(v->domain, &v->domain->vm_event->share);
> > +    if ( likely(v->domain->vm_event_share->ring_page != NULL) )
> > +        vm_event_resume(v->domain, v->domain->vm_event_share);
> >  }
> >  #endif
> For all three a local variable holding v->domain would certain help;
> eventually the functions should even be passed struct domain *
> instead of struct vcpu *, I think.
> 
> > 
> > @@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d,
> > xen_domctl_vm_event_op_t *vec,
> >  #ifdef CONFIG_HAS_MEM_PAGING
> >      case XEN_DOMCTL_VM_EVENT_OP_PAGING:
> >      {
> > -        struct vm_event_domain *ved = &d->vm_event->paging;
> Dropping this local variable (and similar ones below) pointlessly
> increases the size of this patch.
I have dropped the local var because in case of a XEN_VM_EVENT_ENABLE
d->vm_event_... is allocated in the vm_event_enable function below so
it will allocate mem for the local variable.
> 
> > 
> > --- a/xen/drivers/passthrough/pci.c
> > +++ b/xen/drivers/passthrough/pci.c
> > @@ -1363,9 +1363,11 @@ static int assign_device(struct domain *d,
> > u16 seg, u8 bus, u8 devfn, u32 flag)
> >  
> >      /* Prevent device assign if mem paging or mem sharing have
> > been 
> >       * enabled for this domain */
> > +    if( !d->vm_event_paging )
> > +        return -EXDEV;
> Is this check the wrong way round? And why can't it be combined
> with ...
> 
> > 
> >      if ( unlikely(!need_iommu(d) &&
> >              (d->arch.hvm_domain.mem_sharing_enabled ||
> > -             d->vm_event->paging.ring_page ||
> > +             d->vm_event_paging->ring_page ||
> >               p2m_get_hostp2m(d)->global_logdirty)) )
> >          return -EXDEV;
> ... this set?
> 
> Jan
Alex
> 
> ________________________
> This email was scanned by Bitdefender
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Jan Beulich]

>>> On 24.08.17 at 17:17, <aisaila@bitdefender.com> wrote:
> On Jo, 2017-08-24 at 07:24 -0600, Jan Beulich wrote:
>> > @@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct
>> > vm_event_domain *ved)
>> >  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain
>> > *ved,
>> >                            bool_t allow_sleep)
>> >  {
>> > +    if ( !ved )
>> > +        return -ENOSYS;
>> I don't think ENOSYS is correct here.
> Can you tell me what is the preferred return value here?

-EOPNOTSUPP is what we commonly use in such cases.

>> > @@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d,
>> > xen_domctl_vm_event_op_t *vec,
>> >  #ifdef CONFIG_HAS_MEM_PAGING
>> >      case XEN_DOMCTL_VM_EVENT_OP_PAGING:
>> >      {
>> > -        struct vm_event_domain *ved = &d->vm_event->paging;
>> Dropping this local variable (and similar ones below) pointlessly
>> increases the size of this patch.
> I have dropped the local var because in case of a XEN_VM_EVENT_ENABLE
> d->vm_event_... is allocated in the vm_event_enable function below so
> it will allocate mem for the local variable.

I don't see how that renders the local variable useless. But anyway,
its the maintainers of that code to judge.

Also - please trim your replies.

Jan


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

Re: [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Thu, Aug 24, 2017 at 5:48 AM, Alexandru Isaila
<aisaila@bitdefender.com> wrote:
> The patch splits the vm_event into three structures:vm_event_share,
> vm_event_paging, vm_event_monitor. The allocation for the
> structure is moved to vm_event_enable so that it can be
> allocated/init when needed and freed in vm_event_disable.
>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>

Thanks for doing this patch, I think it improves the code a lot!


> @@ -51,8 +51,7 @@ int mem_access_memop(unsigned long cmd,
>      if ( rc )
>          goto out;
>

Why are you removing setting the rc below?

> -    rc = -ENODEV;
> -    if ( unlikely(!d->vm_event->monitor.ring_page) )
> +    if ( !d->vm_event_monitor || unlikely(!d->vm_event_monitor->ring_page) )
>          goto out;
>
>      switch ( mao.op )

...

> @@ -187,39 +194,45 @@ void vm_event_wake(struct domain *d, struct vm_event_domain *ved)
>          vm_event_wake_blocked(d, ved);
>  }
>
> -static int vm_event_disable(struct domain *d, struct vm_event_domain *ved)
> +static int vm_event_disable(struct domain *d, struct vm_event_domain **ved)
>  {

I think you should check for *ved and *ved->ring_page all in one go below.

> -    if ( ved->ring_page )
> +    if ( !*ved )
> +        return 0;
> +
> +    if ( (*ved)->ring_page )
>      {
>          struct vcpu *v;
>

Thanks,
Tamas

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

Re: [PATCH v2] common/vm_event: Initialize vm_event lists on domain creation

[Tamas K Lengyel]

On Thu, Aug 24, 2017 at 9:24 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 24.08.17 at 17:17, <aisaila@bitdefender.com> wrote:
>> On Jo, 2017-08-24 at 07:24 -0600, Jan Beulich wrote:
>>> > @@ -500,6 +519,9 @@ bool_t vm_event_check_ring(struct
>>> > vm_event_domain *ved)
>>> >  int __vm_event_claim_slot(struct domain *d, struct vm_event_domain
>>> > *ved,
>>> >                            bool_t allow_sleep)
>>> >  {
>>> > +    if ( !ved )
>>> > +        return -ENOSYS;
>>> I don't think ENOSYS is correct here.
>> Can you tell me what is the preferred return value here?
>
> -EOPNOTSUPP is what we commonly use in such cases.
>
>>> > @@ -599,7 +621,6 @@ int vm_event_domctl(struct domain *d,
>>> > xen_domctl_vm_event_op_t *vec,
>>> >  #ifdef CONFIG_HAS_MEM_PAGING
>>> >      case XEN_DOMCTL_VM_EVENT_OP_PAGING:
>>> >      {
>>> > -        struct vm_event_domain *ved = &d->vm_event->paging;
>>> Dropping this local variable (and similar ones below) pointlessly
>>> increases the size of this patch.
>> I have dropped the local var because in case of a XEN_VM_EVENT_ENABLE
>> d->vm_event_... is allocated in the vm_event_enable function below so
>> it will allocate mem for the local variable.
>
> I don't see how that renders the local variable useless. But anyway,
> its the maintainers of that code to judge.

I guess the reason for dropping it is that vm_event_enable will place
the location of the structure into the pointer that was passed in, so
if you are passing in a pointer that was locally declared you would
then still have to copy it back to d->vm_event_ which doesn't make
much sense. IMHO it's fine how it's changed in the patch.

Tamas

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