[libvirt] [PATCH 08/11] qemu: Fix GIC behavior for the default case

[John Ferlan]

On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
> When no GIC version is specified, we currently default to GIC v2;
> however, that's not a great default, since guests will fail to
> start if the hardware only supports GIC v3.
> 
> Change the behavior so that a sensible default is chosen instead.
> That basically means using the same algorithm whether the user
> didn't explicitly enable the GIC feature or they explicitly
> enabled it but didn't specify any GIC version.
> 
> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
> ---
>  src/qemu/qemu_domain.c                             | 25 +++++++++++-----------
>  .../qemuxml2argvdata/aarch64-gic-default-both.args |  2 +-
>  tests/qemuxml2argvdata/aarch64-gic-default-v3.args |  2 +-
>  .../aarch64-gic-default-both.xml                   |  2 +-
>  .../qemuxml2xmloutdata/aarch64-gic-default-v3.xml  |  2 +-
>  5 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 98cba8789..ee720d328 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2838,13 +2838,14 @@ static void
>  qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
>                                     virQEMUCapsPtr qemuCaps)
>  {
> -    virGICVersion version;
> -
> -    /* The virt machine type always uses GIC: if the relevant element
> +    /* The virt machine type always uses GIC: if the relevant information
>       * was not included in the domain XML, we need to choose a suitable
>       * GIC version ourselves */
> -    if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT &&
> -        qemuDomainIsVirt(def)) {
> +    if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT &&
> +         qemuDomainIsVirt(def)) ||
> +        (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&

And patch3 (e.g. verification) takes care of us if this is "on", but
!qemuDomainIsVirt(def) ends up being true... It's a tangled web.

> +         def->gic_version == VIR_GIC_VERSION_NONE)) {
> +        virGICVersion version;
>  
>          VIR_DEBUG("Looking for usable GIC version in domain capabilities");
>          for (version = VIR_GIC_VERSION_LAST - 1;
> @@ -2878,17 +2879,17 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
>              }
>          }
>  

FWIW: There's a hunk above here which notes something about bz
1414081... If one goes and reads that bz, one finds it's closed notabug.

In any case, there's a lot of verbiage there.. Can any of it be shaved?

What's being done seems reasonable, but if we can take the opportunity
to revisit the comment as well - that'd be good.

For what's here as long as the above comment doesn't cause you to have
an oh sh* moment...


Reviewed-by: John Ferlan <jferlan at redhat.com>

John

> +        /* Use the default GIC version (GICv2) as a last-ditch attempt
> +         * if no match could be found above */
> +        if (def->gic_version == VIR_GIC_VERSION_NONE) {
> +            VIR_DEBUG("Using GIC version 2 (default)");
> +            def->gic_version = VIR_GIC_VERSION_2;
> +        }
> +
>          /* 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 (GICv2) if no version was specified */
> -    if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
> -        def->gic_version == VIR_GIC_VERSION_NONE) {
> -        VIR_DEBUG("Using GIC version 2 (default)");
> -        def->gic_version = VIR_GIC_VERSION_2;
> -    }
>  }
>  
>  
[...]