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

From: Markus Armbruster <armbru@redhat.com>
To: Eugenio Perez Martin <eperezma@redhat.com>
Cc: Parav Pandit <parav@mellanox.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	Juan Quintela <quintela@redhat.com>,
	qemu-level <qemu-devel@nongnu.org>,
	virtualization@lists.linux-foundation.org,
	Harpreet Singh Anand <hanand@xilinx.com>,
	Xiao W Wang <xiao.w.wang@intel.com>,
	Stefan Hajnoczi <stefanha@redhat.com>,
	Eli Cohen <eli@mellanox.com>, Michael Lilja <ml@napatech.com>,
	Stefano Garzarella <sgarzare@redhat.com>
Subject: Re: [RFC v3 04/29] vhost: Add x-vhost-enable-shadow-vq qmp
Date: Tue, 08 Jun 2021 16:23:23 +0200	[thread overview]
Message-ID: <87k0n4xwo4.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <CAJaqyWf9HZKb+Mtn_UNHZQj4EqzybPzwbLE7vcbCXJPK7wtXKw@mail.gmail.com> (Eugenio Perez Martin's message of "Mon, 24 May 2021 09:13:39 +0200")

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?

[...]