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

[David Hildenbrand]

On 04.02.20 16:23, Igor Mammedov wrote:
> On Fri, 17 Jan 2020 17:45:16 +0000
> Shameer Kolothum <address@hidden> 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 <address@hidden>
>> ---
>> 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 <address@hidden>

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;
     }

-- 
Thanks,

David / dhildenb