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