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

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: David Hildenbrand [mailto:address@hidden]
> Sent: 10 February 2020 09:54
> 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
> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
> 
> On 10.02.20 10:50, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: David Hildenbrand [mailto:address@hidden]
> >> Sent: 10 February 2020 09:29
> >> 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
> >> Subject: Re: [PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback
> >>
> >>>> Can you look the original value up somehow and us the resize callback
> >>>> only as a notification that something changed? (that value would have to
> >>>> be stored somewhere and migrated I assume - maybe that's already
> being
> >>>> done)
> >>>
> >>> Ok. I will take a look at that. But can we instead pass the
> block->used_length
> >> to
> >>> fw_cfg_add_file_callback(). That way we don’t have to change the
> >> qemu_ram_resize()
> >>> as well. I think Igor has suggested this before[1] and I had a go at it 
> >>> before
> >> coming up
> >>> with the "req_length" proposal here.
> >>
> >> You mean, passing the old size as well? I don't see how that will solve
> >> the issue, but yeah, nothing speaks against simply sending the old and
> >> the new size.
> >
> > Nope. I actually meant using the block->used_length to store in the
> > s->files->f[index].size.
> >
> > virt_acpi_setup()
> >   acpi_add_rom_blob()
> >     rom_add_blob()
> >       rom_set_mr()  --> used_length  = page aligned blob size
> >         fw_cfg_add_file_callback()  --> uses actual blob size.
> >
> >
> > Right now what we do is use the actual blob size to store in FWCfgEntry.
> > Instead pass the RAMBlock used_length to fw_cfg_add_file_callback().
> > Of course by this, the firmware will see an aligned size, but that is fine 
> > I think.
> > But at the same time this means the qemu_ram_resize() can stay as it is
> > because it will invoke the callback when the size changes beyond the aligned
> > page size. And also during migration, there won't be any inconsistency as
> everyone
> > works on aligned page size.
> >
> > Does that make sense? Or I am again missing something here?
> 
> Oh, you mean simply rounding up to full pages in the fw entries? If we
> can drop the "sub-page" restriction, that would be awesome!
> 
> Need to double check if that could be an issue for fw/migration/whatever.

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#L85

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,

diff --git a/hw/core/loader.c b/hw/core/loader.c
index d1b78f60cd..f07f6a7a35 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -60,6 +60,7 @@
 #include "hw/boards.h"
 #include "qemu/cutils.h"
 #include "sysemu/runstate.h"
+#include "hw/acpi/aml-build.h"
 
 #include <zlib.h>
 
@@ -1056,6 +1057,7 @@ MemoryRegion *rom_add_blob(const char *name, const void 
*blob, size_t len,
     if (fw_file_name && fw_cfg) {
         char devpath[100];
         void *data;
+        size_t size = rom->datasize;
 
         if (read_only) {
             snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
@@ -1066,13 +1068,21 @@ MemoryRegion *rom_add_blob(const char *name, const void 
*blob, size_t len,
         if (mc->rom_file_has_mr) {
             data = rom_set_mr(rom, OBJECT(fw_cfg), devpath, read_only);
             mr = rom->mr;
+           /*
+            * Use the RAMblk used_length for acpi tables as this avoids any
+            * inconsistency with table size changes during hot add and during
+            * migration.
+            */
+           if (strcmp(fw_file_name, ACPI_BUILD_TABLE_FILE) == 0) {
+                size = memory_region_get_used_length(mr);
+           }
         } else {
             data = rom->data;
         }
 
         fw_cfg_add_file_callback(fw_cfg, fw_file_name,
                                  fw_callback, NULL, callback_opaque,
-                                 data, rom->datasize, read_only);
+                                 data, size, read_only);
     }
     return mr;
 }


Thoughts?

Thanks,
Shameer