From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
To: Igor Mammedov <imammedo@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>
Cc: "peter.maydell@linaro.org" <peter.maydell@linaro.org>,
"shannon.zhaosl@gmail.com" <shannon.zhaosl@gmail.com>,
Linuxarm <linuxarm@huawei.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
"eric.auger@redhat.com" <eric.auger@redhat.com>,
"qemu-arm@nongnu.org" <qemu-arm@nongnu.org>,
"xuwei \(O\)" <xuwei5@huawei.com>,
"lersek@redhat.com" <lersek@redhat.com>
Subject: RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
Date: Mon, 9 Dec 2019 13:04:51 +0000 [thread overview]
Message-ID: <323aa74a92934b6a989e6e4dbe0dfe21@huawei.com> (raw)
In-Reply-To: <3ae89f87a5d64f57bea7246772c41301@huawei.com>
Hi Igor/ Michael,
> -----Original Message-----
> From: Linuxarm [mailto:linuxarm-bounces@huawei.com] On Behalf Of
> Shameerali Kolothum Thodi
> Sent: 11 November 2019 12:47
> To: Igor Mammedov <imammedo@redhat.com>
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com; Michael S. Tsirkin
> <mst@redhat.com>; qemu-devel@nongnu.org; Linuxarm
> <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; lersek@redhat.com
> Subject: RE: [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size
>
> Hi Igor,
>
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 08 November 2019 16:18
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org;
> > shannon.zhaosl@gmail.com; xuwei (O) <xuwei5@huawei.com>;
> > lersek@redhat.com; Linuxarm <linuxarm@huawei.com>; Michael S. Tsirkin
> > <mst@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.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.
>
> 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--
next prev parent reply other threads:[~2019-12-09 13:06 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-04 15:52 [PATCH 0/5] ARM virt: Add NVDIMM support Shameer Kolothum
2019-10-04 15:52 ` [PATCH 1/5] hw/arm: Align ACPI blob len to PAGE size Shameer Kolothum
2019-11-08 16:17 ` Igor Mammedov
2019-11-11 12:47 ` Shameerali Kolothum Thodi
2019-12-09 13:04 ` Shameerali Kolothum Thodi [this message]
2019-10-04 15:52 ` [PATCH 2/5] nvdimm: Use configurable ACPI IO base and size Shameer Kolothum
2019-11-11 14:24 ` Igor Mammedov
2019-10-04 15:53 ` [PATCH 3/5] hw/arm/virt: Add nvdimm hot-plug infrastructure Shameer Kolothum
2019-11-11 14:38 ` Igor Mammedov
2019-10-04 15:53 ` [PATCH 4/5] hw/arm/boot: Expose the pmem nodes in the DT Shameer Kolothum
2019-11-11 14:46 ` Igor Mammedov
2019-10-04 15:53 ` [PATCH 5/5] hw/arm/virt: Add nvdimm hotplug support Shameer Kolothum
2019-11-12 13:01 ` Igor Mammedov
2019-10-18 16:39 ` [PATCH 0/5] ARM virt: Add NVDIMM support Auger Eric
2019-10-22 14:05 ` Shameerali Kolothum Thodi
2019-11-25 13:20 ` Shameerali Kolothum Thodi
2019-11-25 15:45 ` Igor Mammedov
2019-11-25 16:25 ` Shameerali Kolothum Thodi
2019-11-26 8:56 ` Igor Mammedov
2019-11-26 9:46 ` Andrew Jones
2019-11-28 12:36 ` Shameerali Kolothum Thodi
2019-12-09 17:39 ` Shameerali Kolothum Thodi
2019-12-11 7:57 ` Igor Mammedov
2019-12-13 12:52 ` Shameerali Kolothum Thodi
2020-01-06 17:06 ` Shameerali Kolothum Thodi
2020-01-09 17:13 ` Igor Mammedov
2020-01-13 13:11 ` Shameerali Kolothum Thodi
2019-11-12 14:39 ` Igor Mammedov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=323aa74a92934b6a989e6e4dbe0dfe21@huawei.com \
--to=shameerali.kolothum.thodi@huawei.com \
--cc=eric.auger@redhat.com \
--cc=imammedo@redhat.com \
--cc=lersek@redhat.com \
--cc=linuxarm@huawei.com \
--cc=mst@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=shannon.zhaosl@gmail.com \
--cc=xuwei5@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).