[libvirt] [PATCH v2 2/3] qemu: Add support for gic-version machine option

[Martin Kletzander]

On Wed, Sep 30, 2015 at 02:04:10PM +0300, Pavel Fedin wrote:
>Support for GICv3 has been recently introduced in qemu using gic-version
>option for the 'virt' machine. The option can actually take values of
>'2', '3' and 'host', however, since in libvirt this is a numeric
>parameter, we limit it only to 2 and 3. Value of 2 is not added to the
>command line in order to keep backward compatibility with older qemu
>versions.
>
>Signed-off-by: Pavel Fedin <p.fedin at samsung.com>
>---
> src/qemu/qemu_command.c | 43 ++++++++++++++++++++++++++++++-------------
> 1 file changed, 30 insertions(+), 13 deletions(-)
>
>diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>index bb1835c..a47e188 100644
>--- a/src/qemu/qemu_command.c
>+++ b/src/qemu/qemu_command.c
>@@ -7702,19 +7702,6 @@ qemuBuildCpuArgStr(virQEMUDriverPtr driver,
>         have_cpu = true;
>     }
>
>-    if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
>-        if (def->gic_version && def->gic_version != 2) {
>-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>-                           _("gic version '%u' is not supported"),
>-                           def->gic_version);
>-            goto cleanup;
>-        }
>-
>-        /* There's no command line argument currently to turn on/off GIC. It's
>-         * done automatically by qemu-system-aarch64. But if this changes, lets
>-         * put the code here. */
>-    }
>-
>     if (virBufferCheckError(&buf) < 0)
>         goto cleanup;
>
>@@ -7931,6 +7918,36 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
>             return -1;
>         }
>
>+	if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
>+	    if (def->gic_version) {
>+       	        if (!STREQ(def->os.machine, "virt") &&
>+       	            !STRPREFIX(def->os.machine, "virt-")) {
>+       	            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                                   _("gic-version option is available "
>+                                     "only for virt machine"));
>+                    virBufferFreeAndReset(&buf);
>+                    return -1;
>+       	        }

Indentation's off here.

Also before this patch we would allow def->gic_version == 2 for any
machine type.  I don't have a problem with this since GIC doesn't make
sense anywhere else then on ARM machines, but shouldn't we check for
the fact that the request is for ARM (in case, for example, if ppc64
gains some 'virt' machine type)?  Because we have no guarantee that
it's ARM just based on the machine type.

>+
>+	        /* 2 is the default, so we don't put it as option for
>+	         * backwards compatibility
>+	         */
>+                if (def->gic_version != 2) {
>+                    if (virQEMUCapsGet(qemuCaps,
>+                                       QEMU_CAPS_MACH_VIRT_GIC_VERSION)) {
>+                        virBufferAsprintf(&buf, ",gic-version=%d",
>+                                          def->gic_version);
>+                    } else {
>+                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>+                                       _("gic-version option is not available "
>+                                         "with this QEMU binary"));
>+                        virBufferFreeAndReset(&buf);
>+                        return -1;

I'd change this to:

if (gic != 2) {
    if (!caps)
        error;
    append_cmd();
}

If you're ok with that, just let me know and I'll push it with the
following diff squashed in, right after the release:

diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c
index a47e1883d976..72149bdd1eef 100644
--- i/src/qemu/qemu_command.c
+++ w/src/qemu/qemu_command.c
@@ -7918,35 +7918,36 @@ qemuBuildMachineArgStr(virCommandPtr cmd,
             return -1;
         }

-	if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
-	    if (def->gic_version) {
-       	        if (!STREQ(def->os.machine, "virt") &&
-       	            !STRPREFIX(def->os.machine, "virt-")) {
-       	            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+        if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
+            if (def->gic_version) {
+                if ((def->os.arch != VIR_ARCH_ARMV7L &&
+                     def->os.arch != VIR_ARCH_AARCH64) ||
+                    (STRNEQ(def->os.machine, "virt") &&
+                     !STRPREFIX(def->os.machine, "virt-"))) {
+                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                                    _("gic-version option is available "
                                      "only for virt machine"));
                     virBufferFreeAndReset(&buf);
                     return -1;
-       	        }
+                }

-	        /* 2 is the default, so we don't put it as option for
-	         * backwards compatibility
-	         */
+                /* 2 is the default, so we don't put it as option for
+                 * backwards compatibility
+                 */
                 if (def->gic_version != 2) {
-                    if (virQEMUCapsGet(qemuCaps,
-                                       QEMU_CAPS_MACH_VIRT_GIC_VERSION)) {
-                        virBufferAsprintf(&buf, ",gic-version=%d",
-                                          def->gic_version);
-                    } else {
+                    if (!virQEMUCapsGet(qemuCaps,
+                                        QEMU_CAPS_MACH_VIRT_GIC_VERSION)) {
                         virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                                        _("gic-version option is not available "
                                          "with this QEMU binary"));
                         virBufferFreeAndReset(&buf);
                         return -1;
                     }
+
+                    virBufferAsprintf(&buf, ",gic-version=%d", def->gic_version);
                 }
-	    }
-	}
+            }
+        }

         virCommandAddArgBuffer(cmd, &buf);
     }
--

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150930/e005b5a5/attachment-0001.sig>