Re: [RFC v3 04/29] vhost: Add x-vhost-enable-shadow-vq qmp

[Eugenio Perez Martin]

On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Eugenio Pérez <eperezma@redhat.com> writes:
>
> > Command to enable shadow virtqueue looks like:
> >
> > { "execute": "x-vhost-enable-shadow-vq",
> >   "arguments": { "name": "dev0", "enable": true } }
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >  qapi/net.json     | 22 ++++++++++++++++++++++
> >  hw/virtio/vhost.c |  6 ++++++
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/qapi/net.json b/qapi/net.json
> > index c31748c87f..660feafdd2 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -77,6 +77,28 @@
> >  ##
> >  { 'command': 'netdev_del', 'data': {'id': 'str'} }
> >
> > +##
> > +# @x-vhost-enable-shadow-vq:
> > +#
> > +# Use vhost shadow virtqueue.
> > +#
> > +# @name: the device name of the VirtIO device
> > +#
> > +# @enable: true to use he alternate shadow VQ notification path
>
> Typo "he".
>

Thanks, I will fix it!

> What's a "notification path", and why should I care?
>
> Maybe
>
>    # @enable: Enable alternate shadow VQ notification
>

Your description is more accurate at some point of the series, I will fix it.

> > +#
> > +# Returns: Error if failure, or 'no error' for success. Not found if vhost 
> > is not enabled.
>
> This is confusing.  What do you mean by "Not found"?
>
> If you mean DeviceNotFound:
>
> 1. Not actually true: qmp_x_vhost_enable_shadow_vq() always fails with
> GenericError.  Perhaps later patches will change that.
>

Right, I left the documentation in an intermediate state. At this
point it will always return failure, and in future ones it depends on
some conditions as you may have seen.

If I carry the QMP command to future series, I will update the doc
accordly to every commit.

> 2. Do you really need to distinguish "vhost is not enabled" from other
> errors?
>

SVQ cannot work if the device backend is not vhost, like qemu VirtIO
dev. What I meant is that "qemu will only look for its name in the set
of vhost devices, so you will have a device not found if the device is
not a vhost one", which may not be 100% clear at first glance. Maybe
this wording is better?

> > +#
> > +# Since: 6.1
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "x-vhost-enable-shadow-vq", "arguments": { "name": 
> > "virtio-net", "enable": false } }
>
> Please break the long line, e.g. like this:
>
>    # -> { "execute": "x-vhost-enable-shadow-vq",
>    #      "arguments": { "name": "virtio-net", "enable": false } }
>
> We normally show output in examples, too.
>

Ok, I will fix both issues.

Thanks!

> > +#
> > +##
> > +{ 'command': 'x-vhost-enable-shadow-vq',
> > +  'data': {'name': 'str', 'enable': 'bool'},
> > +  'if': 'defined(CONFIG_VHOST_KERNEL)' }
> > +
> >  ##
> >  # @NetLegacyNicOptions:
> >  #
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 40f9f64ebd..c4c1f80661 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -15,6 +15,7 @@
> >
> >  #include "qemu/osdep.h"
> >  #include "qapi/error.h"
> > +#include "qapi/qapi-commands-net.h"
> >  #include "hw/virtio/vhost.h"
> >  #include "qemu/atomic.h"
> >  #include "qemu/range.h"
> > @@ -1831,3 +1832,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> >
> >      return -1;
> >  }
> > +
> > +void qmp_x_vhost_enable_shadow_vq(const char *name, bool enable, Error 
> > **errp)
> > +{
> > +    error_setg(errp, "Shadow virtqueue still not implemented");
> > +}
>