From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
To: Igor Mammedov <imammedo@redhat.com>
Cc: "peter.maydell@linaro.org" <peter.maydell@linaro.org>,
"shannon.zhaosl@gmail.com" <shannon.zhaosl@gmail.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
Linuxarm <linuxarm@huawei.com>,
"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, 11 Nov 2019 12:47:15 +0000 [thread overview]
Message-ID: <3ae89f87a5d64f57bea7246772c41301@huawei.com> (raw)
In-Reply-To: <20191108171745.1465295b@redhat.com>
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?
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);
> > }
next prev parent reply other threads:[~2019-11-11 12:48 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 [this message]
2019-12-09 13:04 ` Shameerali Kolothum Thodi
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=3ae89f87a5d64f57bea7246772c41301@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).