| Message ID | 20210129205415.876290-9-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vDPA shadow virtqueue - notifications forwarding | expand |
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 '.'.
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!
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."); +}
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(+)