* [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable
2019-01-28 11:05 [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support Shameer Kolothum
@ 2019-01-28 11:05 ` Shameer Kolothum
2019-02-27 16:27 ` Igor Mammedov
2019-02-27 17:52 ` Auger Eric
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 2/4] hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support Shameer Kolothum
` (2 subsequent siblings)
3 siblings, 2 replies; 36+ messages in thread
From: Shameer Kolothum @ 2019-01-28 11:05 UTC (permalink / raw)
To: eric.auger, shannon.zhaosl, peter.maydell, imammedo, qemu-devel,
qemu-arm
Cc: xuwei5, linuxarm
This is in preparation for adding support for ARM64 platforms
where it doesn't use port mapped IO for ACPI IO space.
Also add a flag to identify hw reduced ACPI platforms as they
might use GPIO hw for signaling ACPI platform events.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
hw/acpi/memory_hotplug.c | 13 ++++++++-----
hw/i386/acpi-build.c | 3 ++-
include/hw/acpi/memory_hotplug.h | 6 ++++--
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 921cad2..05f1d45 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -34,7 +34,7 @@
#define MEMORY_HOTPLUG_IO_LEN 24
#define MEMORY_DEVICES_CONTAINER "\\_SB.MHPC"
-static uint16_t memhp_io_base;
+static hwaddr memhp_io_base;
static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
{
@@ -208,7 +208,7 @@ static const MemoryRegionOps acpi_memory_hotplug_ops = {
};
void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
- MemHotplugState *state, uint16_t io_base)
+ MemHotplugState *state, hwaddr io_base)
{
MachineState *machine = MACHINE(qdev_get_machine());
@@ -279,7 +279,9 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
mdev->is_enabled = true;
if (dev->hotplugged) {
mdev->is_inserting = true;
- acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
+ if (!mem_st->hw_reduced_acpi) {
+ acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
+ }
}
}
@@ -341,7 +343,8 @@ const VMStateDescription vmstate_memory_hotplug = {
void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
const char *res_root,
- const char *event_handler_method)
+ const char *event_handler_method,
+ AmlRegionSpace rs)
{
int i;
Aml *ifctx;
@@ -371,7 +374,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
aml_append(mem_ctrl_dev, aml_name_decl("_CRS", crs));
aml_append(mem_ctrl_dev, aml_operation_region(
- MEMORY_HOTPLUG_IO_REGION, AML_SYSTEM_IO,
+ MEMORY_HOTPLUG_IO_REGION, rs,
aml_int(memhp_io_base), MEMORY_HOTPLUG_IO_LEN)
);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2e21a31..b62c1a3 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1852,7 +1852,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
"\\_SB.PCI0", "\\_GPE._E02");
}
- build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0", "\\_GPE._E03");
+ build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
+ "\\_GPE._E03", AML_SYSTEM_IO);
scope = aml_scope("_GPE");
{
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index 77c6576..ec56579 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -26,10 +26,11 @@ typedef struct MemHotplugState {
uint32_t selector;
uint32_t dev_count;
MemStatus *devs;
+ bool hw_reduced_acpi; /*true for hw reduced acpi platform */
} MemHotplugState;
void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
- MemHotplugState *state, uint16_t io_base);
+ MemHotplugState *state, hwaddr io_base);
void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
DeviceState *dev, Error **errp);
@@ -48,5 +49,6 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list);
void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
const char *res_root,
- const char *event_handler_method);
+ const char *event_handler_method,
+ AmlRegionSpace rs);
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable Shameer Kolothum
@ 2019-02-27 16:27 ` Igor Mammedov
2019-02-28 12:14 ` Shameerali Kolothum Thodi
2019-02-27 17:52 ` Auger Eric
1 sibling, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2019-02-27 16:27 UTC (permalink / raw)
To: Shameer Kolothum
Cc: eric.auger, shannon.zhaosl, peter.maydell, qemu-devel, qemu-arm,
xuwei5, linuxarm
On Mon, 28 Jan 2019 11:05:43 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> This is in preparation for adding support for ARM64 platforms
> where it doesn't use port mapped IO for ACPI IO space.
>
> Also add a flag to identify hw reduced ACPI platforms as they
> might use GPIO hw for signaling ACPI platform events.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/acpi/memory_hotplug.c | 13 ++++++++-----
> hw/i386/acpi-build.c | 3 ++-
> include/hw/acpi/memory_hotplug.h | 6 ++++--
> 3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 921cad2..05f1d45 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -34,7 +34,7 @@
> #define MEMORY_HOTPLUG_IO_LEN 24
> #define MEMORY_DEVICES_CONTAINER "\\_SB.MHPC"
>
> -static uint16_t memhp_io_base;
> +static hwaddr memhp_io_base;
>
> static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
> {
> @@ -208,7 +208,7 @@ static const MemoryRegionOps acpi_memory_hotplug_ops = {
> };
>
> void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> - MemHotplugState *state, uint16_t io_base)
> + MemHotplugState *state, hwaddr io_base)
> {
> MachineState *machine = MACHINE(qdev_get_machine());
>
> @@ -279,7 +279,9 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
> mdev->is_enabled = true;
> if (dev->hotplugged) {
> mdev->is_inserting = true;
> - acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
> + if (!mem_st->hw_reduced_acpi) {
> + acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
> + }
Why do you restrict it to non hw_reduced_acpi?
If anything else, I'd expect virt board (or hotplug controller (GED or whatever)
implement AcpiDeviceIfClass and implement arm/virt board specific call backs)
then you won't need duplicate and carry hw_reduced_acpi in generic code.
The rest of the patch looks good.
> }
> }
>
> @@ -341,7 +343,8 @@ const VMStateDescription vmstate_memory_hotplug = {
>
> void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> const char *res_root,
> - const char *event_handler_method)
> + const char *event_handler_method,
> + AmlRegionSpace rs)
> {
> int i;
> Aml *ifctx;
> @@ -371,7 +374,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> aml_append(mem_ctrl_dev, aml_name_decl("_CRS", crs));
>
> aml_append(mem_ctrl_dev, aml_operation_region(
> - MEMORY_HOTPLUG_IO_REGION, AML_SYSTEM_IO,
> + MEMORY_HOTPLUG_IO_REGION, rs,
> aml_int(memhp_io_base), MEMORY_HOTPLUG_IO_LEN)
> );
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2e21a31..b62c1a3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1852,7 +1852,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> "\\_SB.PCI0", "\\_GPE._E02");
> }
> - build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0", "\\_GPE._E03");
> + build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> + "\\_GPE._E03", AML_SYSTEM_IO);
>
> scope = aml_scope("_GPE");
> {
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index 77c6576..ec56579 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -26,10 +26,11 @@ typedef struct MemHotplugState {
> uint32_t selector;
> uint32_t dev_count;
> MemStatus *devs;
> + bool hw_reduced_acpi; /*true for hw reduced acpi platform */
> } MemHotplugState;
>
> void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> - MemHotplugState *state, uint16_t io_base);
> + MemHotplugState *state, hwaddr io_base);
>
> void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
> DeviceState *dev, Error **errp);
> @@ -48,5 +49,6 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list);
>
> void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> const char *res_root,
> - const char *event_handler_method);
> + const char *event_handler_method,
> + AmlRegionSpace rs);
> #endif
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable
2019-02-27 16:27 ` Igor Mammedov
@ 2019-02-28 12:14 ` Shameerali Kolothum Thodi
0 siblings, 0 replies; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-02-28 12:14 UTC (permalink / raw)
To: Igor Mammedov
Cc: eric.auger@redhat.com, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, xuwei (O), Linuxarm
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 27 February 2019 16:28
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: eric.auger@redhat.com; shannon.zhaosl@gmail.com;
> peter.maydell@linaro.org; qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space
> configurable
>
> On Mon, 28 Jan 2019 11:05:43 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > This is in preparation for adding support for ARM64 platforms where it
> > doesn't use port mapped IO for ACPI IO space.
> >
> > Also add a flag to identify hw reduced ACPI platforms as they might
> > use GPIO hw for signaling ACPI platform events.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > hw/acpi/memory_hotplug.c | 13 ++++++++-----
> > hw/i386/acpi-build.c | 3 ++-
> > include/hw/acpi/memory_hotplug.h | 6 ++++--
> > 3 files changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index
> > 921cad2..05f1d45 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -34,7 +34,7 @@
> > #define MEMORY_HOTPLUG_IO_LEN 24
> > #define MEMORY_DEVICES_CONTAINER "\\_SB.MHPC"
> >
> > -static uint16_t memhp_io_base;
> > +static hwaddr memhp_io_base;
> >
> > static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus
> > *mdev) { @@ -208,7 +208,7 @@ static const MemoryRegionOps
> > acpi_memory_hotplug_ops = { };
> >
> > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> > - MemHotplugState *state, uint16_t
> io_base)
> > + MemHotplugState *state, hwaddr
> io_base)
> > {
> > MachineState *machine = MACHINE(qdev_get_machine());
> >
> > @@ -279,7 +279,9 @@ void acpi_memory_plug_cb(HotplugHandler
> *hotplug_dev, MemHotplugState *mem_st,
> > mdev->is_enabled = true;
> > if (dev->hotplugged) {
> > mdev->is_inserting = true;
> > - acpi_send_event(DEVICE(hotplug_dev),
> ACPI_MEMORY_HOTPLUG_STATUS);
> > + if (!mem_st->hw_reduced_acpi) {
> > + acpi_send_event(DEVICE(hotplug_dev),
> ACPI_MEMORY_HOTPLUG_STATUS);
> > + }
> Why do you restrict it to non hw_reduced_acpi?
>
> If anything else, I'd expect virt board (or hotplug controller (GED or whatever)
> implement AcpiDeviceIfClass and implement arm/virt board specific call backs)
Right. Since this was based on GPIO I was keeping it simple and didn't attempt
to take the AcpiDeviceIfClass path. I will address that with GED.
> then you won't need duplicate and carry hw_reduced_acpi in generic code.
>
> The rest of the patch looks good.
Thanks,
Shameer
> > }
> > }
> >
> > @@ -341,7 +343,8 @@ const VMStateDescription
> vmstate_memory_hotplug =
> > {
> >
> > void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> > const char *res_root,
> > - const char *event_handler_method)
> > + const char *event_handler_method,
> > + AmlRegionSpace rs)
> > {
> > int i;
> > Aml *ifctx;
> > @@ -371,7 +374,7 @@ void build_memory_hotplug_aml(Aml *table,
> uint32_t nr_mem,
> > aml_append(mem_ctrl_dev, aml_name_decl("_CRS", crs));
> >
> > aml_append(mem_ctrl_dev, aml_operation_region(
> > - MEMORY_HOTPLUG_IO_REGION, AML_SYSTEM_IO,
> > + MEMORY_HOTPLUG_IO_REGION, rs,
> > aml_int(memhp_io_base), MEMORY_HOTPLUG_IO_LEN)
> > );
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index
> > 2e21a31..b62c1a3 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1852,7 +1852,8 @@ build_dsdt(GArray *table_data, BIOSLinker
> *linker,
> > build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > "\\_SB.PCI0", "\\_GPE._E02");
> > }
> > - build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> "\\_GPE._E03");
> > + build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> > + "\\_GPE._E03", AML_SYSTEM_IO);
> >
> > scope = aml_scope("_GPE");
> > {
> > diff --git a/include/hw/acpi/memory_hotplug.h
> > b/include/hw/acpi/memory_hotplug.h
> > index 77c6576..ec56579 100644
> > --- a/include/hw/acpi/memory_hotplug.h
> > +++ b/include/hw/acpi/memory_hotplug.h
> > @@ -26,10 +26,11 @@ typedef struct MemHotplugState {
> > uint32_t selector;
> > uint32_t dev_count;
> > MemStatus *devs;
> > + bool hw_reduced_acpi; /*true for hw reduced acpi platform */
> > } MemHotplugState;
> >
> > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> > - MemHotplugState *state, uint16_t
> io_base);
> > + MemHotplugState *state, hwaddr
> > + io_base);
> >
> > void acpi_memory_plug_cb(HotplugHandler *hotplug_dev,
> MemHotplugState *mem_st,
> > DeviceState *dev, Error **errp); @@ -48,5
> > +49,6 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st,
> > ACPIOSTInfoList ***list);
> >
> > void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> > const char *res_root,
> > - const char *event_handler_method);
> > + const char *event_handler_method,
> > + AmlRegionSpace rs);
> > #endif
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable Shameer Kolothum
2019-02-27 16:27 ` Igor Mammedov
@ 2019-02-27 17:52 ` Auger Eric
2019-02-28 16:09 ` Shameerali Kolothum Thodi
1 sibling, 1 reply; 36+ messages in thread
From: Auger Eric @ 2019-02-27 17:52 UTC (permalink / raw)
To: Shameer Kolothum, shannon.zhaosl, peter.maydell, imammedo,
qemu-devel, qemu-arm
Cc: xuwei5, linuxarm
Hi Shameer,
On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> This is in preparation for adding support for ARM64 platforms
> where it doesn't use port mapped IO for ACPI IO space.
>
> Also add a flag to identify hw reduced ACPI platforms as they
> might use GPIO hw for signaling ACPI platform events.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/acpi/memory_hotplug.c | 13 ++++++++-----
> hw/i386/acpi-build.c | 3 ++-
> include/hw/acpi/memory_hotplug.h | 6 ++++--
> 3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 921cad2..05f1d45 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -34,7 +34,7 @@
> #define MEMORY_HOTPLUG_IO_LEN 24
> #define MEMORY_DEVICES_CONTAINER "\\_SB.MHPC"
>
> -static uint16_t memhp_io_base;
> +static hwaddr memhp_io_base;
>
> static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
> {
> @@ -208,7 +208,7 @@ static const MemoryRegionOps acpi_memory_hotplug_ops = {
> };
>
> void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> - MemHotplugState *state, uint16_t io_base)
> + MemHotplugState *state, hwaddr io_base)
> {
> MachineState *machine = MACHINE(qdev_get_machine());
>
> @@ -279,7 +279,9 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
> mdev->is_enabled = true;
> if (dev->hotplugged) {
> mdev->is_inserting = true;
> - acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
> + if (!mem_st->hw_reduced_acpi) {
> + acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS);
> + }
> }
> }
>
> @@ -341,7 +343,8 @@ const VMStateDescription vmstate_memory_hotplug = {
>
> void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> const char *res_root,
> - const char *event_handler_method)
> + const char *event_handler_method,
> + AmlRegionSpace rs)
> {
> int i;
> Aml *ifctx;
> @@ -371,7 +374,7 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> aml_append(mem_ctrl_dev, aml_name_decl("_CRS", crs));
>
> aml_append(mem_ctrl_dev, aml_operation_region(
> - MEMORY_HOTPLUG_IO_REGION, AML_SYSTEM_IO,
> + MEMORY_HOTPLUG_IO_REGION, rs,
> aml_int(memhp_io_base), MEMORY_HOTPLUG_IO_LEN)
Given the change in the datatype (memhp_io_base) is it OK to keep the
aml_int() cast.
Also we have
"
aml_append(crs,
aml_io(AML_DECODE16, memhp_io_base, memhp_io_base, 0,
MEMORY_HOTPLUG_IO_LEN)
);
" where we have aml_io being used + AML_decode16. Shouldn't we have
aml_*_memory depending on rs?
I am not knowledged enough about the aml description but just in case.
Thanks
Eric
> );
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2e21a31..b62c1a3 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1852,7 +1852,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> "\\_SB.PCI0", "\\_GPE._E02");
> }
> - build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0", "\\_GPE._E03");
> + build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> + "\\_GPE._E03", AML_SYSTEM_IO);
>
> scope = aml_scope("_GPE");
> {
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index 77c6576..ec56579 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -26,10 +26,11 @@ typedef struct MemHotplugState {
> uint32_t selector;
> uint32_t dev_count;
> MemStatus *devs;
> + bool hw_reduced_acpi; /*true for hw reduced acpi platform */
> } MemHotplugState;
>
> void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> - MemHotplugState *state, uint16_t io_base);
> + MemHotplugState *state, hwaddr io_base);
>
> void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st,
> DeviceState *dev, Error **errp);
> @@ -48,5 +49,6 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list);
>
> void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> const char *res_root,
> - const char *event_handler_method);
> + const char *event_handler_method,
> + AmlRegionSpace rs);
> #endif
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable
2019-02-27 17:52 ` Auger Eric
@ 2019-02-28 16:09 ` Shameerali Kolothum Thodi
2019-02-28 16:44 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-02-28 16:09 UTC (permalink / raw)
To: Auger Eric, shannon.zhaosl@gmail.com, peter.maydell@linaro.org,
imammedo@redhat.com, qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm
Hi Eric,
> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 27 February 2019 17:53
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>
> Subject: Re: [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space
> configurable
>
> Hi Shameer,
> On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> > This is in preparation for adding support for ARM64 platforms where it
> > doesn't use port mapped IO for ACPI IO space.
> >
> > Also add a flag to identify hw reduced ACPI platforms as they might
> > use GPIO hw for signaling ACPI platform events.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > hw/acpi/memory_hotplug.c | 13 ++++++++-----
> > hw/i386/acpi-build.c | 3 ++-
> > include/hw/acpi/memory_hotplug.h | 6 ++++--
> > 3 files changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index
> > 921cad2..05f1d45 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -34,7 +34,7 @@
> > #define MEMORY_HOTPLUG_IO_LEN 24
> > #define MEMORY_DEVICES_CONTAINER "\\_SB.MHPC"
> >
> > -static uint16_t memhp_io_base;
> > +static hwaddr memhp_io_base;
> >
> > static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus
> > *mdev) { @@ -208,7 +208,7 @@ static const MemoryRegionOps
> > acpi_memory_hotplug_ops = { };
> >
> > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> > - MemHotplugState *state, uint16_t
> io_base)
> > + MemHotplugState *state, hwaddr
> io_base)
> > {
> > MachineState *machine = MACHINE(qdev_get_machine());
> >
> > @@ -279,7 +279,9 @@ void acpi_memory_plug_cb(HotplugHandler
> *hotplug_dev, MemHotplugState *mem_st,
> > mdev->is_enabled = true;
> > if (dev->hotplugged) {
> > mdev->is_inserting = true;
> > - acpi_send_event(DEVICE(hotplug_dev),
> ACPI_MEMORY_HOTPLUG_STATUS);
> > + if (!mem_st->hw_reduced_acpi) {
> > + acpi_send_event(DEVICE(hotplug_dev),
> ACPI_MEMORY_HOTPLUG_STATUS);
> > + }
> > }
> > }
> >
> > @@ -341,7 +343,8 @@ const VMStateDescription
> vmstate_memory_hotplug =
> > {
> >
> > void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> > const char *res_root,
> > - const char *event_handler_method)
> > + const char *event_handler_method,
> > + AmlRegionSpace rs)
> > {
> > int i;
> > Aml *ifctx;
> > @@ -371,7 +374,7 @@ void build_memory_hotplug_aml(Aml *table,
> uint32_t nr_mem,
> > aml_append(mem_ctrl_dev, aml_name_decl("_CRS", crs));
> >
> > aml_append(mem_ctrl_dev, aml_operation_region(
> > - MEMORY_HOTPLUG_IO_REGION, AML_SYSTEM_IO,
> > + MEMORY_HOTPLUG_IO_REGION, rs,
> > aml_int(memhp_io_base), MEMORY_HOTPLUG_IO_LEN)
> Given the change in the datatype (memhp_io_base) is it OK to keep the
> aml_int() cast.
Hmm...aml_int() has uint64_t but not sure hwaddr will cause any unwarranted effects on
other platforms. Do you see any potential issues here?
> Also we have
> "
> aml_append(crs,
> aml_io(AML_DECODE16, memhp_io_base, memhp_io_base, 0,
> MEMORY_HOTPLUG_IO_LEN)
> );
> " where we have aml_io being used + AML_decode16. Shouldn't we have
> aml_*_memory depending on rs?
That looks like a valid point, but then I wonder how this worked. I will double check.
> I am not knowledged enough about the aml description but just in case.
Same here :).
Thanks,
Shameer
> Thanks
>
> Eric
>
> > );
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index
> > 2e21a31..b62c1a3 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1852,7 +1852,8 @@ build_dsdt(GArray *table_data, BIOSLinker
> *linker,
> > build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > "\\_SB.PCI0", "\\_GPE._E02");
> > }
> > - build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> "\\_GPE._E03");
> > + build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> > + "\\_GPE._E03", AML_SYSTEM_IO);
> >
> > scope = aml_scope("_GPE");
> > {
> > diff --git a/include/hw/acpi/memory_hotplug.h
> > b/include/hw/acpi/memory_hotplug.h
> > index 77c6576..ec56579 100644
> > --- a/include/hw/acpi/memory_hotplug.h
> > +++ b/include/hw/acpi/memory_hotplug.h
> > @@ -26,10 +26,11 @@ typedef struct MemHotplugState {
> > uint32_t selector;
> > uint32_t dev_count;
> > MemStatus *devs;
> > + bool hw_reduced_acpi; /*true for hw reduced acpi platform */
> > } MemHotplugState;
> >
> > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> > - MemHotplugState *state, uint16_t
> io_base);
> > + MemHotplugState *state, hwaddr
> > + io_base);
> >
> > void acpi_memory_plug_cb(HotplugHandler *hotplug_dev,
> MemHotplugState *mem_st,
> > DeviceState *dev, Error **errp); @@ -48,5
> > +49,6 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st,
> > ACPIOSTInfoList ***list);
> >
> > void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> > const char *res_root,
> > - const char *event_handler_method);
> > + const char *event_handler_method,
> > + AmlRegionSpace rs);
> > #endif
> >
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable
2019-02-28 16:09 ` Shameerali Kolothum Thodi
@ 2019-02-28 16:44 ` Igor Mammedov
0 siblings, 0 replies; 36+ messages in thread
From: Igor Mammedov @ 2019-02-28 16:44 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: Auger Eric, shannon.zhaosl@gmail.com, peter.maydell@linaro.org,
qemu-devel@nongnu.org, qemu-arm@nongnu.org, xuwei (O), Linuxarm
On Thu, 28 Feb 2019 16:09:48 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> Hi Eric,
>
> > -----Original Message-----
> > From: Auger Eric [mailto:eric.auger@redhat.com]
> > Sent: 27 February 2019 17:53
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> > shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> > imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> > Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>
> > Subject: Re: [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space
> > configurable
> >
> > Hi Shameer,
> > On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> > > This is in preparation for adding support for ARM64 platforms where it
> > > doesn't use port mapped IO for ACPI IO space.
> > >
> > > Also add a flag to identify hw reduced ACPI platforms as they might
> > > use GPIO hw for signaling ACPI platform events.
> > >
> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > > hw/acpi/memory_hotplug.c | 13 ++++++++-----
> > > hw/i386/acpi-build.c | 3 ++-
> > > include/hw/acpi/memory_hotplug.h | 6 ++++--
> > > 3 files changed, 14 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index
> > > 921cad2..05f1d45 100644
> > > --- a/hw/acpi/memory_hotplug.c
> > > +++ b/hw/acpi/memory_hotplug.c
> > > @@ -34,7 +34,7 @@
> > > #define MEMORY_HOTPLUG_IO_LEN 24
> > > #define MEMORY_DEVICES_CONTAINER "\\_SB.MHPC"
> > >
> > > -static uint16_t memhp_io_base;
> > > +static hwaddr memhp_io_base;
> > >
> > > static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus
> > > *mdev) { @@ -208,7 +208,7 @@ static const MemoryRegionOps
> > > acpi_memory_hotplug_ops = { };
> > >
> > > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> > > - MemHotplugState *state, uint16_t
> > io_base)
> > > + MemHotplugState *state, hwaddr
> > io_base)
> > > {
> > > MachineState *machine = MACHINE(qdev_get_machine());
> > >
> > > @@ -279,7 +279,9 @@ void acpi_memory_plug_cb(HotplugHandler
> > *hotplug_dev, MemHotplugState *mem_st,
> > > mdev->is_enabled = true;
> > > if (dev->hotplugged) {
> > > mdev->is_inserting = true;
> > > - acpi_send_event(DEVICE(hotplug_dev),
> > ACPI_MEMORY_HOTPLUG_STATUS);
> > > + if (!mem_st->hw_reduced_acpi) {
> > > + acpi_send_event(DEVICE(hotplug_dev),
> > ACPI_MEMORY_HOTPLUG_STATUS);
> > > + }
> > > }
> > > }
> > >
> > > @@ -341,7 +343,8 @@ const VMStateDescription
> > vmstate_memory_hotplug =
> > > {
> > >
> > > void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> > > const char *res_root,
> > > - const char *event_handler_method)
> > > + const char *event_handler_method,
> > > + AmlRegionSpace rs)
> > > {
> > > int i;
> > > Aml *ifctx;
> > > @@ -371,7 +374,7 @@ void build_memory_hotplug_aml(Aml *table,
> > uint32_t nr_mem,
> > > aml_append(mem_ctrl_dev, aml_name_decl("_CRS", crs));
> > >
> > > aml_append(mem_ctrl_dev, aml_operation_region(
> > > - MEMORY_HOTPLUG_IO_REGION, AML_SYSTEM_IO,
> > > + MEMORY_HOTPLUG_IO_REGION, rs,
> > > aml_int(memhp_io_base), MEMORY_HOTPLUG_IO_LEN)
> > Given the change in the datatype (memhp_io_base) is it OK to keep the
> > aml_int() cast.
>
> Hmm...aml_int() has uint64_t but not sure hwaddr will cause any unwarranted effects on
> other platforms. Do you see any potential issues here?
aml_int encodes it according to the value so it should e fine
>
> > Also we have
> > "
> > aml_append(crs,
> > aml_io(AML_DECODE16, memhp_io_base, memhp_io_base, 0,
> > MEMORY_HOTPLUG_IO_LEN)
> > );
> > " where we have aml_io being used + AML_decode16. Shouldn't we have
> > aml_*_memory depending on rs?
>
> That looks like a valid point, but then I wonder how this worked. I will double check.
maybe use aml_memory32_fixed() or aml_[dq]word_memory()
> > I am not knowledged enough about the aml description but just in case.
>
> Same here :).
>
> Thanks,
> Shameer
>
>
> > Thanks
> >
> > Eric
> >
> > > );
> > >
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index
> > > 2e21a31..b62c1a3 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -1852,7 +1852,8 @@ build_dsdt(GArray *table_data, BIOSLinker
> > *linker,
> > > build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
> > > "\\_SB.PCI0", "\\_GPE._E02");
> > > }
> > > - build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> > "\\_GPE._E03");
> > > + build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> > > + "\\_GPE._E03", AML_SYSTEM_IO);
> > >
> > > scope = aml_scope("_GPE");
> > > {
> > > diff --git a/include/hw/acpi/memory_hotplug.h
> > > b/include/hw/acpi/memory_hotplug.h
> > > index 77c6576..ec56579 100644
> > > --- a/include/hw/acpi/memory_hotplug.h
> > > +++ b/include/hw/acpi/memory_hotplug.h
> > > @@ -26,10 +26,11 @@ typedef struct MemHotplugState {
> > > uint32_t selector;
> > > uint32_t dev_count;
> > > MemStatus *devs;
> > > + bool hw_reduced_acpi; /*true for hw reduced acpi platform */
> > > } MemHotplugState;
> > >
> > > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner,
> > > - MemHotplugState *state, uint16_t
> > io_base);
> > > + MemHotplugState *state, hwaddr
> > > + io_base);
> > >
> > > void acpi_memory_plug_cb(HotplugHandler *hotplug_dev,
> > MemHotplugState *mem_st,
> > > DeviceState *dev, Error **errp); @@ -48,5
> > > +49,6 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st,
> > > ACPIOSTInfoList ***list);
> > >
> > > void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
> > > const char *res_root,
> > > - const char *event_handler_method);
> > > + const char *event_handler_method,
> > > + AmlRegionSpace rs);
> > > #endif
> > >
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [RFC PATCH 2/4] hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
2019-01-28 11:05 [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support Shameer Kolothum
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable Shameer Kolothum
@ 2019-01-28 11:05 ` Shameer Kolothum
2019-02-27 16:44 ` Igor Mammedov
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support Shameer Kolothum
2019-02-22 16:03 ` [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory " Auger Eric
3 siblings, 1 reply; 36+ messages in thread
From: Shameer Kolothum @ 2019-01-28 11:05 UTC (permalink / raw)
To: eric.auger, shannon.zhaosl, peter.maydell, imammedo, qemu-devel,
qemu-arm
Cc: xuwei5, linuxarm
This adds support for using PL061 GPIO controller pin to trigger
pcdimm hotplug event to guest.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
default-configs/arm-softmmu.mak | 1 +
hw/arm/virt-acpi-build.c | 28 ++++++++++++++++++++++++----
hw/arm/virt.c | 37 +++++++++++++++++++++++++++++++++----
include/hw/arm/virt.h | 14 ++++++++++++++
4 files changed, 72 insertions(+), 8 deletions(-)
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 5deabc1..ebbc67b 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -163,3 +163,4 @@ CONFIG_MEM_DEVICE=y
CONFIG_DIMM=y
CONFIG_NVDIMM=y
CONFIG_ACPI_NVDIMM=y
+CONFIG_ACPI_MEMORY_HOTPLUG=y
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index ab30e28..eedd323 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -40,6 +40,7 @@
#include "hw/loader.h"
#include "hw/hw.h"
#include "hw/acpi/aml-build.h"
+#include "hw/acpi/memory_hotplug.h"
#include "hw/pci/pcie_host.h"
#include "hw/pci/pci.h"
#include "hw/arm/virt.h"
@@ -327,8 +328,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
}
static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
- uint32_t gpio_irq)
+ uint32_t gpio_irq, VirtMachineState *vms)
{
+ uint32_t pin_list[1];
+
Aml *dev = aml_device("GPO0");
aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
@@ -342,11 +345,21 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
aml_append(dev, aml_name_decl("_CRS", crs));
Aml *aei = aml_resource_template();
- /* Pin 3 for power button */
- const uint32_t pin_list[1] = {3};
+
+ /* GPIO Interrupt connection descriptor for power button */
+ pin_list[0] = GPIO_PWRB;
aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
"GPO0", NULL, 0));
+
+ if (vms->acpi_memhp_state.is_enabled) {
+ /* GPIO Interrupt connection descriptor for pc-dimm hotplug */
+ pin_list[0] = GPIO_PCDIMM;
+ aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
+ AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list,
+ 1, "GPO0", NULL, 0));
+ }
+
aml_append(dev, aml_name_decl("_AEI", aei));
/* _E03 is handle for power button */
@@ -735,6 +748,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
Aml *scope, *dsdt;
const MemMapEntry *memmap = vms->memmap;
const int *irqmap = vms->irqmap;
+ MachineState *ms = MACHINE(vms);
+ uint32_t nr_mem = ms->ram_slots;
dsdt = init_aml_allocator();
/* Reserve space for header */
@@ -756,11 +771,16 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
vms->highmem, vms->highmem_ecam);
acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
- (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
+ (irqmap[VIRT_GPIO] + ARM_SPI_BASE), vms);
acpi_dsdt_add_power_button(scope);
aml_append(dsdt, scope);
+ if (vms->acpi_memhp_state.is_enabled) {
+ build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.GPO0",
+ "\\_SB.GPO0._E02", AML_SYSTEM_MEMORY);
+ }
+
/* copy AML table into ACPI tables blob and patch header there */
g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len);
build_header(linker, table_data,
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8c6dd59..884960d 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -173,6 +173,9 @@ static const char *valid_cpus[] = {
ARM_CPU_TYPE_NAME("max"),
};
+static QLIST_HEAD(, GPIODevice) gpio_devices =
+ QLIST_HEAD_INITIALIZER(gpio_devices);
+
static bool cpu_type_valid(const char *cpu)
{
int i;
@@ -740,11 +743,34 @@ static void create_rtc(const VirtMachineState *vms, qemu_irq *pic)
g_free(nodename);
}
-static DeviceState *gpio_key_dev;
+static DeviceState *virt_get_gpio_dev(int pin)
+{
+ GPIODevice *gpio_dev;
+
+ QLIST_FOREACH(gpio_dev, &gpio_devices, next) {
+ if (pin == gpio_dev->pin_num) {
+ return gpio_dev->dev;
+ }
+ }
+ return NULL;
+}
+
+static void virt_create_gpio_dev(DeviceState *pl061_dev, int pin)
+{
+ GPIODevice *gpio_dev;
+
+ gpio_dev = g_malloc0(sizeof(*gpio_dev));
+ gpio_dev->dev = sysbus_create_simple("gpio-key", -1,
+ qdev_get_gpio_in(pl061_dev, pin));
+ gpio_dev->pin_num = pin;
+ QLIST_INSERT_HEAD(&gpio_devices, gpio_dev, next);
+}
+
static void virt_powerdown_req(Notifier *n, void *opaque)
{
+ DeviceState *dev = virt_get_gpio_dev(GPIO_PWRB);
/* use gpio Pin 3 for power button event */
- qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
+ qemu_set_irq(qdev_get_gpio_in(dev, 0), 1);
}
static Notifier virt_system_powerdown_notifier = {
@@ -777,8 +803,11 @@ static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
- gpio_key_dev = sysbus_create_simple("gpio-key", -1,
- qdev_get_gpio_in(pl061_dev, 3));
+ virt_create_gpio_dev(pl061_dev, GPIO_PWRB);
+
+ if (vms->acpi_memhp_state.is_enabled) {
+ virt_create_gpio_dev(pl061_dev, GPIO_PCDIMM);
+ }
qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible", "gpio-keys");
qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 6bb7f92..ef39ce6 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -38,6 +38,7 @@
#include "sysemu/kvm.h"
#include "hw/intc/arm_gicv3_common.h"
#include "hw/mem/nvdimm.h"
+#include "hw/acpi/memory_hotplug.h"
#define NUM_GICV2M_SPIS 64
#define NUM_VIRTIO_TRANSPORTS 32
@@ -136,8 +137,21 @@ typedef struct {
hwaddr high_io_base;
bool extended_memmap;
AcpiNVDIMMState acpi_nvdimm_state;
+ MemHotplugState acpi_memhp_state;
} VirtMachineState;
+/* GPIO pins for ACPI events */
+enum {
+ GPIO_PCDIMM = 2,
+ GPIO_PWRB,
+};
+
+typedef struct GPIODevice {
+ DeviceState *dev;
+ int pin_num;
+ QLIST_ENTRY(GPIODevice) next;
+} GPIODevice;
+
#define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
#define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
--
2.7.4
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 2/4] hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 2/4] hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support Shameer Kolothum
@ 2019-02-27 16:44 ` Igor Mammedov
0 siblings, 0 replies; 36+ messages in thread
From: Igor Mammedov @ 2019-02-27 16:44 UTC (permalink / raw)
To: Shameer Kolothum
Cc: eric.auger, shannon.zhaosl, peter.maydell, qemu-devel, qemu-arm,
linuxarm, xuwei5
On Mon, 28 Jan 2019 11:05:44 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> This adds support for using PL061 GPIO controller pin to trigger
> pcdimm hotplug event to guest.
I'll wait for GED reincarnation of this patch.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> default-configs/arm-softmmu.mak | 1 +
> hw/arm/virt-acpi-build.c | 28 ++++++++++++++++++++++++----
> hw/arm/virt.c | 37 +++++++++++++++++++++++++++++++++----
> include/hw/arm/virt.h | 14 ++++++++++++++
> 4 files changed, 72 insertions(+), 8 deletions(-)
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 5deabc1..ebbc67b 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -163,3 +163,4 @@ CONFIG_MEM_DEVICE=y
> CONFIG_DIMM=y
> CONFIG_NVDIMM=y
> CONFIG_ACPI_NVDIMM=y
> +CONFIG_ACPI_MEMORY_HOTPLUG=y
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index ab30e28..eedd323 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -40,6 +40,7 @@
> #include "hw/loader.h"
> #include "hw/hw.h"
> #include "hw/acpi/aml-build.h"
> +#include "hw/acpi/memory_hotplug.h"
> #include "hw/pci/pcie_host.h"
> #include "hw/pci/pci.h"
> #include "hw/arm/virt.h"
> @@ -327,8 +328,10 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
> }
>
> static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
> - uint32_t gpio_irq)
> + uint32_t gpio_irq, VirtMachineState *vms)
> {
> + uint32_t pin_list[1];
> +
> Aml *dev = aml_device("GPO0");
> aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> @@ -342,11 +345,21 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
> aml_append(dev, aml_name_decl("_CRS", crs));
>
> Aml *aei = aml_resource_template();
> - /* Pin 3 for power button */
> - const uint32_t pin_list[1] = {3};
> +
> + /* GPIO Interrupt connection descriptor for power button */
> + pin_list[0] = GPIO_PWRB;
> aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
> "GPO0", NULL, 0));
> +
> + if (vms->acpi_memhp_state.is_enabled) {
> + /* GPIO Interrupt connection descriptor for pc-dimm hotplug */
> + pin_list[0] = GPIO_PCDIMM;
> + aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> + AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list,
> + 1, "GPO0", NULL, 0));
> + }
> +
> aml_append(dev, aml_name_decl("_AEI", aei));
>
> /* _E03 is handle for power button */
> @@ -735,6 +748,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> Aml *scope, *dsdt;
> const MemMapEntry *memmap = vms->memmap;
> const int *irqmap = vms->irqmap;
> + MachineState *ms = MACHINE(vms);
> + uint32_t nr_mem = ms->ram_slots;
>
> dsdt = init_aml_allocator();
> /* Reserve space for header */
> @@ -756,11 +771,16 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
> vms->highmem, vms->highmem_ecam);
> acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
> - (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
> + (irqmap[VIRT_GPIO] + ARM_SPI_BASE), vms);
> acpi_dsdt_add_power_button(scope);
>
> aml_append(dsdt, scope);
>
> + if (vms->acpi_memhp_state.is_enabled) {
> + build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.GPO0",
> + "\\_SB.GPO0._E02", AML_SYSTEM_MEMORY);
> + }
> +
> /* copy AML table into ACPI tables blob and patch header there */
> g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len);
> build_header(linker, table_data,
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 8c6dd59..884960d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -173,6 +173,9 @@ static const char *valid_cpus[] = {
> ARM_CPU_TYPE_NAME("max"),
> };
>
> +static QLIST_HEAD(, GPIODevice) gpio_devices =
> + QLIST_HEAD_INITIALIZER(gpio_devices);
> +
> static bool cpu_type_valid(const char *cpu)
> {
> int i;
> @@ -740,11 +743,34 @@ static void create_rtc(const VirtMachineState *vms, qemu_irq *pic)
> g_free(nodename);
> }
>
> -static DeviceState *gpio_key_dev;
> +static DeviceState *virt_get_gpio_dev(int pin)
> +{
> + GPIODevice *gpio_dev;
> +
> + QLIST_FOREACH(gpio_dev, &gpio_devices, next) {
> + if (pin == gpio_dev->pin_num) {
> + return gpio_dev->dev;
> + }
> + }
> + return NULL;
> +}
> +
> +static void virt_create_gpio_dev(DeviceState *pl061_dev, int pin)
> +{
> + GPIODevice *gpio_dev;
> +
> + gpio_dev = g_malloc0(sizeof(*gpio_dev));
> + gpio_dev->dev = sysbus_create_simple("gpio-key", -1,
> + qdev_get_gpio_in(pl061_dev, pin));
> + gpio_dev->pin_num = pin;
> + QLIST_INSERT_HEAD(&gpio_devices, gpio_dev, next);
> +}
> +
> static void virt_powerdown_req(Notifier *n, void *opaque)
> {
> + DeviceState *dev = virt_get_gpio_dev(GPIO_PWRB);
> /* use gpio Pin 3 for power button event */
> - qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
> + qemu_set_irq(qdev_get_gpio_in(dev, 0), 1);
> }
>
> static Notifier virt_system_powerdown_notifier = {
> @@ -777,8 +803,11 @@ static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
> qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
> qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
>
> - gpio_key_dev = sysbus_create_simple("gpio-key", -1,
> - qdev_get_gpio_in(pl061_dev, 3));
> + virt_create_gpio_dev(pl061_dev, GPIO_PWRB);
> +
> + if (vms->acpi_memhp_state.is_enabled) {
> + virt_create_gpio_dev(pl061_dev, GPIO_PCDIMM);
> + }
> qemu_fdt_add_subnode(vms->fdt, "/gpio-keys");
> qemu_fdt_setprop_string(vms->fdt, "/gpio-keys", "compatible", "gpio-keys");
> qemu_fdt_setprop_cell(vms->fdt, "/gpio-keys", "#size-cells", 0);
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 6bb7f92..ef39ce6 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -38,6 +38,7 @@
> #include "sysemu/kvm.h"
> #include "hw/intc/arm_gicv3_common.h"
> #include "hw/mem/nvdimm.h"
> +#include "hw/acpi/memory_hotplug.h"
>
> #define NUM_GICV2M_SPIS 64
> #define NUM_VIRTIO_TRANSPORTS 32
> @@ -136,8 +137,21 @@ typedef struct {
> hwaddr high_io_base;
> bool extended_memmap;
> AcpiNVDIMMState acpi_nvdimm_state;
> + MemHotplugState acpi_memhp_state;
> } VirtMachineState;
>
> +/* GPIO pins for ACPI events */
> +enum {
> + GPIO_PCDIMM = 2,
> + GPIO_PWRB,
> +};
> +
> +typedef struct GPIODevice {
> + DeviceState *dev;
> + int pin_num;
> + QLIST_ENTRY(GPIODevice) next;
> +} GPIODevice;
> +
> #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
>
> #define TYPE_VIRT_MACHINE MACHINE_TYPE_NAME("virt")
^ permalink raw reply [flat|nested] 36+ messages in thread
* [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-01-28 11:05 [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support Shameer Kolothum
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 1/4] hw:acpi: Make ACPI IO address space configurable Shameer Kolothum
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 2/4] hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support Shameer Kolothum
@ 2019-01-28 11:05 ` Shameer Kolothum
2019-02-27 17:14 ` Igor Mammedov
2019-03-01 9:12 ` Igor Mammedov
2019-02-22 16:03 ` [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory " Auger Eric
3 siblings, 2 replies; 36+ messages in thread
From: Shameer Kolothum @ 2019-01-28 11:05 UTC (permalink / raw)
To: eric.auger, shannon.zhaosl, peter.maydell, imammedo, qemu-devel,
qemu-arm
Cc: xuwei5, linuxarm
pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
event. Hot removal functionality is not yet supported.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 55 insertions(+), 2 deletions(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 884960d..cf64554 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -62,6 +62,7 @@
#include "hw/mem/pc-dimm.h"
#include "hw/mem/nvdimm.h"
#include "hw/acpi/acpi.h"
+#include "hw/acpi/pc-hotplug.h"
#define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
vms->fw_cfg, OBJECT(vms));
}
+ if (vms->acpi_memhp_state.is_enabled) {
+ MemHotplugState *state = &vms->acpi_memhp_state;
+ hwaddr base;
+ state->hw_reduced_acpi = true;
+ base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
+ acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
+ }
vms->bootinfo.ram_size = machine->ram_size;
vms->bootinfo.kernel_filename = machine->kernel_filename;
vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
@@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object *obj, const char *value,
nvdimm_state->persistence_string = g_strdup(value);
}
+static bool virt_get_memhp_support(Object *obj, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(obj);
+
+ return vms->acpi_memhp_state.is_enabled;
+}
+
+static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(obj);
+
+ vms->acpi_memhp_state.is_enabled = value;
+}
+
static CpuInstanceProperties
virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
{
@@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
- if (dev->hotplugged) {
- error_setg(errp, "memory hotplug is not supported");
+ if (dev->hotplugged && is_nvdimm) {
+ error_setg(errp, "nvdimm hotplug is not supported");
}
if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
@@ -1875,6 +1897,22 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
}
+static void virt_memhp_send_event(HotplugHandler *hotplug_dev, DeviceState *dev,
+ Error **errp)
+{
+ DeviceState *gpio_dev;
+ VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+
+ gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
+ if (!gpio_dev) {
+ error_setg(errp, "No dev interface to send hotplug event");
+ return;
+ }
+ acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
+ dev, errp);
+ qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
+}
+
static void virt_memory_plug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
@@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
nvdimm_plug(&vms->acpi_nvdimm_state);
}
+ if (dev->hotplugged && !is_nvdimm) {
+ virt_memhp_send_event(hotplug_dev, dev, errp);
+ }
+
out:
error_propagate(errp, local_err);
}
@@ -1898,6 +1940,11 @@ out:
static void virt_memory_unplug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
+ if (dev->hotplugged) {
+ error_setg(errp, "memory hot unplug is not supported");
+ return;
+ }
+
pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
object_unparent(OBJECT(dev));
}
@@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
"Set NVDIMM persistence"
"Valid values are cpu and mem-ctrl", NULL);
+ vms->acpi_memhp_state.is_enabled = true;
+ object_property_add_bool(obj, "memory-hotplug-support",
+ virt_get_memhp_support,
+ virt_set_memhp_support,
+ NULL);
+
vms->irqmap = a15irqmap;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support Shameer Kolothum
@ 2019-02-27 17:14 ` Igor Mammedov
2019-02-28 9:57 ` Auger Eric
2019-02-28 12:27 ` Shameerali Kolothum Thodi
2019-03-01 9:12 ` Igor Mammedov
1 sibling, 2 replies; 36+ messages in thread
From: Igor Mammedov @ 2019-02-27 17:14 UTC (permalink / raw)
To: Shameer Kolothum
Cc: eric.auger, shannon.zhaosl, peter.maydell, qemu-devel, qemu-arm,
linuxarm, xuwei5
On Mon, 28 Jan 2019 11:05:45 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> event. Hot removal functionality is not yet supported.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 884960d..cf64554 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -62,6 +62,7 @@
> #include "hw/mem/pc-dimm.h"
> #include "hw/mem/nvdimm.h"
> #include "hw/acpi/acpi.h"
> +#include "hw/acpi/pc-hotplug.h"
>
> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
> nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> vms->fw_cfg, OBJECT(vms));
> }
> + if (vms->acpi_memhp_state.is_enabled) {
> + MemHotplugState *state = &vms->acpi_memhp_state;
> + hwaddr base;
>
> + state->hw_reduced_acpi = true;
> + base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
> + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> + }
this hunk should be a part of 'acpi' device that owns respective interrupts and mmio regions.
(something like we do in x86)
In this case I'd suggest to make 'base' its property and the board will set it during
device creation.
> vms->bootinfo.ram_size = machine->ram_size;
> vms->bootinfo.kernel_filename = machine->kernel_filename;
> vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object *obj, const char *value,
> nvdimm_state->persistence_string = g_strdup(value);
> }
>
> +static bool virt_get_memhp_support(Object *obj, Error **errp)
> +{
> + VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> + return vms->acpi_memhp_state.is_enabled;
> +}
> +
> +static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
> +{
> + VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> + vms->acpi_memhp_state.is_enabled = value;
> +}
> +
> static CpuInstanceProperties
> virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
> {
> @@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>
> - if (dev->hotplugged) {
> - error_setg(errp, "memory hotplug is not supported");
> + if (dev->hotplugged && is_nvdimm) {
> + error_setg(errp, "nvdimm hotplug is not supported");
> }
>
> if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
> @@ -1875,6 +1897,22 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
> }
>
> +static void virt_memhp_send_event(HotplugHandler *hotplug_dev, DeviceState *dev,
> + Error **errp)
> +{
> + DeviceState *gpio_dev;
> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +
> + gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
> + if (!gpio_dev) {
> + error_setg(errp, "No dev interface to send hotplug event");
^^^^^^ confusing
> + return;
> + }
> + acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
> + dev, errp);
> + qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
> +}
> +
> static void virt_memory_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> @@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> nvdimm_plug(&vms->acpi_nvdimm_state);
> }
>
> + if (dev->hotplugged && !is_nvdimm) {
> + virt_memhp_send_event(hotplug_dev, dev, errp);
...
acpi_memory_plug_cb();
hotplug_handler_plug(HOTPLUG_HANDLER(pcms->gpio_dev), dev, &error_abort);
^^^^ forward snd process hotplug notification event in gpio_dev,
machine should not care about which and how to deal with random IRQs
> + }
> +
> out:
> error_propagate(errp, local_err);
> }
> @@ -1898,6 +1940,11 @@ out:
> static void virt_memory_unplug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> + if (dev->hotplugged) {
> + error_setg(errp, "memory hot unplug is not supported");
> + return;
> + }
what if unplug is called on cold-plugged device?
Better way to disable mgmt initiated unplug is to forbid it in unplug_request()
For guest initiated one ('unplug' handler), the best we can do is log error
and ignore it (provided guest won't get in confused). it's also possible
to hide _EJ method and then it would be even fine to abort if it gets here,
since guest is not supposed to interface with MMIO interface without using AML.
> +
> pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
> object_unparent(OBJECT(dev));
> }
> @@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
> "Set NVDIMM persistence"
> "Valid values are cpu and mem-ctrl", NULL);
>
> + vms->acpi_memhp_state.is_enabled = true;
> + object_property_add_bool(obj, "memory-hotplug-support",
> + virt_get_memhp_support,
> + virt_set_memhp_support,
> + NULL);
> +
> vms->irqmap = a15irqmap;
> }
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-02-27 17:14 ` Igor Mammedov
@ 2019-02-28 9:57 ` Auger Eric
2019-02-28 12:44 ` Igor Mammedov
2019-02-28 12:27 ` Shameerali Kolothum Thodi
1 sibling, 1 reply; 36+ messages in thread
From: Auger Eric @ 2019-02-28 9:57 UTC (permalink / raw)
To: Igor Mammedov, Shameer Kolothum
Cc: shannon.zhaosl, peter.maydell, qemu-devel, qemu-arm, linuxarm,
xuwei5
Hi Igor,
On 2/27/19 6:14 PM, Igor Mammedov wrote:
> On Mon, 28 Jan 2019 11:05:45 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
>> pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
>> event. Hot removal functionality is not yet supported.
>>
>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>> ---
>> hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 884960d..cf64554 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -62,6 +62,7 @@
>> #include "hw/mem/pc-dimm.h"
>> #include "hw/mem/nvdimm.h"
>> #include "hw/acpi/acpi.h"
>> +#include "hw/acpi/pc-hotplug.h"
>>
>> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>> @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
>> nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
>> vms->fw_cfg, OBJECT(vms));
>> }
>> + if (vms->acpi_memhp_state.is_enabled) {
>> + MemHotplugState *state = &vms->acpi_memhp_state;
>> + hwaddr base;
>>
>> + state->hw_reduced_acpi = true;
>> + base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
>> + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
>> + }
> this hunk should be a part of 'acpi' device that owns respective interrupts and mmio regions.
> (something like we do in x86)
> In this case I'd suggest to make 'base' its property and the board will set it during
> device creation.
>
>> vms->bootinfo.ram_size = machine->ram_size;
>> vms->bootinfo.kernel_filename = machine->kernel_filename;
>> vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
>> @@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object *obj, const char *value,
>> nvdimm_state->persistence_string = g_strdup(value);
>> }
>>
>> +static bool virt_get_memhp_support(Object *obj, Error **errp)
>> +{
>> + VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> + return vms->acpi_memhp_state.is_enabled;
>> +}
>> +
>> +static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
>> +{
>> + VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> + vms->acpi_memhp_state.is_enabled = value;
>> +}
>> +
>> static CpuInstanceProperties
>> virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>> {
>> @@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>>
>> - if (dev->hotplugged) {
>> - error_setg(errp, "memory hotplug is not supported");
>> + if (dev->hotplugged && is_nvdimm) {
>> + error_setg(errp, "nvdimm hotplug is not supported");
>> }
>>
>> if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
>> @@ -1875,6 +1897,22 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
>> }
>>
>> +static void virt_memhp_send_event(HotplugHandler *hotplug_dev, DeviceState *dev,
>> + Error **errp)
>> +{
>> + DeviceState *gpio_dev;
>> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>> +
>> + gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
>> + if (!gpio_dev) {
>> + error_setg(errp, "No dev interface to send hotplug event");
> ^^^^^^ confusing
>> + return;
>> + }
>> + acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
>> + dev, errp);
>> + qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
>> +}
>> +
>> static void virt_memory_plug(HotplugHandler *hotplug_dev,
>> DeviceState *dev, Error **errp)
>> {
>> @@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>> nvdimm_plug(&vms->acpi_nvdimm_state);
>> }
>>
>> + if (dev->hotplugged && !is_nvdimm) {
>> + virt_memhp_send_event(hotplug_dev, dev, errp);
> ...
> acpi_memory_plug_cb();
> hotplug_handler_plug(HOTPLUG_HANDLER(pcms->gpio_dev), dev, &error_abort);
> ^^^^ forward snd process hotplug notification event in gpio_dev,
> machine should not care about which and how to deal with random IRQs
>
>> + }
>> +
>> out:
>> error_propagate(errp, local_err);
>> }
>> @@ -1898,6 +1940,11 @@ out:
>> static void virt_memory_unplug(HotplugHandler *hotplug_dev,
>> DeviceState *dev, Error **errp)
>> {
>> + if (dev->hotplugged) {
>> + error_setg(errp, "memory hot unplug is not supported");
>> + return;
>> + }
> what if unplug is called on cold-plugged device?
>
> Better way to disable mgmt initiated unplug is to forbid it in unplug_request()
> For guest initiated one ('unplug' handler), the best we can do is log error
> and ignore it (provided guest won't get in confused). it's also possible
> to hide _EJ method and then it would be even fine to abort if it gets here,
> since guest is not supposed to interface with MMIO interface without using AML.
I don't understand the guest initiated unplug handling. How does it
differ from x86 handling as we are using the same dynamic methods?
Besides In ARM case I checked
/sys/devices/system/memory/memoryX/removable state for cold-plugged and
hot-plugged DIMMs and it is equal to 0. I don't know if it is a
limitation of the guest or a bug in our ACPI description somewhere. I
would not be surprised if hot-unplug is not supported at kernel leve
though.
Thanks
Eric
>
>> +
>> pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
>> object_unparent(OBJECT(dev));
>> }
>> @@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
>> "Set NVDIMM persistence"
>> "Valid values are cpu and mem-ctrl", NULL);
>>
>> + vms->acpi_memhp_state.is_enabled = true;
>> + object_property_add_bool(obj, "memory-hotplug-support",
>> + virt_get_memhp_support,
>> + virt_set_memhp_support,
>> + NULL);
>> +
>> vms->irqmap = a15irqmap;
>> }
>>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-02-28 9:57 ` Auger Eric
@ 2019-02-28 12:44 ` Igor Mammedov
0 siblings, 0 replies; 36+ messages in thread
From: Igor Mammedov @ 2019-02-28 12:44 UTC (permalink / raw)
To: Auger Eric
Cc: Shameer Kolothum, shannon.zhaosl, peter.maydell, qemu-devel,
qemu-arm, linuxarm, xuwei5
On Thu, 28 Feb 2019 10:57:41 +0100
Auger Eric <eric.auger@redhat.com> wrote:
> Hi Igor,
>
> On 2/27/19 6:14 PM, Igor Mammedov wrote:
> > On Mon, 28 Jan 2019 11:05:45 +0000
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> >> pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> >> event. Hot removal functionality is not yet supported.
> >>
> >> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> >> ---
> >> hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 55 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index 884960d..cf64554 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -62,6 +62,7 @@
> >> #include "hw/mem/pc-dimm.h"
> >> #include "hw/mem/nvdimm.h"
> >> #include "hw/acpi/acpi.h"
> >> +#include "hw/acpi/pc-hotplug.h"
> >>
> >> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> >> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> >> @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
> >> nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> >> vms->fw_cfg, OBJECT(vms));
> >> }
> >> + if (vms->acpi_memhp_state.is_enabled) {
> >> + MemHotplugState *state = &vms->acpi_memhp_state;
> >> + hwaddr base;
> >>
> >> + state->hw_reduced_acpi = true;
> >> + base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
> >> + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> >> + }
> > this hunk should be a part of 'acpi' device that owns respective interrupts and mmio regions.
> > (something like we do in x86)
> > In this case I'd suggest to make 'base' its property and the board will set it during
> > device creation.
> >
> >> vms->bootinfo.ram_size = machine->ram_size;
> >> vms->bootinfo.kernel_filename = machine->kernel_filename;
> >> vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> >> @@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object *obj, const char *value,
> >> nvdimm_state->persistence_string = g_strdup(value);
> >> }
> >>
> >> +static bool virt_get_memhp_support(Object *obj, Error **errp)
> >> +{
> >> + VirtMachineState *vms = VIRT_MACHINE(obj);
> >> +
> >> + return vms->acpi_memhp_state.is_enabled;
> >> +}
> >> +
> >> +static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
> >> +{
> >> + VirtMachineState *vms = VIRT_MACHINE(obj);
> >> +
> >> + vms->acpi_memhp_state.is_enabled = value;
> >> +}
> >> +
> >> static CpuInstanceProperties
> >> virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
> >> {
> >> @@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >>
> >> - if (dev->hotplugged) {
> >> - error_setg(errp, "memory hotplug is not supported");
> >> + if (dev->hotplugged && is_nvdimm) {
> >> + error_setg(errp, "nvdimm hotplug is not supported");
> >> }
> >>
> >> if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
> >> @@ -1875,6 +1897,22 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
> >> }
> >>
> >> +static void virt_memhp_send_event(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> + Error **errp)
> >> +{
> >> + DeviceState *gpio_dev;
> >> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >> +
> >> + gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
> >> + if (!gpio_dev) {
> >> + error_setg(errp, "No dev interface to send hotplug event");
> > ^^^^^^ confusing
> >> + return;
> >> + }
> >> + acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
> >> + dev, errp);
> >> + qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
> >> +}
> >> +
> >> static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >> DeviceState *dev, Error **errp)
> >> {
> >> @@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >> nvdimm_plug(&vms->acpi_nvdimm_state);
> >> }
> >>
> >> + if (dev->hotplugged && !is_nvdimm) {
> >> + virt_memhp_send_event(hotplug_dev, dev, errp);
> > ...
> > acpi_memory_plug_cb();
> > hotplug_handler_plug(HOTPLUG_HANDLER(pcms->gpio_dev), dev, &error_abort);
> > ^^^^ forward snd process hotplug notification event in gpio_dev,
> > machine should not care about which and how to deal with random IRQs
> >
> >> + }
> >> +
> >> out:
> >> error_propagate(errp, local_err);
> >> }
> >> @@ -1898,6 +1940,11 @@ out:
> >> static void virt_memory_unplug(HotplugHandler *hotplug_dev,
> >> DeviceState *dev, Error **errp)
> >> {
> >> + if (dev->hotplugged) {
> >> + error_setg(errp, "memory hot unplug is not supported");
> >> + return;
> >> + }
> > what if unplug is called on cold-plugged device?
> >
> > Better way to disable mgmt initiated unplug is to forbid it in unplug_request()
> > For guest initiated one ('unplug' handler), the best we can do is log error
> > and ignore it (provided guest won't get in confused). it's also possible
> > to hide _EJ method and then it would be even fine to abort if it gets here,
> > since guest is not supposed to interface with MMIO interface without using AML.
> I don't understand the guest initiated unplug handling. How does it
> differ from x86 handling as we are using the same dynamic methods?
The only differences I'm aware of, should be event delivery mechanism and
using MMIO vs IO. Otherwise it's the same as described in
docs/specs/acpi_mem_hotplug.txt
> Besides In ARM case I checked
> /sys/devices/system/memory/memoryX/removable state for cold-plugged and
> hot-plugged DIMMs and it is equal to 0. I don't know if it is a
> limitation of the guest or a bug in our ACPI description somewhere. I
> would not be surprised if hot-unplug is not supported at kernel leve
> though.
enabling mhp_acpi_foo trace events might help to check what's going on,
and a dump of ACPI tables too (SRAT + DSDT).
It's quite possible that kernel doesn't support unplug yet, all I care at
the moment is to make sure that we won't have cross-version compatibility
issues when kernel gets there so we won't have to add and support compat
knobs later on.
> Thanks
>
> Eric
> >
> >> +
> >> pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
> >> object_unparent(OBJECT(dev));
> >> }
> >> @@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
> >> "Set NVDIMM persistence"
> >> "Valid values are cpu and mem-ctrl", NULL);
> >>
> >> + vms->acpi_memhp_state.is_enabled = true;
> >> + object_property_add_bool(obj, "memory-hotplug-support",
> >> + virt_get_memhp_support,
> >> + virt_set_memhp_support,
> >> + NULL);
> >> +
> >> vms->irqmap = a15irqmap;
> >> }
> >>
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-02-27 17:14 ` Igor Mammedov
2019-02-28 9:57 ` Auger Eric
@ 2019-02-28 12:27 ` Shameerali Kolothum Thodi
1 sibling, 0 replies; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-02-28 12:27 UTC (permalink / raw)
To: Igor Mammedov
Cc: eric.auger@redhat.com, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, Linuxarm, xuwei (O)
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 27 February 2019 17:14
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: eric.auger@redhat.com; shannon.zhaosl@gmail.com;
> peter.maydell@linaro.org; qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> hotplug support
>
> On Mon, 28 Jan 2019 11:05:45 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > event. Hot removal functionality is not yet supported.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > hw/arm/virt.c | 57
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 884960d..cf64554 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -62,6 +62,7 @@
> > #include "hw/mem/pc-dimm.h"
> > #include "hw/mem/nvdimm.h"
> > #include "hw/acpi/acpi.h"
> > +#include "hw/acpi/pc-hotplug.h"
> >
> > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> *machine)
> > nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > vms->fw_cfg, OBJECT(vms));
> > }
> > + if (vms->acpi_memhp_state.is_enabled) {
> > + MemHotplugState *state = &vms->acpi_memhp_state;
> > + hwaddr base;
> >
> > + state->hw_reduced_acpi = true;
> > + base = vms->memmap[VIRT_ACPI_IO].base +
> ACPI_MEMORY_HOTPLUG_BASE;
> > + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> > + }
> this hunk should be a part of 'acpi' device that owns respective interrupts and
> mmio regions.
> (something like we do in x86)
> In this case I'd suggest to make 'base' its property and the board will set it
> during
> device creation.
Ok. I will take a look at x86.
> > vms->bootinfo.ram_size = machine->ram_size;
> > vms->bootinfo.kernel_filename = machine->kernel_filename;
> > vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > @@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object
> *obj, const char *value,
> > nvdimm_state->persistence_string = g_strdup(value);
> > }
> >
> > +static bool virt_get_memhp_support(Object *obj, Error **errp)
> > +{
> > + VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > + return vms->acpi_memhp_state.is_enabled;
> > +}
> > +
> > +static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
> > +{
> > + VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > + vms->acpi_memhp_state.is_enabled = value;
> > +}
> > +
> > static CpuInstanceProperties
> > virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
> > {
> > @@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> > const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),
> TYPE_NVDIMM);
> > VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >
> > - if (dev->hotplugged) {
> > - error_setg(errp, "memory hotplug is not supported");
> > + if (dev->hotplugged && is_nvdimm) {
> > + error_setg(errp, "nvdimm hotplug is not supported");
> > }
> >
> > if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
> > @@ -1875,6 +1897,22 @@ static void
> virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL,
> errp);
> > }
> >
> > +static void virt_memhp_send_event(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> > + Error **errp)
> > +{
> > + DeviceState *gpio_dev;
> > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > +
> > + gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
> > + if (!gpio_dev) {
> > + error_setg(errp, "No dev interface to send hotplug event");
> ^^^^^^ confusing
Ok. I think this will anyway change with AcpiDeviceIfClass implementation.
> > + return;
> > + }
> > + acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
> > + dev, errp);
> > + qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
> > +}
> > +
> > static void virt_memory_plug(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > @@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler
> *hotplug_dev,
> > nvdimm_plug(&vms->acpi_nvdimm_state);
> > }
> >
> > + if (dev->hotplugged && !is_nvdimm) {
> > + virt_memhp_send_event(hotplug_dev, dev, errp);
> ...
> acpi_memory_plug_cb();
> hotplug_handler_plug(HOTPLUG_HANDLER(pcms->gpio_dev), dev,
> &error_abort);
> ^^^^ forward snd process hotplug notification event in gpio_dev,
> machine should not care about which and how to deal with random
> IRQs
Ok.
> > + }
> > +
> > out:
> > error_propagate(errp, local_err);
> > }
> > @@ -1898,6 +1940,11 @@ out:
> > static void virt_memory_unplug(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > + if (dev->hotplugged) {
> > + error_setg(errp, "memory hot unplug is not supported");
> > + return;
> > + }
> what if unplug is called on cold-plugged device?
>
> Better way to disable mgmt initiated unplug is to forbid it in unplug_request()
> For guest initiated one ('unplug' handler), the best we can do is log error
> and ignore it (provided guest won't get in confused). it's also possible
> to hide _EJ method and then it would be even fine to abort if it gets here,
> since guest is not supposed to interface with MMIO interface without using
> AML.
Hmm.. I haven't tested yet with Guest initiated unplug as ARM kernel doesn't
support it yet. I will try that if possible.
Thanks,
Shameer
> > +
> > pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
> > object_unparent(OBJECT(dev));
> > }
> > @@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
> > "Set NVDIMM persistence"
> > "Valid values are cpu and
> mem-ctrl", NULL);
> >
> > + vms->acpi_memhp_state.is_enabled = true;
> > + object_property_add_bool(obj, "memory-hotplug-support",
> > + virt_get_memhp_support,
> > + virt_set_memhp_support,
> > + NULL);
> > +
> > vms->irqmap = a15irqmap;
> > }
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support Shameer Kolothum
2019-02-27 17:14 ` Igor Mammedov
@ 2019-03-01 9:12 ` Igor Mammedov
2019-03-01 9:23 ` Shameerali Kolothum Thodi
1 sibling, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2019-03-01 9:12 UTC (permalink / raw)
To: Shameer Kolothum
Cc: eric.auger, shannon.zhaosl, peter.maydell, qemu-devel, qemu-arm,
linuxarm, xuwei5
On Mon, 28 Jan 2019 11:05:45 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> event. Hot removal functionality is not yet supported.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 55 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 884960d..cf64554 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -62,6 +62,7 @@
> #include "hw/mem/pc-dimm.h"
> #include "hw/mem/nvdimm.h"
> #include "hw/acpi/acpi.h"
> +#include "hw/acpi/pc-hotplug.h"
it looks like x86 specific file, what is this here for?
>
> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
> nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> vms->fw_cfg, OBJECT(vms));
> }
> + if (vms->acpi_memhp_state.is_enabled) {
> + MemHotplugState *state = &vms->acpi_memhp_state;
> + hwaddr base;
>
> + state->hw_reduced_acpi = true;
> + base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
> + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> + }
> vms->bootinfo.ram_size = machine->ram_size;
> vms->bootinfo.kernel_filename = machine->kernel_filename;
> vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
[...]
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-03-01 9:12 ` Igor Mammedov
@ 2019-03-01 9:23 ` Shameerali Kolothum Thodi
2019-03-01 10:26 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-03-01 9:23 UTC (permalink / raw)
To: Igor Mammedov
Cc: eric.auger@redhat.com, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, Linuxarm, xuwei (O)
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 01 March 2019 09:12
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: eric.auger@redhat.com; shannon.zhaosl@gmail.com;
> peter.maydell@linaro.org; qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> hotplug support
>
> On Mon, 28 Jan 2019 11:05:45 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > event. Hot removal functionality is not yet supported.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> > hw/arm/virt.c | 57
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 884960d..cf64554 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -62,6 +62,7 @@
> > #include "hw/mem/pc-dimm.h"
> > #include "hw/mem/nvdimm.h"
> > #include "hw/acpi/acpi.h"
> > +#include "hw/acpi/pc-hotplug.h"
> it looks like x86 specific file, what is this here for?
Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?
> >
> > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> *machine)
> > nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > vms->fw_cfg, OBJECT(vms));
> > }
> > + if (vms->acpi_memhp_state.is_enabled) {
> > + MemHotplugState *state = &vms->acpi_memhp_state;
> > + hwaddr base;
> >
> > + state->hw_reduced_acpi = true;
> > + base = vms->memmap[VIRT_ACPI_IO].base +
> ACPI_MEMORY_HOTPLUG_BASE;
> > + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> > + }
> > vms->bootinfo.ram_size = machine->ram_size;
> > vms->bootinfo.kernel_filename = machine->kernel_filename;
> > vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> [...]
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-03-01 9:23 ` Shameerali Kolothum Thodi
@ 2019-03-01 10:26 ` Igor Mammedov
2019-03-01 10:33 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2019-03-01 10:26 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: eric.auger@redhat.com, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, Linuxarm, xuwei (O)
On Fri, 1 Mar 2019 09:23:11 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 01 March 2019 09:12
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: eric.auger@redhat.com; shannon.zhaosl@gmail.com;
> > peter.maydell@linaro.org; qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > hotplug support
> >
> > On Mon, 28 Jan 2019 11:05:45 +0000
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > > event. Hot removal functionality is not yet supported.
> > >
> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > > ---
> > > hw/arm/virt.c | 57
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 55 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 884960d..cf64554 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -62,6 +62,7 @@
> > > #include "hw/mem/pc-dimm.h"
> > > #include "hw/mem/nvdimm.h"
> > > #include "hw/acpi/acpi.h"
> > > +#include "hw/acpi/pc-hotplug.h"
> > it looks like x86 specific file, what is this here for?
>
> Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
> at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?
it's GPA and pc/q35 impl. specific so you should use it,
this address will always be board specific one.
Makeup a virt specific one
>
> > >
> > > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > > static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> > *machine)
> > > nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > > vms->fw_cfg, OBJECT(vms));
> > > }
> > > + if (vms->acpi_memhp_state.is_enabled) {
> > > + MemHotplugState *state = &vms->acpi_memhp_state;
> > > + hwaddr base;
> > >
> > > + state->hw_reduced_acpi = true;
> > > + base = vms->memmap[VIRT_ACPI_IO].base +
> > ACPI_MEMORY_HOTPLUG_BASE;
well, this is confusing, why adding 2 base addresses?
If vms->memmap[VIRT_ACPI_IO].base is already set than why not use it
as is without adding an offset?
> > > + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> > > + }
> > > vms->bootinfo.ram_size = machine->ram_size;
> > > vms->bootinfo.kernel_filename = machine->kernel_filename;
> > > vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > [...]
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-03-01 10:26 ` Igor Mammedov
@ 2019-03-01 10:33 ` Igor Mammedov
2019-03-01 10:51 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2019-03-01 10:33 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: peter.maydell@linaro.org, shannon.zhaosl@gmail.com, Linuxarm,
qemu-devel@nongnu.org, eric.auger@redhat.com, qemu-arm@nongnu.org,
xuwei (O)
On Fri, 1 Mar 2019 11:26:35 +0100
Igor Mammedov <imammedo@redhat.com> wrote:
> On Fri, 1 Mar 2019 09:23:11 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > Sent: 01 March 2019 09:12
> > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > > Cc: eric.auger@redhat.com; shannon.zhaosl@gmail.com;
> > > peter.maydell@linaro.org; qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > > Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> > > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > > hotplug support
> > >
> > > On Mon, 28 Jan 2019 11:05:45 +0000
> > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > >
> > > > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > > > event. Hot removal functionality is not yet supported.
> > > >
> > > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > > > ---
> > > > hw/arm/virt.c | 57
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > 1 file changed, 55 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > index 884960d..cf64554 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -62,6 +62,7 @@
> > > > #include "hw/mem/pc-dimm.h"
> > > > #include "hw/mem/nvdimm.h"
> > > > #include "hw/acpi/acpi.h"
> > > > +#include "hw/acpi/pc-hotplug.h"
> > > it looks like x86 specific file, what is this here for?
> >
> > Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
> > at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?
> it's GPA and pc/q35 impl. specific so you should use it,
s/should/should not/
> this address will always be board specific one.
> Makeup a virt specific one
>
> >
> > > >
> > > > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > > > static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > > > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> > > *machine)
> > > > nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > > > vms->fw_cfg, OBJECT(vms));
> > > > }
> > > > + if (vms->acpi_memhp_state.is_enabled) {
> > > > + MemHotplugState *state = &vms->acpi_memhp_state;
> > > > + hwaddr base;
> > > >
> > > > + state->hw_reduced_acpi = true;
> > > > + base = vms->memmap[VIRT_ACPI_IO].base +
> > > ACPI_MEMORY_HOTPLUG_BASE;
> well, this is confusing, why adding 2 base addresses?
> If vms->memmap[VIRT_ACPI_IO].base is already set than why not use it
> as is without adding an offset?
>
>
> > > > + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> > > > + }
> > > > vms->bootinfo.ram_size = machine->ram_size;
> > > > vms->bootinfo.kernel_filename = machine->kernel_filename;
> > > > vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > > [...]
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-03-01 10:33 ` Igor Mammedov
@ 2019-03-01 10:51 ` Shameerali Kolothum Thodi
2019-03-01 13:09 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-03-01 10:51 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell@linaro.org, shannon.zhaosl@gmail.com,
qemu-devel@nongnu.org, Linuxarm, eric.auger@redhat.com,
qemu-arm@nongnu.org, xuwei (O)
> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 01 March 2019 10:34
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>;
> eric.auger@redhat.com; qemu-arm@nongnu.org; xuwei (O)
> <xuwei5@huawei.com>
> Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> hotplug support
>
> On Fri, 1 Mar 2019 11:26:35 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
>
> > On Fri, 1 Mar 2019 09:23:11 +0000
> > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> wrote:
> >
> > > > -----Original Message-----
> > > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > > Sent: 01 March 2019 09:12
> > > > To: Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>
> > > > Cc: eric.auger@redhat.com; shannon.zhaosl@gmail.com;
> > > > peter.maydell@linaro.org; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org;
> > > > Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> > > > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > > > hotplug support
> > > >
> > > > On Mon, 28 Jan 2019 11:05:45 +0000
> > > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > > >
> > > > > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > > > > event. Hot removal functionality is not yet supported.
> > > > >
> > > > > Signed-off-by: Shameer Kolothum
> <shameerali.kolothum.thodi@huawei.com>
> > > > > ---
> > > > > hw/arm/virt.c | 57
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > 1 file changed, 55 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > index 884960d..cf64554 100644
> > > > > --- a/hw/arm/virt.c
> > > > > +++ b/hw/arm/virt.c
> > > > > @@ -62,6 +62,7 @@
> > > > > #include "hw/mem/pc-dimm.h"
> > > > > #include "hw/mem/nvdimm.h"
> > > > > #include "hw/acpi/acpi.h"
> > > > > +#include "hw/acpi/pc-hotplug.h"
> > > > it looks like x86 specific file, what is this here for?
> > >
> > > Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
> > > at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?
> > it's GPA and pc/q35 impl. specific so you should use it,
> s/should/should not/
>
> > this address will always be board specific one.
> > Makeup a virt specific one
Ok. I was under the impression that the offsets can be reused as it is defined
here docs/specs/acpi_mem_hotplug.txt(though that is GPE and pc/q35 acpi dev
specific). But agree, it doesn't make sense to make it generic.
> > >
> > > > >
> > > > > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > > > > static void virt_##major##_##minor##_class_init(ObjectClass *oc,
> \
> > > > > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> > > > *machine)
> > > > > nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > > > > vms->fw_cfg, OBJECT(vms));
> > > > > }
> > > > > + if (vms->acpi_memhp_state.is_enabled) {
> > > > > + MemHotplugState *state = &vms->acpi_memhp_state;
> > > > > + hwaddr base;
> > > > >
> > > > > + state->hw_reduced_acpi = true;
> > > > > + base = vms->memmap[VIRT_ACPI_IO].base +
> > > > ACPI_MEMORY_HOTPLUG_BASE;
> > well, this is confusing, why adding 2 base addresses?
> > If vms->memmap[VIRT_ACPI_IO].base is already set than why not use it
> > as is without adding an offset?
Well, Eric's work on which this was based had one NVDIMM_ACPI_IO_BASE offset
from what appears to be a generic VIRT_ACPI_IO region. Now I see that, it is
renamed to VIRT_NVDIMM_ACPI_IO. Do we really need two separate regions?
Thanks,
Shameer
> >
> > > > > + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state,
> base);
> > > > > + }
> > > > > vms->bootinfo.ram_size = machine->ram_size;
> > > > > vms->bootinfo.kernel_filename = machine->kernel_filename;
> > > > > vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > > > [...]
> >
> >
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support
2019-03-01 10:51 ` Shameerali Kolothum Thodi
@ 2019-03-01 13:09 ` Igor Mammedov
0 siblings, 0 replies; 36+ messages in thread
From: Igor Mammedov @ 2019-03-01 13:09 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: peter.maydell@linaro.org, shannon.zhaosl@gmail.com,
qemu-devel@nongnu.org, Linuxarm, eric.auger@redhat.com,
qemu-arm@nongnu.org, xuwei (O)
On Fri, 1 Mar 2019 10:51:45 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> > -----Original Message-----
> > From: Qemu-devel
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 01 March 2019 10:34
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> > qemu-devel@nongnu.org; Linuxarm <linuxarm@huawei.com>;
> > eric.auger@redhat.com; qemu-arm@nongnu.org; xuwei (O)
> > <xuwei5@huawei.com>
> > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > hotplug support
> >
> > On Fri, 1 Mar 2019 11:26:35 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > > On Fri, 1 Mar 2019 09:23:11 +0000
> > > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > > > Sent: 01 March 2019 09:12
> > > > > To: Shameerali Kolothum Thodi
> > <shameerali.kolothum.thodi@huawei.com>
> > > > > Cc: eric.auger@redhat.com; shannon.zhaosl@gmail.com;
> > > > > peter.maydell@linaro.org; qemu-devel@nongnu.org;
> > qemu-arm@nongnu.org;
> > > > > Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> > > > > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > > > > hotplug support
> > > > >
> > > > > On Mon, 28 Jan 2019 11:05:45 +0000
> > > > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > > > >
> > > > > > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > > > > > event. Hot removal functionality is not yet supported.
> > > > > >
> > > > > > Signed-off-by: Shameer Kolothum
> > <shameerali.kolothum.thodi@huawei.com>
> > > > > > ---
> > > > > > hw/arm/virt.c | 57
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > > > 1 file changed, 55 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > > index 884960d..cf64554 100644
> > > > > > --- a/hw/arm/virt.c
> > > > > > +++ b/hw/arm/virt.c
> > > > > > @@ -62,6 +62,7 @@
> > > > > > #include "hw/mem/pc-dimm.h"
> > > > > > #include "hw/mem/nvdimm.h"
> > > > > > #include "hw/acpi/acpi.h"
> > > > > > +#include "hw/acpi/pc-hotplug.h"
> > > > > it looks like x86 specific file, what is this here for?
> > > >
> > > > Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
> > > > at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?
> > > it's GPA and pc/q35 impl. specific so you should use it,
> > s/should/should not/
> >
> > > this address will always be board specific one.
> > > Makeup a virt specific one
>
> Ok. I was under the impression that the offsets can be reused as it is defined
> here docs/specs/acpi_mem_hotplug.txt(though that is GPE and pc/q35 acpi dev
> specific). But agree, it doesn't make sense to make it generic.
Offsets defined by docs/specs/acpi_mem_hotplug.txt are meant to be reused
but IO port address (0xa00) where interface's address space starts is board specific.
> > > >
> > > > > >
> > > > > > #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > > > > > static void virt_##major##_##minor##_class_init(ObjectClass *oc,
> > \
> > > > > > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> > > > > *machine)
> > > > > > nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > > > > > vms->fw_cfg, OBJECT(vms));
> > > > > > }
> > > > > > + if (vms->acpi_memhp_state.is_enabled) {
> > > > > > + MemHotplugState *state = &vms->acpi_memhp_state;
> > > > > > + hwaddr base;
> > > > > >
> > > > > > + state->hw_reduced_acpi = true;
> > > > > > + base = vms->memmap[VIRT_ACPI_IO].base +
> > > > > ACPI_MEMORY_HOTPLUG_BASE;
> > > well, this is confusing, why adding 2 base addresses?
> > > If vms->memmap[VIRT_ACPI_IO].base is already set than why not use it
> > > as is without adding an offset?
>
> Well, Eric's work on which this was based had one NVDIMM_ACPI_IO_BASE offset
> from what appears to be a generic VIRT_ACPI_IO region. Now I see that, it is
> renamed to VIRT_NVDIMM_ACPI_IO. Do we really need two separate regions?
I'm afraid we can't reuse MMIO regions as ther might be used at the same time
and do different things (we would have done this for x86 if it was possible)
As for naming try to find some consensus/coordinate it with Eric
> Thanks,
> Shameer
>
> > >
> > > > > > + acpi_memory_hotplug_init(sysmem, OBJECT(vms), state,
> > base);
> > > > > > + }
> > > > > > vms->bootinfo.ram_size = machine->ram_size;
> > > > > > vms->bootinfo.kernel_filename = machine->kernel_filename;
> > > > > > vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > > > > [...]
> > >
> > >
> >
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-01-28 11:05 [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support Shameer Kolothum
` (2 preceding siblings ...)
2019-01-28 11:05 ` [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support Shameer Kolothum
@ 2019-02-22 16:03 ` Auger Eric
2019-02-22 19:11 ` Laszlo Ersek
2019-02-25 9:47 ` Shameerali Kolothum Thodi
3 siblings, 2 replies; 36+ messages in thread
From: Auger Eric @ 2019-02-22 16:03 UTC (permalink / raw)
To: Shameer Kolothum, shannon.zhaosl, peter.maydell, imammedo,
qemu-devel, qemu-arm
Cc: xuwei5, linuxarm, Ard Biesheuvel, Leif Lindholm (Linaro address),
Laszlo Ersek
Hi Shameer,
On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> This series is an attempt to provide hotplug support to both
> pc-dimm and nvdimm device memory on ARM virt platform. This is
> based on Eric's recent works to support PCDIMM/NVDIMM device memory[1].
> The kernel support for arm64 memory hot add was added only
> recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.
>
> This makes use of PL061 GPIO controller to sent related ACPI events
s/sent/send
> to the Guest. The only reference I could find with respect to the GPIO
> pins usage is here[3] which says, "use PIN 3 for system_powerdown,
> reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
> Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.
>
> This is sanity tested on a HiSilicon ARM64 platform and appreciate
> any further testing.
I did some testing on another platform and I got the exactly the same
results as yours: PCDIMM hot plug works fine. Also after system_reset I
still can see the slots.
Hot-unplug is not supported though.
For NVDIMM, hot-add works fine and and I can see the slots using ndctl
on guest. But after system_reset, the guest does not boot properly.
>
> This series can be applied on top of Eric's branch here[4]
>
> Test:
> ------
> Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
> hotplug related CONFIGs enabled.
>
> ./qemu-system-aarch64 \
> -machine virt,gic-version=3,nvdimm \
> -m 1G,maxmem=4G,slots=4 \
> -cpu host \
> -kernel Image \
> -initrd rootfs-iperf.cpio \
> -bios QEMU_EFI.fd \
> -numa node,nodeid=0 \
> -net none \
> -nographic -enable-kvm \
> -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
>
> Enter Qemu monitor,
> Add pc-dimm:
> object_add memory-backend-ram,id=mem1,size=1G
> device_add pc-dimm,id=dimm1,memdev=mem1
>
> Add nvdimm:
> object_add memory-backend-ram,id=mem2,size=1G
> device_add nvdimm,id=dimm2,memdev=mem2
>
> Known Issue:
>
> It is observed that hot adding nvdimm will results in guest reboot
> failure. EDK2 fails to build the ACPI tables on reboot. Please find
> below EDK2 log on Guest reboot after nvdimm hot-add,
>
> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>
> The root cause seems to be EDK2 ACPI table checksum failure
> as NFIT table is getting updated on hot-add. This needs further
> investigation.
+ Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
Thanks
Eric
>
> Thanks,
> Shameer
>
> [1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
> [2]https://patchwork.kernel.org/patch/10724455/
> [3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
> [4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5
>
> Shameer Kolothum (4):
> hw:acpi: Make ACPI IO address space configurable
> hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
> hw/arm/virt: Enable pc-dimm hotplug support
> hw/arm/virt: Add nvdimm hotplug support
>
> default-configs/arm-softmmu.mak | 1 +
> hw/acpi/memory_hotplug.c | 13 +++--
> hw/arm/virt-acpi-build.c | 45 +++++++++++++++--
> hw/arm/virt.c | 105 ++++++++++++++++++++++++++++++++++++---
> hw/i386/acpi-build.c | 3 +-
> include/hw/acpi/memory_hotplug.h | 6 ++-
> include/hw/arm/virt.h | 15 ++++++
> 7 files changed, 168 insertions(+), 20 deletions(-)
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-22 16:03 ` [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory " Auger Eric
@ 2019-02-22 19:11 ` Laszlo Ersek
2019-02-25 9:54 ` Shameerali Kolothum Thodi
2019-02-27 12:55 ` Shameerali Kolothum Thodi
2019-02-25 9:47 ` Shameerali Kolothum Thodi
1 sibling, 2 replies; 36+ messages in thread
From: Laszlo Ersek @ 2019-02-22 19:11 UTC (permalink / raw)
To: Auger Eric, Shameer Kolothum, shannon.zhaosl, peter.maydell,
imammedo, qemu-devel, qemu-arm
Cc: xuwei5, linuxarm, Ard Biesheuvel, Leif Lindholm (Linaro address)
On 02/22/19 17:03, Auger Eric wrote:
> Hi Shameer,
>
> On 1/28/19 12:05 PM, Shameer Kolothum wrote:
>> This series is an attempt to provide hotplug support to both
>> pc-dimm and nvdimm device memory on ARM virt platform. This is
>> based on Eric's recent works to support PCDIMM/NVDIMM device memory[1].
>> The kernel support for arm64 memory hot add was added only
>> recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.
>>
>> This makes use of PL061 GPIO controller to sent related ACPI events
> s/sent/send
>> to the Guest. The only reference I could find with respect to the GPIO
>> pins usage is here[3] which says, "use PIN 3 for system_powerdown,
>> reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
>> Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.
>>
>> This is sanity tested on a HiSilicon ARM64 platform and appreciate
>> any further testing.
>
> I did some testing on another platform and I got the exactly the same
> results as yours: PCDIMM hot plug works fine. Also after system_reset I
> still can see the slots.
> Hot-unplug is not supported though.
> For NVDIMM, hot-add works fine and and I can see the slots using ndctl
> on guest. But after system_reset, the guest does not boot properly.
>
>>
>> This series can be applied on top of Eric's branch here[4]
>>
>> Test:
>> ------
>> Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
>> hotplug related CONFIGs enabled.
>>
>> ./qemu-system-aarch64 \
>> -machine virt,gic-version=3,nvdimm \
>> -m 1G,maxmem=4G,slots=4 \
>> -cpu host \
>> -kernel Image \
>> -initrd rootfs-iperf.cpio \
>> -bios QEMU_EFI.fd \
>> -numa node,nodeid=0 \
>> -net none \
>> -nographic -enable-kvm \
>> -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
>>
>> Enter Qemu monitor,
>> Add pc-dimm:
>> object_add memory-backend-ram,id=mem1,size=1G
>> device_add pc-dimm,id=dimm1,memdev=mem1
>>
>> Add nvdimm:
>> object_add memory-backend-ram,id=mem2,size=1G
>> device_add nvdimm,id=dimm2,memdev=mem2
>>
>> Known Issue:
>>
>> It is observed that hot adding nvdimm will results in guest reboot
>> failure. EDK2 fails to build the ACPI tables on reboot. Please find
>> below EDK2 log on Guest reboot after nvdimm hot-add,
>>
>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>>
>> The root cause seems to be EDK2 ACPI table checksum failure
>> as NFIT table is getting updated on hot-add. This needs further
>> investigation.
> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
Huh, very interesting; I usually don't expect my sanity checks to fire
in practice. :)
The message
ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
linker/loader script.
Please see the command definition in QEMU's
"hw/acpi/bios-linker-loader.c". In particular, please refer to the
function bios_linker_loader_add_checksum(), which builds the command
structure, and documents the fields.
(You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
"OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
same information.)
The error message is logged if:
- the offset at which the checksum should be stored falls outside of the
size of the fw_cfg blob, or
- the range over which the checksum should be calculated falls outside
(at least in part) of the fw_cfg blob.
To me this suggests that QEMU generates an invalid COMMAND_ADD_CHECKSUM
command for the firmware.
... I've tried to skim the patches briefly. I think there must be an
error in the DSDT building logic that is only active on reboot if an
nvdimm module was hot-added before the reboot.
Thanks,
Laszlo
>> [1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
>> [2]https://patchwork.kernel.org/patch/10724455/
>> [3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
>> [4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5
>>
>> Shameer Kolothum (4):
>> hw:acpi: Make ACPI IO address space configurable
>> hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
>> hw/arm/virt: Enable pc-dimm hotplug support
>> hw/arm/virt: Add nvdimm hotplug support
>>
>> default-configs/arm-softmmu.mak | 1 +
>> hw/acpi/memory_hotplug.c | 13 +++--
>> hw/arm/virt-acpi-build.c | 45 +++++++++++++++--
>> hw/arm/virt.c | 105 ++++++++++++++++++++++++++++++++++++---
>> hw/i386/acpi-build.c | 3 +-
>> include/hw/acpi/memory_hotplug.h | 6 ++-
>> include/hw/arm/virt.h | 15 ++++++
>> 7 files changed, 168 insertions(+), 20 deletions(-)
>>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-22 19:11 ` Laszlo Ersek
@ 2019-02-25 9:54 ` Shameerali Kolothum Thodi
2019-02-27 12:55 ` Shameerali Kolothum Thodi
1 sibling, 0 replies; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-02-25 9:54 UTC (permalink / raw)
To: Laszlo Ersek, Auger Eric, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: 22 February 2019 19:11
> To: Auger Eric <eric.auger@redhat.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; shannon.zhaosl@gmail.com;
> peter.maydell@linaro.org; imammedo@redhat.com; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org
> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> <leif.lindholm@linaro.org>
> Subject: Re: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>
> On 02/22/19 17:03, Auger Eric wrote:
> > Hi Shameer,
> >
> > On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> >> This series is an attempt to provide hotplug support to both
> >> pc-dimm and nvdimm device memory on ARM virt platform. This is
> >> based on Eric's recent works to support PCDIMM/NVDIMM device
> memory[1].
> >> The kernel support for arm64 memory hot add was added only
> >> recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.
> >>
> >> This makes use of PL061 GPIO controller to sent related ACPI events
> > s/sent/send
> >> to the Guest. The only reference I could find with respect to the GPIO
> >> pins usage is here[3] which says, "use PIN 3 for system_powerdown,
> >> reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
> >> Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.
> >>
> >> This is sanity tested on a HiSilicon ARM64 platform and appreciate
> >> any further testing.
> >
> > I did some testing on another platform and I got the exactly the same
> > results as yours: PCDIMM hot plug works fine. Also after system_reset I
> > still can see the slots.
> > Hot-unplug is not supported though.
> > For NVDIMM, hot-add works fine and and I can see the slots using ndctl
> > on guest. But after system_reset, the guest does not boot properly.
> >
> >>
> >> This series can be applied on top of Eric's branch here[4]
> >>
> >> Test:
> >> ------
> >> Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
> >> hotplug related CONFIGs enabled.
> >>
> >> ./qemu-system-aarch64 \
> >> -machine virt,gic-version=3,nvdimm \
> >> -m 1G,maxmem=4G,slots=4 \
> >> -cpu host \
> >> -kernel Image \
> >> -initrd rootfs-iperf.cpio \
> >> -bios QEMU_EFI.fd \
> >> -numa node,nodeid=0 \
> >> -net none \
> >> -nographic -enable-kvm \
> >> -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
> >>
> >> Enter Qemu monitor,
> >> Add pc-dimm:
> >> object_add memory-backend-ram,id=mem1,size=1G
> >> device_add pc-dimm,id=dimm1,memdev=mem1
> >>
> >> Add nvdimm:
> >> object_add memory-backend-ram,id=mem2,size=1G
> >> device_add nvdimm,id=dimm2,memdev=mem2
> >>
> >> Known Issue:
> >>
> >> It is observed that hot adding nvdimm will results in guest reboot
> >> failure. EDK2 fails to build the ACPI tables on reboot. Please find
> >> below EDK2 log on Guest reboot after nvdimm hot-add,
> >>
> >> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> >>
> >> The root cause seems to be EDK2 ACPI table checksum failure
> >> as NFIT table is getting updated on hot-add. This needs further
> >> investigation.
> > + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>
> Huh, very interesting; I usually don't expect my sanity checks to fire
> in practice. :)
>
> The message
>
> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>
> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> linker/loader script.
>
> Please see the command definition in QEMU's
> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> function bios_linker_loader_add_checksum(), which builds the command
> structure, and documents the fields.
>
> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
> same information.)
>
> The error message is logged if:
> - the offset at which the checksum should be stored falls outside of the
> size of the fw_cfg blob, or
> - the range over which the checksum should be calculated falls outside
> (at least in part) of the fw_cfg blob.
>
> To me this suggests that QEMU generates an invalid
> COMMAND_ADD_CHECKSUM
> command for the firmware.
>
> ... I've tried to skim the patches briefly. I think there must be an
> error in the DSDT building logic that is only active on reboot if an
> nvdimm module was hot-added before the reboot.
Thanks for taking a look and the pointers. I will debug this further
and get back.
Thanks,
Shameer
> Thanks,
> Laszlo
>
>
> >> [1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
> >> [2]https://patchwork.kernel.org/patch/10724455/
> >> [3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
> >> [4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5
> >>
> >> Shameer Kolothum (4):
> >> hw:acpi: Make ACPI IO address space configurable
> >> hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
> >> hw/arm/virt: Enable pc-dimm hotplug support
> >> hw/arm/virt: Add nvdimm hotplug support
> >>
> >> default-configs/arm-softmmu.mak | 1 +
> >> hw/acpi/memory_hotplug.c | 13 +++--
> >> hw/arm/virt-acpi-build.c | 45 +++++++++++++++--
> >> hw/arm/virt.c | 105
> ++++++++++++++++++++++++++++++++++++---
> >> hw/i386/acpi-build.c | 3 +-
> >> include/hw/acpi/memory_hotplug.h | 6 ++-
> >> include/hw/arm/virt.h | 15 ++++++
> >> 7 files changed, 168 insertions(+), 20 deletions(-)
> >>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-22 19:11 ` Laszlo Ersek
2019-02-25 9:54 ` Shameerali Kolothum Thodi
@ 2019-02-27 12:55 ` Shameerali Kolothum Thodi
2019-02-27 16:42 ` Igor Mammedov
2019-02-27 20:14 ` Laszlo Ersek
1 sibling, 2 replies; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-02-27 12:55 UTC (permalink / raw)
To: Laszlo Ersek, Auger Eric, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
Hi Laszlo,
> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 25 February 2019 09:54
> To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric <eric.auger@redhat.com>;
> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> <leif.lindholm@linaro.org>
> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
[...]
> > >> The root cause seems to be EDK2 ACPI table checksum failure
> > >> as NFIT table is getting updated on hot-add. This needs further
> > >> investigation.
> > > + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> >
> > Huh, very interesting; I usually don't expect my sanity checks to fire
> > in practice. :)
> >
> > The message
> >
> > ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >
> > is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
> > finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> > linker/loader script.
> >
> > Please see the command definition in QEMU's
> > "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> > function bios_linker_loader_add_checksum(), which builds the command
> > structure, and documents the fields.
> >
> > (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> > "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
> > same information.)
> >
> > The error message is logged if:
> > - the offset at which the checksum should be stored falls outside of the
> > size of the fw_cfg blob, or
> > - the range over which the checksum should be calculated falls outside
> > (at least in part) of the fw_cfg blob.
> >
> > To me this suggests that QEMU generates an invalid
> > COMMAND_ADD_CHECKSUM
> > command for the firmware.
> >
> > ... I've tried to skim the patches briefly. I think there must be an
> > error in the DSDT building logic that is only active on reboot if an
> > nvdimm module was hot-added before the reboot.
>
> Thanks for taking a look and the pointers. I will debug this further
> and get back.
The root cause of the issue seems to be UEFI not seeing the updated acpi
table blob size on reboot once a new NFIT table is added(nvdimm hot added).
Please see the debug logs below,
Initial Guest boot
---------------------------
Debug logs from Qemu:
build_header: acpi sig DSDT len 0x5127
build_header: acpi sig FACP len 0x10c
build_header: acpi sig APIC len 0xa8
build_header: acpi sig GTDT len 0x60
build_header: acpi sig MCFG len 0x3c
build_header: acpi sig SPCR len 0x50
build_header: acpi sig SRAT len 0x92
build_header: acpi sig SSDT len 0x38f
build_header: acpi sig XSDT len 0x5c
virt_acpi_build: acpi table_blob len 0x5844
Debug logs from UEFI:
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
InstallQemuFwCfgTables: installed 8 tables
Guest Reboot after ndimm hot added
------------------------------------
Debug logs from Qemu:
build_header: acpi sig DSDT len 0x5127
build_header: acpi sig FACP len 0x10c
build_header: acpi sig APIC len 0xa8
build_header: acpi sig GTDT len 0x60
build_header: acpi sig MCFG len 0x3c
build_header: acpi sig SPCR len 0x50
build_header: acpi sig SRAT len 0x92
build_header: acpi sig SSDT len 0x38f
build_header: acpi sig NFIT len 0xe0 -->New
build_header: acpi sig XSDT len 0x64
virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
Debug logs from UEFI:
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
OnRootBridgesConnected: InstallAcpiTables: Protocol Error
To me it seems on ARM vit acpi path, the blob len is calculated based
on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
path. I had a look at the x86 code and it looks like, there, the blob len gets updated
with an additional buffer to take care of table resizing[1].
As a hack i added the same to ARM virt and it seems to resolve the issue.
I am not sure this is the best approach to fix this though.
Please let me know your thoughts.
Thanks,
Shameer
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 132414c..4291553 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -50,6 +50,8 @@
#define ARM_SPI_BASE 32
#define ACPI_POWER_BUTTON_DEVICE "PWRB"
+#define ACPI_BUILD_TABLE_SIZE 0x20000
+
static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
{
uint16_t i;
@@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
}
+ /* Make sure we have a buffer in case we need to resize the tables. */
+ g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
+ ACPI_BUILD_TABLE_SIZE));
+
/* Cleanup memory that's no longer used. */
g_array_free(table_offsets, true);
}
[1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-27 12:55 ` Shameerali Kolothum Thodi
@ 2019-02-27 16:42 ` Igor Mammedov
2019-02-27 20:14 ` Laszlo Ersek
1 sibling, 0 replies; 36+ messages in thread
From: Igor Mammedov @ 2019-02-27 16:42 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: Laszlo Ersek, Auger Eric, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
On Wed, 27 Feb 2019 12:55:18 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> Hi Laszlo,
>
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 25 February 2019 09:54
> > To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric <eric.auger@redhat.com>;
> > shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> > imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> > Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
> > Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> > <leif.lindholm@linaro.org>
> > Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>
> [...]
>
> > > >> The root cause seems to be EDK2 ACPI table checksum failure
> > > >> as NFIT table is getting updated on hot-add. This needs further
> > > >> investigation.
> > > > + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> > >
> > > Huh, very interesting; I usually don't expect my sanity checks to fire
> > > in practice. :)
> > >
> > > The message
> > >
> > > ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> > >
> > > is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
> > > finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> > > linker/loader script.
> > >
> > > Please see the command definition in QEMU's
> > > "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> > > function bios_linker_loader_add_checksum(), which builds the command
> > > structure, and documents the fields.
> > >
> > > (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> > > "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
> > > same information.)
> > >
> > > The error message is logged if:
> > > - the offset at which the checksum should be stored falls outside of the
> > > size of the fw_cfg blob, or
> > > - the range over which the checksum should be calculated falls outside
> > > (at least in part) of the fw_cfg blob.
> > >
> > > To me this suggests that QEMU generates an invalid
> > > COMMAND_ADD_CHECKSUM
> > > command for the firmware.
> > >
> > > ... I've tried to skim the patches briefly. I think there must be an
> > > error in the DSDT building logic that is only active on reboot if an
> > > nvdimm module was hot-added before the reboot.
> >
> > Thanks for taking a look and the pointers. I will debug this further
> > and get back.
>
> The root cause of the issue seems to be UEFI not seeing the updated acpi
> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
>
> Please see the debug logs below,
>
> Initial Guest boot
> ---------------------------
>
> Debug logs from Qemu:
>
> build_header: acpi sig DSDT len 0x5127
> build_header: acpi sig FACP len 0x10c
> build_header: acpi sig APIC len 0xa8
> build_header: acpi sig GTDT len 0x60
> build_header: acpi sig MCFG len 0x3c
> build_header: acpi sig SPCR len 0x50
> build_header: acpi sig SRAT len 0x92
> build_header: acpi sig SSDT len 0x38f
> build_header: acpi sig XSDT len 0x5c
> virt_acpi_build: acpi table_blob len 0x5844
>
> Debug logs from UEFI:
>
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
> InstallQemuFwCfgTables: installed 8 tables
>
> Guest Reboot after ndimm hot added
> ------------------------------------
>
> Debug logs from Qemu:
>
> build_header: acpi sig DSDT len 0x5127
> build_header: acpi sig FACP len 0x10c
> build_header: acpi sig APIC len 0xa8
> build_header: acpi sig GTDT len 0x60
> build_header: acpi sig MCFG len 0x3c
> build_header: acpi sig SPCR len 0x50
> build_header: acpi sig SRAT len 0x92
> build_header: acpi sig SSDT len 0x38f
> build_header: acpi sig NFIT len 0xe0 -->New
> build_header: acpi sig XSDT len 0x64
> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
>
> Debug logs from UEFI:
>
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>
>
> To me it seems on ARM vit acpi path, the blob len is calculated based
> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
> with an additional buffer to take care of table resizing[1].
>
> As a hack i added the same to ARM virt and it seems to resolve the issue.
> I am not sure this is the best approach to fix this though.
>
> Please let me know your thoughts.
>
> Thanks,
> Shameer
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 132414c..4291553 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -50,6 +50,8 @@
> #define ARM_SPI_BASE 32
> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>
> +#define ACPI_BUILD_TABLE_SIZE 0x20000
> +
> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> {
> uint16_t i;
> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> }
>
> + /* Make sure we have a buffer in case we need to resize the tables. */
> + g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
> + ACPI_BUILD_TABLE_SIZE));
not sure fixup is correct approach.
On reset (on QEMU level), it's upto to QEMU to rebuild tables and it's
upto firmware to reread those.
Maybe issue existed before hotplug it's just that hotplug exposes it.
(something is missing compared to x86 or we have the same issue
there too just no one have triggered it yet).
I suggest to find root cause first before we start paper over it.
> +
> /* Cleanup memory that's no longer used. */
> g_array_free(table_offsets, true);
> }
>
> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-27 12:55 ` Shameerali Kolothum Thodi
2019-02-27 16:42 ` Igor Mammedov
@ 2019-02-27 20:14 ` Laszlo Ersek
2019-02-28 10:12 ` Auger Eric
1 sibling, 1 reply; 36+ messages in thread
From: Laszlo Ersek @ 2019-02-27 20:14 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Auger Eric, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
> Hi Laszlo,
>
>> -----Original Message-----
>> From: Shameerali Kolothum Thodi
>> Sent: 25 February 2019 09:54
>> To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric <eric.auger@redhat.com>;
>> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
>> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
>> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
>> <leif.lindholm@linaro.org>
>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>
> [...]
>
>>>>> The root cause seems to be EDK2 ACPI table checksum failure
>>>>> as NFIT table is getting updated on hot-add. This needs further
>>>>> investigation.
>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>>>
>>> Huh, very interesting; I usually don't expect my sanity checks to fire
>>> in practice. :)
>>>
>>> The message
>>>
>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>
>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
>>> linker/loader script.
>>>
>>> Please see the command definition in QEMU's
>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
>>> function bios_linker_loader_add_checksum(), which builds the command
>>> structure, and documents the fields.
>>>
>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
>>> same information.)
>>>
>>> The error message is logged if:
>>> - the offset at which the checksum should be stored falls outside of the
>>> size of the fw_cfg blob, or
>>> - the range over which the checksum should be calculated falls outside
>>> (at least in part) of the fw_cfg blob.
>>>
>>> To me this suggests that QEMU generates an invalid
>>> COMMAND_ADD_CHECKSUM
>>> command for the firmware.
>>>
>>> ... I've tried to skim the patches briefly. I think there must be an
>>> error in the DSDT building logic that is only active on reboot if an
>>> nvdimm module was hot-added before the reboot.
>>
>> Thanks for taking a look and the pointers. I will debug this further
>> and get back.
>
> The root cause of the issue seems to be UEFI not seeing the updated acpi
> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
>
> Please see the debug logs below,
>
> Initial Guest boot
> ---------------------------
>
> Debug logs from Qemu:
>
> build_header: acpi sig DSDT len 0x5127
> build_header: acpi sig FACP len 0x10c
> build_header: acpi sig APIC len 0xa8
> build_header: acpi sig GTDT len 0x60
> build_header: acpi sig MCFG len 0x3c
> build_header: acpi sig SPCR len 0x50
> build_header: acpi sig SRAT len 0x92
> build_header: acpi sig SSDT len 0x38f
> build_header: acpi sig XSDT len 0x5c
> virt_acpi_build: acpi table_blob len 0x5844
>
> Debug logs from UEFI:
>
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
> InstallQemuFwCfgTables: installed 8 tables
>
> Guest Reboot after ndimm hot added
> ------------------------------------
>
> Debug logs from Qemu:
>
> build_header: acpi sig DSDT len 0x5127
> build_header: acpi sig FACP len 0x10c
> build_header: acpi sig APIC len 0xa8
> build_header: acpi sig GTDT len 0x60
> build_header: acpi sig MCFG len 0x3c
> build_header: acpi sig SPCR len 0x50
> build_header: acpi sig SRAT len 0x92
> build_header: acpi sig SSDT len 0x38f
> build_header: acpi sig NFIT len 0xe0 -->New
> build_header: acpi sig XSDT len 0x64
> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
>
> Debug logs from UEFI:
>
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>
>
> To me it seems on ARM vit acpi path, the blob len is calculated based
> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
> with an additional buffer to take care of table resizing[1].
>
> As a hack i added the same to ARM virt and it seems to resolve the issue.
> I am not sure this is the best approach to fix this though.
>
> Please let me know your thoughts.
>
> Thanks,
> Shameer
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 132414c..4291553 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -50,6 +50,8 @@
> #define ARM_SPI_BASE 32
> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>
> +#define ACPI_BUILD_TABLE_SIZE 0x20000
> +
> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> {
> uint16_t i;
> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> }
>
> + /* Make sure we have a buffer in case we need to resize the tables. */
> + g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
> + ACPI_BUILD_TABLE_SIZE));
> +
> /* Cleanup memory that's no longer used. */
> g_array_free(table_offsets, true);
> }
>
> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
Nice analysis, thanks.
I think the line that you reference, i.e.
acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
a side effect. To my understanding, the alignment / padding exists there
for migration compatibility. It doesn't exist for updating the size of
the ACPI blobs in fw_cfg across reboots. The issue is masked because the
alignment is large enough (un-changed) to contain the regenerated blobs
as well.
Given that the "virt" machine type is versioned, I think migration
compat is a valid concern there too. This in itself would justify a
similar padding.
I don't know if we want to specifically care about size-changing
ACPI-regen across reboot. I believe measures for that specific use case
don't exist in x86 machine types either.
Another trick that is occasionally used (but might not apply here, I'm
uncertain) is to always generate the relevant ACPI objects, but, in case
they are not justified for the virtual hardware config, invalidate them
by overwriting particular parts of them (for example, one or two bytes
of their names). Hopefully this shouldn't introduce ACPI or AML errors,
just make the ACPI interpreter ignore the affected objects.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-27 20:14 ` Laszlo Ersek
@ 2019-02-28 10:12 ` Auger Eric
2019-02-28 12:04 ` Shameerali Kolothum Thodi
` (2 more replies)
0 siblings, 3 replies; 36+ messages in thread
From: Auger Eric @ 2019-02-28 10:12 UTC (permalink / raw)
To: Laszlo Ersek, Shameerali Kolothum Thodi, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
Hi Laszlo,
On 2/27/19 9:14 PM, Laszlo Ersek wrote:
> On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
>> Hi Laszlo,
>>
>>> -----Original Message-----
>>> From: Shameerali Kolothum Thodi
>>> Sent: 25 February 2019 09:54
>>> To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric <eric.auger@redhat.com>;
>>> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
>>> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>>> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
>>> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
>>> <leif.lindholm@linaro.org>
>>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>>
>> [...]
>>
>>>>>> The root cause seems to be EDK2 ACPI table checksum failure
>>>>>> as NFIT table is getting updated on hot-add. This needs further
>>>>>> investigation.
>>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>>>>
>>>> Huh, very interesting; I usually don't expect my sanity checks to fire
>>>> in practice. :)
>>>>
>>>> The message
>>>>
>>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>>
>>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
>>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
>>>> linker/loader script.
>>>>
>>>> Please see the command definition in QEMU's
>>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
>>>> function bios_linker_loader_add_checksum(), which builds the command
>>>> structure, and documents the fields.
>>>>
>>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
>>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
>>>> same information.)
>>>>
>>>> The error message is logged if:
>>>> - the offset at which the checksum should be stored falls outside of the
>>>> size of the fw_cfg blob, or
>>>> - the range over which the checksum should be calculated falls outside
>>>> (at least in part) of the fw_cfg blob.
>>>>
>>>> To me this suggests that QEMU generates an invalid
>>>> COMMAND_ADD_CHECKSUM
>>>> command for the firmware.
>>>>
>>>> ... I've tried to skim the patches briefly. I think there must be an
>>>> error in the DSDT building logic that is only active on reboot if an
>>>> nvdimm module was hot-added before the reboot.
>>>
>>> Thanks for taking a look and the pointers. I will debug this further
>>> and get back.
>>
>> The root cause of the issue seems to be UEFI not seeing the updated acpi
>> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
>>
>> Please see the debug logs below,
>>
>> Initial Guest boot
>> ---------------------------
>>
>> Debug logs from Qemu:
>>
>> build_header: acpi sig DSDT len 0x5127
>> build_header: acpi sig FACP len 0x10c
>> build_header: acpi sig APIC len 0xa8
>> build_header: acpi sig GTDT len 0x60
>> build_header: acpi sig MCFG len 0x3c
>> build_header: acpi sig SPCR len 0x50
>> build_header: acpi sig SRAT len 0x92
>> build_header: acpi sig SSDT len 0x38f
>> build_header: acpi sig XSDT len 0x5c
>> virt_acpi_build: acpi table_blob len 0x5844
>>
>> Debug logs from UEFI:
>>
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
>> InstallQemuFwCfgTables: installed 8 tables
>>
>> Guest Reboot after ndimm hot added
>> ------------------------------------
>>
>> Debug logs from Qemu:
>>
>> build_header: acpi sig DSDT len 0x5127
>> build_header: acpi sig FACP len 0x10c
>> build_header: acpi sig APIC len 0xa8
>> build_header: acpi sig GTDT len 0x60
>> build_header: acpi sig MCFG len 0x3c
>> build_header: acpi sig SPCR len 0x50
>> build_header: acpi sig SRAT len 0x92
>> build_header: acpi sig SSDT len 0x38f
>> build_header: acpi sig NFIT len 0xe0 -->New
>> build_header: acpi sig XSDT len 0x64
>> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
>>
>> Debug logs from UEFI:
>>
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>>
>>
>> To me it seems on ARM vit acpi path, the blob len is calculated based
>> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
>> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
>> with an additional buffer to take care of table resizing[1].
>>
>> As a hack i added the same to ARM virt and it seems to resolve the issue.
>> I am not sure this is the best approach to fix this though.
>>
>> Please let me know your thoughts.
>>
>> Thanks,
>> Shameer
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index 132414c..4291553 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -50,6 +50,8 @@
>> #define ARM_SPI_BASE 32
>> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>
>> +#define ACPI_BUILD_TABLE_SIZE 0x20000
>> +
>> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>> {
>> uint16_t i;
>> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>> build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>> }
>>
>> + /* Make sure we have a buffer in case we need to resize the tables. */
>> + g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
>> + ACPI_BUILD_TABLE_SIZE));
>> +
>> /* Cleanup memory that's no longer used. */
>> g_array_free(table_offsets, true);
>> }
>>
>> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
>
> Nice analysis, thanks.
>
> I think the line that you reference, i.e.
>
> acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>
> in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
> a side effect. To my understanding, the alignment / padding exists there
> for migration compatibility. It doesn't exist for updating the size of
> the ACPI blobs in fw_cfg across reboots. The issue is masked because the
> alignment is large enough (un-changed) to contain the regenerated blobs
> as well.>
> Given that the "virt" machine type is versioned, I think migration
> compat is a valid concern there too. This in itself would justify a
> similar padding.
I don't understand the migration compat issue. Please could you elaborate?
>
> I don't know if we want to specifically care about size-changing
> ACPI-regen across reboot. I believe measures for that specific use case
> don't exist in x86 machine types either.
The NFIT redimensioning should exit on x86 too?
>
> Another trick that is occasionally used (but might not apply here, I'm
> uncertain) is to always generate the relevant ACPI objects, but, in case
> they are not justified for the virtual hardware config, invalidate them
> by overwriting particular parts of them (for example, one or two bytes
> of their names). Hopefully this shouldn't introduce ACPI or AML errors,
> just make the ACPI interpreter ignore the affected objects.
Thanks!
Eric
>
> Thanks,
> Laszlo
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-28 10:12 ` Auger Eric
@ 2019-02-28 12:04 ` Shameerali Kolothum Thodi
2019-02-28 12:27 ` Laszlo Ersek
2019-02-28 14:02 ` Shameerali Kolothum Thodi
2 siblings, 0 replies; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-02-28 12:04 UTC (permalink / raw)
To: Auger Eric, Laszlo Ersek, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 28 February 2019 10:12
> To: Laszlo Ersek <lersek@redhat.com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@huawei.com>; shannon.zhaosl@gmail.com;
> peter.maydell@linaro.org; imammedo@redhat.com; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org
> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> <leif.lindholm@linaro.org>
> Subject: Re: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>
> Hi Laszlo,
>
> On 2/27/19 9:14 PM, Laszlo Ersek wrote:
> > On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
> >> Hi Laszlo,
> >>
> >>> -----Original Message-----
> >>> From: Shameerali Kolothum Thodi
> >>> Sent: 25 February 2019 09:54
> >>> To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric
> <eric.auger@redhat.com>;
> >>> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> >>> imammedo@redhat.com; qemu-devel@nongnu.org;
> qemu-arm@nongnu.org
> >>> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> Ard
> >>> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> >>> <leif.lindholm@linaro.org>
> >>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> >>
> >> [...]
> >>
> >>>>>> The root cause seems to be EDK2 ACPI table checksum failure
> >>>>>> as NFIT table is getting updated on hot-add. This needs further
> >>>>>> investigation.
> >>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> >>>>
> >>>> Huh, very interesting; I usually don't expect my sanity checks to fire
> >>>> in practice. :)
> >>>>
> >>>> The message
> >>>>
> >>>> ProcessCmdAddChecksum: invalid checksum range in
> "etc/acpi/tables"
> >>>>
> >>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when
> it
> >>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> >>>> linker/loader script.
> >>>>
> >>>> Please see the command definition in QEMU's
> >>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> >>>> function bios_linker_loader_add_checksum(), which builds the command
> >>>> structure, and documents the fields.
> >>>>
> >>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> >>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for
> the
> >>>> same information.)
> >>>>
> >>>> The error message is logged if:
> >>>> - the offset at which the checksum should be stored falls outside of the
> >>>> size of the fw_cfg blob, or
> >>>> - the range over which the checksum should be calculated falls outside
> >>>> (at least in part) of the fw_cfg blob.
> >>>>
> >>>> To me this suggests that QEMU generates an invalid
> >>>> COMMAND_ADD_CHECKSUM
> >>>> command for the firmware.
> >>>>
> >>>> ... I've tried to skim the patches briefly. I think there must be an
> >>>> error in the DSDT building logic that is only active on reboot if an
> >>>> nvdimm module was hot-added before the reboot.
> >>>
> >>> Thanks for taking a look and the pointers. I will debug this further
> >>> and get back.
> >>
> >> The root cause of the issue seems to be UEFI not seeing the updated acpi
> >> table blob size on reboot once a new NFIT table is added(nvdimm hot
> added).
> >>
> >> Please see the debug logs below,
> >>
> >> Initial Guest boot
> >> ---------------------------
> >>
> >> Debug logs from Qemu:
> >>
> >> build_header: acpi sig DSDT len 0x5127
> >> build_header: acpi sig FACP len 0x10c
> >> build_header: acpi sig APIC len 0xa8
> >> build_header: acpi sig GTDT len 0x60
> >> build_header: acpi sig MCFG len 0x3c
> >> build_header: acpi sig SPCR len 0x50
> >> build_header: acpi sig SRAT len 0x92
> >> build_header: acpi sig SSDT len 0x38f
> >> build_header: acpi sig XSDT len 0x5c
> >> virt_acpi_build: acpi table_blob len 0x5844
> >>
> >> Debug logs from UEFI:
> >>
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9
> Start=0x0 Length=0x5127 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130
> Start=0x5127 Length=0x10C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C
> Start=0x5233 Length=0xA8 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4
> Start=0x52DB Length=0x60 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344
> Start=0x533B Length=0x3C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380
> Start=0x5377 Length=0x50 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0
> Start=0x53C7 Length=0x92 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462
> Start=0x5459 Length=0x38F Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1
> Start=0x57E8 Length=0x5C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0
> Length=0x14 Blob->Size=0x24
> >> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20
> Start=0x0 Length=0x24 Blob->Size=0x24
> >> InstallQemuFwCfgTables: installed 8 tables
> >>
> >> Guest Reboot after ndimm hot added
> >> ------------------------------------
> >>
> >> Debug logs from Qemu:
> >>
> >> build_header: acpi sig DSDT len 0x5127
> >> build_header: acpi sig FACP len 0x10c
> >> build_header: acpi sig APIC len 0xa8
> >> build_header: acpi sig GTDT len 0x60
> >> build_header: acpi sig MCFG len 0x3c
> >> build_header: acpi sig SPCR len 0x50
> >> build_header: acpi sig SRAT len 0x92
> >> build_header: acpi sig SSDT len 0x38f
> >> build_header: acpi sig NFIT len 0xe0 -->New
> >> build_header: acpi sig XSDT len 0x64
> >> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
> >>
> >> Debug logs from UEFI:
> >>
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9
> Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130
> Start=0x5127 Length=0x10C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C
> Start=0x5233 Length=0xA8 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4
> Start=0x52DB Length=0x60 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344
> Start=0x533B Length=0x3C Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380
> Start=0x5377 Length=0x50 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0
> Start=0x53C7 Length=0x92 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462
> Start=0x5459 Length=0x38F Blob->Size=0x5844
> >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1
> Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> >> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> >>
> >>
> >> To me it seems on ARM vit acpi path, the blob len is calculated based
> >> on actual tables and is updated only in virt_acpi_setup() -->
> acpi_add_rom_blob()
> >> path. I had a look at the x86 code and it looks like, there, the blob len gets
> updated
> >> with an additional buffer to take care of table resizing[1].
> >>
> >> As a hack i added the same to ARM virt and it seems to resolve the issue.
> >> I am not sure this is the best approach to fix this though.
> >>
> >> Please let me know your thoughts.
> >>
> >> Thanks,
> >> Shameer
> >>
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 132414c..4291553 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -50,6 +50,8 @@
> >> #define ARM_SPI_BASE 32
> >> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >>
> >> +#define ACPI_BUILD_TABLE_SIZE 0x20000
> >> +
> >> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >> {
> >> uint16_t i;
> >> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms,
> AcpiBuildTables *tables)
> >> build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> >> }
> >>
> >> + /* Make sure we have a buffer in case we need to resize the tables. */
> >> + g_array_set_size(tables_blob,
> ROUND_UP(acpi_data_len(tables_blob),
> >> + ACPI_BUILD_TABLE_SIZE));
> >> +
> >> /* Cleanup memory that's no longer used. */
> >> g_array_free(table_offsets, true);
> >> }
> >>
> >> [1]
> https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
> >
> > Nice analysis, thanks.
> >
> > I think the line that you reference, i.e.
> >
> > acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
Yes.
> > in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
> > a side effect. To my understanding, the alignment / padding exists there
> > for migration compatibility. It doesn't exist for updating the size of
> > the ACPI blobs in fw_cfg across reboots. The issue is masked because the
> > alignment is large enough (un-changed) to contain the regenerated blobs
> > as well.>
> > Given that the "virt" machine type is versioned, I think migration
> > compat is a valid concern there too. This in itself would justify a
> > similar padding.
> I don't understand the migration compat issue. Please could you elaborate?
Yes. Please elaborate.
> > I don't know if we want to specifically care about size-changing
> > ACPI-regen across reboot. I believe measures for that specific use case
> > don't exist in x86 machine types either.
> The NFIT redimensioning should exit on x86 too?
I had a go with x86 commenting out the padding completely. But interestingly
somewhere in x86 code path the blob length gets aligned to 0x1000 boundary
and it escapes the sanity check.
With padding, UEFI always sees, ACPI_BUILD_TABLE_SIZE (0x20000)
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 Length=0x19E9 Blob->Size=0x20000
With padding removed,
From Qemu,
acpi_build: tables_blob len 0x2c62
UEFI:
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 Length=0x1F33 Blob->Size=0x3000
Guest Reboot after adding a number of nvdimms,
Qemu:
acpi_build: tables_blob len 0x380e
UEFI:
ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49 Start=0x40 Length=0x2401 Blob->Size=0x4000
I couldn’t figure out from the code where that round up is happening. But
x86 also has same issue if the padding is removed, I guess.
Thanks,
Shameer
> > Another trick that is occasionally used (but might not apply here, I'm
> > uncertain) is to always generate the relevant ACPI objects, but, in case
> > they are not justified for the virtual hardware config, invalidate them
> > by overwriting particular parts of them (for example, one or two bytes
> > of their names). Hopefully this shouldn't introduce ACPI or AML errors,
> > just make the ACPI interpreter ignore the affected objects.
>
> Thanks!
>
> Eric
> >
> > Thanks,
> > Laszlo
> >
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-28 10:12 ` Auger Eric
2019-02-28 12:04 ` Shameerali Kolothum Thodi
@ 2019-02-28 12:27 ` Laszlo Ersek
2019-02-28 13:32 ` Auger Eric
2019-02-28 13:43 ` Igor Mammedov
2019-02-28 14:02 ` Shameerali Kolothum Thodi
2 siblings, 2 replies; 36+ messages in thread
From: Laszlo Ersek @ 2019-02-28 12:27 UTC (permalink / raw)
To: Auger Eric, Shameerali Kolothum Thodi, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
On 02/28/19 11:12, Auger Eric wrote:
> Hi Laszlo,
>
> On 2/27/19 9:14 PM, Laszlo Ersek wrote:
>> On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
>>> Hi Laszlo,
>>>
>>>> -----Original Message-----
>>>> From: Shameerali Kolothum Thodi
>>>> Sent: 25 February 2019 09:54
>>>> To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric <eric.auger@redhat.com>;
>>>> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
>>>> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>>>> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
>>>> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
>>>> <leif.lindholm@linaro.org>
>>>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>>>
>>> [...]
>>>
>>>>>>> The root cause seems to be EDK2 ACPI table checksum failure
>>>>>>> as NFIT table is getting updated on hot-add. This needs further
>>>>>>> investigation.
>>>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>>>>>
>>>>> Huh, very interesting; I usually don't expect my sanity checks to fire
>>>>> in practice. :)
>>>>>
>>>>> The message
>>>>>
>>>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>>>
>>>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
>>>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
>>>>> linker/loader script.
>>>>>
>>>>> Please see the command definition in QEMU's
>>>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
>>>>> function bios_linker_loader_add_checksum(), which builds the command
>>>>> structure, and documents the fields.
>>>>>
>>>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
>>>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
>>>>> same information.)
>>>>>
>>>>> The error message is logged if:
>>>>> - the offset at which the checksum should be stored falls outside of the
>>>>> size of the fw_cfg blob, or
>>>>> - the range over which the checksum should be calculated falls outside
>>>>> (at least in part) of the fw_cfg blob.
>>>>>
>>>>> To me this suggests that QEMU generates an invalid
>>>>> COMMAND_ADD_CHECKSUM
>>>>> command for the firmware.
>>>>>
>>>>> ... I've tried to skim the patches briefly. I think there must be an
>>>>> error in the DSDT building logic that is only active on reboot if an
>>>>> nvdimm module was hot-added before the reboot.
>>>>
>>>> Thanks for taking a look and the pointers. I will debug this further
>>>> and get back.
>>>
>>> The root cause of the issue seems to be UEFI not seeing the updated acpi
>>> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
>>>
>>> Please see the debug logs below,
>>>
>>> Initial Guest boot
>>> ---------------------------
>>>
>>> Debug logs from Qemu:
>>>
>>> build_header: acpi sig DSDT len 0x5127
>>> build_header: acpi sig FACP len 0x10c
>>> build_header: acpi sig APIC len 0xa8
>>> build_header: acpi sig GTDT len 0x60
>>> build_header: acpi sig MCFG len 0x3c
>>> build_header: acpi sig SPCR len 0x50
>>> build_header: acpi sig SRAT len 0x92
>>> build_header: acpi sig SSDT len 0x38f
>>> build_header: acpi sig XSDT len 0x5c
>>> virt_acpi_build: acpi table_blob len 0x5844
>>>
>>> Debug logs from UEFI:
>>>
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
>>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
>>> InstallQemuFwCfgTables: installed 8 tables
>>>
>>> Guest Reboot after ndimm hot added
>>> ------------------------------------
>>>
>>> Debug logs from Qemu:
>>>
>>> build_header: acpi sig DSDT len 0x5127
>>> build_header: acpi sig FACP len 0x10c
>>> build_header: acpi sig APIC len 0xa8
>>> build_header: acpi sig GTDT len 0x60
>>> build_header: acpi sig MCFG len 0x3c
>>> build_header: acpi sig SPCR len 0x50
>>> build_header: acpi sig SRAT len 0x92
>>> build_header: acpi sig SSDT len 0x38f
>>> build_header: acpi sig NFIT len 0xe0 -->New
>>> build_header: acpi sig XSDT len 0x64
>>> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
>>>
>>> Debug logs from UEFI:
>>>
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>>>
>>>
>>> To me it seems on ARM vit acpi path, the blob len is calculated based
>>> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
>>> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
>>> with an additional buffer to take care of table resizing[1].
>>>
>>> As a hack i added the same to ARM virt and it seems to resolve the issue.
>>> I am not sure this is the best approach to fix this though.
>>>
>>> Please let me know your thoughts.
>>>
>>> Thanks,
>>> Shameer
>>>
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index 132414c..4291553 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -50,6 +50,8 @@
>>> #define ARM_SPI_BASE 32
>>> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>>
>>> +#define ACPI_BUILD_TABLE_SIZE 0x20000
>>> +
>>> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>>> {
>>> uint16_t i;
>>> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>> build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>>> }
>>>
>>> + /* Make sure we have a buffer in case we need to resize the tables. */
>>> + g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
>>> + ACPI_BUILD_TABLE_SIZE));
>>> +
>>> /* Cleanup memory that's no longer used. */
>>> g_array_free(table_offsets, true);
>>> }
>>>
>>> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
>>
>> Nice analysis, thanks.
>>
>> I think the line that you reference, i.e.
>>
>> acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>>
>> in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
>> a side effect. To my understanding, the alignment / padding exists there
>> for migration compatibility. It doesn't exist for updating the size of
>> the ACPI blobs in fw_cfg across reboots. The issue is masked because the
>> alignment is large enough (un-changed) to contain the regenerated blobs
>> as well.>
>> Given that the "virt" machine type is versioned, I think migration
>> compat is a valid concern there too. This in itself would justify a
>> similar padding.
> I don't understand the migration compat issue. Please could you elaborate?
git-blame explains it to some extent -- please see commit 07fb61760cde
("pc: hack for migration compatibility from QEMU 2.0", 2014-07-28).
I don't remember any details at this point that the commit does not
state. (I see that I reviewed the patch back then, so perhaps the
mailing list archive has some discussion.)
Interestingly, the commit message refers to "memory hotplug work" too.
... Ahh, wait, I do remember the main issue now. Here's the thing. The
ACPI payload that QEMU generates for the firmware is considered a part
of the firmware itself. Therefore, it is not versioned -- because the
firmware itself is not versioned. (In other words, if you migrate a VM
from one host to another host, and that other host has different
firmware that the VM will pick up after re-launch (from cold boot), then
the firmware will change in the VM.)
By considering ACPI a part of the firmware, QEMU never versioned the
ACPI payload, just like the actual firmware was never versioned. In
other words, if you have machine type Foo on qemu release Bar, and
machine type Foo on qemu release Baz, compat properties and such will
ensure that the virtual hardware looks the same to the guest, but QEMU
will *not* ensure that the ACPI payload generated at QEMU startup (more
precisely, at "machine done") will be identical. Despite the fact that
both QEMU instances use machine type Foo.
Now, combine this with the feature that fw_cfg has been backed by RAM
Blocks, for a quite long time now (this wasn't always the case, but it
has been for multiple years now). The end result is that the RAM
block(s) holding the initial ACPI payload may differ between releases
Bar and Baz, within the same machine type Foo. This means that migration
between them will fail, due to RAMBlock size difference.
Hence the padding -- it tries to cancel out small variances in ACPI
payload size.
>>
>> I don't know if we want to specifically care about size-changing
>> ACPI-regen across reboot. I believe measures for that specific use case
>> don't exist in x86 machine types either.
> The NFIT redimensioning should exit on x86 too?
That's not my point. My point was that the padding, which was originally
supposed to mask variances in ACPI payload size across *QEMU releases*,
for migration compat, ended up masking a variance of different origin:
namely ACPI regeneration at reboot (with different contents). In other
words, we never implemented any specific measures for this
resize-on-reboot issue, instead we allowed the migration compat code
(the padding) to take care of it as well.
In virt, there is no such ACPI padding code (for migration compat) --
for whatever reason --, and so it *also* cannot take care of the
resize-on-reboot problem.
[...]
Thanks
Laszlo
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-28 12:27 ` Laszlo Ersek
@ 2019-02-28 13:32 ` Auger Eric
2019-02-28 13:43 ` Igor Mammedov
1 sibling, 0 replies; 36+ messages in thread
From: Auger Eric @ 2019-02-28 13:32 UTC (permalink / raw)
To: Laszlo Ersek, Shameerali Kolothum Thodi, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
Hi Laszlo,
On 2/28/19 1:27 PM, Laszlo Ersek wrote:
> On 02/28/19 11:12, Auger Eric wrote:
>> Hi Laszlo,
>>
>> On 2/27/19 9:14 PM, Laszlo Ersek wrote:
>>> On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
>>>> Hi Laszlo,
>>>>
>>>>> -----Original Message-----
>>>>> From: Shameerali Kolothum Thodi
>>>>> Sent: 25 February 2019 09:54
>>>>> To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric <eric.auger@redhat.com>;
>>>>> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
>>>>> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>>>>> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
>>>>> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
>>>>> <leif.lindholm@linaro.org>
>>>>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>>>>
>>>> [...]
>>>>
>>>>>>>> The root cause seems to be EDK2 ACPI table checksum failure
>>>>>>>> as NFIT table is getting updated on hot-add. This needs further
>>>>>>>> investigation.
>>>>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>>>>>>
>>>>>> Huh, very interesting; I usually don't expect my sanity checks to fire
>>>>>> in practice. :)
>>>>>>
>>>>>> The message
>>>>>>
>>>>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>>>>
>>>>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
>>>>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
>>>>>> linker/loader script.
>>>>>>
>>>>>> Please see the command definition in QEMU's
>>>>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
>>>>>> function bios_linker_loader_add_checksum(), which builds the command
>>>>>> structure, and documents the fields.
>>>>>>
>>>>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
>>>>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
>>>>>> same information.)
>>>>>>
>>>>>> The error message is logged if:
>>>>>> - the offset at which the checksum should be stored falls outside of the
>>>>>> size of the fw_cfg blob, or
>>>>>> - the range over which the checksum should be calculated falls outside
>>>>>> (at least in part) of the fw_cfg blob.
>>>>>>
>>>>>> To me this suggests that QEMU generates an invalid
>>>>>> COMMAND_ADD_CHECKSUM
>>>>>> command for the firmware.
>>>>>>
>>>>>> ... I've tried to skim the patches briefly. I think there must be an
>>>>>> error in the DSDT building logic that is only active on reboot if an
>>>>>> nvdimm module was hot-added before the reboot.
>>>>>
>>>>> Thanks for taking a look and the pointers. I will debug this further
>>>>> and get back.
>>>>
>>>> The root cause of the issue seems to be UEFI not seeing the updated acpi
>>>> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
>>>>
>>>> Please see the debug logs below,
>>>>
>>>> Initial Guest boot
>>>> ---------------------------
>>>>
>>>> Debug logs from Qemu:
>>>>
>>>> build_header: acpi sig DSDT len 0x5127
>>>> build_header: acpi sig FACP len 0x10c
>>>> build_header: acpi sig APIC len 0xa8
>>>> build_header: acpi sig GTDT len 0x60
>>>> build_header: acpi sig MCFG len 0x3c
>>>> build_header: acpi sig SPCR len 0x50
>>>> build_header: acpi sig SRAT len 0x92
>>>> build_header: acpi sig SSDT len 0x38f
>>>> build_header: acpi sig XSDT len 0x5c
>>>> virt_acpi_build: acpi table_blob len 0x5844
>>>>
>>>> Debug logs from UEFI:
>>>>
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
>>>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
>>>> InstallQemuFwCfgTables: installed 8 tables
>>>>
>>>> Guest Reboot after ndimm hot added
>>>> ------------------------------------
>>>>
>>>> Debug logs from Qemu:
>>>>
>>>> build_header: acpi sig DSDT len 0x5127
>>>> build_header: acpi sig FACP len 0x10c
>>>> build_header: acpi sig APIC len 0xa8
>>>> build_header: acpi sig GTDT len 0x60
>>>> build_header: acpi sig MCFG len 0x3c
>>>> build_header: acpi sig SPCR len 0x50
>>>> build_header: acpi sig SRAT len 0x92
>>>> build_header: acpi sig SSDT len 0x38f
>>>> build_header: acpi sig NFIT len 0xe0 -->New
>>>> build_header: acpi sig XSDT len 0x64
>>>> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
>>>>
>>>> Debug logs from UEFI:
>>>>
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
>>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
>>>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
>>>>
>>>>
>>>> To me it seems on ARM vit acpi path, the blob len is calculated based
>>>> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
>>>> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
>>>> with an additional buffer to take care of table resizing[1].
>>>>
>>>> As a hack i added the same to ARM virt and it seems to resolve the issue.
>>>> I am not sure this is the best approach to fix this though.
>>>>
>>>> Please let me know your thoughts.
>>>>
>>>> Thanks,
>>>> Shameer
>>>>
>>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>>> index 132414c..4291553 100644
>>>> --- a/hw/arm/virt-acpi-build.c
>>>> +++ b/hw/arm/virt-acpi-build.c
>>>> @@ -50,6 +50,8 @@
>>>> #define ARM_SPI_BASE 32
>>>> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>>>
>>>> +#define ACPI_BUILD_TABLE_SIZE 0x20000
>>>> +
>>>> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>>>> {
>>>> uint16_t i;
>>>> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
>>>> build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
>>>> }
>>>>
>>>> + /* Make sure we have a buffer in case we need to resize the tables. */
>>>> + g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
>>>> + ACPI_BUILD_TABLE_SIZE));
>>>> +
>>>> /* Cleanup memory that's no longer used. */
>>>> g_array_free(table_offsets, true);
>>>> }
>>>>
>>>> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
>>>
>>> Nice analysis, thanks.
>>>
>>> I think the line that you reference, i.e.
>>>
>>> acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>>>
>>> in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
>>> a side effect. To my understanding, the alignment / padding exists there
>>> for migration compatibility. It doesn't exist for updating the size of
>>> the ACPI blobs in fw_cfg across reboots. The issue is masked because the
>>> alignment is large enough (un-changed) to contain the regenerated blobs
>>> as well.>
>>> Given that the "virt" machine type is versioned, I think migration
>>> compat is a valid concern there too. This in itself would justify a
>>> similar padding.
>> I don't understand the migration compat issue. Please could you elaborate?
>
> git-blame explains it to some extent -- please see commit 07fb61760cde
> ("pc: hack for migration compatibility from QEMU 2.0", 2014-07-28).
>
> I don't remember any details at this point that the commit does not
> state. (I see that I reviewed the patch back then, so perhaps the
> mailing list archive has some discussion.)
>
> Interestingly, the commit message refers to "memory hotplug work" too.
>
> ... Ahh, wait, I do remember the main issue now. Here's the thing. The
> ACPI payload that QEMU generates for the firmware is considered a part
> of the firmware itself. Therefore, it is not versioned -- because the
> firmware itself is not versioned. (In other words, if you migrate a VM
> from one host to another host, and that other host has different
> firmware that the VM will pick up after re-launch (from cold boot), then
> the firmware will change in the VM.)
>
> By considering ACPI a part of the firmware, QEMU never versioned the
> ACPI payload, just like the actual firmware was never versioned. In
> other words, if you have machine type Foo on qemu release Bar, and
> machine type Foo on qemu release Baz, compat properties and such will
> ensure that the virtual hardware looks the same to the guest, but QEMU
> will *not* ensure that the ACPI payload generated at QEMU startup (more
> precisely, at "machine done") will be identical. Despite the fact that
> both QEMU instances use machine type Foo.
>
> Now, combine this with the feature that fw_cfg has been backed by RAM
> Blocks, for a quite long time now (this wasn't always the case, but it
> has been for multiple years now). The end result is that the RAM
> block(s) holding the initial ACPI payload may differ between releases
> Bar and Baz, within the same machine type Foo. This means that migration
> between them will fail, due to RAMBlock size difference.
>
> Hence the padding -- it tries to cancel out small variances in ACPI
> payload size.
>
>>>
>>> I don't know if we want to specifically care about size-changing
>>> ACPI-regen across reboot. I believe measures for that specific use case
>>> don't exist in x86 machine types either.
>> The NFIT redimensioning should exit on x86 too?
>
> That's not my point. My point was that the padding, which was originally
> supposed to mask variances in ACPI payload size across *QEMU releases*,
> for migration compat, ended up masking a variance of different origin:
> namely ACPI regeneration at reboot (with different contents). In other
> words, we never implemented any specific measures for this
> resize-on-reboot issue, instead we allowed the migration compat code
> (the padding) to take care of it as well.
>
> In virt, there is no such ACPI padding code (for migration compat) --
> for whatever reason --, and so it *also* cannot take care of the
> resize-on-reboot problem.
That's clearer now. Thank you for the explanations.
Thanks
Eric
>
> [...]
>
> Thanks
> Laszlo
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-28 12:27 ` Laszlo Ersek
2019-02-28 13:32 ` Auger Eric
@ 2019-02-28 13:43 ` Igor Mammedov
1 sibling, 0 replies; 36+ messages in thread
From: Igor Mammedov @ 2019-02-28 13:43 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Auger Eric, Shameerali Kolothum Thodi, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
On Thu, 28 Feb 2019 13:27:54 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/28/19 11:12, Auger Eric wrote:
> > Hi Laszlo,
> >
> > On 2/27/19 9:14 PM, Laszlo Ersek wrote:
> >> On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
> >>> Hi Laszlo,
> >>>
> >>>> -----Original Message-----
> >>>> From: Shameerali Kolothum Thodi
> >>>> Sent: 25 February 2019 09:54
> >>>> To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric <eric.auger@redhat.com>;
> >>>> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> >>>> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> >>>> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
> >>>> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> >>>> <leif.lindholm@linaro.org>
> >>>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> >>>
> >>> [...]
> >>>
> >>>>>>> The root cause seems to be EDK2 ACPI table checksum failure
> >>>>>>> as NFIT table is getting updated on hot-add. This needs further
> >>>>>>> investigation.
> >>>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> >>>>>
> >>>>> Huh, very interesting; I usually don't expect my sanity checks to fire
> >>>>> in practice. :)
> >>>>>
> >>>>> The message
> >>>>>
> >>>>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >>>>>
> >>>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when it
> >>>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's ACPI
> >>>>> linker/loader script.
> >>>>>
> >>>>> Please see the command definition in QEMU's
> >>>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> >>>>> function bios_linker_loader_add_checksum(), which builds the command
> >>>>> structure, and documents the fields.
> >>>>>
> >>>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> >>>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for the
> >>>>> same information.)
> >>>>>
> >>>>> The error message is logged if:
> >>>>> - the offset at which the checksum should be stored falls outside of the
> >>>>> size of the fw_cfg blob, or
> >>>>> - the range over which the checksum should be calculated falls outside
> >>>>> (at least in part) of the fw_cfg blob.
> >>>>>
> >>>>> To me this suggests that QEMU generates an invalid
> >>>>> COMMAND_ADD_CHECKSUM
> >>>>> command for the firmware.
> >>>>>
> >>>>> ... I've tried to skim the patches briefly. I think there must be an
> >>>>> error in the DSDT building logic that is only active on reboot if an
> >>>>> nvdimm module was hot-added before the reboot.
> >>>>
> >>>> Thanks for taking a look and the pointers. I will debug this further
> >>>> and get back.
> >>>
> >>> The root cause of the issue seems to be UEFI not seeing the updated acpi
> >>> table blob size on reboot once a new NFIT table is added(nvdimm hot added).
> >>>
> >>> Please see the debug logs below,
> >>>
> >>> Initial Guest boot
> >>> ---------------------------
> >>>
> >>> Debug logs from Qemu:
> >>>
> >>> build_header: acpi sig DSDT len 0x5127
> >>> build_header: acpi sig FACP len 0x10c
> >>> build_header: acpi sig APIC len 0xa8
> >>> build_header: acpi sig GTDT len 0x60
> >>> build_header: acpi sig MCFG len 0x3c
> >>> build_header: acpi sig SPCR len 0x50
> >>> build_header: acpi sig SRAT len 0x92
> >>> build_header: acpi sig SSDT len 0x38f
> >>> build_header: acpi sig XSDT len 0x5c
> >>> virt_acpi_build: acpi table_blob len 0x5844
> >>>
> >>> Debug logs from UEFI:
> >>>
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0x5C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8 Start=0x0 Length=0x14 Blob->Size=0x24
> >>> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20 Start=0x0 Length=0x24 Blob->Size=0x24
> >>> InstallQemuFwCfgTables: installed 8 tables
> >>>
> >>> Guest Reboot after ndimm hot added
> >>> ------------------------------------
> >>>
> >>> Debug logs from Qemu:
> >>>
> >>> build_header: acpi sig DSDT len 0x5127
> >>> build_header: acpi sig FACP len 0x10c
> >>> build_header: acpi sig APIC len 0xa8
> >>> build_header: acpi sig GTDT len 0x60
> >>> build_header: acpi sig MCFG len 0x3c
> >>> build_header: acpi sig SPCR len 0x50
> >>> build_header: acpi sig SRAT len 0x92
> >>> build_header: acpi sig SSDT len 0x38f
> >>> build_header: acpi sig NFIT len 0xe0 -->New
> >>> build_header: acpi sig XSDT len 0x64
> >>> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
> >>>
> >>> Debug logs from UEFI:
> >>>
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9 Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130 Start=0x5127 Length=0x10C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C Start=0x5233 Length=0xA8 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4 Start=0x52DB Length=0x60 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344 Start=0x533B Length=0x3C Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380 Start=0x5377 Length=0x50 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0 Start=0x53C7 Length=0x92 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462 Start=0x5459 Length=0x38F Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1 Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> >>> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> >>> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> >>>
> >>>
> >>> To me it seems on ARM vit acpi path, the blob len is calculated based
> >>> on actual tables and is updated only in virt_acpi_setup() --> acpi_add_rom_blob()
> >>> path. I had a look at the x86 code and it looks like, there, the blob len gets updated
> >>> with an additional buffer to take care of table resizing[1].
> >>>
> >>> As a hack i added the same to ARM virt and it seems to resolve the issue.
> >>> I am not sure this is the best approach to fix this though.
> >>>
> >>> Please let me know your thoughts.
> >>>
> >>> Thanks,
> >>> Shameer
> >>>
> >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >>> index 132414c..4291553 100644
> >>> --- a/hw/arm/virt-acpi-build.c
> >>> +++ b/hw/arm/virt-acpi-build.c
> >>> @@ -50,6 +50,8 @@
> >>> #define ARM_SPI_BASE 32
> >>> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >>>
> >>> +#define ACPI_BUILD_TABLE_SIZE 0x20000
> >>> +
> >>> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >>> {
> >>> uint16_t i;
> >>> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms, AcpiBuildTables *tables)
> >>> build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> >>> }
> >>>
> >>> + /* Make sure we have a buffer in case we need to resize the tables. */
> >>> + g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
> >>> + ACPI_BUILD_TABLE_SIZE));
> >>> +
> >>> /* Cleanup memory that's no longer used. */
> >>> g_array_free(table_offsets, true);
> >>> }
> >>>
> >>> [1] https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
> >>
> >> Nice analysis, thanks.
> >>
> >> I think the line that you reference, i.e.
> >>
> >> acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> >>
> >> in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
> >> a side effect. To my understanding, the alignment / padding exists there
> >> for migration compatibility. It doesn't exist for updating the size of
> >> the ACPI blobs in fw_cfg across reboots. The issue is masked because the
> >> alignment is large enough (un-changed) to contain the regenerated blobs
> >> as well.>
> >> Given that the "virt" machine type is versioned, I think migration
> >> compat is a valid concern there too. This in itself would justify a
> >> similar padding.
> > I don't understand the migration compat issue. Please could you elaborate?
>
> git-blame explains it to some extent -- please see commit 07fb61760cde
> ("pc: hack for migration compatibility from QEMU 2.0", 2014-07-28).
>
> I don't remember any details at this point that the commit does not
> state. (I see that I reviewed the patch back then, so perhaps the
> mailing list archive has some discussion.)
>
> Interestingly, the commit message refers to "memory hotplug work" too.
>
> ... Ahh, wait, I do remember the main issue now. Here's the thing. The
> ACPI payload that QEMU generates for the firmware is considered a part
> of the firmware itself. Therefore, it is not versioned -- because the
> firmware itself is not versioned. (In other words, if you migrate a VM
> from one host to another host, and that other host has different
> firmware that the VM will pick up after re-launch (from cold boot), then
> the firmware will change in the VM.)
>
> By considering ACPI a part of the firmware, QEMU never versioned the
> ACPI payload, just like the actual firmware was never versioned. In
> other words, if you have machine type Foo on qemu release Bar, and
> machine type Foo on qemu release Baz, compat properties and such will
> ensure that the virtual hardware looks the same to the guest, but QEMU
> will *not* ensure that the ACPI payload generated at QEMU startup (more
> precisely, at "machine done") will be identical. Despite the fact that
> both QEMU instances use machine type Foo.
>
> Now, combine this with the feature that fw_cfg has been backed by RAM
> Blocks, for a quite long time now (this wasn't always the case, but it
> has been for multiple years now). The end result is that the RAM
> block(s) holding the initial ACPI payload may differ between releases
> Bar and Baz, within the same machine type Foo. This means that migration
> between them will fail, due to RAMBlock size difference.
>
> Hence the padding -- it tries to cancel out small variances in ACPI
> payload size.
>
> >>
> >> I don't know if we want to specifically care about size-changing
> >> ACPI-regen across reboot. I believe measures for that specific use case
> >> don't exist in x86 machine types either.
> > The NFIT redimensioning should exit on x86 too?
>
> That's not my point. My point was that the padding, which was originally
> supposed to mask variances in ACPI payload size across *QEMU releases*,
> for migration compat, ended up masking a variance of different origin:
> namely ACPI regeneration at reboot (with different contents). In other
> words, we never implemented any specific measures for this
> resize-on-reboot issue, instead we allowed the migration compat code
> (the padding) to take care of it as well.
>
> In virt, there is no such ACPI padding code (for migration compat) --
> for whatever reason --, and so it *also* cannot take care of the
> resize-on-reboot problem.
What's describe above is a bit outdate, let me paint a rough current state
in chronological order (I haven't checked how it actually works now).
--
1. how it work(s|ed):
* tables are generated during machine_done and registered with fw_cfg
* then oops: we have device_add that could be called later but before
guest runs and we don't have complete picture wrt CRS resources for
PCI as that is programmed by firmware, so here goes a trick to
regenerate tables when firmware accesses its fwcfg entry
(see how build_state->patched is used), byt that time FW has intialized
PCI to the extent necessary to build valid tables.
* then we do hot add ACPI table are not updated until reset
(acpi_build_reset) and all dance with patching repeats when FW
gets tables form QEMU.
* at those versions of QEMU tables size was static
2. Later on hotplugging bridges (I think) caused issues on reboot/migration
as it caused DSDT size increase, that made as invent padding race train-wreak.
2.2 then there were cases where tables changed size depending where a device
was cold or hot plugged and may be some other issues I don't recall.
3. With padding race being a lost game (there always were border line configs
where it broke migration), Michael introduced re-sizable blobs 42d859001d for
acpi table blobs, which could be resized on target to accommodate larger
tables blob during migration.
4. Even with resizable memory regions for the sake of backward migration
compatibility we needed to keep padding we used with legacy machine. It's
still broken in some cases but mostly works (we declared padding approach
broken by design and left it as is)
--
Thankfully arm/virt ACPI support materialized after all of the above
so we don't have legacy padding obligations at all. Hence suggest to
find were the bug is instead of trying to start yet another padding race.
> [...]
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-28 10:12 ` Auger Eric
2019-02-28 12:04 ` Shameerali Kolothum Thodi
2019-02-28 12:27 ` Laszlo Ersek
@ 2019-02-28 14:02 ` Shameerali Kolothum Thodi
2019-03-01 13:49 ` Laszlo Ersek
2 siblings, 1 reply; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-02-28 14:02 UTC (permalink / raw)
To: Auger Eric, Laszlo Ersek, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 28 February 2019 12:04
> To: 'Auger Eric' <eric.auger@redhat.com>; Laszlo Ersek <lersek@redhat.com>;
> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> <leif.lindholm@linaro.org>
> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>
>
>
> > -----Original Message-----
> > From: Auger Eric [mailto:eric.auger@redhat.com]
> > Sent: 28 February 2019 10:12
> > To: Laszlo Ersek <lersek@redhat.com>; Shameerali Kolothum Thodi
> > <shameerali.kolothum.thodi@huawei.com>; shannon.zhaosl@gmail.com;
> > peter.maydell@linaro.org; imammedo@redhat.com;
> qemu-devel@nongnu.org;
> > qemu-arm@nongnu.org
> > Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>;
> Ard
> > Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> > <leif.lindholm@linaro.org>
> > Subject: Re: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> >
> > Hi Laszlo,
> >
> > On 2/27/19 9:14 PM, Laszlo Ersek wrote:
> > > On 02/27/19 13:55, Shameerali Kolothum Thodi wrote:
> > >> Hi Laszlo,
> > >>
> > >>> -----Original Message-----
> > >>> From: Shameerali Kolothum Thodi
> > >>> Sent: 25 February 2019 09:54
> > >>> To: 'Laszlo Ersek' <lersek@redhat.com>; Auger Eric
> > <eric.auger@redhat.com>;
> > >>> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> > >>> imammedo@redhat.com; qemu-devel@nongnu.org;
> > qemu-arm@nongnu.org
> > >>> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm
> <linuxarm@huawei.com>;
> > Ard
> > >>> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> > >>> <leif.lindholm@linaro.org>
> > >>> Subject: RE: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
> > >>
> > >> [...]
> > >>
> > >>>>>> The root cause seems to be EDK2 ACPI table checksum failure
> > >>>>>> as NFIT table is getting updated on hot-add. This needs further
> > >>>>>> investigation.
> > >>>>> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
> > >>>>
> > >>>> Huh, very interesting; I usually don't expect my sanity checks to fire
> > >>>> in practice. :)
> > >>>>
> > >>>> The message
> > >>>>
> > >>>> ProcessCmdAddChecksum: invalid checksum range in
> > "etc/acpi/tables"
> > >>>>
> > >>>> is logged by OVMF's and ArmVirtQemu's ACPI Platform DXE Driver when
> > it
> > >>>> finds an invalid COMMAND_ADD_CHECKSUM command in QEMU's
> ACPI
> > >>>> linker/loader script.
> > >>>>
> > >>>> Please see the command definition in QEMU's
> > >>>> "hw/acpi/bios-linker-loader.c". In particular, please refer to the
> > >>>> function bios_linker_loader_add_checksum(), which builds the
> command
> > >>>> structure, and documents the fields.
> > >>>>
> > >>>> (You may also refer to QEMU_LOADER_ADD_CHECKSUM in file
> > >>>> "OvmfPkg/AcpiPlatformDxe/QemuLoader.h" in the edk2 source tree, for
> > the
> > >>>> same information.)
> > >>>>
> > >>>> The error message is logged if:
> > >>>> - the offset at which the checksum should be stored falls outside of the
> > >>>> size of the fw_cfg blob, or
> > >>>> - the range over which the checksum should be calculated falls outside
> > >>>> (at least in part) of the fw_cfg blob.
> > >>>>
> > >>>> To me this suggests that QEMU generates an invalid
> > >>>> COMMAND_ADD_CHECKSUM
> > >>>> command for the firmware.
> > >>>>
> > >>>> ... I've tried to skim the patches briefly. I think there must be an
> > >>>> error in the DSDT building logic that is only active on reboot if an
> > >>>> nvdimm module was hot-added before the reboot.
> > >>>
> > >>> Thanks for taking a look and the pointers. I will debug this further
> > >>> and get back.
> > >>
> > >> The root cause of the issue seems to be UEFI not seeing the updated acpi
> > >> table blob size on reboot once a new NFIT table is added(nvdimm hot
> > added).
> > >>
> > >> Please see the debug logs below,
> > >>
> > >> Initial Guest boot
> > >> ---------------------------
> > >>
> > >> Debug logs from Qemu:
> > >>
> > >> build_header: acpi sig DSDT len 0x5127
> > >> build_header: acpi sig FACP len 0x10c
> > >> build_header: acpi sig APIC len 0xa8
> > >> build_header: acpi sig GTDT len 0x60
> > >> build_header: acpi sig MCFG len 0x3c
> > >> build_header: acpi sig SPCR len 0x50
> > >> build_header: acpi sig SRAT len 0x92
> > >> build_header: acpi sig SSDT len 0x38f
> > >> build_header: acpi sig XSDT len 0x5c
> > >> virt_acpi_build: acpi table_blob len 0x5844
> > >>
> > >> Debug logs from UEFI:
> > >>
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9
> > Start=0x0 Length=0x5127 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130
> > Start=0x5127 Length=0x10C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C
> > Start=0x5233 Length=0xA8 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4
> > Start=0x52DB Length=0x60 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344
> > Start=0x533B Length=0x3C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380
> > Start=0x5377 Length=0x50 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0
> > Start=0x53C7 Length=0x92 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462
> > Start=0x5459 Length=0x38F Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1
> > Start=0x57E8 Length=0x5C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x8
> Start=0x0
> > Length=0x14 Blob->Size=0x24
> > >> ProcessCmdAddChecksum: File="etc/acpi/rsdp" ResultOffset=0x20
> > Start=0x0 Length=0x24 Blob->Size=0x24
> > >> InstallQemuFwCfgTables: installed 8 tables
> > >>
> > >> Guest Reboot after ndimm hot added
> > >> ------------------------------------
> > >>
> > >> Debug logs from Qemu:
> > >>
> > >> build_header: acpi sig DSDT len 0x5127
> > >> build_header: acpi sig FACP len 0x10c
> > >> build_header: acpi sig APIC len 0xa8
> > >> build_header: acpi sig GTDT len 0x60
> > >> build_header: acpi sig MCFG len 0x3c
> > >> build_header: acpi sig SPCR len 0x50
> > >> build_header: acpi sig SRAT len 0x92
> > >> build_header: acpi sig SSDT len 0x38f
> > >> build_header: acpi sig NFIT len 0xe0 -->New
> > >> build_header: acpi sig XSDT len 0x64
> > >> virt_acpi_build: acpi table_blob len 0x592c -->blob len updated
> > >>
> > >> Debug logs from UEFI:
> > >>
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x9
> > Start=0x0 Length=0x5127 Blob->Size=0x5844 -->Wrong blob size.
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5130
> > Start=0x5127 Length=0x10C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x523C
> > Start=0x5233 Length=0xA8 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x52E4
> > Start=0x52DB Length=0x60 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5344
> > Start=0x533B Length=0x3C Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5380
> > Start=0x5377 Length=0x50 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x53D0
> > Start=0x53C7 Length=0x92 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x5462
> > Start=0x5459 Length=0x38F Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x57F1
> > Start=0x57E8 Length=0xE0 Blob->Size=0x5844
> > >> ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> > >> OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> > >>
> > >>
> > >> To me it seems on ARM vit acpi path, the blob len is calculated based
> > >> on actual tables and is updated only in virt_acpi_setup() -->
> > acpi_add_rom_blob()
> > >> path. I had a look at the x86 code and it looks like, there, the blob len gets
> > updated
> > >> with an additional buffer to take care of table resizing[1].
> > >>
> > >> As a hack i added the same to ARM virt and it seems to resolve the issue.
> > >> I am not sure this is the best approach to fix this though.
> > >>
> > >> Please let me know your thoughts.
> > >>
> > >> Thanks,
> > >> Shameer
> > >>
> > >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > >> index 132414c..4291553 100644
> > >> --- a/hw/arm/virt-acpi-build.c
> > >> +++ b/hw/arm/virt-acpi-build.c
> > >> @@ -50,6 +50,8 @@
> > >> #define ARM_SPI_BASE 32
> > >> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> > >>
> > >> +#define ACPI_BUILD_TABLE_SIZE 0x20000
> > >> +
> > >> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> > >> {
> > >> uint16_t i;
> > >> @@ -886,6 +888,10 @@ void virt_acpi_build(VirtMachineState *vms,
> > AcpiBuildTables *tables)
> > >> build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> > >> }
> > >>
> > >> + /* Make sure we have a buffer in case we need to resize the tables.
> */
> > >> + g_array_set_size(tables_blob,
> > ROUND_UP(acpi_data_len(tables_blob),
> > >> + ACPI_BUILD_TABLE_SIZE));
> > >> +
> > >> /* Cleanup memory that's no longer used. */
> > >> g_array_free(table_offsets, true);
> > >> }
> > >>
> > >> [1]
> > https://github.com/qemu/qemu/blob/master/hw/i386/acpi-build.c#L2792
> > >
> > > Nice analysis, thanks.
> > >
> > > I think the line that you reference, i.e.
> > >
> > > acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
>
> Yes.
>
> > > in acpi_build() [hw/i386/acpi-build.c] masks this issue for x86 only as
> > > a side effect. To my understanding, the alignment / padding exists there
> > > for migration compatibility. It doesn't exist for updating the size of
> > > the ACPI blobs in fw_cfg across reboots. The issue is masked because the
> > > alignment is large enough (un-changed) to contain the regenerated blobs
> > > as well.>
> > > Given that the "virt" machine type is versioned, I think migration
> > > compat is a valid concern there too. This in itself would justify a
> > > similar padding.
> > I don't understand the migration compat issue. Please could you elaborate?
>
> Yes. Please elaborate.
>
> > > I don't know if we want to specifically care about size-changing
> > > ACPI-regen across reboot. I believe measures for that specific use case
> > > don't exist in x86 machine types either.
> > The NFIT redimensioning should exit on x86 too?
>
> I had a go with x86 commenting out the padding completely. But interestingly
> somewhere in x86 code path the blob length gets aligned to 0x1000 boundary
> and it escapes the sanity check.
>
> With padding, UEFI always sees, ACPI_BUILD_TABLE_SIZE (0x20000)
>
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49
> Start=0x40 Length=0x19E9 Blob->Size=0x20000
>
> With padding removed,
>
> From Qemu,
> acpi_build: tables_blob len 0x2c62
>
> UEFI:
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49
> Start=0x40 Length=0x1F33 Blob->Size=0x3000
>
> Guest Reboot after adding a number of nvdimms,
>
> Qemu:
> acpi_build: tables_blob len 0x380e
>
> UEFI:
> ProcessCmdAddChecksum: File="etc/acpi/tables" ResultOffset=0x49
> Start=0x40 Length=0x2401 Blob->Size=0x4000
>
> I couldn’t figure out from the code where that round up is happening. But
> x86 also has same issue if the padding is removed, I guess.
Ah..I missed the fact that, firmware indeed sees an update in the blob len here
(rounded or not) after reboot. So don’t think x86 has the same issue and padding
is not the right solution as Igor explained in his reply.
I will try to debug this further. Any pointers welcome.
Cheers,
Shameer
> Thanks,
> Shameer
>
> > > Another trick that is occasionally used (but might not apply here, I'm
> > > uncertain) is to always generate the relevant ACPI objects, but, in case
> > > they are not justified for the virtual hardware config, invalidate them
> > > by overwriting particular parts of them (for example, one or two bytes
> > > of their names). Hopefully this shouldn't introduce ACPI or AML errors,
> > > just make the ACPI interpreter ignore the affected objects.
> >
> > Thanks!
> >
> > Eric
> > >
> > > Thanks,
> > > Laszlo
> > >
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-28 14:02 ` Shameerali Kolothum Thodi
@ 2019-03-01 13:49 ` Laszlo Ersek
2019-03-01 17:39 ` Igor Mammedov
0 siblings, 1 reply; 36+ messages in thread
From: Laszlo Ersek @ 2019-03-01 13:49 UTC (permalink / raw)
To: Shameerali Kolothum Thodi, Auger Eric, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, imammedo@redhat.com,
qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
On 02/28/19 15:02, Shameerali Kolothum Thodi wrote:
> Ah..I missed the fact that, firmware indeed sees an update in the blob len here
> (rounded or not) after reboot. So don’t think x86 has the same issue and padding
> is not the right solution as Igor explained in his reply.
>
> I will try to debug this further. Any pointers welcome.
How about this.
(1) The firmware looks up the fw_cfg file called "etc/table-loader" in
the fw_cfg file directory (identified by constant selector key 0x0019,
FW_CFG_FILE_DIR).
(2) The directory entry, once found, tells the firmware two things
simultaneously. The selector key, and the size of the blob.
(3) The firmware selects the selector key from step (2).
(4) QEMU regenerates the ACPI payload (as a select callback).
(5) The firmware reads the number of bytes from the fw_cfg blob that it
learned in step (2).
Here's the problem. As long as QEMU used to perform step (4) only for
the purpose of refreshing PCI resources in the ACPI payload, step (4)
wouldn't *resize* the blob.
However, if step (4) enlarges the blob, then the byte count that step
(5) uses -- from step (2) -- for reading, is obsolete.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-03-01 13:49 ` Laszlo Ersek
@ 2019-03-01 17:39 ` Igor Mammedov
2019-03-05 12:14 ` Laszlo Ersek
0 siblings, 1 reply; 36+ messages in thread
From: Igor Mammedov @ 2019-03-01 17:39 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Shameerali Kolothum Thodi, Auger Eric, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
On Fri, 1 Mar 2019 14:49:45 +0100
Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/28/19 15:02, Shameerali Kolothum Thodi wrote:
>
> > Ah..I missed the fact that, firmware indeed sees an update in the blob len here
> > (rounded or not) after reboot. So don’t think x86 has the same issue and padding
> > is not the right solution as Igor explained in his reply.
> >
> > I will try to debug this further. Any pointers welcome.
>
> How about this.
>
> (1) The firmware looks up the fw_cfg file called "etc/table-loader" in
> the fw_cfg file directory (identified by constant selector key 0x0019,
> FW_CFG_FILE_DIR).
>
> (2) The directory entry, once found, tells the firmware two things
> simultaneously. The selector key, and the size of the blob.
>
> (3) The firmware selects the selector key from step (2).
>
> (4) QEMU regenerates the ACPI payload (as a select callback).
>
> (5) The firmware reads the number of bytes from the fw_cfg blob that it
> learned in step (2).
>
> Here's the problem. As long as QEMU used to perform step (4) only for
> the purpose of refreshing PCI resources in the ACPI payload, step (4)
> wouldn't *resize* the blob.
>
> However, if step (4) enlarges the blob, then the byte count that step
> (5) uses -- from step (2) -- for reading, is obsolete.
I've thought that was a problem with IO based fw_cfg, as reading size/content
were separates steps and that it was solved by DMA based fw_cfg file read.
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-03-01 17:39 ` Igor Mammedov
@ 2019-03-05 12:14 ` Laszlo Ersek
0 siblings, 0 replies; 36+ messages in thread
From: Laszlo Ersek @ 2019-03-05 12:14 UTC (permalink / raw)
To: Igor Mammedov
Cc: Shameerali Kolothum Thodi, Auger Eric, shannon.zhaosl@gmail.com,
peter.maydell@linaro.org, qemu-devel@nongnu.org,
qemu-arm@nongnu.org, xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address)
On 03/01/19 18:39, Igor Mammedov wrote:
> On Fri, 1 Mar 2019 14:49:45 +0100
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> On 02/28/19 15:02, Shameerali Kolothum Thodi wrote:
>>
>>> Ah..I missed the fact that, firmware indeed sees an update in the blob len here
>>> (rounded or not) after reboot. So don’t think x86 has the same issue and padding
>>> is not the right solution as Igor explained in his reply.
>>>
>>> I will try to debug this further. Any pointers welcome.
>>
>> How about this.
>>
>> (1) The firmware looks up the fw_cfg file called "etc/table-loader" in
>> the fw_cfg file directory (identified by constant selector key 0x0019,
>> FW_CFG_FILE_DIR).
>>
>> (2) The directory entry, once found, tells the firmware two things
>> simultaneously. The selector key, and the size of the blob.
>>
>> (3) The firmware selects the selector key from step (2).
>>
>> (4) QEMU regenerates the ACPI payload (as a select callback).
>>
>> (5) The firmware reads the number of bytes from the fw_cfg blob that it
>> learned in step (2).
>>
>> Here's the problem. As long as QEMU used to perform step (4) only for
>> the purpose of refreshing PCI resources in the ACPI payload, step (4)
>> wouldn't *resize* the blob.
>>
>> However, if step (4) enlarges the blob, then the byte count that step
>> (5) uses -- from step (2) -- for reading, is obsolete.
> I've thought that was a problem with IO based fw_cfg, as reading size/content
> were separates steps and that it was solved by DMA based fw_cfg file read.
The DMA backend is not relevant for this question, for two reasons:
(a) The question whether the fw_cfg transfer takes places with port IO
vs. DMA is hidden from the fw_cfg client code; that code goes through an
abstract library API.
(b) While the DMA method indeed lets the firmware specify the details of
the transfer with one action, the issue is with the number of bytes that
the firmware requests (that is, not with *how* the firmware requests the
transfer). The firmware has to know the size of the transfer before it
can initiate the transfer (regardless of port IO vs. DMA).
My question is: assume the firmware item in question is selected, and
the QEMU-side select callback runs (regenerating the ACPI payload). Does
this action update the blob size in the fw_cfg file directory as well?
If it does, then I can work around the problem in the firmware. I can
add a re-lookup to the code after the item selection, in order to get
the fresh blob size from the fw_cfg file directory. Then we can use that
size for the actual transfer.
This won't help old firmware on new QEMU, but at least new firmware on
old QEMU will not be hurt (the re-fetching of the fw_cfg file directory
will come with a small performance penalty, but functionally it will be
a no-op).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
2019-02-22 16:03 ` [Qemu-devel] [RFC PATCH 0/4] ARM virt: ACPI memory " Auger Eric
2019-02-22 19:11 ` Laszlo Ersek
@ 2019-02-25 9:47 ` Shameerali Kolothum Thodi
1 sibling, 0 replies; 36+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-02-25 9:47 UTC (permalink / raw)
To: Auger Eric, shannon.zhaosl@gmail.com, peter.maydell@linaro.org,
imammedo@redhat.com, qemu-devel@nongnu.org, qemu-arm@nongnu.org
Cc: xuwei (O), Linuxarm, Ard Biesheuvel,
Leif Lindholm (Linaro address), Laszlo Ersek
Hi Eric,
> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 22 February 2019 16:03
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> shannon.zhaosl@gmail.com; peter.maydell@linaro.org;
> imammedo@redhat.com; qemu-devel@nongnu.org; qemu-arm@nongnu.org
> Cc: xuwei (O) <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>; Leif Lindholm (Linaro address)
> <leif.lindholm@linaro.org>; Laszlo Ersek <lersek@redhat.com>
> Subject: Re: [RFC PATCH 0/4] ARM virt: ACPI memory hotplug support
>
> Hi Shameer,
>
> On 1/28/19 12:05 PM, Shameer Kolothum wrote:
> > This series is an attempt to provide hotplug support to both
> > pc-dimm and nvdimm device memory on ARM virt platform. This is
> > based on Eric's recent works to support PCDIMM/NVDIMM device
> memory[1].
> > The kernel support for arm64 memory hot add was added only
> > recently by Robin[2] and hence the guest kernel should be => 5.0-rc1.
> >
> > This makes use of PL061 GPIO controller to sent related ACPI events
> s/sent/send
> > to the Guest. The only reference I could find with respect to the GPIO
> > pins usage is here[3] which says, "use PIN 3 for system_powerdown,
> > reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and memory hotplug".
> > Hence Pin 2 is used for PCDIMM and pin 4 for NVDIMM.
> >
> > This is sanity tested on a HiSilicon ARM64 platform and appreciate
> > any further testing.
>
> I did some testing on another platform and I got the exactly the same
> results as yours: PCDIMM hot plug works fine. Also after system_reset I
> still can see the slots.
> Hot-unplug is not supported though.
Thanks for giving it a spin. Hot unplug is disabled for now as kernel doesn’t
have support for it yet.
> For NVDIMM, hot-add works fine and and I can see the slots using ndctl
> on guest. But after system_reset, the guest does not boot properly.
Right. And I assume you are seeing the same error message as below.
Thanks for adding relevant people to the discussion.
Cheers,
Shameer
> >
> > This series can be applied on top of Eric's branch here[4]
> >
> > Test:
> > ------
> > Please use a Guest kernel image >5.0-rc1 with all the mem/nvdimm
> > hotplug related CONFIGs enabled.
> >
> > ./qemu-system-aarch64 \
> > -machine virt,gic-version=3,nvdimm \
> > -m 1G,maxmem=4G,slots=4 \
> > -cpu host \
> > -kernel Image \
> > -initrd rootfs-iperf.cpio \
> > -bios QEMU_EFI.fd \
> > -numa node,nodeid=0 \
> > -net none \
> > -nographic -enable-kvm \
> > -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
> >
> > Enter Qemu monitor,
> > Add pc-dimm:
> > object_add memory-backend-ram,id=mem1,size=1G
> > device_add pc-dimm,id=dimm1,memdev=mem1
> >
> > Add nvdimm:
> > object_add memory-backend-ram,id=mem2,size=1G
> > device_add nvdimm,id=dimm2,memdev=mem2
> >
> > Known Issue:
> >
> > It is observed that hot adding nvdimm will results in guest reboot
> > failure. EDK2 fails to build the ACPI tables on reboot. Please find
> > below EDK2 log on Guest reboot after nvdimm hot-add,
> >
> > ProcessCmdAddChecksum: invalid checksum range in "etc/acpi/tables"
> > OnRootBridgesConnected: InstallAcpiTables: Protocol Error
> >
> > The root cause seems to be EDK2 ACPI table checksum failure
> > as NFIT table is getting updated on hot-add. This needs further
> > investigation.
> + Ard, Leif, Laszlo if they have any idea of what is missing/wrong.
>
> Thanks
>
> Eric
> >
> > Thanks,
> > Shameer
> >
> > [1]https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg05740.html
> > [2]https://patchwork.kernel.org/patch/10724455/
> > [3]https://lists.gnu.org/archive/html/qemu-arm/2015-12/msg00095.html
> > [4]https://github.com/eauger/qemu/tree/v3.1.0-dimm-v5
> >
> > Shameer Kolothum (4):
> > hw:acpi: Make ACPI IO address space configurable
> > hw/arm/virt: Add GPIO based pcdimm hotplug ACPI event support
> > hw/arm/virt: Enable pc-dimm hotplug support
> > hw/arm/virt: Add nvdimm hotplug support
> >
> > default-configs/arm-softmmu.mak | 1 +
> > hw/acpi/memory_hotplug.c | 13 +++--
> > hw/arm/virt-acpi-build.c | 45 +++++++++++++++--
> > hw/arm/virt.c | 105
> ++++++++++++++++++++++++++++++++++++---
> > hw/i386/acpi-build.c | 3 +-
> > include/hw/acpi/memory_hotplug.h | 6 ++-
> > include/hw/arm/virt.h | 15 ++++++
> > 7 files changed, 168 insertions(+), 20 deletions(-)
> >
^ permalink raw reply [flat|nested] 36+ messages in thread