[PATCH v6 0/2] Improve vhost-user VQ notifier unmap

[PATCH v6 0/2] Improve vhost-user VQ notifier unmap

[Xueming Li]

When vDPA applicaiton in client mode shutdown, unmapped VQ notifier
might being accessed by vCPU thread under high tx traffic, it will
crash VM in rare conditon. This patch try to fix it with better RCU
sychronization of new flatview.

v2: no RCU draining on vCPU thread
v3: minor fix on coding style and comments
    https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01764.html
v4: fix first patch compilation
    https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg04060.html
v5: update 2/2 commit message
    https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg04115.html
v6: use call_rcu to avoid dead lock in rcu reader

Xueming Li (2):
  vhost-user: remove VirtQ notifier restore
  vhost-user: fix VirtQ notifier cleanup

 hw/virtio/vhost-user.c         | 63 ++++++++++++++++------------------
 include/hw/virtio/vhost-user.h |  3 +-
 2 files changed, 31 insertions(+), 35 deletions(-)

-- 
2.33.0

[PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Xueming Li]

When vhost-user vdpa client suspend, backend may close all resources,
VQ notifier mmap address become invalid, restore MR which contains
the invalid address is wrong. vdpa client will set VQ notifier after
reconnect.

This patch removes VQ notifier restore and related flags to avoid reusing
invalid address.

Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
Cc: qemu-stable@nongnu.org
Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 hw/virtio/vhost-user.c         | 19 +------------------
 include/hw/virtio/vhost-user.h |  1 -
 2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index bf6e50223c..c671719e9b 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
     return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
 }
 
-static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
-                                             int queue_idx)
-{
-    struct vhost_user *u = dev->opaque;
-    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
-    VirtIODevice *vdev = dev->vdev;
-
-    if (n->addr && !n->set) {
-        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
-        n->set = true;
-    }
-}
-
 static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
                                             int queue_idx)
 {
@@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
     VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
     VirtIODevice *vdev = dev->vdev;
 
-    if (n->addr && n->set) {
+    if (n->addr) {
         virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
-        n->set = false;
     }
 }
 
 static int vhost_user_set_vring_base(struct vhost_dev *dev,
                                      struct vhost_vring_state *ring)
 {
-    vhost_user_host_notifier_restore(dev, ring->index);
-
     return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
 }
 
@@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
     }
 
     n->addr = addr;
-    n->set = true;
 
     return 0;
 }
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index a9abca3288..f6012b2078 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -14,7 +14,6 @@
 typedef struct VhostUserHostNotifier {
     MemoryRegion mr;
     void *addr;
-    bool set;
 } VhostUserHostNotifier;
 
 typedef struct VhostUserState {
-- 
2.33.0

[PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup

[Xueming Li]

When vhost-user device cleanup is executed and un-mmaps notifier
address, VM cpu thread writing the notifier fails by accessing invalid
address error.

To avoid this concurrent issue, call RCU and wait for a memory flatview
update, then un-mmap notifiers in callback.

Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
Cc: qemu-stable@nongnu.org
Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
Signed-off-by: Xueming Li <xuemingl@nvidia.com>
---
 hw/virtio/vhost-user.c         | 50 +++++++++++++++++++++-------------
 include/hw/virtio/vhost-user.h |  2 ++
 2 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index c671719e9b..5adad4d029 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -25,6 +25,7 @@
 #include "migration/migration.h"
 #include "migration/postcopy-ram.h"
 #include "trace.h"
+#include "exec/ramblock.h"
 
 #include <sys/ioctl.h>
 #include <sys/socket.h>
@@ -1143,15 +1144,27 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
     return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
 }
 
-static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
-                                            int queue_idx)
+static void vhost_user_host_notifier_free(VhostUserHostNotifier *n)
 {
-    struct vhost_user *u = dev->opaque;
-    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
-    VirtIODevice *vdev = dev->vdev;
+    assert(n && n->old_addr);
+    munmap(n->old_addr, qemu_real_host_page_size);
+    n->old_addr = NULL;
+}
+
+static void vhost_user_host_notifier_remove(VhostUserState *user,
+                                            VirtIODevice *vdev, int queue_idx)
+{
+    VhostUserHostNotifier *n = &user->notifier[queue_idx];
 
     if (n->addr) {
-        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
+        if (vdev) {
+            virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
+        }
+        assert(n->addr);
+        assert(!n->old_addr);
+        n->old_addr = n->addr;
+        n->addr = NULL;
+        call_rcu(n, vhost_user_host_notifier_free, rcu);
     }
 }
 
@@ -1190,8 +1203,9 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev,
         .payload.state = *ring,
         .hdr.size = sizeof(msg.payload.state),
     };
+    struct vhost_user *u = dev->opaque;
 
-    vhost_user_host_notifier_remove(dev, ring->index);
+    vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index);
 
     if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
         return -1;
@@ -1486,12 +1500,7 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
 
     n = &user->notifier[queue_idx];
 
-    if (n->addr) {
-        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
-        object_unparent(OBJECT(&n->mr));
-        munmap(n->addr, page_size);
-        n->addr = NULL;
-    }
+    vhost_user_host_notifier_remove(user, vdev, queue_idx);
 
     if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
         return 0;
@@ -1510,9 +1519,12 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
 
     name = g_strdup_printf("vhost-user/host-notifier@%p mmaps[%d]",
                            user, queue_idx);
-    if (!n->mr.ram) /* Don't init again after suspend. */
+    if (!n->mr.ram) { /* Don't init again after suspend. */
         memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
                                           page_size, addr);
+    } else {
+        n->mr.ram_block->host = addr;
+    }
     g_free(name);
 
     if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
@@ -2460,17 +2472,17 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp)
 void vhost_user_cleanup(VhostUserState *user)
 {
     int i;
+    VhostUserHostNotifier *n;
 
     if (!user->chr) {
         return;
     }
     memory_region_transaction_begin();
     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
-        if (user->notifier[i].addr) {
-            object_unparent(OBJECT(&user->notifier[i].mr));
-            munmap(user->notifier[i].addr, qemu_real_host_page_size);
-            user->notifier[i].addr = NULL;
-        }
+        n = &user->notifier[i];
+        assert(!n->addr);
+        vhost_user_host_notifier_remove(user, NULL, i);
+        object_unparent(OBJECT(&n->mr));
     }
     memory_region_transaction_commit();
     user->chr = NULL;
diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
index f6012b2078..03aa22d450 100644
--- a/include/hw/virtio/vhost-user.h
+++ b/include/hw/virtio/vhost-user.h
@@ -12,8 +12,10 @@
 #include "hw/virtio/virtio.h"
 
 typedef struct VhostUserHostNotifier {
+    struct rcu_head rcu;
     MemoryRegion mr;
     void *addr;
+    void *old_addr;
 } VhostUserHostNotifier;
 
 typedef struct VhostUserState {
-- 
2.33.0

Re: [PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup

[Michael S. Tsirkin]

On Mon, Nov 01, 2021 at 04:38:13PM +0800, Xueming Li wrote:
> When vhost-user device cleanup is executed and un-mmaps notifier
> address, VM cpu thread writing the notifier fails by accessing invalid
> address error.
> 
> To avoid this concurrent issue, call RCU and wait for a memory flatview
> update, then un-mmap notifiers in callback.
> 
> Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> Cc: qemu-stable@nongnu.org
> Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
>  hw/virtio/vhost-user.c         | 50 +++++++++++++++++++++-------------
>  include/hw/virtio/vhost-user.h |  2 ++
>  2 files changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index c671719e9b..5adad4d029 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -25,6 +25,7 @@
>  #include "migration/migration.h"
>  #include "migration/postcopy-ram.h"
>  #include "trace.h"
> +#include "exec/ramblock.h"
>  
>  #include <sys/ioctl.h>
>  #include <sys/socket.h>
> @@ -1143,15 +1144,27 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
>      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
>  }
>  
> -static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> -                                            int queue_idx)
> +static void vhost_user_host_notifier_free(VhostUserHostNotifier *n)
>  {
> -    struct vhost_user *u = dev->opaque;
> -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> -    VirtIODevice *vdev = dev->vdev;
> +    assert(n && n->old_addr);
> +    munmap(n->old_addr, qemu_real_host_page_size);
> +    n->old_addr = NULL;
> +}
> +
> +static void vhost_user_host_notifier_remove(VhostUserState *user,
> +                                            VirtIODevice *vdev, int queue_idx)
> +{
> +    VhostUserHostNotifier *n = &user->notifier[queue_idx];
>  
>      if (n->addr) {
> -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> +        if (vdev) {
> +            virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> +        }
> +        assert(n->addr);
> +        assert(!n->old_addr);
> +        n->old_addr = n->addr;
> +        n->addr = NULL;
> +        call_rcu(n, vhost_user_host_notifier_free, rcu);
>      }
>  }
>  
> @@ -1190,8 +1203,9 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev,
>          .payload.state = *ring,
>          .hdr.size = sizeof(msg.payload.state),
>      };
> +    struct vhost_user *u = dev->opaque;
>  
> -    vhost_user_host_notifier_remove(dev, ring->index);
> +    vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index);
>  
>      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>          return -1;
> @@ -1486,12 +1500,7 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
>  
>      n = &user->notifier[queue_idx];
>  
> -    if (n->addr) {
> -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> -        object_unparent(OBJECT(&n->mr));
> -        munmap(n->addr, page_size);
> -        n->addr = NULL;
> -    }
> +    vhost_user_host_notifier_remove(user, vdev, queue_idx);
>  
>      if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
>          return 0;
> @@ -1510,9 +1519,12 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
>  
>      name = g_strdup_printf("vhost-user/host-notifier@%p mmaps[%d]",
>                             user, queue_idx);
> -    if (!n->mr.ram) /* Don't init again after suspend. */
> +    if (!n->mr.ram) { /* Don't init again after suspend. */
>          memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
>                                            page_size, addr);
> +    } else {
> +        n->mr.ram_block->host = addr;
> +    }
>      g_free(name);
>  
>      if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
> @@ -2460,17 +2472,17 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp)
>  void vhost_user_cleanup(VhostUserState *user)
>  {
>      int i;
> +    VhostUserHostNotifier *n;
>  
>      if (!user->chr) {
>          return;
>      }
>      memory_region_transaction_begin();
>      for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -        if (user->notifier[i].addr) {
> -            object_unparent(OBJECT(&user->notifier[i].mr));
> -            munmap(user->notifier[i].addr, qemu_real_host_page_size);
> -            user->notifier[i].addr = NULL;
> -        }
> +        n = &user->notifier[i];
> +        assert(!n->addr);

I'm pretty confused as to why this assert holds.
Add a comment?

> +        vhost_user_host_notifier_remove(user, NULL, i);
> +        object_unparent(OBJECT(&n->mr));
>      }
>      memory_region_transaction_commit();
>      user->chr = NULL;

I'm also confused on why we can do unparent for notifiers which have
never been set up. Won't n->mr be invalid then?


> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> index f6012b2078..03aa22d450 100644
> --- a/include/hw/virtio/vhost-user.h
> +++ b/include/hw/virtio/vhost-user.h
> @@ -12,8 +12,10 @@
>  #include "hw/virtio/virtio.h"
>  
>  typedef struct VhostUserHostNotifier {
> +    struct rcu_head rcu;
>      MemoryRegion mr;
>      void *addr;
> +    void *old_addr;

That's not a very clear name. Is this literally just
"address for the rcu callback to unmap"?
Maybe unmap_addr then?

>  } VhostUserHostNotifier;
>  
>  typedef struct VhostUserState {
> -- 
> 2.33.0

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Michael S. Tsirkin]

On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:
> When vhost-user vdpa client suspend, backend may close all resources,
> VQ notifier mmap address become invalid, restore MR which contains
> the invalid address is wrong. vdpa client will set VQ notifier after
> reconnect.
> 
> This patch removes VQ notifier restore and related flags to avoid reusing
> invalid address.
> 
> Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> Cc: qemu-stable@nongnu.org
> Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
>  hw/virtio/vhost-user.c         | 19 +------------------
>  include/hw/virtio/vhost-user.h |  1 -
>  2 files changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index bf6e50223c..c671719e9b 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
>      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
>  }
>  
> -static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
> -                                             int queue_idx)
> -{
> -    struct vhost_user *u = dev->opaque;
> -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> -    VirtIODevice *vdev = dev->vdev;
> -
> -    if (n->addr && !n->set) {
> -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
> -        n->set = true;
> -    }
> -}
> -
>  static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
>                                              int queue_idx)
>  {
> @@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
>      VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
>      VirtIODevice *vdev = dev->vdev;
>  
> -    if (n->addr && n->set) {
> +    if (n->addr) {
>          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> -        n->set = false;
>      }
>  }
>

So on vq stop we still remove the notifier...
  
>  static int vhost_user_set_vring_base(struct vhost_dev *dev,
>                                       struct vhost_vring_state *ring)
>  {
> -    vhost_user_host_notifier_restore(dev, ring->index);
> -
>      return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
>  }
>  

but on vq start we do not reinstate it? Does this not mean that
notifiers won't work after stop then start?


> @@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
>      }
>  
>      n->addr = addr;
> -    n->set = true;
>  
>      return 0;
>  }
> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> index a9abca3288..f6012b2078 100644
> --- a/include/hw/virtio/vhost-user.h
> +++ b/include/hw/virtio/vhost-user.h
> @@ -14,7 +14,6 @@
>  typedef struct VhostUserHostNotifier {
>      MemoryRegion mr;
>      void *addr;
> -    bool set;
>  } VhostUserHostNotifier;
>  
>  typedef struct VhostUserState {
> -- 
> 2.33.0

Re: [PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup

[Xueming(Steven) Li]

On Mon, 2021-11-01 at 17:00 -0400, Michael S. Tsirkin wrote:
> On Mon, Nov 01, 2021 at 04:38:13PM +0800, Xueming Li wrote:
> > When vhost-user device cleanup is executed and un-mmaps notifier
> > address, VM cpu thread writing the notifier fails by accessing invalid
> > address error.
> > 
> > To avoid this concurrent issue, call RCU and wait for a memory flatview
> > update, then un-mmap notifiers in callback.
> > 
> > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > Cc: qemu-stable@nongnu.org
> > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > ---
> >  hw/virtio/vhost-user.c         | 50 +++++++++++++++++++++-------------
> >  include/hw/virtio/vhost-user.h |  2 ++
> >  2 files changed, 33 insertions(+), 19 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index c671719e9b..5adad4d029 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -25,6 +25,7 @@
> >  #include "migration/migration.h"
> >  #include "migration/postcopy-ram.h"
> >  #include "trace.h"
> > +#include "exec/ramblock.h"
> >  
> >  #include <sys/ioctl.h>
> >  #include <sys/socket.h>
> > @@ -1143,15 +1144,27 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> >  }
> >  
> > -static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > -                                            int queue_idx)
> > +static void vhost_user_host_notifier_free(VhostUserHostNotifier *n)
> >  {
> > -    struct vhost_user *u = dev->opaque;
> > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > -    VirtIODevice *vdev = dev->vdev;
> > +    assert(n && n->old_addr);
> > +    munmap(n->old_addr, qemu_real_host_page_size);
> > +    n->old_addr = NULL;
> > +}
> > +
> > +static void vhost_user_host_notifier_remove(VhostUserState *user,
> > +                                            VirtIODevice *vdev, int queue_idx)
> > +{
> > +    VhostUserHostNotifier *n = &user->notifier[queue_idx];
> >  
> >      if (n->addr) {
> > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > +        if (vdev) {
> > +            virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > +        }
> > +        assert(n->addr);
> > +        assert(!n->old_addr);
> > +        n->old_addr = n->addr;
> > +        n->addr = NULL;
> > +        call_rcu(n, vhost_user_host_notifier_free, rcu);
> >      }
> >  }
> >  
> > @@ -1190,8 +1203,9 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev,
> >          .payload.state = *ring,
> >          .hdr.size = sizeof(msg.payload.state),
> >      };
> > +    struct vhost_user *u = dev->opaque;
> >  
> > -    vhost_user_host_notifier_remove(dev, ring->index);
> > +    vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index);
> >  
> >      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> >          return -1;
> > @@ -1486,12 +1500,7 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> >  
> >      n = &user->notifier[queue_idx];
> >  
> > -    if (n->addr) {
> > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > -        object_unparent(OBJECT(&n->mr));
> > -        munmap(n->addr, page_size);
> > -        n->addr = NULL;
> > -    }
> > +    vhost_user_host_notifier_remove(user, vdev, queue_idx);
> >  
> >      if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
> >          return 0;
> > @@ -1510,9 +1519,12 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> >  
> >      name = g_strdup_printf("vhost-user/host-notifier@%p mmaps[%d]",
> >                             user, queue_idx);
> > -    if (!n->mr.ram) /* Don't init again after suspend. */
> > +    if (!n->mr.ram) { /* Don't init again after suspend. */
> >          memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
> >                                            page_size, addr);
> > +    } else {
> > +        n->mr.ram_block->host = addr;
> > +    }
> >      g_free(name);
> >  
> >      if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
> > @@ -2460,17 +2472,17 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp)
> >  void vhost_user_cleanup(VhostUserState *user)
> >  {
> >      int i;
> > +    VhostUserHostNotifier *n;
> >  
> >      if (!user->chr) {
> >          return;
> >      }
> >      memory_region_transaction_begin();
> >      for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> > -        if (user->notifier[i].addr) {
> > -            object_unparent(OBJECT(&user->notifier[i].mr));
> > -            munmap(user->notifier[i].addr, qemu_real_host_page_size);
> > -            user->notifier[i].addr = NULL;
> > -        }
> > +        n = &user->notifier[i];
> > +        assert(!n->addr);
> 
> I'm pretty confused as to why this assert holds.
> Add a comment?

Seems notifiers are removed by vhost_user_get_vring_base(), I used this
assert to catch exception. Will remove it.

> 
> > +        vhost_user_host_notifier_remove(user, NULL, i);
> > +        object_unparent(OBJECT(&n->mr));
> >      }
> >      memory_region_transaction_commit();
> >      user->chr = NULL;
> 
> I'm also confused on why we can do unparent for notifiers which have
> never been set up. Won't n->mr be invalid then?

There is a parent check in object_unparent().

> 
> 
> > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > index f6012b2078..03aa22d450 100644
> > --- a/include/hw/virtio/vhost-user.h
> > +++ b/include/hw/virtio/vhost-user.h
> > @@ -12,8 +12,10 @@
> >  #include "hw/virtio/virtio.h"
> >  
> >  typedef struct VhostUserHostNotifier {
> > +    struct rcu_head rcu;
> >      MemoryRegion mr;
> >      void *addr;
> > +    void *old_addr;
> 
> That's not a very clear name. Is this literally just
> "address for the rcu callback to unmap"?
> Maybe unmap_addr then?

LGTM, thanks!


> 
> >  } VhostUserHostNotifier;
> >  
> >  typedef struct VhostUserState {
> > -- 
> > 2.33.0
> 

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Xueming(Steven) Li]

On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:
> On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:
> > When vhost-user vdpa client suspend, backend may close all resources,
> > VQ notifier mmap address become invalid, restore MR which contains
> > the invalid address is wrong. vdpa client will set VQ notifier after
> > reconnect.
> > 
> > This patch removes VQ notifier restore and related flags to avoid reusing
> > invalid address.
> > 
> > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > Cc: qemu-stable@nongnu.org
> > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > ---
> >  hw/virtio/vhost-user.c         | 19 +------------------
> >  include/hw/virtio/vhost-user.h |  1 -
> >  2 files changed, 1 insertion(+), 19 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index bf6e50223c..c671719e9b 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> >  }
> >  
> > -static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
> > -                                             int queue_idx)
> > -{
> > -    struct vhost_user *u = dev->opaque;
> > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > -    VirtIODevice *vdev = dev->vdev;
> > -
> > -    if (n->addr && !n->set) {
> > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
> > -        n->set = true;
> > -    }
> > -}
> > -
> >  static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> >                                              int queue_idx)
> >  {
> > @@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> >      VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> >      VirtIODevice *vdev = dev->vdev;
> >  
> > -    if (n->addr && n->set) {
> > +    if (n->addr) {
> >          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > -        n->set = false;
> >      }
> >  }
> > 
> 
> So on vq stop we still remove the notifier...
>   
> >  static int vhost_user_set_vring_base(struct vhost_dev *dev,
> >                                       struct vhost_vring_state *ring)
> >  {
> > -    vhost_user_host_notifier_restore(dev, ring->index);
> > -
> >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
> >  }
> >  
> 
> but on vq start we do not reinstate it? Does this not mean that
> notifiers won't work after stop then start?


Yes, backend initially work w/o host notifier, request VM driver to
install notifier if needed - call this function through slave socket.

> 
> 
> > @@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> >      }
> >  
> >      n->addr = addr;
> > -    n->set = true;
> >  
> >      return 0;
> >  }
> > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > index a9abca3288..f6012b2078 100644
> > --- a/include/hw/virtio/vhost-user.h
> > +++ b/include/hw/virtio/vhost-user.h
> > @@ -14,7 +14,6 @@
> >  typedef struct VhostUserHostNotifier {
> >      MemoryRegion mr;
> >      void *addr;
> > -    bool set;
> >  } VhostUserHostNotifier;
> >  
> >  typedef struct VhostUserState {
> > -- 
> > 2.33.0
> 

Re: [PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup

[Michael S. Tsirkin]

On Tue, Nov 02, 2021 at 06:00:58AM +0000, Xueming(Steven) Li wrote:
> On Mon, 2021-11-01 at 17:00 -0400, Michael S. Tsirkin wrote:
> > On Mon, Nov 01, 2021 at 04:38:13PM +0800, Xueming Li wrote:
> > > When vhost-user device cleanup is executed and un-mmaps notifier
> > > address, VM cpu thread writing the notifier fails by accessing invalid
> > > address error.
> > > 
> > > To avoid this concurrent issue, call RCU and wait for a memory flatview
> > > update, then un-mmap notifiers in callback.
> > > 
> > > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > > Cc: qemu-stable@nongnu.org
> > > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > ---
> > >  hw/virtio/vhost-user.c         | 50 +++++++++++++++++++++-------------
> > >  include/hw/virtio/vhost-user.h |  2 ++
> > >  2 files changed, 33 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > index c671719e9b..5adad4d029 100644
> > > --- a/hw/virtio/vhost-user.c
> > > +++ b/hw/virtio/vhost-user.c
> > > @@ -25,6 +25,7 @@
> > >  #include "migration/migration.h"
> > >  #include "migration/postcopy-ram.h"
> > >  #include "trace.h"
> > > +#include "exec/ramblock.h"
> > >  
> > >  #include <sys/ioctl.h>
> > >  #include <sys/socket.h>
> > > @@ -1143,15 +1144,27 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > >  }
> > >  
> > > -static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > -                                            int queue_idx)
> > > +static void vhost_user_host_notifier_free(VhostUserHostNotifier *n)
> > >  {
> > > -    struct vhost_user *u = dev->opaque;
> > > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > -    VirtIODevice *vdev = dev->vdev;
> > > +    assert(n && n->old_addr);
> > > +    munmap(n->old_addr, qemu_real_host_page_size);
> > > +    n->old_addr = NULL;
> > > +}
> > > +
> > > +static void vhost_user_host_notifier_remove(VhostUserState *user,
> > > +                                            VirtIODevice *vdev, int queue_idx)
> > > +{
> > > +    VhostUserHostNotifier *n = &user->notifier[queue_idx];
> > >  
> > >      if (n->addr) {
> > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > +        if (vdev) {
> > > +            virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > +        }
> > > +        assert(n->addr);
> > > +        assert(!n->old_addr);
> > > +        n->old_addr = n->addr;
> > > +        n->addr = NULL;
> > > +        call_rcu(n, vhost_user_host_notifier_free, rcu);
> > >      }
> > >  }
> > >  
> > > @@ -1190,8 +1203,9 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev,
> > >          .payload.state = *ring,
> > >          .hdr.size = sizeof(msg.payload.state),
> > >      };
> > > +    struct vhost_user *u = dev->opaque;
> > >  
> > > -    vhost_user_host_notifier_remove(dev, ring->index);
> > > +    vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index);
> > >  
> > >      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > >          return -1;
> > > @@ -1486,12 +1500,7 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > >  
> > >      n = &user->notifier[queue_idx];
> > >  
> > > -    if (n->addr) {
> > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > -        object_unparent(OBJECT(&n->mr));
> > > -        munmap(n->addr, page_size);
> > > -        n->addr = NULL;
> > > -    }
> > > +    vhost_user_host_notifier_remove(user, vdev, queue_idx);
> > >  
> > >      if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
> > >          return 0;
> > > @@ -1510,9 +1519,12 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > >  
> > >      name = g_strdup_printf("vhost-user/host-notifier@%p mmaps[%d]",
> > >                             user, queue_idx);
> > > -    if (!n->mr.ram) /* Don't init again after suspend. */
> > > +    if (!n->mr.ram) { /* Don't init again after suspend. */
> > >          memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
> > >                                            page_size, addr);
> > > +    } else {
> > > +        n->mr.ram_block->host = addr;
> > > +    }
> > >      g_free(name);
> > >  
> > >      if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
> > > @@ -2460,17 +2472,17 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp)
> > >  void vhost_user_cleanup(VhostUserState *user)
> > >  {
> > >      int i;
> > > +    VhostUserHostNotifier *n;
> > >  
> > >      if (!user->chr) {
> > >          return;
> > >      }
> > >      memory_region_transaction_begin();
> > >      for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> > > -        if (user->notifier[i].addr) {
> > > -            object_unparent(OBJECT(&user->notifier[i].mr));
> > > -            munmap(user->notifier[i].addr, qemu_real_host_page_size);
> > > -            user->notifier[i].addr = NULL;
> > > -        }
> > > +        n = &user->notifier[i];
> > > +        assert(!n->addr);
> > 
> > I'm pretty confused as to why this assert holds.
> > Add a comment?
> 
> Seems notifiers are removed by vhost_user_get_vring_base(), I used this
> assert to catch exception. Will remove it.

Um I'm not actually asking about that. asserts are good but
how do we know this one holds?

> > 
> > > +        vhost_user_host_notifier_remove(user, NULL, i);
> > > +        object_unparent(OBJECT(&n->mr));
> > >      }
> > >      memory_region_transaction_commit();
> > >      user->chr = NULL;
> > 
> > I'm also confused on why we can do unparent for notifiers which have
> > never been set up. Won't n->mr be invalid then?
> 
> There is a parent check in object_unparent().

It does not seem to be idempotent though in that it does not
set parent to NULL. What if this is called twice?

> > 
> > 
> > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > > index f6012b2078..03aa22d450 100644
> > > --- a/include/hw/virtio/vhost-user.h
> > > +++ b/include/hw/virtio/vhost-user.h
> > > @@ -12,8 +12,10 @@
> > >  #include "hw/virtio/virtio.h"
> > >  
> > >  typedef struct VhostUserHostNotifier {
> > > +    struct rcu_head rcu;
> > >      MemoryRegion mr;
> > >      void *addr;
> > > +    void *old_addr;
> > 
> > That's not a very clear name. Is this literally just
> > "address for the rcu callback to unmap"?
> > Maybe unmap_addr then?
> 
> LGTM, thanks!
> 
> 
> > 
> > >  } VhostUserHostNotifier;
> > >  
> > >  typedef struct VhostUserState {
> > > -- 
> > > 2.33.0
> > 
> 

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Michael S. Tsirkin]

On Tue, Nov 02, 2021 at 06:08:39AM +0000, Xueming(Steven) Li wrote:
> On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:
> > On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:
> > > When vhost-user vdpa client suspend, backend may close all resources,
> > > VQ notifier mmap address become invalid, restore MR which contains
> > > the invalid address is wrong. vdpa client will set VQ notifier after
> > > reconnect.
> > > 
> > > This patch removes VQ notifier restore and related flags to avoid reusing
> > > invalid address.
> > > 
> > > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > > Cc: qemu-stable@nongnu.org
> > > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > ---
> > >  hw/virtio/vhost-user.c         | 19 +------------------
> > >  include/hw/virtio/vhost-user.h |  1 -
> > >  2 files changed, 1 insertion(+), 19 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > index bf6e50223c..c671719e9b 100644
> > > --- a/hw/virtio/vhost-user.c
> > > +++ b/hw/virtio/vhost-user.c
> > > @@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > >  }
> > >  
> > > -static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
> > > -                                             int queue_idx)
> > > -{
> > > -    struct vhost_user *u = dev->opaque;
> > > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > -    VirtIODevice *vdev = dev->vdev;
> > > -
> > > -    if (n->addr && !n->set) {
> > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
> > > -        n->set = true;
> > > -    }
> > > -}
> > > -
> > >  static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > >                                              int queue_idx)
> > >  {
> > > @@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > >      VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > >      VirtIODevice *vdev = dev->vdev;
> > >  
> > > -    if (n->addr && n->set) {
> > > +    if (n->addr) {
> > >          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > -        n->set = false;
> > >      }
> > >  }
> > > 
> > 
> > So on vq stop we still remove the notifier...
> >   
> > >  static int vhost_user_set_vring_base(struct vhost_dev *dev,
> > >                                       struct vhost_vring_state *ring)
> > >  {
> > > -    vhost_user_host_notifier_restore(dev, ring->index);
> > > -
> > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
> > >  }
> > >  
> > 
> > but on vq start we do not reinstate it? Does this not mean that
> > notifiers won't work after stop then start?
> 
> 
> Yes, backend initially work w/o host notifier, request VM driver to
> install notifier if needed - call this function through slave socket.

I think it's cleaner if qemu handles this itself like it did before, it
knows vm is stopped without getting called.

> > 
> > 
> > > @@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > >      }
> > >  
> > >      n->addr = addr;
> > > -    n->set = true;
> > >  
> > >      return 0;
> > >  }
> > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > > index a9abca3288..f6012b2078 100644
> > > --- a/include/hw/virtio/vhost-user.h
> > > +++ b/include/hw/virtio/vhost-user.h
> > > @@ -14,7 +14,6 @@
> > >  typedef struct VhostUserHostNotifier {
> > >      MemoryRegion mr;
> > >      void *addr;
> > > -    bool set;
> > >  } VhostUserHostNotifier;
> > >  
> > >  typedef struct VhostUserState {
> > > -- 
> > > 2.33.0
> > 
> 

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Xueming(Steven) Li]

On Tue, 2021-11-02 at 02:49 -0400, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2021 at 06:08:39AM +0000, Xueming(Steven) Li wrote:
> > On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:
> > > On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:
> > > > When vhost-user vdpa client suspend, backend may close all resources,
> > > > VQ notifier mmap address become invalid, restore MR which contains
> > > > the invalid address is wrong. vdpa client will set VQ notifier after
> > > > reconnect.
> > > > 
> > > > This patch removes VQ notifier restore and related flags to avoid reusing
> > > > invalid address.
> > > > 
> > > > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > > > Cc: qemu-stable@nongnu.org
> > > > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > > ---
> > > >  hw/virtio/vhost-user.c         | 19 +------------------
> > > >  include/hw/virtio/vhost-user.h |  1 -
> > > >  2 files changed, 1 insertion(+), 19 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index bf6e50223c..c671719e9b 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > > >  }
> > > >  
> > > > -static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
> > > > -                                             int queue_idx)
> > > > -{
> > > > -    struct vhost_user *u = dev->opaque;
> > > > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > -    VirtIODevice *vdev = dev->vdev;
> > > > -
> > > > -    if (n->addr && !n->set) {
> > > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
> > > > -        n->set = true;
> > > > -    }
> > > > -}
> > > > -
> > > >  static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > >                                              int queue_idx)
> > > >  {
> > > > @@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > >      VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > >      VirtIODevice *vdev = dev->vdev;
> > > >  
> > > > -    if (n->addr && n->set) {
> > > > +    if (n->addr) {
> > > >          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > > -        n->set = false;
> > > >      }
> > > >  }
> > > > 
> > > 
> > > So on vq stop we still remove the notifier...
> > >   
> > > >  static int vhost_user_set_vring_base(struct vhost_dev *dev,
> > > >                                       struct vhost_vring_state *ring)
> > > >  {
> > > > -    vhost_user_host_notifier_restore(dev, ring->index);
> > > > -
> > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
> > > >  }
> > > >  
> > > 
> > > but on vq start we do not reinstate it? Does this not mean that
> > > notifiers won't work after stop then start?
> > 
> > 
> > Yes, backend initially work w/o host notifier, request VM driver to
> > install notifier if needed - call this function through slave socket.
> 
> I think it's cleaner if qemu handles this itself like it did before, it
> knows vm is stopped without getting called.

If vhost play as server, there are 2 scenario that remove the notifier:
1. VM suspend: backend still there, it's okay to keep mmap address.
2. vhost backend stopped or process killed: resources from backend
should be released. That's why patch 2/2 munmap in notifier remove
function. Then the restore function get nothing to restore, maybe I
shouldn't reverse patch order.

> 
> > > 
> > > 
> > > > @@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > >      }
> > > >  
> > > >      n->addr = addr;
> > > > -    n->set = true;
> > > >  
> > > >      return 0;
> > > >  }
> > > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > > > index a9abca3288..f6012b2078 100644
> > > > --- a/include/hw/virtio/vhost-user.h
> > > > +++ b/include/hw/virtio/vhost-user.h
> > > > @@ -14,7 +14,6 @@
> > > >  typedef struct VhostUserHostNotifier {
> > > >      MemoryRegion mr;
> > > >      void *addr;
> > > > -    bool set;
> > > >  } VhostUserHostNotifier;
> > > >  
> > > >  typedef struct VhostUserState {
> > > > -- 
> > > > 2.33.0
> > > 
> > 
> 

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Michael S. Tsirkin]

On Wed, Nov 03, 2021 at 02:48:41PM +0000, Xueming(Steven) Li wrote:
> On Tue, 2021-11-02 at 02:49 -0400, Michael S. Tsirkin wrote:
> > On Tue, Nov 02, 2021 at 06:08:39AM +0000, Xueming(Steven) Li wrote:
> > > On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:
> > > > On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:
> > > > > When vhost-user vdpa client suspend, backend may close all resources,
> > > > > VQ notifier mmap address become invalid, restore MR which contains
> > > > > the invalid address is wrong. vdpa client will set VQ notifier after
> > > > > reconnect.
> > > > > 
> > > > > This patch removes VQ notifier restore and related flags to avoid reusing
> > > > > invalid address.
> > > > > 
> > > > > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > > > > Cc: qemu-stable@nongnu.org
> > > > > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > > > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > > > ---
> > > > >  hw/virtio/vhost-user.c         | 19 +------------------
> > > > >  include/hw/virtio/vhost-user.h |  1 -
> > > > >  2 files changed, 1 insertion(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > index bf6e50223c..c671719e9b 100644
> > > > > --- a/hw/virtio/vhost-user.c
> > > > > +++ b/hw/virtio/vhost-user.c
> > > > > @@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > > > >  }
> > > > >  
> > > > > -static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
> > > > > -                                             int queue_idx)
> > > > > -{
> > > > > -    struct vhost_user *u = dev->opaque;
> > > > > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > > -    VirtIODevice *vdev = dev->vdev;
> > > > > -
> > > > > -    if (n->addr && !n->set) {
> > > > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
> > > > > -        n->set = true;
> > > > > -    }
> > > > > -}
> > > > > -
> > > > >  static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > >                                              int queue_idx)
> > > > >  {
> > > > > @@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > >      VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > >      VirtIODevice *vdev = dev->vdev;
> > > > >  
> > > > > -    if (n->addr && n->set) {
> > > > > +    if (n->addr) {
> > > > >          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > > > -        n->set = false;
> > > > >      }
> > > > >  }
> > > > > 
> > > > 
> > > > So on vq stop we still remove the notifier...
> > > >   
> > > > >  static int vhost_user_set_vring_base(struct vhost_dev *dev,
> > > > >                                       struct vhost_vring_state *ring)
> > > > >  {
> > > > > -    vhost_user_host_notifier_restore(dev, ring->index);
> > > > > -
> > > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
> > > > >  }
> > > > >  
> > > > 
> > > > but on vq start we do not reinstate it? Does this not mean that
> > > > notifiers won't work after stop then start?
> > > 
> > > 
> > > Yes, backend initially work w/o host notifier, request VM driver to
> > > install notifier if needed - call this function through slave socket.
> > 
> > I think it's cleaner if qemu handles this itself like it did before, it
> > knows vm is stopped without getting called.
> 
> If vhost play as server, there are 2 scenario that remove the notifier:
> 1. VM suspend: backend still there, it's okay to keep mmap address.
> 2. vhost backend stopped or process killed: resources from backend
> should be released. That's why patch 2/2 munmap in notifier remove
> function. Then the restore function get nothing to restore, maybe I
> shouldn't reverse patch order.

I can't say I understand what you mean here. Do you plan to change
the patchset in some way?
When you do, pls include a cover letter with a changelog and
Cc all people you include on patches on the cover letter too.

> > 
> > > > 
> > > > 
> > > > > @@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > > >      }
> > > > >  
> > > > >      n->addr = addr;
> > > > > -    n->set = true;
> > > > >  
> > > > >      return 0;
> > > > >  }
> > > > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > > > > index a9abca3288..f6012b2078 100644
> > > > > --- a/include/hw/virtio/vhost-user.h
> > > > > +++ b/include/hw/virtio/vhost-user.h
> > > > > @@ -14,7 +14,6 @@
> > > > >  typedef struct VhostUserHostNotifier {
> > > > >      MemoryRegion mr;
> > > > >      void *addr;
> > > > > -    bool set;
> > > > >  } VhostUserHostNotifier;
> > > > >  
> > > > >  typedef struct VhostUserState {
> > > > > -- 
> > > > > 2.33.0
> > > > 
> > > 
> > 
> 

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Xueming(Steven) Li]

On Wed, 2021-11-03 at 16:30 -0400, Michael S. Tsirkin wrote:
> On Wed, Nov 03, 2021 at 02:48:41PM +0000, Xueming(Steven) Li wrote:
> > On Tue, 2021-11-02 at 02:49 -0400, Michael S. Tsirkin wrote:
> > > On Tue, Nov 02, 2021 at 06:08:39AM +0000, Xueming(Steven) Li wrote:
> > > > On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:
> > > > > On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:
> > > > > > When vhost-user vdpa client suspend, backend may close all resources,
> > > > > > VQ notifier mmap address become invalid, restore MR which contains
> > > > > > the invalid address is wrong. vdpa client will set VQ notifier after
> > > > > > reconnect.
> > > > > > 
> > > > > > This patch removes VQ notifier restore and related flags to avoid reusing
> > > > > > invalid address.
> > > > > > 
> > > > > > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > > > > > Cc: qemu-stable@nongnu.org
> > > > > > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > > > > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > > > > ---
> > > > > >  hw/virtio/vhost-user.c         | 19 +------------------
> > > > > >  include/hw/virtio/vhost-user.h |  1 -
> > > > > >  2 files changed, 1 insertion(+), 19 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > > index bf6e50223c..c671719e9b 100644
> > > > > > --- a/hw/virtio/vhost-user.c
> > > > > > +++ b/hw/virtio/vhost-user.c
> > > > > > @@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > > > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > > > > >  }
> > > > > >  
> > > > > > -static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
> > > > > > -                                             int queue_idx)
> > > > > > -{
> > > > > > -    struct vhost_user *u = dev->opaque;
> > > > > > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > > > -    VirtIODevice *vdev = dev->vdev;
> > > > > > -
> > > > > > -    if (n->addr && !n->set) {
> > > > > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
> > > > > > -        n->set = true;
> > > > > > -    }
> > > > > > -}
> > > > > > -
> > > > > >  static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > > >                                              int queue_idx)
> > > > > >  {
> > > > > > @@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > > >      VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > > >      VirtIODevice *vdev = dev->vdev;
> > > > > >  
> > > > > > -    if (n->addr && n->set) {
> > > > > > +    if (n->addr) {
> > > > > >          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > > > > -        n->set = false;
> > > > > >      }
> > > > > >  }
> > > > > > 
> > > > > 
> > > > > So on vq stop we still remove the notifier...
> > > > >   
> > > > > >  static int vhost_user_set_vring_base(struct vhost_dev *dev,
> > > > > >                                       struct vhost_vring_state *ring)
> > > > > >  {
> > > > > > -    vhost_user_host_notifier_restore(dev, ring->index);
> > > > > > -
> > > > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
> > > > > >  }
> > > > > >  
> > > > > 
> > > > > but on vq start we do not reinstate it? Does this not mean that
> > > > > notifiers won't work after stop then start?
> > > > 
> > > > 
> > > > Yes, backend initially work w/o host notifier, request VM driver to
> > > > install notifier if needed - call this function through slave socket.
> > > 
> > > I think it's cleaner if qemu handles this itself like it did before, it
> > > knows vm is stopped without getting called.
> > 
> > If vhost play as server, there are 2 scenario that remove the notifier:
> > 1. VM suspend: backend still there, it's okay to keep mmap address.
> > 2. vhost backend stopped or process killed: resources from backend
> > should be released. That's why patch 2/2 munmap in notifier remove
> > function. Then the restore function get nothing to restore, maybe I
> > shouldn't reverse patch order.
> 
> I can't say I understand what you mean here. Do you plan to change
> the patchset in some way?
> When you do, pls include a cover letter with a changelog and
> Cc all people you include on patches on the cover letter too. 

Here is the detail of the problem I encountered, my vhost backend is
DPDK vdpa user space application. Notifier address is set when vdpa ask
qemu to mmap a FD and an offset from vdpa, see function
vhost_user_slave_handle_vring_host_notifier(). If the vdpa application
restart of get killed for some reason,
vhost_user_host_notifier_remove() is called and notifier MR is
uninstalled, the notifier address that retrieved from mmap is
referencing to an invalid FD, not released. This will cause HW
resources on kernel side still referenced, most important, when vdpa
connection restored, this invalid notifier will be be restored as
notifier MR.

To resolve it, have to remove the notifer restore mechanism, vDPA
application will issue client socket request again to install notifier
to VM, so no concern that the notifier will be lost after resume.

Since vdpa might be killed, no chance to notify qemu to remove
notifier. An alternative solution is to detect sock disconnection and
unmmap notifier, but it looks more complex to me. How do you think?


> 
> > > 
> > > > > 
> > > > > 
> > > > > > @@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > > > >      }
> > > > > >  
> > > > > >      n->addr = addr;
> > > > > > -    n->set = true;
> > > > > >  
> > > > > >      return 0;
> > > > > >  }
> > > > > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > > > > > index a9abca3288..f6012b2078 100644
> > > > > > --- a/include/hw/virtio/vhost-user.h
> > > > > > +++ b/include/hw/virtio/vhost-user.h
> > > > > > @@ -14,7 +14,6 @@
> > > > > >  typedef struct VhostUserHostNotifier {
> > > > > >      MemoryRegion mr;
> > > > > >      void *addr;
> > > > > > -    bool set;
> > > > > >  } VhostUserHostNotifier;
> > > > > >  
> > > > > >  typedef struct VhostUserState {
> > > > > > -- 
> > > > > > 2.33.0
> > > > > 
> > > > 
> > > 
> > 
> 

Re: [PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup

[Xueming(Steven) Li]

On Tue, 2021-11-02 at 02:47 -0400, Michael S. Tsirkin wrote:
> On Tue, Nov 02, 2021 at 06:00:58AM +0000, Xueming(Steven) Li wrote:
> > On Mon, 2021-11-01 at 17:00 -0400, Michael S. Tsirkin wrote:
> > > On Mon, Nov 01, 2021 at 04:38:13PM +0800, Xueming Li wrote:
> > > > When vhost-user device cleanup is executed and un-mmaps notifier
> > > > address, VM cpu thread writing the notifier fails by accessing invalid
> > > > address error.
> > > > 
> > > > To avoid this concurrent issue, call RCU and wait for a memory flatview
> > > > update, then un-mmap notifiers in callback.
> > > > 
> > > > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > > > Cc: qemu-stable@nongnu.org
> > > > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > > ---
> > > >  hw/virtio/vhost-user.c         | 50 +++++++++++++++++++++-------------
> > > >  include/hw/virtio/vhost-user.h |  2 ++
> > > >  2 files changed, 33 insertions(+), 19 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index c671719e9b..5adad4d029 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -25,6 +25,7 @@
> > > >  #include "migration/migration.h"
> > > >  #include "migration/postcopy-ram.h"
> > > >  #include "trace.h"
> > > > +#include "exec/ramblock.h"
> > > >  
> > > >  #include <sys/ioctl.h>
> > > >  #include <sys/socket.h>
> > > > @@ -1143,15 +1144,27 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > > >  }
> > > >  
> > > > -static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > -                                            int queue_idx)
> > > > +static void vhost_user_host_notifier_free(VhostUserHostNotifier *n)
> > > >  {
> > > > -    struct vhost_user *u = dev->opaque;
> > > > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > -    VirtIODevice *vdev = dev->vdev;
> > > > +    assert(n && n->old_addr);
> > > > +    munmap(n->old_addr, qemu_real_host_page_size);
> > > > +    n->old_addr = NULL;
> > > > +}
> > > > +
> > > > +static void vhost_user_host_notifier_remove(VhostUserState *user,
> > > > +                                            VirtIODevice *vdev, int queue_idx)
> > > > +{
> > > > +    VhostUserHostNotifier *n = &user->notifier[queue_idx];
> > > >  
> > > >      if (n->addr) {
> > > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > > +        if (vdev) {
> > > > +            virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > > +        }
> > > > +        assert(n->addr);
> > > > +        assert(!n->old_addr);
> > > > +        n->old_addr = n->addr;
> > > > +        n->addr = NULL;
> > > > +        call_rcu(n, vhost_user_host_notifier_free, rcu);
> > > >      }
> > > >  }
> > > >  
> > > > @@ -1190,8 +1203,9 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev,
> > > >          .payload.state = *ring,
> > > >          .hdr.size = sizeof(msg.payload.state),
> > > >      };
> > > > +    struct vhost_user *u = dev->opaque;
> > > >  
> > > > -    vhost_user_host_notifier_remove(dev, ring->index);
> > > > +    vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index);
> > > >  
> > > >      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> > > >          return -1;
> > > > @@ -1486,12 +1500,7 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > >  
> > > >      n = &user->notifier[queue_idx];
> > > >  
> > > > -    if (n->addr) {
> > > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > > -        object_unparent(OBJECT(&n->mr));
> > > > -        munmap(n->addr, page_size);
> > > > -        n->addr = NULL;
> > > > -    }
> > > > +    vhost_user_host_notifier_remove(user, vdev, queue_idx);
> > > >  
> > > >      if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
> > > >          return 0;
> > > > @@ -1510,9 +1519,12 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > >  
> > > >      name = g_strdup_printf("vhost-user/host-notifier@%p mmaps[%d]",
> > > >                             user, queue_idx);
> > > > -    if (!n->mr.ram) /* Don't init again after suspend. */
> > > > +    if (!n->mr.ram) { /* Don't init again after suspend. */
> > > >          memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
> > > >                                            page_size, addr);
> > > > +    } else {
> > > > +        n->mr.ram_block->host = addr;
> > > > +    }
> > > >      g_free(name);
> > > >  
> > > >      if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
> > > > @@ -2460,17 +2472,17 @@ bool vhost_user_init(VhostUserState *user, CharBackend *chr, Error **errp)
> > > >  void vhost_user_cleanup(VhostUserState *user)
> > > >  {
> > > >      int i;
> > > > +    VhostUserHostNotifier *n;
> > > >  
> > > >      if (!user->chr) {
> > > >          return;
> > > >      }
> > > >      memory_region_transaction_begin();
> > > >      for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> > > > -        if (user->notifier[i].addr) {
> > > > -            object_unparent(OBJECT(&user->notifier[i].mr));
> > > > -            munmap(user->notifier[i].addr, qemu_real_host_page_size);
> > > > -            user->notifier[i].addr = NULL;
> > > > -        }
> > > > +        n = &user->notifier[i];
> > > > +        assert(!n->addr);
> > > 
> > > I'm pretty confused as to why this assert holds.
> > > Add a comment?
> > 
> > Seems notifiers are removed by vhost_user_get_vring_base(), I used this
> > assert to catch exception. Will remove it.
> 
> Um I'm not actually asking about that. asserts are good but
> how do we know this one holds?

Normally notifier is removed in get_vring_base stage, not sure why here
we try to cleanup notifiers again. That's why I added assert to caputre
the case, just for debug. We don't need it for sure as formal version.

> 
> > > 
> > > > +        vhost_user_host_notifier_remove(user, NULL, i);
> > > > +        object_unparent(OBJECT(&n->mr));
> > > >      }
> > > >      memory_region_transaction_commit();
> > > >      user->chr = NULL;
> > > 
> > > I'm also confused on why we can do unparent for notifiers which have
> > > never been set up. Won't n->mr be invalid then?
> > 
> > There is a parent check in object_unparent().
> 
> It does not seem to be idempotent though in that it does not
> set parent to NULL. What if this is called twice?

This patch introduced why we need it:
1f89d3b91e3e ("hw/virtio: Fix leak of host-notifier memory-region")

If called twice, n->mr->parent is NULL and object_unparent() will do
nothing.

> 
> > > 
> > > 
> > > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > > > index f6012b2078..03aa22d450 100644
> > > > --- a/include/hw/virtio/vhost-user.h
> > > > +++ b/include/hw/virtio/vhost-user.h
> > > > @@ -12,8 +12,10 @@
> > > >  #include "hw/virtio/virtio.h"
> > > >  
> > > >  typedef struct VhostUserHostNotifier {
> > > > +    struct rcu_head rcu;
> > > >      MemoryRegion mr;
> > > >      void *addr;
> > > > +    void *old_addr;
> > > 
> > > That's not a very clear name. Is this literally just
> > > "address for the rcu callback to unmap"?
> > > Maybe unmap_addr then?
> > 
> > LGTM, thanks!
> > 
> > 
> > > 
> > > >  } VhostUserHostNotifier;
> > > >  
> > > >  typedef struct VhostUserState {
> > > > -- 
> > > > 2.33.0
> > > 
> > 
> 

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Michael S. Tsirkin]

I dropped this for now as I'm a bit lost with this patchset.
Cc Raphael maybe he'll understand it better.

On Wed, Jan 12, 2022 at 03:05:15PM +0000, Xueming(Steven) Li wrote:
> On Wed, 2021-11-03 at 16:30 -0400, Michael S. Tsirkin wrote:
> > On Wed, Nov 03, 2021 at 02:48:41PM +0000, Xueming(Steven) Li wrote:
> > > On Tue, 2021-11-02 at 02:49 -0400, Michael S. Tsirkin wrote:
> > > > On Tue, Nov 02, 2021 at 06:08:39AM +0000, Xueming(Steven) Li wrote:
> > > > > On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:
> > > > > > On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:
> > > > > > > When vhost-user vdpa client suspend, backend may close all resources,
> > > > > > > VQ notifier mmap address become invalid, restore MR which contains
> > > > > > > the invalid address is wrong. vdpa client will set VQ notifier after
> > > > > > > reconnect.
> > > > > > > 
> > > > > > > This patch removes VQ notifier restore and related flags to avoid reusing
> > > > > > > invalid address.
> > > > > > > 
> > > > > > > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > > > > > > Cc: qemu-stable@nongnu.org
> > > > > > > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > > > > > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > > > > > ---
> > > > > > >  hw/virtio/vhost-user.c         | 19 +------------------
> > > > > > >  include/hw/virtio/vhost-user.h |  1 -
> > > > > > >  2 files changed, 1 insertion(+), 19 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > > > index bf6e50223c..c671719e9b 100644
> > > > > > > --- a/hw/virtio/vhost-user.c
> > > > > > > +++ b/hw/virtio/vhost-user.c
> > > > > > > @@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > > > > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > > > > > >  }
> > > > > > >  
> > > > > > > -static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
> > > > > > > -                                             int queue_idx)
> > > > > > > -{
> > > > > > > -    struct vhost_user *u = dev->opaque;
> > > > > > > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > > > > -    VirtIODevice *vdev = dev->vdev;
> > > > > > > -
> > > > > > > -    if (n->addr && !n->set) {
> > > > > > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
> > > > > > > -        n->set = true;
> > > > > > > -    }
> > > > > > > -}
> > > > > > > -
> > > > > > >  static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > > > >                                              int queue_idx)
> > > > > > >  {
> > > > > > > @@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > > > >      VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > > > >      VirtIODevice *vdev = dev->vdev;
> > > > > > >  
> > > > > > > -    if (n->addr && n->set) {
> > > > > > > +    if (n->addr) {
> > > > > > >          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > > > > > -        n->set = false;
> > > > > > >      }
> > > > > > >  }
> > > > > > > 
> > > > > > 
> > > > > > So on vq stop we still remove the notifier...
> > > > > >   
> > > > > > >  static int vhost_user_set_vring_base(struct vhost_dev *dev,
> > > > > > >                                       struct vhost_vring_state *ring)
> > > > > > >  {
> > > > > > > -    vhost_user_host_notifier_restore(dev, ring->index);
> > > > > > > -
> > > > > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
> > > > > > >  }
> > > > > > >  
> > > > > > 
> > > > > > but on vq start we do not reinstate it? Does this not mean that
> > > > > > notifiers won't work after stop then start?
> > > > > 
> > > > > 
> > > > > Yes, backend initially work w/o host notifier, request VM driver to
> > > > > install notifier if needed - call this function through slave socket.
> > > > 
> > > > I think it's cleaner if qemu handles this itself like it did before, it
> > > > knows vm is stopped without getting called.
> > > 
> > > If vhost play as server, there are 2 scenario that remove the notifier:
> > > 1. VM suspend: backend still there, it's okay to keep mmap address.
> > > 2. vhost backend stopped or process killed: resources from backend
> > > should be released. That's why patch 2/2 munmap in notifier remove
> > > function. Then the restore function get nothing to restore, maybe I
> > > shouldn't reverse patch order.
> > 
> > I can't say I understand what you mean here. Do you plan to change
> > the patchset in some way?
> > When you do, pls include a cover letter with a changelog and
> > Cc all people you include on patches on the cover letter too. 
> 
> Here is the detail of the problem I encountered, my vhost backend is
> DPDK vdpa user space application. Notifier address is set when vdpa ask
> qemu to mmap a FD and an offset from vdpa, see function
> vhost_user_slave_handle_vring_host_notifier(). If the vdpa application
> restart of get killed for some reason,
> vhost_user_host_notifier_remove() is called and notifier MR is
> uninstalled, the notifier address that retrieved from mmap is
> referencing to an invalid FD, not released. This will cause HW
> resources on kernel side still referenced, most important, when vdpa
> connection restored, this invalid notifier will be be restored as
> notifier MR.
> 
> To resolve it, have to remove the notifer restore mechanism, vDPA
> application will issue client socket request again to install notifier
> to VM, so no concern that the notifier will be lost after resume.
> 
> Since vdpa might be killed, no chance to notify qemu to remove
> notifier. An alternative solution is to detect sock disconnection and
> unmmap notifier, but it looks more complex to me. How do you think?
> 
> 
> > 
> > > > 
> > > > > > 
> > > > > > 
> > > > > > > @@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > > > > >      }
> > > > > > >  
> > > > > > >      n->addr = addr;
> > > > > > > -    n->set = true;
> > > > > > >  
> > > > > > >      return 0;
> > > > > > >  }
> > > > > > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > > > > > > index a9abca3288..f6012b2078 100644
> > > > > > > --- a/include/hw/virtio/vhost-user.h
> > > > > > > +++ b/include/hw/virtio/vhost-user.h
> > > > > > > @@ -14,7 +14,6 @@
> > > > > > >  typedef struct VhostUserHostNotifier {
> > > > > > >      MemoryRegion mr;
> > > > > > >      void *addr;
> > > > > > > -    bool set;
> > > > > > >  } VhostUserHostNotifier;
> > > > > > >  
> > > > > > >  typedef struct VhostUserState {
> > > > > > > -- 
> > > > > > > 2.33.0
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Xueming(Steven) Li]

[-- Attachment #1: Type: text/plain, Size: 5928 bytes --]

v7 sent with more description, please check.


Thanks,

Xueming


On Fri, 2022-02-04 at 07:25 -0500, Michael S. Tsirkin wrote:

I dropped this for now as I'm a bit lost with this patchset.

Cc Raphael maybe he'll understand it better.


On Wed, Jan 12, 2022 at 03:05:15PM +0000, Xueming(Steven) Li wrote:

On Wed, 2021-11-03 at 16:30 -0400, Michael S. Tsirkin wrote:

On Wed, Nov 03, 2021 at 02:48:41PM +0000, Xueming(Steven) Li wrote:

On Tue, 2021-11-02 at 02:49 -0400, Michael S. Tsirkin wrote:

On Tue, Nov 02, 2021 at 06:08:39AM +0000, Xueming(Steven) Li wrote:

On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:

On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:

When vhost-user vdpa client suspend, backend may close all resources,

VQ notifier mmap address become invalid, restore MR which contains

the invalid address is wrong. vdpa client will set VQ notifier after

reconnect.


This patch removes VQ notifier restore and related flags to avoid reusing

invalid address.


Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")

Cc: qemu-stable@nongnu.org<mailto:qemu-stable@nongnu.org>

Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com<mailto:zhangyuwei.9149@bytedance.com>>

Signed-off-by: Xueming Li <xuemingl@nvidia.com<mailto:xuemingl@nvidia.com>>

---

 hw/virtio/vhost-user.c         | 19 +------------------

 include/hw/virtio/vhost-user.h |  1 -

 2 files changed, 1 insertion(+), 19 deletions(-)


diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c

index bf6e50223c..c671719e9b 100644

--- a/hw/virtio/vhost-user.c

+++ b/hw/virtio/vhost-user.c

@@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,

     return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);

 }



-static void vhost_user_host_notifier_restore(struct vhost_dev *dev,

-                                             int queue_idx)

-{

-    struct vhost_user *u = dev->opaque;

-    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];

-    VirtIODevice *vdev = dev->vdev;

-

-    if (n->addr && !n->set) {

-        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);

-        n->set = true;

-    }

-}

-

 static void vhost_user_host_notifier_remove(struct vhost_dev *dev,

                                             int queue_idx)

 {

@@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,

     VhostUserHostNotifier *n = &u->user->notifier[queue_idx];

     VirtIODevice *vdev = dev->vdev;



-    if (n->addr && n->set) {

+    if (n->addr) {

         virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);

-        n->set = false;

     }

 }



So on vq stop we still remove the notifier...



 static int vhost_user_set_vring_base(struct vhost_dev *dev,

                                      struct vhost_vring_state *ring)

 {

-    vhost_user_host_notifier_restore(dev, ring->index);

-

     return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);

 }




but on vq start we do not reinstate it? Does this not mean that

notifiers won't work after stop then start?



Yes, backend initially work w/o host notifier, request VM driver to

install notifier if needed - call this function through slave socket.


I think it's cleaner if qemu handles this itself like it did before, it

knows vm is stopped without getting called.


If vhost play as server, there are 2 scenario that remove the notifier:

1. VM suspend: backend still there, it's okay to keep mmap address.

2. vhost backend stopped or process killed: resources from backend

should be released. That's why patch 2/2 munmap in notifier remove

function. Then the restore function get nothing to restore, maybe I

shouldn't reverse patch order.


I can't say I understand what you mean here. Do you plan to change

the patchset in some way?

When you do, pls include a cover letter with a changelog and

Cc all people you include on patches on the cover letter too.


Here is the detail of the problem I encountered, my vhost backend is

DPDK vdpa user space application. Notifier address is set when vdpa ask

qemu to mmap a FD and an offset from vdpa, see function

vhost_user_slave_handle_vring_host_notifier(). If the vdpa application

restart of get killed for some reason,

vhost_user_host_notifier_remove() is called and notifier MR is

uninstalled, the notifier address that retrieved from mmap is

referencing to an invalid FD, not released. This will cause HW

resources on kernel side still referenced, most important, when vdpa

connection restored, this invalid notifier will be be restored as

notifier MR.


To resolve it, have to remove the notifer restore mechanism, vDPA

application will issue client socket request again to install notifier

to VM, so no concern that the notifier will be lost after resume.


Since vdpa might be killed, no chance to notify qemu to remove

notifier. An alternative solution is to detect sock disconnection and

unmmap notifier, but it looks more complex to me. How do you think?







@@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,

     }



     n->addr = addr;

-    n->set = true;



     return 0;

 }

diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h

index a9abca3288..f6012b2078 100644

--- a/include/hw/virtio/vhost-user.h

+++ b/include/hw/virtio/vhost-user.h

@@ -14,7 +14,6 @@

 typedef struct VhostUserHostNotifier {

     MemoryRegion mr;

     void *addr;

-    bool set;

 } VhostUserHostNotifier;



 typedef struct VhostUserState {

--

2.33.0










[-- Attachment #2: Type: text/html, Size: 9577 bytes --]

Re: [PATCH v6 1/2] vhost-user: remove VirtQ notifier restore

[Xueming(Steven) Li]

v7 sent with more description, please check.

Thanks,
Xueming

On Fri, 2022-02-04 at 07:25 -0500, Michael S. Tsirkin wrote:
> I dropped this for now as I'm a bit lost with this patchset.
> Cc Raphael maybe he'll understand it better.
> 
> On Wed, Jan 12, 2022 at 03:05:15PM +0000, Xueming(Steven) Li wrote:
> > On Wed, 2021-11-03 at 16:30 -0400, Michael S. Tsirkin wrote:
> > > On Wed, Nov 03, 2021 at 02:48:41PM +0000, Xueming(Steven) Li wrote:
> > > > On Tue, 2021-11-02 at 02:49 -0400, Michael S. Tsirkin wrote:
> > > > > On Tue, Nov 02, 2021 at 06:08:39AM +0000, Xueming(Steven) Li wrote:
> > > > > > On Mon, 2021-11-01 at 17:06 -0400, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Nov 01, 2021 at 04:38:12PM +0800, Xueming Li wrote:
> > > > > > > > When vhost-user vdpa client suspend, backend may close all resources,
> > > > > > > > VQ notifier mmap address become invalid, restore MR which contains
> > > > > > > > the invalid address is wrong. vdpa client will set VQ notifier after
> > > > > > > > reconnect.
> > > > > > > > 
> > > > > > > > This patch removes VQ notifier restore and related flags to avoid reusing
> > > > > > > > invalid address.
> > > > > > > > 
> > > > > > > > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > > > > > > > Cc: qemu-stable@nongnu.org
> > > > > > > > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > > > > > > > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> > > > > > > > ---
> > > > > > > >  hw/virtio/vhost-user.c         | 19 +------------------
> > > > > > > >  include/hw/virtio/vhost-user.h |  1 -
> > > > > > > >  2 files changed, 1 insertion(+), 19 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > > > > index bf6e50223c..c671719e9b 100644
> > > > > > > > --- a/hw/virtio/vhost-user.c
> > > > > > > > +++ b/hw/virtio/vhost-user.c
> > > > > > > > @@ -1143,19 +1143,6 @@ static int vhost_user_set_vring_num(struct vhost_dev *dev,
> > > > > > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > -static void vhost_user_host_notifier_restore(struct vhost_dev *dev,
> > > > > > > > -                                             int queue_idx)
> > > > > > > > -{
> > > > > > > > -    struct vhost_user *u = dev->opaque;
> > > > > > > > -    VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > > > > > -    VirtIODevice *vdev = dev->vdev;
> > > > > > > > -
> > > > > > > > -    if (n->addr && !n->set) {
> > > > > > > > -        virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true);
> > > > > > > > -        n->set = true;
> > > > > > > > -    }
> > > > > > > > -}
> > > > > > > > -
> > > > > > > >  static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > > > > >                                              int queue_idx)
> > > > > > > >  {
> > > > > > > > @@ -1163,17 +1150,14 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> > > > > > > >      VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> > > > > > > >      VirtIODevice *vdev = dev->vdev;
> > > > > > > >  
> > > > > > > > -    if (n->addr && n->set) {
> > > > > > > > +    if (n->addr) {
> > > > > > > >          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > > > > > > > -        n->set = false;
> > > > > > > >      }
> > > > > > > >  }
> > > > > > > > 
> > > > > > > 
> > > > > > > So on vq stop we still remove the notifier...
> > > > > > >   
> > > > > > > >  static int vhost_user_set_vring_base(struct vhost_dev *dev,
> > > > > > > >                                       struct vhost_vring_state *ring)
> > > > > > > >  {
> > > > > > > > -    vhost_user_host_notifier_restore(dev, ring->index);
> > > > > > > > -
> > > > > > > >      return vhost_set_vring(dev, VHOST_USER_SET_VRING_BASE, ring);
> > > > > > > >  }
> > > > > > > >  
> > > > > > > 
> > > > > > > but on vq start we do not reinstate it? Does this not mean that
> > > > > > > notifiers won't work after stop then start?
> > > > > > 
> > > > > > 
> > > > > > Yes, backend initially work w/o host notifier, request VM driver to
> > > > > > install notifier if needed - call this function through slave socket.
> > > > > 
> > > > > I think it's cleaner if qemu handles this itself like it did before, it
> > > > > knows vm is stopped without getting called.
> > > > 
> > > > If vhost play as server, there are 2 scenario that remove the notifier:
> > > > 1. VM suspend: backend still there, it's okay to keep mmap address.
> > > > 2. vhost backend stopped or process killed: resources from backend
> > > > should be released. That's why patch 2/2 munmap in notifier remove
> > > > function. Then the restore function get nothing to restore, maybe I
> > > > shouldn't reverse patch order.
> > > 
> > > I can't say I understand what you mean here. Do you plan to change
> > > the patchset in some way?
> > > When you do, pls include a cover letter with a changelog and
> > > Cc all people you include on patches on the cover letter too. 
> > 
> > Here is the detail of the problem I encountered, my vhost backend is
> > DPDK vdpa user space application. Notifier address is set when vdpa ask
> > qemu to mmap a FD and an offset from vdpa, see function
> > vhost_user_slave_handle_vring_host_notifier(). If the vdpa application
> > restart of get killed for some reason,
> > vhost_user_host_notifier_remove() is called and notifier MR is
> > uninstalled, the notifier address that retrieved from mmap is
> > referencing to an invalid FD, not released. This will cause HW
> > resources on kernel side still referenced, most important, when vdpa
> > connection restored, this invalid notifier will be be restored as
> > notifier MR.
> > 
> > To resolve it, have to remove the notifer restore mechanism, vDPA
> > application will issue client socket request again to install notifier
> > to VM, so no concern that the notifier will be lost after resume.
> > 
> > Since vdpa might be killed, no chance to notify qemu to remove
> > notifier. An alternative solution is to detect sock disconnection and
> > unmmap notifier, but it looks more complex to me. How do you think?
> > 
> > 
> > > 
> > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > > @@ -1538,7 +1522,6 @@ static int vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
> > > > > > > >      }
> > > > > > > >  
> > > > > > > >      n->addr = addr;
> > > > > > > > -    n->set = true;
> > > > > > > >  
> > > > > > > >      return 0;
> > > > > > > >  }
> > > > > > > > diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> > > > > > > > index a9abca3288..f6012b2078 100644
> > > > > > > > --- a/include/hw/virtio/vhost-user.h
> > > > > > > > +++ b/include/hw/virtio/vhost-user.h
> > > > > > > > @@ -14,7 +14,6 @@
> > > > > > > >  typedef struct VhostUserHostNotifier {
> > > > > > > >      MemoryRegion mr;
> > > > > > > >      void *addr;
> > > > > > > > -    bool set;
> > > > > > > >  } VhostUserHostNotifier;
> > > > > > > >  
> > > > > > > >  typedef struct VhostUserState {
> > > > > > > > -- 
> > > > > > > > 2.33.0
> > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
>