[PATCH 0/3] remove dimm auto-align on hotplug/unplug
This series fixes bug [1]. See patch 2 for details. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1780506 Daniel Henrique Barboza (3): qemu_domain.c: make qemuDomainGetMemoryModuleSizeAlignment() public qemu_hotplug.c: remove dimm auto-align on hotplug/unplug qemu_domain.c: remove qemuDomainMemoryDeviceAlignSize() src/qemu/qemu_domain.c | 23 +++-------------------- src/qemu/qemu_domain.h | 4 ++-- src/qemu/qemu_hotplug.c | 23 +++++++++++++++++++++-- 3 files changed, 26 insertions(+), 24 deletions(-) -- 2.24.1
This function will be used in qemu_hotplug.c in the next patch. While we're at it, we can remove the virDomainMemoryDef argument since the alignment size doesn't change with this object. The only existent caller can also be brought out of a loop which iterates in a virDomainMemoryDef list. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 7 +++---- src/qemu/qemu_domain.h | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3dfa71650d..27f400005e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12308,9 +12308,8 @@ qemuDomainGetMemorySizeAlignment(virDomainDefPtr def) } -static unsigned long long -qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def, - const virDomainMemoryDef *mem G_GNUC_UNUSED) +unsigned long long +qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def) { /* PPC requires the memory sizes to be rounded to 256MiB increments, so * round them to the size always. */ @@ -12368,8 +12367,8 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) } /* Align memory module sizes */ + align = qemuDomainGetMemoryModuleSizeAlignment(def); for (i = 0; i < def->nmems; i++) { - align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]); def->mems[i]->size = VIR_ROUND_UP(def->mems[i]->size, align); hotplugmem += def->mems[i]->size; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index f8fb48f2ff..d27d7e78d9 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -996,6 +996,8 @@ int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, virQEMUCapsPtr qemuCaps, const virDomainMemoryDef *mem); +unsigned long long qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def); + bool qemuDomainSupportsNewVcpuHotplug(virDomainObjPtr vm); bool qemuDomainHasVcpuPids(virDomainObjPtr vm); pid_t qemuDomainGetVcpuPid(virDomainObjPtr vm, unsigned int vcpuid); -- 2.24.1
ppc64 guests need the dimm to be aligned to 256MiB. The existing auto-align mechanic in qemuDomainAttachMemory(), via qemuDomainMemoryDeviceAlignSize(), is rounding up the dimms to the next 256MiB alignment. This leads to confusing situations in which the user intended to hotplug 300MiB, but see 512MiB memory being added, an extra 212MiB due to this rounding. The auto-align is also active for x86, but it is currently broken. qemuDomainMemoryDeviceAlignSize() is checking for the memory alignment (1MiB) instead of memory module alignment (2MiB). The end result is that QEMU ends up firing an error, warning the user of the 2MiB alignment requirement. Removing the auto-alignment of memory dimms in hotplug/unplug, warning the user of the dimm alignment error instead, allows ppc64 users to get a proper error message instead of seeing unintended extra MBs appearing in the guest. For x86, instead of fixing the auto-align and changing the current user expectation of seeing an error message on dimm misalign, while adding yet another x86 vs ppc64 difference in the code, remove auto-align for x86 as well. x86 users will benefit from a friendlier error message, and this time an intended one. This is the current QEMU error when an misaligned x86 dimm is hotplugged: $ sudo ./run virsh attach-device memtest mem_err.xml error: Failed to attach device from mem_err.xml error: internal error: unable to execute QEMU command 'device_add': backend memory size must be multiple of 0x200000 This is the new error message: $ sudo ./run virsh detach-device memtest mem_err.xml error: Failed to detach device from mem_err.xml error: unsupported configuration: dimm memory must be aligned with 2 MiB A similar error message is displayed in the ppc64 case. Reported-by: Dan Zheng <dzheng@redhat.com> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1780506 Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_hotplug.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index ca18bb9e5f..576eb37f0b 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2317,6 +2317,23 @@ qemuDomainAttachRNGDevice(virQEMUDriverPtr driver, } +static int +qemuDomainMemoryDimmIsAligned(const virDomainDef *def, + virDomainMemoryDefPtr mem) +{ + unsigned long long align = qemuDomainGetMemoryModuleSizeAlignment(def); + + if (mem->size % align != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("dimm memory must be aligned with %llu MiB"), + align / 1024); + return -1; + } + + return 0; +} + + /** * qemuDomainAttachMemory: * @driver: qemu driver data @@ -2348,7 +2365,8 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, int id; int ret = -1; - qemuDomainMemoryDeviceAlignSize(vm->def, mem); + if (qemuDomainMemoryDimmIsAligned(vm->def, mem) < 0) + goto cleanup; if (qemuDomainDefValidateMemoryHotplug(vm->def, priv->qemuCaps, mem) < 0) goto cleanup; @@ -5637,7 +5655,8 @@ qemuDomainDetachPrepMemory(virDomainObjPtr vm, virDomainMemoryDefPtr mem; int idx; - qemuDomainMemoryDeviceAlignSize(vm->def, match); + if (qemuDomainMemoryDimmIsAligned(vm->def, match) < 0) + return -1; if ((idx = virDomainMemoryFindByDef(vm->def, match)) < 0) { virReportError(VIR_ERR_DEVICE_MISSING, -- 2.24.1
Previous patch removed all the callers of the function. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 16 ---------------- src/qemu/qemu_domain.h | 2 -- 2 files changed, 18 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 27f400005e..1c22bba052 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -12386,22 +12386,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 rouding to page, - * size so this should be safe). - */ -void -qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, - virDomainMemoryDefPtr mem) -{ - mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def)); -} - - /** * qemuDomainGetMonitor: * @vm: domain object diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d27d7e78d9..efd65be779 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -956,8 +956,6 @@ bool qemuDomainHasBlockjob(virDomainObjPtr vm, bool copy_only) ATTRIBUTE_NONNULL(1); int qemuDomainAlignMemorySizes(virDomainDefPtr def); -void qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, - virDomainMemoryDefPtr mem); virDomainChrDefPtr qemuFindAgentConfig(virDomainDefPtr def); -- 2.24.1
Hi, After talking with the PowerPC folks about it, specially Shiva (CCed), I had a change of heart about the changes I'm proposing here. The reasons are: - DIMM align up for ppc64 guests can lead to funny situations like a 100MiB hotplug being rounded up to 256MiB, but in reality PowerPC users are hotplugging several gigabytes of RAM per DIMM and the align up of 256MiB is of no consequence. But removing the align up and erroring out, like I'm proposing here, will most likely annoy the existing users with technical details they are not used to and, arguably, shouldn't care. - This initiative was due to the findings of bug [1], which compared the results of a unaligned PowePC hotplug (512+1 MiB being succesfully hotplugged as 3*256MiB) versus a x86 scenario (256+1 MiB erroring out in QEMU). This is happening because QEMU was expecting a 2MiB alignment **in that scenario**. Shiva mentioned that x86 isn't standardized, with each operational system operating with different alignments, and there is no universal answer for dimm alignment like PowerPC has. That all said, I will ask you to disregard this series as a whole. I have no intentions to deteriorate PowerPC user experience with something that will do more harm than good for their actual usage. At the same time, although I boldly claimed that Libvirt was considering a wrong x86 alignment value, I am not so sure anymore after learning that x86 works with several memory block sizes. This is something that I'll leave for the x86 folks to decide if it's worth looking at or not. Thanks, DHB [1] https://bugzilla.redhat.com/show_bug.cgi?id=1780506 On 2/29/20 2:51 PM, Daniel Henrique Barboza wrote:
This series fixes bug [1]. See patch 2 for details.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1780506
Daniel Henrique Barboza (3): qemu_domain.c: make qemuDomainGetMemoryModuleSizeAlignment() public qemu_hotplug.c: remove dimm auto-align on hotplug/unplug qemu_domain.c: remove qemuDomainMemoryDeviceAlignSize()
src/qemu/qemu_domain.c | 23 +++-------------------- src/qemu/qemu_domain.h | 4 ++-- src/qemu/qemu_hotplug.c | 23 +++++++++++++++++++++-- 3 files changed, 26 insertions(+), 24 deletions(-)
participants (1)
-
Daniel Henrique Barboza