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

[David Hildenbrand]

On 13.02.20 17:38, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: David Hildenbrand [mailto:address@hidden]
>> Sent: 12 February 2020 18:21
>> 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;
>> address@hidden; Juan Jose Quintela Carreira <address@hidden>
>> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
> 
> [...]
> 
>>> Hmm..it breaks x86 + seabios boot. The issue is seabios expects RSDP in FSEG
>>> memory. With the above proposed change, RSDP will be aligned to PAGE_SIZE
>> and
>>> seabios mem allocation for RSDP fails at,
>>>
>>>
>> https://github.com/coreboot/seabios/blob/master/src/fw/romfile_loader.c#L8
>> 5
>>>
>>> To get pass the above, I changed "alloc_fseg" flag to false in 
>>> build_rsdp(), but
>>> seabios seems to make the assumption that RSDP has to be placed in FSEG
>> memory
>>> here,
>>> https://github.com/coreboot/seabios/blob/master/src/fw/biostables.c#L126
>>>
>>> So doesn’t look like there is an easy fix for this without changing the 
>>> seabios
>> code.
>>>
>>> Between, OVMF works fine with the aligned size on x86.
>>>
>>> One thing we can do is treat the RSDP case separately or only use the 
>>> aligned
>>> page size for "etc/acpi/tables" as below,
> 
> [...]
> 
>>>
>>> Thoughts?
>>
>> I don't think introducing memory_region_get_used_length() is a
>> good idea. I also dislike, that the ram block size can differ
>> to the memory region size. I wasn't aware of that condition, sorry!
> 
> Right. They can differ in size and is the case here.
> 
>> Making the memory region always store an aligned size might break other use
>> cases.
>>
>> Summarizing the issue:
>> 1. Memory regions contain ram blocks with a different size, if the size is
>>    not properly aligned. While memory regions can have an unaligned size,
>>    ram blocks can't. This is true when creating resizable memory region with
>>    an unaligned size.
>> 2. When resizing a ram block/memory region, the size of the memory region
>>    is set to the aligned size. The callback is called with the aligned size.
>>    The unaligned piece is lost.
>> 3. When migrating, we migrate the aligned size.
>>
>>
>> What about something like this: (untested)
> 
> Thanks for that. I had a go with the below patch and it indeed fixes the issue
> of callback not being called on resize. But the migration fails with the below
> error,
> 
> For x86
> ---------
> qemu-system-x86_64: Unknown combination of migration flags: 0x14
> qemu-system-x86_64: error while loading state for instance 0x0 of device 'ram'
> qemu-system-x86_64: load of migration failed: Invalid argument 
> 
> For arm64
> --------------
> qemu-system-aarch64: Received an unexpected compressed page
> qemu-system-aarch64: error while loading state for instance 0x0 of device 
> 'ram'
> qemu-system-aarch64: load of migration failed: Invalid argument
>  
> I haven’t debugged this further but looks like there is a corruption 
> happening.
> Please let me know if you have any clue.

The issue is

qemu_put_be64(f, ram_bytes_total_common(true) | RAM_SAVE_FLAG_MEM_SIZE)

The total ram size we store must be page aligned, otherwise it will be
detected as flags. Hm ... maybe we can round it up ...

-- 
Thanks,

David / dhildenb