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

[Shameerali Kolothum Thodi]

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?

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