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

[Michal Privoznik]

On 11.05.2016 00:42, Cole Robinson wrote:
> 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?

Agreed. This looks like too heavy hammer for nail this small.

Michal