Hi Igor/ Michael,
> -----Original Message-----
> From: Linuxarm [mailto:linuxarm-boun...@huawei.com] On Behalf Of
> Shameerali Kolothum Thodi
> Sent: 11 November 2019 12:47
> To: Igor Mammedov <imamm...@redhat.com>
> Cc: peter.mayd...@linaro.org; shannon.zha...@gmail.com; Michael S. Tsirkin
> <m...@redhat.com>; qemu-devel@nongnu.org; Linuxarm
> <linux...@huawei.com>; eric.au...@redhat.com; qemu-...@nongnu.org;
> xuwei (O) <xuw...@huawei.com>; ler...@redhat.com
> Subject: RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
>
> 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?
>
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--
