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

[David Hildenbrand]

On 13.02.20 17:59, David Hildenbrand wrote:
> 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 ...
> 

I'm afraid we can't otherwise we will run into issues in
ram_load_precopy(). Hm ...

-- 
Thanks,

David / dhildenb