Jason Wang
2022-Mar-04 01:39 UTC
[PATCH v2 02/14] vhost: Add Shadow VirtQueue kick forwarding capabilities
On Thu, Mar 3, 2022 at 5:25 PM Eugenio Perez Martin <eperezma at redhat.com> wrote:> > On Thu, Mar 3, 2022 at 8:12 AM Jason Wang <jasowang at redhat.com> wrote: > > > > > > ? 2022/3/2 ??2:49, Eugenio Perez Martin ??: > > > On Mon, Feb 28, 2022 at 3:57 AM Jason Wang<jasowang at redhat.com> wrote: > > >> ? 2022/2/27 ??9:40, Eugenio P?rez ??: > > >>> At this mode no buffer forwarding will be performed in SVQ mode: Qemu > > >>> will just forward the guest's kicks to the device. > > >>> > > >>> Host memory notifiers regions are left out for simplicity, and they will > > >>> not be addressed in this series. > > >>> > > >>> Signed-off-by: Eugenio P?rez<eperezma at redhat.com> > > >>> --- > > >>> hw/virtio/vhost-shadow-virtqueue.h | 14 +++ > > >>> include/hw/virtio/vhost-vdpa.h | 4 + > > >>> hw/virtio/vhost-shadow-virtqueue.c | 52 +++++++++++ > > >>> hw/virtio/vhost-vdpa.c | 145 ++++++++++++++++++++++++++++- > > >>> 4 files changed, 213 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h > > >>> index f1519e3c7b..1cbc87d5d8 100644 > > >>> --- a/hw/virtio/vhost-shadow-virtqueue.h > > >>> +++ b/hw/virtio/vhost-shadow-virtqueue.h > > >>> @@ -18,8 +18,22 @@ typedef struct VhostShadowVirtqueue { > > >>> EventNotifier hdev_kick; > > >>> /* Shadow call notifier, sent to vhost */ > > >>> EventNotifier hdev_call; > > >>> + > > >>> + /* > > >>> + * Borrowed virtqueue's guest to host notifier. To borrow it in this event > > >>> + * notifier allows to recover the VhostShadowVirtqueue from the event loop > > >>> + * easily. If we use the VirtQueue's one, we don't have an easy way to > > >>> + * retrieve VhostShadowVirtqueue. > > >>> + * > > >>> + * So shadow virtqueue must not clean it, or we would lose VirtQueue one. > > >>> + */ > > >>> + EventNotifier svq_kick; > > >>> } VhostShadowVirtqueue; > > >>> > > >>> +void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd); > > >>> + > > >>> +void vhost_svq_stop(VhostShadowVirtqueue *svq); > > >>> + > > >>> VhostShadowVirtqueue *vhost_svq_new(void); > > >>> > > >>> void vhost_svq_free(gpointer vq); > > >>> diff --git a/include/hw/virtio/vhost-vdpa.h b/include/hw/virtio/vhost-vdpa.h > > >>> index 3ce79a646d..009a9f3b6b 100644 > > >>> --- a/include/hw/virtio/vhost-vdpa.h > > >>> +++ b/include/hw/virtio/vhost-vdpa.h > > >>> @@ -12,6 +12,8 @@ > > >>> #ifndef HW_VIRTIO_VHOST_VDPA_H > > >>> #define HW_VIRTIO_VHOST_VDPA_H > > >>> > > >>> +#include <gmodule.h> > > >>> + > > >>> #include "hw/virtio/virtio.h" > > >>> #include "standard-headers/linux/vhost_types.h" > > >>> > > >>> @@ -27,6 +29,8 @@ typedef struct vhost_vdpa { > > >>> bool iotlb_batch_begin_sent; > > >>> MemoryListener listener; > > >>> struct vhost_vdpa_iova_range iova_range; > > >>> + bool shadow_vqs_enabled; > > >>> + GPtrArray *shadow_vqs; > > >>> struct vhost_dev *dev; > > >>> VhostVDPAHostNotifier notifier[VIRTIO_QUEUE_MAX]; > > >>> } VhostVDPA; > > >>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c > > >>> index 019cf1950f..a5d0659f86 100644 > > >>> --- a/hw/virtio/vhost-shadow-virtqueue.c > > >>> +++ b/hw/virtio/vhost-shadow-virtqueue.c > > >>> @@ -11,6 +11,56 @@ > > >>> #include "hw/virtio/vhost-shadow-virtqueue.h" > > >>> > > >>> #include "qemu/error-report.h" > > >>> +#include "qemu/main-loop.h" > > >>> +#include "linux-headers/linux/vhost.h" > > >>> + > > >>> +/** Forward guest notifications */ > > >>> +static void vhost_handle_guest_kick(EventNotifier *n) > > >>> +{ > > >>> + VhostShadowVirtqueue *svq = container_of(n, VhostShadowVirtqueue, > > >>> + svq_kick); > > >>> + event_notifier_test_and_clear(n); > > >>> + event_notifier_set(&svq->hdev_kick); > > >>> +} > > >>> + > > >>> +/** > > >>> + * Set a new file descriptor for the guest to kick the SVQ and notify for avail > > >>> + * > > >>> + * @svq The svq > > >>> + * @svq_kick_fd The svq kick fd > > >>> + * > > >>> + * Note that the SVQ will never close the old file descriptor. > > >>> + */ > > >>> +void vhost_svq_set_svq_kick_fd(VhostShadowVirtqueue *svq, int svq_kick_fd) > > >>> +{ > > >>> + EventNotifier *svq_kick = &svq->svq_kick; > > >>> + bool poll_stop = VHOST_FILE_UNBIND != event_notifier_get_fd(svq_kick); > > >> I wonder if this is robust. E.g is there any chance that may end up with > > >> both poll_stop and poll_start are false? > > >> > > > I cannot make that happen in qemu, but the function supports that case > > > well: It will do nothing. It's more or less the same code as used in > > > the vhost kernel, and is the expected behaviour if you send two > > > VHOST_FILE_UNBIND one right after another to me. > > > > > > I would think it's just stop twice. > > > > > > > > > >> If not, can we simple detect poll_stop as below and treat !poll_start > > >> and poll_stop? > > >> > > > I'm not sure what does it add. Is there an unexpected consequence with > > > the current do-nothing behavior I've missed? > > > > > > I'm not sure, but it feels odd if poll_start is not the reverse value of > > poll_stop. > > > > If we want to not to restrict the inputs, we need to handle for situations: > > a) old_fd = -1, new_fd = -1, > > This is the situation you described, and it's basically a no-op. > poll_stop == poll_start == false. > > If we make poll_stop = true and poll_stop = false, we call > event_notifier_set_handler(-1, ...). Hopefully it will return just an > error. > > If we make poll_stop = false and poll_stop = true, we are calling > event_notifier_set(-1) and event_notifier_set_handler(-1, > poll_callback). Same situation, hopefully an error, but unexpected. > > b) old_fd = -1, new_fd = >-1, > > We need to start polling the new_fd. No need for stop polling the > old_fd, since we are not polling it actually. > > c) old_fd = >-1, new_fd = >-1, > > We need to stop polling the old_fd and start polling the new one. > > If we make poll_stop = true and poll_stop = false, we don't register a > new polling function for the new kick_fd so we will miss guest's > kicks. > > If we make poll_stop = false and poll_stop = true, we keep polling the > old file descriptor too, so whatever it gets assigned to could call > vhost_handle_guest_kick if it does not override poll callback. > > We *could* detect if old_fd == new_fd so we skip all the work, but I > think it is not worth it to complicate the code, since we're only > being called with the kick_fd at dev start. > > d) c) old_fd = >-1, new_fd = -1, > > We need to stop polling, or we could get invalid kicks callbacks if it > gets writed after this. No need to poll anything beyond this.I see, thanks for the clarification.> > > Thanks > > > > >