[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 05/12/2016 11:53 AM, Andrea Bolognani wrote:
> On Tue, 2016-05-10 at 18:42 -0400, Cole Robinson wrote:
>>>  
>>> +        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?
> 
> Nothing more to it :)
> 
> Do you mean I should make virQEMUCapsFillDomainFeatureGICCaps()
> public and use it here to fill only the part of the domain
> capabilities I'm actually going to use, or create a new function
> altogether?
> 
> Because right now I'm not seeing a way to do the latter without
> introducing some code duplication or making things quite a bit
> uglier... Maybe I'm just tired :)
> 

Can you break apart the logic like the attached patch, then call the new
function from the above code? I didn't try plugging it into your patches but
it looks to me like it should work

- Cole


diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 1bddf43..f05efd2 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -4256,6 +4256,32 @@ virQEMUCapsFillDomainDeviceHostdevCaps(virQEMUCapsPtr qemuCaps,
 }
 
 
+static bool
+virQEMUCapsGICSupported(virArch arch,
+                        virDomainVirtType virttype,
+                        const char *machine,
+                        virGICCapabilityPtr cap)
+{
+    if (arch != VIR_ARCH_ARMV7L &&
+        arch != VIR_ARCH_AARCH64)
+        return false;
+
+    if (STRNEQ(machine, "virt") &&
+        !STRPREFIX(machine, "virt-"))
+        return false;
+
+    if (virttype == VIR_DOMAIN_VIRT_KVM &&
+        !(cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL))
+        return false;
+
+    if (virttype == VIR_DOMAIN_VIRT_QEMU &&
+        !(cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED))
+        return false;
+
+    return true;
+}
+
+
 /**
  * virQEMUCapsFillDomainFeatureGICCaps:
  * @qemuCaps: QEMU capabilities
@@ -4282,23 +4308,12 @@ virQEMUCapsFillDomainFeatureGICCaps(virQEMUCapsPtr qemuCaps,
     virDomainCapsFeatureGICPtr gic = &domCaps->gic;
     size_t i;
 
-    if (domCaps->arch != VIR_ARCH_ARMV7L &&
-        domCaps->arch != VIR_ARCH_AARCH64)
-        return 0;
-
-    if (STRNEQ(domCaps->machine, "virt") &&
-        !STRPREFIX(domCaps->machine, "virt-"))
-        return 0;
-
     for (i = 0; i < qemuCaps->ngicCapabilities; i++) {
         virGICCapabilityPtr cap = &qemuCaps->gicCapabilities[i];
-
-        if (domCaps->virttype == VIR_DOMAIN_VIRT_KVM &&
-            !(cap->implementation & VIR_GIC_IMPLEMENTATION_KERNEL))
-            continue;
-
-        if (domCaps->virttype == VIR_DOMAIN_VIRT_QEMU &&
-            !(cap->implementation & VIR_GIC_IMPLEMENTATION_EMULATED))
+        if (!virQEMUCapsGICSupported(domCaps->arch,
+                                     domCaps->virttype,
+                                     domCaps->machine,
+                                     cap))
             continue;
 
         gic->supported = true;

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]