On Tue, Feb 2, 2021 at 4:38 PM Eric Blake <ebl...@redhat.com> wrote:
>
> On 1/29/21 2:54 PM, Eugenio Pérez wrote:
> > Command to enable shadow virtqueue looks like:
> >
> > { "execute": "x-vhost-enable-shadow-vq", "arguments": { "name": "dev0",
> > "enable": true } }
> >
> > Signed-off-by: Eugenio Pérez <epere...@redhat.com>
> > ---
> > qapi/net.json | 23 +++++++++++++++++++++++
> > hw/virtio/vhost.c | 6 ++++++
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/qapi/net.json b/qapi/net.json
> > index c31748c87f..6170d69798 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -77,6 +77,29 @@
> > ##
> > { 'command': 'netdev_del', 'data': {'id': 'str'} }
> >
> > +##
> > +# @x-vhost-enable-shadow-vq:
>
> This spelling is the preferred form...[1]
>
> > +#
> > +# Use vhost shadow virtqueue.
> > +#
> > +# @name: the device name of the virtual network adapter
> > +#
> > +# @enable: true to use he alternate shadow VQ notification path
> > +#
> > +# Returns: Error if failure, or 'no error' for success
>
> This line...[2]
>
> > +#
> > +# Since: 5.3
>
> The next release is 6.0, not 5.3.
>
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "x-vhost_enable_shadow_vq", "arguments": {"enable":
> > true} }
>
> [1]...but doesn't match the example.
>
> > +# <- { "return": { "enabled" : true } }
>
> [2]...doesn't match this comment. I'd just drop the line, since there
> is no explicit return listed.
>
Hi Eric.
Thanks for your comments, they will be addressed in the next revision.
> > +#
> > +##
> > +{ '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 040f68ff2e..42836e45f3 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"
> > @@ -1841,3 +1842,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.");
>
> error_setg() should not be passed a trailing '.'.
>
Oh, sorry I missed the comment in the error_setg doc.
I copy&pasted from the call to error_setg "Migration disabled: vhost
lacks VHOST_F_LOG_ALL feature.". I'm wondering if it's a good moment
to delete the dot there too, since other tools could depend on parsing
it.
Thanks!
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
