[PATCH v2 3/6] Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()"

Daniel Henrique Barboza posted 6 patches 2 months ago

[PATCH v2 3/6] Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()"

Posted by Daniel Henrique Barboza 2 months ago
The code to align ppc64 NVDIMMs on post parse was introduced in
commit d3f3c2c97f9b. That commit failed to realize that we
can't align memory unconditionally. As of commit c7d7ba85a624
("qemu: command: Align memory sizes only on fresh starts"),
all memory alignment should be executed only when we're not
migrating or in a snapshot.

This revert does not break any guests in the wild, given that
ppc64 NVDIMMs are still being aligned in qemuDomainAlignMemorySizes().

Next patch will introduce a mechanism where we can have post
parse NVDIMM alignment for pSeries without breaking the
intended design, as defined by c7d7ba85a624.

This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/conf/domain_conf.c                        | 23 +------------------
 .../memory-hotplug-nvdimm-ppc64.xml           |  2 +-
 2 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 498a8b6ef0..961f292e1f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5351,24 +5351,6 @@ virDomainVsockDefPostParse(virDomainVsockDefPtr vsock)
 }
 
 
-static int
-virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem,
-                            const virDomainDef *def)
-{
-    /* Although only the QEMU driver implements PPC64 support, this
-     * code is related to the platform specification (PAPR), i.e. it
-     * is hypervisor agnostic, and any future PPC64 hypervisor driver
-     * will have the same restriction.
-     */
-    if (ARCH_IS_PPC64(def->os.arch) &&
-        mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
-        virDomainNVDimmAlignSizePseries(mem) < 0)
-        return -1;
-
-    return 0;
-}
-
-
 static int
 virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
                                   const virDomainDef *def,
@@ -5414,10 +5396,6 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
         ret = 0;
         break;
 
-    case VIR_DOMAIN_DEVICE_MEMORY:
-        ret = virDomainMemoryDefPostParse(dev->data.memory, def);
-        break;
-
     case VIR_DOMAIN_DEVICE_LEASE:
     case VIR_DOMAIN_DEVICE_FS:
     case VIR_DOMAIN_DEVICE_INPUT:
@@ -5432,6 +5410,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
     case VIR_DOMAIN_DEVICE_SHMEM:
     case VIR_DOMAIN_DEVICE_TPM:
     case VIR_DOMAIN_DEVICE_PANIC:
+    case VIR_DOMAIN_DEVICE_MEMORY:
     case VIR_DOMAIN_DEVICE_IOMMU:
     case VIR_DOMAIN_DEVICE_AUDIO:
         ret = 0;
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
index ecb1b83b4a..ae5a17d3c8 100644
--- a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
@@ -38,7 +38,7 @@
         <path>/tmp/nvdimm</path>
       </source>
       <target>
-        <size unit='KiB'>524416</size>
+        <size unit='KiB'>550000</size>
         <node>0</node>
         <label>
           <size unit='KiB'>128</size>
-- 
2.26.2

Re: [PATCH v2 3/6] Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()"

Posted by Andrea Bolognani 1 month, 2 weeks ago
On Wed, 2020-11-18 at 16:58 -0300, Daniel Henrique Barboza wrote:
> The code to align ppc64 NVDIMMs on post parse was introduced in
> commit d3f3c2c97f9b. That commit failed to realize that we
> can't align memory unconditionally. As of commit c7d7ba85a624
> ("qemu: command: Align memory sizes only on fresh starts"),
> all memory alignment should be executed only when we're not
> migrating or in a snapshot.
> 
> This revert does not break any guests in the wild, given that
> ppc64 NVDIMMs are still being aligned in qemuDomainAlignMemorySizes().
> 
> Next patch will introduce a mechanism where we can have post
> parse NVDIMM alignment for pSeries without breaking the
> intended design, as defined by c7d7ba85a624.
> 
> This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88.
> 
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
>  src/conf/domain_conf.c                        | 23 +------------------
>  .../memory-hotplug-nvdimm-ppc64.xml           |  2 +-
>  2 files changed, 2 insertions(+), 23 deletions(-)

I think it would make sense to also revert

  commit ace5931553c87beebb6b3cd994061742b3f88238
  Author: Daniel Henrique Barboza <danielhb413@gmail.com>
  Date:   Mon Sep 14 22:02:47 2020 -0300

    conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c
    
    We'll use the auto-alignment function during parse time, in
    domain_conf.c. Let's move the function to that file, renaming
    it to virDomainNVDimmAlignSizePseries(). This will also make it
    clearer that, although QEMU is the only driver that currently
    supports it, pSeries NVDIMM restrictions aren't tied to QEMU.
    
    Reviewed-by: Andrea Bolognani <abologna@redhat.com>
    Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

at the same time: as of this commit, there are no longer any callers
for virDomainNVDimmAlignSizePseries() outside of the QEMU driver.

-- 
Andrea Bolognani / Red Hat / Virtualization