On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> Eugenio Pérez <epere...@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 <epere...@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");
> > +}
>
