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

[David Hildenbrand]

On 06.02.20 11:20, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: David Hildenbrand [mailto:address@hidden]
>> Sent: 05 February 2020 16:41
>> To: Shameerali Kolothum Thodi <address@hidden>;
>> Igor Mammedov <address@hidden>
>> Cc: address@hidden; address@hidden;
>> address@hidden; address@hidden; address@hidden;
>> xuwei (O) <address@hidden>; Linuxarm <address@hidden>;
>> address@hidden; address@hidden; address@hidden
>> 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.

Assume on your source, the old size is 12345 bytes. So 16384 aligned up
(4 pages).

Assume on your target, the new size is 123456 bytes, so 126976 aligned
up (31 pages).

If you migrate from source to destination, the migration code would
resize to 16384, although the "actual size" is 12345. The callback will
be called with the aligned size, not the actual size. Same the other way
around. That's what's inconsistent IMHO.

-- 
Thanks,

David / dhildenb