11:51 a.m.
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
11:51 a.m.
New subject: [PATCH 1/3] qemu_domain.c: make qemuDomainGetMemoryModuleSizeAlignment() public
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(a)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
11:51 a.m.
New subject: [PATCH 2/3] qemu_hotplug.c: remove dimm auto-align on hotplug/unplug
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(a)redhat.com>
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1780506
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
11:51 a.m.
New subject: [PATCH 3/3] qemu_domain.c: remove qemuDomainMemoryDeviceAlignSize()
Previous patch removed all the callers of the function.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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
6:51 a.m.
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(-)
1752
days inactive
1758
days old
4 comments
1 participants
participants (1)
-
Daniel Henrique Barboza