[libvirt] [PATCH 17/20] qemu: Support newer ivshmem device variants

[Peter Krempa]

On Thu, Sep 15, 2016 at 18:14:42 +0200, Martin Kletzander wrote:
> QEMU added support for ivshmem-plain and ivshmem-doorbell.  Those are
> reworked varians of legacy ivshmem that are compatible from the guest
> POV, but not from host's POV and have sane specification and handling.
> 
> Details about the newer device type can be found in qemu's commit
> 5400c02b90bb:
> 
>   http://git.qemu.org/?p=qemu.git;a=commit;h=5400c02b90bb
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  src/qemu/qemu_command.c                            | 137 ++++++++++++++++++++-
>  src/qemu/qemu_command.h                            |  10 ++
>  .../qemuxml2argv-shmem-plain-doorbell.args         |  46 +++++++
>  .../qemuxml2argv-shmem-plain-doorbell.xml          |  63 ++++++++++
>  tests/qemuxml2argvtest.c                           |   3 +
>  .../qemuxml2xmlout-shmem-plain-doorbell.xml        |  70 +++++++++++
>  tests/qemuxml2xmltest.c                            |   2 +
>  7 files changed, 328 insertions(+), 3 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-shmem-plain-doorbell.xml
>  create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-shmem-plain-doorbell.xml
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a28d503943d2..45e5b8e5ce27 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -8597,6 +8597,82 @@ qemuBuildShmemDevLegacyStr(virDomainDefPtr def,
>      return NULL;
>  }
> 
> +char *
> +qemuBuildShmemDevStr(virDomainDefPtr def,
> +                     virDomainShmemDefPtr shmem,
> +                     virQEMUCapsPtr qemuCaps)
> +{
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM_PLAIN) {

This is weird and unflexible and not checked by the compiler. Either
duplicate the switch from the caller function here or preferably split
the generators for the doorbel and plain device into separate functions
rather than separate branches.

> +        if (shmem->server.enabled) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("ivshmem-plain is only supported without server "
> +                             "option, try using ivshmem-doorbell instead"));
> +            return NULL;
> +        }

Since you've added post parse handling for this you can move checks like
this there so they don't clutter up the command line generator.

> +
> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM_PLAIN)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("ivshmem-plain device is not supported "
> +                             "with this QEMU binary"));
> +            return NULL;
> +        }
> +    } else {
> +        if (!shmem->server.enabled) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("ivshmem-doorbell is only supported with server "
> +                             "option, try using ivshmem-doorbell instead"));
> +            return NULL;
> +        }
> +
> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_IVSHMEM_PLAIN)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("ivshmem-doorbell device is not supported "
> +                             "with this QEMU binary"));
> +            return NULL;
> +        }
> +    }
> +
> +    if (shmem->server.enabled) {

This is true but needs to be compiled from implications of the above
check. Decide based on the type and make sure that configuration is
right for the given type rather than relying on other stuff.

> +        virBufferAddLit(&buf, "ivshmem-doorbell");
> +        virBufferAsprintf(&buf, ",id=%s,chardev=char%s",
> +                          shmem->info.alias, shmem->info.alias);
> +        if (shmem->msi.vectors)
> +            virBufferAsprintf(&buf, ",vectors=%u", shmem->msi.vectors);
> +        if (shmem->msi.ioeventfd)
> +            virBufferAsprintf(&buf, ",ioeventfd=%s",
> +                              virTristateSwitchTypeToString(shmem->msi.ioeventfd));
> +    } else {
> +        virBufferAddLit(&buf, "ivshmem-plain");
> +        virBufferAsprintf(&buf, ",id=%s,memdev=shmmem-%s",
> +                          shmem->info.alias, shmem->info.alias);
> +
> +        virBufferAddLit(&buf, ",master=");
> +        switch ((virDomainShmemRole) shmem->role) {
> +        case VIR_DOMAIN_SHMEM_ROLE_MASTER:
> +            virBufferAddLit(&buf, "on");
> +            break;
> +        case VIR_DOMAIN_SHMEM_ROLE_PEER:
> +            virBufferAddLit(&buf, "off");
> +            break;
> +        case VIR_DOMAIN_SHMEM_ROLE_DEFAULT:
> +        case VIR_DOMAIN_SHMEM_ROLE_LAST:
> +            break;
> +        }
> +    }

Peter