[libvirt] [PATCH 1/4] qemu: Automatically choose usable GIC version

[Cole Robinson]

On 05/10/2016 08:46 AM, Andrea Bolognani wrote:
> When the <gic/> element in not present in the domain XML, use the
> domain capabilities to figure out what GIC version is usable and
> choose that one automatically.
> 
> This allows guests to be created on hardware that only supports
> GIC v3 without having to update virt-manager and similar tools.
> 
> Keep using the default GIC version if the <gic/> element has been
> added to the domain XML but no version has been specified, as not
> to break existing guests.
> ---
>  src/conf/domain_capabilities.c | 25 ++++++++++++++++
>  src/conf/domain_capabilities.h |  8 +++++
>  src/libvirt_private.syms       |  1 +
>  src/qemu/qemu_domain.c         | 66 ++++++++++++++++++++++++++++++++++--------
>  4 files changed, 88 insertions(+), 12 deletions(-)
> 
> diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c
> index 1676f0e..f9adeb9 100644
> --- a/src/conf/domain_capabilities.c
> +++ b/src/conf/domain_capabilities.c
> @@ -130,6 +130,31 @@ virDomainCapsEnumSet(virDomainCapsEnumPtr capsEnum,
>  }
>  
>  
> +bool
> +virDomainCapsEnumIsSet(const virDomainCapsEnum *capsEnum,
> +                       const char *capsEnumName,
> +                       unsigned int value)
> +{
> +    unsigned int val = 1 << value;
> +    bool ret = false;
> +
> +

Extra newline

> +    if (!val) {
> +        /* Integer overflow */
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("integer overflow on %s. Please contact the "
> +                         "libvirt development team at libvir-list at redhat.com"),
> +                       capsEnumName);
> +        goto cleanup;
> +    }
> +

Heh that's new to me, but I see you're just following the template of
virDomainCapsEnumSet, so okay

> +    ret = (capsEnum->values & val);
> +
> + cleanup:
> +    return ret;
> +}
> +
> +
>  void
>  virDomainCapsEnumClear(virDomainCapsEnumPtr capsEnum)
>  {
> diff --git a/src/conf/domain_capabilities.h b/src/conf/domain_capabilities.h
> index 492a9cf..8ed4111 100644
> --- a/src/conf/domain_capabilities.h
> +++ b/src/conf/domain_capabilities.h
> @@ -137,10 +137,18 @@ virDomainCapsPtr virDomainCapsNew(const char *path,
>                               __nvalues, __values);          \
>      } while (0)
>  
> +# define VIR_DOMAIN_CAPS_ENUM_IS_SET(capsEnum, value)       \
> +    virDomainCapsEnumIsSet(&(capsEnum), #capsEnum, value)   \
> +
>  int virDomainCapsEnumSet(virDomainCapsEnumPtr capsEnum,
>                           const char *capsEnumName,
>                           size_t nvalues,
>                           unsigned int *values);
> +
> +bool virDomainCapsEnumIsSet(const virDomainCapsEnum *capsEnum,
> +                            const char *capsEnumName,
> +                            unsigned int value);
> +
>  void virDomainCapsEnumClear(virDomainCapsEnumPtr capsEnum);
>  
>  char * virDomainCapsFormat(virDomainCapsPtr const caps);
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fff8c30..183e84a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -141,6 +141,7 @@ virDomainAuditVcpu;
>  
>  # conf/domain_capabilities.h
>  virDomainCapsEnumClear;
> +virDomainCapsEnumIsSet;
>  virDomainCapsEnumSet;
>  virDomainCapsFormat;
>  virDomainCapsNew;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 93f0a01..d34450f 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1921,26 +1921,67 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,
>   * Make sure that features that should be enabled by default are actually
>   * enabled and configure default values related to those features.
>   */
> -static void
> -qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def)
> +static int
> +qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
> +                                   virQEMUCapsPtr qemuCaps)
>  {
> -    switch (def->os.arch) {
> -    case VIR_ARCH_ARMV7L:
> -    case VIR_ARCH_AARCH64:
> -        if (qemuDomainMachineIsVirt(def)) {
> -            /* GIC is always available to ARM virt machines */
> -            def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
> +    virDomainCapsPtr caps = NULL;
> +    virDomainCapsFeatureGICPtr gic = NULL;
> +    int ret = -1;
> +
> +    /* The virt machine type always uses GIC: if the relevant element
> +     * was not included in the domain XML, we need to choose a suitable
> +     * GIC version ourselves */
> +    if ((def->os.arch == VIR_ARCH_ARMV7L ||
> +         def->os.arch == VIR_ARCH_AARCH64) &&
> +        qemuDomainMachineIsVirt(def) &&
> +        def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT) {
> +
> +        if (!(caps = virDomainCapsNew(def->emulator,
> +                                      def->os.machine,
> +                                      def->os.arch,
> +                                      def->virtType)))
> +            goto cleanup;
> +

Call this domCaps ? 'caps' name is usually used for virCapabilities

> +        if (virQEMUCapsFillDomainCaps(caps, qemuCaps, NULL, 0) < 0)
> +            goto cleanup;
> +
> +        gic = &(caps->gic);
> +
> +        /* Pick the best GIC version from those available */
> +        if (gic->supported) {
> +            virGICVersion version;
> +
> +            VIR_DEBUG("Looking for usable GIC version in domain capabilities");
> +            for (version = VIR_GIC_VERSION_LAST - 1;
> +                 version > VIR_GIC_VERSION_NONE;
> +                 version--) {
> +                if (VIR_DOMAIN_CAPS_ENUM_IS_SET(gic->version, version)) {
> +
> +                    VIR_DEBUG("Using GIC version %s",
> +                              virGICVersionTypeToString(version));
> +                    def->gic_version = version;
> +                    break;
> +                }
> +            }
>          }

Hmm that's a bit of a new pattern... it seems the only thing you really need
from domcaps is the bit of logic we encode via
virQEMUCapsFillDomainFeatureGICCaps. Maybe break that logic out into a public
function and call it here, rather than spinning up domcaps for a small bit of
info? Or is there more to it?

- Cole

> -        break;
>  
> -    default:
> -        break;
> +        /* Even if we haven't found a usable GIC version in the domain 
> +         * capabilities, we still want to enable this */
> +        def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
>      }
>  
>      /* Use the default GIC version if no version was specified */
>      if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
>          def->gic_version == VIR_GIC_VERSION_NONE)
>          def->gic_version = VIR_GIC_VERSION_DEFAULT;
> +
> +    ret = 0;
> +
> + cleanup:
> +    virObjectUnref(caps);
> +
> +    return ret;
>  }
>  
>  
> @@ -2033,7 +2074,8 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>      if (qemuCanonicalizeMachine(def, qemuCaps) < 0)
>          goto cleanup;
>  
> -    qemuDomainDefEnableDefaultFeatures(def);
> +    if (qemuDomainDefEnableDefaultFeatures(def, qemuCaps) < 0)
> +        goto cleanup;
>  
>      qemuDomainRecheckInternalPaths(def, cfg, parseFlags);
>  
>