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
