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