[PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE
Hi, This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work. Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed. Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done. Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed. Test cases were added to help me diagnose and assert what I was changing and what would remain untouched. [1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE src/conf/domain_conf.c | 23 +-------- src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c | 39 ++++++++++++++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 +++++++++++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 ++++++++++++ tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 +++++++++++++++++ tests/qemuxml2xmltest.c | 20 +++++++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml -- 2.26.2
qemuProcessCreatePretendCmdPrepare() is setting the VIR_QEMU_PROCESS_START_NEW regardless of whether this is a migration case or not. This behavior differs from what we're doing in qemuProcessStart(), where the flag is set only if !migrate && !snapshot. Fix it by making the flag setting consistent with what we're doing in qemuProcessStart(). Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 579b3c3713..3677da635c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7436,7 +7436,10 @@ qemuProcessCreatePretendCmdPrepare(virQEMUDriverPtr driver, VIR_QEMU_PROCESS_START_AUTODESTROY, -1); flags |= VIR_QEMU_PROCESS_START_PRETEND; - flags |= VIR_QEMU_PROCESS_START_NEW; + + if (!migrateURI) + flags |= VIR_QEMU_PROCESS_START_NEW; + if (standalone) flags |= VIR_QEMU_PROCESS_START_STANDALONE; -- 2.26.2
On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote:
qemuProcessCreatePretendCmdPrepare() is setting the VIR_QEMU_PROCESS_START_NEW regardless of whether this is a migration case or not. This behavior differs from what we're doing in qemuProcessStart(), where the flag is set only if !migrate && !snapshot.
Fix it by making the flag setting consistent with what we're doing in qemuProcessStart().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 579b3c3713..3677da635c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7436,7 +7436,10 @@ qemuProcessCreatePretendCmdPrepare(virQEMUDriverPtr driver, VIR_QEMU_PROCESS_START_AUTODESTROY, -1);
flags |= VIR_QEMU_PROCESS_START_PRETEND; - flags |= VIR_QEMU_PROCESS_START_NEW; + + if (!migrateURI) + flags |= VIR_QEMU_PROCESS_START_NEW; + if (standalone) flags |= VIR_QEMU_PROCESS_START_STANDALONE;
Le sigh, this lead me down the rabbit hole of VIR_QEMU_PROCESS_START_NEW flag and where it's used and what consequences this can have. Then I read the commit message properly and realized this is only for "pretend" case. And of course it's what we do for qemuProcessStart(). Lesson learned :-) Michal
On Wed, 2020-11-18 at 16:58 -0300, Daniel Henrique Barboza wrote:
qemuProcessCreatePretendCmdPrepare() is setting the VIR_QEMU_PROCESS_START_NEW regardless of whether this is a migration case or not. This behavior differs from what we're doing in qemuProcessStart(), where the flag is set only if !migrate && !snapshot.
Fix it by making the flag setting consistent with what we're doing in qemuProcessStart().
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_process.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
qemuBuildCommandLine() is calling qemuDomainAlignMemorySizes(), which is an operation that changes live XML and domain and has little to do with the command line build process. Move it to qemuProcessPrepareDomain() where we're supposed to make live XML and domain changes before launch. qemuProcessStart() is setting VIR_QEMU_PROCESS_START_NEW if !migrate && !snapshot, same conditions used in qemuBuildCommandLine() to call qemuDomainAlignMemorySizes(), making this change seamless. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_command.c | 3 --- src/qemu/qemu_process.c | 6 ++++++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 34b5746c1a..2bcdb28244 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9792,9 +9792,6 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, qemuBuildDomainLoaderCommandLine(cmd, def, qemuCaps); - if (!migrateURI && !snapshot && qemuDomainAlignMemorySizes(def) < 0) - return NULL; - if (qemuBuildMemCommandLine(cmd, def, qemuCaps, priv) < 0) return NULL; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 3677da635c..39c3edf4b9 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -6493,6 +6493,12 @@ qemuProcessPrepareDomain(virQEMUDriverPtr driver, if (qemuExtDevicesPrepareDomain(driver, vm) < 0) return -1; + if (flags & VIR_QEMU_PROCESS_START_NEW) { + VIR_DEBUG("Aligning guest memory"); + if (qemuDomainAlignMemorySizes(vm->def) < 0) + return -1; + } + for (i = 0; i < vm->def->nchannels; i++) { if (qemuDomainPrepareChannel(vm->def->channels[i], priv->channelTargetDir) < 0) -- 2.26.2
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
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
At this moment, it is not possible to create a test specifying ARG_PARSEFLAGS because info->parseFlags is not being forwarded to testCompareDomXML2XMLFiles(). Let's fix it now so next patch can make use of it. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- tests/qemuxml2xmltest.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index c006719dee..603ba71686 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -32,7 +32,8 @@ testXML2XMLActive(const void *opaque) const struct testQemuInfo *info = opaque; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, - info->infile, info->outfile, true, 0, + info->infile, info->outfile, true, + info->parseFlags, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); } @@ -43,7 +44,8 @@ testXML2XMLInactive(const void *opaque) const struct testQemuInfo *info = opaque; return testCompareDomXML2XMLFiles(driver.caps, driver.xmlopt, - info->infile, info->outfile, false, 0, + info->infile, info->outfile, false, + info->parseFlags, TEST_COMPARE_DOM_XML2XML_RESULT_SUCCESS); } -- 2.26.2
On Wed, 2020-11-18 at 16:58 -0300, Daniel Henrique Barboza wrote:
At this moment, it is not possible to create a test specifying ARG_PARSEFLAGS because info->parseFlags is not being forwarded to testCompareDomXML2XMLFiles(). Let's fix it now so next patch can make use of it.
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- tests/qemuxml2xmltest.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
A previous patch removed the pSeries NVDIMM align that wasn't being done properly. This patch reintroduces it in the right fashion, making it reliant on VIR_DOMAIN_DEF_PARSE_ABI_UPDATE. This makes it complying with the intended design defined by commit c7d7ba85a624. Since the PARSE_ABI_FLAG is more restrictive than checking for !migrate && !snapshot, like is being currently done with qemuDomainAlignMemorySizes(), this means that we'll align the pSeries NVDIMMs in two places - in post parse time for new guests, and in qemuDomainAlignMemorySizes() for all guests that aren't migrating or in a snapshot. Another difference is that the logic is now in the QEMU driver instead of domain_conf.c. This was necessary because all considerations made about the PARSE_ABI_UPDATE flag were done under QEMU. Given that no other driver supports ppc64 there is no impact in this change. A new test was added to exercise what we're doing. It consists of a a copy of the existing 'memory-hotplug-nvdimm-ppc64' xml2xml test, called with the PARSE_ABI_UPDATE flag. As intended, we're not changing QEMU command line or any XML without the flag, while the pseries NVDIMM memory is being aligned when the flag is used. Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 33 +++++++++++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ tests/qemuxml2xmltest.c | 7 +++ 4 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2158080a56..b9eb54a11c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5337,6 +5337,33 @@ qemuDomainTPMDefPostParse(virDomainTPMDefPtr tpm, } +static int +qemuDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, virArch arch, + unsigned int parseFlags) +{ + /* Memory alignment can't be done for migration or snapshot + * scenarios. This logic was defined by commit c7d7ba85a624. + * + * There is no easy way to replicate at this point the same conditions + * used to call qemuDomainAlignMemorySizes(), which means checking if + * we're not migrating and not in a snapshot. + * + * We can use the PARSE_ABI_UPDATE flag, which is more strict - + * existing guests will not activate the flag to avoid breaking + * boot ABI. This means that any alignment done here will be replicated + * later on by qemuDomainAlignMemorySizes() to contemplate existing + * guests as well. */ + if (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) { + if (ARCH_IS_PPC64(arch) && + mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && + virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5394,6 +5421,11 @@ 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, + parseFlags); + break; + case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -5406,7 +5438,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; diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml new file mode 100644 index 0000000000..ae5a17d3c8 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml @@ -0,0 +1,50 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> + <memory unit='KiB'>1267710</memory> + <currentMemory unit='KiB'>1267710</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='2' dies='1' cores='1' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + <panic model='pseries'/> + <memory model='nvdimm'> + <uuid>49545eb3-75e1-2d0a-acdd-f0294406c99e</uuid> + <source> + <path>/tmp/nvdimm</path> + </source> + <target> + <size unit='KiB'>550000</size> + <node>0</node> + <label> + <size unit='KiB'>128</size> + </label> + </target> + <address type='dimm' slot='0'/> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml new file mode 100644 index 0000000000..24b0982a7b --- /dev/null +++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml @@ -0,0 +1,50 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> + <memory unit='KiB'>1572992</memory> + <currentMemory unit='KiB'>1267710</currentMemory> + <vcpu placement='static' cpuset='0-1'>2</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <topology sockets='2' dies='1' cores='1' threads='1'/> + <numa> + <cell id='0' cpus='0-1' memory='1048576' unit='KiB'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + <panic model='pseries'/> + <memory model='nvdimm'> + <uuid>49545eb3-75e1-2d0a-acdd-f0294406c99e</uuid> + <source> + <path>/tmp/nvdimm</path> + </source> + <target> + <size unit='KiB'>524416</size> + <node>0</node> + <label> + <size unit='KiB'>128</size> + </label> + </target> + <address type='dimm' slot='0'/> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 603ba71686..595a897a70 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1240,6 +1240,13 @@ mymain(void) QEMU_CAPS_DEVICE_NVDIMM_UNARMED); DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_DEVICE_NVDIMM); + DO_TEST_FULL("memory-hotplug-nvdimm-ppc64-abi-update", WHEN_BOTH, + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + ARG_QEMU_CAPS, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_DEVICE_NVDIMM, + QEMU_CAPS_LAST); + DO_TEST("net-udp", NONE); DO_TEST("video-virtio-gpu-device", QEMU_CAPS_DEVICE_VIRTIO_GPU); -- 2.26.2
On Wed, 2020-11-18 at 16:58 -0300, Daniel Henrique Barboza wrote:
A previous patch removed the pSeries NVDIMM align that wasn't being done properly. This patch reintroduces it in the right fashion, making it reliant on VIR_DOMAIN_DEF_PARSE_ABI_UPDATE. This makes it complying with the intended design defined by commit c7d7ba85a624.
Since the PARSE_ABI_FLAG is more restrictive than checking for
s/PARSE_ABI_FLAG/PARSE_ABI_UPDATE flag/
+++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml @@ -0,0 +1,50 @@ + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller>
This part could be simplified a bit. Please take a look at https://www.redhat.com/archives/libvir-list/2020-December/msg00244.html which I'm hoping you'll agree to rebase your series on top of.
+++ b/tests/qemuxml2xmltest.c @@ -1240,6 +1240,13 @@ mymain(void) + DO_TEST_FULL("memory-hotplug-nvdimm-ppc64-abi-update", WHEN_BOTH, + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + ARG_QEMU_CAPS, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_DEVICE_NVDIMM, + QEMU_CAPS_LAST);
Please add this new test to qemuxml2argv as well. -- Andrea Bolognani / Red Hat / Virtualization
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. Existing guests can't be fixed without breaking ABI, but we have code already in place to align pSeries NVDIMM modules for new guests. Let's extend it to align all pSeries mem modules. A new test, 'memory-hotplug-ppc64-nonuma-abi-update', a copy of the existing 'memory-hotplug-ppc64-nonuma', was added to demonstrate the result for new pSeries guests. For the same unaligned XML mentioned above, after applying this patch: - starting QEMU mem size without PARSE_ABI_UPDATE: -m size=1310720k,slots=16,maxmem=4194304k \ (no changes) - starting QEMU mem size with PARSE_ABI_UPDATE: -m size=1048576k,slots=16,maxmem=4194304k \ (size fixed) Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 14 ++++-- ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 ++++++++++++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 +++++++++++++ tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 +++++++++++++++++++ tests/qemuxml2xmltest.c | 7 +++ 6 files changed, 135 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index b9eb54a11c..a16ec9ac58 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5354,10 +5354,16 @@ qemuDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, virArch arch, * later on by qemuDomainAlignMemorySizes() to contemplate existing * guests as well. */ if (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE) { - if (ARCH_IS_PPC64(arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + if (ARCH_IS_PPC64(arch)) { + unsigned long long ppc64MemModuleAlign = 256 * 1024; + + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (virDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + } else { + mem->size = VIR_ROUND_UP(mem->size, ppc64MemModuleAlign); + } + } } return 0; diff --git a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args new file mode 100644 index 0000000000..78406f7f04 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args @@ -0,0 +1,34 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/tmp/lib/domain--1-QEMUGuest1 \ +USER=test \ +LOGNAME=test \ +XDG_DATA_HOME=/tmp/lib/domain--1-QEMUGuest1/.local/share \ +XDG_CACHE_HOME=/tmp/lib/domain--1-QEMUGuest1/.cache \ +XDG_CONFIG_HOME=/tmp/lib/domain--1-QEMUGuest1/.config \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-ppc64 \ +-name QEMUGuest1 \ +-S \ +-machine pseries,accel=kvm,usb=off,dump-guest-core=off \ +-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 \ +-device pc-dimm,memdev=memdimm0,id=dimm0,slot=0 \ +-object memory-backend-ram,id=memdimm1,size=536870912 \ +-device pc-dimm,memdev=memdimm1,id=dimm1,slot=1 \ +-uuid 49545eb3-75e1-2d0a-acdd-f0294406c99e \ +-display none \ +-no-user-config \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=control \ +-rtc base=utc \ +-no-shutdown \ +-kernel /media/ram/uImage \ +-initrd /media/ram/ramdisk \ +-append 'root=/dev/ram rw console=ttyS0,115200' \ +-usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 diff --git a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml new file mode 100644 index 0000000000..7c68cd6aa2 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml @@ -0,0 +1,32 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>49545eb3-75e1-2d0a-acdd-f0294406c99e</uuid> + <maxMemory slots='16' unit='KiB'>4194304</maxMemory> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <kernel>/media/ram/uImage</kernel> + <initrd>/media/ram/ramdisk</initrd> + <cmdline>root=/dev/ram rw console=ttyS0,115200</cmdline> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <memballoon model='virtio'/> + <memory model='dimm'> + <target> + <size unit='KiB'>523264</size> + </target> + </memory> + <memory model='dimm'> + <target> + <size unit='KiB'>524287</size> + </target> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 174294c0f1..dabf00a53a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3005,6 +3005,13 @@ mymain(void) DO_TEST("memory-hotplug-ppc64-nonuma", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE); + DO_TEST_FULL("memory-hotplug-ppc64-nonuma-abi-update", + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + ARG_QEMU_CAPS, + QEMU_CAPS_KVM, QEMU_CAPS_DEVICE_PC_DIMM, + QEMU_CAPS_NUMA, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_OBJECT_MEMORY_RAM, + QEMU_CAPS_OBJECT_MEMORY_FILE); DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm"); DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-access"); DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-label"); diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml b/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml new file mode 100644 index 0000000000..832c4cea27 --- /dev/null +++ b/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml @@ -0,0 +1,45 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>49545eb3-75e1-2d0a-acdd-f0294406c99e</uuid> + <maxMemory slots='16' unit='KiB'>4194304</maxMemory> + <memory unit='KiB'>2097152</memory> + <currentMemory unit='KiB'>2097152</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <kernel>/media/ram/uImage</kernel> + <initrd>/media/ram/ramdisk</initrd> + <cmdline>root=/dev/ram rw console=ttyS0,115200</cmdline> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x0'/> + </memballoon> + <panic model='pseries'/> + <memory model='dimm'> + <target> + <size unit='KiB'>524288</size> + </target> + <address type='dimm' slot='0'/> + </memory> + <memory model='dimm'> + <target> + <size unit='KiB'>524288</size> + </target> + <address type='dimm' slot='1'/> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 595a897a70..f20616fdac 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1246,6 +1246,13 @@ mymain(void) QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_DEVICE_NVDIMM, QEMU_CAPS_LAST); + DO_TEST_FULL("memory-hotplug-ppc64-nonuma-abi-update", WHEN_BOTH, + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + ARG_QEMU_CAPS, + QEMU_CAPS_KVM, QEMU_CAPS_DEVICE_PC_DIMM, + QEMU_CAPS_NUMA, QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_OBJECT_MEMORY_RAM, + QEMU_CAPS_OBJECT_MEMORY_FILE, QEMU_CAPS_LAST); DO_TEST("net-udp", NONE); -- 2.26.2
On Wed, 2020-11-18 at 16:58 -0300, Daniel Henrique Barboza wrote:
+++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml @@ -0,0 +1,32 @@ + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <kernel>/media/ram/uImage</kernel> + <initrd>/media/ram/ramdisk</initrd> + <cmdline>root=/dev/ram rw console=ttyS0,115200</cmdline> + </os>
This can also be simplified. Once again, please look at https://www.redhat.com/archives/libvir-list/2020-December/msg00244.html -- Andrea Bolognani / Red Hat / Virtualization
Ping On 11/18/20 4:58 PM, Daniel Henrique Barboza wrote:
Hi,
This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work.
Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed.
Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done.
Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed.
Test cases were added to help me diagnose and assert what I was changing and what would remain untouched.
[1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html
Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE
src/conf/domain_conf.c | 23 +-------- src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c | 39 ++++++++++++++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 +++++++++++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 ++++++++++++ tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 +++++++++++++++++ tests/qemuxml2xmltest.c | 20 +++++++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml
On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote:
Hi,
This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work.
Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed.
Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done.
Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed.
Test cases were added to help me diagnose and assert what I was changing and what would remain untouched.
[1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html
Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE
src/conf/domain_conf.c | 23 +-------- src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c | 39 ++++++++++++++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 +++++++++++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 ++++++++++++ tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 +++++++++++++++++ tests/qemuxml2xmltest.c | 20 +++++++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On 12/2/20 4:17 AM, Michal Privoznik wrote:
On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote:
Hi,
This is a follow up of [1] after really comprehending what I was messing with and what I couldn't do. This version has a shorter scope, only pSeries guests are being taken care of. I can send a follow up to handle x86 as well depending on the popularity of this work.
Patches 1 and 2 moves the existing qemuDomainAlignMemorySizes() from qemu_command.c to qemuProcessPrepareDomain(). They are not related/tied to everything else done here and can be pushed independently if needed.
Patches 3, 4 and 5 are reworking the existing code to be consistent to our prerrogative of not aligning memory when migrating or 'snapshotting'. No significant behavioral change is done.
Patch 6 is where the bacon is. For new ppc64 guests, without ABI breakage, the mem alignment that are overshooting the intended initial memory is fixed.
Test cases were added to help me diagnose and assert what I was changing and what would remain untouched.
[1] https://www.redhat.com/archives/libvir-list/2020-November/msg00544.html
Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE
src/conf/domain_conf.c | 23 +-------- src/qemu/qemu_command.c | 3 -- src/qemu/qemu_domain.c | 39 ++++++++++++++- src/qemu/qemu_process.c | 11 +++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ ...emory-hotplug-ppc64-nonuma-abi-update.args | 34 +++++++++++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 32 ++++++++++++ tests/qemuxml2argvtest.c | 7 +++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 50 +++++++++++++++++++ .../memory-hotplug-nvdimm-ppc64.xml | 2 +- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 45 +++++++++++++++++ tests/qemuxml2xmltest.c | 20 +++++++- 12 files changed, 286 insertions(+), 30 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Thanks for the review! Andrea/Peter, do you want to take a look again to see if there's something that I missed? DHB
Michal
On Wed, 2020-12-02 at 09:14 -0300, Daniel Henrique Barboza wrote:
On 12/2/20 4:17 AM, Michal Privoznik wrote:
On 11/18/20 8:58 PM, Daniel Henrique Barboza wrote:
Daniel Henrique Barboza (6): qemu_process.c: check migrateURI when setting VIR_QEMU_PROCESS_START_NEW qemu: move memory size align to qemuProcessPrepareDomain() Revert "domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()" qemuxml2xmltest.c: honor ARG_PARSEFLAGS qemu_domain.c: post parse pSeries NVDIMM align with PARSE_ABI_UPDATE qemu_domain.c: align all pSeries mem modules when PARSE_ABI_UPDATE
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Thanks for the review!
Andrea/Peter, do you want to take a look again to see if there's something that I missed?
Yeah, sorry for not being very responsive, and thanks a lot Michal for picking up the slack! I'll try to give it at least a quick look by the end of the day. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2020-12-02 at 15:08 +0100, Andrea Bolognani wrote:
On Wed, 2020-12-02 at 09:14 -0300, Daniel Henrique Barboza wrote:
Andrea/Peter, do you want to take a look again to see if there's something that I missed?
Yeah, sorry for not being very responsive, and thanks a lot Michal for picking up the slack! I'll try to give it at least a quick look by the end of the day.
Sorry I didn't managed to get back to you yesterday but, given that we managed to get this at least partially wrong in every previous iteration, you'll hopefully forgive me for being perhaps a bit overcautious O:-) As mentioned elsewhere, in the process of trying to convince myself that these changes are in fact correct I found it useful be able to make a direct comparison between the ABI_UPDATE case and the vanilla one, and to facilitate that I've produced a couple of simple patches[1] that I'm hoping you'll agree to rebase your work on top of. I have actually already done that locally, so feel free to simply pick up the corresponding branch[2]. Anyway, assuming you're working from that branch, here are the differences that are introduced by using ABI_UPDATE: $ diff -Naru tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml --- tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml 2020-12-03 14:19:21.486145577 +0100 +++ tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml 2020-12-03 14:57:09.857621727 +0100 @@ -24,13 +24,13 @@ <panic model='pseries'/> <memory model='dimm'> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> </target> <address type='dimm' slot='0'/> </memory> <memory model='dimm'> <target> - <size unit='KiB'>524287</size> + <size unit='KiB'>524288</size> </target> <address type='dimm' slot='1'/> </memory> $ diff -Naru tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args --- tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args 2020-12-03 14:19:21.486145577 +0100 +++ tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args 2020-12-03 14:57:09.857621727 +0100 @@ -11,7 +11,7 @@ -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 \ $ You explain very well the command line change in the commit message for patch 6/6, and the output XML now reflects the aligned size for DIMMs that was used on the command line even before your changes, so this all looks good. $ diff -Naru tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml --- tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml 2020-12-03 14:57:09.857621727 +0100 +++ tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml 2020-12-03 14:57:09.857621727 +0100 @@ -2,7 +2,7 @@ <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> - <memory unit='KiB'>1267710</memory> + <memory unit='KiB'>1572992</memory> <currentMemory unit='KiB'>1267710</currentMemory> <vcpu placement='static' cpuset='0-1'>2</vcpu> <os> @@ -34,7 +34,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>550000</size> + <size unit='KiB'>524416</size> <node>0</node> <label> <size unit='KiB'>128</size> $ diff -Naru tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.args $ This is where I'm a bit confused. IIUC the new value for <memory>, 1572992 KiB, is exactly 1 GiB (initial NUMA memory) + 512 MiB (NVDIMM guest area size) + 128 KiB (NVDIMM label size). Is that the value we expect users to see in the XML? If the label size were not there I would certainly say yes, but those extra 128 KiB make me pause. Then again, the <target><size> for the <memory type='nvdimm'> element also includes the label size, so perhaps it's all good? I just want to make sure it is intentional :) The last bit of confusion is given by the fact that the <currentMemory> element is not updated along with the <memory> element. How will that work? Do I understand correctly that the guest will actually get the full <memory> size, but if a memory balloon is also present then the difference between <memory> and <currentMemory> will be (temporarily) returned to the host using that mechanism? Sorry again for all the questions and for delaying your work. I'm just trying to make sure we don't have to come back to it again in a couple of months O:-) [1] https://www.redhat.com/archives/libvir-list/2020-December/msg00244.html [2] https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign -- Andrea Bolognani / Red Hat / Virtualization
On 12/3/20 11:37 AM, Andrea Bolognani wrote:
On Wed, 2020-12-02 at 15:08 +0100, Andrea Bolognani wrote:
On Wed, 2020-12-02 at 09:14 -0300, Daniel Henrique Barboza wrote:
Andrea/Peter, do you want to take a look again to see if there's something that I missed?
Yeah, sorry for not being very responsive, and thanks a lot Michal for picking up the slack! I'll try to give it at least a quick look by the end of the day.
Sorry I didn't managed to get back to you yesterday but, given that we managed to get this at least partially wrong in every previous iteration, you'll hopefully forgive me for being perhaps a bit overcautious O:-)
Always good to try to minimize error :)
As mentioned elsewhere, in the process of trying to convince myself that these changes are in fact correct I found it useful be able to make a direct comparison between the ABI_UPDATE case and the vanilla one, and to facilitate that I've produced a couple of simple patches[1] that I'm hoping you'll agree to rebase your work on top of. I have actually already done that locally, so feel free to simply pick up the corresponding branch[2].
That's what I'll end up doing. I'll probably push patches 1, 2 and 4 first, since they got the R-bs and aren't affected by this rebase, then I'll make more adjustments based on your review and post a v3.
Anyway, assuming you're working from that branch, here are the differences that are introduced by using ABI_UPDATE:
$ diff -Naru tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml --- tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml 2020-12-03 14:19:21.486145577 +0100 +++ tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml 2020-12-03 14:57:09.857621727 +0100 @@ -24,13 +24,13 @@ <panic model='pseries'/> <memory model='dimm'> <target> - <size unit='KiB'>523264</size> + <size unit='KiB'>524288</size> </target> <address type='dimm' slot='0'/> </memory> <memory model='dimm'> <target> - <size unit='KiB'>524287</size> + <size unit='KiB'>524288</size> </target> <address type='dimm' slot='1'/> </memory> $ diff -Naru tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args --- tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args 2020-12-03 14:19:21.486145577 +0100 +++ tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args 2020-12-03 14:57:09.857621727 +0100 @@ -11,7 +11,7 @@ -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 \ $
You explain very well the command line change in the commit message for patch 6/6, and the output XML now reflects the aligned size for DIMMs that was used on the command line even before your changes, so this all looks good.
$ diff -Naru tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml --- tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml 2020-12-03 14:57:09.857621727 +0100 +++ tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml 2020-12-03 14:57:09.857621727 +0100 @@ -2,7 +2,7 @@ <name>QEMUGuest1</name> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory> - <memory unit='KiB'>1267710</memory> + <memory unit='KiB'>1572992</memory> <currentMemory unit='KiB'>1267710</currentMemory> <vcpu placement='static' cpuset='0-1'>2</vcpu> <os> @@ -34,7 +34,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>550000</size> + <size unit='KiB'>524416</size> <node>0</node> <label> <size unit='KiB'>128</size> $ diff -Naru tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.args $
This is where I'm a bit confused. IIUC the new value for <memory>, 1572992 KiB, is exactly 1 GiB (initial NUMA memory) + 512 MiB (NVDIMM guest area size) + 128 KiB (NVDIMM label size). Is that the value we expect users to see in the XML? If the label size were not there I would certainly say yes, but those extra 128 KiB make me pause. Then again, the <target><size> for the <memory type='nvdimm'> element also includes the label size, so perhaps it's all good? I just want to make sure it is intentional :)
This is intentional. The target_size of the NVDIMM must contain the size of the guest visible area (256MB aligned) plus the label_size.
The last bit of confusion is given by the fact that the <currentMemory> element is not updated along with the <memory> element. How will that work? Do I understand correctly that the guest will actually get the full <memory> size, but if a memory balloon is also present then the difference between <memory> and <currentMemory> will be (temporarily) returned to the host using that mechanism?
Yes. <memory> is the max amount of memory the guest can have at boot time. For our case (pSeries) it consists of the base RAM + space for the DMA window for VFIO devices and PHBs and hotplug. This is what is being directly impacted by patch 06 and this series as a whole. <currentMemory> is represented by our internal value of def->mem.cur_balloon. If there is a balloon device then <currentMemory> follows the lead of the device. If there is no RAM ballooning, def->mem.cur_balloon = <memory> = <currentMemory>. Thanks, DHB
Sorry again for all the questions and for delaying your work. I'm just trying to make sure we don't have to come back to it again in a couple of months O:-)
[1] https://www.redhat.com/archives/libvir-list/2020-December/msg00244.html [2] https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign
On Thu, 2020-12-03 at 17:04 -0300, Daniel Henrique Barboza wrote:
On 12/3/20 11:37 AM, Andrea Bolognani wrote:
This is where I'm a bit confused. IIUC the new value for <memory>, 1572992 KiB, is exactly 1 GiB (initial NUMA memory) + 512 MiB (NVDIMM guest area size) + 128 KiB (NVDIMM label size). Is that the value we expect users to see in the XML? If the label size were not there I would certainly say yes, but those extra 128 KiB make me pause. Then again, the <target><size> for the <memory type='nvdimm'> element also includes the label size, so perhaps it's all good? I just want to make sure it is intentional :)
This is intentional. The target_size of the NVDIMM must contain the size of the guest visible area (256MB aligned) plus the label_size.
The last bit of confusion is given by the fact that the <currentMemory> element is not updated along with the <memory> element. How will that work? Do I understand correctly that the guest will actually get the full <memory> size, but if a memory balloon is also present then the difference between <memory> and <currentMemory> will be (temporarily) returned to the host using that mechanism?
Yes. <memory> is the max amount of memory the guest can have at boot time. For our case (pSeries) it consists of the base RAM + space for the DMA window for VFIO devices and PHBs and hotplug. This is what is being directly impacted by patch 06 and this series as a whole.
<currentMemory> is represented by our internal value of def->mem.cur_balloon. If there is a balloon device then <currentMemory> follows the lead of the device. If there is no RAM ballooning, def->mem.cur_balloon = <memory> = <currentMemory>.
Thank you for your explanation! It all sounds good :) -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel Henrique Barboza -
Michal Privoznik