On Thu, Mar 3, 2022 at 8:12 AM Jason Wang <jasow...@redhat.com> wrote: > > > 在 2022/3/2 上午2:49, Eugenio Perez Martin 写道: > > On Mon, Feb 28, 2022 at 3:57 AM Jason Wang<jasow...@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<epere...@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. > Thanks > >