qemuDomainAlignMemorySizes() has an operation order problem. We are calculating 'initialmem' without aligning the memory modules first. Since we're aligning the dimms afterwards this can create inconsistencies in the end result. x86 has alignment of 1-2MiB and it's not severely impacted by it, but pSeries works with 256MiB alignment and the difference is noticeable. This is the case of the existing 'memory-hotplug-ppc64-nonuma' test. The test consists of a 2GiB (aligned value) guest with 2 ~520MiB dimms, both unaligned. 'initialmem' is calculated by taking total_mem and subtracting the dimms size (via virDomainDefGetMemoryInitial()), which wil give us 2GiB - 520MiB - 520MiB, ending up with a little more than an 1GiB of 'initialmem'. Note that this value is now unaligned, and will be aligned up via VIR_ROUND_UP(), and we'll end up with 'initialmem' of 1GiB + 256MiB. Given that the dimms are aligned later on, the end result for QEMU is that the guest will have a 'mem' size of 1310720k, plus the two 512 MiB dimms, exceeding in 256MiB the desired 2GiB memory and currentMemory specified in the XML. This behavior was found when trying to push ppc64 memory alignment to postparse, as done in the next patch. In that case, the memory modules are aligned before entering qemuDomainAlignMemorySizes() and the 'memory-hotplug-ppc64-nonuma' test started to fail. Fortunately the fix is simple: align the memory modules before calculating 'initialmem' in qemuDomainAlignMemorySizes(). Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/qemu/qemu_domain.c | 42 ++++++++++--------- .../memory-hotplug-ppc64-nonuma.args | 2 +- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 375a9e5075..5a17887fa1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8083,25 +8083,9 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) virDomainNumaSetNodeMemorySize(def->numa, i, mem); } - /* align initial memory size, if NUMA is present calculate it as total of - * individual aligned NUMA node sizes */ - if (initialmem == 0) - initialmem = VIR_ROUND_UP(virDomainDefGetMemoryInitial(def), align); - - if (initialmem > maxmemcapped) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("initial memory size overflowed after alignment")); - return -1; - } - - def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align); - if (def->mem.max_memory > maxmemkb) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("maximum memory size overflowed after alignment")); - return -1; - } - - /* Align memory module sizes */ + /* Align memory module sizes. This needs to occur before 'initialmem' + * calculation because virDomainDefGetMemoryInitial() uses the size + * of the modules in the math. */ for (i = 0; i < def->nmems; i++) { if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && ARCH_IS_PPC64(def->os.arch)) { @@ -8122,6 +8106,26 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) } } + /* Align initial memory size, if NUMA is present calculate it as total of + * individual aligned NUMA node sizes. */ + if (initialmem == 0) { + align = qemuDomainGetMemorySizeAlignment(def); + initialmem = VIR_ROUND_UP(virDomainDefGetMemoryInitial(def), align); + } + + if (initialmem > maxmemcapped) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("initial memory size overflowed after alignment")); + return -1; + } + + def->mem.max_memory = VIR_ROUND_UP(def->mem.max_memory, align); + if (def->mem.max_memory > maxmemkb) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("maximum memory size overflowed after alignment")); + return -1; + } + virDomainDefSetMemoryTotal(def, initialmem + hotplugmem); return 0; diff --git a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args index 91cea9d8bf..78406f7f04 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args +++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args @@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=none \ -name QEMUGuest1 \ -S \ -machine pseries,accel=kvm,usb=off,dump-guest-core=off \ --m size=1310720k,slots=16,maxmem=4194304k \ +-m size=1048576k,slots=16,maxmem=4194304k \ -realtime mlock=off \ -smp 1,sockets=1,cores=1,threads=1 \ -object memory-backend-ram,id=memdimm0,size=536870912 \ -- 2.26.2

On 11/13/20 7:30 AM, Andrea Bolognani wrote: Good point. I failed to consider ABI stability for ppc64 guest migration. Yes, this will break older guests that happen to have extra memory. In fact, this can be specially harmful for migration. This means that I can't proceed with any other ppc64 changes made here. Aligning ppc64 DIMMs in PostParse will achieve the same results even without this patch - the DIMMs will be aligned before qemuDomainAlignMemorySizes() and initialmem will be rounded to a more precise value of 'currentMemory'. I can think of ways to align ppc64 DIMMs while not touching initialmem, but all of them will require extra state in the domain definition. The benefits are there (the DIMMs will be aligned in the live XML) but I'm not sure it's worth the extra code. Thanks for pointing this out. I'll evaluate if the x86 bits are still valuable and re-send them, since they're not messing with initialmem calculation of x86 guests. DHB

On 11/13/20 10:51 AM, Andrea Bolognani wrote: Interesting. I suppose that moving the logic of "qemuDomainAlignMemorySizes" to PostParse might allows us to align DIMMs while being able to not touch 'initialmem'. I'm not entirely sure if that code is dependent on runtime stuff though (the NUMA related code for instance). I'll investigate. DHB

Until now, we were aligning just ppc64 NVDIMM modules in post parse time. Let's extend the treatment to all ppc64 memory models. This is possible because the alignment restrictions for the architecture are imposed by the platform specification, being hypervisor-agnostic. This will remove the need for mem module alignment for pSeries in the QEMU driver, simplifying the code a bit. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/conf/domain_conf.c | 1 - src/qemu/qemu_domain.c | 13 +++---------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c98e760aa2..e4aecc9a4d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5360,7 +5360,6 @@ virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, * will have the same restriction. */ if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && virDomainMemoryDeviceAlignSizePseries(mem) < 0) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 5a17887fa1..33a5765c57 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8087,11 +8087,8 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) * calculation because virDomainDefGetMemoryInitial() uses the size * of the modules in the math. */ for (i = 0; i < def->nmems; i++) { - if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - ARCH_IS_PPC64(def->os.arch)) { - if (virDomainMemoryDeviceAlignSizePseries(def->mems[i]) < 0) - return -1; - } else { + /* ppc64 memory modules are aligned by virDomainMemoryDefPostParse(). */ + if (!ARCH_IS_PPC64(def->os.arch)) { align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]); def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align); } @@ -8144,13 +8141,9 @@ int qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, virDomainMemoryDefPtr mem) { - if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - ARCH_IS_PPC64(def->os.arch)) { - return virDomainMemoryDeviceAlignSizePseries(mem); - } else { + if (!ARCH_IS_PPC64(def->os.arch)) mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def)); - } return 0; } -- 2.26.2

This is already being done for pSeries guests, so let's extend the treatment for x86. The difference here is that the existing x86 alignment in the QEMU driver might be restricted only to QEMU, meaning that we can't put it in virDomainMemoryDefPostParse() without infering that all hypervisors would handle it the same way. Instead, let's create a qemuDomainMemoryDefPostParse() that will be called in qemuDomainDeviceDefPostParse() to do that. After this change, def->nmems will always be aligned after parsing time. qemuDomainAlignMemorySizes() doesn't need to align each memory module before calculating 'hotplugmem'. We'll also generated aligned values in the resulting XML as well. Some xml2xml tests were changed to reflect this new behavior. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/qemu/qemu_domain.c | 45 +++++++++++-------- tests/qemuxml2argvdata/hugepages-nvdimm.xml | 2 +- .../memory-hotplug-nvdimm-access.xml | 2 +- .../memory-hotplug-nvdimm-align.xml | 2 +- .../memory-hotplug-nvdimm-label.xml | 2 +- .../memory-hotplug-nvdimm-pmem.xml | 2 +- .../memory-hotplug-nvdimm-readonly.xml | 2 +- .../memory-hotplug-nvdimm.xml | 2 +- tests/qemuxml2argvdata/pages-dimm-discard.xml | 2 +- .../memory-hotplug-dimm.xml | 4 +- 10 files changed, 36 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e40c122311..c06c33f8d0 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5343,6 +5343,23 @@ qemuDomainTPMDefPostParse(virDomainTPMDefPtr tpm, } +static int +qemuDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, + virArch arch) +{ + /* For x86, dimm memory modules require 2MiB alignment rather than + * the 1MiB we are using elsewhere. */ + unsigned int x86MemoryModuleSizeAlignment = 2048; + + /* ppc64 memory module alignment is done in + * virDomainMemoryDefPostParse(). */ + if (!ARCH_IS_PPC64(arch)) + mem->size = VIR_ROUND_UP(mem->size, x86MemoryModuleSizeAlignment); + + return 0; +} + + static int qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5400,6 +5417,10 @@ 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); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -5412,7 +5433,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; @@ -8042,15 +8062,6 @@ qemuDomainGetMemorySizeAlignment(const virDomainDef *def) } -static unsigned long long -qemuDomainGetMemoryModuleSizeAlignment(void) -{ - /* For x86, dimm memory modules require 2MiB alignment rather than - * the 1MiB we are using elsewhere. */ - return 2048; -} - - int qemuDomainAlignMemorySizes(virDomainDefPtr def) { @@ -8077,16 +8088,12 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) virDomainNumaSetNodeMemorySize(def->numa, i, mem); } - /* Align memory module sizes. This needs to occur before 'initialmem' - * calculation because virDomainDefGetMemoryInitial() uses the size - * of the modules in the math. */ + /* Calculate hotplugmem. The memory modules are already aligned at this + * point: + * + * - ppc64 mem modules are being aligned by virDomainMemoryDefPostParse(); + * - x86 mem modules are being aligned by qemuDomainMemoryDefPostParse(). */ for (i = 0; i < def->nmems; i++) { - /* ppc64 memory modules are aligned by virDomainMemoryDefPostParse(). */ - if (!ARCH_IS_PPC64(def->os.arch)) { - align = qemuDomainGetMemoryModuleSizeAlignment(); - def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align); - } - hotplugmem += def->mems[i]->size; if (def->mems[i]->size > maxmemkb) { diff --git a/tests/qemuxml2argvdata/hugepages-nvdimm.xml b/tests/qemuxml2argvdata/hugepages-nvdimm.xml index 144d02b56e..d356ad12a8 100644 --- a/tests/qemuxml2argvdata/hugepages-nvdimm.xml +++ b/tests/qemuxml2argvdata/hugepages-nvdimm.xml @@ -39,7 +39,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> <node>0</node> </target> <address type='dimm' slot='0'/> diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml index a1cc1264eb..4ac5589076 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-access.xml @@ -48,7 +48,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> <node>0</node> </target> <address type='dimm' slot='0'/> diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml index 018a693aaf..2436b1a1af 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-align.xml @@ -49,7 +49,7 @@ <alignsize unit='KiB'>2048</alignsize> </source> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> <node>0</node> </target> <address type='dimm' slot='0'/> diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-label.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-label.xml index c9d54a6088..6e8902a25e 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-label.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-label.xml @@ -48,7 +48,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> <node>0</node> <label> <size unit='KiB'>128</size> diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml index 391d70f20e..a1a49f68ad 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-pmem.xml @@ -49,7 +49,7 @@ <pmem/> </source> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> <node>0</node> </target> <address type='dimm' slot='0'/> diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-readonly.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-readonly.xml index 09b2c5c833..32000acb6d 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-readonly.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-readonly.xml @@ -48,7 +48,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> <node>0</node> <readonly/> </target> diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm.xml index a32474da06..815fca3412 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm.xml @@ -48,7 +48,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> <node>0</node> </target> <address type='dimm' slot='0'/> diff --git a/tests/qemuxml2argvdata/pages-dimm-discard.xml b/tests/qemuxml2argvdata/pages-dimm-discard.xml index 3d233687e1..a0ba528823 100644 --- a/tests/qemuxml2argvdata/pages-dimm-discard.xml +++ b/tests/qemuxml2argvdata/pages-dimm-discard.xml @@ -40,7 +40,7 @@ </memory> <memory model='dimm' access='private' discard='yes'> <target> - <size unit='KiB'>524287</size> + <size unit='KiB'>524288</size> <node>0</node> </target> <address type='dimm' slot='1'/> diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-dimm.xml b/tests/qemuxml2xmloutdata/memory-hotplug-dimm.xml index 326b5c954c..985a4fc094 100644 --- a/tests/qemuxml2xmloutdata/memory-hotplug-dimm.xml +++ b/tests/qemuxml2xmloutdata/memory-hotplug-dimm.xml @@ -45,7 +45,7 @@ </memballoon> <memory model='dimm'> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> <node>0</node> </target> <address type='dimm' slot='0'/> @@ -56,7 +56,7 @@ <pagesize unit='KiB'>2048</pagesize> </source> <target> - <size unit='KiB'>524287</size> + <size unit='KiB'>524288</size> <node>0</node> </target> <address type='dimm' slot='1'/> -- 2.26.2

The alignment of the mem object in qemuDomainDetachPrepMemory() allows for the virDomainMemoryFindByDef() call to find the right memory object for detach. Turns out that, after recent changes, the alignment of the mem object is already granted at this point. This function is used by qemuDomainDetachDeviceLive(), which in turn is used in two places with a memory device: - qemuDomainDetachDeviceLiveAndConfig(), after a virDomainDeviceDefParse() calll that will align the mem object via PostParse callbacks; - qemuDomainDetachDeviceAliasLiveAndConfig(), after a call to virDomainDefFindDevice() that will get the already aligned mem object from the domain definition. This means that the qemuDomainMemoryDeviceAlignSize() call at the start of this function is now obsolete and can be removed. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/qemu/qemu_hotplug.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index c1461ac621..e0495ef59e 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -5682,9 +5682,9 @@ qemuDomainDetachPrepMemory(virDomainObjPtr vm, { int idx; - if (qemuDomainMemoryDeviceAlignSize(vm->def, match) < 0) - return -1; - + /* 'match' is granted to be aligned at this point, either + * by PostParse callbacks when parsing it or by fetching it + * from the domain definition. */ if ((idx = virDomainMemoryFindByDef(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, _("model '%s' memory device not present " -- 2.26.2

This function has no callers left after we moved the mem module alignment to PostParse callbacks. Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com&gt; --- src/qemu/qemu_domain.c | 20 -------------------- src/qemu/qemu_domain.h | 2 -- 2 files changed, 22 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7067d49d7a..8d69be682e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8130,26 +8130,6 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) } -/** - * qemuDomainMemoryDeviceAlignSize: - * @mem: memory device definition object - * - * Aligns the size of the memory module as qemu enforces it. The size is updated - * inplace. Default rounding is now to 1 MiB (qemu requires rounding to page, - * size so this should be safe). - */ -int -qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, - virDomainMemoryDefPtr mem) -{ - if (!ARCH_IS_PPC64(def->os.arch)) - mem->size = VIR_ROUND_UP(mem->size, - qemuDomainGetMemorySizeAlignment(def)); - - return 0; -} - - /** * qemuDomainGetMonitor: * @vm: domain object diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index ca041e207b..8e6fde412e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -757,8 +757,6 @@ bool qemuDomainHasBlockjob(virDomainObjPtr vm, bool copy_only) ATTRIBUTE_NONNULL(1); int qemuDomainAlignMemorySizes(virDomainDefPtr def); -int qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, - virDomainMemoryDefPtr mem); virDomainChrDefPtr qemuFindAgentConfig(virDomainDefPtr def); -- 2.26.2