[PATCH v2 5/6] qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE

Daniel Henrique Barboza posted 6 patches 2 months ago

[PATCH v2 5/6] qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE

Posted by Daniel Henrique Barboza 2 months ago
A previous patch removed the pSeries NVDIMM align that wasn't
being done properly. This patch reintroduces it in the right
fashion, making it reliant on VIR_DOMAIN_DEF_PARSE_ABI_UPDATE.
This makes it complying with the intended design defined by
commit c7d7ba85a624.

Since the PARSE_ABI_FLAG is more restrictive than checking for
!migrate && !snapshot, like is being currently done with
qemuDomainAlignMemorySizes(), this means that we'll align the
pSeries NVDIMMs in two places - in post parse time for new
guests, and in qemuDomainAlignMemorySizes() for all guests
that aren't migrating or in a snapshot.

Another difference is that the logic is now in the QEMU driver
instead of domain_conf.c. This was necessary because all
considerations made about the PARSE_ABI_UPDATE flag were done
under QEMU. Given that no other driver supports ppc64 there is no
impact in this change.

A new test was added to exercise what we're doing. It consists
of a a copy of the existing 'memory-hotplug-nvdimm-ppc64' xml2xml
test, called with the PARSE_ABI_UPDATE flag. As intended, we're
not changing QEMU command line or any XML without the flag,
while the pseries NVDIMM memory is being aligned when the
flag is used.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/qemu/qemu_domain.c                        | 33 +++++++++++-
 ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++
 ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++
 tests/qemuxml2xmltest.c                       |  7 +++
 4 files changed, 139 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
 create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2158080a56..b9eb54a11c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -5337,6 +5337,33 @@ qemuDomainTPMDefPostParse(virDomainTPMDefPtr tpm,
 }
 
 
+static int
+qemuDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, virArch arch,
+                             unsigned int parseFlags)
+{
+    /* Memory alignment can't be done for migration or snapshot
+     * scenarios. This logic was defined by commit c7d7ba85a624.
+     *
+     * There is no easy way to replicate at this point the same conditions
+     * used to call qemuDomainAlignMemorySizes(), which means checking if
+     * we're not migrating and not in a snapshot.
+     *
+     * We can use the PARSE_ABI_UPDATE flag, which is more strict -
+     * existing guests will not activate the flag to avoid breaking
+     * boot ABI. This means that any alignment done here will be replicated
+     * later on by qemuDomainAlignMemorySizes() to contemplate existing
+     * guests as well. */
+    if (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) {
+        if (ARCH_IS_PPC64(arch) &&
+            mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
+            virDomainNVDimmAlignSizePseries(mem) < 0)
+            return -1;
+    }
+
+    return 0;
+}
+
+
 static int
 qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
                              const virDomainDef *def,
@@ -5394,6 +5421,11 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
         ret = qemuDomainTPMDefPostParse(dev->data.tpm, def->os.arch);
         break;
 
+    case VIR_DOMAIN_DEVICE_MEMORY:
+        ret = qemuDomainMemoryDefPostParse(dev->data.memory, def->os.arch,
+                                           parseFlags);
+        break;
+
     case VIR_DOMAIN_DEVICE_LEASE:
     case VIR_DOMAIN_DEVICE_FS:
     case VIR_DOMAIN_DEVICE_INPUT:
@@ -5406,7 +5438,6 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev,
     case VIR_DOMAIN_DEVICE_MEMBALLOON:
     case VIR_DOMAIN_DEVICE_NVRAM:
     case VIR_DOMAIN_DEVICE_RNG:
-    case VIR_DOMAIN_DEVICE_MEMORY:
     case VIR_DOMAIN_DEVICE_IOMMU:
     case VIR_DOMAIN_DEVICE_AUDIO:
         ret = 0;
diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
new file mode 100644
index 0000000000..ae5a17d3c8
--- /dev/null
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
@@ -0,0 +1,50 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+  <memory unit='KiB'>1267710</memory>
+  <currentMemory unit='KiB'>1267710</currentMemory>
+  <vcpu placement='static' cpuset='0-1'>2</vcpu>
+  <os>
+    <type arch='ppc64' machine='pseries'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <cpu>
+    <topology sockets='2' dies='1' cores='1' threads='1'/>
+    <numa>
+      <cell id='0' cpus='0-1' memory='1048576' unit='KiB'/>
+    </numa>
+  </cpu>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-ppc64</emulator>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'>
+      <model name='spapr-pci-host-bridge'/>
+      <target index='0'/>
+    </controller>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </memballoon>
+    <panic model='pseries'/>
+    <memory model='nvdimm'>
+      <uuid>49545eb3-75e1-2d0a-acdd-f0294406c99e</uuid>
+      <source>
+        <path>/tmp/nvdimm</path>
+      </source>
+      <target>
+        <size unit='KiB'>550000</size>
+        <node>0</node>
+        <label>
+          <size unit='KiB'>128</size>
+        </label>
+      </target>
+      <address type='dimm' slot='0'/>
+    </memory>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
new file mode 100644
index 0000000000..24b0982a7b
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
@@ -0,0 +1,50 @@
+<domain type='qemu'>
+  <name>QEMUGuest1</name>
+  <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
+  <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
+  <memory unit='KiB'>1572992</memory>
+  <currentMemory unit='KiB'>1267710</currentMemory>
+  <vcpu placement='static' cpuset='0-1'>2</vcpu>
+  <os>
+    <type arch='ppc64' machine='pseries'>hvm</type>
+    <boot dev='hd'/>
+  </os>
+  <cpu>
+    <topology sockets='2' dies='1' cores='1' threads='1'/>
+    <numa>
+      <cell id='0' cpus='0-1' memory='1048576' unit='KiB'/>
+    </numa>
+  </cpu>
+  <clock offset='utc'/>
+  <on_poweroff>destroy</on_poweroff>
+  <on_reboot>restart</on_reboot>
+  <on_crash>destroy</on_crash>
+  <devices>
+    <emulator>/usr/bin/qemu-system-ppc64</emulator>
+    <controller type='usb' index='0'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
+    </controller>
+    <controller type='pci' index='0' model='pci-root'>
+      <model name='spapr-pci-host-bridge'/>
+      <target index='0'/>
+    </controller>
+    <memballoon model='virtio'>
+      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/>
+    </memballoon>
+    <panic model='pseries'/>
+    <memory model='nvdimm'>
+      <uuid>49545eb3-75e1-2d0a-acdd-f0294406c99e</uuid>
+      <source>
+        <path>/tmp/nvdimm</path>
+      </source>
+      <target>
+        <size unit='KiB'>524416</size>
+        <node>0</node>
+        <label>
+          <size unit='KiB'>128</size>
+        </label>
+      </target>
+      <address type='dimm' slot='0'/>
+    </memory>
+  </devices>
+</domain>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 603ba71686..595a897a70 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1240,6 +1240,13 @@ mymain(void)
                                               QEMU_CAPS_DEVICE_NVDIMM_UNARMED);
     DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
                                            QEMU_CAPS_DEVICE_NVDIMM);
+    DO_TEST_FULL("memory-hotplug-nvdimm-ppc64-abi-update", WHEN_BOTH,
+                 ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE,
+                 ARG_QEMU_CAPS,
+                 QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
+                 QEMU_CAPS_DEVICE_NVDIMM,
+                 QEMU_CAPS_LAST);
+
     DO_TEST("net-udp", NONE);
 
     DO_TEST("video-virtio-gpu-device", QEMU_CAPS_DEVICE_VIRTIO_GPU);
-- 
2.26.2

Re: [PATCH v2 5/6] qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE

Posted by Andrea Bolognani 1 month, 2 weeks ago
On Wed, 2020-11-18 at 16:58 -0300, Daniel Henrique Barboza wrote:
> A previous patch removed the pSeries NVDIMM align that wasn't
> being done properly. This patch reintroduces it in the right
> fashion, making it reliant on VIR_DOMAIN_DEF_PARSE_ABI_UPDATE.
> This makes it complying with the intended design defined by
> commit c7d7ba85a624.
> 
> Since the PARSE_ABI_FLAG is more restrictive than checking for

s/PARSE_ABI_FLAG/PARSE_ABI_UPDATE flag/

> +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml
> @@ -0,0 +1,50 @@
> +  <devices>
> +    <emulator>/usr/bin/qemu-system-ppc64</emulator>
> +    <controller type='usb' index='0'>
> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
> +    </controller>

This part could be simplified a bit. Please take a look at

  https://www.redhat.com/archives/libvir-list/2020-December/msg00244.html

which I'm hoping you'll agree to rebase your series on top of.

> +++ b/tests/qemuxml2xmltest.c
> @@ -1240,6 +1240,13 @@ mymain(void)
> +    DO_TEST_FULL("memory-hotplug-nvdimm-ppc64-abi-update", WHEN_BOTH,
> +                 ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE,
> +                 ARG_QEMU_CAPS,
> +                 QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE,
> +                 QEMU_CAPS_DEVICE_NVDIMM,
> +                 QEMU_CAPS_LAST);

Please add this new test to qemuxml2argv as well.

-- 
Andrea Bolognani / Red Hat / Virtualization