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.

Commit d3f3c2c97f9b was predecessed by a move/rename of the former
qemuDomainNVDimmAlignSizePseries() function, commit ace5931553c8.
Reverting d3f3c2c97f9b will make the function rename obsolete since all
callers will now reside in QEMU drivers files, and it will remain that
way in the next patches, so let's revert ace5931553c8 as well.

This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88
(domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()).

This reverts commit ace5931553c87beebb6b3cd994061742b3f88238
(conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 src/conf/domain_conf.c                        | 59 +------------------
 src/conf/domain_conf.h                        |  3 -
 src/libvirt_private.syms                      |  1 -
 src/qemu/qemu_domain.c                        | 42 ++++++++++++-
 .../memory-hotplug-nvdimm-ppc64.xml           |  2 +-
 5 files changed, 42 insertions(+), 65 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 425e3c3710..d069a3ecc2 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5359,24 +5359,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,
@@ -5422,10 +5404,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:
@@ -5440,6 +5418,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;
@@ -16836,42 +16815,6 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode,
     return NULL;
 }
 
-int
-virDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem)
-{
-    /* For NVDIMMs in ppc64 in we want to align down the guest
-     * visible space, instead of align up, to avoid writing
-     * beyond the end of file by adding a potential 256MiB
-     * to the user specified size.
-     *
-     * The label-size is mandatory for ppc64 as well, meaning that
-     * the guest visible space will be target_size-label_size.
-     *
-     * Finally, target_size must include label_size.
-     *
-     * The above can be summed up as follows:
-     *
-     * target_size = AlignDown(target_size - label_size) + label_size
-     */
-    unsigned long long ppc64AlignSize =  256 * 1024;
-    unsigned long long guestArea = mem->size - mem->labelsize;
-
-    /* Align down guest_area. 256MiB is the minimum size. Error
-     * out if target_size is smaller than 256MiB + label_size,
-     * since aligning it up will cause QEMU errors. */
-    if (mem->size < (ppc64AlignSize + mem->labelsize)) {
-        virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("minimum target size for the NVDIMM "
-                         "must be 256MB plus the label size"));
-        return -1;
-    }
-
-    guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize;
-    mem->size = guestArea + mem->labelsize;
-
-    return 0;
-}
-
 static virDomainMemoryDefPtr
 virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt,
                            xmlNodePtr memdevNode,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 34cde22965..b1571d423c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3912,9 +3912,6 @@ bool
 virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a,
                               const virDomainBlockIoTuneInfo *b);
 
-int
-virDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem);
-
 bool
 virHostdevIsSCSIDevice(const virDomainHostdevDef *hostdev)
     ATTRIBUTE_NONNULL(1);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2f640ef1c4..5cf39bdf6c 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -546,7 +546,6 @@ virDomainNetTypeToString;
 virDomainNetUpdate;
 virDomainNostateReasonTypeFromString;
 virDomainNostateReasonTypeToString;
-virDomainNVDimmAlignSizePseries;
 virDomainObjAssignDef;
 virDomainObjBroadcast;
 virDomainObjCheckActive;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 663c0af867..af3c0e269a 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -8044,6 +8044,44 @@ qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def,
 }
 
 
+static int
+qemuDomainNVDimmAlignSizePseries(virDomainDefPtr def,
+                                 virDomainMemoryDefPtr mem)
+{
+    /* For NVDIMMs in ppc64 in we want to align down the guest
+     * visible space, instead of align up, to avoid writing
+     * beyond the end of file by adding a potential 256MiB
+     * to the user specified size.
+     *
+     * The label-size is mandatory for ppc64 as well, meaning that
+     * the guest visible space will be target_size-label_size.
+     *
+     * Finally, target_size must include label_size.
+     *
+     * The above can be summed up as follows:
+     *
+     * target_size = AlignDown(target_size - label_size) + label_size
+     */
+    unsigned long long ppc64AlignSize = qemuDomainGetMemorySizeAlignment(def);
+    unsigned long long guestArea = mem->size - mem->labelsize;
+
+    /* Align down guest_area. 256MiB is the minimum size. Error
+     * out if target_size is smaller than 256MiB + label_size,
+     * since aligning it up will cause QEMU errors. */
+    if (mem->size < (ppc64AlignSize + mem->labelsize)) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("minimum target size for the NVDIMM "
+                         "must be 256MB plus the label size"));
+        return -1;
+    }
+
+    guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize;
+    mem->size = guestArea + mem->labelsize;
+
+    return 0;
+}
+
+
 int
 qemuDomainAlignMemorySizes(virDomainDefPtr def)
 {
@@ -8092,7 +8130,7 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def)
     for (i = 0; i < def->nmems; i++) {
         if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
             ARCH_IS_PPC64(def->os.arch)) {
-            if (virDomainNVDimmAlignSizePseries(def->mems[i]) < 0)
+            if (qemuDomainNVDimmAlignSizePseries(def, def->mems[i]) < 0)
                 return -1;
         } else {
             align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]);
@@ -8129,7 +8167,7 @@ qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def,
 {
     if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM &&
         ARCH_IS_PPC64(def->os.arch)) {
-        return virDomainNVDimmAlignSizePseries(mem);
+        return qemuDomainNVDimmAlignSizePseries(def, mem);
     } else {
         mem->size = VIR_ROUND_UP(mem->size,
                                  qemuDomainGetMemorySizeAlignment(def));
diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
index 0c0b9f96fb..bf7df9a259 100644
--- a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
+++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml
@@ -34,7 +34,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

On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
> This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88
> (domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()).
> 
> This reverts commit ace5931553c87beebb6b3cd994061742b3f88238
> (conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).

Please revert the two commits separately.

-- 
Andrea Bolognani / Red Hat / Virtualization

On Fri, 2020-12-04 at 15:55 +0100, Andrea Bolognani wrote:
> On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
> > This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88
> > (domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()).
> > 
> > This reverts commit ace5931553c87beebb6b3cd994061742b3f88238
> > (conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).
> 
> Please revert the two commits separately.

Actually, looking at the rest of the series, I think it's better if
you do *not* go ahead with a straight revert, since that results in

  1) returning the function to its old place and signature;
  2) having to change the signature back;
  3) having to move the function because you need it to be usable
     earlier in the file.

That's a whole lot of churn for arguably very little benefit.

I suggest you instead do a pseudo-revert, where you rename the
function (without otherwise altering its signature) and move it to
the part of qemu_domain.c where you are ultimately going to need it;
in the commit message, you can still mention the commit that such a
change is a "spiritual revert" of, but this way we avoid muddying the
waters more than necessary.

Sorry for not realizing earlier that my initial suggestion would have
these consequences.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 12/4/20 12:13 PM, Andrea Bolognani wrote:
> On Fri, 2020-12-04 at 15:55 +0100, Andrea Bolognani wrote:
>> On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
>>> This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88
>>> (domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()).
>>>
>>> This reverts commit ace5931553c87beebb6b3cd994061742b3f88238
>>> (conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).
>>
>> Please revert the two commits separately.
> 
> Actually, looking at the rest of the series, I think it's better if
> you do *not* go ahead with a straight revert, since that results in
> 
>    1) returning the function to its old place and signature;
>    2) having to change the signature back;
>    3) having to move the function because you need it to be usable
>       earlier in the file.
> 
> That's a whole lot of churn for arguably very little benefit.
> 
> I suggest you instead do a pseudo-revert, where you rename the
> function (without otherwise altering its signature) and move it to
> the part of qemu_domain.c where you are ultimately going to need it;
> in the commit message, you can still mention the commit that such a
> change is a "spiritual revert" of, but this way we avoid muddying the
> waters more than necessary.


The change in signature was done to avoid using qemuDomainGetMemorySizeAlignment(def),
because that would be another function that would need to be moved up. I forgot to
mention that in patch 04.

IIUC what you're suggesting can be implemented as follows:

- go back to the first revert I was doing in v2 (remove the code from PostParse).
Single revert of d3f3c2c97f9b92;

- apply the other 2 patches;

- do an extra patch to rename/move the function that is now being used only
in QEMU files, but without a straight up revert of ace5931553c8. I can also
simplify the signature in this step.


The order here is intentional - the double revert in patch 3 done in this series
needed to be followed up by changes in patches 4 and 5 (given that the function was
renamed in patch 3). In this order, I can pick the patches from your local branch
directly and just bother with the renaming in a single follow-up patch.


Sounds good?



DHB


> 
> Sorry for not realizing earlier that my initial suggestion would have
> these consequences.
> 

On Fri, 2020-12-04 at 13:50 -0300, Daniel Henrique Barboza wrote:
> On 12/4/20 12:13 PM, Andrea Bolognani wrote:
> > I suggest you instead do a pseudo-revert, where you rename the
> > function (without otherwise altering its signature) and move it to
> > the part of qemu_domain.c where you are ultimately going to need it;
> > in the commit message, you can still mention the commit that such a
> > change is a "spiritual revert" of, but this way we avoid muddying the
> > waters more than necessary.
> 
> The change in signature was done to avoid using qemuDomainGetMemorySizeAlignment(def),
> because that would be another function that would need to be moved up. I forgot to
> mention that in patch 04.

Why would you want to avoid using it? It's exactly what that function
is intended to do. Moving it around is no big deal, and by using it
you can avoid open-coding the same value twice. See attached diff.

> IIUC what you're suggesting can be implemented as follows:
> 
> - go back to the first revert I was doing in v2 (remove the code from PostParse).
> Single revert of d3f3c2c97f9b92;
> 
> - apply the other 2 patches;
> 
> - do an extra patch to rename/move the function that is now being used only
> in QEMU files, but without a straight up revert of ace5931553c8. I can also
> simplify the signature in this step.

Yes, except of course the other two patches should still include the
changes that were implemented after my review, eg. they should be as
seen in

  https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign

and not how they showed up initially on the list.

> The order here is intentional - the double revert in patch 3 done in this series
> needed to be followed up by changes in patches 4 and 5 (given that the function was
> renamed in patch 3). In this order, I can pick the patches from your local branch
> directly and just bother with the renaming in a single follow-up patch.
> 
> 
> Sounds good?

Yeah, it sounds like a plan. You can consider all patches from the
branch linked above

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

and push them at your leisure.

-- 
Andrea Bolognani / Red Hat / Virtualization

On 12/4/20 3:15 PM, Andrea Bolognani wrote:
> On Fri, 2020-12-04 at 13:50 -0300, Daniel Henrique Barboza wrote:
>> On 12/4/20 12:13 PM, Andrea Bolognani wrote:
>>> I suggest you instead do a pseudo-revert, where you rename the
>>> function (without otherwise altering its signature) and move it to
>>> the part of qemu_domain.c where you are ultimately going to need it;
>>> in the commit message, you can still mention the commit that such a
>>> change is a "spiritual revert" of, but this way we avoid muddying the
>>> waters more than necessary.
>>
>> The change in signature was done to avoid using qemuDomainGetMemorySizeAlignment(def),
>> because that would be another function that would need to be moved up. I forgot to
>> mention that in patch 04.
> 
> Why would you want to avoid using it? It's exactly what that function
> is intended to do. Moving it around is no big deal, and by using it
> you can avoid open-coding the same value twice. See attached diff.

Fair enough.

> 
>> IIUC what you're suggesting can be implemented as follows:
>>
>> - go back to the first revert I was doing in v2 (remove the code from PostParse).
>> Single revert of d3f3c2c97f9b92;
>>
>> - apply the other 2 patches;
>>
>> - do an extra patch to rename/move the function that is now being used only
>> in QEMU files, but without a straight up revert of ace5931553c8. I can also
>> simplify the signature in this step.
> 
> Yes, except of course the other two patches should still include the
> changes that were implemented after my review, eg. they should be as
> seen in
> 
>    https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign
> 
> and not how they showed up initially on the list.

Yep, that's the idea.

> 
>> The order here is intentional - the double revert in patch 3 done in this series
>> needed to be followed up by changes in patches 4 and 5 (given that the function was
>> renamed in patch 3). In this order, I can pick the patches from your local branch
>> directly and just bother with the renaming in a single follow-up patch.
>>
>>
>> Sounds good?
> 
> Yeah, it sounds like a plan. You can consider all patches from the
> branch linked above
> 
>    Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> and push them at your leisure.


Alright! I'll do the adjustments and push with your R-b.


Thanks,


DHB

>