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: [email protected]<mailto:[email protected]>
Cc: Yuwei Zhang
<[email protected]<mailto:[email protected]>>
Signed-off-by: Xueming Li <[email protected]<mailto:[email protected]>>
---
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
