conf: Don't mangle vcpu placement randomly

https://bugzilla.redhat.com/show_bug.cgi?id=1170492

In one of our previous commits (dc8b7ce7) we've done a functional
change even though it was intended as pure refactor. The problem is,
that the following XML:

 <vcpu placement='static' current='2'>6</vcpu>
 <cputune>
   <emulatorpin cpuset='1-3'/>
 </cputune>
 <numatune>
   <memory mode='strict' placement='auto'/>
 </numatune>

gets translated into this one:

 <vcpu placement='auto' current='2'>6</vcpu>
 <cputune>
   <emulatorpin cpuset='1-3'/>
 </cputune>
 <numatune>
   <memory mode='strict' placement='auto'/>
 </numatune>

We should not change the vcpu placement mode. Moreover, we're doing
something similar in case of emulatorpin and iothreadpin. If they were
set, but vcpu placement was auto, we've mistakenly removed them from
the domain XML even though we are able to set them independently on
vcpus.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>

docs/formatdomain.html.in

@@ -577,14 +577,12 @@
        <dt><code>emulatorpin</code></dt>
        <dd>
          The optional <code>emulatorpin</code> element specifies which of host
-         physical CPUs the "emulator", a subset of a domain not including vcpu,
+         physical CPUs the "emulator", a subset of a domain not including vcpu
-         will be pinned to. If this is omitted, and attribute
+         or iothreads will be pinned to. If this is omitted, and attribute
          <code>cpuset</code> of element <code>vcpu</code> is not specified,
          "emulator" is pinned to all the physical CPUs by default. It contains
          one required attribute <code>cpuset</code> specifying which physical
-         CPUs to pin to. NB, <code>emulatorpin</code> is not allowed if
+         CPUs to pin to.
-         attribute <code>placement</code> of element <code>vcpu</code> is
-         "auto".
        </dd>
        <dt><code>iothreadpin</code></dt>
        <dd>
@@ -598,8 +596,6 @@
          <code>iothread</code> value begins at "1" through the number of
           <a href="#elementsIOThreadsAllocation"><code>iothreads</code></a>
          allocated to the domain. A value of "0" is not permitted.
-         NB, <code>iothreadpin</code> is not allowed if attribute
-         <code>placement</code> of element <code>vcpu</code> is "auto".
         <span class="since">Since 1.2.9</span>
        </dd>
       <dt><code>shares</code></dt>

src/conf/domain_conf.c

@@ -13084,28 +13084,20 @@ virDomainDefParseXML(xmlDocPtr xml,
         goto error;
     }
 
-    /* Ignore emulatorpin if <vcpu> placement is "auto", they
-     * conflicts with each other, and <vcpu> placement can't be
-     * simply ignored, as <numatune>'s placement defaults to it.
-     */
     if (n) {
-        if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
+        if (n > 1) {
-            if (n > 1) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
-                virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("only one emulatorpin is supported"));
-                               _("only one emulatorpin is supported"));
+            VIR_FREE(nodes);
-                VIR_FREE(nodes);
+            goto error;
-                goto error;
-            }
-
-            def->cputune.emulatorpin = virDomainVcpuPinDefParseXML(nodes[0],
-                                                                   ctxt, 0,
-                                                                   true, false);
-
-            if (!def->cputune.emulatorpin)
-                goto error;
-        } else {
-            VIR_WARN("Ignore emulatorpin for <vcpu> placement is 'auto'");
         }
+
+        def->cputune.emulatorpin = virDomainVcpuPinDefParseXML(nodes[0],
+                                                               ctxt, 0,
+                                                               true, false);
+
+        if (!def->cputune.emulatorpin)
+            goto error;
     }
     VIR_FREE(nodes);
 
@@ -13116,38 +13108,28 @@ virDomainDefParseXML(xmlDocPtr xml,
         goto error;
     }
 
-    /* Ignore iothreadpin if <vcpu> placement is "auto", they
+    if (n && VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0)
-     * conflict with each other, and <vcpu> placement can't be
+        goto error;
-     * simply ignored, as <numatune>'s placement defaults to it.
-     */
-    if (n) {
-        if (def->placement_mode != VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO) {
-            if (VIR_ALLOC_N(def->cputune.iothreadspin, n) < 0)
-                goto error;
 
-            for (i = 0; i < n; i++) {
+    for (i = 0; i < n; i++) {
-                virDomainVcpuPinDefPtr iothreadpin = NULL;
+        virDomainVcpuPinDefPtr iothreadpin = NULL;
-                iothreadpin = virDomainVcpuPinDefParseXML(nodes[i], ctxt,
+        iothreadpin = virDomainVcpuPinDefParseXML(nodes[i], ctxt,
-                                                          def->iothreads,
+                                                  def->iothreads,
-                                                          false, true);
+                                                  false, true);
-                if (!iothreadpin)
+        if (!iothreadpin)
-                    goto error;
+            goto error;
 
-                if (virDomainVcpuPinIsDuplicate(def->cputune.iothreadspin,
+        if (virDomainVcpuPinIsDuplicate(def->cputune.iothreadspin,
-                                                def->cputune.niothreadspin,
+                                        def->cputune.niothreadspin,
-                                                iothreadpin->vcpuid)) {
+                                        iothreadpin->vcpuid)) {
-                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                                   _("duplicate iothreadpin for same iothread"));
+                           _("duplicate iothreadpin for same iothread"));
-                    virDomainVcpuPinDefFree(iothreadpin);
+            virDomainVcpuPinDefFree(iothreadpin);
-                    goto error;
+            goto error;
-                }
-
-                def->cputune.iothreadspin[def->cputune.niothreadspin++] =
-                    iothreadpin;
-            }
-        } else {
-            VIR_WARN("Ignore iothreadpin for <vcpu> placement is 'auto'");
         }
+
+        def->cputune.iothreadspin[def->cputune.niothreadspin++] =
+            iothreadpin;
     }
     VIR_FREE(nodes);
 
@@ -13185,7 +13167,9 @@ virDomainDefParseXML(xmlDocPtr xml,
                                   ctxt) < 0)
         goto error;
 
-    if (virDomainNumatuneHasPlacementAuto(def->numatune) && !def->cpumask)
+    if (virDomainNumatuneHasPlacementAuto(def->numatune) &&
+        !def->cpumask && !def->cputune.vcpupin &&
+        !def->cputune.emulatorpin && !def->cputune.iothreadspin)
         def->placement_mode = VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO;
 
     if ((n = virXPathNodeSet("./resource", ctxt, &nodes)) < 0) {

tests/qemuxml2argvdata/qemuxml2argv-cputune-numatune.xml

@@ -0,0 +1,37 @@
+<domain type='kvm'>
+  <name>dummy2</name>
+  <uuid>4d92ec27-9ebf-400b-ae91-20c71c647c19</uuid>
+  <memory unit='KiB'>131072</memory>
+  <currentMemory unit='KiB'>65536</currentMemory>
+  <vcpu placement='auto' current='2'>6</vcpu>
+  <iothreads>2</iothreads>
+  <cputune>
+    <emulatorpin cpuset='1-3'/>
+    <iothreadpin iothread='1' cpuset='2'/>
+  </cputune>
+  <numatune>
+    <memory mode='strict' placement='auto'/>
+  </numatune>
+  <os>
+    <type arch='x86_64' machine='pc-q35-2.3'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='sata' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pcie-root'/>
+    <controller type='pci' index='1' model='dmi-to-pci-bridge'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
+    </controller>
+    <controller type='pci' index='2' model='pci-bridge'>
+      <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/>
+    </controller>
+    <memballoon model='none'/>
+  </devices>
+</domain>

tests/qemuxml2argvdata/qemuxml2argv-vcpu-placement-static.xml

@@ -0,0 +1,37 @@
+<domain type='kvm'>
+  <name>dummy2</name>
+  <uuid>4d92ec27-9ebf-400b-ae91-20c71c647c19</uuid>
+  <memory unit='KiB'>131072</memory>
+  <currentMemory unit='KiB'>65536</currentMemory>
+  <vcpu placement='static' current='2'>6</vcpu>
+  <iothreads>2</iothreads>
+  <cputune>
+    <emulatorpin cpuset='1-3'/>
+    <iothreadpin iothread='1' cpuset='2'/>
+  </cputune>
+  <numatune>
+    <memory mode='strict' placement='auto'/>
+  </numatune>
+  <os>
+    <type arch='x86_64' machine='pc-q35-2.3'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-x86_64</emulator>
+    <controller type='sata' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
+    </controller>
+    <controller type='pci' index='0' model='pcie-root'/>
+    <controller type='pci' index='1' model='dmi-to-pci-bridge'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x1e' function='0x0'/>
+    </controller>
+    <controller type='pci' index='2' model='pci-bridge'>
+      <address type='pci' domain='0x0000' bus='0x01' slot='0x01' function='0x0'/>
+    </controller>
+    <memballoon model='none'/>
+  </devices>
+</domain>

tests/qemuxml2xmltest.c

@@ -310,6 +310,8 @@ mymain(void)
     DO_TEST("blkiotune-device");
     DO_TEST("cputune");
     DO_TEST("cputune-zero-shares");
+    DO_TEST("cputune-numatune");
+    DO_TEST("vcpu-placement-static");
 
     DO_TEST("smp");
     DO_TEST("iothreads");