| Message ID | 20211001070603.307037-12-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | vDPA shadow virtqueue | expand |
在 2021/10/1 下午3:05, Eugenio Pérez 写道: > This will make qemu aware of the device used buffers, allowing it to > write the guest memory with its contents if needed. > > Since the use of vhost_virtqueue_start can unmasks and discard call > events, vhost_virtqueue_start should be modified in one of these ways: > * Split in two: One of them uses all logic to start a queue with no > side effects for the guest, and another one tha actually assumes that > the guest has just started the device. Vdpa should use just the > former. > * Actually store and check if the guest notifier is masked, and do it > conditionally. > * Left as it is, and duplicate all the logic in vhost-vdpa. > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/vhost-shadow-virtqueue.c | 19 +++++++++++++++ > hw/virtio/vhost-vdpa.c | 38 +++++++++++++++++++++++++++++- > 2 files changed, 56 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > index 21dc99ab5d..3fe129cf63 100644 > --- a/hw/virtio/vhost-shadow-virtqueue.c > +++ b/hw/virtio/vhost-shadow-virtqueue.c > @@ -53,6 +53,22 @@ static void vhost_handle_guest_kick(EventNotifier *n) > event_notifier_set(&svq->kick_notifier); > } > > +/* Forward vhost notifications */ > +static void vhost_svq_handle_call_no_test(EventNotifier *n) > +{ > + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > + call_notifier); > + > + event_notifier_set(&svq->guest_call_notifier); > +} > + > +static void vhost_svq_handle_call(EventNotifier *n) > +{ > + if (likely(event_notifier_test_and_clear(n))) { > + vhost_svq_handle_call_no_test(n); > + } > +} > + > /* > * Obtain the SVQ call notifier, where vhost device notifies SVQ that there > * exists pending used buffers. > @@ -180,6 +196,8 @@ VhostShadowVirtqueue *vhost_svq_new(struct vhost_dev *dev, int idx) > } > > svq->vq = virtio_get_queue(dev->vdev, vq_idx); > + event_notifier_set_handler(&svq->call_notifier, > + vhost_svq_handle_call); > return g_steal_pointer(&svq); > > err_init_call_notifier: > @@ -195,6 +213,7 @@ err_init_kick_notifier: > void vhost_svq_free(VhostShadowVirtqueue *vq) > { > event_notifier_cleanup(&vq->kick_notifier); > + event_notifier_set_handler(&vq->call_notifier, NULL); > event_notifier_cleanup(&vq->call_notifier); > g_free(vq); > } > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index bc34de2439..6c5f4c98b8 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -712,13 +712,40 @@ static bool vhost_vdpa_svq_start_vq(struct vhost_dev *dev, unsigned idx) > { > struct vhost_vdpa *v = dev->opaque; > VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, idx); > - return vhost_svq_start(dev, idx, svq); > + EventNotifier *vhost_call_notifier = vhost_svq_get_svq_call_notifier(svq); > + struct vhost_vring_file vhost_call_file = { > + .index = idx + dev->vq_index, > + .fd = event_notifier_get_fd(vhost_call_notifier), > + }; > + int r; > + bool b; > + > + /* Set shadow vq -> guest notifier */ > + assert(v->call_fd[idx]); We need aovid the asser() here. On which case we can hit this? > + vhost_svq_set_guest_call_notifier(svq, v->call_fd[idx]); > + > + b = vhost_svq_start(dev, idx, svq); > + if (unlikely(!b)) { > + return false; > + } > + > + /* Set device -> SVQ notifier */ > + r = vhost_vdpa_set_vring_dev_call(dev, &vhost_call_file); > + if (unlikely(r)) { > + error_report("vhost_vdpa_set_vring_call for shadow vq failed"); > + return false; > + } Similar to kick, do we need to set_vring_call() before vhost_svq_start()? > + > + /* Check for pending calls */ > + event_notifier_set(vhost_call_notifier); Interesting, can this result spurious interrupt? > + return true; > } > > static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > { > struct vhost_dev *hdev = v->dev; > unsigned n; > + int r; > > if (enable == v->shadow_vqs_enabled) { > return hdev->nvqs; > @@ -752,9 +779,18 @@ static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > if (!enable) { > /* Disable all queues or clean up failed start */ > for (n = 0; n < v->shadow_vqs->len; ++n) { > + struct vhost_vring_file file = { > + .index = vhost_vdpa_get_vq_index(hdev, n), > + .fd = v->call_fd[n], > + }; > + > + r = vhost_vdpa_set_vring_call(hdev, &file); > + assert(r == 0); > + > unsigned vq_idx = vhost_vdpa_get_vq_index(hdev, n); > VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, n); > vhost_svq_stop(hdev, n, svq); > + /* TODO: This can unmask or override call fd! */ I don't get this comment. Does this mean the current code can't work with mask_notifiers? If yes, this is something we need to fix. Thanks > vhost_virtqueue_start(hdev, hdev->vdev, &hdev->vqs[n], vq_idx); > } >
在 2021/10/1 下午3:05, Eugenio Pérez 写道: > This will make qemu aware of the device used buffers, allowing it to > write the guest memory with its contents if needed. > > Since the use of vhost_virtqueue_start can unmasks and discard call > events, vhost_virtqueue_start should be modified in one of these ways: > * Split in two: One of them uses all logic to start a queue with no > side effects for the guest, and another one tha actually assumes that > the guest has just started the device. Vdpa should use just the > former. > * Actually store and check if the guest notifier is masked, and do it > conditionally. > * Left as it is, and duplicate all the logic in vhost-vdpa. Btw, the log looks not clear. I guess this patch goes for method 3. If yes, we need explain it and why. Thanks > > Signed-off-by: Eugenio Pérez<eperezma@redhat.com>
On Wed, Oct 13, 2021 at 5:49 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/10/1 下午3:05, Eugenio Pérez 写道: > > This will make qemu aware of the device used buffers, allowing it to > > write the guest memory with its contents if needed. > > > > Since the use of vhost_virtqueue_start can unmasks and discard call > > events, vhost_virtqueue_start should be modified in one of these ways: > > * Split in two: One of them uses all logic to start a queue with no > > side effects for the guest, and another one tha actually assumes that > > the guest has just started the device. Vdpa should use just the > > former. > > * Actually store and check if the guest notifier is masked, and do it > > conditionally. > > * Left as it is, and duplicate all the logic in vhost-vdpa. > > > Btw, the log looks not clear. I guess this patch goes for method 3. If > yes, we need explain it and why. > > Thanks > Sorry about being unclear. This commit log (and code) just exposes the problem and the solutions I came up with but does nothing to solve it. I'm actually going for method 3 for the next series but I'm open to doing it differently. > > > > > Signed-off-by: Eugenio Pérez<eperezma@redhat.com> >
On Wed, Oct 13, 2021 at 5:47 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/10/1 下午3:05, Eugenio Pérez 写道: > > This will make qemu aware of the device used buffers, allowing it to > > write the guest memory with its contents if needed. > > > > Since the use of vhost_virtqueue_start can unmasks and discard call > > events, vhost_virtqueue_start should be modified in one of these ways: > > * Split in two: One of them uses all logic to start a queue with no > > side effects for the guest, and another one tha actually assumes that > > the guest has just started the device. Vdpa should use just the > > former. > > * Actually store and check if the guest notifier is masked, and do it > > conditionally. > > * Left as it is, and duplicate all the logic in vhost-vdpa. > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/vhost-shadow-virtqueue.c | 19 +++++++++++++++ > > hw/virtio/vhost-vdpa.c | 38 +++++++++++++++++++++++++++++- > > 2 files changed, 56 insertions(+), 1 deletion(-) > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > index 21dc99ab5d..3fe129cf63 100644 > > --- a/hw/virtio/vhost-shadow-virtqueue.c > > +++ b/hw/virtio/vhost-shadow-virtqueue.c > > @@ -53,6 +53,22 @@ static void vhost_handle_guest_kick(EventNotifier *n) > > event_notifier_set(&svq->kick_notifier); > > } > > > > +/* Forward vhost notifications */ > > +static void vhost_svq_handle_call_no_test(EventNotifier *n) > > +{ > > + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > > + call_notifier); > > + > > + event_notifier_set(&svq->guest_call_notifier); > > +} > > + > > +static void vhost_svq_handle_call(EventNotifier *n) > > +{ > > + if (likely(event_notifier_test_and_clear(n))) { > > + vhost_svq_handle_call_no_test(n); > > + } > > +} > > + > > /* > > * Obtain the SVQ call notifier, where vhost device notifies SVQ that there > > * exists pending used buffers. > > @@ -180,6 +196,8 @@ VhostShadowVirtqueue *vhost_svq_new(struct vhost_dev *dev, int idx) > > } > > > > svq->vq = virtio_get_queue(dev->vdev, vq_idx); > > + event_notifier_set_handler(&svq->call_notifier, > > + vhost_svq_handle_call); > > return g_steal_pointer(&svq); > > > > err_init_call_notifier: > > @@ -195,6 +213,7 @@ err_init_kick_notifier: > > void vhost_svq_free(VhostShadowVirtqueue *vq) > > { > > event_notifier_cleanup(&vq->kick_notifier); > > + event_notifier_set_handler(&vq->call_notifier, NULL); > > event_notifier_cleanup(&vq->call_notifier); > > g_free(vq); > > } > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > index bc34de2439..6c5f4c98b8 100644 > > --- a/hw/virtio/vhost-vdpa.c > > +++ b/hw/virtio/vhost-vdpa.c > > @@ -712,13 +712,40 @@ static bool vhost_vdpa_svq_start_vq(struct vhost_dev *dev, unsigned idx) > > { > > struct vhost_vdpa *v = dev->opaque; > > VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, idx); > > - return vhost_svq_start(dev, idx, svq); > > + EventNotifier *vhost_call_notifier = vhost_svq_get_svq_call_notifier(svq); > > + struct vhost_vring_file vhost_call_file = { > > + .index = idx + dev->vq_index, > > + .fd = event_notifier_get_fd(vhost_call_notifier), > > + }; > > + int r; > > + bool b; > > + > > + /* Set shadow vq -> guest notifier */ > > + assert(v->call_fd[idx]); > > > We need aovid the asser() here. On which case we can hit this? > I would say that there is no way we can actually hit it, so let's remove it. > > > + vhost_svq_set_guest_call_notifier(svq, v->call_fd[idx]); > > + > > + b = vhost_svq_start(dev, idx, svq); > > + if (unlikely(!b)) { > > + return false; > > + } > > + > > + /* Set device -> SVQ notifier */ > > + r = vhost_vdpa_set_vring_dev_call(dev, &vhost_call_file); > > + if (unlikely(r)) { > > + error_report("vhost_vdpa_set_vring_call for shadow vq failed"); > > + return false; > > + } > > > Similar to kick, do we need to set_vring_call() before vhost_svq_start()? > It should not matter at this moment because the device should not be started at this point and device calls should not run vhost_svq_handle_call until BQL is released. The "logic" of doing it after is to make clear that svq must be fully initialized before processing device calls, even in the case that we extract SVQ in its own iothread or similar. But this could be done before vhost_svq_start for sure. > > > + > > + /* Check for pending calls */ > > + event_notifier_set(vhost_call_notifier); > > > Interesting, can this result spurious interrupt? > This actually "queues" a vhost_svq_handle_call after the BQL release, where the device should be fully reset. In that regard, if there are no used descriptors there will not be an irq raised to the guest. Does that answer the question? Or have I missed something? > > > + return true; > > } > > > > static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > > { > > struct vhost_dev *hdev = v->dev; > > unsigned n; > > + int r; > > > > if (enable == v->shadow_vqs_enabled) { > > return hdev->nvqs; > > @@ -752,9 +779,18 @@ static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > > if (!enable) { > > /* Disable all queues or clean up failed start */ > > for (n = 0; n < v->shadow_vqs->len; ++n) { > > + struct vhost_vring_file file = { > > + .index = vhost_vdpa_get_vq_index(hdev, n), > > + .fd = v->call_fd[n], > > + }; > > + > > + r = vhost_vdpa_set_vring_call(hdev, &file); > > + assert(r == 0); > > + > > unsigned vq_idx = vhost_vdpa_get_vq_index(hdev, n); > > VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, n); > > vhost_svq_stop(hdev, n, svq); > > + /* TODO: This can unmask or override call fd! */ > > > I don't get this comment. Does this mean the current code can't work > with mask_notifiers? If yes, this is something we need to fix. > Yes, but it will be addressed in the next series. I should have explained it bette here, sorry :). Thanks! > Thanks > > > > vhost_virtqueue_start(hdev, hdev->vdev, &hdev->vqs[n], vq_idx); > > } > > >
在 2021/10/14 下午11:58, Eugenio Perez Martin 写道: > On Wed, Oct 13, 2021 at 5:49 AM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2021/10/1 下午3:05, Eugenio Pérez 写道: >>> This will make qemu aware of the device used buffers, allowing it to >>> write the guest memory with its contents if needed. >>> >>> Since the use of vhost_virtqueue_start can unmasks and discard call >>> events, vhost_virtqueue_start should be modified in one of these ways: >>> * Split in two: One of them uses all logic to start a queue with no >>> side effects for the guest, and another one tha actually assumes that >>> the guest has just started the device. Vdpa should use just the >>> former. >>> * Actually store and check if the guest notifier is masked, and do it >>> conditionally. >>> * Left as it is, and duplicate all the logic in vhost-vdpa. >> >> Btw, the log looks not clear. I guess this patch goes for method 3. If >> yes, we need explain it and why. >> >> Thanks >> > Sorry about being unclear. This commit log (and code) just exposes the > problem and the solutions I came up with but does nothing to solve it. > I'm actually going for method 3 for the next series but I'm open to > doing it differently. That's fine, but need to doc that method 3 is something that is done in the patch. Thanks > >>> Signed-off-by: Eugenio Pérez<eperezma@redhat.com>
在 2021/10/15 上午12:39, Eugenio Perez Martin 写道: > On Wed, Oct 13, 2021 at 5:47 AM Jason Wang <jasowang@redhat.com> wrote: >> >> 在 2021/10/1 下午3:05, Eugenio Pérez 写道: >>> This will make qemu aware of the device used buffers, allowing it to >>> write the guest memory with its contents if needed. >>> >>> Since the use of vhost_virtqueue_start can unmasks and discard call >>> events, vhost_virtqueue_start should be modified in one of these ways: >>> * Split in two: One of them uses all logic to start a queue with no >>> side effects for the guest, and another one tha actually assumes that >>> the guest has just started the device. Vdpa should use just the >>> former. >>> * Actually store and check if the guest notifier is masked, and do it >>> conditionally. >>> * Left as it is, and duplicate all the logic in vhost-vdpa. >>> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> >>> --- >>> hw/virtio/vhost-shadow-virtqueue.c | 19 +++++++++++++++ >>> hw/virtio/vhost-vdpa.c | 38 +++++++++++++++++++++++++++++- >>> 2 files changed, 56 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c >>> index 21dc99ab5d..3fe129cf63 100644 >>> --- a/hw/virtio/vhost-shadow-virtqueue.c >>> +++ b/hw/virtio/vhost-shadow-virtqueue.c >>> @@ -53,6 +53,22 @@ static void vhost_handle_guest_kick(EventNotifier *n) >>> event_notifier_set(&svq->kick_notifier); >>> } >>> >>> +/* Forward vhost notifications */ >>> +static void vhost_svq_handle_call_no_test(EventNotifier *n) >>> +{ >>> + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, >>> + call_notifier); >>> + >>> + event_notifier_set(&svq->guest_call_notifier); >>> +} >>> + >>> +static void vhost_svq_handle_call(EventNotifier *n) >>> +{ >>> + if (likely(event_notifier_test_and_clear(n))) { >>> + vhost_svq_handle_call_no_test(n); >>> + } >>> +} >>> + >>> /* >>> * Obtain the SVQ call notifier, where vhost device notifies SVQ that there >>> * exists pending used buffers. >>> @@ -180,6 +196,8 @@ VhostShadowVirtqueue *vhost_svq_new(struct vhost_dev *dev, int idx) >>> } >>> >>> svq->vq = virtio_get_queue(dev->vdev, vq_idx); >>> + event_notifier_set_handler(&svq->call_notifier, >>> + vhost_svq_handle_call); >>> return g_steal_pointer(&svq); >>> >>> err_init_call_notifier: >>> @@ -195,6 +213,7 @@ err_init_kick_notifier: >>> void vhost_svq_free(VhostShadowVirtqueue *vq) >>> { >>> event_notifier_cleanup(&vq->kick_notifier); >>> + event_notifier_set_handler(&vq->call_notifier, NULL); >>> event_notifier_cleanup(&vq->call_notifier); >>> g_free(vq); >>> } >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >>> index bc34de2439..6c5f4c98b8 100644 >>> --- a/hw/virtio/vhost-vdpa.c >>> +++ b/hw/virtio/vhost-vdpa.c >>> @@ -712,13 +712,40 @@ static bool vhost_vdpa_svq_start_vq(struct vhost_dev *dev, unsigned idx) >>> { >>> struct vhost_vdpa *v = dev->opaque; >>> VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, idx); >>> - return vhost_svq_start(dev, idx, svq); >>> + EventNotifier *vhost_call_notifier = vhost_svq_get_svq_call_notifier(svq); >>> + struct vhost_vring_file vhost_call_file = { >>> + .index = idx + dev->vq_index, >>> + .fd = event_notifier_get_fd(vhost_call_notifier), >>> + }; >>> + int r; >>> + bool b; >>> + >>> + /* Set shadow vq -> guest notifier */ >>> + assert(v->call_fd[idx]); >> >> We need aovid the asser() here. On which case we can hit this? >> > I would say that there is no way we can actually hit it, so let's remove it. > >>> + vhost_svq_set_guest_call_notifier(svq, v->call_fd[idx]); >>> + >>> + b = vhost_svq_start(dev, idx, svq); >>> + if (unlikely(!b)) { >>> + return false; >>> + } >>> + >>> + /* Set device -> SVQ notifier */ >>> + r = vhost_vdpa_set_vring_dev_call(dev, &vhost_call_file); >>> + if (unlikely(r)) { >>> + error_report("vhost_vdpa_set_vring_call for shadow vq failed"); >>> + return false; >>> + } >> >> Similar to kick, do we need to set_vring_call() before vhost_svq_start()? >> > It should not matter at this moment because the device should not be > started at this point and device calls should not run > vhost_svq_handle_call until BQL is released. Yes, we stop virtqueue before. > > The "logic" of doing it after is to make clear that svq must be fully > initialized before processing device calls, even in the case that we > extract SVQ in its own iothread or similar. But this could be done > before vhost_svq_start for sure. > >>> + >>> + /* Check for pending calls */ >>> + event_notifier_set(vhost_call_notifier); >> >> Interesting, can this result spurious interrupt? >> > This actually "queues" a vhost_svq_handle_call after the BQL release, > where the device should be fully reset. In that regard, if there are > no used descriptors there will not be an irq raised to the guest. Does > that answer the question? Or have I missed something? Yes, please explain this in the comment. > >>> + return true; >>> } >>> >>> static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) >>> { >>> struct vhost_dev *hdev = v->dev; >>> unsigned n; >>> + int r; >>> >>> if (enable == v->shadow_vqs_enabled) { >>> return hdev->nvqs; >>> @@ -752,9 +779,18 @@ static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) >>> if (!enable) { >>> /* Disable all queues or clean up failed start */ >>> for (n = 0; n < v->shadow_vqs->len; ++n) { >>> + struct vhost_vring_file file = { >>> + .index = vhost_vdpa_get_vq_index(hdev, n), >>> + .fd = v->call_fd[n], >>> + }; >>> + >>> + r = vhost_vdpa_set_vring_call(hdev, &file); >>> + assert(r == 0); >>> + >>> unsigned vq_idx = vhost_vdpa_get_vq_index(hdev, n); >>> VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, n); >>> vhost_svq_stop(hdev, n, svq); >>> + /* TODO: This can unmask or override call fd! */ >> >> I don't get this comment. Does this mean the current code can't work >> with mask_notifiers? If yes, this is something we need to fix. >> > Yes, but it will be addressed in the next series. I should have > explained it bette here, sorry :). Ok. Thanks > > Thanks! > >> Thanks >> >> >>> vhost_virtqueue_start(hdev, hdev->vdev, &hdev->vqs[n], vq_idx); >>> } >>>
On Fri, Oct 15, 2021 at 6:42 AM Jason Wang <jasowang@redhat.com> wrote: > > > 在 2021/10/15 上午12:39, Eugenio Perez Martin 写道: > > On Wed, Oct 13, 2021 at 5:47 AM Jason Wang <jasowang@redhat.com> wrote: > >> > >> 在 2021/10/1 下午3:05, Eugenio Pérez 写道: > >>> This will make qemu aware of the device used buffers, allowing it to > >>> write the guest memory with its contents if needed. > >>> > >>> Since the use of vhost_virtqueue_start can unmasks and discard call > >>> events, vhost_virtqueue_start should be modified in one of these ways: > >>> * Split in two: One of them uses all logic to start a queue with no > >>> side effects for the guest, and another one tha actually assumes that > >>> the guest has just started the device. Vdpa should use just the > >>> former. > >>> * Actually store and check if the guest notifier is masked, and do it > >>> conditionally. > >>> * Left as it is, and duplicate all the logic in vhost-vdpa. > >>> > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > >>> --- > >>> hw/virtio/vhost-shadow-virtqueue.c | 19 +++++++++++++++ > >>> hw/virtio/vhost-vdpa.c | 38 +++++++++++++++++++++++++++++- > >>> 2 files changed, 56 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > >>> index 21dc99ab5d..3fe129cf63 100644 > >>> --- a/hw/virtio/vhost-shadow-virtqueue.c > >>> +++ b/hw/virtio/vhost-shadow-virtqueue.c > >>> @@ -53,6 +53,22 @@ static void vhost_handle_guest_kick(EventNotifier *n) > >>> event_notifier_set(&svq->kick_notifier); > >>> } > >>> > >>> +/* Forward vhost notifications */ > >>> +static void vhost_svq_handle_call_no_test(EventNotifier *n) > >>> +{ > >>> + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > >>> + call_notifier); > >>> + > >>> + event_notifier_set(&svq->guest_call_notifier); > >>> +} > >>> + > >>> +static void vhost_svq_handle_call(EventNotifier *n) > >>> +{ > >>> + if (likely(event_notifier_test_and_clear(n))) { > >>> + vhost_svq_handle_call_no_test(n); > >>> + } > >>> +} > >>> + > >>> /* > >>> * Obtain the SVQ call notifier, where vhost device notifies SVQ that there > >>> * exists pending used buffers. > >>> @@ -180,6 +196,8 @@ VhostShadowVirtqueue *vhost_svq_new(struct vhost_dev *dev, int idx) > >>> } > >>> > >>> svq->vq = virtio_get_queue(dev->vdev, vq_idx); > >>> + event_notifier_set_handler(&svq->call_notifier, > >>> + vhost_svq_handle_call); > >>> return g_steal_pointer(&svq); > >>> > >>> err_init_call_notifier: > >>> @@ -195,6 +213,7 @@ err_init_kick_notifier: > >>> void vhost_svq_free(VhostShadowVirtqueue *vq) > >>> { > >>> event_notifier_cleanup(&vq->kick_notifier); > >>> + event_notifier_set_handler(&vq->call_notifier, NULL); > >>> event_notifier_cleanup(&vq->call_notifier); > >>> g_free(vq); > >>> } > >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > >>> index bc34de2439..6c5f4c98b8 100644 > >>> --- a/hw/virtio/vhost-vdpa.c > >>> +++ b/hw/virtio/vhost-vdpa.c > >>> @@ -712,13 +712,40 @@ static bool vhost_vdpa_svq_start_vq(struct vhost_dev *dev, unsigned idx) > >>> { > >>> struct vhost_vdpa *v = dev->opaque; > >>> VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, idx); > >>> - return vhost_svq_start(dev, idx, svq); > >>> + EventNotifier *vhost_call_notifier = vhost_svq_get_svq_call_notifier(svq); > >>> + struct vhost_vring_file vhost_call_file = { > >>> + .index = idx + dev->vq_index, > >>> + .fd = event_notifier_get_fd(vhost_call_notifier), > >>> + }; > >>> + int r; > >>> + bool b; > >>> + > >>> + /* Set shadow vq -> guest notifier */ > >>> + assert(v->call_fd[idx]); > >> > >> We need aovid the asser() here. On which case we can hit this? > >> > > I would say that there is no way we can actually hit it, so let's remove it. > > > >>> + vhost_svq_set_guest_call_notifier(svq, v->call_fd[idx]); > >>> + > >>> + b = vhost_svq_start(dev, idx, svq); > >>> + if (unlikely(!b)) { > >>> + return false; > >>> + } > >>> + > >>> + /* Set device -> SVQ notifier */ > >>> + r = vhost_vdpa_set_vring_dev_call(dev, &vhost_call_file); > >>> + if (unlikely(r)) { > >>> + error_report("vhost_vdpa_set_vring_call for shadow vq failed"); > >>> + return false; > >>> + } > >> > >> Similar to kick, do we need to set_vring_call() before vhost_svq_start()? > >> > > It should not matter at this moment because the device should not be > > started at this point and device calls should not run > > vhost_svq_handle_call until BQL is released. > > > Yes, we stop virtqueue before. > > > > > > The "logic" of doing it after is to make clear that svq must be fully > > initialized before processing device calls, even in the case that we > > extract SVQ in its own iothread or similar. But this could be done > > before vhost_svq_start for sure. > > > >>> + > >>> + /* Check for pending calls */ > >>> + event_notifier_set(vhost_call_notifier); > >> > >> Interesting, can this result spurious interrupt? > >> > > This actually "queues" a vhost_svq_handle_call after the BQL release, > > where the device should be fully reset. In that regard, if there are > > no used descriptors there will not be an irq raised to the guest. Does > > that answer the question? Or have I missed something? > > > Yes, please explain this in the comment. > I'm reviewing this again, and actually I think I was wrong in solving the issue. Since at this point the device is being configured, there is no chance that we had a missing call notification here: A previous kick is needed for the device to generate any calls, and these cannot be processed. What is not solved in this series is that we could have pending used buffers in vdpa device stopping SVQ, but queuing a check for that is not going to solve anything, since SVQ vring would be already destroyed: * vdpa device marks N > 0 buffers as used, and calls. * Before processing them, SVQ stop is called. SVQ have not processed these, and cleans them, making this event_notifier_set useless. So this would require a few changes. Mainly, instead of queueing a check for used, these need to be checked before svq cleaning. After that, obtain the VQ state (is not obtained in the stop at the moment, trusting in guest's used idx) and run a last vhost_svq_handle_call_no_test while the device is paused. Thanks! > > > > >>> + return true; > >>> } > >>> > >>> static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > >>> { > >>> struct vhost_dev *hdev = v->dev; > >>> unsigned n; > >>> + int r; > >>> > >>> if (enable == v->shadow_vqs_enabled) { > >>> return hdev->nvqs; > >>> @@ -752,9 +779,18 @@ static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > >>> if (!enable) { > >>> /* Disable all queues or clean up failed start */ > >>> for (n = 0; n < v->shadow_vqs->len; ++n) { > >>> + struct vhost_vring_file file = { > >>> + .index = vhost_vdpa_get_vq_index(hdev, n), > >>> + .fd = v->call_fd[n], > >>> + }; > >>> + > >>> + r = vhost_vdpa_set_vring_call(hdev, &file); > >>> + assert(r == 0); > >>> + > >>> unsigned vq_idx = vhost_vdpa_get_vq_index(hdev, n); > >>> VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, n); > >>> vhost_svq_stop(hdev, n, svq); > >>> + /* TODO: This can unmask or override call fd! */ > >> > >> I don't get this comment. Does this mean the current code can't work > >> with mask_notifiers? If yes, this is something we need to fix. > >> > > Yes, but it will be addressed in the next series. I should have > > explained it bette here, sorry :). > > > Ok. > > Thanks > > > > > > Thanks! > > > >> Thanks > >> > >> > >>> vhost_virtqueue_start(hdev, hdev->vdev, &hdev->vqs[n], vq_idx); > >>> } > >>> >
On Tue, Oct 19, 2021 at 4:40 PM Eugenio Perez Martin <eperezma@redhat.com> wrote: > > On Fri, Oct 15, 2021 at 6:42 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > 在 2021/10/15 上午12:39, Eugenio Perez Martin 写道: > > > On Wed, Oct 13, 2021 at 5:47 AM Jason Wang <jasowang@redhat.com> wrote: > > >> > > >> 在 2021/10/1 下午3:05, Eugenio Pérez 写道: > > >>> This will make qemu aware of the device used buffers, allowing it to > > >>> write the guest memory with its contents if needed. > > >>> > > >>> Since the use of vhost_virtqueue_start can unmasks and discard call > > >>> events, vhost_virtqueue_start should be modified in one of these ways: > > >>> * Split in two: One of them uses all logic to start a queue with no > > >>> side effects for the guest, and another one tha actually assumes that > > >>> the guest has just started the device. Vdpa should use just the > > >>> former. > > >>> * Actually store and check if the guest notifier is masked, and do it > > >>> conditionally. > > >>> * Left as it is, and duplicate all the logic in vhost-vdpa. > > >>> > > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > >>> --- > > >>> hw/virtio/vhost-shadow-virtqueue.c | 19 +++++++++++++++ > > >>> hw/virtio/vhost-vdpa.c | 38 +++++++++++++++++++++++++++++- > > >>> 2 files changed, 56 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > >>> index 21dc99ab5d..3fe129cf63 100644 > > >>> --- a/hw/virtio/vhost-shadow-virtqueue.c > > >>> +++ b/hw/virtio/vhost-shadow-virtqueue.c > > >>> @@ -53,6 +53,22 @@ static void vhost_handle_guest_kick(EventNotifier *n) > > >>> event_notifier_set(&svq->kick_notifier); > > >>> } > > >>> > > >>> +/* Forward vhost notifications */ > > >>> +static void vhost_svq_handle_call_no_test(EventNotifier *n) > > >>> +{ > > >>> + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > > >>> + call_notifier); > > >>> + > > >>> + event_notifier_set(&svq->guest_call_notifier); > > >>> +} > > >>> + > > >>> +static void vhost_svq_handle_call(EventNotifier *n) > > >>> +{ > > >>> + if (likely(event_notifier_test_and_clear(n))) { > > >>> + vhost_svq_handle_call_no_test(n); > > >>> + } > > >>> +} > > >>> + > > >>> /* > > >>> * Obtain the SVQ call notifier, where vhost device notifies SVQ that there > > >>> * exists pending used buffers. > > >>> @@ -180,6 +196,8 @@ VhostShadowVirtqueue *vhost_svq_new(struct vhost_dev *dev, int idx) > > >>> } > > >>> > > >>> svq->vq = virtio_get_queue(dev->vdev, vq_idx); > > >>> + event_notifier_set_handler(&svq->call_notifier, > > >>> + vhost_svq_handle_call); > > >>> return g_steal_pointer(&svq); > > >>> > > >>> err_init_call_notifier: > > >>> @@ -195,6 +213,7 @@ err_init_kick_notifier: > > >>> void vhost_svq_free(VhostShadowVirtqueue *vq) > > >>> { > > >>> event_notifier_cleanup(&vq->kick_notifier); > > >>> + event_notifier_set_handler(&vq->call_notifier, NULL); > > >>> event_notifier_cleanup(&vq->call_notifier); > > >>> g_free(vq); > > >>> } > > >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > >>> index bc34de2439..6c5f4c98b8 100644 > > >>> --- a/hw/virtio/vhost-vdpa.c > > >>> +++ b/hw/virtio/vhost-vdpa.c > > >>> @@ -712,13 +712,40 @@ static bool vhost_vdpa_svq_start_vq(struct vhost_dev *dev, unsigned idx) > > >>> { > > >>> struct vhost_vdpa *v = dev->opaque; > > >>> VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, idx); > > >>> - return vhost_svq_start(dev, idx, svq); > > >>> + EventNotifier *vhost_call_notifier = vhost_svq_get_svq_call_notifier(svq); > > >>> + struct vhost_vring_file vhost_call_file = { > > >>> + .index = idx + dev->vq_index, > > >>> + .fd = event_notifier_get_fd(vhost_call_notifier), > > >>> + }; > > >>> + int r; > > >>> + bool b; > > >>> + > > >>> + /* Set shadow vq -> guest notifier */ > > >>> + assert(v->call_fd[idx]); > > >> > > >> We need aovid the asser() here. On which case we can hit this? > > >> > > > I would say that there is no way we can actually hit it, so let's remove it. > > > > > >>> + vhost_svq_set_guest_call_notifier(svq, v->call_fd[idx]); > > >>> + > > >>> + b = vhost_svq_start(dev, idx, svq); > > >>> + if (unlikely(!b)) { > > >>> + return false; > > >>> + } > > >>> + > > >>> + /* Set device -> SVQ notifier */ > > >>> + r = vhost_vdpa_set_vring_dev_call(dev, &vhost_call_file); > > >>> + if (unlikely(r)) { > > >>> + error_report("vhost_vdpa_set_vring_call for shadow vq failed"); > > >>> + return false; > > >>> + } > > >> > > >> Similar to kick, do we need to set_vring_call() before vhost_svq_start()? > > >> > > > It should not matter at this moment because the device should not be > > > started at this point and device calls should not run > > > vhost_svq_handle_call until BQL is released. > > > > > > Yes, we stop virtqueue before. > > > > > > > > > > The "logic" of doing it after is to make clear that svq must be fully > > > initialized before processing device calls, even in the case that we > > > extract SVQ in its own iothread or similar. But this could be done > > > before vhost_svq_start for sure. > > > > > >>> + > > >>> + /* Check for pending calls */ > > >>> + event_notifier_set(vhost_call_notifier); > > >> > > >> Interesting, can this result spurious interrupt? > > >> > > > This actually "queues" a vhost_svq_handle_call after the BQL release, > > > where the device should be fully reset. In that regard, if there are > > > no used descriptors there will not be an irq raised to the guest. Does > > > that answer the question? Or have I missed something? > > > > > > Yes, please explain this in the comment. > > > > I'm reviewing this again, and actually I think I was wrong in solving the issue. > > Since at this point the device is being configured, there is no chance > that we had a missing call notification here: A previous kick is > needed for the device to generate any calls, and these cannot be > processed. > > What is not solved in this series is that we could have pending used > buffers in vdpa device stopping SVQ, but queuing a check for that is > not going to solve anything, since SVQ vring would be already > destroyed: > > * vdpa device marks N > 0 buffers as used, and calls. > * Before processing them, SVQ stop is called. SVQ have not processed > these, and cleans them, making this event_notifier_set useless. > > So this would require a few changes. Mainly, instead of queueing a > check for used, these need to be checked before svq cleaning. After > that, obtain the VQ state (is not obtained in the stop at the moment, > trusting in guest's used idx) and run a last > vhost_svq_handle_call_no_test while the device is paused. It looks to me what's really important is that SVQ needs to drain/forwared used buffers after vdpa is stopped. Then we should be fine. > > Thanks! > > > > > > > > >>> + return true; > > >>> } > > >>> > > >>> static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > > >>> { > > >>> struct vhost_dev *hdev = v->dev; > > >>> unsigned n; > > >>> + int r; > > >>> > > >>> if (enable == v->shadow_vqs_enabled) { > > >>> return hdev->nvqs; > > >>> @@ -752,9 +779,18 @@ static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > > >>> if (!enable) { > > >>> /* Disable all queues or clean up failed start */ > > >>> for (n = 0; n < v->shadow_vqs->len; ++n) { > > >>> + struct vhost_vring_file file = { > > >>> + .index = vhost_vdpa_get_vq_index(hdev, n), > > >>> + .fd = v->call_fd[n], > > >>> + }; > > >>> + > > >>> + r = vhost_vdpa_set_vring_call(hdev, &file); > > >>> + assert(r == 0); > > >>> + > > >>> unsigned vq_idx = vhost_vdpa_get_vq_index(hdev, n); > > >>> VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, n); > > >>> vhost_svq_stop(hdev, n, svq); > > >>> + /* TODO: This can unmask or override call fd! */ > > >> > > >> I don't get this comment. Does this mean the current code can't work > > >> with mask_notifiers? If yes, this is something we need to fix. > > >> > > > Yes, but it will be addressed in the next series. I should have > > > explained it bette here, sorry :). > > > > > > Ok. > > > > Thanks > > > > > > > > > > Thanks! > > > > > >> Thanks > > >> > > >> > > >>> vhost_virtqueue_start(hdev, hdev->vdev, &hdev->vqs[n], vq_idx); > > >>> } > > >>> > > >
On Wed, Oct 20, 2021 at 4:01 AM Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Oct 19, 2021 at 4:40 PM Eugenio Perez Martin > <eperezma@redhat.com> wrote: > > > > On Fri, Oct 15, 2021 at 6:42 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > 在 2021/10/15 上午12:39, Eugenio Perez Martin 写道: > > > > On Wed, Oct 13, 2021 at 5:47 AM Jason Wang <jasowang@redhat.com> wrote: > > > >> > > > >> 在 2021/10/1 下午3:05, Eugenio Pérez 写道: > > > >>> This will make qemu aware of the device used buffers, allowing it to > > > >>> write the guest memory with its contents if needed. > > > >>> > > > >>> Since the use of vhost_virtqueue_start can unmasks and discard call > > > >>> events, vhost_virtqueue_start should be modified in one of these ways: > > > >>> * Split in two: One of them uses all logic to start a queue with no > > > >>> side effects for the guest, and another one tha actually assumes that > > > >>> the guest has just started the device. Vdpa should use just the > > > >>> former. > > > >>> * Actually store and check if the guest notifier is masked, and do it > > > >>> conditionally. > > > >>> * Left as it is, and duplicate all the logic in vhost-vdpa. > > > >>> > > > >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > > >>> --- > > > >>> hw/virtio/vhost-shadow-virtqueue.c | 19 +++++++++++++++ > > > >>> hw/virtio/vhost-vdpa.c | 38 +++++++++++++++++++++++++++++- > > > >>> 2 files changed, 56 insertions(+), 1 deletion(-) > > > >>> > > > >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > > >>> index 21dc99ab5d..3fe129cf63 100644 > > > >>> --- a/hw/virtio/vhost-shadow-virtqueue.c > > > >>> +++ b/hw/virtio/vhost-shadow-virtqueue.c > > > >>> @@ -53,6 +53,22 @@ static void vhost_handle_guest_kick(EventNotifier *n) > > > >>> event_notifier_set(&svq->kick_notifier); > > > >>> } > > > >>> > > > >>> +/* Forward vhost notifications */ > > > >>> +static void vhost_svq_handle_call_no_test(EventNotifier *n) > > > >>> +{ > > > >>> + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > > > >>> + call_notifier); > > > >>> + > > > >>> + event_notifier_set(&svq->guest_call_notifier); > > > >>> +} > > > >>> + > > > >>> +static void vhost_svq_handle_call(EventNotifier *n) > > > >>> +{ > > > >>> + if (likely(event_notifier_test_and_clear(n))) { > > > >>> + vhost_svq_handle_call_no_test(n); > > > >>> + } > > > >>> +} > > > >>> + > > > >>> /* > > > >>> * Obtain the SVQ call notifier, where vhost device notifies SVQ that there > > > >>> * exists pending used buffers. > > > >>> @@ -180,6 +196,8 @@ VhostShadowVirtqueue *vhost_svq_new(struct vhost_dev *dev, int idx) > > > >>> } > > > >>> > > > >>> svq->vq = virtio_get_queue(dev->vdev, vq_idx); > > > >>> + event_notifier_set_handler(&svq->call_notifier, > > > >>> + vhost_svq_handle_call); > > > >>> return g_steal_pointer(&svq); > > > >>> > > > >>> err_init_call_notifier: > > > >>> @@ -195,6 +213,7 @@ err_init_kick_notifier: > > > >>> void vhost_svq_free(VhostShadowVirtqueue *vq) > > > >>> { > > > >>> event_notifier_cleanup(&vq->kick_notifier); > > > >>> + event_notifier_set_handler(&vq->call_notifier, NULL); > > > >>> event_notifier_cleanup(&vq->call_notifier); > > > >>> g_free(vq); > > > >>> } > > > >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > > > >>> index bc34de2439..6c5f4c98b8 100644 > > > >>> --- a/hw/virtio/vhost-vdpa.c > > > >>> +++ b/hw/virtio/vhost-vdpa.c > > > >>> @@ -712,13 +712,40 @@ static bool vhost_vdpa_svq_start_vq(struct vhost_dev *dev, unsigned idx) > > > >>> { > > > >>> struct vhost_vdpa *v = dev->opaque; > > > >>> VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, idx); > > > >>> - return vhost_svq_start(dev, idx, svq); > > > >>> + EventNotifier *vhost_call_notifier = vhost_svq_get_svq_call_notifier(svq); > > > >>> + struct vhost_vring_file vhost_call_file = { > > > >>> + .index = idx + dev->vq_index, > > > >>> + .fd = event_notifier_get_fd(vhost_call_notifier), > > > >>> + }; > > > >>> + int r; > > > >>> + bool b; > > > >>> + > > > >>> + /* Set shadow vq -> guest notifier */ > > > >>> + assert(v->call_fd[idx]); > > > >> > > > >> We need aovid the asser() here. On which case we can hit this? > > > >> > > > > I would say that there is no way we can actually hit it, so let's remove it. > > > > > > > >>> + vhost_svq_set_guest_call_notifier(svq, v->call_fd[idx]); > > > >>> + > > > >>> + b = vhost_svq_start(dev, idx, svq); > > > >>> + if (unlikely(!b)) { > > > >>> + return false; > > > >>> + } > > > >>> + > > > >>> + /* Set device -> SVQ notifier */ > > > >>> + r = vhost_vdpa_set_vring_dev_call(dev, &vhost_call_file); > > > >>> + if (unlikely(r)) { > > > >>> + error_report("vhost_vdpa_set_vring_call for shadow vq failed"); > > > >>> + return false; > > > >>> + } > > > >> > > > >> Similar to kick, do we need to set_vring_call() before vhost_svq_start()? > > > >> > > > > It should not matter at this moment because the device should not be > > > > started at this point and device calls should not run > > > > vhost_svq_handle_call until BQL is released. > > > > > > > > > Yes, we stop virtqueue before. > > > > > > > > > > > > > > The "logic" of doing it after is to make clear that svq must be fully > > > > initialized before processing device calls, even in the case that we > > > > extract SVQ in its own iothread or similar. But this could be done > > > > before vhost_svq_start for sure. > > > > > > > >>> + > > > >>> + /* Check for pending calls */ > > > >>> + event_notifier_set(vhost_call_notifier); > > > >> > > > >> Interesting, can this result spurious interrupt? > > > >> > > > > This actually "queues" a vhost_svq_handle_call after the BQL release, > > > > where the device should be fully reset. In that regard, if there are > > > > no used descriptors there will not be an irq raised to the guest. Does > > > > that answer the question? Or have I missed something? > > > > > > > > > Yes, please explain this in the comment. > > > > > > > I'm reviewing this again, and actually I think I was wrong in solving the issue. > > > > Since at this point the device is being configured, there is no chance > > that we had a missing call notification here: A previous kick is > > needed for the device to generate any calls, and these cannot be > > processed. > > > > What is not solved in this series is that we could have pending used > > buffers in vdpa device stopping SVQ, but queuing a check for that is > > not going to solve anything, since SVQ vring would be already > > destroyed: > > > > * vdpa device marks N > 0 buffers as used, and calls. > > * Before processing them, SVQ stop is called. SVQ have not processed > > these, and cleans them, making this event_notifier_set useless. > > > > So this would require a few changes. Mainly, instead of queueing a > > check for used, these need to be checked before svq cleaning. After > > that, obtain the VQ state (is not obtained in the stop at the moment, > > trusting in guest's used idx) and run a last > > vhost_svq_handle_call_no_test while the device is paused. > > It looks to me what's really important is that SVQ needs to > drain/forwared used buffers after vdpa is stopped. Then we should be > fine. > Right. I think I picked the wrong place to raise the concern, but the next revision will include the drain of the pending buffers. Thanks! > > > > Thanks! > > > > > > > > > > > > >>> + return true; > > > >>> } > > > >>> > > > >>> static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > > > >>> { > > > >>> struct vhost_dev *hdev = v->dev; > > > >>> unsigned n; > > > >>> + int r; > > > >>> > > > >>> if (enable == v->shadow_vqs_enabled) { > > > >>> return hdev->nvqs; > > > >>> @@ -752,9 +779,18 @@ static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) > > > >>> if (!enable) { > > > >>> /* Disable all queues or clean up failed start */ > > > >>> for (n = 0; n < v->shadow_vqs->len; ++n) { > > > >>> + struct vhost_vring_file file = { > > > >>> + .index = vhost_vdpa_get_vq_index(hdev, n), > > > >>> + .fd = v->call_fd[n], > > > >>> + }; > > > >>> + > > > >>> + r = vhost_vdpa_set_vring_call(hdev, &file); > > > >>> + assert(r == 0); > > > >>> + > > > >>> unsigned vq_idx = vhost_vdpa_get_vq_index(hdev, n); > > > >>> VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, n); > > > >>> vhost_svq_stop(hdev, n, svq); > > > >>> + /* TODO: This can unmask or override call fd! */ > > > >> > > > >> I don't get this comment. Does this mean the current code can't work > > > >> with mask_notifiers? If yes, this is something we need to fix. > > > >> > > > > Yes, but it will be addressed in the next series. I should have > > > > explained it bette here, sorry :). > > > > > > > > > Ok. > > > > > > Thanks > > > > > > > > > > > > > > Thanks! > > > > > > > >> Thanks > > > >> > > > >> > > > >>> vhost_virtqueue_start(hdev, hdev->vdev, &hdev->vqs[n], vq_idx); > > > >>> } > > > >>> > > > > > >
diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c index 21dc99ab5d..3fe129cf63 100644 --- a/hw/virtio/vhost-shadow-virtqueue.c +++ b/hw/virtio/vhost-shadow-virtqueue.c @@ -53,6 +53,22 @@ static void vhost_handle_guest_kick(EventNotifier *n) event_notifier_set(&svq->kick_notifier); } +/* Forward vhost notifications */ +static void vhost_svq_handle_call_no_test(EventNotifier *n) +{ + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, + call_notifier); + + event_notifier_set(&svq->guest_call_notifier); +} + +static void vhost_svq_handle_call(EventNotifier *n) +{ + if (likely(event_notifier_test_and_clear(n))) { + vhost_svq_handle_call_no_test(n); + } +} + /* * Obtain the SVQ call notifier, where vhost device notifies SVQ that there * exists pending used buffers. @@ -180,6 +196,8 @@ VhostShadowVirtqueue *vhost_svq_new(struct vhost_dev *dev, int idx) } svq->vq = virtio_get_queue(dev->vdev, vq_idx); + event_notifier_set_handler(&svq->call_notifier, + vhost_svq_handle_call); return g_steal_pointer(&svq); err_init_call_notifier: @@ -195,6 +213,7 @@ err_init_kick_notifier: void vhost_svq_free(VhostShadowVirtqueue *vq) { event_notifier_cleanup(&vq->kick_notifier); + event_notifier_set_handler(&vq->call_notifier, NULL); event_notifier_cleanup(&vq->call_notifier); g_free(vq); } diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index bc34de2439..6c5f4c98b8 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -712,13 +712,40 @@ static bool vhost_vdpa_svq_start_vq(struct vhost_dev *dev, unsigned idx) { struct vhost_vdpa *v = dev->opaque; VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, idx); - return vhost_svq_start(dev, idx, svq); + EventNotifier *vhost_call_notifier = vhost_svq_get_svq_call_notifier(svq); + struct vhost_vring_file vhost_call_file = { + .index = idx + dev->vq_index, + .fd = event_notifier_get_fd(vhost_call_notifier), + }; + int r; + bool b; + + /* Set shadow vq -> guest notifier */ + assert(v->call_fd[idx]); + vhost_svq_set_guest_call_notifier(svq, v->call_fd[idx]); + + b = vhost_svq_start(dev, idx, svq); + if (unlikely(!b)) { + return false; + } + + /* Set device -> SVQ notifier */ + r = vhost_vdpa_set_vring_dev_call(dev, &vhost_call_file); + if (unlikely(r)) { + error_report("vhost_vdpa_set_vring_call for shadow vq failed"); + return false; + } + + /* Check for pending calls */ + event_notifier_set(vhost_call_notifier); + return true; } static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) { struct vhost_dev *hdev = v->dev; unsigned n; + int r; if (enable == v->shadow_vqs_enabled) { return hdev->nvqs; @@ -752,9 +779,18 @@ static unsigned vhost_vdpa_enable_svq(struct vhost_vdpa *v, bool enable) if (!enable) { /* Disable all queues or clean up failed start */ for (n = 0; n < v->shadow_vqs->len; ++n) { + struct vhost_vring_file file = { + .index = vhost_vdpa_get_vq_index(hdev, n), + .fd = v->call_fd[n], + }; + + r = vhost_vdpa_set_vring_call(hdev, &file); + assert(r == 0); + unsigned vq_idx = vhost_vdpa_get_vq_index(hdev, n); VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, n); vhost_svq_stop(hdev, n, svq); + /* TODO: This can unmask or override call fd! */ vhost_virtqueue_start(hdev, hdev->vdev, &hdev->vqs[n], vq_idx); }
This will make qemu aware of the device used buffers, allowing it to write the guest memory with its contents if needed. Since the use of vhost_virtqueue_start can unmasks and discard call events, vhost_virtqueue_start should be modified in one of these ways: * Split in two: One of them uses all logic to start a queue with no side effects for the guest, and another one tha actually assumes that the guest has just started the device. Vdpa should use just the former. * Actually store and check if the guest notifier is masked, and do it conditionally. * Left as it is, and duplicate all the logic in vhost-vdpa. Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/vhost-shadow-virtqueue.c | 19 +++++++++++++++ hw/virtio/vhost-vdpa.c | 38 +++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 1 deletion(-)