| Message ID | 20210209153757.1653598-8-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vDPA shadow virtqueue - notifications forwarding | expand |
On Tue, Feb 09, 2021 at 04:37:57PM +0100, Eugenio Pérez wrote: > @@ -40,6 +42,26 @@ static void vhost_handle_guest_kick(EventNotifier *n) > } > } > > +/* Forward vhost notifications */ > +static void vhost_handle_call(EventNotifier *n) The name vhost_shadow_vq_handle_call() expresses the purpose of the function more clearly. > @@ -75,8 +102,19 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > /* Check for pending notifications from the guest */ > vhost_handle_guest_kick(&svq->host_notifier); > > + r = dev->vhost_ops->vhost_set_vring_call(dev, &call_file); > + if (r != 0) { > + error_report("Couldn't set call fd: %s", strerror(errno)); > + goto err_set_vring_call; > + } This ignores notifier_is_masked and always unmasks. > @@ -1608,6 +1607,10 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n, > if (mask) { > assert(vdev->use_guest_notifier_mask); > file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier); > + } else if (hdev->sw_lm_enabled) { > + VhostShadowVirtqueue *svq = hdev->shadow_vqs[n]; > + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq); > + file.fd = event_notifier_get_fd(e); > } else { > file.fd = event_notifier_get_fd(virtio_queue_get_guest_notifier(vvq)); > } Maybe you can extend this function so it can be called unconditionally from both vhost_shadow_vq_start_rcu() and vhost_shadow_vq_stop_rcu(). It would be a single place that invokes vhost_set_vring_call().
On Wed, Feb 17, 2021 at 6:24 PM Stefan Hajnoczi <stefanha@redhat.com> wrote: > > On Tue, Feb 09, 2021 at 04:37:57PM +0100, Eugenio Pérez wrote: > > @@ -40,6 +42,26 @@ static void vhost_handle_guest_kick(EventNotifier *n) > > } > > } > > > > +/* Forward vhost notifications */ > > +static void vhost_handle_call(EventNotifier *n) > > The name vhost_shadow_vq_handle_call() expresses the purpose of the > function more clearly. > I will rename it. > > @@ -75,8 +102,19 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > > /* Check for pending notifications from the guest */ > > vhost_handle_guest_kick(&svq->host_notifier); > > > > + r = dev->vhost_ops->vhost_set_vring_call(dev, &call_file); > > + if (r != 0) { > > + error_report("Couldn't set call fd: %s", strerror(errno)); > > + goto err_set_vring_call; > > + } > > This ignores notifier_is_masked and always unmasks. Right, I will check for it too. > > > @@ -1608,6 +1607,10 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n, > > if (mask) { > > assert(vdev->use_guest_notifier_mask); > > file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier); > > + } else if (hdev->sw_lm_enabled) { > > + VhostShadowVirtqueue *svq = hdev->shadow_vqs[n]; > > + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq); > > + file.fd = event_notifier_get_fd(e); > > } else { > > file.fd = event_notifier_get_fd(virtio_queue_get_guest_notifier(vvq)); > > } > > Maybe you can extend this function so it can be called unconditionally > from both vhost_shadow_vq_start_rcu() and vhost_shadow_vq_stop_rcu(). It > would be a single place that invokes vhost_set_vring_call(). Your proposal is better, but I'm not sure if something depends on calling mask(..., false) repeatedly. For example, if guest_notifier changes. However, from SVQ point of view we are only interested in avoiding to set masked notifiers over a vhost already masked: unmasked state will always incur on a file descriptor change. So I think it should be fine if we allow unmasking, In other words, allow calling vhost_set_vring_call(guest_notifier()) repeatedly, so old behavior is preserved, and only omit the mask(..., true) if the notifier is already masked, so we can call it unconditionally on vhost_shadow_vq_start/stop. Thanks!
On 2021/2/9 11:37 下午, Eugenio Pérez wrote: > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-shadow-virtqueue.h | 2 ++ > hw/virtio/vhost-shadow-virtqueue.c | 49 ++++++++++++++++++++++++++++++ > hw/virtio/vhost.c | 5 ++- > 3 files changed, 55 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h > index c45035c4b7..210133434c 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.h > +++ b/hw/virtio/vhost-shadow-virtqueue.h > @@ -17,6 +17,8 @@ > > typedef struct VhostShadowVirtqueue VhostShadowVirtqueue; > > +EventNotifier *vhost_shadow_vq_get_call_notifier(VhostShadowVirtqueue *vq); > + > bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > unsigned idx, > VhostShadowVirtqueue *svq); > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > index 01f282d434..61d98ae652 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.c > +++ b/hw/virtio/vhost-shadow-virtqueue.c > @@ -24,6 +24,8 @@ typedef struct VhostShadowVirtqueue { > > /* Borrowed virtqueue's guest to host notifier. */ > EventNotifier host_notifier; > + /* Host to guest notifier */ > + EventNotifier *guest_notifier; > > /* Virtio queue shadowing */ > VirtQueue *vq; > @@ -40,6 +42,26 @@ static void vhost_handle_guest_kick(EventNotifier *n) > } > } > > +/* Forward vhost notifications */ > +static void vhost_handle_call(EventNotifier *n) > +{ > + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > + call_notifier); > + > + if (event_notifier_test_and_clear(n)) { > + event_notifier_set(svq->guest_notifier); > + } > +} So I wonder how this is synchonized with virtqueue mask/unmask. Or the masking is totally transparent to shadow virtqueue? Thanks > + > +/* > + * Get the vhost call notifier of the shadow vq > + * @vq Shadow virtqueue > + */ > +EventNotifier *vhost_shadow_vq_get_call_notifier(VhostShadowVirtqueue *vq) > +{ > + return &vq->call_notifier; > +} > + > /* > * Start shadow virtqueue operation. > * @dev vhost device > @@ -57,6 +79,10 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > .index = idx, > .fd = event_notifier_get_fd(&svq->kick_notifier), > }; > + struct vhost_vring_file call_file = { > + .index = idx, > + .fd = event_notifier_get_fd(&svq->call_notifier), > + }; > int r; > > /* Check that notifications are still going directly to vhost dev */ > @@ -66,6 +92,7 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > event_notifier_get_fd(vq_host_notifier)); > event_notifier_set_handler(&svq->host_notifier, vhost_handle_guest_kick); > > + svq->guest_notifier = virtio_queue_get_guest_notifier(svq->vq); > r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); > if (unlikely(r != 0)) { > error_report("Couldn't set kick fd: %s", strerror(errno)); > @@ -75,8 +102,19 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > /* Check for pending notifications from the guest */ > vhost_handle_guest_kick(&svq->host_notifier); > > + r = dev->vhost_ops->vhost_set_vring_call(dev, &call_file); > + if (r != 0) { > + error_report("Couldn't set call fd: %s", strerror(errno)); > + goto err_set_vring_call; > + } > + > return true; > > +err_set_vring_call: > + kick_file.fd = event_notifier_get_fd(vq_host_notifier); > + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); > + assert(r == 0); > + > err_set_vring_kick: > event_notifier_set_handler(&svq->host_notifier, NULL); > > @@ -108,6 +146,16 @@ void vhost_shadow_vq_stop_rcu(struct vhost_dev *dev, > assert(r == 0); > > event_notifier_set_handler(&svq->host_notifier, NULL); > + > + if (!dev->vqs[idx].notifier_is_masked) { > + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq); > + > + /* Restore vhost call */ > + vhost_virtqueue_mask(dev, dev->vdev, dev->vq_index + idx, false); > + > + /* Check for pending calls */ > + vhost_handle_call(e); > + } > } > > /* > @@ -136,6 +184,7 @@ VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx) > goto err_init_call_notifier; > } > > + event_notifier_set_handler(&svq->call_notifier, vhost_handle_call); > return g_steal_pointer(&svq); > > err_init_call_notifier: > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 9d4728e545..0dc95679e9 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -975,7 +975,6 @@ static int vhost_sw_live_migration_start(struct vhost_dev *dev) > for (idx = 0; idx < dev->nvqs; ++idx) { > bool ok = vhost_shadow_vq_start_rcu(dev, idx, > dev->shadow_vqs[idx]); > - > if (!ok) { > int stop_idx = idx; > > @@ -1608,6 +1607,10 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n, > if (mask) { > assert(vdev->use_guest_notifier_mask); > file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier); > + } else if (hdev->sw_lm_enabled) { > + VhostShadowVirtqueue *svq = hdev->shadow_vqs[n]; > + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq); > + file.fd = event_notifier_get_fd(e); > } else { > file.fd = event_notifier_get_fd(virtio_queue_get_guest_notifier(vvq)); > }
On Mon, Mar 1, 2021 at 7:24 AM Jason Wang <jasowang@redhat.com> wrote: > > > On 2021/2/9 11:37 下午, Eugenio Pérez wrote: > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-shadow-virtqueue.h | 2 ++ > > hw/virtio/vhost-shadow-virtqueue.c | 49 ++++++++++++++++++++++++++++++ > > hw/virtio/vhost.c | 5 ++- > > 3 files changed, 55 insertions(+), 1 deletion(-) > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h > > index c45035c4b7..210133434c 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.h > > +++ b/hw/virtio/vhost-shadow-virtqueue.h > > @@ -17,6 +17,8 @@ > > > > typedef struct VhostShadowVirtqueue VhostShadowVirtqueue; > > > > +EventNotifier *vhost_shadow_vq_get_call_notifier(VhostShadowVirtqueue *vq); > > + > > bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > > unsigned idx, > > VhostShadowVirtqueue *svq); > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > index 01f282d434..61d98ae652 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > @@ -24,6 +24,8 @@ typedef struct VhostShadowVirtqueue { > > > > /* Borrowed virtqueue's guest to host notifier. */ > > EventNotifier host_notifier; > > + /* Host to guest notifier */ > > + EventNotifier *guest_notifier; > > > > /* Virtio queue shadowing */ > > VirtQueue *vq; > > @@ -40,6 +42,26 @@ static void vhost_handle_guest_kick(EventNotifier *n) > > } > > } > > > > +/* Forward vhost notifications */ > > +static void vhost_handle_call(EventNotifier *n) > > +{ > > + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > > + call_notifier); > > + > > + if (event_notifier_test_and_clear(n)) { > > + event_notifier_set(svq->guest_notifier); > > + } > > +} > > > So I wonder how this is synchonized with virtqueue mask/unmask. Or the > masking is totally transparent to shadow virtqueue? > Hi Jason. Thanks for pointing it out. Actually, the design proposed in this series is wrong. In this series, when the guest masks a virtqueue, qemu still sends the masked notifier as call fd to the vhost device. While that ioctl acts as a synchronization point, it disables shadow vq polling and used buffers forwarding entirely. The guest could be actively polling the virtqueue used ring and waiting for used buffers before unmasking: This is valid as far as I can tell. At unmasking, either guest's notifier or shadow virtqueue call notifier is set, and masked is checked as usual. Again this is transparent for the dataplane (vhost_handle_call in this series). We could have a race in vhost_shadow_vq_stop_rcu, since mask/unmask status is set by vcpu thread and these are called by QMP here [1]. As Stefan pointed out, vhost_shadow_vq_start_rcu is bad because it unmasks unconditionally. In the next revision the masked status will be also tracked by the shadow virtqueue, but will only affect qemu->guest used notifications. > Thanks > > > > + > > +/* > > + * Get the vhost call notifier of the shadow vq > > + * @vq Shadow virtqueue > > + */ > > +EventNotifier *vhost_shadow_vq_get_call_notifier(VhostShadowVirtqueue *vq) > > +{ > > + return &vq->call_notifier; > > +} > > + > > /* > > * Start shadow virtqueue operation. > > * @dev vhost device > > @@ -57,6 +79,10 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > > .index = idx, > > .fd = event_notifier_get_fd(&svq->kick_notifier), > > }; > > + struct vhost_vring_file call_file = { > > + .index = idx, > > + .fd = event_notifier_get_fd(&svq->call_notifier), > > + }; > > int r; > > > > /* Check that notifications are still going directly to vhost dev */ > > @@ -66,6 +92,7 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > > event_notifier_get_fd(vq_host_notifier)); > > event_notifier_set_handler(&svq->host_notifier, vhost_handle_guest_kick); > > > > + svq->guest_notifier = virtio_queue_get_guest_notifier(svq->vq); > > r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); > > if (unlikely(r != 0)) { > > error_report("Couldn't set kick fd: %s", strerror(errno)); > > @@ -75,8 +102,19 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, > > /* Check for pending notifications from the guest */ > > vhost_handle_guest_kick(&svq->host_notifier); > > > > + r = dev->vhost_ops->vhost_set_vring_call(dev, &call_file); > > + if (r != 0) { > > + error_report("Couldn't set call fd: %s", strerror(errno)); > > + goto err_set_vring_call; > > + } > > + > > return true; > > > > +err_set_vring_call: > > + kick_file.fd = event_notifier_get_fd(vq_host_notifier); > > + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); > > + assert(r == 0); > > + > > err_set_vring_kick: > > event_notifier_set_handler(&svq->host_notifier, NULL); > > > > @@ -108,6 +146,16 @@ void vhost_shadow_vq_stop_rcu(struct vhost_dev *dev, > > assert(r == 0); > > > > event_notifier_set_handler(&svq->host_notifier, NULL); > > + > > + if (!dev->vqs[idx].notifier_is_masked) { > > + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq); > > + > > + /* Restore vhost call */ > > + vhost_virtqueue_mask(dev, dev->vdev, dev->vq_index + idx, false); > > + > > + /* Check for pending calls */ > > + vhost_handle_call(e); > > + } [1] We could have a race condition if vcpu mask/unmask just between reading it and calling vhost_virtqueue_mask: vhost_shadow_vq_stop_rcu would override whatever guest set. It will be fixed in the next revision. > > } > > > > /* > > @@ -136,6 +184,7 @@ VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx) > > goto err_init_call_notifier; > > } > > > > + event_notifier_set_handler(&svq->call_notifier, vhost_handle_call); > > return g_steal_pointer(&svq); > > > > err_init_call_notifier: > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index 9d4728e545..0dc95679e9 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -975,7 +975,6 @@ static int vhost_sw_live_migration_start(struct vhost_dev *dev) > > for (idx = 0; idx < dev->nvqs; ++idx) { > > bool ok = vhost_shadow_vq_start_rcu(dev, idx, > > dev->shadow_vqs[idx]); > > - > > if (!ok) { > > int stop_idx = idx; > > > > @@ -1608,6 +1607,10 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n, > > if (mask) { > > assert(vdev->use_guest_notifier_mask); > > file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier); > > + } else if (hdev->sw_lm_enabled) { > > + VhostShadowVirtqueue *svq = hdev->shadow_vqs[n]; > > + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq); > > + file.fd = event_notifier_get_fd(e); > > } else { > > file.fd = event_notifier_get_fd(virtio_queue_get_guest_notifier(vvq)); > > } >
diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h index c45035c4b7..210133434c 100644 --- a/hw/virtio/vhost-shadow-virtqueue.h +++ b/hw/virtio/vhost-shadow-virtqueue.h @@ -17,6 +17,8 @@ typedef struct VhostShadowVirtqueue VhostShadowVirtqueue; +EventNotifier *vhost_shadow_vq_get_call_notifier(VhostShadowVirtqueue *vq); + bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, unsigned idx, VhostShadowVirtqueue *svq); diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c index 01f282d434..61d98ae652 100644 --- a/hw/virtio/vhost-shadow-virtqueue.c +++ b/hw/virtio/vhost-shadow-virtqueue.c @@ -24,6 +24,8 @@ typedef struct VhostShadowVirtqueue { /* Borrowed virtqueue's guest to host notifier. */ EventNotifier host_notifier; + /* Host to guest notifier */ + EventNotifier *guest_notifier; /* Virtio queue shadowing */ VirtQueue *vq; @@ -40,6 +42,26 @@ static void vhost_handle_guest_kick(EventNotifier *n) } } +/* Forward vhost notifications */ +static void vhost_handle_call(EventNotifier *n) +{ + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, + call_notifier); + + if (event_notifier_test_and_clear(n)) { + event_notifier_set(svq->guest_notifier); + } +} + +/* + * Get the vhost call notifier of the shadow vq + * @vq Shadow virtqueue + */ +EventNotifier *vhost_shadow_vq_get_call_notifier(VhostShadowVirtqueue *vq) +{ + return &vq->call_notifier; +} + /* * Start shadow virtqueue operation. * @dev vhost device @@ -57,6 +79,10 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, .index = idx, .fd = event_notifier_get_fd(&svq->kick_notifier), }; + struct vhost_vring_file call_file = { + .index = idx, + .fd = event_notifier_get_fd(&svq->call_notifier), + }; int r; /* Check that notifications are still going directly to vhost dev */ @@ -66,6 +92,7 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, event_notifier_get_fd(vq_host_notifier)); event_notifier_set_handler(&svq->host_notifier, vhost_handle_guest_kick); + svq->guest_notifier = virtio_queue_get_guest_notifier(svq->vq); r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); if (unlikely(r != 0)) { error_report("Couldn't set kick fd: %s", strerror(errno)); @@ -75,8 +102,19 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev, /* Check for pending notifications from the guest */ vhost_handle_guest_kick(&svq->host_notifier); + r = dev->vhost_ops->vhost_set_vring_call(dev, &call_file); + if (r != 0) { + error_report("Couldn't set call fd: %s", strerror(errno)); + goto err_set_vring_call; + } + return true; +err_set_vring_call: + kick_file.fd = event_notifier_get_fd(vq_host_notifier); + r = dev->vhost_ops->vhost_set_vring_kick(dev, &kick_file); + assert(r == 0); + err_set_vring_kick: event_notifier_set_handler(&svq->host_notifier, NULL); @@ -108,6 +146,16 @@ void vhost_shadow_vq_stop_rcu(struct vhost_dev *dev, assert(r == 0); event_notifier_set_handler(&svq->host_notifier, NULL); + + if (!dev->vqs[idx].notifier_is_masked) { + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq); + + /* Restore vhost call */ + vhost_virtqueue_mask(dev, dev->vdev, dev->vq_index + idx, false); + + /* Check for pending calls */ + vhost_handle_call(e); + } } /* @@ -136,6 +184,7 @@ VhostShadowVirtqueue *vhost_shadow_vq_new(struct vhost_dev *dev, int idx) goto err_init_call_notifier; } + event_notifier_set_handler(&svq->call_notifier, vhost_handle_call); return g_steal_pointer(&svq); err_init_call_notifier: diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 9d4728e545..0dc95679e9 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -975,7 +975,6 @@ static int vhost_sw_live_migration_start(struct vhost_dev *dev) for (idx = 0; idx < dev->nvqs; ++idx) { bool ok = vhost_shadow_vq_start_rcu(dev, idx, dev->shadow_vqs[idx]); - if (!ok) { int stop_idx = idx; @@ -1608,6 +1607,10 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n, if (mask) { assert(vdev->use_guest_notifier_mask); file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier); + } else if (hdev->sw_lm_enabled) { + VhostShadowVirtqueue *svq = hdev->shadow_vqs[n]; + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq); + file.fd = event_notifier_get_fd(e); } else { file.fd = event_notifier_get_fd(virtio_queue_get_guest_notifier(vvq)); }
Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-shadow-virtqueue.h | 2 ++ hw/virtio/vhost-shadow-virtqueue.c | 49 ++++++++++++++++++++++++++++++ hw/virtio/vhost.c | 5 ++- 3 files changed, 55 insertions(+), 1 deletion(-)