[PATCH v2 0/6] add post parse pSeries mem align, when ABI_UPDATE

Thu Dec 3 20:04:10 UTC 2020

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
> 