[libvirt] [PATCH v2 2/5] qemu: Automatically choose usable GIC version

[John Ferlan]

On 05/16/2016 06:00 PM, 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/qemu/qemu_domain.c | 49 +++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b0eb3b6..ec59763 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -1945,25 +1945,50 @@ qemuDomainDefAddDefaultDevices(virDomainDefPtr def,

Document @qemuCaps


>   * enabled and configure default values related to those features.
>   */
>  static void
> -qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def)
> +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;
> +    do {

Not a fan...  Isn't what you're doing the same as:

    if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT &&
        (def->os.arch == VIR_ARCH_ARMV7L || def->os.arch == VIR_ARCH_AARCH64) &&
        qemuDomainMachineIsVirt(def)) {

> +        virGICVersion version;
> +
> +        if (def->features[VIR_DOMAIN_FEATURE_GIC] != VIR_TRISTATE_SWITCH_ABSENT)
> +            break;
> +
> +        if (def->os.arch != VIR_ARCH_ARMV7L &&
> +            def->os.arch != VIR_ARCH_AARCH64)
> +            break;
> +
> +        if (!qemuDomainMachineIsVirt(def))
> +            break;

removing these checks

> +
> +        /* 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 */
> +        VIR_DEBUG("Looking for usable GIC version in domain capabilities");
> +        for (version = VIR_GIC_VERSION_LAST - 1;
> +             version > VIR_GIC_VERSION_NONE;
> +             version--) {
> +            if (virQEMUCapsSupportsGICVersion(qemuCaps,
> +                                              def->virtType,
> +                                              version)) {
> +                VIR_DEBUG("Using GIC version %s",
> +                          virGICVersionTypeToString(version));
> +                def->gic_version = version;
> +                break;
> +            }
>          }
> -        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;
> +    } while (false);

Removing the " while (false);"

>  
>      /* 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;
> +
> +    return;

Unnecessary for void function can be removed anyway

ACK with the adjustments

John

>  }
>  
>  
> @@ -2056,7 +2081,7 @@ qemuDomainDefPostParse(virDomainDefPtr def,
>      if (qemuCanonicalizeMachine(def, qemuCaps) < 0)
>          goto cleanup;
>  
> -    qemuDomainDefEnableDefaultFeatures(def);
> +    qemuDomainDefEnableDefaultFeatures(def, qemuCaps);
>  
>      qemuDomainRecheckInternalPaths(def, cfg, parseFlags);
>  
>