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.th...@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.th...@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 <imamm...@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