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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
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;
> - }
> }
>
>
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
