RE: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: David Hildenbrand [mailto:david@redhat.com]
> Sent: 05 February 2020 16:41
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; xiaoguangrong.eric@gmail.com;
> mst@redhat.com; shannon.zhaosl@gmail.com; qemu-devel@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; lersek@redhat.com
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
> 
> >> Oh, and one more reason why the proposal in this patch is inconsistent:
> >>
> >> When migrating resizable memory regions (RAM_SAVE_FLAG_MEM_SIZE)
> we
> >> store the block->used_length (ram_save_setup()) and use that value to
> >> resize the region on the target (ram_load_precopy() -> qemu_ram_resize()).
> >>
> >> This will be the value the callback will be called with. Page aligned.
> >>
> >
> > Sorry, I didn’t quite get that point and not sure how "req_length" approach
> > will affect the migration.
> 
> The issue is that on migration, you will lose the sub-page size either
> way. So your callback will be called
> - on the migration source with a sub-page size (via
>   memory_region_ram_resize() from e.g., hw/i386/acpi-build.c)
> - on the migration target with a page-aligned size (via
>   qemu_ram_resize() from migration/ram.c)
> 
> So this is inconsistent, especially when migrating.

Thanks for explaining. I tried to add some debug prints to further understand
what actually happens during migration case.

Guest-source with initial one nvdimm
----------------------------------------------------

-object memory-backend-ram,id=mem1,size=1G \
-device nvdimm,id=dimm1,memdev=mem1 \

fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
fw_cfg_add_file_callback: filename etc/acpi/tables size 0x55f4
fw_cfg_add_file_callback: filename etc/table-loader size 0xd00
fw_cfg_add_file_callback: filename etc/tpm/log size 0x0
fw_cfg_add_file_callback: filename etc/acpi/rsdp size 0x24
fw_cfg_add_file_callback: filename etc/smbios/smbios-tables size 0x104
fw_cfg_add_file_callback: filename etc/smbios/smbios-anchor size 0x18
fw_cfg_modify_file: filename bootorder len 0x0
fw_cfg_add_file_callback: filename bootorder size 0x0
fw_cfg_modify_file: filename bios-geometry len 0x0
fw_cfg_add_file_callback: filename bios-geometry size 0x0
fw_cfg_modify_file: filename etc/acpi/tables len 0x55f4
fw_cfg_modify_file: filename etc/acpi/rsdp len 0x24
fw_cfg_modify_file: filename etc/table-loader len 0xd00
....

hot add another nvdimm device,

(qemu) object_add memory-backend-ram,id=mem2,size=1G
(qemu) device_add nvdimm,id=dimm2,memdev=mem2


root@ubuntu:/# cat /dev/pmem
pmem0  pmem1

Guest-target with two nvdimms
-----------------------------------------------

-object memory-backend-ram,id=mem1,size=1G \
-device nvdimm,id=dimm1,memdev=mem1 \
-object memory-backend-ram,id=mem2,size=1G \
-device nvdimm,id=dimm2,memdev=mem2 \

fw_cfg_add_file_callback: filename etc/boot-fail-wait size 0x4
fw_cfg_add_file_callback: filename etc/acpi/nvdimm-mem size 0x1000
fw_cfg_add_file_callback: filename etc/acpi/tables size 0x56ac
fw_cfg_add_file_callback: filename etc/table-loader size 0xd00
fw_cfg_add_file_callback: filename etc/tpm/log size 0x0
fw_cfg_add_file_callback: filename etc/acpi/rsdp size 0x24
fw_cfg_add_file_callback: filename etc/smbios/smbios-tables size 0x104
fw_cfg_add_file_callback: filename etc/smbios/smbios-anchor size 0x18
fw_cfg_modify_file: filename bootorder len 0x0
fw_cfg_add_file_callback: filename bootorder size 0x0
fw_cfg_modify_file: filename bios-geometry len 0x0
fw_cfg_add_file_callback: filename bios-geometry size 0x0


Initiate migration Source --> Target,

ram_load_precopy: Ram blk mach-virt.ram length 0x100000000 used_length 0x100000000
ram_load_precopy: Ram blk mem1 length 0x40000000 used_length 0x40000000
ram_load_precopy: Ram blk mem2 length 0x40000000 used_length 0x40000000
ram_load_precopy: Ram blk virt.flash0 length 0x4000000 used_length 0x4000000
ram_load_precopy: Ram blk virt.flash1 length 0x4000000 used_length 0x4000000
ram_load_precopy: Ram blk /rom@etc/acpi/tables length 0x6000 used_length 0x6000
ram_load_precopy: Ram blk 0000:00:01.0/virtio-net-pci.rom length 0x40000 used_length 0x40000
ram_load_precopy: Ram blk /rom@etc/table-loader length 0x1000 used_length 0x1000
ram_load_precopy: Ram blk /rom@etc/acpi/rsdp length 0x1000 used_length 0x1000


root@ubuntu:/# cat /dev/pmem
pmem0  pmem1  

From the logs, it looks like the ram_load_precopy() --> qemu_ram_resize() is not
called as length == used_length and both seems to be page aligned values.
And from https://github.com/qemu/qemu/blob/master/migration/ram.c#L3421
qemu_ram_resize() is called with length if length != used_length.

Of course my knowledge on Qemu migration is very limited and maybe I am
missing something here. But I am trying to see under what circumstances 
qemu_ram_resize() will get invoked with a page aligned size that will be different
to the size used in the FWCfgEntry . Please let me know,

Much appreciated,
Shameer

> Is there a way to get access to the sub-page size without passing it
> through the callback?
> 
> Like in fw_cfg_modify_file() do some fancy lookup and use the sub-page
> size instead of the passed size? (might have to be stored somewhere and
> refetched - and migrated)
> 
> --
> Thanks,
> 
> David / dhildenb