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

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/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?