* [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support
@ 2019-07-26 10:45 Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 1/9] hw/acpi: Make ACPI IO address space configurable Shameer Kolothum
` (8 more replies)
0 siblings, 9 replies; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
This series is an attempt to provide device memory hotplug support
on ARM virt platform. This is based on Eric's recent works here[1]
and carries some of the pc-dimm related patches dropped from his
series.
The kernel support for arm64 memory hot add was added recently by
Robin and hence the guest kernel should be => 5.0-rc1.
NVDIM support is not included currently as we still have an unresolved
issue while hot adding NVDIMM[2]. However NVDIMM cold plug patches
can be included, but not done for now, for keeping it simple.
This makes use of GED device to sent hotplug ACPI events to the
Guest. GED code is based on Nemu. Thanks to the efforts of Samuel and
Sebastien to add the hardware-reduced support to Nemu using GED
device[3]. (Please shout if I got the author/signed-off wrong for
those patches or missed any names).
This is sanity tested on a HiSilicon ARM64 platform and appreciate
any further testing.
Note:
Attempted adding dimm_pxm test case to bios-tables-test for arm/virt.
But noticed the issue decribed here[5]. This is under investigation
now.
Thanks,
Shameer
[1] https://patchwork.kernel.org/cover/10837565/
[2] https://patchwork.kernel.org/cover/10783589/
[3] https://github.com/intel/nemu/blob/topic/virt-x86/hw/acpi/ged.c
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2019-May/651763.html
[5] https://www.mail-archive.com/qemu-devel@nongnu.org/msg632651.html
v7 --> v8
-Addressed comments from Igor.Please see individual patches.
-Updated bios-tables-test-allowed-diff.h to avoid "make check"
failure (patch #6) and dropped patch #10
-Added Igor's R-by to patches 4 & 5.
-Dropped Erics's R-by from patch #9 for now.
v6 --> v7
- Added 4.2 machine support and restricted GED creation for < 4.2
This is to address the migration test fail reported by Eric.
- Included "tests: Update DSDT ACPI table.." patch(#10) from Eric
to fix the "make check" bios-tables-test failure.
v5 --> v6
-Addressed comments from Eric.
-Added R-by from Eric and Igor.
v4 --> v5
-Removed gsi/ged-irq routing in virt.
-Added Migration support.
-Dropped support for DT coldplug case based on the discussions
here[4]
-Added system_powerdown support through GED.
v3 --> v4
Addressed comments from Igor and Eric,
-Renamed "virt-acpi" to "acpi-ged".
-Changed ged device parent to TYPE_DEVICE.
-Introduced DT memory node property "hotpluggable" to resolve device
memory being treated as early boot memory issue(patch #7).
-Combined patches #3 and #9 from v3 into #3.
v2 --> v3
Addressed comments from Igor and Eric,
-Made virt acpi device platform independent and moved
to hw/acpi/generic_event_device.c
-Moved ged specific code into hw/acpi/generic_event_device.c
-Introduced an opt-in feature "fdt" to resolve device-memory being
treated as early boot memory.
-Dropped patch #1 from v2.
RFC --> v2
-Use GED device instead of GPIO for ACPI hotplug events.
-Removed NVDIMM support for now.
-Includes dropped patches from Eric's v9 series.
Eric Auger (1):
hw/arm/virt: Add memory hotplug framework
Samuel Ortiz (2):
hw/acpi: Do not create memory hotplug method when handler is not
defined
hw/acpi: Add ACPI Generic Event Device Support
Shameer Kolothum (6):
hw/acpi: Make ACPI IO address space configurable
hw/arm/virt: Add 4.2 machine type
hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
hw/acpi: Add system power down support to GED
hw/arm: Use GED for system_powerdown event
hw/acpi/Kconfig | 4 +
hw/acpi/Makefile.objs | 1 +
hw/acpi/generic_event_device.c | 330 +++++++++++++++++++++++++
hw/acpi/memory_hotplug.c | 35 +--
hw/arm/Kconfig | 4 +
hw/arm/virt-acpi-build.c | 29 ++-
hw/arm/virt.c | 125 +++++++++-
hw/core/machine.c | 3 +
hw/i386/acpi-build.c | 3 +-
include/hw/acpi/acpi_dev_interface.h | 1 +
include/hw/acpi/generic_event_device.h | 103 ++++++++
include/hw/acpi/memory_hotplug.h | 9 +-
include/hw/arm/virt.h | 5 +
include/hw/boards.h | 3 +
tests/bios-tables-test-allowed-diff.h | 1 +
15 files changed, 623 insertions(+), 33 deletions(-)
create mode 100644 hw/acpi/generic_event_device.c
create mode 100644 include/hw/acpi/generic_event_device.h
--
2.17.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 1/9] hw/acpi: Make ACPI IO address space configurable
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 2/9] hw/acpi: Do not create memory hotplug method when handler is not defined Shameer Kolothum
` (7 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
This is in preparation for adding support for ARM64 platforms
where it doesn't use port mapped IO for ACPI IO space.
Also move few MEMORY_* definitions to header so that other memory
hotplug event signalling mechanisms (eg. Generic Event Device on
HW-reduced acpi platforms) can use the same from their respective
event handler code.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/memory_hotplug.c | 25 ++++++++++++++-----------
hw/i386/acpi-build.c | 3 ++-
include/hw/acpi/memory_hotplug.h | 9 +++++++--
3 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 297812d5f7..c724f5f1e4 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -29,12 +29,9 @@
#define MEMORY_SLOT_PROXIMITY_METHOD "MPXM"
#define MEMORY_SLOT_EJECT_METHOD "MEJ0"
#define MEMORY_SLOT_NOTIFY_METHOD "MTFY"
-#define MEMORY_SLOT_SCAN_METHOD "MSCN"
#define MEMORY_HOTPLUG_DEVICE "MHPD"
-#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)
{
@@ -209,7 +206,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());
@@ -342,7 +339,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;
@@ -365,14 +363,19 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
aml_name_decl("_UID", aml_string("Memory hotplug resources")));
crs = aml_resource_template();
- aml_append(crs,
- aml_io(AML_DECODE16, memhp_io_base, memhp_io_base, 0,
- MEMORY_HOTPLUG_IO_LEN)
- );
+ if (rs == AML_SYSTEM_IO) {
+ aml_append(crs,
+ aml_io(AML_DECODE16, memhp_io_base, memhp_io_base, 0,
+ MEMORY_HOTPLUG_IO_LEN)
+ );
+ } else {
+ aml_append(crs, aml_memory32_fixed(memhp_io_base,
+ MEMORY_HOTPLUG_IO_LEN, AML_READ_WRITE));
+ }
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 d281ffa89e..4aae365347 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1865,7 +1865,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 77c65765d6..e3a4b89235 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -5,6 +5,10 @@
#include "hw/acpi/acpi.h"
#include "hw/acpi/aml-build.h"
+#define MEMORY_SLOT_SCAN_METHOD "MSCN"
+#define MEMORY_DEVICES_CONTAINER "\\_SB.MHPC"
+#define MEMORY_HOTPLUG_IO_LEN 24
+
/**
* MemStatus:
* @is_removing: the memory device in slot has been requested to be ejected.
@@ -29,7 +33,7 @@ typedef struct MemHotplugState {
} 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 +52,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.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 2/9] hw/acpi: Do not create memory hotplug method when handler is not defined
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 1/9] hw/acpi: Make ACPI IO address space configurable Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support Shameer Kolothum
` (6 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
From: Samuel Ortiz <sameo@linux.intel.com>
With Hardware-reduced ACPI, the GED device will manage ACPI
hotplug entirely. As a consequence, make the memory specific
events AML generation optional. The code will only be added
when the method name is not NULL.
Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/memory_hotplug.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index c724f5f1e4..7e30e6f886 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -719,10 +719,12 @@ void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
}
aml_append(table, dev_container);
- method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
- aml_append(method,
- aml_call0(MEMORY_DEVICES_CONTAINER "." MEMORY_SLOT_SCAN_METHOD));
- aml_append(table, method);
+ if (event_handler_method) {
+ method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
+ aml_append(method, aml_call0(MEMORY_DEVICES_CONTAINER "."
+ MEMORY_SLOT_SCAN_METHOD));
+ aml_append(table, method);
+ }
g_free(mhp_res_path);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 1/9] hw/acpi: Make ACPI IO address space configurable Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 2/9] hw/acpi: Do not create memory hotplug method when handler is not defined Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-07-30 15:25 ` Igor Mammedov
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 4/9] hw/arm/virt: Add memory hotplug framework Shameer Kolothum
` (5 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
From: Samuel Ortiz <sameo@linux.intel.com>
The ACPI Generic Event Device (GED) is a hardware-reduced specific
device[ACPI v6.1 Section 5.6.9] that handles all platform events,
including the hotplug ones. This patch generates the AML code that
defines GEDs.
Platforms need to specify their own GED Event bitmap to describe
what kind of events they want to support through GED. Also this
uses a a single interrupt for the GED device, relying on IO
memory region to communicate the type of device affected by the
interrupt. This way, we can support up to 32 events with a unique
interrupt.
This supports only memory hotplug for now.
Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
v7 --> v8.
-Removed qemu_mutex_lock() across the ged state selector access.
-Rephrased comments section in acpi_ged_send_event().
-Moved acpi_ged_event() code into acpi_ged_send_event().
-Added check for memhp_base and ged_base in realize().
---
hw/acpi/Kconfig | 4 +
hw/acpi/Makefile.objs | 1 +
hw/acpi/generic_event_device.c | 322 +++++++++++++++++++++++++
include/hw/acpi/generic_event_device.h | 100 ++++++++
4 files changed, 427 insertions(+)
create mode 100644 hw/acpi/generic_event_device.c
create mode 100644 include/hw/acpi/generic_event_device.h
diff --git a/hw/acpi/Kconfig b/hw/acpi/Kconfig
index 7c59cf900b..12e3f1e86e 100644
--- a/hw/acpi/Kconfig
+++ b/hw/acpi/Kconfig
@@ -31,3 +31,7 @@ config ACPI_VMGENID
bool
default y
depends on PC
+
+config ACPI_HW_REDUCED
+ bool
+ depends on ACPI
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 9bb2101e3b..655a9c1973 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
+common-obj-$(CONFIG_ACPI_HW_REDUCED) += generic_event_device.o
common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
common-obj-y += acpi_interface.o
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
new file mode 100644
index 0000000000..7902e9d706
--- /dev/null
+++ b/hw/acpi/generic_event_device.c
@@ -0,0 +1,322 @@
+/*
+ *
+ * Copyright (c) 2018 Intel Corporation
+ * Copyright (c) 2019 Huawei Technologies R & D (UK) Ltd
+ * Written by Samuel Ortiz, Shameer Kolothum
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "exec/address-spaces.h"
+#include "hw/acpi/acpi.h"
+#include "hw/acpi/generic_event_device.h"
+#include "hw/mem/pc-dimm.h"
+#include "qemu/error-report.h"
+
+static const uint32_t ged_supported_events[] = {
+ ACPI_GED_MEM_HOTPLUG_EVT,
+};
+
+/*
+ * The ACPI Generic Event Device (GED) is a hardware-reduced specific
+ * device[ACPI v6.1 Section 5.6.9] that handles all platform events,
+ * including the hotplug ones. Platforms need to specify their own
+ * GED Event bitmap to describe what kind of events they want to support
+ * through GED. This routine uses a single interrupt for the GED device,
+ * relying on IO memory region to communicate the type of device
+ * affected by the interrupt. This way, we can support up to 32 events
+ * with a unique interrupt.
+ */
+void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
+ uint32_t ged_irq, AmlRegionSpace rs)
+{
+ AcpiGedState *s = ACPI_GED(hotplug_dev);
+ Aml *crs = aml_resource_template();
+ Aml *evt, *field;
+ Aml *dev = aml_device("%s", name);
+ Aml *evt_sel = aml_local(0);
+ Aml *esel = aml_name(AML_GED_EVT_SEL);
+
+ assert(s->ged_base);
+
+ /* _CRS interrupt */
+ aml_append(crs, aml_interrupt(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
+ AML_EXCLUSIVE, &ged_irq, 1));
+
+ aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0013")));
+ aml_append(dev, aml_name_decl("_UID", aml_string(GED_DEVICE)));
+ aml_append(dev, aml_name_decl("_CRS", crs));
+
+ /* Append IO region */
+ aml_append(dev, aml_operation_region(AML_GED_EVT_REG, rs,
+ aml_int(s->ged_base + ACPI_GED_EVT_SEL_OFFSET),
+ ACPI_GED_EVT_SEL_LEN));
+ field = aml_field(AML_GED_EVT_REG, AML_DWORD_ACC, AML_NOLOCK,
+ AML_WRITE_AS_ZEROS);
+ aml_append(field, aml_named_field(AML_GED_EVT_SEL,
+ ACPI_GED_EVT_SEL_LEN * BITS_PER_BYTE));
+ aml_append(dev, field);
+
+ /*
+ * For each GED event we:
+ * - Add a conditional block for each event, inside a loop.
+ * - Call a method for each supported GED event type.
+ *
+ * The resulting ASL code looks like:
+ *
+ * Local0 = ESEL
+ * If ((Local0 & One) == One)
+ * {
+ * MethodEvent0()
+ * }
+ *
+ * If ((Local0 & 0x2) == 0x2)
+ * {
+ * MethodEvent1()
+ * }
+ * ...
+ */
+ evt = aml_method("_EVT", 1, AML_SERIALIZED);
+ {
+ Aml *if_ctx;
+ uint32_t i;
+ uint32_t ged_events = ctpop32(s->ged_event_bitmap);
+
+ /* Local0 = ESEL */
+ aml_append(evt, aml_store(esel, evt_sel));
+
+ for (i = 0; i < ARRAY_SIZE(ged_supported_events) && ged_events; i++) {
+ uint32_t event = s->ged_event_bitmap & ged_supported_events[i];
+
+ if (!event) {
+ continue;
+ }
+
+ if_ctx = aml_if(aml_equal(aml_and(evt_sel, aml_int(event), NULL),
+ aml_int(event)));
+ switch (event) {
+ case ACPI_GED_MEM_HOTPLUG_EVT:
+ aml_append(if_ctx, aml_call0(MEMORY_DEVICES_CONTAINER "."
+ MEMORY_SLOT_SCAN_METHOD));
+ break;
+ default:
+ /*
+ * Please make sure all the events in ged_supported_events[]
+ * are handled above.
+ */
+ g_assert_not_reached();
+ }
+
+ aml_append(evt, if_ctx);
+ ged_events--;
+ }
+
+ if (ged_events) {
+ error_report("GED: Unsupported events specified");
+ exit(1);
+ }
+ }
+
+ /* Append _EVT method */
+ aml_append(dev, evt);
+
+ aml_append(table, dev);
+}
+
+/* Memory read by the GED _EVT AML dynamic method */
+static uint64_t ged_read(void *opaque, hwaddr addr, unsigned size)
+{
+ uint64_t val = 0;
+ GEDState *ged_st = opaque;
+
+ switch (addr) {
+ case ACPI_GED_EVT_SEL_OFFSET:
+ /* Read the selector value and reset it */
+ val = ged_st->sel;
+ ged_st->sel = 0;
+ break;
+ default:
+ break;
+ }
+
+ return val;
+}
+
+/* Nothing is expected to be written to the GED memory region */
+static void ged_write(void *opaque, hwaddr addr, uint64_t data,
+ unsigned int size)
+{
+}
+
+static const MemoryRegionOps ged_ops = {
+ .read = ged_read,
+ .write = ged_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .valid = {
+ .min_access_size = 4,
+ .max_access_size = 4,
+ },
+};
+
+static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st)
+{
+ AcpiGedState *s = ACPI_GED(dev);
+
+ memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
+ TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN);
+ memory_region_add_subregion(as, s->ged_base, &ged_st->io);
+ qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1);
+}
+
+static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ AcpiGedState *s = ACPI_GED(hotplug_dev);
+
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ if (s->memhp_state.is_enabled) {
+ acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
+ } else {
+ error_setg(errp,
+ "memory hotplug is not enabled: %s.memory-hotplug-support "
+ "is not set", object_get_typename(OBJECT(s)));
+ }
+ } else {
+ error_setg(errp, "virt: device plug request for unsupported device"
+ " type: %s", object_get_typename(OBJECT(dev)));
+ }
+}
+
+static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
+{
+ AcpiGedState *s = ACPI_GED(adev);
+ GEDState *ged_st = &s->ged_state;
+ uint32_t sel;
+
+ if (ev & ACPI_MEMORY_HOTPLUG_STATUS) {
+ sel = ACPI_GED_MEM_HOTPLUG_EVT;
+ } else {
+ /* Unknown event. Return without generating interrupt. */
+ warn_report("GED: Unsupported event %d. No irq injected", ev);
+ return;
+ }
+
+ /*
+ * Set the GED selector field to communicate the event type.
+ * This will be read by GED aml code to select the appropriate
+ * event method.
+ */
+ ged_st->sel |= sel;
+
+ /* Trigger the event by sending an interrupt to the guest. */
+ qemu_irq_pulse(s->irq);
+}
+
+static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
+{
+ AcpiGedState *s = ACPI_GED(dev);
+
+ assert(s->ged_base);
+ acpi_ged_init(get_system_memory(), dev, &s->ged_state);
+
+ if (s->memhp_state.is_enabled) {
+ assert(s->memhp_base);
+ acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
+ &s->memhp_state,
+ s->memhp_base);
+ }
+}
+
+static Property acpi_ged_properties[] = {
+ /*
+ * Memory hotplug base address is a property of GED here,
+ * because GED handles memory hotplug event and acpi-mem-hotplug
+ * memory region gets initialized when GED device is realized.
+ */
+ DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base, 0),
+ DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState,
+ memhp_state.is_enabled, true),
+ DEFINE_PROP_UINT64("ged-base", AcpiGedState, ged_base, 0),
+ DEFINE_PROP_UINT32("ged-event", AcpiGedState, ged_event_bitmap, 0),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static bool vmstate_test_use_memhp(void *opaque)
+{
+ AcpiGedState *s = opaque;
+ return s->memhp_state.is_enabled;
+}
+
+static const VMStateDescription vmstate_memhp_state = {
+ .name = "acpi-ged/memhp",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = vmstate_test_use_memhp,
+ .fields = (VMStateField[]) {
+ VMSTATE_MEMORY_HOTPLUG(memhp_state, AcpiGedState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_ged_state = {
+ .name = "acpi-ged-state",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(sel, GEDState),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static const VMStateDescription vmstate_acpi_ged = {
+ .name = "acpi-ged",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT(ged_state, AcpiGedState, 1, vmstate_ged_state, GEDState),
+ VMSTATE_END_OF_LIST(),
+ },
+ .subsections = (const VMStateDescription * []) {
+ &vmstate_memhp_state,
+ NULL
+ }
+};
+
+static void acpi_ged_class_init(ObjectClass *class, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(class);
+ HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(class);
+ AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_CLASS(class);
+
+ dc->desc = "ACPI Generic Event Device";
+ dc->props = acpi_ged_properties;
+ dc->realize = acpi_ged_device_realize;
+ dc->vmsd = &vmstate_acpi_ged;
+
+ hc->plug = acpi_ged_device_plug_cb;
+
+ adevc->send_event = acpi_ged_send_event;
+}
+
+static const TypeInfo acpi_ged_info = {
+ .name = TYPE_ACPI_GED,
+ .parent = TYPE_DEVICE,
+ .instance_size = sizeof(AcpiGedState),
+ .class_init = acpi_ged_class_init,
+ .interfaces = (InterfaceInfo[]) {
+ { TYPE_HOTPLUG_HANDLER },
+ { TYPE_ACPI_DEVICE_IF },
+ { }
+ }
+};
+
+static void acpi_ged_register_types(void)
+{
+ type_register_static(&acpi_ged_info);
+}
+
+type_init(acpi_ged_register_types)
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
new file mode 100644
index 0000000000..f0152b0018
--- /dev/null
+++ b/include/hw/acpi/generic_event_device.h
@@ -0,0 +1,100 @@
+/*
+ *
+ * Copyright (c) 2018 Intel Corporation
+ * Copyright (c) 2019 Huawei Technologies R & D (UK) Ltd
+ * Written by Samuel Ortiz, Shameer Kolothum
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * The ACPI Generic Event Device (GED) is a hardware-reduced specific
+ * device[ACPI v6.1 Section 5.6.9] that handles all platform events,
+ * including the hotplug ones. Generic Event Device allows platforms
+ * to handle interrupts in ACPI ASL statements. It follows a very
+ * similar approach like the _EVT method from GPIO events. All
+ * interrupts are listed in _CRS and the handler is written in _EVT
+ * method. Here, we use a single interrupt for the GED device, relying
+ * on IO memory region to communicate the type of device affected by
+ * the interrupt. This way, we can support up to 32 events with a
+ * unique interrupt.
+ *
+ * Here is an example.
+ *
+ * Device (\_SB.GED)
+ * {
+ * Name (_HID, "ACPI0013")
+ * Name (_UID, Zero)
+ * Name (_CRS, ResourceTemplate ()
+ * {
+ * Interrupt (ResourceConsumer, Edge, ActiveHigh, Exclusive, ,, )
+ * {
+ * 0x00000029,
+ * }
+ * })
+ * OperationRegion (EREG, SystemMemory, 0x09080000, 0x04)
+ * Field (EREG, DWordAcc, NoLock, WriteAsZeros)
+ * {
+ * ESEL, 32
+ * }
+ *
+ * Method (_EVT, 1, Serialized) // _EVT: Event
+ * {
+ * Local0 = ESEL // ESEL = IO memory region which specifies the
+ * // device type.
+ * If (((Local0 & One) == One))
+ * {
+ * MethodEvent1()
+ * }
+ * If ((Local0 & 0x2) == 0x2)
+ * {
+ * MethodEvent2()
+ * }
+ * ...
+ * }
+ * }
+ *
+ */
+
+#ifndef HW_ACPI_GED_H
+#define HW_ACPI_GED_H
+
+#include "hw/acpi/memory_hotplug.h"
+
+#define TYPE_ACPI_GED "acpi-ged"
+#define ACPI_GED(obj) \
+ OBJECT_CHECK(AcpiGedState, (obj), TYPE_ACPI_GED)
+
+#define ACPI_GED_EVT_SEL_OFFSET 0x0
+#define ACPI_GED_EVT_SEL_LEN 0x4
+
+#define GED_DEVICE "GED"
+#define AML_GED_EVT_REG "EREG"
+#define AML_GED_EVT_SEL "ESEL"
+
+/*
+ * Platforms need to specify the GED event bitmap
+ * to describe what kind of events they want to support
+ * through GED.
+ */
+#define ACPI_GED_MEM_HOTPLUG_EVT 0x1
+
+typedef struct GEDState {
+ MemoryRegion io;
+ uint32_t sel;
+} GEDState;
+
+typedef struct AcpiGedState {
+ DeviceClass parent_obj;
+ MemHotplugState memhp_state;
+ hwaddr memhp_base;
+ hwaddr ged_base;
+ GEDState ged_state;
+ uint32_t ged_event_bitmap;
+ qemu_irq irq;
+} AcpiGedState;
+
+void build_ged_aml(Aml *table, const char* name, HotplugHandler *hotplug_dev,
+ uint32_t ged_irq, AmlRegionSpace rs);
+
+#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 4/9] hw/arm/virt: Add memory hotplug framework
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
` (2 preceding siblings ...)
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 5/9] hw/arm/virt: Add 4.2 machine type Shameer Kolothum
` (4 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
From: Eric Auger <eric.auger@redhat.com>
This patch adds the memory hot-plug/hot-unplug infrastructure
in machvirt. The device memory is not yet exposed to the Guest
either through DT or ACPI and hence both cold/hot plug of memory
is explicitly disabled for now.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Kwangwoo Lee <kwangwoo.lee@sk.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/arm/Kconfig | 2 ++
hw/arm/virt.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index ab65ecd216..84961c17ab 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -20,6 +20,8 @@ config ARM_VIRT
select SMBIOS
select VIRTIO_MMIO
select ACPI_PCI
+ select MEM_DEVICE
+ select DIMM
config CHEETAH
bool
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d9496c9363..907fb64bb9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -64,6 +64,8 @@
#include "hw/arm/smmuv3.h"
#include "hw/acpi/acpi.h"
#include "target/arm/internals.h"
+#include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
#define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -1871,6 +1873,40 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
return ms->possible_cpus;
}
+static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
+ Error **errp)
+{
+
+ /*
+ * The device memory is not yet exposed to the Guest either through
+ * DT or ACPI and hence both cold/hot plug of memory is explicitly
+ * disabled for now.
+ */
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ error_setg(errp, "memory cold/hot plug is not yet supported");
+ return;
+ }
+
+ pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
+}
+
+static void virt_memory_plug(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+
+ pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
+
+}
+
+static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ virt_memory_pre_plug(hotplug_dev, dev, errp);
+ }
+}
+
static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
@@ -1882,12 +1918,23 @@ static void virt_machine_device_plug_cb(HotplugHandler *hotplug_dev,
SYS_BUS_DEVICE(dev));
}
}
+ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+ virt_memory_plug(hotplug_dev, dev, errp);
+ }
+}
+
+static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
+ DeviceState *dev, Error **errp)
+{
+ error_setg(errp, "device unplug request for unsupported device"
+ " type: %s", object_get_typename(OBJECT(dev)));
}
static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
DeviceState *dev)
{
- if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE)) {
+ if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) ||
+ (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) {
return HOTPLUG_HANDLER(machine);
}
@@ -1951,7 +1998,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
mc->kvm_type = virt_kvm_type;
assert(!mc->get_hotplug_handler);
mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
+ hc->pre_plug = virt_machine_device_pre_plug_cb;
hc->plug = virt_machine_device_plug_cb;
+ hc->unplug_request = virt_machine_device_unplug_request_cb;
mc->numa_mem_supported = true;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 5/9] hw/arm/virt: Add 4.2 machine type
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
` (3 preceding siblings ...)
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 4/9] hw/arm/virt: Add memory hotplug framework Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot Shameer Kolothum
` (3 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
This is in preparation to create ACPI GED device as we
need to disable it for <4.2 for migration to work.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/arm/virt.c | 9 ++++++++-
hw/core/machine.c | 3 +++
include/hw/boards.h | 3 +++
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 907fb64bb9..bbe156dc35 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2095,10 +2095,17 @@ static void machvirt_machine_init(void)
}
type_init(machvirt_machine_init);
+static void virt_machine_4_2_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
+
static void virt_machine_4_1_options(MachineClass *mc)
{
+ virt_machine_4_2_options(mc);
+ compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
}
-DEFINE_VIRT_MACHINE_AS_LATEST(4, 1)
+DEFINE_VIRT_MACHINE(4, 1)
static void virt_machine_4_0_options(MachineClass *mc)
{
diff --git a/hw/core/machine.c b/hw/core/machine.c
index c58a8e594e..a79d4ad740 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -27,6 +27,9 @@
#include "hw/pci/pci.h"
#include "hw/mem/nvdimm.h"
+GlobalProperty hw_compat_4_1[] = {};
+const size_t hw_compat_4_1_len = G_N_ELEMENTS(hw_compat_4_1);
+
GlobalProperty hw_compat_4_0[] = {
{ "VGA", "edid", "false" },
{ "secondary-vga", "edid", "false" },
diff --git a/include/hw/boards.h b/include/hw/boards.h
index a71d1a53a5..d9ec37d807 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -317,6 +317,9 @@ struct MachineState {
} \
type_init(machine_initfn##_register_types)
+extern GlobalProperty hw_compat_4_1[];
+extern const size_t hw_compat_4_1_len;
+
extern GlobalProperty hw_compat_4_0[];
extern const size_t hw_compat_4_0_len;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
` (4 preceding siblings ...)
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 5/9] hw/arm/virt: Add 4.2 machine type Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-08-06 13:08 ` Igor Mammedov
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT Shameer Kolothum
` (2 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
This initializes the GED device with base memory and irq, configures
ged memory hotplug event and builds the corresponding aml code. With
this, both hot and cold plug of device memory is enabled now for Guest
with ACPI boot.
Memory cold plug support with Guest DT boot is not yet supported.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
-Changed no_acpi_dev to no_ged.
-Fixed 'dev' reference leak by object_new().
-Updated bios-tables-test-allowed-diff.h to avoid "make check"
failure.
---
hw/arm/Kconfig | 2 +
hw/arm/virt-acpi-build.c | 14 +++++++
hw/arm/virt.c | 54 +++++++++++++++++++++++----
include/hw/arm/virt.h | 4 ++
tests/bios-tables-test-allowed-diff.h | 1 +
5 files changed, 68 insertions(+), 7 deletions(-)
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 84961c17ab..ad7f7c089b 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -22,6 +22,8 @@ config ARM_VIRT
select ACPI_PCI
select MEM_DEVICE
select DIMM
+ select ACPI_MEMORY_HOTPLUG
+ select ACPI_HW_REDUCED
config CHEETAH
bool
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 0afb372769..018b1e326d 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -40,6 +40,8 @@
#include "hw/acpi/aml-build.h"
#include "hw/acpi/utils.h"
#include "hw/acpi/pci.h"
+#include "hw/acpi/memory_hotplug.h"
+#include "hw/acpi/generic_event_device.h"
#include "hw/pci/pcie_host.h"
#include "hw/pci/pci.h"
#include "hw/arm/virt.h"
@@ -705,6 +707,7 @@ static void
build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
{
Aml *scope, *dsdt;
+ MachineState *ms = MACHINE(vms);
const MemMapEntry *memmap = vms->memmap;
const int *irqmap = vms->irqmap;
@@ -729,6 +732,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
vms->highmem, vms->highmem_ecam);
acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
(irqmap[VIRT_GPIO] + ARM_SPI_BASE));
+ if (vms->acpi_dev) {
+ build_ged_aml(scope, "\\_SB."GED_DEVICE,
+ HOTPLUG_HANDLER(vms->acpi_dev),
+ irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY);
+ }
+
+ if (vms->acpi_dev && ms->ram_slots) {
+ build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB", NULL,
+ AML_SYSTEM_MEMORY);
+ }
+
acpi_dsdt_add_power_button(scope);
aml_append(dsdt, scope);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index bbe156dc35..41386a6eb7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -66,6 +66,7 @@
#include "target/arm/internals.h"
#include "hw/mem/pc-dimm.h"
#include "hw/mem/nvdimm.h"
+#include "hw/acpi/generic_event_device.h"
#define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -136,6 +137,8 @@ static const MemMapEntry base_memmap[] = {
[VIRT_GPIO] = { 0x09030000, 0x00001000 },
[VIRT_SECURE_UART] = { 0x09040000, 0x00001000 },
[VIRT_SMMU] = { 0x09050000, 0x00020000 },
+ [VIRT_PCDIMM_ACPI] = { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
+ [VIRT_ACPI_GED] = { 0x09080000, ACPI_GED_EVT_SEL_LEN },
[VIRT_MMIO] = { 0x0a000000, 0x00000200 },
/* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
[VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
@@ -171,6 +174,7 @@ static const int a15irqmap[] = {
[VIRT_PCIE] = 3, /* ... to 6 */
[VIRT_GPIO] = 7,
[VIRT_SECURE_UART] = 8,
+ [VIRT_ACPI_GED] = 9,
[VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
[VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
[VIRT_SMMU] = 74, /* ...to 74 + NUM_SMMU_IRQS - 1 */
@@ -520,6 +524,27 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
}
}
+static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
+{
+ DeviceState *dev;
+ int irq = vms->irqmap[VIRT_ACPI_GED];
+ uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
+
+ dev = DEVICE(object_new(TYPE_ACPI_GED));
+ qdev_prop_set_uint64(dev, "memhp-base",
+ vms->memmap[VIRT_PCDIMM_ACPI].base);
+ qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
+ qdev_prop_set_uint32(dev, "ged-event", event);
+ object_property_add_child(qdev_get_machine(), "acpi-ged",
+ OBJECT(dev), NULL);
+ qdev_init_nofail(dev);
+ qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
+
+ object_unref(OBJECT(dev));
+
+ return dev;
+}
+
static void create_its(VirtMachineState *vms, DeviceState *gicdev)
{
const char *itsclass = its_class_name();
@@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState *machine)
MemoryRegion *ram = g_new(MemoryRegion, 1);
bool firmware_loaded;
bool aarch64 = true;
+ bool has_ged = !vmc->no_ged;
unsigned int smp_cpus = machine->smp.cpus;
unsigned int max_cpus = machine->smp.max_cpus;
@@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState *machine)
create_gpio(vms, pic);
+ if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
+ vms->acpi_dev = create_acpi_ged(vms, pic);
+ }
+
/* Create mmio transports, so the user can create virtio backends
* (which will be automatically plugged in to the transports). If
* no backend is created the transport will just sit harmlessly idle.
@@ -1876,27 +1906,34 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
Error **errp)
{
+ VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+ const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
- /*
- * The device memory is not yet exposed to the Guest either through
- * DT or ACPI and hence both cold/hot plug of memory is explicitly
- * disabled for now.
- */
- if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
- error_setg(errp, "memory cold/hot plug is not yet supported");
+ if (is_nvdimm) {
+ error_setg(errp, "nvdimm is not yet supported");
return;
}
+ if (!vms->acpi_dev) {
+ error_setg(errp, "memory hotplug is not enabled: missing acpi device");
+ return;
+ }
+
+ hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, errp);
+
pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
}
static void virt_memory_plug(HotplugHandler *hotplug_dev,
DeviceState *dev, Error **errp)
{
+ HotplugHandlerClass *hhc;
VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
+ hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
+ hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
}
static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
@@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
static void virt_machine_4_1_options(MachineClass *mc)
{
+ VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
virt_machine_4_2_options(mc);
compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
+ vmc->no_ged = true;
}
DEFINE_VIRT_MACHINE(4, 1)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index a72094204e..577ee49b4b 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -77,6 +77,8 @@ enum {
VIRT_GPIO,
VIRT_SECURE_UART,
VIRT_SECURE_MEM,
+ VIRT_PCDIMM_ACPI,
+ VIRT_ACPI_GED,
VIRT_LOWMEMMAP_LAST,
};
@@ -106,6 +108,7 @@ typedef struct {
bool claim_edge_triggered_timers;
bool smbios_old_sys_ver;
bool no_highmem_ecam;
+ bool no_ged; /* Machines < 4.2 has no support for ACPI GED device */
} VirtMachineClass;
typedef struct {
@@ -133,6 +136,7 @@ typedef struct {
uint32_t iommu_phandle;
int psci_conduit;
hwaddr highest_gpa;
+ DeviceState *acpi_dev;
} VirtMachineState;
#define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
index dfb8523c8b..7b4adbc822 100644
--- a/tests/bios-tables-test-allowed-diff.h
+++ b/tests/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@
/* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/DSDT",
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
` (5 preceding siblings ...)
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-08-06 13:21 ` Igor Mammedov
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 8/9] hw/acpi: Add system power down support to GED Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 9/9] hw/arm: Use GED for system_powerdown event Shameer Kolothum
8 siblings, 1 reply; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
Generate Memory Affinity Structures for PC-DIMM ranges.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
hw/arm/virt-acpi-build.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 018b1e326d..75657caa36 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -518,6 +518,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
int i, srat_start;
uint64_t mem_base;
MachineClass *mc = MACHINE_GET_CLASS(vms);
+ MachineState *ms = MACHINE(vms);
const CPUArchIdList *cpu_list = mc->possible_cpu_arch_ids(MACHINE(vms));
srat_start = table_data->len;
@@ -543,6 +544,14 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
}
}
+ if (ms->device_memory) {
+ numamem = acpi_data_push(table_data, sizeof *numamem);
+ build_srat_memory(numamem, ms->device_memory->base,
+ memory_region_size(&ms->device_memory->mr),
+ nb_numa_nodes - 1,
+ MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
+ }
+
build_header(linker, table_data, (void *)(table_data->data + srat_start),
"SRAT", table_data->len - srat_start, 3, NULL, NULL);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 8/9] hw/acpi: Add system power down support to GED
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
` (6 preceding siblings ...)
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 9/9] hw/arm: Use GED for system_powerdown event Shameer Kolothum
8 siblings, 0 replies; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
This adds support to use GED for system power down event.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
hw/acpi/generic_event_device.c | 6 ++++++
include/hw/acpi/generic_event_device.h | 3 +++
2 files changed, 9 insertions(+)
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 7902e9d706..0b3214eff4 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -19,6 +19,7 @@
static const uint32_t ged_supported_events[] = {
ACPI_GED_MEM_HOTPLUG_EVT,
+ ACPI_GED_PWR_DOWN_EVT,
};
/*
@@ -103,6 +104,11 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
aml_append(if_ctx, aml_call0(MEMORY_DEVICES_CONTAINER "."
MEMORY_SLOT_SCAN_METHOD));
break;
+ case ACPI_GED_PWR_DOWN_EVT:
+ aml_append(if_ctx,
+ aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
+ aml_int(0x80)));
+ break;
default:
/*
* Please make sure all the events in ged_supported_events[]
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index f0152b0018..63104f1344 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -61,6 +61,8 @@
#include "hw/acpi/memory_hotplug.h"
+#define ACPI_POWER_BUTTON_DEVICE "PWRB"
+
#define TYPE_ACPI_GED "acpi-ged"
#define ACPI_GED(obj) \
OBJECT_CHECK(AcpiGedState, (obj), TYPE_ACPI_GED)
@@ -78,6 +80,7 @@
* through GED.
*/
#define ACPI_GED_MEM_HOTPLUG_EVT 0x1
+#define ACPI_GED_PWR_DOWN_EVT 0x2
typedef struct GEDState {
MemoryRegion io;
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH-for-4.2 v8 9/9] hw/arm: Use GED for system_powerdown event
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
` (7 preceding siblings ...)
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 8/9] hw/acpi: Add system power down support to GED Shameer Kolothum
@ 2019-07-26 10:45 ` Shameer Kolothum
2019-08-06 13:38 ` Igor Mammedov
8 siblings, 1 reply; 26+ messages in thread
From: Shameer Kolothum @ 2019-07-26 10:45 UTC (permalink / raw)
To: qemu-devel, qemu-arm, eric.auger, imammedo
Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
shannon.zhaosl, sebastien.boeuf, lersek
For machines 4.2 or higher with ACPI boot use GED for system_powerdown
event instead of GPIO. Guest boot with DT still uses GPIO.
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
v7 --> v8
-Retained gpio based system_powerdown support for machines < 4.2.
-Reuse of virt_powerdown_req() for ACPI GED use.
-Dropped Eric's R-by for now because of above.
---
hw/acpi/generic_event_device.c | 2 ++
hw/arm/virt-acpi-build.c | 6 +++---
hw/arm/virt.c | 27 ++++++++++++++++-----------
include/hw/acpi/acpi_dev_interface.h | 1 +
include/hw/arm/virt.h | 1 +
5 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 0b3214eff4..f00b0ab14b 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -205,6 +205,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
if (ev & ACPI_MEMORY_HOTPLUG_STATUS) {
sel = ACPI_GED_MEM_HOTPLUG_EVT;
+ } else if (ev & ACPI_POWER_DOWN_STATUS) {
+ sel = ACPI_GED_PWR_DOWN_EVT;
} else {
/* Unknown event. Return without generating interrupt. */
warn_report("GED: Unsupported event %d. No irq injected", ev);
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 75657caa36..31bf2bf775 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -49,7 +49,6 @@
#include "kvm_arm.h"
#define ARM_SPI_BASE 32
-#define ACPI_POWER_BUTTON_DEVICE "PWRB"
static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
{
@@ -739,12 +738,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
(irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
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));
if (vms->acpi_dev) {
build_ged_aml(scope, "\\_SB."GED_DEVICE,
HOTPLUG_HANDLER(vms->acpi_dev),
irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY);
+ } else {
+ acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
+ (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
}
if (vms->acpi_dev && ms->ram_slots) {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 41386a6eb7..73a758d9a9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -528,7 +528,7 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
{
DeviceState *dev;
int irq = vms->irqmap[VIRT_ACPI_GED];
- uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
+ uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT | ACPI_GED_PWR_DOWN_EVT;
dev = DEVICE(object_new(TYPE_ACPI_GED));
qdev_prop_set_uint64(dev, "memhp-base",
@@ -784,13 +784,15 @@ static void create_rtc(const VirtMachineState *vms, qemu_irq *pic)
static DeviceState *gpio_key_dev;
static void virt_powerdown_req(Notifier *n, void *opaque)
{
- /* use gpio Pin 3 for power button event */
- qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
-}
+ VirtMachineState *s = container_of(n, VirtMachineState, powerdown_notifier);
-static Notifier virt_system_powerdown_notifier = {
- .notify = virt_powerdown_req
-};
+ if (s->acpi_dev) {
+ acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
+ } else {
+ /* use gpio Pin 3 for power button event */
+ qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
+ }
+}
static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
{
@@ -833,9 +835,6 @@ static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
"gpios", phandle, 3, 0);
- /* connect powerdown request */
- qemu_register_powerdown_notifier(&virt_system_powerdown_notifier);
-
g_free(nodename);
}
@@ -1721,12 +1720,18 @@ static void machvirt_init(MachineState *machine)
create_pcie(vms, pic);
- create_gpio(vms, pic);
if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
vms->acpi_dev = create_acpi_ged(vms, pic);
+ } else {
+ create_gpio(vms, pic);
}
+ /* connect powerdown request */
+ vms->powerdown_notifier.notify = virt_powerdown_req;
+ qemu_register_powerdown_notifier(&vms->powerdown_notifier);
+
+
/* Create mmio transports, so the user can create virtio backends
* (which will be automatically plugged in to the transports). If
* no backend is created the transport will just sit harmlessly idle.
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index 43ff119179..adcb3a816c 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -11,6 +11,7 @@ typedef enum {
ACPI_MEMORY_HOTPLUG_STATUS = 8,
ACPI_NVDIMM_HOTPLUG_STATUS = 16,
ACPI_VMGENID_CHANGE_STATUS = 32,
+ ACPI_POWER_DOWN_STATUS = 64,
} AcpiEventStatusBits;
#define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 577ee49b4b..0b41083e9d 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -137,6 +137,7 @@ typedef struct {
int psci_conduit;
hwaddr highest_gpa;
DeviceState *acpi_dev;
+ Notifier powerdown_notifier;
} VirtMachineState;
#define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support Shameer Kolothum
@ 2019-07-30 15:25 ` Igor Mammedov
2019-08-01 8:36 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 26+ messages in thread
From: Igor Mammedov @ 2019-07-30 15:25 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, sameo, ard.biesheuvel, shannon.zhaosl, qemu-devel,
xuwei5, linuxarm, eric.auger, qemu-arm, Paolo Bonzini,
sebastien.boeuf, lersek
On Fri, 26 Jul 2019 11:45:13 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> From: Samuel Ortiz <sameo@linux.intel.com>
>
> The ACPI Generic Event Device (GED) is a hardware-reduced specific
> device[ACPI v6.1 Section 5.6.9] that handles all platform events,
> including the hotplug ones. This patch generates the AML code that
> defines GEDs.
>
> Platforms need to specify their own GED Event bitmap to describe
> what kind of events they want to support through GED. Also this
> uses a a single interrupt for the GED device, relying on IO
> memory region to communicate the type of device affected by the
> interrupt. This way, we can support up to 32 events with a unique
> interrupt.
>
> This supports only memory hotplug for now.
>
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> new file mode 100644
> index 0000000000..7902e9d706
> --- /dev/null
> +++ b/hw/acpi/generic_event_device.c
[...]
> +void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
> + uint32_t ged_irq, AmlRegionSpace rs)
> +{
[...]
> +
> + if (ged_events) {
> + error_report("GED: Unsupported events specified");
> + exit(1);
I'd use error_abort instead, since it's programing error, if you have to respin series.
> + }
> + }
> +
> + /* Append _EVT method */
> + aml_append(dev, evt);
> +
> + aml_append(table, dev);
> +}
> +
[...]
> +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> +{
> + AcpiGedState *s = ACPI_GED(dev);
> +
> + assert(s->ged_base);
> + acpi_ged_init(get_system_memory(), dev, &s->ged_state);
calling get_system_memory() from device code used to be a reason for rejecting patch,
I'm not sure what suggest though.
Maybe Paolo could suggest something.
> +
> + if (s->memhp_state.is_enabled) {
> + assert(s->memhp_base);
> + acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
^^^^
ditto
> + &s->memhp_state,
> + s->memhp_base);
> + }
> +}
[...]
> +
> +#endif
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support
2019-07-30 15:25 ` Igor Mammedov
@ 2019-08-01 8:36 ` Shameerali Kolothum Thodi
2019-08-05 13:30 ` Igor Mammedov
0 siblings, 1 reply; 26+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-08-01 8:36 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, sameo, ard.biesheuvel, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
Paolo Bonzini, sebastien.boeuf, lersek, eric.auger
Hi Igor,
> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 30 July 2019 16:25
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> ard.biesheuvel@linaro.org; shannon.zhaosl@gmail.com;
> qemu-devel@nongnu.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm
> <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> Paolo Bonzini <pbonzini@redhat.com>; sebastien.boeuf@intel.com;
> lersek@redhat.com
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic
> Event Device Support
>
> On Fri, 26 Jul 2019 11:45:13 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > From: Samuel Ortiz <sameo@linux.intel.com>
> >
> > The ACPI Generic Event Device (GED) is a hardware-reduced specific
> > device[ACPI v6.1 Section 5.6.9] that handles all platform events,
> > including the hotplug ones. This patch generates the AML code that
> > defines GEDs.
> >
> > Platforms need to specify their own GED Event bitmap to describe
> > what kind of events they want to support through GED. Also this
> > uses a a single interrupt for the GED device, relying on IO
> > memory region to communicate the type of device affected by the
> > interrupt. This way, we can support up to 32 events with a unique
> > interrupt.
> >
> > This supports only memory hotplug for now.
> >
>
> > diff --git a/hw/acpi/generic_event_device.c
> b/hw/acpi/generic_event_device.c
> > new file mode 100644
> > index 0000000000..7902e9d706
> > --- /dev/null
> > +++ b/hw/acpi/generic_event_device.c
> [...]
> > +void build_ged_aml(Aml *table, const char *name, HotplugHandler
> *hotplug_dev,
> > + uint32_t ged_irq, AmlRegionSpace rs)
> > +{
> [...]
> > +
> > + if (ged_events) {
> > + error_report("GED: Unsupported events specified");
> > + exit(1);
> I'd use error_abort instead, since it's programing error, if you have to respin
> series.
Ok.
> > + }
> > + }
> > +
> > + /* Append _EVT method */
> > + aml_append(dev, evt);
> > +
> > + aml_append(table, dev);
> > +}
> > +
> [...]
> > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > +{
> > + AcpiGedState *s = ACPI_GED(dev);
> > +
> > + assert(s->ged_base);
> > + acpi_ged_init(get_system_memory(), dev, &s->ged_state);
>
> calling get_system_memory() from device code used to be a reason for
> rejecting patch,
> I'm not sure what suggest though.
>
> Maybe Paolo could suggest something.
How about using object_property_set_link()? Something like below.
------8-----
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index f00b0ab14b..eb1ed38f4a 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -229,11 +229,12 @@ static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
AcpiGedState *s = ACPI_GED(dev);
assert(s->ged_base);
- acpi_ged_init(get_system_memory(), dev, &s->ged_state);
+ assert(s->sys_mem);
+ acpi_ged_init(s->sys_mem, dev, &s->ged_state);
if (s->memhp_state.is_enabled) {
assert(s->memhp_base);
- acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
+ acpi_memory_hotplug_init(s->sys_mem, OBJECT(dev),
&s->memhp_state,
s->memhp_base);
}
@@ -245,6 +246,8 @@ static Property acpi_ged_properties[] = {
* because GED handles memory hotplug event and acpi-mem-hotplug
* memory region gets initialized when GED device is realized.
*/
+ DEFINE_PROP_LINK("memory", AcpiGedState, sys_mem, TYPE_MEMORY_REGION,
+ MemoryRegion *),
DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base, 0),
DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState,
memhp_state.is_enabled, true),
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 73a758d9a9..0cbaf6c6e1 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -529,8 +529,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
DeviceState *dev;
int irq = vms->irqmap[VIRT_ACPI_GED];
uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT | ACPI_GED_PWR_DOWN_EVT;
+ MemoryRegion *sys_mem = get_system_memory();
dev = DEVICE(object_new(TYPE_ACPI_GED));
+
+ object_property_set_link(OBJECT(dev), OBJECT(sys_mem),
+ "memory", &error_abort);
qdev_prop_set_uint64(dev, "memhp-base",
vms->memmap[VIRT_PCDIMM_ACPI].base);
qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index 63104f1344..f1f2f337f7 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -89,6 +89,7 @@ typedef struct GEDState {
typedef struct AcpiGedState {
DeviceClass parent_obj;
+ MemoryRegion *sys_mem;
MemHotplugState memhp_state;
hwaddr memhp_base;
hwaddr ged_base;
---8----
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support
2019-08-01 8:36 ` Shameerali Kolothum Thodi
@ 2019-08-05 13:30 ` Igor Mammedov
2019-08-05 13:42 ` Peter Maydell
0 siblings, 1 reply; 26+ messages in thread
From: Igor Mammedov @ 2019-08-05 13:30 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: peter.maydell, sameo, ard.biesheuvel, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
Paolo Bonzini, sebastien.boeuf, lersek, eric.auger
On Thu, 1 Aug 2019 08:36:33 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> Hi Igor,
>
> > -----Original Message-----
> > From: Qemu-devel
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 30 July 2019 16:25
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> > ard.biesheuvel@linaro.org; shannon.zhaosl@gmail.com;
> > qemu-devel@nongnu.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm
> > <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> > Paolo Bonzini <pbonzini@redhat.com>; sebastien.boeuf@intel.com;
> > lersek@redhat.com
> > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic
> > Event Device Support
> >
> > On Fri, 26 Jul 2019 11:45:13 +0100
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > > From: Samuel Ortiz <sameo@linux.intel.com>
> > >
> > > The ACPI Generic Event Device (GED) is a hardware-reduced specific
> > > device[ACPI v6.1 Section 5.6.9] that handles all platform events,
> > > including the hotplug ones. This patch generates the AML code that
> > > defines GEDs.
> > >
> > > Platforms need to specify their own GED Event bitmap to describe
> > > what kind of events they want to support through GED. Also this
> > > uses a a single interrupt for the GED device, relying on IO
> > > memory region to communicate the type of device affected by the
> > > interrupt. This way, we can support up to 32 events with a unique
> > > interrupt.
> > >
> > > This supports only memory hotplug for now.
> > >
> >
> > > diff --git a/hw/acpi/generic_event_device.c
> > b/hw/acpi/generic_event_device.c
> > > new file mode 100644
> > > index 0000000000..7902e9d706
> > > --- /dev/null
> > > +++ b/hw/acpi/generic_event_device.c
> > [...]
> > > +void build_ged_aml(Aml *table, const char *name, HotplugHandler
> > *hotplug_dev,
> > > + uint32_t ged_irq, AmlRegionSpace rs)
> > > +{
> > [...]
> > > +
> > > + if (ged_events) {
> > > + error_report("GED: Unsupported events specified");
> > > + exit(1);
> > I'd use error_abort instead, since it's programing error, if you have to respin
> > series.
>
> Ok.
>
> > > + }
> > > + }
> > > +
> > > + /* Append _EVT method */
> > > + aml_append(dev, evt);
> > > +
> > > + aml_append(table, dev);
> > > +}
> > > +
> > [...]
> > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > > +{
> > > + AcpiGedState *s = ACPI_GED(dev);
> > > +
> > > + assert(s->ged_base);
> > > + acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> >
> > calling get_system_memory() from device code used to be a reason for
> > rejecting patch,
> > I'm not sure what suggest though.
> >
> > Maybe Paolo could suggest something.
>
> How about using object_property_set_link()? Something like below.
I'm afraid it doesn't help much. Issue here is that we are letting
device to manage whole address space (which should be managed by machine)
So I'd just keep get_system_memory() as is for now if there aren't any
objections.
> ------8-----
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index f00b0ab14b..eb1ed38f4a 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -229,11 +229,12 @@ static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> AcpiGedState *s = ACPI_GED(dev);
>
> assert(s->ged_base);
> - acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> + assert(s->sys_mem);
> + acpi_ged_init(s->sys_mem, dev, &s->ged_state);
>
> if (s->memhp_state.is_enabled) {
> assert(s->memhp_base);
> - acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
> + acpi_memory_hotplug_init(s->sys_mem, OBJECT(dev),
> &s->memhp_state,
> s->memhp_base);
> }
> @@ -245,6 +246,8 @@ static Property acpi_ged_properties[] = {
> * because GED handles memory hotplug event and acpi-mem-hotplug
> * memory region gets initialized when GED device is realized.
> */
> + DEFINE_PROP_LINK("memory", AcpiGedState, sys_mem, TYPE_MEMORY_REGION,
> + MemoryRegion *),
> DEFINE_PROP_UINT64("memhp-base", AcpiGedState, memhp_base, 0),
> DEFINE_PROP_BOOL("memory-hotplug-support", AcpiGedState,
> memhp_state.is_enabled, true),
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 73a758d9a9..0cbaf6c6e1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -529,8 +529,12 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
> DeviceState *dev;
> int irq = vms->irqmap[VIRT_ACPI_GED];
> uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT | ACPI_GED_PWR_DOWN_EVT;
> + MemoryRegion *sys_mem = get_system_memory();
>
> dev = DEVICE(object_new(TYPE_ACPI_GED));
> +
> + object_property_set_link(OBJECT(dev), OBJECT(sys_mem),
> + "memory", &error_abort);
> qdev_prop_set_uint64(dev, "memhp-base",
> vms->memmap[VIRT_PCDIMM_ACPI].base);
> qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index 63104f1344..f1f2f337f7 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -89,6 +89,7 @@ typedef struct GEDState {
>
> typedef struct AcpiGedState {
> DeviceClass parent_obj;
> + MemoryRegion *sys_mem;
> MemHotplugState memhp_state;
> hwaddr memhp_base;
> hwaddr ged_base;
>
> ---8----
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support
2019-08-05 13:30 ` Igor Mammedov
@ 2019-08-05 13:42 ` Peter Maydell
2019-08-05 15:46 ` Igor Mammedov
0 siblings, 1 reply; 26+ messages in thread
From: Peter Maydell @ 2019-08-05 13:42 UTC (permalink / raw)
To: Igor Mammedov
Cc: sameo, ard.biesheuvel, qemu-devel, Shameerali Kolothum Thodi,
Linuxarm, shannon.zhaosl, qemu-arm, xuwei (O),
Paolo Bonzini, sebastien.boeuf, lersek, eric.auger
On Mon, 5 Aug 2019 at 14:30, Igor Mammedov <imammedo@redhat.com> wrote:
>
> On Thu, 1 Aug 2019 08:36:33 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > Hi Igor,
> >
> > > -----Original Message-----
> > > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > > > +{
> > > > + AcpiGedState *s = ACPI_GED(dev);
> > > > +
> > > > + assert(s->ged_base);
> > > > + acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> > >
> > > calling get_system_memory() from device code used to be a reason for
> > > rejecting patch,
> > > I'm not sure what suggest though.
> > >
> > > Maybe Paolo could suggest something.
> >
> > How about using object_property_set_link()? Something like below.
> I'm afraid it doesn't help much. Issue here is that we are letting
> device to manage whole address space (which should be managed by machine)
> So I'd just keep get_system_memory() as is for now if there aren't any
> objections.
What are we trying to do with this device, and what does it need
the system memory region for?
In this case, we seem to do:
+static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st)
+{
+ AcpiGedState *s = ACPI_GED(dev);
+
+ memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
+ TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN);
+ memory_region_add_subregion(as, s->ged_base, &ged_st->io);
+ qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1);
+}
This is definitely a bad idea -- devices should not add their
own memory regions to the system memory MR. They should
expose their MRs (by being a sysbus-device) and let the board
code do the wiring up of the MRs into the right memory space
at the right address.
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support
2019-08-05 13:42 ` Peter Maydell
@ 2019-08-05 15:46 ` Igor Mammedov
2019-08-05 15:54 ` Peter Maydell
0 siblings, 1 reply; 26+ messages in thread
From: Igor Mammedov @ 2019-08-05 15:46 UTC (permalink / raw)
To: Peter Maydell
Cc: sameo, ard.biesheuvel, qemu-devel, Shameerali Kolothum Thodi,
Linuxarm, shannon.zhaosl, qemu-arm, xuwei (O),
Paolo Bonzini, sebastien.boeuf, lersek, eric.auger
On Mon, 5 Aug 2019 14:42:38 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On Mon, 5 Aug 2019 at 14:30, Igor Mammedov <imammedo@redhat.com> wrote:
> >
> > On Thu, 1 Aug 2019 08:36:33 +0000
> > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > > Hi Igor,
> > >
> > > > -----Original Message-----
> > > > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > > > > +{
> > > > > + AcpiGedState *s = ACPI_GED(dev);
> > > > > +
> > > > > + assert(s->ged_base);
> > > > > + acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> > > >
> > > > calling get_system_memory() from device code used to be a reason for
> > > > rejecting patch,
> > > > I'm not sure what suggest though.
> > > >
> > > > Maybe Paolo could suggest something.
> > >
> > > How about using object_property_set_link()? Something like below.
> > I'm afraid it doesn't help much. Issue here is that we are letting
> > device to manage whole address space (which should be managed by machine)
> > So I'd just keep get_system_memory() as is for now if there aren't any
> > objections.
>
> What are we trying to do with this device, and what does it need
> the system memory region for?
>
> In this case, we seem to do:
>
> +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st)
> +{
> + AcpiGedState *s = ACPI_GED(dev);
> +
> + memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
> + TYPE_ACPI_GED, ACPI_GED_EVT_SEL_LEN);
> + memory_region_add_subregion(as, s->ged_base, &ged_st->io);
> + qdev_init_gpio_out_named(DEVICE(s), &s->irq, "ged-irq", 1);
> +}
>
>
> This is definitely a bad idea -- devices should not add their
> own memory regions to the system memory MR. They should
> expose their MRs (by being a sysbus-device) and let the board
> code do the wiring up of the MRs into the right memory space
> at the right address.
it's not the only place in GED that is trying to add to system address
space, optionally if called acpi_memory_hotplug_init() will do the same,
then later we could add cpu hotplug memory region over there.
Perhaps we could use bus-less device plug code path,
in that case memory_region_init_io()/qdev_init_gpio_out_named()
should be moved to ged_initfn() and mapping part into specialized helper
(similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb)
callback during completing device realize stage, it would be something like:
virt.c:
virt_machine_device_plug_cb()
if dev == GED_TYPE
machine_ged_plug_helper(system_memory)
generic_event_device.c:
machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized
connect_irq()
memory_region_add_subregion(as, ged->ged_base, &ged->io)
if ged->memory-hotplug-support
memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io)
in this case addresses could be normally hard-codded in board code if device is not optional
(as in patch 6/9: create_acpi_ged() )
or potentially they could come from CLI as -device parameters
(/me thinking about building blocks that allow to create machine from config)
sysbus device might be fine as shortcut if we are thinking about
only creating device during machine_init (although I have a reservations towards
sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th
region at some address)).
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support
2019-08-05 15:46 ` Igor Mammedov
@ 2019-08-05 15:54 ` Peter Maydell
2019-08-06 9:47 ` Igor Mammedov
0 siblings, 1 reply; 26+ messages in thread
From: Peter Maydell @ 2019-08-05 15:54 UTC (permalink / raw)
To: Igor Mammedov
Cc: sameo, ard.biesheuvel, qemu-devel, Shameerali Kolothum Thodi,
Linuxarm, shannon.zhaosl, qemu-arm, xuwei (O),
Paolo Bonzini, sebastien.boeuf, lersek, eric.auger
On Mon, 5 Aug 2019 at 16:47, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 5 Aug 2019 14:42:38 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> > This is definitely a bad idea -- devices should not add their
> > own memory regions to the system memory MR. They should
> > expose their MRs (by being a sysbus-device) and let the board
> > code do the wiring up of the MRs into the right memory space
> > at the right address.
>
> it's not the only place in GED that is trying to add to system address
> space, optionally if called acpi_memory_hotplug_init() will do the same,
> then later we could add cpu hotplug memory region over there.
>
> Perhaps we could use bus-less device plug code path,
> in that case memory_region_init_io()/qdev_init_gpio_out_named()
> should be moved to ged_initfn() and mapping part into specialized helper
> (similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb)
> callback during completing device realize stage, it would be something like:
>
> virt.c:
> virt_machine_device_plug_cb()
> if dev == GED_TYPE
> machine_ged_plug_helper(system_memory)
>
> generic_event_device.c:
> machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized
> connect_irq()
> memory_region_add_subregion(as, ged->ged_base, &ged->io)
> if ged->memory-hotplug-support
> memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io)
I don't really understand why we want to do this complicated
thing, rather than just doing the normal thing for devices
that live at particular addresses, ie make them sysbus devices
and have the board map their memory regions in the right place.
> in this case addresses could be normally hard-codded in board code if device is not optional
> (as in patch 6/9: create_acpi_ged() )
> or potentially they could come from CLI as -device parameters
> (/me thinking about building blocks that allow to create machine from config)
I don't think we want to do this. The user should not have to
know anything about addresses or have to specify them on the
command line. (This is why you can't create sysbus devices
with -device except for some odd special cases to do with passthrough
of hardware.)
> sysbus device might be fine as shortcut if we are thinking about
> only creating device during machine_init (although I have a reservations towards
> sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th
> region at some address)).
Not sure entirely what you have in mind here? (though yes, the
sysbus device API has its awkward corners, some of which are
just down to how old it is.)
thanks
-- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support
2019-08-05 15:54 ` Peter Maydell
@ 2019-08-06 9:47 ` Igor Mammedov
0 siblings, 0 replies; 26+ messages in thread
From: Igor Mammedov @ 2019-08-06 9:47 UTC (permalink / raw)
To: Peter Maydell
Cc: sameo, ard.biesheuvel, qemu-devel, Shameerali Kolothum Thodi,
Linuxarm, shannon.zhaosl, qemu-arm, xuwei (O),
Paolo Bonzini, sebastien.boeuf, lersek, eric.auger
On Mon, 5 Aug 2019 16:54:06 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:
> On Mon, 5 Aug 2019 at 16:47, Igor Mammedov <imammedo@redhat.com> wrote:
> > On Mon, 5 Aug 2019 14:42:38 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> > > This is definitely a bad idea -- devices should not add their
> > > own memory regions to the system memory MR. They should
> > > expose their MRs (by being a sysbus-device) and let the board
> > > code do the wiring up of the MRs into the right memory space
> > > at the right address.
> >
> > it's not the only place in GED that is trying to add to system address
> > space, optionally if called acpi_memory_hotplug_init() will do the same,
> > then later we could add cpu hotplug memory region over there.
> >
> > Perhaps we could use bus-less device plug code path,
> > in that case memory_region_init_io()/qdev_init_gpio_out_named()
> > should be moved to ged_initfn() and mapping part into specialized helper
> > (similar to pc_dimm_plug() ) that's called by board (from virt_machine_device_plug_cb)
> > callback during completing device realize stage, it would be something like:
> >
> > virt.c:
> > virt_machine_device_plug_cb()
> > if dev == GED_TYPE
> > machine_ged_plug_helper(system_memory)
> >
> > generic_event_device.c:
> > machine_ged_plug_helper(as, irq) // similar to sysbus_mmio_map() but ged specialized
> > connect_irq()
> > memory_region_add_subregion(as, ged->ged_base, &ged->io)
> > if ged->memory-hotplug-support
> > memory_region_add_subregion(as, ged->memhp_base , &ged->memhp_state.memhp_io)
>
> I don't really understand why we want to do this complicated
> thing, rather than just doing the normal thing for devices
> that live at particular addresses, ie make them sysbus devices
> and have the board map their memory regions in the right place.
hotplug path is basically the same as sysbus the only difference is
that it uses machine's (pre_)plug handler to wire up devices and more
flexible than sysbus.
> > in this case addresses could be normally hard-codded in board code if device is not optional
> > (as in patch 6/9: create_acpi_ged() )
> > or potentially they could come from CLI as -device parameters
> > (/me thinking about building blocks that allow to create machine from config)
>
> I don't think we want to do this. The user should not have to
> know anything about addresses or have to specify them on the
> command line. (This is why you can't create sysbus devices
> with -device except for some odd special cases to do with passthrough
> of hardware.)
>
> > sysbus device might be fine as shortcut if we are thinking about
> > only creating device during machine_init (although I have a reservations towards
> > sysbus interface (ex: caller of sysbus_mmio_map() has no clue when mapping N-th
> > region at some address)).
>
> Not sure entirely what you have in mind here? (though yes, the
> sysbus device API has its awkward corners, some of which are
> just down to how old it is.)
since it's a fixed device I don't mind using sysbus either,
lets do it this way.
> thanks
> -- PMM
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot Shameer Kolothum
@ 2019-08-06 13:08 ` Igor Mammedov
2019-08-07 8:19 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 26+ messages in thread
From: Igor Mammedov @ 2019-08-06 13:08 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, sameo, ard.biesheuvel, shannon.zhaosl, qemu-devel,
xuwei5, linuxarm, eric.auger, qemu-arm, sebastien.boeuf, lersek
On Fri, 26 Jul 2019 11:45:16 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> This initializes the GED device with base memory and irq, configures
> ged memory hotplug event and builds the corresponding aml code. With
> this, both hot and cold plug of device memory is enabled now for Guest
> with ACPI boot.
>
> Memory cold plug support with Guest DT boot is not yet supported.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
> -Changed no_acpi_dev to no_ged.
> -Fixed 'dev' reference leak by object_new().
> -Updated bios-tables-test-allowed-diff.h to avoid "make check"
> failure.
>
> ---
> hw/arm/Kconfig | 2 +
> hw/arm/virt-acpi-build.c | 14 +++++++
> hw/arm/virt.c | 54 +++++++++++++++++++++++----
> include/hw/arm/virt.h | 4 ++
> tests/bios-tables-test-allowed-diff.h | 1 +
> 5 files changed, 68 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 84961c17ab..ad7f7c089b 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -22,6 +22,8 @@ config ARM_VIRT
> select ACPI_PCI
> select MEM_DEVICE
> select DIMM
> + select ACPI_MEMORY_HOTPLUG
> + select ACPI_HW_REDUCED
>
> config CHEETAH
> bool
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0afb372769..018b1e326d 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -40,6 +40,8 @@
> #include "hw/acpi/aml-build.h"
> #include "hw/acpi/utils.h"
> #include "hw/acpi/pci.h"
> +#include "hw/acpi/memory_hotplug.h"
> +#include "hw/acpi/generic_event_device.h"
> #include "hw/pci/pcie_host.h"
> #include "hw/pci/pci.h"
> #include "hw/arm/virt.h"
> @@ -705,6 +707,7 @@ static void
> build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> {
> Aml *scope, *dsdt;
> + MachineState *ms = MACHINE(vms);
> const MemMapEntry *memmap = vms->memmap;
> const int *irqmap = vms->irqmap;
>
> @@ -729,6 +732,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> vms->highmem, vms->highmem_ecam);
> acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
> (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
> + if (vms->acpi_dev) {
> + build_ged_aml(scope, "\\_SB."GED_DEVICE,
> + HOTPLUG_HANDLER(vms->acpi_dev),
> + irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY);
> + }
> +
> + if (vms->acpi_dev && ms->ram_slots) {
> + build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB", NULL,
> + AML_SYSTEM_MEMORY);
> + }
> +
> acpi_dsdt_add_power_button(scope);
>
> aml_append(dsdt, scope);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index bbe156dc35..41386a6eb7 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -66,6 +66,7 @@
> #include "target/arm/internals.h"
> #include "hw/mem/pc-dimm.h"
> #include "hw/mem/nvdimm.h"
> +#include "hw/acpi/generic_event_device.h"
>
> #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -136,6 +137,8 @@ static const MemMapEntry base_memmap[] = {
> [VIRT_GPIO] = { 0x09030000, 0x00001000 },
> [VIRT_SECURE_UART] = { 0x09040000, 0x00001000 },
> [VIRT_SMMU] = { 0x09050000, 0x00020000 },
> + [VIRT_PCDIMM_ACPI] = { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
> + [VIRT_ACPI_GED] = { 0x09080000, ACPI_GED_EVT_SEL_LEN },
> [VIRT_MMIO] = { 0x0a000000, 0x00000200 },
> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
> [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
> @@ -171,6 +174,7 @@ static const int a15irqmap[] = {
> [VIRT_PCIE] = 3, /* ... to 6 */
> [VIRT_GPIO] = 7,
> [VIRT_SECURE_UART] = 8,
> + [VIRT_ACPI_GED] = 9,
> [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
> [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
> [VIRT_SMMU] = 74, /* ...to 74 + NUM_SMMU_IRQS - 1 */
> @@ -520,6 +524,27 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> }
> }
>
> +static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
> +{
> + DeviceState *dev;
> + int irq = vms->irqmap[VIRT_ACPI_GED];
> + uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> +
> + dev = DEVICE(object_new(TYPE_ACPI_GED));
> + qdev_prop_set_uint64(dev, "memhp-base",
> + vms->memmap[VIRT_PCDIMM_ACPI].base);
> + qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
> + qdev_prop_set_uint32(dev, "ged-event", event);
> + object_property_add_child(qdev_get_machine(), "acpi-ged",
> + OBJECT(dev), NULL);
> + qdev_init_nofail(dev);
> + qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> +
> + object_unref(OBJECT(dev));
> +
> + return dev;
> +}
this function will need changes to accommodate for sysbus device
init sequence [3/9].
> +
> static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> {
> const char *itsclass = its_class_name();
> @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState *machine)
> MemoryRegion *ram = g_new(MemoryRegion, 1);
> bool firmware_loaded;
> bool aarch64 = true;
> + bool has_ged = !vmc->no_ged;
> unsigned int smp_cpus = machine->smp.cpus;
> unsigned int max_cpus = machine->smp.max_cpus;
>
> @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState *machine)
>
> create_gpio(vms, pic);
>
> + if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> + vms->acpi_dev = create_acpi_ged(vms, pic);
> + }
> +
> /* Create mmio transports, so the user can create virtio backends
> * (which will be automatically plugged in to the transports). If
> * no backend is created the transport will just sit harmlessly idle.
> @@ -1876,27 +1906,34 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
> static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> Error **errp)
> {
> + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> + const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>
> - /*
> - * The device memory is not yet exposed to the Guest either through
> - * DT or ACPI and hence both cold/hot plug of memory is explicitly
> - * disabled for now.
> - */
> - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> - error_setg(errp, "memory cold/hot plug is not yet supported");
> + if (is_nvdimm) {
> + error_setg(errp, "nvdimm is not yet supported");
> return;
> }
>
> + if (!vms->acpi_dev) {
> + error_setg(errp, "memory hotplug is not enabled: missing acpi device");
> + return;
> + }
> +
> + hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, errp);
use local_error and check for error condition here. see pc_memory_pre_plug()
> +
> pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
> }
>
> static void virt_memory_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev, Error **errp)
> {
> + HotplugHandlerClass *hhc;
> VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>
> pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
^^^^
>
> + hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> + hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
^^^^
why errors are ignored here, pls check for errors and propagate
them to the caller.
> }
>
> static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
>
> static void virt_machine_4_1_options(MachineClass *mc)
> {
> + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
> virt_machine_4_2_options(mc);
> compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
> + vmc->no_ged = true;
> }
> DEFINE_VIRT_MACHINE(4, 1)
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index a72094204e..577ee49b4b 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -77,6 +77,8 @@ enum {
> VIRT_GPIO,
> VIRT_SECURE_UART,
> VIRT_SECURE_MEM,
> + VIRT_PCDIMM_ACPI,
> + VIRT_ACPI_GED,
> VIRT_LOWMEMMAP_LAST,
> };
>
> @@ -106,6 +108,7 @@ typedef struct {
> bool claim_edge_triggered_timers;
> bool smbios_old_sys_ver;
> bool no_highmem_ecam;
> + bool no_ged; /* Machines < 4.2 has no support for ACPI GED device */
> } VirtMachineClass;
>
> typedef struct {
> @@ -133,6 +136,7 @@ typedef struct {
> uint32_t iommu_phandle;
> int psci_conduit;
> hwaddr highest_gpa;
> + DeviceState *acpi_dev;
> } VirtMachineState;
>
> #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
> index dfb8523c8b..7b4adbc822 100644
> --- a/tests/bios-tables-test-allowed-diff.h
> +++ b/tests/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
> /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/virt/DSDT",
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT Shameer Kolothum
@ 2019-08-06 13:21 ` Igor Mammedov
2019-08-09 16:02 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 26+ messages in thread
From: Igor Mammedov @ 2019-08-06 13:21 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, sameo, ard.biesheuvel, shannon.zhaosl, qemu-devel,
xuwei5, linuxarm, eric.auger, qemu-arm, sebastien.boeuf, lersek
On Fri, 26 Jul 2019 11:45:17 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> Generate Memory Affinity Structures for PC-DIMM ranges.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/arm/virt-acpi-build.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 018b1e326d..75657caa36 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -518,6 +518,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> int i, srat_start;
> uint64_t mem_base;
> MachineClass *mc = MACHINE_GET_CLASS(vms);
> + MachineState *ms = MACHINE(vms);
> const CPUArchIdList *cpu_list = mc->possible_cpu_arch_ids(MACHINE(vms));
>
> srat_start = table_data->len;
> @@ -543,6 +544,14 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> }
> }
>
> + if (ms->device_memory) {
> + numamem = acpi_data_push(table_data, sizeof *numamem);
> + build_srat_memory(numamem, ms->device_memory->base,
> + memory_region_size(&ms->device_memory->mr),
> + nb_numa_nodes - 1,
> + MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
> + }
> +
> build_header(linker, table_data, (void *)(table_data->data + srat_start),
> "SRAT", table_data->len - srat_start, 3, NULL, NULL);
> }
missing entry in
tests/bios-tables-test-allowed-diff.h
PS:
I don't really know what ARM guest kernel expects but on x86 we had to enable numa
for guest to figure out max_possible_pfn
(see: in linux.git: 8dd330300197 / ec941c5ffede).
It's worth to check if we might need a patch for turning on NUMA
(how to do it in QEMU see: auto_enable_numa_with_memhp)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 9/9] hw/arm: Use GED for system_powerdown event
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 9/9] hw/arm: Use GED for system_powerdown event Shameer Kolothum
@ 2019-08-06 13:38 ` Igor Mammedov
0 siblings, 0 replies; 26+ messages in thread
From: Igor Mammedov @ 2019-08-06 13:38 UTC (permalink / raw)
To: Shameer Kolothum
Cc: peter.maydell, sameo, ard.biesheuvel, shannon.zhaosl, qemu-devel,
xuwei5, linuxarm, eric.auger, qemu-arm, sebastien.boeuf, lersek
On Fri, 26 Jul 2019 11:45:19 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> For machines 4.2 or higher with ACPI boot use GED for system_powerdown
> event instead of GPIO. Guest boot with DT still uses GPIO.
one more thing,
though GED is sort of described in ACPI spec, MMIO event mapping
is a totally QEMU invention.
so we probably should describe its ABI in our own spec, maybe in
docs/specs/acpi_hw_reduced_hotplug.txt
see docs/specs/acpi_foo for examples.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> v7 --> v8
> -Retained gpio based system_powerdown support for machines < 4.2.
> -Reuse of virt_powerdown_req() for ACPI GED use.
> -Dropped Eric's R-by for now because of above.
>
> ---
> hw/acpi/generic_event_device.c | 2 ++
> hw/arm/virt-acpi-build.c | 6 +++---
> hw/arm/virt.c | 27 ++++++++++++++++-----------
> include/hw/acpi/acpi_dev_interface.h | 1 +
> include/hw/arm/virt.h | 1 +
> 5 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 0b3214eff4..f00b0ab14b 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -205,6 +205,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>
> if (ev & ACPI_MEMORY_HOTPLUG_STATUS) {
> sel = ACPI_GED_MEM_HOTPLUG_EVT;
> + } else if (ev & ACPI_POWER_DOWN_STATUS) {
> + sel = ACPI_GED_PWR_DOWN_EVT;
> } else {
> /* Unknown event. Return without generating interrupt. */
> warn_report("GED: Unsupported event %d. No irq injected", ev);
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 75657caa36..31bf2bf775 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -49,7 +49,6 @@
> #include "kvm_arm.h"
>
> #define ARM_SPI_BASE 32
> -#define ACPI_POWER_BUTTON_DEVICE "PWRB"
look like this one belong to the previous patch.
Maybe we should merge both patches together?
> static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> {
> @@ -739,12 +738,13 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
> 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));
> if (vms->acpi_dev) {
> build_ged_aml(scope, "\\_SB."GED_DEVICE,
> HOTPLUG_HANDLER(vms->acpi_dev),
> irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY);
> + } else {
> + acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
> + (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
> }
>
> if (vms->acpi_dev && ms->ram_slots) {
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 41386a6eb7..73a758d9a9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -528,7 +528,7 @@ static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
> {
> DeviceState *dev;
> int irq = vms->irqmap[VIRT_ACPI_GED];
> - uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> + uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT | ACPI_GED_PWR_DOWN_EVT;
>
> dev = DEVICE(object_new(TYPE_ACPI_GED));
> qdev_prop_set_uint64(dev, "memhp-base",
> @@ -784,13 +784,15 @@ static void create_rtc(const VirtMachineState *vms, qemu_irq *pic)
> static DeviceState *gpio_key_dev;
> static void virt_powerdown_req(Notifier *n, void *opaque)
> {
> - /* use gpio Pin 3 for power button event */
> - qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
> -}
> + VirtMachineState *s = container_of(n, VirtMachineState, powerdown_notifier);
>
> -static Notifier virt_system_powerdown_notifier = {
> - .notify = virt_powerdown_req
> -};
> + if (s->acpi_dev) {
> + acpi_send_event(s->acpi_dev, ACPI_POWER_DOWN_STATUS);
> + } else {
> + /* use gpio Pin 3 for power button event */
> + qemu_set_irq(qdev_get_gpio_in(gpio_key_dev, 0), 1);
> + }
> +}
>
> static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
> {
> @@ -833,9 +835,6 @@ static void create_gpio(const VirtMachineState *vms, qemu_irq *pic)
> qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
> "gpios", phandle, 3, 0);
>
> - /* connect powerdown request */
> - qemu_register_powerdown_notifier(&virt_system_powerdown_notifier);
> -
> g_free(nodename);
> }
>
> @@ -1721,12 +1720,18 @@ static void machvirt_init(MachineState *machine)
>
> create_pcie(vms, pic);
>
> - create_gpio(vms, pic);
>
> if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> vms->acpi_dev = create_acpi_ged(vms, pic);
> + } else {
> + create_gpio(vms, pic);
> }
>
> + /* connect powerdown request */
> + vms->powerdown_notifier.notify = virt_powerdown_req;
> + qemu_register_powerdown_notifier(&vms->powerdown_notifier);
> +
> +
stray wight space?
I'd move factoring out powerdown_notifier from create_gpio into
a separate patch that precedes this one.
> /* Create mmio transports, so the user can create virtio backends
> * (which will be automatically plugged in to the transports). If
> * no backend is created the transport will just sit harmlessly idle.
> diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
> index 43ff119179..adcb3a816c 100644
> --- a/include/hw/acpi/acpi_dev_interface.h
> +++ b/include/hw/acpi/acpi_dev_interface.h
> @@ -11,6 +11,7 @@ typedef enum {
> ACPI_MEMORY_HOTPLUG_STATUS = 8,
> ACPI_NVDIMM_HOTPLUG_STATUS = 16,
> ACPI_VMGENID_CHANGE_STATUS = 32,
> + ACPI_POWER_DOWN_STATUS = 64,
> } AcpiEventStatusBits;
>
> #define TYPE_ACPI_DEVICE_IF "acpi-device-interface"
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 577ee49b4b..0b41083e9d 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -137,6 +137,7 @@ typedef struct {
> int psci_conduit;
> hwaddr highest_gpa;
> DeviceState *acpi_dev;
> + Notifier powerdown_notifier;
> } VirtMachineState;
>
> #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
2019-08-06 13:08 ` Igor Mammedov
@ 2019-08-07 8:19 ` Shameerali Kolothum Thodi
2019-08-07 9:15 ` Igor Mammedov
0 siblings, 1 reply; 26+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-08-07 8:19 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, sameo, ard.biesheuvel, shannon.zhaosl, qemu-devel,
Linuxarm, eric.auger, qemu-arm, xuwei (O),
sebastien.boeuf, lersek
Hi Igor,
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 06 August 2019 14:09
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; sameo@linux.intel.com;
> ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> sebastien.boeuf@intel.com; lersek@redhat.com
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device
> memory cold/hot plug with ACPI boot
[...]
> > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,
> qemu_irq *pic)
> > +{
> > + DeviceState *dev;
> > + int irq = vms->irqmap[VIRT_ACPI_GED];
> > + uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > +
> > + dev = DEVICE(object_new(TYPE_ACPI_GED));
> > + qdev_prop_set_uint64(dev, "memhp-base",
> > + vms->memmap[VIRT_PCDIMM_ACPI].base);
> > + qdev_prop_set_uint64(dev, "ged-base",
> vms->memmap[VIRT_ACPI_GED].base);
> > + qdev_prop_set_uint32(dev, "ged-event", event);
> > + object_property_add_child(qdev_get_machine(), "acpi-ged",
> > + OBJECT(dev), NULL);
> > + qdev_init_nofail(dev);
> > + qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> > +
> > + object_unref(OBJECT(dev));
> > +
> > + return dev;
> > +}
>
> this function will need changes to accommodate for sysbus device
> init sequence [3/9].
Yes. I think we are proposing to use sysbus_mmio_map() here for "ged-base".
But what about " memhp-base"? Is it ok to invoke
acpi_memory_hotplug_init(get_system_memoty(), ...) from ged device?
Or go with _set_link() function to pass the address space ?
Thanks,
Shameer
> > +
> > static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> > {
> > const char *itsclass = its_class_name();
> > @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState *machine)
> > MemoryRegion *ram = g_new(MemoryRegion, 1);
> > bool firmware_loaded;
> > bool aarch64 = true;
> > + bool has_ged = !vmc->no_ged;
> > unsigned int smp_cpus = machine->smp.cpus;
> > unsigned int max_cpus = machine->smp.max_cpus;
> >
> > @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState
> *machine)
> >
> > create_gpio(vms, pic);
> >
> > + if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> > + vms->acpi_dev = create_acpi_ged(vms, pic);
> > + }
> > +
> > /* Create mmio transports, so the user can create virtio backends
> > * (which will be automatically plugged in to the transports). If
> > * no backend is created the transport will just sit harmlessly idle.
> > @@ -1876,27 +1906,34 @@ static const CPUArchIdList
> *virt_possible_cpu_arch_ids(MachineState *ms)
> > static void virt_memory_pre_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> > Error **errp)
> > {
> > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > + const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),
> TYPE_NVDIMM);
> >
> > - /*
> > - * The device memory is not yet exposed to the Guest either through
> > - * DT or ACPI and hence both cold/hot plug of memory is explicitly
> > - * disabled for now.
> > - */
> > - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > - error_setg(errp, "memory cold/hot plug is not yet supported");
> > + if (is_nvdimm) {
> > + error_setg(errp, "nvdimm is not yet supported");
> > return;
> > }
> >
> > + if (!vms->acpi_dev) {
> > + error_setg(errp, "memory hotplug is not enabled: missing acpi
> device");
> > + return;
> > + }
> > +
> > + hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev), dev,
> errp);
> use local_error and check for error condition here. see pc_memory_pre_plug()
>
> > +
> > pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL,
> errp);
> > }
> >
> > static void virt_memory_plug(HotplugHandler *hotplug_dev,
> > DeviceState *dev, Error **errp)
> > {
> > + HotplugHandlerClass *hhc;
> > VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >
> > pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
> ^^^^
> >
> > + hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> > + hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
> ^^^^
> why errors are ignored here, pls check for errors and propagate
> them to the caller.
>
> > }
> >
> > static void virt_machine_device_pre_plug_cb(HotplugHandler
> *hotplug_dev,
> > @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> >
> > static void virt_machine_4_1_options(MachineClass *mc)
> > {
> > + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > +
> > virt_machine_4_2_options(mc);
> > compat_props_add(mc->compat_props, hw_compat_4_1,
> hw_compat_4_1_len);
> > + vmc->no_ged = true;
> > }
> > DEFINE_VIRT_MACHINE(4, 1)
> >
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index a72094204e..577ee49b4b 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -77,6 +77,8 @@ enum {
> > VIRT_GPIO,
> > VIRT_SECURE_UART,
> > VIRT_SECURE_MEM,
> > + VIRT_PCDIMM_ACPI,
> > + VIRT_ACPI_GED,
> > VIRT_LOWMEMMAP_LAST,
> > };
> >
> > @@ -106,6 +108,7 @@ typedef struct {
> > bool claim_edge_triggered_timers;
> > bool smbios_old_sys_ver;
> > bool no_highmem_ecam;
> > + bool no_ged; /* Machines < 4.2 has no support for ACPI GED device
> */
> > } VirtMachineClass;
> >
> > typedef struct {
> > @@ -133,6 +136,7 @@ typedef struct {
> > uint32_t iommu_phandle;
> > int psci_conduit;
> > hwaddr highest_gpa;
> > + DeviceState *acpi_dev;
> > } VirtMachineState;
> >
> > #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> VIRT_PCIE_ECAM)
> > diff --git a/tests/bios-tables-test-allowed-diff.h
> b/tests/bios-tables-test-allowed-diff.h
> > index dfb8523c8b..7b4adbc822 100644
> > --- a/tests/bios-tables-test-allowed-diff.h
> > +++ b/tests/bios-tables-test-allowed-diff.h
> > @@ -1 +1,2 @@
> > /* List of comma-separated changed AML files to ignore */
> > +"tests/data/acpi/virt/DSDT",
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
2019-08-07 8:19 ` Shameerali Kolothum Thodi
@ 2019-08-07 9:15 ` Igor Mammedov
2019-08-07 14:00 ` Shameerali Kolothum Thodi
0 siblings, 1 reply; 26+ messages in thread
From: Igor Mammedov @ 2019-08-07 9:15 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: peter.maydell, sameo, ard.biesheuvel, shannon.zhaosl, qemu-devel,
Linuxarm, eric.auger, qemu-arm, xuwei (O),
sebastien.boeuf, lersek
On Wed, 7 Aug 2019 08:19:16 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> Hi Igor,
>
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 06 August 2019 14:09
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org; sameo@linux.intel.com;
> > ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> > <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> > sebastien.boeuf@intel.com; lersek@redhat.com
> > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device
> > memory cold/hot plug with ACPI boot
>
> [...]
>
> > > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,
> > qemu_irq *pic)
> > > +{
> > > + DeviceState *dev;
> > > + int irq = vms->irqmap[VIRT_ACPI_GED];
> > > + uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > > +
> > > + dev = DEVICE(object_new(TYPE_ACPI_GED));
> > > + qdev_prop_set_uint64(dev, "memhp-base",
> > > + vms->memmap[VIRT_PCDIMM_ACPI].base);
> > > + qdev_prop_set_uint64(dev, "ged-base",
> > vms->memmap[VIRT_ACPI_GED].base);
> > > + qdev_prop_set_uint32(dev, "ged-event", event);
> > > + object_property_add_child(qdev_get_machine(), "acpi-ged",
> > > + OBJECT(dev), NULL);
> > > + qdev_init_nofail(dev);
> > > + qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> > > +
> > > + object_unref(OBJECT(dev));
> > > +
> > > + return dev;
> > > +}
> >
> > this function will need changes to accommodate for sysbus device
> > init sequence [3/9].
>
> Yes. I think we are proposing to use sysbus_mmio_map() here for "ged-base".
> But what about " memhp-base"? Is it ok to invoke
> acpi_memory_hotplug_init(get_system_memoty(), ...) from ged device?
no it's not ok.
One could expose container memory region as sysbus mmio and then put
ged-io and AcpiGedState::memhp_state::io within it.
something like:
board:
sysbus_mmio_map(ged, 0 /* io_contaner number */, ged-base)
ged_initfn()
register io_container as sysbus mmio region
ged_realize()
memory_region_add_subregion(&ged->io_container, 0, &ged_st->io);
acpi_memory_hotplug_init(&ged->io_container,, &ged->acpi_memory_hotplug, AFTER_GED_IO_OFFSET)
that would make GED's MMIO available to guest at ged-base and memhp IO
will be available at address after it.
You can go even further (more flexible) and register ged_st->io as separate
sysbus mmio and use a container exclusively for memhp, in this case you'd be
able to map memhp io from board independently from ged-base.
> Or go with _set_link() function to pass the address space ?
>
> Thanks,
> Shameer
>
>
> > > +
> > > static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> > > {
> > > const char *itsclass = its_class_name();
> > > @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState *machine)
> > > MemoryRegion *ram = g_new(MemoryRegion, 1);
> > > bool firmware_loaded;
> > > bool aarch64 = true;
> > > + bool has_ged = !vmc->no_ged;
> > > unsigned int smp_cpus = machine->smp.cpus;
> > > unsigned int max_cpus = machine->smp.max_cpus;
> > >
> > > @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState
> > *machine)
> > >
> > > create_gpio(vms, pic);
> > >
> > > + if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> > > + vms->acpi_dev = create_acpi_ged(vms, pic);
> > > + }
> > > +
> > > /* Create mmio transports, so the user can create virtio backends
> > > * (which will be automatically plugged in to the transports). If
> > > * no backend is created the transport will just sit harmlessly idle.
> > > @@ -1876,27 +1906,34 @@ static const CPUArchIdList
> > *virt_possible_cpu_arch_ids(MachineState *ms)
> > > static void virt_memory_pre_plug(HotplugHandler *hotplug_dev,
> > DeviceState *dev,
> > > Error **errp)
> > > {
> > > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > + const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),
> > TYPE_NVDIMM);
> > >
> > > - /*
> > > - * The device memory is not yet exposed to the Guest either through
> > > - * DT or ACPI and hence both cold/hot plug of memory is explicitly
> > > - * disabled for now.
> > > - */
> > > - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > - error_setg(errp, "memory cold/hot plug is not yet supported");
> > > + if (is_nvdimm) {
> > > + error_setg(errp, "nvdimm is not yet supported");
> > > return;
> > > }
> > >
> > > + if (!vms->acpi_dev) {
> > > + error_setg(errp, "memory hotplug is not enabled: missing acpi
> > device");
> > > + return;
> > > + }
> > > +
> > > + hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev), dev,
> > errp);
> > use local_error and check for error condition here. see pc_memory_pre_plug()
> >
> > > +
> > > pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL,
> > errp);
> > > }
> > >
> > > static void virt_memory_plug(HotplugHandler *hotplug_dev,
> > > DeviceState *dev, Error **errp)
> > > {
> > > + HotplugHandlerClass *hhc;
> > > VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > >
> > > pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
> > ^^^^
> > >
> > > + hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> > > + hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
> > ^^^^
> > why errors are ignored here, pls check for errors and propagate
> > them to the caller.
> >
> > > }
> > >
> > > static void virt_machine_device_pre_plug_cb(HotplugHandler
> > *hotplug_dev,
> > > @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> > >
> > > static void virt_machine_4_1_options(MachineClass *mc)
> > > {
> > > + VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > +
> > > virt_machine_4_2_options(mc);
> > > compat_props_add(mc->compat_props, hw_compat_4_1,
> > hw_compat_4_1_len);
> > > + vmc->no_ged = true;
> > > }
> > > DEFINE_VIRT_MACHINE(4, 1)
> > >
> > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > index a72094204e..577ee49b4b 100644
> > > --- a/include/hw/arm/virt.h
> > > +++ b/include/hw/arm/virt.h
> > > @@ -77,6 +77,8 @@ enum {
> > > VIRT_GPIO,
> > > VIRT_SECURE_UART,
> > > VIRT_SECURE_MEM,
> > > + VIRT_PCDIMM_ACPI,
> > > + VIRT_ACPI_GED,
> > > VIRT_LOWMEMMAP_LAST,
> > > };
> > >
> > > @@ -106,6 +108,7 @@ typedef struct {
> > > bool claim_edge_triggered_timers;
> > > bool smbios_old_sys_ver;
> > > bool no_highmem_ecam;
> > > + bool no_ged; /* Machines < 4.2 has no support for ACPI GED device
> > */
> > > } VirtMachineClass;
> > >
> > > typedef struct {
> > > @@ -133,6 +136,7 @@ typedef struct {
> > > uint32_t iommu_phandle;
> > > int psci_conduit;
> > > hwaddr highest_gpa;
> > > + DeviceState *acpi_dev;
> > > } VirtMachineState;
> > >
> > > #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > VIRT_PCIE_ECAM)
> > > diff --git a/tests/bios-tables-test-allowed-diff.h
> > b/tests/bios-tables-test-allowed-diff.h
> > > index dfb8523c8b..7b4adbc822 100644
> > > --- a/tests/bios-tables-test-allowed-diff.h
> > > +++ b/tests/bios-tables-test-allowed-diff.h
> > > @@ -1 +1,2 @@
> > > /* List of comma-separated changed AML files to ignore */
> > > +"tests/data/acpi/virt/DSDT",
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
2019-08-07 9:15 ` Igor Mammedov
@ 2019-08-07 14:00 ` Shameerali Kolothum Thodi
2019-08-07 15:04 ` Igor Mammedov
0 siblings, 1 reply; 26+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-08-07 14:00 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, sameo, ard.biesheuvel, shannon.zhaosl, qemu-devel,
Linuxarm, eric.auger, qemu-arm, xuwei (O),
sebastien.boeuf, lersek
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 07 August 2019 10:15
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; sameo@linux.intel.com;
> ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> sebastien.boeuf@intel.com; lersek@redhat.com
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device
> memory cold/hot plug with ACPI boot
>
> On Wed, 7 Aug 2019 08:19:16 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > Hi Igor,
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > Sent: 06 August 2019 14:09
> > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > > eric.auger@redhat.com; peter.maydell@linaro.org;
> sameo@linux.intel.com;
> > > ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> > > <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> > > sebastien.boeuf@intel.com; lersek@redhat.com
> > > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable
> device
> > > memory cold/hot plug with ACPI boot
> >
> > [...]
> >
> > > > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,
> > > qemu_irq *pic)
> > > > +{
> > > > + DeviceState *dev;
> > > > + int irq = vms->irqmap[VIRT_ACPI_GED];
> > > > + uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > > > +
> > > > + dev = DEVICE(object_new(TYPE_ACPI_GED));
> > > > + qdev_prop_set_uint64(dev, "memhp-base",
> > > > +
> vms->memmap[VIRT_PCDIMM_ACPI].base);
> > > > + qdev_prop_set_uint64(dev, "ged-base",
> > > vms->memmap[VIRT_ACPI_GED].base);
> > > > + qdev_prop_set_uint32(dev, "ged-event", event);
> > > > + object_property_add_child(qdev_get_machine(), "acpi-ged",
> > > > + OBJECT(dev), NULL);
> > > > + qdev_init_nofail(dev);
> > > > + qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> > > > +
> > > > + object_unref(OBJECT(dev));
> > > > +
> > > > + return dev;
> > > > +}
> > >
> > > this function will need changes to accommodate for sysbus device
> > > init sequence [3/9].
> >
> > Yes. I think we are proposing to use sysbus_mmio_map() here for "ged-base".
> > But what about " memhp-base"? Is it ok to invoke
> > acpi_memory_hotplug_init(get_system_memoty(), ...) from ged device?
> no it's not ok.
>
> One could expose container memory region as sysbus mmio and then put
> ged-io and AcpiGedState::memhp_state::io within it.
> something like:
>
> board:
> sysbus_mmio_map(ged, 0 /* io_contaner number */, ged-base)
>
> ged_initfn()
> register io_container as sysbus mmio region
>
> ged_realize()
> memory_region_add_subregion(&ged->io_container, 0,
> &ged_st->io);
> acpi_memory_hotplug_init(&ged->io_container,,
> &ged->acpi_memory_hotplug, AFTER_GED_IO_OFFSET)
>
> that would make GED's MMIO available to guest at ged-base and memhp IO
> will be available at address after it.
> You can go even further (more flexible) and register ged_st->io as separate
> sysbus mmio and use a container exclusively for memhp, in this case you'd be
> able to map memhp io from board independently from ged-base.
Ok. Understood. Thanks.
But looks like both the approaches would require changes to build_memory_hotplug_aml()
code as acpi_memory_hotplug_init() stores the io_base and reuse that in _aml() code.
I will have a go and see.
Thanks,
Shameer
>
> > Or go with _set_link() function to pass the address space ?
> >
> > Thanks,
> > Shameer
> >
> >
> > > > +
> > > > static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> > > > {
> > > > const char *itsclass = its_class_name();
> > > > @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState
> *machine)
> > > > MemoryRegion *ram = g_new(MemoryRegion, 1);
> > > > bool firmware_loaded;
> > > > bool aarch64 = true;
> > > > + bool has_ged = !vmc->no_ged;
> > > > unsigned int smp_cpus = machine->smp.cpus;
> > > > unsigned int max_cpus = machine->smp.max_cpus;
> > > >
> > > > @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState
> > > *machine)
> > > >
> > > > create_gpio(vms, pic);
> > > >
> > > > + if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> > > > + vms->acpi_dev = create_acpi_ged(vms, pic);
> > > > + }
> > > > +
> > > > /* Create mmio transports, so the user can create virtio backends
> > > > * (which will be automatically plugged in to the transports). If
> > > > * no backend is created the transport will just sit harmlessly idle.
> > > > @@ -1876,27 +1906,34 @@ static const CPUArchIdList
> > > *virt_possible_cpu_arch_ids(MachineState *ms)
> > > > static void virt_memory_pre_plug(HotplugHandler *hotplug_dev,
> > > DeviceState *dev,
> > > > Error **errp)
> > > > {
> > > > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > > + const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),
> > > TYPE_NVDIMM);
> > > >
> > > > - /*
> > > > - * The device memory is not yet exposed to the Guest either
> through
> > > > - * DT or ACPI and hence both cold/hot plug of memory is explicitly
> > > > - * disabled for now.
> > > > - */
> > > > - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > > - error_setg(errp, "memory cold/hot plug is not yet supported");
> > > > + if (is_nvdimm) {
> > > > + error_setg(errp, "nvdimm is not yet supported");
> > > > return;
> > > > }
> > > >
> > > > + if (!vms->acpi_dev) {
> > > > + error_setg(errp, "memory hotplug is not enabled: missing acpi
> > > device");
> > > > + return;
> > > > + }
> > > > +
> > > > + hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev),
> dev,
> > > errp);
> > > use local_error and check for error condition here. see
> pc_memory_pre_plug()
> > >
> > > > +
> > > > pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
> NULL,
> > > errp);
> > > > }
> > > >
> > > > static void virt_memory_plug(HotplugHandler *hotplug_dev,
> > > > DeviceState *dev, Error **errp)
> > > > {
> > > > + HotplugHandlerClass *hhc;
> > > > VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > >
> > > > pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
> > > ^^^^
> > > >
> > > > + hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> > > > + hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
> > > ^^^^
> > > why errors are ignored here, pls check for errors and propagate
> > > them to the caller.
> > >
> > > > }
> > > >
> > > > static void virt_machine_device_pre_plug_cb(HotplugHandler
> > > *hotplug_dev,
> > > > @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> > > >
> > > > static void virt_machine_4_1_options(MachineClass *mc)
> > > > {
> > > > + VirtMachineClass *vmc =
> VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > > +
> > > > virt_machine_4_2_options(mc);
> > > > compat_props_add(mc->compat_props, hw_compat_4_1,
> > > hw_compat_4_1_len);
> > > > + vmc->no_ged = true;
> > > > }
> > > > DEFINE_VIRT_MACHINE(4, 1)
> > > >
> > > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > > index a72094204e..577ee49b4b 100644
> > > > --- a/include/hw/arm/virt.h
> > > > +++ b/include/hw/arm/virt.h
> > > > @@ -77,6 +77,8 @@ enum {
> > > > VIRT_GPIO,
> > > > VIRT_SECURE_UART,
> > > > VIRT_SECURE_MEM,
> > > > + VIRT_PCDIMM_ACPI,
> > > > + VIRT_ACPI_GED,
> > > > VIRT_LOWMEMMAP_LAST,
> > > > };
> > > >
> > > > @@ -106,6 +108,7 @@ typedef struct {
> > > > bool claim_edge_triggered_timers;
> > > > bool smbios_old_sys_ver;
> > > > bool no_highmem_ecam;
> > > > + bool no_ged; /* Machines < 4.2 has no support for ACPI GED
> device
> > > */
> > > > } VirtMachineClass;
> > > >
> > > > typedef struct {
> > > > @@ -133,6 +136,7 @@ typedef struct {
> > > > uint32_t iommu_phandle;
> > > > int psci_conduit;
> > > > hwaddr highest_gpa;
> > > > + DeviceState *acpi_dev;
> > > > } VirtMachineState;
> > > >
> > > > #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > > VIRT_PCIE_ECAM)
> > > > diff --git a/tests/bios-tables-test-allowed-diff.h
> > > b/tests/bios-tables-test-allowed-diff.h
> > > > index dfb8523c8b..7b4adbc822 100644
> > > > --- a/tests/bios-tables-test-allowed-diff.h
> > > > +++ b/tests/bios-tables-test-allowed-diff.h
> > > > @@ -1 +1,2 @@
> > > > /* List of comma-separated changed AML files to ignore */
> > > > +"tests/data/acpi/virt/DSDT",
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
2019-08-07 14:00 ` Shameerali Kolothum Thodi
@ 2019-08-07 15:04 ` Igor Mammedov
0 siblings, 0 replies; 26+ messages in thread
From: Igor Mammedov @ 2019-08-07 15:04 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: peter.maydell, sameo, ard.biesheuvel, shannon.zhaosl, qemu-devel,
Linuxarm, eric.auger, qemu-arm, xuwei (O),
sebastien.boeuf, lersek
On Wed, 7 Aug 2019 14:00:44 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 07 August 2019 10:15
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org; sameo@linux.intel.com;
> > ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> > <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> > sebastien.boeuf@intel.com; lersek@redhat.com
> > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device
> > memory cold/hot plug with ACPI boot
> >
> > On Wed, 7 Aug 2019 08:19:16 +0000
> > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > > Hi Igor,
> > >
> > > > -----Original Message-----
> > > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > > Sent: 06 August 2019 14:09
> > > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > > > eric.auger@redhat.com; peter.maydell@linaro.org;
> > sameo@linux.intel.com;
> > > > ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> > > > <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> > > > sebastien.boeuf@intel.com; lersek@redhat.com
> > > > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable
> > device
> > > > memory cold/hot plug with ACPI boot
> > >
> > > [...]
> > >
> > > > > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,
> > > > qemu_irq *pic)
> > > > > +{
> > > > > + DeviceState *dev;
> > > > > + int irq = vms->irqmap[VIRT_ACPI_GED];
> > > > > + uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > > > > +
> > > > > + dev = DEVICE(object_new(TYPE_ACPI_GED));
> > > > > + qdev_prop_set_uint64(dev, "memhp-base",
> > > > > +
> > vms->memmap[VIRT_PCDIMM_ACPI].base);
> > > > > + qdev_prop_set_uint64(dev, "ged-base",
> > > > vms->memmap[VIRT_ACPI_GED].base);
> > > > > + qdev_prop_set_uint32(dev, "ged-event", event);
> > > > > + object_property_add_child(qdev_get_machine(), "acpi-ged",
> > > > > + OBJECT(dev), NULL);
> > > > > + qdev_init_nofail(dev);
> > > > > + qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> > > > > +
> > > > > + object_unref(OBJECT(dev));
> > > > > +
> > > > > + return dev;
> > > > > +}
> > > >
> > > > this function will need changes to accommodate for sysbus device
> > > > init sequence [3/9].
> > >
> > > Yes. I think we are proposing to use sysbus_mmio_map() here for "ged-base".
> > > But what about " memhp-base"? Is it ok to invoke
> > > acpi_memory_hotplug_init(get_system_memoty(), ...) from ged device?
> > no it's not ok.
> >
> > One could expose container memory region as sysbus mmio and then put
> > ged-io and AcpiGedState::memhp_state::io within it.
> > something like:
> >
> > board:
> > sysbus_mmio_map(ged, 0 /* io_contaner number */, ged-base)
> >
> > ged_initfn()
> > register io_container as sysbus mmio region
> >
> > ged_realize()
> > memory_region_add_subregion(&ged->io_container, 0,
> > &ged_st->io);
> > acpi_memory_hotplug_init(&ged->io_container,,
> > &ged->acpi_memory_hotplug, AFTER_GED_IO_OFFSET)
> >
> > that would make GED's MMIO available to guest at ged-base and memhp IO
> > will be available at address after it.
> > You can go even further (more flexible) and register ged_st->io as separate
> > sysbus mmio and use a container exclusively for memhp, in this case you'd be
> > able to map memhp io from board independently from ged-base.
>
> Ok. Understood. Thanks.
>
> But looks like both the approaches would require changes to build_memory_hotplug_aml()
> code as acpi_memory_hotplug_init() stores the io_base and reuse that in _aml() code.
I'd just pass io_base as an argument to build_memory_hotplug_aml().
For x86 we can add a field to PCMachine to provide address for pc or q35
and just use constant in arm/virt case.
>
> I will have a go and see.
>
> Thanks,
> Shameer
>
> >
> > > Or go with _set_link() function to pass the address space ?
> > >
> > > Thanks,
> > > Shameer
> > >
> > >
> > > > > +
> > > > > static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> > > > > {
> > > > > const char *itsclass = its_class_name();
> > > > > @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState
> > *machine)
> > > > > MemoryRegion *ram = g_new(MemoryRegion, 1);
> > > > > bool firmware_loaded;
> > > > > bool aarch64 = true;
> > > > > + bool has_ged = !vmc->no_ged;
> > > > > unsigned int smp_cpus = machine->smp.cpus;
> > > > > unsigned int max_cpus = machine->smp.max_cpus;
> > > > >
> > > > > @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState
> > > > *machine)
> > > > >
> > > > > create_gpio(vms, pic);
> > > > >
> > > > > + if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> > > > > + vms->acpi_dev = create_acpi_ged(vms, pic);
> > > > > + }
> > > > > +
> > > > > /* Create mmio transports, so the user can create virtio backends
> > > > > * (which will be automatically plugged in to the transports). If
> > > > > * no backend is created the transport will just sit harmlessly idle.
> > > > > @@ -1876,27 +1906,34 @@ static const CPUArchIdList
> > > > *virt_possible_cpu_arch_ids(MachineState *ms)
> > > > > static void virt_memory_pre_plug(HotplugHandler *hotplug_dev,
> > > > DeviceState *dev,
> > > > > Error **errp)
> > > > > {
> > > > > + VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > > > + const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),
> > > > TYPE_NVDIMM);
> > > > >
> > > > > - /*
> > > > > - * The device memory is not yet exposed to the Guest either
> > through
> > > > > - * DT or ACPI and hence both cold/hot plug of memory is explicitly
> > > > > - * disabled for now.
> > > > > - */
> > > > > - if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > > > - error_setg(errp, "memory cold/hot plug is not yet supported");
> > > > > + if (is_nvdimm) {
> > > > > + error_setg(errp, "nvdimm is not yet supported");
> > > > > return;
> > > > > }
> > > > >
> > > > > + if (!vms->acpi_dev) {
> > > > > + error_setg(errp, "memory hotplug is not enabled: missing acpi
> > > > device");
> > > > > + return;
> > > > > + }
> > > > > +
> > > > > + hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev),
> > dev,
> > > > errp);
> > > > use local_error and check for error condition here. see
> > pc_memory_pre_plug()
> > > >
> > > > > +
> > > > > pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
> > NULL,
> > > > errp);
> > > > > }
> > > > >
> > > > > static void virt_memory_plug(HotplugHandler *hotplug_dev,
> > > > > DeviceState *dev, Error **errp)
> > > > > {
> > > > > + HotplugHandlerClass *hhc;
> > > > > VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > > >
> > > > > pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
> > > > ^^^^
> > > > >
> > > > > + hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> > > > > + hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
> > > > ^^^^
> > > > why errors are ignored here, pls check for errors and propagate
> > > > them to the caller.
> > > >
> > > > > }
> > > > >
> > > > > static void virt_machine_device_pre_plug_cb(HotplugHandler
> > > > *hotplug_dev,
> > > > > @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> > > > >
> > > > > static void virt_machine_4_1_options(MachineClass *mc)
> > > > > {
> > > > > + VirtMachineClass *vmc =
> > VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > > > +
> > > > > virt_machine_4_2_options(mc);
> > > > > compat_props_add(mc->compat_props, hw_compat_4_1,
> > > > hw_compat_4_1_len);
> > > > > + vmc->no_ged = true;
> > > > > }
> > > > > DEFINE_VIRT_MACHINE(4, 1)
> > > > >
> > > > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > > > index a72094204e..577ee49b4b 100644
> > > > > --- a/include/hw/arm/virt.h
> > > > > +++ b/include/hw/arm/virt.h
> > > > > @@ -77,6 +77,8 @@ enum {
> > > > > VIRT_GPIO,
> > > > > VIRT_SECURE_UART,
> > > > > VIRT_SECURE_MEM,
> > > > > + VIRT_PCDIMM_ACPI,
> > > > > + VIRT_ACPI_GED,
> > > > > VIRT_LOWMEMMAP_LAST,
> > > > > };
> > > > >
> > > > > @@ -106,6 +108,7 @@ typedef struct {
> > > > > bool claim_edge_triggered_timers;
> > > > > bool smbios_old_sys_ver;
> > > > > bool no_highmem_ecam;
> > > > > + bool no_ged; /* Machines < 4.2 has no support for ACPI GED
> > device
> > > > */
> > > > > } VirtMachineClass;
> > > > >
> > > > > typedef struct {
> > > > > @@ -133,6 +136,7 @@ typedef struct {
> > > > > uint32_t iommu_phandle;
> > > > > int psci_conduit;
> > > > > hwaddr highest_gpa;
> > > > > + DeviceState *acpi_dev;
> > > > > } VirtMachineState;
> > > > >
> > > > > #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > > > VIRT_PCIE_ECAM)
> > > > > diff --git a/tests/bios-tables-test-allowed-diff.h
> > > > b/tests/bios-tables-test-allowed-diff.h
> > > > > index dfb8523c8b..7b4adbc822 100644
> > > > > --- a/tests/bios-tables-test-allowed-diff.h
> > > > > +++ b/tests/bios-tables-test-allowed-diff.h
> > > > > @@ -1 +1,2 @@
> > > > > /* List of comma-separated changed AML files to ignore */
> > > > > +"tests/data/acpi/virt/DSDT",
> > >
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
2019-08-06 13:21 ` Igor Mammedov
@ 2019-08-09 16:02 ` Shameerali Kolothum Thodi
2019-08-12 13:47 ` Igor Mammedov
0 siblings, 1 reply; 26+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-08-09 16:02 UTC (permalink / raw)
To: Igor Mammedov
Cc: peter.maydell, sameo, ard.biesheuvel, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
sebastien.boeuf, lersek, eric.auger
Hi Igor,
> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 06 August 2019 14:22
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> ard.biesheuvel@linaro.org; shannon.zhaosl@gmail.com;
> qemu-devel@nongnu.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm
> <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> sebastien.boeuf@intel.com; lersek@redhat.com
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add
> PC-DIMM in SRAT
>
> On Fri, 26 Jul 2019 11:45:17 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
>
> > Generate Memory Affinity Structures for PC-DIMM ranges.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > hw/arm/virt-acpi-build.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 018b1e326d..75657caa36 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -518,6 +518,7 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> > int i, srat_start;
> > uint64_t mem_base;
> > MachineClass *mc = MACHINE_GET_CLASS(vms);
> > + MachineState *ms = MACHINE(vms);
> > const CPUArchIdList *cpu_list =
> mc->possible_cpu_arch_ids(MACHINE(vms));
> >
> > srat_start = table_data->len;
> > @@ -543,6 +544,14 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> > }
> > }
> >
> > + if (ms->device_memory) {
> > + numamem = acpi_data_push(table_data, sizeof *numamem);
> > + build_srat_memory(numamem, ms->device_memory->base,
> > +
> memory_region_size(&ms->device_memory->mr),
> > + nb_numa_nodes - 1,
> > + MEM_AFFINITY_HOTPLUGGABLE |
> MEM_AFFINITY_ENABLED);
> > + }
> > +
> > build_header(linker, table_data, (void *)(table_data->data +
> srat_start),
> > "SRAT", table_data->len - srat_start, 3, NULL, NULL);
> > }
>
> missing entry in
> tests/bios-tables-test-allowed-diff.h
I can't find any SRAT file in tests/data/acpi/virt. Arm/virt doesn't have much
tests in bios-tables-test.c. So does it make any difference?
> PS:
> I don't really know what ARM guest kernel expects but on x86 we had to enable
> numa
> for guest to figure out max_possible_pfn
> (see: in linux.git: 8dd330300197 / ec941c5ffede).
From whatever I can find, doesn't look like there is any special handling of
max_possible_pfn in ARM64 world. The variable seems to be only updated
in acpi_numa_memory_affinity_init()
https://elixir.bootlin.com/linux/v5.3-rc3/source/drivers/acpi/numa.c#L298
Is there any way to test this in Guest to see whether this is actually a problem?
Thanks,
Shameer
> It's worth to check if we might need a patch for turning on NUMA
> (how to do it in QEMU see: auto_enable_numa_with_memhp)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
2019-08-09 16:02 ` Shameerali Kolothum Thodi
@ 2019-08-12 13:47 ` Igor Mammedov
0 siblings, 0 replies; 26+ messages in thread
From: Igor Mammedov @ 2019-08-12 13:47 UTC (permalink / raw)
To: Shameerali Kolothum Thodi
Cc: peter.maydell, sameo, ard.biesheuvel, qemu-devel, Linuxarm,
shannon.zhaosl, qemu-arm, xuwei (O),
sebastien.boeuf, lersek, eric.auger
On Fri, 9 Aug 2019 16:02:39 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> Hi Igor,
>
> > -----Original Message-----
> > From: Qemu-devel
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 06 August 2019 14:22
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> > ard.biesheuvel@linaro.org; shannon.zhaosl@gmail.com;
> > qemu-devel@nongnu.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm
> > <linuxarm@huawei.com>; eric.auger@redhat.com; qemu-arm@nongnu.org;
> > sebastien.boeuf@intel.com; lersek@redhat.com
> > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add
> > PC-DIMM in SRAT
> >
> > On Fri, 26 Jul 2019 11:45:17 +0100
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > > Generate Memory Affinity Structures for PC-DIMM ranges.
> > >
> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > hw/arm/virt-acpi-build.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 018b1e326d..75657caa36 100644
> > > --- a/hw/arm/virt-acpi-build.c
> > > +++ b/hw/arm/virt-acpi-build.c
> > > @@ -518,6 +518,7 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> > > int i, srat_start;
> > > uint64_t mem_base;
> > > MachineClass *mc = MACHINE_GET_CLASS(vms);
> > > + MachineState *ms = MACHINE(vms);
> > > const CPUArchIdList *cpu_list =
> > mc->possible_cpu_arch_ids(MACHINE(vms));
> > >
> > > srat_start = table_data->len;
> > > @@ -543,6 +544,14 @@ build_srat(GArray *table_data, BIOSLinker *linker,
> > VirtMachineState *vms)
> > > }
> > > }
> > >
> > > + if (ms->device_memory) {
> > > + numamem = acpi_data_push(table_data, sizeof *numamem);
> > > + build_srat_memory(numamem, ms->device_memory->base,
> > > +
> > memory_region_size(&ms->device_memory->mr),
> > > + nb_numa_nodes - 1,
> > > + MEM_AFFINITY_HOTPLUGGABLE |
> > MEM_AFFINITY_ENABLED);
> > > + }
> > > +
> > > build_header(linker, table_data, (void *)(table_data->data +
> > srat_start),
> > > "SRAT", table_data->len - srat_start, 3, NULL, NULL);
> > > }
> >
> > missing entry in
> > tests/bios-tables-test-allowed-diff.h
>
> I can't find any SRAT file in tests/data/acpi/virt. Arm/virt doesn't have much
> tests in bios-tables-test.c. So does it make any difference?
acpi tests for arm/virt are new and are enabled only since 4.1,
now it should be trivial to add extra cases for code you are adding.
Since you're touching her SRAT, I'd suggest to enable 'numamem' and 'memhp'
tests with this series (for example see: test_acpi_piix4_tcg_numamem/test_acpi_piix4_tcg_memhp).
> > PS:
> > I don't really know what ARM guest kernel expects but on x86 we had to enable
> > numa
> > for guest to figure out max_possible_pfn
> > (see: in linux.git: 8dd330300197 / ec941c5ffede).
>
> From whatever I can find, doesn't look like there is any special handling of
> max_possible_pfn in ARM64 world. The variable seems to be only updated
> in acpi_numa_memory_affinity_init()
>
> https://elixir.bootlin.com/linux/v5.3-rc3/source/drivers/acpi/numa.c#L298
problem was that drivers (stub dma ops) (guest booted with RAM below 4Gb)
were breaking when they received RAM buffers above 4Gb. To fix it we needed
to turn on swiotlb if possible max PFN could be above 4Gb.
That's where SRAT played its role to let guest know what possible max PFN
could be.
> Is there any way to test this in Guest to see whether this is actually a problem?
from my x86 experience:
1. for linux:
* start guest with RAM that not goes over 4Gb PFN mark (for example with -m 1Gb)
and native drivers (not virtio ones see linux.git commit message ec941c5ffede4)
* hotplug RAM to go over 4Gb boundary
* stress test drivers (that should trigger various issues)
(on x64 it were ATA and various usb drivers leading to data corruption and not
working mouse in guests)
2. for Windows guests memory hotplug doesn't work at all unless NUMA is enabled.
Based on above I'd assume, we need to turn on numa for ARM as well if
memhp is enabled since SRAT is the only way of describing max possible RAM end
to the guest OS.
> Thanks,
> Shameer
>
> > It's worth to check if we might need a patch for turning on NUMA
> > (how to do it in QEMU see: auto_enable_numa_with_memhp)
>
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2019-08-12 13:47 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-26 10:45 [Qemu-devel] [PATCH-for-4.2 v8 0/9] ARM virt: ACPI memory hotplug support Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 1/9] hw/acpi: Make ACPI IO address space configurable Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 2/9] hw/acpi: Do not create memory hotplug method when handler is not defined Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 3/9] hw/acpi: Add ACPI Generic Event Device Support Shameer Kolothum
2019-07-30 15:25 ` Igor Mammedov
2019-08-01 8:36 ` Shameerali Kolothum Thodi
2019-08-05 13:30 ` Igor Mammedov
2019-08-05 13:42 ` Peter Maydell
2019-08-05 15:46 ` Igor Mammedov
2019-08-05 15:54 ` Peter Maydell
2019-08-06 9:47 ` Igor Mammedov
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 4/9] hw/arm/virt: Add memory hotplug framework Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 5/9] hw/arm/virt: Add 4.2 machine type Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot Shameer Kolothum
2019-08-06 13:08 ` Igor Mammedov
2019-08-07 8:19 ` Shameerali Kolothum Thodi
2019-08-07 9:15 ` Igor Mammedov
2019-08-07 14:00 ` Shameerali Kolothum Thodi
2019-08-07 15:04 ` Igor Mammedov
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 7/9] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT Shameer Kolothum
2019-08-06 13:21 ` Igor Mammedov
2019-08-09 16:02 ` Shameerali Kolothum Thodi
2019-08-12 13:47 ` Igor Mammedov
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 8/9] hw/acpi: Add system power down support to GED Shameer Kolothum
2019-07-26 10:45 ` [Qemu-devel] [PATCH-for-4.2 v8 9/9] hw/arm: Use GED for system_powerdown event Shameer Kolothum
2019-08-06 13:38 ` Igor Mammedov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).