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

[David Hildenbrand <david@redhat.com>]

On 04.02.20 17:44, David Hildenbrand wrote:
> On 04.02.20 16:23, Igor Mammedov wrote:
>> On Fri, 17 Jan 2020 17:45:16 +0000
>> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>>
>>> If ACPI blob length modifications happens after the initial
>>> virt_acpi_build() call, and the changed blob length is within
>>> the PAGE size boundary, then the revised size is not seen by
>>> the firmware on Guest reboot. The is because in the
>>> virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
>>> path, qemu_ram_resize() uses used_length (ram_block size which is
>>> aligned to PAGE size) and the "resize callback" to update the size
>>> seen by firmware is not getting invoked.
>>>
>>> Hence make sure callback is called if the new size is different
>>> from original requested size.
>>>
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>> Please find the previous discussions on this issue here,
>>> https://patchwork.kernel.org/patch/11174947/
>>>
>>> But this one attempts a different solution to fix it by introducing
>>> req_length var to RAMBlock struct. 
>>>
>>
>> looks fine to me, so
>> Acked-by: Igor Mammedov <imammedo@redhat.com>
> 
> Thanks for CCing.
> 
> This in fact collides with my changes ... but not severely :)
> 
>>
>> CCing David who touches this area in his latest series for and
>> might have an opinion on how it should be handled.
>>
> 
> So we are talking about sub-page size changes? I somewhat dislike
> storing "req_length" in ram blocks. Looks like sub-pages stuff does not
> belong there.
> 
> Ram blocks only operate on page granularity. Ram block notifiers only
> operate on page granularity. Memory regions only operate on page
> granularity. Dirty bitmaps operate on page granularity. Especially,
> memory_region_size(mr) will always return aligned values.
> 
> I think users/owner should deal with anything smaller manually if
> they really need it.
> 
> What about always calling the resized() callback and letting the
> actual owner figure out if the size changed on sub-page granularity
> or not? (by looking up the size manually using some mechanism not glued to
> memory regions/ram blocks/whatever)
> 
> diff --git a/exec.c b/exec.c
> index 67e520d18e..59d46cc388 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2130,6 +2130,13 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
>      newsize = HOST_PAGE_ALIGN(newsize);
>  
>      if (block->used_length == newsize) {
> +        /*
> +         * The owner might want to handle sub-page resizes. We only provide
> +         * the aligned size - because ram blocks are always page aligned.
> +         */
> +        if (block->resized) {
> +            block->resized(block->idstr, newsize, block->host);
> +        }
>          return 0;
>      }
>

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.

-- 
Thanks,

David / dhildenb