Jason Wang
2021-Oct-15 04:42 UTC
[RFC PATCH v4 11/20] vhost: Route host->guest notification through shadow virtqueue
? 2021/10/15 ??12:39, Eugenio Perez Martin ??:> On Wed, Oct 13, 2021 at 5:47 AM Jason Wang <jasowang at 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 at 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); >>> } >>>