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

[Eugenio Perez Martin]

On Tue, Jun 8, 2021 at 4:23 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Eugenio Perez Martin <eperezma@redhat.com> writes:
>
> > 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
>
> [...]
>
> >> > +#
> >> > +# 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.
>
> [...]
>
> >> 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?
>
> We might be talking past each other.  Let me try again :)
>
> The following question is *not* about the doc comment, it's about the
> *code*: what practical problem is solved by using DeviceNotFound instead
> of GenericError for some errors?
>

Sorry, I'm not sure if I follow you :). At risk of being circular in
this topic, the only use case I can think is to actually tell the
difference between "the device does not exists, or is not a vhost
device" and "the device does not support SVQ because X", where X can
be "it uses a packed ring", "it uses event idx", ...

I can only think of one practical use case, "if you see this error,
you probably forgot to set vhost=on in the command line, or something
is forbidding this device to be a vhost one". Having said that, I'm
totally fine with using GenericError always, but I see the more
fine-grained the error the better. What would be the advantage of also
using GenericError here?

Just to be sure that we are on the same page, I think this is better
seen from PATCH 07/39: vhost: Route guest->host notification through
shadow virtqueue.

> [...]
>