Hi Igor, > -----Original Message----- > From: Igor Mammedov [mailto:imamm...@redhat.com] > Sent: 08 November 2019 16:18 > To: Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; > eric.au...@redhat.com; peter.mayd...@linaro.org; > shannon.zha...@gmail.com; xuwei (O) <xuw...@huawei.com>; > ler...@redhat.com; Linuxarm <linux...@huawei.com>; Michael S. Tsirkin > <m...@redhat.com> > 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 <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.
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); > > }
