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

[Shameerali Kolothum Thodi]

Hi David,

> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of David Hildenbrand
> Sent: 04 February 2020 19:05
> To: Igor Mammedov <address@hidden>; Shameerali Kolothum Thodi
> <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
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
> 
> 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 <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.

Thanks for taking a look at this. Agree, I didn’t like that "req_length" either.

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

Does it make sense to pass the requested size in the callback than the aligned 
size
as the owner might be interested more in the org_req_size vs new_req _size case?

> > +        }
> >          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.
> 

Sorry, I didn’t quite get that point and not sure how "req_length" approach 
will affect the migration.

Anyway, I have reworked the patch(below) with the above suggestion, that is
always calling the resized() callback, but modified it to pass the requested 
size
instead of the page aligned size. Please let me know your feedback.

Thanks,
Shameer

diff --git a/exec.c b/exec.c
index 67e520d18e..c9cb9a54fa 100644
--- a/exec.c
+++ b/exec.c
@@ -2123,13 +2123,22 @@ static int memory_try_enable_merging(void *addr, size_t 
len)
  * resize callback to update device state and/or add assertions to detect
  * misuse, if necessary.
  */
-int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
+int qemu_ram_resize(RAMBlock *block, ram_addr_t size, Error **errp)
 {
+    ram_addr_t newsize;
     assert(block);
 
-    newsize = HOST_PAGE_ALIGN(newsize);
+    newsize = HOST_PAGE_ALIGN(size);
 
     if (block->used_length == newsize) {
+        /*
+         * RAM blocks are always page aligned, but the owner might want
+         * to handle sub-page resizes. Let the owner know about any
+         * sub-page changes.
+         */
+        if (block->resized) {
+            block->resized(block->idstr, size, block->host);
+        }
         return 0;
     }
 
@@ -2155,7 +2164,7 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, 
Error **errp)
                                         DIRTY_CLIENTS_ALL);
     memory_region_set_size(block->mr, newsize);
     if (block->resized) {
-        block->resized(block->idstr, newsize, block->host);
+        block->resized(block->idstr, size, block->host);
     }
     return 0;
 }
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 179b302f01..ec9cfa4d10 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -934,9 +934,11 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
*filename,
 
     for (i = 0; i < index; i++) {
         if (strcmp(filename, s->files->f[i].name) == 0) {
-            ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
-                                           data, len);
-            s->files->f[i].size   = cpu_to_be32(len);
+            if (s->files->f[i].size != cpu_to_be32(len)) {
+                ptr = fw_cfg_modify_bytes_read(s, FW_CFG_FILE_FIRST + i,
+                                               data, len);
+                s->files->f[i].size   = cpu_to_be32(len);
+            }
             return ptr;
         }
     }
---