On Wed, Jun 9, 2021 at 1:46 PM Markus Armbruster <[email protected]> wrote: > > Eugenio Perez Martin <[email protected]> writes: > > > On Tue, Jun 8, 2021 at 4:23 PM Markus Armbruster <[email protected]> wrote: > >> > >> Eugenio Perez Martin <[email protected]> writes: > >> > >> > On Fri, May 21, 2021 at 9:05 AM Markus Armbruster <[email protected]> > >> > wrote: > >> >> > >> >> Eugenio Pérez <[email protected]> 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 <[email protected]> > >> >> > --- > >> >> > 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? > > In the initial design of the Error API, every error had its own distinct > class. This provided for fine-grained errors. > > Adding a new error was bothersome: you had to define a new class, in > qerror.h. Recompile the world. Conflict magnet. Constant temptation > to reuse an existing error even when its error message is suboptimal, > and the reuse of the class for another error conflates errors. > > After a bit under three years, we had 70 classes, used in almost 400 > places. Management applications actually cared for just six classes. > > Bad error messages and development friction had turned out to be a real > problem. Conflating errors pretty much not. > > We concluded that providing for fine-grained errors when next to nothing > uses them was not worth the pain. So we ditched them: > > https://lists.nongnu.org/archive/html/qemu-devel/2012-08/msg01958.html > Commit ac839ccd8c3..adb2072ed0f > > Since them, we recommend to use GenericError unless there is a > compelling reason not to. "Something might care someday" doesn't > qualify. > > Learning by doing the wrong thing is painful and expensive, but at least > the lessons tends to stick ;) > > Today, we have more than 4000 callers of error_setg(), and less than 40 > of error_set(). >
So let's do it with GenericError then :). Thanks for pointing it out, it will be fixed in the next revision! > > 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. > > > >> [...] > >> >
