RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size

[Shameerali Kolothum Thodi]

Hi Igor/ Michael,

> -----Original Message-----
> From: Linuxarm [mailto:address@hidden] On Behalf Of
> Shameerali Kolothum Thodi
> Sent: 11 November 2019 12:47
> To: Igor Mammedov <address@hidden>
> Cc: address@hidden; address@hidden; Michael S. Tsirkin
> <address@hidden>; address@hidden; Linuxarm
> <address@hidden>; address@hidden; address@hidden;
> xuwei (O) <address@hidden>; address@hidden
> Subject: RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
> 
> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:address@hidden]
> > Sent: 08 November 2019 16:18
> > To: Shameerali Kolothum Thodi <address@hidden>
> > Cc: address@hidden; address@hidden;
> > address@hidden; address@hidden;
> > address@hidden; xuwei (O) <address@hidden>;
> > address@hidden; Linuxarm <address@hidden>; Michael S. Tsirkin
> > <address@hidden>
> > Subject: Re: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
> >
> > On Fri, 4 Oct 2019 16:52:58 +0100
> > 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 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 align ACPI blob sizes
> > > to PAGE boundary.
> > >
> > > Signed-off-by: Shameer Kolothum
> <address@hidden>
> > > ---
> > > More details on this issue can be found here,
> > > https://patchwork.kernel.org/patch/11154757/
> > re-read it again and it seems to me that this patch is workaround
> > rather than a solution to the problem.
> 
> Thanks for taking a look at this. Yes, I was also not very sure about this
> approach
> as the root cause of the issue is in qemu_ram_resize().
> 
> > CCing Michael as an author this code.
> > on x86 we have crazy history of manually aligning acpi blobs, see code under
> > comment
> >
> >   /* We'll expose it all to Guest so we want to reduce
> >
> > so used_length endups with over-sized value which includes table and
> padding
> > and it happens that ACPI_BUILD_TABLE_SIZE is much bigger than host page
> > size
> > so if on reboot we happen to exceed ACPI_BUILD_TABLE_SIZE, the next
> padded
> > table
> > size (used_length) would be  2 x ACPI_BUILD_TABLE_SIZE which doesn't
> > trigger
> >   block->used_length == HOST_PAGE_ALIGN(newsize)
> > condition so fwcfg gets updated value.
> 
> Yes, this is the reason why the issue is not visible on x86.
> 
> >
> > > ---
> > >  hw/arm/virt-acpi-build.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 4cd50175e0..074e0c858e 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -790,6 +790,7 @@ void virt_acpi_build(VirtMachineState *vms,
> > AcpiBuildTables *tables)
> > >      GArray *table_offsets;
> > >      unsigned dsdt, xsdt;
> > >      GArray *tables_blob = tables->table_data;
> > > +    GArray *cmd_blob = tables->linker->cmd_blob;
> > >      MachineState *ms = MACHINE(vms);
> > >
> > >      table_offsets = g_array_new(false, true /* clear */,
> > > @@ -854,6 +855,19 @@ void virt_acpi_build(VirtMachineState *vms,
> > AcpiBuildTables *tables)
> > >          build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> > >      }
> > >
> > > +    /*
> > > +     * Align the ACPI blob lengths to PAGE size so that on ACPI table
> > > +     * regeneration, the length that firmware sees really gets updated
> > > +     * through 'resize' callback in qemu_ram_resize() in the
> > > +     * virt_acpi_build_update() -> acpi_ram_update() ->
> > qemu_ram_resize()
> > > +     * path.
> > > +     */
> > > +    g_array_set_size(tables_blob,
> > > +
> > TARGET_PAGE_ALIGN(acpi_data_len(tables_blob)));
> > here it would depend on TARGET_PAGE_ALIGN vs HOST_PAGE_ALIGN
> relation
> > so depending on host it could flip it's behavior to opposite.
> 
> Ok.
> 
> >
> > one thing we could do is dropping (block->used_length == newsize) condition
> 
> I tried this before and strangely for some reason on reboot path,
> 
> virt_acpi_build_update() is called with build_state being NULL and no
> acpi_ram_update()
> happens. Not sure what causes this behavior when we drop the above
> condition.
> 
> > another is to use value of block->used_length for s->files->f[index].size.
> 
> I just tried this by passing block->used_length to fw_cfg_add_file_callback() 
> .
> This could work for this case. But not sure there will be any corner cases
> and also there isn't any easy way to access the mr->ram_balck->used_length
> from
> hw/core/loader.c.
> 
> >
> > Michael,
> > what's your take in this?
> 

This is how(below) I tried to use the RAMBlock used_length for 
s->files->f[index].size.
As used_length is abstracted here, I had to introduce a new api to retrieve the
same. Please take a look and let me know if there is a better way of achieving 
this.

Thanks.
Shameer


---8---

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 5099f27dc8..e862c8c0e1 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1055,6 +1055,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;
 
         if (read_only) {
             snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name);
@@ -1065,13 +1066,15 @@ 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;
+            size = memory_region_get_used_length(mr);
         } else {
             data = rom->data;
+            size = rom->datasize;
         }
 
         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;
 }
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e499dc215b..c51e6cdb9a 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1584,6 +1584,12 @@ void memory_region_add_subregion_overlap(MemoryRegion 
*mr,
  */
 ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr);
 
+/**
+ * memory_region_get_used_length: Get the used length associated with a memory
+ *                             region
+ */
+ram_addr_t memory_region_get_used_length(MemoryRegion *mr);
+
 uint64_t memory_region_get_alignment(const MemoryRegion *mr);
 /**
  * memory_region_del_subregion: Remove a subregion.
diff --git a/memory.c b/memory.c
index 06484c2bff..d1f60c0c9a 100644
--- a/memory.c
+++ b/memory.c
@@ -2200,6 +2200,11 @@ ram_addr_t memory_region_get_ram_addr(MemoryRegion *mr)
     return mr->ram_block ? mr->ram_block->offset : RAM_ADDR_INVALID;
 }
 
+ram_addr_t memory_region_get_used_length(MemoryRegion *mr)
+{
+    return mr->ram_block ? mr->ram_block->used_length : RAM_ADDR_INVALID;
+}
+
 void memory_region_ram_resize(MemoryRegion *mr, ram_addr_t newsize, Error 
**errp)
 {
     assert(mr->ram_block);
---8--