[PATCH v3 0/5] add post parse pSeries mem align, when ABI_UPDATE
Hi, This is the v3 of https://www.redhat.com/archives/libvir-list/2020-November/msg01057.html Some patches were already pushed. This series contains the remaining ones that got feedback from Andrea. Michal, I added your Reviewed-by tag in all my patches because I considered that the changes made weren't worth making you review them again. Changes in v3: - former patches 1, 2 and 4: already pushed. - patches 1 and 2: these are Andrea's patches from [1]. The other patches were rebased upon them. They're being sent in this series because they're not pushed upstream in the moment of this posting. - patch 3: reverted an extra commit that was made deprecated after reverting the post parse code, as suggested by Andrea. - patch 4 and 5 (former 5 and 6): these were taken from Andrea's local branch in [2], with the test changes suggested by Andrea. [1] https://www.redhat.com/archives/libvir-list/2020-December/msg00244.html [2] https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign Andrea Bolognani (2): tests: Sync some ppc64 tests tests: Simplify some ppc64 tests Daniel Henrique Barboza (3): domain_conf.c: revert auto-alignment of pSeries NVDIMM in post parse 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 | 59 +------------- src/conf/domain_conf.h | 3 - src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 80 ++++++++++++++++++- ...mory-hotplug-nvdimm-ppc64-abi-update.args} | 20 ++--- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 1 + .../memory-hotplug-nvdimm-ppc64.args | 6 +- .../memory-hotplug-nvdimm-ppc64.xml | 8 +- ...emory-hotplug-ppc64-nonuma-abi-update.args | 29 +++++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 1 + .../memory-hotplug-ppc64-nonuma.args | 7 +- .../memory-hotplug-ppc64-nonuma.xml | 14 +++- tests/qemuxml2argvtest.c | 18 ++++- ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 46 +++++++++++ .../memory-hotplug-nvdimm-ppc64.xml | 10 +-- ...memory-hotplug-ppc64-nonuma-abi-update.xml | 38 +++++++++ .../memory-hotplug-ppc64-nonuma.xml | 1 + tests/qemuxml2xmltest.c | 19 +++++ 18 files changed, 254 insertions(+), 107 deletions(-) rename tests/qemuxml2argvdata/{memory-hotplug-nvdimm-ppc64.ppc64-latest.args => memory-hotplug-nvdimm-ppc64-abi-update.args} (62%) create mode 120000 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 120000 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 create mode 120000 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml -- 2.26.2
From: Andrea Bolognani <abologna@redhat.com> The ppc64 tests memory-hotplug-ppc64-nonuma memory-hotplug-nvdimm-ppc64 are not passed the same information for qemuxml2argv and qemuxml2xml tests; the former, in particular, doesn't show up at all in qemuxml2xml. Address this inconsistency. Note that one of the new output files had been introduced with 5540acb9a2bd despite not being actually used as of that commit. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../memory-hotplug-nvdimm-ppc64.args | 2 +- ...ory-hotplug-nvdimm-ppc64.ppc64-latest.args | 38 ---------------- tests/qemuxml2argvtest.c | 4 +- .../memory-hotplug-ppc64-nonuma.xml | 45 +++++++++++++++++++ tests/qemuxml2xmltest.c | 5 +++ 5 files changed, 54 insertions(+), 40 deletions(-) delete mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.ppc64-latest.args create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args index 92e6c538fb..94bd86ada0 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args @@ -15,7 +15,7 @@ QEMU_AUDIO_DRV=none \ -realtime mlock=off \ -smp 2,sockets=2,cores=1,threads=1 \ -numa node,nodeid=0,cpus=0-1,mem=1024 \ --object memory-backend-file,id=memnvdimm0,prealloc=yes,mem-path=/tmp/nvdimm,\ +-object memory-backend-file,id=memnvdimm0,mem-path=/tmp/nvdimm,prealloc=yes,\ size=537001984 \ -device nvdimm,node=0,label-size=131072,\ uuid=49545eb3-75e1-2d0a-acdd-f0294406c99e,memdev=memnvdimm0,id=nvdimm0,slot=0 \ diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.ppc64-latest.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.ppc64-latest.args deleted file mode 100644 index e7be7216c1..0000000000 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.ppc64-latest.args +++ /dev/null @@ -1,38 +0,0 @@ -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 guest=QEMUGuest1,debug-threads=on \ --S \ --object secret,id=masterKey0,format=raw,\ -file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ --machine pseries,accel=tcg,usb=off,dump-guest-core=off,nvdimm=on \ --cpu POWER9 \ --m size=1048576k,slots=16,maxmem=1099511627776k \ --overcommit mem-lock=off \ --smp 2,sockets=2,dies=1,cores=1,threads=1 \ --numa node,nodeid=0,cpus=0-1,mem=1024 \ --object memory-backend-file,id=memnvdimm0,mem-path=/tmp/nvdimm,prealloc=yes,\ -size=537001984 \ --device nvdimm,node=0,label-size=131072,\ -uuid=49545eb3-75e1-2d0a-acdd-f0294406c99e,memdev=memnvdimm0,id=nvdimm0,slot=0 \ --uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ --display none \ --no-user-config \ --nodefaults \ --chardev socket,id=charmonitor,fd=1729,server,nowait \ --mon chardev=charmonitor,id=monitor,mode=control \ --rtc base=utc \ --no-shutdown \ --boot strict=on \ --device pci-ohci,id=usb,bus=pci.0,addr=0x1 \ --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ --sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ -resourcecontrol=deny \ --msg timestamp=on diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 409680c84e..8c8426e699 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3011,7 +3011,9 @@ mymain(void) DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-align"); DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-pmem"); DO_TEST_CAPS_LATEST("memory-hotplug-nvdimm-readonly"); - DO_TEST_CAPS_ARCH_LATEST("memory-hotplug-nvdimm-ppc64", "ppc64"); + DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_OBJECT_MEMORY_FILE, + QEMU_CAPS_DEVICE_NVDIMM); DO_TEST("machine-aeskeywrap-on-caps", QEMU_CAPS_AES_KEY_WRAP, diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml b/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml new file mode 100644 index 0000000000..a5905e64b5 --- /dev/null +++ b/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.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'>523264</size> + </target> + <address type='dimm' slot='0'/> + </memory> + <memory model='dimm'> + <target> + <size unit='KiB'>524287</size> + </target> + <address type='dimm' slot='1'/> + </memory> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 603ba71686..83e5dd0cbe 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1229,6 +1229,9 @@ mymain(void) /* SVE aarch64 CPU features work on modern QEMU */ DO_TEST_CAPS_ARCH_LATEST("aarch64-features-sve", "aarch64"); + 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("memory-hotplug", NONE); DO_TEST("memory-hotplug-dimm", QEMU_CAPS_DEVICE_PC_DIMM); DO_TEST("memory-hotplug-nvdimm", QEMU_CAPS_DEVICE_NVDIMM); @@ -1239,7 +1242,9 @@ mymain(void) DO_TEST("memory-hotplug-nvdimm-readonly", QEMU_CAPS_DEVICE_NVDIMM, QEMU_CAPS_DEVICE_NVDIMM_UNARMED); DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_OBJECT_MEMORY_FILE, QEMU_CAPS_DEVICE_NVDIMM); + DO_TEST("net-udp", NONE); DO_TEST("video-virtio-gpu-device", QEMU_CAPS_DEVICE_VIRTIO_GPU); -- 2.26.2
On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
From: Andrea Bolognani <abologna@redhat.com>
The ppc64 tests
memory-hotplug-ppc64-nonuma memory-hotplug-nvdimm-ppc64
are not passed the same information for qemuxml2argv and qemuxml2xml tests; the former, in particular, doesn't show up at all in qemuxml2xml. Address this inconsistency.
Note that one of the new output files had been introduced with 5540acb9a2bd despite not being actually used as of that commit.
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../memory-hotplug-nvdimm-ppc64.args | 2 +- ...ory-hotplug-nvdimm-ppc64.ppc64-latest.args | 38 ---------------- tests/qemuxml2argvtest.c | 4 +- .../memory-hotplug-ppc64-nonuma.xml | 45 +++++++++++++++++++ tests/qemuxml2xmltest.c | 5 +++ 5 files changed, 54 insertions(+), 40 deletions(-) delete mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.ppc64-latest.args create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml
I have now pushed this patch and the next one with your R-b as well as Jano's. -- Andrea Bolognani / Red Hat / Virtualization
From: Andrea Bolognani <abologna@redhat.com> We can leave out things like USB controller, memballoon device, kernel and initrd since they're not the focus of the tests. Propagating some information from the output files back to the input files makes it easier to compare them, as it reduces the resulting diff, and in the case of the qemuxml2xml test for memory-hotplug-ppc64-nonuma it allows us to convert the output file into a symlink, since in the specific case the XML doesn't change at all. Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- .../memory-hotplug-nvdimm-ppc64.args | 4 +- .../memory-hotplug-nvdimm-ppc64.xml | 8 +--- .../memory-hotplug-ppc64-nonuma.args | 7 +-- .../memory-hotplug-ppc64-nonuma.xml | 14 ++++-- .../memory-hotplug-nvdimm-ppc64.xml | 8 +--- .../memory-hotplug-ppc64-nonuma.xml | 46 +------------------ 6 files changed, 17 insertions(+), 70 deletions(-) mode change 100644 => 120000 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args index 94bd86ada0..f50444e47e 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.args @@ -27,6 +27,4 @@ uuid=49545eb3-75e1-2d0a-acdd-f0294406c99e,memdev=memnvdimm0,id=nvdimm0,slot=0 \ server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ --no-shutdown \ --usb \ --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 +-no-shutdown diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml index ae5a17d3c8..bf7df9a259 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64.xml @@ -21,16 +21,12 @@ <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='usb' index='0' model='none'/> <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> + <memballoon model='none'/> <panic model='pseries'/> <memory model='nvdimm'> <uuid>49545eb3-75e1-2d0a-acdd-f0294406c99e</uuid> diff --git a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args index 91cea9d8bf..f7f151d1ca 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args +++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.args @@ -26,9 +26,4 @@ QEMU_AUDIO_DRV=none \ 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 +-no-shutdown diff --git a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.xml b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.xml index 7c68cd6aa2..a0806c4b54 100644 --- a/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.xml +++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma.xml @@ -7,9 +7,7 @@ <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> @@ -17,16 +15,24 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu-system-ppc64</emulator> - <memballoon model='virtio'/> + <controller type='usb' index='0' model='none'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <memballoon model='none'/> + <panic model='pseries'/> <memory model='dimm'> <target> <size unit='KiB'>523264</size> </target> + <address type='dimm' slot='0'/> </memory> <memory model='dimm'> <target> <size unit='KiB'>524287</size> </target> + <address type='dimm' slot='1'/> </memory> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml index ecb1b83b4a..0c0b9f96fb 100644 --- a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml +++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml @@ -21,16 +21,12 @@ <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='usb' index='0' model='none'/> <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> + <memballoon model='none'/> <panic model='pseries'/> <memory model='nvdimm'> <uuid>49545eb3-75e1-2d0a-acdd-f0294406c99e</uuid> diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml b/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml deleted file mode 100644 index a5905e64b5..0000000000 --- a/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml +++ /dev/null @@ -1,45 +0,0 @@ -<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'>523264</size> - </target> - <address type='dimm' slot='0'/> - </memory> - <memory model='dimm'> - <target> - <size unit='KiB'>524287</size> - </target> - <address type='dimm' slot='1'/> - </memory> - </devices> -</domain> diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml b/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml new file mode 120000 index 0000000000..0822e409b4 --- /dev/null +++ b/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma.xml @@ -0,0 +1 @@ +../qemuxml2argvdata/memory-hotplug-ppc64-nonuma.xml \ No newline at end of file -- 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. Commit d3f3c2c97f9b was predecessed by a move/rename of the former qemuDomainNVDimmAlignSizePseries() function, commit ace5931553c8. Reverting d3f3c2c97f9b will make the function rename obsolete since all callers will now reside in QEMU drivers files, and it will remain that way in the next patches, so let's revert ace5931553c8 as well. This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88 (domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()). This reverts commit ace5931553c87beebb6b3cd994061742b3f88238 (conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c). Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/conf/domain_conf.c | 59 +------------------ src/conf/domain_conf.h | 3 - src/libvirt_private.syms | 1 - src/qemu/qemu_domain.c | 42 ++++++++++++- .../memory-hotplug-nvdimm-ppc64.xml | 2 +- 5 files changed, 42 insertions(+), 65 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 425e3c3710..d069a3ecc2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5359,24 +5359,6 @@ virDomainVsockDefPostParse(virDomainVsockDefPtr vsock) } -static int -virDomainMemoryDefPostParse(virDomainMemoryDefPtr mem, - const virDomainDef *def) -{ - /* Although only the QEMU driver implements PPC64 support, this - * code is related to the platform specification (PAPR), i.e. it - * is hypervisor agnostic, and any future PPC64 hypervisor driver - * will have the same restriction. - */ - if (ARCH_IS_PPC64(def->os.arch) && - mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && - virDomainNVDimmAlignSizePseries(mem) < 0) - return -1; - - return 0; -} - - static int virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5422,10 +5404,6 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, ret = 0; break; - case VIR_DOMAIN_DEVICE_MEMORY: - ret = virDomainMemoryDefPostParse(dev->data.memory, def); - break; - case VIR_DOMAIN_DEVICE_LEASE: case VIR_DOMAIN_DEVICE_FS: case VIR_DOMAIN_DEVICE_INPUT: @@ -5440,6 +5418,7 @@ virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev, case VIR_DOMAIN_DEVICE_SHMEM: case VIR_DOMAIN_DEVICE_TPM: case VIR_DOMAIN_DEVICE_PANIC: + case VIR_DOMAIN_DEVICE_MEMORY: case VIR_DOMAIN_DEVICE_IOMMU: case VIR_DOMAIN_DEVICE_AUDIO: ret = 0; @@ -16836,42 +16815,6 @@ virDomainSEVDefParseXML(xmlNodePtr sevNode, return NULL; } -int -virDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem) -{ - /* For NVDIMMs in ppc64 in we want to align down the guest - * visible space, instead of align up, to avoid writing - * beyond the end of file by adding a potential 256MiB - * to the user specified size. - * - * The label-size is mandatory for ppc64 as well, meaning that - * the guest visible space will be target_size-label_size. - * - * Finally, target_size must include label_size. - * - * The above can be summed up as follows: - * - * target_size = AlignDown(target_size - label_size) + label_size - */ - unsigned long long ppc64AlignSize = 256 * 1024; - unsigned long long guestArea = mem->size - mem->labelsize; - - /* Align down guest_area. 256MiB is the minimum size. Error - * out if target_size is smaller than 256MiB + label_size, - * since aligning it up will cause QEMU errors. */ - if (mem->size < (ppc64AlignSize + mem->labelsize)) { - virReportError(VIR_ERR_XML_ERROR, "%s", - _("minimum target size for the NVDIMM " - "must be 256MB plus the label size")); - return -1; - } - - guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize; - mem->size = guestArea + mem->labelsize; - - return 0; -} - static virDomainMemoryDefPtr virDomainMemoryDefParseXML(virDomainXMLOptionPtr xmlopt, xmlNodePtr memdevNode, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 34cde22965..b1571d423c 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3912,9 +3912,6 @@ bool virDomainBlockIoTuneInfoEqual(const virDomainBlockIoTuneInfo *a, const virDomainBlockIoTuneInfo *b); -int -virDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem); - bool virHostdevIsSCSIDevice(const virDomainHostdevDef *hostdev) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 2f640ef1c4..5cf39bdf6c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -546,7 +546,6 @@ virDomainNetTypeToString; virDomainNetUpdate; virDomainNostateReasonTypeFromString; virDomainNostateReasonTypeToString; -virDomainNVDimmAlignSizePseries; virDomainObjAssignDef; virDomainObjBroadcast; virDomainObjCheckActive; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 663c0af867..af3c0e269a 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8044,6 +8044,44 @@ qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def, } +static int +qemuDomainNVDimmAlignSizePseries(virDomainDefPtr def, + virDomainMemoryDefPtr mem) +{ + /* For NVDIMMs in ppc64 in we want to align down the guest + * visible space, instead of align up, to avoid writing + * beyond the end of file by adding a potential 256MiB + * to the user specified size. + * + * The label-size is mandatory for ppc64 as well, meaning that + * the guest visible space will be target_size-label_size. + * + * Finally, target_size must include label_size. + * + * The above can be summed up as follows: + * + * target_size = AlignDown(target_size - label_size) + label_size + */ + unsigned long long ppc64AlignSize = qemuDomainGetMemorySizeAlignment(def); + unsigned long long guestArea = mem->size - mem->labelsize; + + /* Align down guest_area. 256MiB is the minimum size. Error + * out if target_size is smaller than 256MiB + label_size, + * since aligning it up will cause QEMU errors. */ + if (mem->size < (ppc64AlignSize + mem->labelsize)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("minimum target size for the NVDIMM " + "must be 256MB plus the label size")); + return -1; + } + + guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize; + mem->size = guestArea + mem->labelsize; + + return 0; +} + + int qemuDomainAlignMemorySizes(virDomainDefPtr def) { @@ -8092,7 +8130,7 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) for (i = 0; i < def->nmems; i++) { if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && ARCH_IS_PPC64(def->os.arch)) { - if (virDomainNVDimmAlignSizePseries(def->mems[i]) < 0) + if (qemuDomainNVDimmAlignSizePseries(def, def->mems[i]) < 0) return -1; } else { align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]); @@ -8129,7 +8167,7 @@ qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, { if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && ARCH_IS_PPC64(def->os.arch)) { - return virDomainNVDimmAlignSizePseries(mem); + return qemuDomainNVDimmAlignSizePseries(def, mem); } else { mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def)); diff --git a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml index 0c0b9f96fb..bf7df9a259 100644 --- a/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml +++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64.xml @@ -34,7 +34,7 @@ <path>/tmp/nvdimm</path> </source> <target> - <size unit='KiB'>524416</size> + <size unit='KiB'>550000</size> <node>0</node> <label> <size unit='KiB'>128</size> -- 2.26.2
On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88 (domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()).
This reverts commit ace5931553c87beebb6b3cd994061742b3f88238 (conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).
Please revert the two commits separately. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, 2020-12-04 at 15:55 +0100, Andrea Bolognani wrote:
On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88 (domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()).
This reverts commit ace5931553c87beebb6b3cd994061742b3f88238 (conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).
Please revert the two commits separately.
Actually, looking at the rest of the series, I think it's better if you do *not* go ahead with a straight revert, since that results in 1) returning the function to its old place and signature; 2) having to change the signature back; 3) having to move the function because you need it to be usable earlier in the file. That's a whole lot of churn for arguably very little benefit. I suggest you instead do a pseudo-revert, where you rename the function (without otherwise altering its signature) and move it to the part of qemu_domain.c where you are ultimately going to need it; in the commit message, you can still mention the commit that such a change is a "spiritual revert" of, but this way we avoid muddying the waters more than necessary. Sorry for not realizing earlier that my initial suggestion would have these consequences. -- Andrea Bolognani / Red Hat / Virtualization
On 12/4/20 12:13 PM, Andrea Bolognani wrote:
On Fri, 2020-12-04 at 15:55 +0100, Andrea Bolognani wrote:
On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
This reverts commit d3f3c2c97f9b92c982ff809479495f44614edb88 (domain_conf.c: auto-align pSeries NVDIMM in virDomainMemoryDefPostParse()).
This reverts commit ace5931553c87beebb6b3cd994061742b3f88238 (conf, qemu: move qemuDomainNVDimmAlignSizePseries to domain_conf.c).
Please revert the two commits separately.
Actually, looking at the rest of the series, I think it's better if you do *not* go ahead with a straight revert, since that results in
1) returning the function to its old place and signature; 2) having to change the signature back; 3) having to move the function because you need it to be usable earlier in the file.
That's a whole lot of churn for arguably very little benefit.
I suggest you instead do a pseudo-revert, where you rename the function (without otherwise altering its signature) and move it to the part of qemu_domain.c where you are ultimately going to need it; in the commit message, you can still mention the commit that such a change is a "spiritual revert" of, but this way we avoid muddying the waters more than necessary.
The change in signature was done to avoid using qemuDomainGetMemorySizeAlignment(def), because that would be another function that would need to be moved up. I forgot to mention that in patch 04. IIUC what you're suggesting can be implemented as follows: - go back to the first revert I was doing in v2 (remove the code from PostParse). Single revert of d3f3c2c97f9b92; - apply the other 2 patches; - do an extra patch to rename/move the function that is now being used only in QEMU files, but without a straight up revert of ace5931553c8. I can also simplify the signature in this step. The order here is intentional - the double revert in patch 3 done in this series needed to be followed up by changes in patches 4 and 5 (given that the function was renamed in patch 3). In this order, I can pick the patches from your local branch directly and just bother with the renaming in a single follow-up patch. Sounds good? DHB
Sorry for not realizing earlier that my initial suggestion would have these consequences.
On Fri, 2020-12-04 at 13:50 -0300, Daniel Henrique Barboza wrote:
On 12/4/20 12:13 PM, Andrea Bolognani wrote:
I suggest you instead do a pseudo-revert, where you rename the function (without otherwise altering its signature) and move it to the part of qemu_domain.c where you are ultimately going to need it; in the commit message, you can still mention the commit that such a change is a "spiritual revert" of, but this way we avoid muddying the waters more than necessary.
The change in signature was done to avoid using qemuDomainGetMemorySizeAlignment(def), because that would be another function that would need to be moved up. I forgot to mention that in patch 04.
Why would you want to avoid using it? It's exactly what that function is intended to do. Moving it around is no big deal, and by using it you can avoid open-coding the same value twice. See attached diff.
IIUC what you're suggesting can be implemented as follows:
- go back to the first revert I was doing in v2 (remove the code from PostParse). Single revert of d3f3c2c97f9b92;
- apply the other 2 patches;
- do an extra patch to rename/move the function that is now being used only in QEMU files, but without a straight up revert of ace5931553c8. I can also simplify the signature in this step.
Yes, except of course the other two patches should still include the changes that were implemented after my review, eg. they should be as seen in https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign and not how they showed up initially on the list.
The order here is intentional - the double revert in patch 3 done in this series needed to be followed up by changes in patches 4 and 5 (given that the function was renamed in patch 3). In this order, I can pick the patches from your local branch directly and just bother with the renaming in a single follow-up patch.
Sounds good?
Yeah, it sounds like a plan. You can consider all patches from the branch linked above Reviewed-by: Andrea Bolognani <abologna@redhat.com> and push them at your leisure. -- Andrea Bolognani / Red Hat / Virtualization
On 12/4/20 3:15 PM, Andrea Bolognani wrote:
On Fri, 2020-12-04 at 13:50 -0300, Daniel Henrique Barboza wrote:
On 12/4/20 12:13 PM, Andrea Bolognani wrote:
I suggest you instead do a pseudo-revert, where you rename the function (without otherwise altering its signature) and move it to the part of qemu_domain.c where you are ultimately going to need it; in the commit message, you can still mention the commit that such a change is a "spiritual revert" of, but this way we avoid muddying the waters more than necessary.
The change in signature was done to avoid using qemuDomainGetMemorySizeAlignment(def), because that would be another function that would need to be moved up. I forgot to mention that in patch 04.
Why would you want to avoid using it? It's exactly what that function is intended to do. Moving it around is no big deal, and by using it you can avoid open-coding the same value twice. See attached diff.
Fair enough.
IIUC what you're suggesting can be implemented as follows:
- go back to the first revert I was doing in v2 (remove the code from PostParse). Single revert of d3f3c2c97f9b92;
- apply the other 2 patches;
- do an extra patch to rename/move the function that is now being used only in QEMU files, but without a straight up revert of ace5931553c8. I can also simplify the signature in this step.
Yes, except of course the other two patches should still include the changes that were implemented after my review, eg. they should be as seen in
https://gitlab.com/abologna/libvirt/-/tree/ppc64-memalign
and not how they showed up initially on the list.
Yep, that's the idea.
The order here is intentional - the double revert in patch 3 done in this series needed to be followed up by changes in patches 4 and 5 (given that the function was renamed in patch 3). In this order, I can pick the patches from your local branch directly and just bother with the renaming in a single follow-up patch.
Sounds good?
Yeah, it sounds like a plan. You can consider all patches from the branch linked above
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
and push them at your leisure.
Alright! I'll do the adjustments and push with your R-b. Thanks, DHB
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_UPDATE_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. qemuDomainNVDimmAlignSizePseries() was moved up to be used by qemuDomainMemoryDefPostParse(). The alignment for pSeries is always 256 * 1024 KiB, making a call to qemuDomainGetMemorySizeAlignment() and the need for the 'def' arg obsolete as well. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 112 +++++++++++------- ...emory-hotplug-nvdimm-ppc64-abi-update.args | 30 +++++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 1 + tests/qemuxml2argvtest.c | 7 ++ ...memory-hotplug-nvdimm-ppc64-abi-update.xml | 46 +++++++ tests/qemuxml2xmltest.c | 7 ++ 6 files changed, 162 insertions(+), 41 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.args create mode 120000 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 af3c0e269a..5fff7d312d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5341,6 +5341,70 @@ qemuDomainTPMDefPostParse(virDomainTPMDefPtr tpm, } +static int +qemuDomainNVDimmAlignSizePseries(virDomainMemoryDefPtr mem) +{ + /* For NVDIMMs in ppc64 in we want to align down the guest + * visible space, instead of align up, to avoid writing + * beyond the end of file by adding a potential 256MiB + * to the user specified size. + * + * The label-size is mandatory for ppc64 as well, meaning that + * the guest visible space will be target_size-label_size. + * + * Finally, target_size must include label_size. + * + * The above can be summed up as follows: + * + * target_size = AlignDown(target_size - label_size) + label_size + */ + unsigned long long ppc64AlignSize = 256 * 1024; + unsigned long long guestArea = mem->size - mem->labelsize; + + /* Align down guest_area. 256MiB is the minimum size. Error + * out if target_size is smaller than 256MiB + label_size, + * since aligning it up will cause QEMU errors. */ + if (mem->size < (ppc64AlignSize + mem->labelsize)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("minimum target size for the NVDIMM " + "must be 256MB plus the label size")); + return -1; + } + + guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize; + mem->size = guestArea + mem->labelsize; + + return 0; +} + + +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 && + qemuDomainNVDimmAlignSizePseries(mem) < 0) + return -1; + } + + return 0; +} + + static int qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, const virDomainDef *def, @@ -5398,6 +5462,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: @@ -5410,7 +5479,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; @@ -8044,44 +8112,6 @@ qemuDomainGetMemoryModuleSizeAlignment(const virDomainDef *def, } -static int -qemuDomainNVDimmAlignSizePseries(virDomainDefPtr def, - virDomainMemoryDefPtr mem) -{ - /* For NVDIMMs in ppc64 in we want to align down the guest - * visible space, instead of align up, to avoid writing - * beyond the end of file by adding a potential 256MiB - * to the user specified size. - * - * The label-size is mandatory for ppc64 as well, meaning that - * the guest visible space will be target_size-label_size. - * - * Finally, target_size must include label_size. - * - * The above can be summed up as follows: - * - * target_size = AlignDown(target_size - label_size) + label_size - */ - unsigned long long ppc64AlignSize = qemuDomainGetMemorySizeAlignment(def); - unsigned long long guestArea = mem->size - mem->labelsize; - - /* Align down guest_area. 256MiB is the minimum size. Error - * out if target_size is smaller than 256MiB + label_size, - * since aligning it up will cause QEMU errors. */ - if (mem->size < (ppc64AlignSize + mem->labelsize)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("minimum target size for the NVDIMM " - "must be 256MB plus the label size")); - return -1; - } - - guestArea = (guestArea/ppc64AlignSize) * ppc64AlignSize; - mem->size = guestArea + mem->labelsize; - - return 0; -} - - int qemuDomainAlignMemorySizes(virDomainDefPtr def) { @@ -8130,7 +8160,7 @@ qemuDomainAlignMemorySizes(virDomainDefPtr def) for (i = 0; i < def->nmems; i++) { if (def->mems[i]->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && ARCH_IS_PPC64(def->os.arch)) { - if (qemuDomainNVDimmAlignSizePseries(def, def->mems[i]) < 0) + if (qemuDomainNVDimmAlignSizePseries(def->mems[i]) < 0) return -1; } else { align = qemuDomainGetMemoryModuleSizeAlignment(def, def->mems[i]); @@ -8167,7 +8197,7 @@ qemuDomainMemoryDeviceAlignSize(virDomainDefPtr def, { if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM && ARCH_IS_PPC64(def->os.arch)) { - return qemuDomainNVDimmAlignSizePseries(def, mem); + return qemuDomainNVDimmAlignSizePseries(mem); } else { mem->size = VIR_ROUND_UP(mem->size, qemuDomainGetMemorySizeAlignment(def)); diff --git a/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.args b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.args new file mode 100644 index 0000000000..f50444e47e --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.args @@ -0,0 +1,30 @@ +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=tcg,usb=off,dump-guest-core=off,nvdimm=on \ +-m size=1048576k,slots=16,maxmem=1099511627776k \ +-realtime mlock=off \ +-smp 2,sockets=2,cores=1,threads=1 \ +-numa node,nodeid=0,cpus=0-1,mem=1024 \ +-object memory-backend-file,id=memnvdimm0,mem-path=/tmp/nvdimm,prealloc=yes,\ +size=537001984 \ +-device nvdimm,node=0,label-size=131072,\ +uuid=49545eb3-75e1-2d0a-acdd-f0294406c99e,memdev=memnvdimm0,id=nvdimm0,slot=0 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-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 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 120000 index 0000000000..c7d71906f6 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-nvdimm-ppc64-abi-update.xml @@ -0,0 +1 @@ +memory-hotplug-nvdimm-ppc64.xml \ No newline at end of file diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 8c8426e699..a9bd210ca3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -3014,6 +3014,13 @@ mymain(void) DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_OBJECT_MEMORY_FILE, QEMU_CAPS_DEVICE_NVDIMM); + DO_TEST_FULL("memory-hotplug-nvdimm-ppc64-abi-update", + ARG_PARSEFLAGS, VIR_DOMAIN_DEF_PARSE_ABI_UPDATE, + ARG_QEMU_CAPS, + QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, + QEMU_CAPS_OBJECT_MEMORY_FILE, + QEMU_CAPS_DEVICE_NVDIMM, + QEMU_CAPS_LAST); DO_TEST("machine-aeskeywrap-on-caps", QEMU_CAPS_AES_KEY_WRAP, 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..3999b1a99f --- /dev/null +++ b/tests/qemuxml2xmloutdata/memory-hotplug-nvdimm-ppc64-abi-update.xml @@ -0,0 +1,46 @@ +<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' model='none'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <memballoon model='none'/> + <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 83e5dd0cbe..18faea2b33 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1244,6 +1244,13 @@ mymain(void) DO_TEST("memory-hotplug-nvdimm-ppc64", QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE, QEMU_CAPS_OBJECT_MEMORY_FILE, 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_OBJECT_MEMORY_FILE, + QEMU_CAPS_DEVICE_NVDIMM, + QEMU_CAPS_LAST); DO_TEST("net-udp", NONE); -- 2.26.2
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) Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 14 +++++-- ...emory-hotplug-ppc64-nonuma-abi-update.args | 29 ++++++++++++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 1 + tests/qemuxml2argvtest.c | 7 ++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 38 +++++++++++++++++++ tests/qemuxml2xmltest.c | 7 ++++ 6 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 120000 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 5fff7d312d..e7f82d42ed 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -5395,10 +5395,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 && - qemuDomainNVDimmAlignSizePseries(mem) < 0) - return -1; + if (ARCH_IS_PPC64(arch)) { + unsigned long long ppc64MemModuleAlign = 256 * 1024; + + if (mem->model == VIR_DOMAIN_MEMORY_MODEL_NVDIMM) { + if (qemuDomainNVDimmAlignSizePseries(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..ac741b80c0 --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args @@ -0,0 +1,29 @@ +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 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 120000 index 0000000000..03410026cc --- /dev/null +++ b/tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml @@ -0,0 +1 @@ +memory-hotplug-ppc64-nonuma.xml \ No newline at end of file diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index a9bd210ca3..7c9dc0e641 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..4edb786e6f --- /dev/null +++ b/tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml @@ -0,0 +1,38 @@ +<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> + <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' model='none'/> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <memballoon model='none'/> + <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 18faea2b33..1968be6782 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1232,6 +1232,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", 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("memory-hotplug", NONE); DO_TEST("memory-hotplug-dimm", QEMU_CAPS_DEVICE_PC_DIMM); DO_TEST("memory-hotplug-nvdimm", QEMU_CAPS_DEVICE_NVDIMM); -- 2.26.2
On Thu, 2020-12-03 at 22:05 -0300, Daniel Henrique Barboza wrote:
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)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_domain.c | 14 +++++-- ...emory-hotplug-ppc64-nonuma-abi-update.args | 29 ++++++++++++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 1 + tests/qemuxml2argvtest.c | 7 ++++ ...memory-hotplug-ppc64-nonuma-abi-update.xml | 38 +++++++++++++++++++ tests/qemuxml2xmltest.c | 7 ++++ 6 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.args create mode 120000 tests/qemuxml2argvdata/memory-hotplug-ppc64-nonuma-abi-update.xml create mode 100644 tests/qemuxml2xmloutdata/memory-hotplug-ppc64-nonuma-abi-update.xml
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Daniel Henrique Barboza