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

Commit Message

Shameerali Kolothum Thodi Oct. 4, 2019, 3:52 p.m. UTC

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 <[e-mail]>
---
More details on this issue can be found here,
https://patchwork.kernel.org/patch/11154757/

---
 hw/arm/virt-acpi-build.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Igor Mammedov Nov. 8, 2019, 4:17 p.m. UTC | #1

On Fri, 4 Oct 2019 16:52:58 +0100
Shameer Kolothum <[e-mail]> 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 <[e-mail]>
> ---
> 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.
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.


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

one thing we could do is dropping (block->used_length == newsize) condition
another is to use value of block->used_length for s->files->f[index].size.

Michael,
what's your take in this?

> +    g_array_set_size(tables->rsdp,
> +                     TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
> +    g_array_set_size(cmd_blob,
> +                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
>      /* Cleanup memory that's no longer used. */
>      g_array_free(table_offsets, true);
>  }

Shameerali Kolothum Thodi Nov. 11, 2019, 12:47 p.m. UTC | #2

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: 08 November 2019 16:18
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; xuwei (O) <[e-mail]>;
> [e-mail]; Linuxarm <[e-mail]>; Michael S. Tsirkin
> <[e-mail]>
> 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 <[e-mail]> 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 <[e-mail]>
> > ---
> > 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?

Thanks,
Shameer

> 
> > +    g_array_set_size(tables->rsdp,
> > +
> TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
> > +    g_array_set_size(cmd_blob,
> > +                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
> >      /* Cleanup memory that's no longer used. */
> >      g_array_free(table_offsets, true);
> >  }

Shameerali Kolothum Thodi Dec. 9, 2019, 1:04 p.m. UTC | #3

Hi Igor/ Michael,

> -----Original Message-----
> From: Linuxarm [mailto:[e-mail]] On Behalf Of
> Shameerali Kolothum Thodi
> Sent: 11 November 2019 12:47
> To: Igor Mammedov <[e-mail]>
> Cc: [e-mail]; [e-mail]; Michael S. Tsirkin
> <[e-mail]>; [e-mail]; Linuxarm
> <[e-mail]>; [e-mail]; [e-mail];
> xuwei (O) <[e-mail]>; [e-mail]
> Subject: RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
> 
> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:[e-mail]]
> > Sent: 08 November 2019 16:18
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; xuwei (O) <[e-mail]>;
> > [e-mail]; Linuxarm <[e-mail]>; Michael S. Tsirkin
> > <[e-mail]>
> > 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 <[e-mail]> 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
> <[e-mail]>
> > > ---
> > > 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--

Patch

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)));
+    g_array_set_size(tables->rsdp,
+                     TARGET_PAGE_ALIGN(acpi_data_len(tables->rsdp)));
+    g_array_set_size(cmd_blob,
+                     TARGET_PAGE_ALIGN(acpi_data_len(cmd_blob)));
     /* Cleanup memory that's no longer used. */
     g_array_free(table_offsets, true);
 }