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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 4 Oct 2019 16:52:58 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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 <shameerali.kolothum.thodi@huawei.com>
> ---
> 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);
>  }