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     | 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:
+#
+# 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
+#
+# Since: 5.3
+#
+# Example:
+#
+# -> { "execute": "x-vhost_enable_shadow_vq", "arguments": {"enable": true} }
+# <- { "return": { "enabled" : true } }
+#
+##
+{ '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.");
+}
-- 
2.27.0

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 <eperezma@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.

> +#
> +##
> +{ '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 '.'.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Feb 2, 2021 at 4:38 PM Eric Blake <eblake@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 <eperezma@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
>

Eugenio Perez Martin <eperezma@redhat.com> writes:

> On Tue, Feb 2, 2021 at 4:38 PM Eric Blake <eblake@redhat.com> wrote:
>>
>> On 1/29/21 2:54 PM, Eugenio Pérez wrote:
[...]
>> > 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.

It's pretty much always a good moment for patches improving error
messages :)

On Thu, Feb 4, 2021 at 1:16 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Eugenio Perez Martin <eperezma@redhat.com> writes:
>
> > On Tue, Feb 2, 2021 at 4:38 PM Eric Blake <eblake@redhat.com> wrote:
> >>
> >> On 1/29/21 2:54 PM, Eugenio Pérez wrote:
> [...]
> >> > 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.
>
> It's pretty much always a good moment for patches improving error
> messages :)
>

Great, changing it too :).

Thanks!