Re: [RFC PATCH v4 15/20] vhost: Shadow virtqueue buffers forwarding

[Eugenio Perez Martin]

On Tue, Oct 12, 2021 at 3:48 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Eugenio Perez Martin <eperezma@redhat.com> writes:
>
> > On Tue, Oct 12, 2021 at 7:21 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> Eugenio Pérez <eperezma@redhat.com> writes:
> >>
> >> > Initial version of shadow virtqueue that actually forward buffers. There
> >> > are no iommu support at the moment, and that will be addressed in future
> >> > patches of this series. Since all vhost-vdpa devices uses forced IOMMU,
> >> > this means that SVQ is not usable at this point of the series on any
> >> > device.
> >> >
> >> > For simplicity it only supports modern devices, that expects vring
> >> > in little endian, with split ring and no event idx or indirect
> >> > descriptors. Support for them will not be added in this series.
> >> >
> >> > It reuses the VirtQueue code for the device part. The driver part is
> >> > based on Linux's virtio_ring driver, but with stripped functionality
> >> > and optimizations so it's easier to review. Later commits add simpler
> >> > ones.
> >> >
> >> > SVQ uses VIRTIO_CONFIG_S_DEVICE_STOPPED to pause the device and
> >> > retrieve its status (next available idx the device was going to
> >> > consume) race-free. It can later reset the device to replace vring
> >> > addresses etc. When SVQ starts qemu can resume consuming the guest's
> >> > driver ring from that state, without notice from the latter.
> >> >
> >> > This status bit VIRTIO_CONFIG_S_DEVICE_STOPPED is currently discussed
> >> > in VirtIO, and is implemented in qemu VirtIO-net devices in previous
> >> > commits.
> >> >
> >> > Removal of _S_DEVICE_STOPPED bit (in other words, resuming the device)
> >> > can be done in the future if an use case arises. At this moment we can
> >> > just rely on reseting the full device.
> >> >
> >> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> > ---
> >> >  qapi/net.json                      |   2 +-
> >> >  hw/virtio/vhost-shadow-virtqueue.c | 237 ++++++++++++++++++++++++++++-
> >> >  hw/virtio/vhost-vdpa.c             | 109 ++++++++++++-
> >> >  3 files changed, 337 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/qapi/net.json b/qapi/net.json
> >> > index fe546b0e7c..1f4a55f2c5 100644
> >> > --- a/qapi/net.json
> >> > +++ b/qapi/net.json
> >> > @@ -86,7 +86,7 @@
> >> >  #
> >> >  # @name: the device name of the VirtIO device
> >> >  #
> >> > -# @enable: true to use the alternate shadow VQ notifications
> >> > +# @enable: true to use the alternate shadow VQ buffers fowarding path
> >>
> >> Uh, why does the flag change meaning half-way through this series?
> >>
> >
> > Before this patch, the SVQ mode just makes an extra hop for
> > notifications. Guest ones are now received by qemu via ioeventfd, and
> > qemu forwards them to the device using a different eventfd. The
> > reverse is also true: the device ones will be received by qemu by
> > device call fd, and then qemu will forward them to the guest using a
> > different irqfd.
> >
> > This intermediate step is not very useful by itself, but helps for
> > checking that that part of the communication works fine, with no need
> > for shadow virtqueue to understand vring format. Doing that way also
> > produces smaller patches.
> >
> > So it makes sense to me to tell what QMP command does exactly at every
> > point of the series. However I can directly document it as "use the
> > alternate shadow VQ buffers forwarding path" from the beginning.
> >
> > Does this make sense, or will it be better to write the final
> > intention of the command?
> >
> > Thanks!
>
> Working your explanation into commit messages and possibly comments
> should do.
>

Got it, I will include them in both. Thanks!