On Tue, Oct 12, 2021 at 7:18 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Eugenio Pérez <eperezma@redhat.com> writes:
>
> > Command to enable shadow virtqueue.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > qapi/net.json | 23 +++++++++++++++++++++++
> > hw/virtio/vhost-vdpa.c | 8 ++++++++
> > 2 files changed, 31 insertions(+)
> >
> > diff --git a/qapi/net.json b/qapi/net.json
> > index 7fab2e7cd8..a2c30fd455 100644
> > --- a/qapi/net.json
> > +++ b/qapi/net.json
> > @@ -79,6 +79,29 @@
> > { 'command': 'netdev_del', 'data': {'id': 'str'},
> > 'allow-preconfig': true }
> >
> > +##
> > +# @x-vhost-enable-shadow-vq:
> > +#
> > +# Use vhost shadow virtqueue.
> > +#
> > +# @name: the device name of the VirtIO device
>
> Is this a qdev ID? A network client name?
>
At this moment is the virtio device name, the one specified at the
call of "virtio_init". But this should change, maybe the qdev id or
something that can be provided by the command line fits better here.
> > +#
> > +# @enable: true to use the alternate shadow VQ notifications
> > +#
> > +# Returns: Always error, since SVQ is not implemented at the moment.
> > +#
> > +# Since: 6.2
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "x-vhost-enable-shadow-vq",
> > +# "arguments": { "name": "virtio-net", "enable": false } }
> > +#
> > +##
> > +{ 'command': 'x-vhost-enable-shadow-vq',
> > + 'data': {'name': 'str', 'enable': 'bool'},
> > + 'if': 'defined(CONFIG_VHOST_KERNEL)' }
> > +
>
> Adding an command just for controlling a flag in some object is fine for
> quick experiments. As a permanent interface, it's problematic: one
> command per flag would result in way too many commands. Better: one
> command to control a set of related properties.
>
> I hesitate to suggest qom-set, because qom-set is not introspectable.
> Recurring complaint about QOM: poor integration with QAPI/QMP.
>
I will take it into account, but it's only temporary, that's why it
has the x- prefix. It's not like event_idx or other device feature
flags: Every vDPA device can potentially use SVQ datapath in a
transparent way, neither the vDPA device nor the guest know that qemu
supports it.
Ideally, this mode will kick in at the migration time automatically,
no need to perform more actions.
> Naming nitpick: since the command can both enable and disable, I'd call
> it -set-vq instead of -enable-vq.
>
Got it, I will replace it.
Thanks!
> > ##
> > # @NetLegacyNicOptions:
> > #
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 4fa414feea..c63e311d7c 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -23,6 +23,8 @@
> > #include "cpu.h"
> > #include "trace.h"
> > #include "qemu-common.h"
> > +#include "qapi/qapi-commands-net.h"
> > +#include "qapi/error.h"
> >
> > static bool vhost_vdpa_listener_skipped_section(MemoryRegionSection *section)
> > {
> > @@ -656,6 +658,12 @@ static bool vhost_vdpa_force_iommu(struct vhost_dev *dev)
> > return true;
> > }
> >
> > +
> > +void qmp_x_vhost_enable_shadow_vq(const char *name, bool enable, Error **errp)
> > +{
> > + error_setg(errp, "Shadow virtqueue still not implemented");
> > +}
> > +
> > const VhostOps vdpa_ops = {
> > .backend_type = VHOST_BACKEND_TYPE_VDPA,
> > .vhost_backend_init = vhost_vdpa_init,
>