Re: [libvirt] [PATCH 08/11] qemu: Fix GIC behavior for the default case
On Mon, 2018-02-12 at 16:05 -0500, John Ferlan wrote: > > 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. That's correct. > > + 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? Not a lot, I don't think... Maybe We'll want to revisit this once MSI support for GICv3 has been implemented in QEMU. but that's about it, everything else still applies. -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 08/11] qemu: Fix GIC behavior for the default case
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> --- > 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 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 08/11] qemu: Fix GIC behavior for the default case
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--- 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 && + 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, } } +/* 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; -} } diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-both.args b/tests/qemuxml2argvdata/aarch64-gic-default-both.args index 04ecd4ce7..6209eff4b 12 --- a/tests/qemuxml2argvdata/aarch64-gic-default-both.args +++ b/tests/qemuxml2argvdata/aarch64-gic-default-both.args @@ -1 +1 @@ -aarch64-gic-v2.args \ No newline at end of file +aarch64-gic-v3.args \ No newline at end of file diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v3.args b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args index 04ecd4ce7..6209eff4b 12 --- a/tests/qemuxml2argvdata/aarch64-gic-default-v3.args +++ b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args @@ -1 +1 @@ -aarch64-gic-v2.args \ No newline at end of file +aarch64-gic-v3.args \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml index ee470fb1f..bf9d58c38 12 --- a/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml +++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml @@ -1 +1 @@ -../qemuxml2argvdata/aarch64-gic-v2.xml \ No newline at end of file +../qemuxml2argvdata/aarch64-gic-v3.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml index ee470fb1f..bf9d58c38 12 --- a/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml +++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml @@ -1 +1 @@ -../qemuxml2argvdata/aarch64-gic-v2.xml \ No newline at end of file +../qemuxml2argvdata/aarch64-gic-v3.xml \ No newline at end of file -- 2.14.3 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list