All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support
@ 2019-07-16 15:38 Shameer Kolothum
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 01/10] hw/acpi: Make ACPI IO address space configurable Shameer Kolothum
                   ` (9 more replies)
  0 siblings, 10 replies; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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.

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

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 (2):
  hw/arm/virt: Add memory hotplug framework
  tests: Update DSDT ACPI table for arm/virt board with PCDIMM related
    changes

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         | 352 +++++++++++++++++++++++++
 hw/acpi/memory_hotplug.c               |  35 +--
 hw/arm/Kconfig                         |   4 +
 hw/arm/virt-acpi-build.c               |  58 ++--
 hw/arm/virt.c                          |  99 ++++++-
 hw/core/machine.c                      |   3 +
 hw/i386/acpi-build.c                   |   3 +-
 include/hw/acpi/generic_event_device.h | 106 ++++++++
 include/hw/acpi/memory_hotplug.h       |   9 +-
 include/hw/arm/virt.h                  |   4 +
 include/hw/boards.h                    |   3 +
 tests/data/acpi/virt/DSDT              | Bin 18476 -> 18493 bytes
 14 files changed, 625 insertions(+), 56 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] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 01/10] hw/acpi: Make ACPI IO address space configurable
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 02/10] hw/acpi: Do not create memory hotplug method when handler is not defined Shameer Kolothum
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 02/10] hw/acpi: Do not create memory hotplug method when handler is not defined
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 01/10] hw/acpi: Make ACPI IO address space configurable Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support Shameer Kolothum
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 01/10] hw/acpi: Make ACPI IO address space configurable Shameer Kolothum
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 02/10] hw/acpi: Do not create memory hotplug method when handler is not defined Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-17 14:32   ` Igor Mammedov
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 04/10] hw/arm/virt: Add memory hotplug framework Shameer Kolothum
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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>
---
 hw/acpi/Kconfig                        |   4 +
 hw/acpi/Makefile.objs                  |   1 +
 hw/acpi/generic_event_device.c         | 336 +++++++++++++++++++++++++
 include/hw/acpi/generic_event_device.h | 102 ++++++++
 4 files changed, 443 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..2a83d68e82
--- /dev/null
+++ b/hw/acpi/generic_event_device.c
@@ -0,0 +1,336 @@
+/*
+ *
+ * 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 */
+        qemu_mutex_lock(&ged_st->lock);
+        val = ged_st->sel;
+        ged_st->sel = 0;
+        qemu_mutex_unlock(&ged_st->lock);
+        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_event(AcpiGedState *s, uint32_t sel)
+{
+    GEDState *ged_st = &s->ged_state;
+    /*
+     * Set the GED IRQ selector to the expected device type value. This
+     * way, the ACPI method will be able to trigger the right code based
+     * on a unique IRQ.
+     */
+    qemu_mutex_lock(&ged_st->lock);
+    ged_st->sel |= sel;
+    qemu_mutex_unlock(&ged_st->lock);
+
+    /* Trigger the event by sending an interrupt to the guest. */
+    qemu_irq_pulse(s->irq);
+}
+
+static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st)
+{
+    AcpiGedState *s = ACPI_GED(dev);
+
+    assert(s->ged_base);
+
+    qemu_mutex_init(&ged_st->lock);
+    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
+                          TYPE_ACPI_GED, ACPI_GED_REG_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);
+    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;
+    }
+
+    /*
+     * We inject the hotplug interrupt. The IRQ selector will make
+     * the difference from the ACPI table.
+     */
+    acpi_ged_event(s, sel);
+}
+
+static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
+{
+    AcpiGedState *s = ACPI_GED(dev);
+
+    if (s->memhp_state.is_enabled) {
+        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
+                                 &s->memhp_state,
+                                 s->memhp_base);
+    }
+
+    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
+}
+
+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..f69d084c89
--- /dev/null
+++ b/include/hw/acpi/generic_event_device.h
@@ -0,0 +1,102 @@
+/*
+ *
+ * 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 ACPI_GED_REG_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;
+    QemuMutex    lock;
+} 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] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 04/10] hw/arm/virt: Add memory hotplug framework
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
                   ` (2 preceding siblings ...)
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-17 14:40   ` Igor Mammedov
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 05/10] hw/arm/virt: Add 4.2 machine type Shameer Kolothum
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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>
---
 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] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 05/10] hw/arm/virt: Add 4.2 machine type
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
                   ` (3 preceding siblings ...)
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 04/10] hw/arm/virt: Add memory hotplug framework Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-17 14:51   ` Igor Mammedov
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 06/10] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot Shameer Kolothum
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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>
---
 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] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 06/10] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
                   ` (4 preceding siblings ...)
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 05/10] hw/arm/virt: Add 4.2 machine type Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-18 12:41   ` Igor Mammedov
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 07/10] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT Shameer Kolothum
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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>
---
v6 --> v7
  Disabled ACPI GED dev creation for machine < 4.2.

---
 hw/arm/Kconfig           |  2 ++
 hw/arm/virt-acpi-build.c | 14 +++++++++++
 hw/arm/virt.c            | 51 ++++++++++++++++++++++++++++++++++------
 include/hw/arm/virt.h    |  4 ++++
 4 files changed, 64 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..ffcccc05ad 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_REG_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,25 @@ 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]);
+
+    return dev;
+}
+
 static void create_its(VirtMachineState *vms, DeviceState *gicdev)
 {
     const char *itsclass = its_class_name();
@@ -1697,6 +1720,10 @@ static void machvirt_init(MachineState *machine)
 
     create_gpio(vms, pic);
 
+    if (!vmc->no_acpi_dev && 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 +1903,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 +2136,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_acpi_dev = true;
 }
 DEFINE_VIRT_MACHINE(4, 1)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index a72094204e..32892af1dd 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_acpi_dev;
 } 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)
-- 
2.17.1




^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 07/10] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
                   ` (5 preceding siblings ...)
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 06/10] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 08/10] hw/acpi: Add system power down support to GED Shameer Kolothum
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 08/10] hw/acpi: Add system power down support to GED
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
                   ` (6 preceding siblings ...)
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 07/10] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-18 13:07   ` Igor Mammedov
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event Shameer Kolothum
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes Shameer Kolothum
  9 siblings, 1 reply; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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         | 16 ++++++++++++++++
 include/hw/acpi/generic_event_device.h |  4 ++++
 2 files changed, 20 insertions(+)

diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 2a83d68e82..2bd786b7ab 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[]
@@ -232,6 +238,13 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
     acpi_ged_event(s, sel);
 }
 
+static void acpi_ged_pm_powerdown_req(Notifier *n, void *opaque)
+{
+    AcpiGedState *s = container_of(n, AcpiGedState, powerdown_notifier);
+
+    acpi_ged_event(s, ACPI_GED_PWR_DOWN_EVT);
+}
+
 static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
 {
     AcpiGedState *s = ACPI_GED(dev);
@@ -243,6 +256,9 @@ static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
     }
 
     acpi_ged_init(get_system_memory(), dev, &s->ged_state);
+
+    s->powerdown_notifier.notify = acpi_ged_pm_powerdown_req;
+    qemu_register_powerdown_notifier(&s->powerdown_notifier);
 }
 
 static Property acpi_ged_properties[] = {
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index f69d084c89..9e63b72cb9 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)
@@ -79,6 +81,7 @@
  * through GED.
  */
 #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
+#define ACPI_GED_PWR_DOWN_EVT      0x2
 
 typedef struct GEDState {
     MemoryRegion io;
@@ -88,6 +91,7 @@ typedef struct GEDState {
 
 typedef struct AcpiGedState {
     DeviceClass parent_obj;
+    Notifier powerdown_notifier;
     MemHotplugState memhp_state;
     hwaddr memhp_base;
     hwaddr ged_base;
-- 
2.17.1




^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
                   ` (7 preceding siblings ...)
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 08/10] hw/acpi: Add system power down support to GED Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-18 13:03   ` Igor Mammedov
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes Shameer Kolothum
  9 siblings, 1 reply; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 UTC (permalink / raw)
  To: qemu-devel, qemu-arm, eric.auger, imammedo
  Cc: peter.maydell, sameo, ard.biesheuvel, linuxarm, xuwei5,
	shannon.zhaosl, sebastien.boeuf, lersek

Use GED for system_powerdown event instead of GPIO for ACPI.
Guest boot with DT still uses GPIO.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt-acpi-build.c | 37 +------------------------------------
 hw/arm/virt.c            |  6 +++---
 2 files changed, 4 insertions(+), 39 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 75657caa36..9178ca8e40 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)
 {
@@ -328,37 +327,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
     aml_append(scope, dev);
 }
 
-static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
-                                           uint32_t gpio_irq)
-{
-    Aml *dev = aml_device("GPO0");
-    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
-    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
-    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
-
-    Aml *crs = aml_resource_template();
-    aml_append(crs, aml_memory32_fixed(gpio_memmap->base, gpio_memmap->size,
-                                       AML_READ_WRITE));
-    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
-                                  AML_EXCLUSIVE, &gpio_irq, 1));
-    aml_append(dev, aml_name_decl("_CRS", crs));
-
-    Aml *aei = aml_resource_template();
-    /* Pin 3 for power button */
-    const uint32_t pin_list[1] = {3};
-    aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
-                                 AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
-                                 "GPO0", NULL, 0));
-    aml_append(dev, aml_name_decl("_AEI", aei));
-
-    /* _E03 is handle for power button */
-    Aml *method = aml_method("_E03", 0, AML_NOTSERIALIZED);
-    aml_append(method, aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
-                                  aml_int(0x80)));
-    aml_append(dev, method);
-    aml_append(scope, dev);
-}
-
 static void acpi_dsdt_add_power_button(Aml *scope)
 {
     Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
@@ -739,9 +707,8 @@ 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) {
+        acpi_dsdt_add_power_button(scope);
         build_ged_aml(scope, "\\_SB."GED_DEVICE,
                       HOTPLUG_HANDLER(vms->acpi_dev),
                       irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY);
@@ -752,8 +719,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                                  AML_SYSTEM_MEMORY);
     }
 
-    acpi_dsdt_add_power_button(scope);
-
     aml_append(dsdt, scope);
 
     /* copy AML table into ACPI tables blob and patch header there */
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ffcccc05ad..6e55c25c5f 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",
@@ -1718,10 +1718,10 @@ static void machvirt_init(MachineState *machine)
 
     create_pcie(vms, pic);
 
-    create_gpio(vms, pic);
-
     if (!vmc->no_acpi_dev && aarch64 && firmware_loaded && acpi_enabled) {
         vms->acpi_dev = create_acpi_ged(vms, pic);
+    } else {
+        create_gpio(vms, pic);
     }
 
     /* Create mmio transports, so the user can create virtio backends
-- 
2.17.1




^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes
  2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
                   ` (8 preceding siblings ...)
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event Shameer Kolothum
@ 2019-07-16 15:38 ` Shameer Kolothum
  2019-07-18 13:13   ` Igor Mammedov
  9 siblings, 1 reply; 25+ messages in thread
From: Shameer Kolothum @ 2019-07-16 15:38 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>

PCDIMM hotplug addition updated the DSDT. Update the reference table.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
---
 tests/data/acpi/virt/DSDT | Bin 18476 -> 18493 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
index 20e85c7f89f645c69935c615c07084e221419960..1a5c3e5243c78eb25345fdf7ffa442afec3c5c28 100644
GIT binary patch
delta 172
zcmZ28fpPByMlP3Nmk?VI1_q{Y6S<_BIvO{s=eSSi^^mWRZt~=a(Tfjuig$N)iRXy-
z@O0sIbPn(|FfcS`;D`?ea)A;+mUB?BARiZZCmRDZqb36bLnT9WgKLnhI|B~`0|zG;
zOLQYQi0kYc?CPT+ViNBf7Q$Ho;v_5(n_Qj1$iNt*&kVB1*CW8$H`v*4fdEjLhl`1U
Ni>WOjJjjWw0RY`tDw+TQ

delta 134
zcmdlxfpN_QMlP3Nmk=Ef1_q{ziCof5?F}2%bKIpo*xdvC4LIUGJY9GlgM2*<49pA}
zIN}{$f+og`c`$P_2rw{ovN13+sxvS!RDxt&Jq4Ay6ndl?7@-g-A<n=c$sh-0Ff)LR
Z11c2ZjdwLLW@rlt4|3vam^{&45den68LI#Q

-- 
2.17.1




^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support Shameer Kolothum
@ 2019-07-17 14:32   ` Igor Mammedov
  2019-07-18 10:52     ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2019-07-17 14:32 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, sameo, shannon.zhaosl, ard.biesheuvel, qemu-devel,
	xuwei5, linuxarm, eric.auger, qemu-arm, sebastien.boeuf, lersek

On Tue, 16 Jul 2019 16:38:09 +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.
> 
> 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>
> ---
>  hw/acpi/Kconfig                        |   4 +
>  hw/acpi/Makefile.objs                  |   1 +
>  hw/acpi/generic_event_device.c         | 336 +++++++++++++++++++++++++
>  include/hw/acpi/generic_event_device.h | 102 ++++++++
>  4 files changed, 443 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..2a83d68e82
> --- /dev/null
> +++ b/hw/acpi/generic_event_device.c
> @@ -0,0 +1,336 @@
> +/*
> + *
> + * 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 */
> +        qemu_mutex_lock(&ged_st->lock);
> +        val = ged_st->sel;
> +        ged_st->sel = 0;
> +        qemu_mutex_unlock(&ged_st->lock);
> +        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_event(AcpiGedState *s, uint32_t sel)
> +{
> +    GEDState *ged_st = &s->ged_state;
> +    /*
> +     * Set the GED IRQ selector to the expected device type value. This
> +     * way, the ACPI method will be able to trigger the right code based
> +     * on a unique IRQ.
comment isn't correct anymore, pls fix it

> +     */
> +    qemu_mutex_lock(&ged_st->lock);
Is this lock really necessary?
(I thought that MMIO and monitor access is guarded by BQL)

> +    ged_st->sel |= sel;
> +    qemu_mutex_unlock(&ged_st->lock);
> +
> +    /* Trigger the event by sending an interrupt to the guest. */
> +    qemu_irq_pulse(s->irq);
> +}
> +
> +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState *ged_st)
> +{
> +    AcpiGedState *s = ACPI_GED(dev);
> +
> +    assert(s->ged_base);
> +
> +    qemu_mutex_init(&ged_st->lock);
> +    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
> +                          TYPE_ACPI_GED, ACPI_GED_REG_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);
> +    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;
> +    }
> +
> +    /*
> +     * We inject the hotplug interrupt. The IRQ selector will make
> +     * the difference from the ACPI table.
I don't get comment at all, pls rephrase/

> +     */
> +    acpi_ged_event(s, sel);
it seems to used only once and only here, suggest to drop acpi_ged_event()
and move it's code here.

> +}
> +
> +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> +{
> +    AcpiGedState *s = ACPI_GED(dev);
> +
> +    if (s->memhp_state.is_enabled) {
> +        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
> +                                 &s->memhp_state,
> +                                 s->memhp_base);
> +    }
> +
> +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> +}
> +
> +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),

I'd add checks to realize() to ensure that both foo_base are not left
uninitialized (i.e. at default values)

> +    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..f69d084c89
> --- /dev/null
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -0,0 +1,102 @@
> +/*
> + *
> + * 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 ACPI_GED_REG_LEN           0x4
Didn't both macros refer to the same thing?
Why not use the same one in AML and in QEMU MMIO handler?


> +
> +#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;
> +    QemuMutex    lock;
> +} 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



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 04/10] hw/arm/virt: Add memory hotplug framework
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 04/10] hw/arm/virt: Add memory hotplug framework Shameer Kolothum
@ 2019-07-17 14:40   ` Igor Mammedov
  0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2019-07-17 14:40 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 Tue, 16 Jul 2019 16:38:10 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> 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;
>  }
>  



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 05/10] hw/arm/virt: Add 4.2 machine type
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 05/10] hw/arm/virt: Add 4.2 machine type Shameer Kolothum
@ 2019-07-17 14:51   ` Igor Mammedov
  0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2019-07-17 14:51 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, sameo, shannon.zhaosl, ard.biesheuvel, qemu-devel,
	xuwei5, linuxarm, eric.auger, qemu-arm, sebastien.boeuf, lersek

On Tue, 16 Jul 2019 16:38:11 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> 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;
>  



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support
  2019-07-17 14:32   ` Igor Mammedov
@ 2019-07-18 10:52     ` Shameerali Kolothum Thodi
  2019-07-18 12:30       ` Igor Mammedov
  0 siblings, 1 reply; 25+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-07-18 10:52 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: 17 July 2019 15:33
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> shannon.zhaosl@gmail.com; ard.biesheuvel@linaro.org;
> 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 v7 03/10] hw/acpi: Add ACPI Generic
> Event Device Support
> 
> On Tue, 16 Jul 2019 16:38:09 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

[...]

> > +static void acpi_ged_event(AcpiGedState *s, uint32_t sel)
> > +{
> > +    GEDState *ged_st = &s->ged_state;
> > +    /*
> > +     * Set the GED IRQ selector to the expected device type value. This
> > +     * way, the ACPI method will be able to trigger the right code based
> > +     * on a unique IRQ.
> comment isn't correct anymore, pls fix it

True.

> 
> > +     */
> > +    qemu_mutex_lock(&ged_st->lock);
> Is this lock really necessary?
> (I thought that MMIO and monitor access is guarded by BQL)

Hmm..I am not sure. This is to synchronize with the ged_st->sel update inside
ged_read(). And also acpi_ged_event() gets called through _power_down_notifier()
as well. BQL guard is in place for all the paths here? 

> 
> > +    ged_st->sel |= sel;
> > +    qemu_mutex_unlock(&ged_st->lock);
> > +
> > +    /* Trigger the event by sending an interrupt to the guest. */
> > +    qemu_irq_pulse(s->irq);
> > +}
> > +
> > +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState
> *ged_st)
> > +{
> > +    AcpiGedState *s = ACPI_GED(dev);
> > +
> > +    assert(s->ged_base);
> > +
> > +    qemu_mutex_init(&ged_st->lock);
> > +    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
> > +                          TYPE_ACPI_GED, ACPI_GED_REG_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);
> > +    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;
> > +    }
> > +
> > +    /*
> > +     * We inject the hotplug interrupt. The IRQ selector will make
> > +     * the difference from the ACPI table.
> I don't get comment at all, pls rephrase/

Ok. I think better to get rid of this comment here and update the one in acpi_ged_event()
appropriately.

> 
> > +     */
> > +    acpi_ged_event(s, sel);
> it seems to used only once and only here, suggest to drop acpi_ged_event()
> and move it's code here.

But patch #10 makes use of it from acpi_ged_pm_powerdown_req().

> > +}
> > +
> > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > +{
> > +    AcpiGedState *s = ACPI_GED(dev);
> > +
> > +    if (s->memhp_state.is_enabled) {
> > +        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
> > +                                 &s->memhp_state,
> > +                                 s->memhp_base);
> > +    }
> > +
> > +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> > +}
> > +
> > +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),
> 
> I'd add checks to realize() to ensure that both foo_base are not left
> uninitialized (i.e. at default values)

Ok. ged_base has a check in acpi_ged_init(). I will add check for memhp_base
and move the ged_base one to realize().

> 
> > +    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..f69d084c89
> > --- /dev/null
> > +++ b/include/hw/acpi/generic_event_device.h
> > @@ -0,0 +1,102 @@
> > +/*
> > + *
> > + * 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 ACPI_GED_REG_LEN           0x4
> Didn't both macros refer to the same thing?
> Why not use the same one in AML and in QEMU MMIO handler?

Ok.

Thanks,
Shameer

> 
> > +
> > +#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;
> > +    QemuMutex    lock;
> > +} 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
> 



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support
  2019-07-18 10:52     ` Shameerali Kolothum Thodi
@ 2019-07-18 12:30       ` Igor Mammedov
  2019-07-22 14:13         ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2019-07-18 12:30 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 Thu, 18 Jul 2019 10:52:10 +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: 17 July 2019 15:33
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> > shannon.zhaosl@gmail.com; ard.biesheuvel@linaro.org;
> > 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 v7 03/10] hw/acpi: Add ACPI Generic
> > Event Device Support
> > 
> > On Tue, 16 Jul 2019 16:38:09 +0100
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:  
> 
> [...]
> 
> > > +static void acpi_ged_event(AcpiGedState *s, uint32_t sel)
> > > +{
> > > +    GEDState *ged_st = &s->ged_state;
> > > +    /*
> > > +     * Set the GED IRQ selector to the expected device type value. This
> > > +     * way, the ACPI method will be able to trigger the right code based
> > > +     * on a unique IRQ.  
> > comment isn't correct anymore, pls fix it  
> 
> True.
> 
> >   
> > > +     */
> > > +    qemu_mutex_lock(&ged_st->lock);  
> > Is this lock really necessary?
> > (I thought that MMIO and monitor access is guarded by BQL)  
> 
> Hmm..I am not sure. This is to synchronize with the ged_st->sel update inside
> ged_read(). And also acpi_ged_event() gets called through _power_down_notifier()
> as well. BQL guard is in place for all the paths here? 
power down command originates from HMP or QMP monitor, so you don't really
need a lock here.

> 
> >   
> > > +    ged_st->sel |= sel;
> > > +    qemu_mutex_unlock(&ged_st->lock);
> > > +
> > > +    /* Trigger the event by sending an interrupt to the guest. */
> > > +    qemu_irq_pulse(s->irq);
> > > +}
> > > +
> > > +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev, GEDState  
> > *ged_st)  
> > > +{
> > > +    AcpiGedState *s = ACPI_GED(dev);
> > > +
> > > +    assert(s->ged_base);
> > > +
> > > +    qemu_mutex_init(&ged_st->lock);
> > > +    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops, ged_st,
> > > +                          TYPE_ACPI_GED, ACPI_GED_REG_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);
> > > +    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;
> > > +    }
> > > +
> > > +    /*
> > > +     * We inject the hotplug interrupt. The IRQ selector will make
> > > +     * the difference from the ACPI table.  
> > I don't get comment at all, pls rephrase/  
> 
> Ok. I think better to get rid of this comment here and update the one in acpi_ged_event()
> appropriately.
> 
> >   
> > > +     */
> > > +    acpi_ged_event(s, sel);  
> > it seems to used only once and only here, suggest to drop acpi_ged_event()
> > and move it's code here.  
> 
> But patch #10 makes use of it from acpi_ged_pm_powerdown_req().
it looks like a valid shortcut but I'd make it follow AcpiInterface->send_event()
path for consistency so only one call chain would exist.


> 
> > > +}
> > > +
> > > +static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
> > > +{
> > > +    AcpiGedState *s = ACPI_GED(dev);
> > > +
> > > +    if (s->memhp_state.is_enabled) {
> > > +        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
> > > +                                 &s->memhp_state,
> > > +                                 s->memhp_base);
> > > +    }
> > > +
> > > +    acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> > > +}
> > > +
> > > +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),  
> > 
> > I'd add checks to realize() to ensure that both foo_base are not left
> > uninitialized (i.e. at default values)  
> 
> Ok. ged_base has a check in acpi_ged_init(). I will add check for memhp_base
> and move the ged_base one to realize().
> 
> >   
> > > +    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..f69d084c89
> > > --- /dev/null
> > > +++ b/include/hw/acpi/generic_event_device.h
> > > @@ -0,0 +1,102 @@
> > > +/*
> > > + *
> > > + * 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 ACPI_GED_REG_LEN           0x4  
> > Didn't both macros refer to the same thing?
> > Why not use the same one in AML and in QEMU MMIO handler?  
> 
> Ok.
> 
> Thanks,
> Shameer
> 
> >   
> > > +
> > > +#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;
> > > +    QemuMutex    lock;
> > > +} 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  
> >   
> 



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 06/10] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 06/10] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot Shameer Kolothum
@ 2019-07-18 12:41   ` Igor Mammedov
  0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2019-07-18 12:41 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 Tue, 16 Jul 2019 16:38:12 +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>
> ---
> v6 --> v7
>   Disabled ACPI GED dev creation for machine < 4.2.
> 
> ---
>  hw/arm/Kconfig           |  2 ++
>  hw/arm/virt-acpi-build.c | 14 +++++++++++
>  hw/arm/virt.c            | 51 ++++++++++++++++++++++++++++++++++------
>  include/hw/arm/virt.h    |  4 ++++
>  4 files changed, 64 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..ffcccc05ad 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_REG_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,25 @@ 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]);

leaking 'dev' reference created by object_new(), you need to unref it here.

> +
> +    return dev;
> +}
> +
>  static void create_its(VirtMachineState *vms, DeviceState *gicdev)
>  {
>      const char *itsclass = its_class_name();
> @@ -1697,6 +1720,10 @@ static void machvirt_init(MachineState *machine)
>  
>      create_gpio(vms, pic);
>  
> +    if (!vmc->no_acpi_dev && aarch64 && firmware_loaded && acpi_enabled) {
suggest s/no_acpi_dev/has_ged/

> +        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 +1903,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 +2136,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_acpi_dev = true;
>  }
>  DEFINE_VIRT_MACHINE(4, 1)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index a72094204e..32892af1dd 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_acpi_dev;
it deserved a comment or maybe be more specific and s/acpi_dev/ged/ + comment

>  } 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)



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event Shameer Kolothum
@ 2019-07-18 13:03   ` Igor Mammedov
  2019-07-22 14:15     ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2019-07-18 13:03 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, sameo, shannon.zhaosl, ard.biesheuvel, qemu-devel,
	xuwei5, linuxarm, eric.auger, qemu-arm, sebastien.boeuf, lersek

On Tue, 16 Jul 2019 16:38:15 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> Use GED for system_powerdown event instead of GPIO for ACPI.
> Guest boot with DT still uses GPIO.
 

I'd hate to keep ACPI GPIO around but taking in account migration
wouldn't this patch break ACPI GPIO based button on 4.0 and older where
GED is not available and guest was booted as ACPI one and then rebooted on new QEMU?


> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 37 +------------------------------------
>  hw/arm/virt.c            |  6 +++---
>  2 files changed, 4 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 75657caa36..9178ca8e40 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)
>  {
> @@ -328,37 +327,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap,
>      aml_append(scope, dev);
>  }
>  
> -static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
> -                                           uint32_t gpio_irq)
> -{
> -    Aml *dev = aml_device("GPO0");
> -    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> -    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> -    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> -
> -    Aml *crs = aml_resource_template();
> -    aml_append(crs, aml_memory32_fixed(gpio_memmap->base, gpio_memmap->size,
> -                                       AML_READ_WRITE));
> -    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
> -                                  AML_EXCLUSIVE, &gpio_irq, 1));
> -    aml_append(dev, aml_name_decl("_CRS", crs));
> -
> -    Aml *aei = aml_resource_template();
> -    /* Pin 3 for power button */
> -    const uint32_t pin_list[1] = {3};
> -    aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> -                                 AML_EXCLUSIVE, AML_PULL_UP, 0, pin_list, 1,
> -                                 "GPO0", NULL, 0));
> -    aml_append(dev, aml_name_decl("_AEI", aei));
> -
> -    /* _E03 is handle for power button */
> -    Aml *method = aml_method("_E03", 0, AML_NOTSERIALIZED);
> -    aml_append(method, aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
> -                                  aml_int(0x80)));
> -    aml_append(dev, method);
> -    aml_append(scope, dev);
> -}
> -
>  static void acpi_dsdt_add_power_button(Aml *scope)
>  {
>      Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> @@ -739,9 +707,8 @@ 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) {
> +        acpi_dsdt_add_power_button(scope);
>          build_ged_aml(scope, "\\_SB."GED_DEVICE,
>                        HOTPLUG_HANDLER(vms->acpi_dev),
>                        irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY);
> @@ -752,8 +719,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                                   AML_SYSTEM_MEMORY);
>      }
>  
> -    acpi_dsdt_add_power_button(scope);
> -
>      aml_append(dsdt, scope);
>  
>      /* copy AML table into ACPI tables blob and patch header there */
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index ffcccc05ad..6e55c25c5f 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",
> @@ -1718,10 +1718,10 @@ static void machvirt_init(MachineState *machine)
>  
>      create_pcie(vms, pic);
>  
> -    create_gpio(vms, pic);
> -
>      if (!vmc->no_acpi_dev && aarch64 && firmware_loaded && acpi_enabled) {
>          vms->acpi_dev = create_acpi_ged(vms, pic);
> +    } else {
> +        create_gpio(vms, pic);
>      }
>  
>      /* Create mmio transports, so the user can create virtio backends



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 08/10] hw/acpi: Add system power down support to GED
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 08/10] hw/acpi: Add system power down support to GED Shameer Kolothum
@ 2019-07-18 13:07   ` Igor Mammedov
  0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2019-07-18 13:07 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, sameo, shannon.zhaosl, ard.biesheuvel, qemu-devel,
	xuwei5, linuxarm, eric.auger, qemu-arm, sebastien.boeuf, lersek

On Tue, 16 Jul 2019 16:38:14 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This adds support to use GED for system power down event.
[...]

> @@ -232,6 +238,13 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>      acpi_ged_event(s, sel);
>  }
>  
> +static void acpi_ged_pm_powerdown_req(Notifier *n, void *opaque)
> +{
> +    AcpiGedState *s = container_of(n, AcpiGedState, powerdown_notifier);
> +
> +    acpi_ged_event(s, ACPI_GED_PWR_DOWN_EVT);
> +}
> +
>  static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
>  {
>      AcpiGedState *s = ACPI_GED(dev);
> @@ -243,6 +256,9 @@ static void acpi_ged_device_realize(DeviceState *dev, Error **errp)
>      }
>  
>      acpi_ged_init(get_system_memory(), dev, &s->ged_state);
> +
> +    s->powerdown_notifier.notify = acpi_ged_pm_powerdown_req;

one more thing:
I'd reuse virt_powerdown_req() instead.

pass it machine pointer and then from notifier you'd be able to get to acpi_dev

> +    qemu_register_powerdown_notifier(&s->powerdown_notifier);
>  }
>  
>  static Property acpi_ged_properties[] = {
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index f69d084c89..9e63b72cb9 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)
> @@ -79,6 +81,7 @@
>   * through GED.
>   */
>  #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
> +#define ACPI_GED_PWR_DOWN_EVT      0x2
>  
>  typedef struct GEDState {
>      MemoryRegion io;
> @@ -88,6 +91,7 @@ typedef struct GEDState {
>  
>  typedef struct AcpiGedState {
>      DeviceClass parent_obj;
> +    Notifier powerdown_notifier;
>      MemHotplugState memhp_state;
>      hwaddr memhp_base;
>      hwaddr ged_base;



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes
  2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes Shameer Kolothum
@ 2019-07-18 13:13   ` Igor Mammedov
  2019-07-22 14:51     ` Shameerali Kolothum Thodi
  0 siblings, 1 reply; 25+ messages in thread
From: Igor Mammedov @ 2019-07-18 13:13 UTC (permalink / raw)
  To: Shameer Kolothum
  Cc: peter.maydell, sameo, shannon.zhaosl, ard.biesheuvel, qemu-devel,
	xuwei5, linuxarm, eric.auger, qemu-arm, sebastien.boeuf, lersek

On Tue, 16 Jul 2019 16:38:16 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> From: Eric Auger <eric.auger@redhat.com>
> 
> PCDIMM hotplug addition updated the DSDT. Update the reference table.

it's not correct process. series should be merged through Michael's pci branch
and see
commit ab50f22309a17c772c51931940596e707c200739 (mst/pci)
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Tue May 21 17:38:47 2019 -0400

    bios-tables-test: add diff allowed list

how to request table update.


Another thing:
bios-tables-test has test_acpi_tcg_dimm_pxm() test case,
pls make use of it to test arm/virt variant

> 
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  tests/data/acpi/virt/DSDT | Bin 18476 -> 18493 bytes
>  1 file changed, 0 insertions(+), 0 deletions(-)
> 
> diff --git a/tests/data/acpi/virt/DSDT b/tests/data/acpi/virt/DSDT
> index 20e85c7f89f645c69935c615c07084e221419960..1a5c3e5243c78eb25345fdf7ffa442afec3c5c28 100644
> GIT binary patch
> delta 172
> zcmZ28fpPByMlP3Nmk?VI1_q{Y6S<_BIvO{s=eSSi^^mWRZt~=a(Tfjuig$N)iRXy-
> z@O0sIbPn(|FfcS`;D`?ea)A;+mUB?BARiZZCmRDZqb36bLnT9WgKLnhI|B~`0|zG;
> zOLQYQi0kYc?CPT+ViNBf7Q$Ho;v_5(n_Qj1$iNt*&kVB1*CW8$H`v*4fdEjLhl`1U
> Ni>WOjJjjWw0RY`tDw+TQ  
> 
> delta 134
> zcmdlxfpN_QMlP3Nmk=Ef1_q{ziCof5?F}2%bKIpo*xdvC4LIUGJY9GlgM2*<49pA}
> zIN}{$f+og`c`$P_2rw{ovN13+sxvS!RDxt&Jq4Ay6ndl?7@-g-A<n=c$sh-0Ff)LR
> Z11c2ZjdwLLW@rlt4|3vam^{&45den68LI#Q
> 



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support
  2019-07-18 12:30       ` Igor Mammedov
@ 2019-07-22 14:13         ` Shameerali Kolothum Thodi
  0 siblings, 0 replies; 25+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-07-22 14:13 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



> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 18 July 2019 13:31
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> shannon.zhaosl@gmail.com; ard.biesheuvel@linaro.org;
> 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 v7 03/10] hw/acpi: Add ACPI Generic
> Event Device Support
> 
> On Thu, 18 Jul 2019 10:52:10 +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: 17 July 2019 15:33
> > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > > Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> > > shannon.zhaosl@gmail.com; ard.biesheuvel@linaro.org;
> > > 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 v7 03/10] hw/acpi: Add ACPI
> Generic
> > > Event Device Support
> > >
> > > On Tue, 16 Jul 2019 16:38:09 +0100
> > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >
> > [...]
> >
> > > > +static void acpi_ged_event(AcpiGedState *s, uint32_t sel)
> > > > +{
> > > > +    GEDState *ged_st = &s->ged_state;
> > > > +    /*
> > > > +     * Set the GED IRQ selector to the expected device type value. This
> > > > +     * way, the ACPI method will be able to trigger the right code based
> > > > +     * on a unique IRQ.
> > > comment isn't correct anymore, pls fix it
> >
> > True.
> >
> > >
> > > > +     */
> > > > +    qemu_mutex_lock(&ged_st->lock);
> > > Is this lock really necessary?
> > > (I thought that MMIO and monitor access is guarded by BQL)
> >
> > Hmm..I am not sure. This is to synchronize with the ged_st->sel update inside
> > ged_read(). And also acpi_ged_event() gets called through
> _power_down_notifier()
> > as well. BQL guard is in place for all the paths here?
> power down command originates from HMP or QMP monitor, so you don't
> really
> need a lock here.

Ok. I will get rid of it then.

> >
> > >
> > > > +    ged_st->sel |= sel;
> > > > +    qemu_mutex_unlock(&ged_st->lock);
> > > > +
> > > > +    /* Trigger the event by sending an interrupt to the guest. */
> > > > +    qemu_irq_pulse(s->irq);
> > > > +}
> > > > +
> > > > +static void acpi_ged_init(MemoryRegion *as, DeviceState *dev,
> GEDState
> > > *ged_st)
> > > > +{
> > > > +    AcpiGedState *s = ACPI_GED(dev);
> > > > +
> > > > +    assert(s->ged_base);
> > > > +
> > > > +    qemu_mutex_init(&ged_st->lock);
> > > > +    memory_region_init_io(&ged_st->io, OBJECT(dev), &ged_ops,
> ged_st,
> > > > +                          TYPE_ACPI_GED, ACPI_GED_REG_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);
> > > > +    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;
> > > > +    }
> > > > +
> > > > +    /*
> > > > +     * We inject the hotplug interrupt. The IRQ selector will make
> > > > +     * the difference from the ACPI table.
> > > I don't get comment at all, pls rephrase/
> >
> > Ok. I think better to get rid of this comment here and update the one in
> acpi_ged_event()
> > appropriately.
> >
> > >
> > > > +     */
> > > > +    acpi_ged_event(s, sel);
> > > it seems to used only once and only here, suggest to drop acpi_ged_event()
> > > and move it's code here.
> >
> > But patch #10 makes use of it from acpi_ged_pm_powerdown_req().
> it looks like a valid shortcut but I'd make it follow AcpiInterface->send_event()
> path for consistency so only one call chain would exist.

Agree. 

Thanks,
Shameer
 



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event
  2019-07-18 13:03   ` Igor Mammedov
@ 2019-07-22 14:15     ` Shameerali Kolothum Thodi
  2019-07-23 12:46       ` Igor Mammedov
  0 siblings, 1 reply; 25+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-07-22 14:15 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



> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 18 July 2019 14:03
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> shannon.zhaosl@gmail.com; ard.biesheuvel@linaro.org;
> 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 v7 09/10] hw/arm: Use GED for
> system_powerdown event
> 
> On Tue, 16 Jul 2019 16:38:15 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > Use GED for system_powerdown event instead of GPIO for ACPI.
> > Guest boot with DT still uses GPIO.
> 
> 
> I'd hate to keep ACPI GPIO around but taking in account migration
> wouldn't this patch break ACPI GPIO based button on 4.0 and older where
> GED is not available and guest was booted as ACPI one and then rebooted on
> new QEMU?

Hmm..That looks like a valid case unfortunately :(. I will keep the GPIO then.

Thanks,
Shameer
 
> 
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > ---
> >  hw/arm/virt-acpi-build.c | 37 +------------------------------------
> >  hw/arm/virt.c            |  6 +++---
> >  2 files changed, 4 insertions(+), 39 deletions(-)
> >
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 75657caa36..9178ca8e40 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)
> >  {
> > @@ -328,37 +327,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const
> MemMapEntry *memmap,
> >      aml_append(scope, dev);
> >  }
> >
> > -static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry
> *gpio_memmap,
> > -                                           uint32_t gpio_irq)
> > -{
> > -    Aml *dev = aml_device("GPO0");
> > -    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> > -    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > -    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > -
> > -    Aml *crs = aml_resource_template();
> > -    aml_append(crs, aml_memory32_fixed(gpio_memmap->base,
> gpio_memmap->size,
> > -                                       AML_READ_WRITE));
> > -    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL,
> AML_ACTIVE_HIGH,
> > -                                  AML_EXCLUSIVE, &gpio_irq, 1));
> > -    aml_append(dev, aml_name_decl("_CRS", crs));
> > -
> > -    Aml *aei = aml_resource_template();
> > -    /* Pin 3 for power button */
> > -    const uint32_t pin_list[1] = {3};
> > -    aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE,
> AML_ACTIVE_HIGH,
> > -                                 AML_EXCLUSIVE, AML_PULL_UP, 0,
> pin_list, 1,
> > -                                 "GPO0", NULL, 0));
> > -    aml_append(dev, aml_name_decl("_AEI", aei));
> > -
> > -    /* _E03 is handle for power button */
> > -    Aml *method = aml_method("_E03", 0, AML_NOTSERIALIZED);
> > -    aml_append(method,
> aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
> > -                                  aml_int(0x80)));
> > -    aml_append(dev, method);
> > -    aml_append(scope, dev);
> > -}
> > -
> >  static void acpi_dsdt_add_power_button(Aml *scope)
> >  {
> >      Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> > @@ -739,9 +707,8 @@ 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) {
> > +        acpi_dsdt_add_power_button(scope);
> >          build_ged_aml(scope, "\\_SB."GED_DEVICE,
> >                        HOTPLUG_HANDLER(vms->acpi_dev),
> >                        irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE,
> AML_SYSTEM_MEMORY);
> > @@ -752,8 +719,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> >                                   AML_SYSTEM_MEMORY);
> >      }
> >
> > -    acpi_dsdt_add_power_button(scope);
> > -
> >      aml_append(dsdt, scope);
> >
> >      /* copy AML table into ACPI tables blob and patch header there */
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index ffcccc05ad..6e55c25c5f 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",
> > @@ -1718,10 +1718,10 @@ static void machvirt_init(MachineState
> *machine)
> >
> >      create_pcie(vms, pic);
> >
> > -    create_gpio(vms, pic);
> > -
> >      if (!vmc->no_acpi_dev && aarch64 && firmware_loaded &&
> acpi_enabled) {
> >          vms->acpi_dev = create_acpi_ged(vms, pic);
> > +    } else {
> > +        create_gpio(vms, pic);
> >      }
> >
> >      /* Create mmio transports, so the user can create virtio backends
> 



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes
  2019-07-18 13:13   ` Igor Mammedov
@ 2019-07-22 14:51     ` Shameerali Kolothum Thodi
  2019-07-23 12:58       ` Igor Mammedov
  0 siblings, 1 reply; 25+ messages in thread
From: Shameerali Kolothum Thodi @ 2019-07-22 14:51 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: 18 July 2019 14:13
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> shannon.zhaosl@gmail.com; ard.biesheuvel@linaro.org;
> 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 v7 10/10] tests: Update DSDT ACPI
> table for arm/virt board with PCDIMM related changes
> 
> On Tue, 16 Jul 2019 16:38:16 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > From: Eric Auger <eric.auger@redhat.com>
> >
> > PCDIMM hotplug addition updated the DSDT. Update the reference table.
> 
> it's not correct process. series should be merged through Michael's pci branch
> and see
> commit ab50f22309a17c772c51931940596e707c200739 (mst/pci)
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Tue May 21 17:38:47 2019 -0400
> 
>     bios-tables-test: add diff allowed list
> 
> how to request table update.

Ok. Just to confirm, this means I can probably add the below diff to patch #6 and
remove this patch(10/10) from the series. 

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",

> 
> Another thing:
> bios-tables-test has test_acpi_tcg_dimm_pxm() test case,
> pls make use of it to test arm/virt variant

I had a go with this, but has found an issue with this.

This is what I added in order to run the dimm_pxm test.

- - 8- -

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index a356ac3489..79af4f4874 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -871,6 +871,36 @@ static void test_acpi_piix4_tcg_dimm_pxm(void)
     test_acpi_tcg_dimm_pxm(MACHINE_PC);
 }
 
+static void test_acpi_virt_tcg_dimm_pxm(void)
+{
+    test_data data = {
+        .machine = "virt",
+        .accel = "tcg",
+        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
+        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
+        .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
+        .ram_start = 0x40000000ULL,
+        .scan_len = 128ULL * 1024 * 1024,
+    };
+
+    data.variant = ".dimmpxm";
+    test_acpi_one(" -cpu cortex-a57"
+                  " -smp 4"
+                  " -m 512M,slots=3,maxmem=2G"
+                  " -object memory-backend-ram,id=ram0,size=128M"
+                  " -object memory-backend-ram,id=ram1,size=128M"
+                  " -object memory-backend-ram,id=ram2,size=128M"
+                  " -object memory-backend-ram,id=ram3,size=128M"
+                  " -numa node,memdev=ram0,nodeid=0"
+                  " -numa node,memdev=ram1,nodeid=1"
+                  " -numa node,memdev=ram2,nodeid=2"
+                  " -numa node,memdev=ram3,nodeid=3"
+                  " -object memory-backend-ram,id=ram4,size=1G"
+                  " -device pc-dimm,id=dimm0,memdev=ram4,node=0",
+                  &data);
+    free_test_data(&data);
+}
+
 static void test_acpi_virt_tcg(void)
 {
     test_data data = {
@@ -917,6 +947,7 @@ int main(int argc, char *argv[])
         qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm);
     } else if (strcmp(arch, "aarch64") == 0) {
         qtest_add_func("acpi/virt", test_acpi_virt_tcg);
+        qtest_add_func("acpi/virt/dimmpxm", test_acpi_virt_tcg_dimm_pxm);
     }
     ret = g_test_run();
     boot_sector_cleanup(disk);

- - 8- -

Then used the script to generate the acpi tables and "make check" runs fine.

But when I changed the memory configuration to,

test_acpi_one(" -cpu cortex-a57"
            " -smp 4"
            " -m 256M,slots=3,maxmem=2G"
            " -object memory-backend-ram,id=ram0,size=64M"
            " -object memory-backend-ram,id=ram1,size=64M"
            " -object memory-backend-ram,id=ram2,size=64M"
            " -object memory-backend-ram,id=ram3,size=64M"
            " -numa node,memdev=ram0,nodeid=0"
            " -numa node,memdev=ram1,nodeid=1"
            " -numa node,memdev=ram2,nodeid=2"
            " -numa node,memdev=ram3,nodeid=3"
            " -object memory-backend-ram,id=ram4,size=1G"
            " -device pc-dimm,id=dimm0,memdev=ram4,node=0",
            &data);

"make check" gets stuck at,

  CC      tests/bios-tables-test.o
  LINK    tests/bios-tables-test
  TEST    check-qtest-aarch64: tests/numa-test
  TEST    check-qtest-aarch64: tests/boot-serial-test
  TEST    check-qtest-aarch64: tests/migration-test
  TEST    check-qtest-aarch64: tests/bios-tables-test -->stuck here.

But if I use the same memory configuration and run the qemu guest independently
it boots fine.

./qemu-system-aarch64 -machine virt,accel=tcg \
-cpu cortex-a57 \
-nographic -smp 4 \
-bios QEMU_EFI_Release.fd \
-kernel Image_5.2rc2 \
-drive if=none,file=ubuntu-est-5.0,id=fs \
-device virtio-blk-device,drive=fs \
-m 256M,slots=3,maxmem=2G \
-object memory-backend-ram,id=ram0,size=64M \
-object memory-backend-ram,id=ram1,size=64M \
-object memory-backend-ram,id=ram2,size=64M \
-object memory-backend-ram,id=ram3,size=64M \
-numa node,memdev=ram0,nodeid=0 \
-numa node,memdev=ram1,nodeid=1 \
-numa node,memdev=ram2,nodeid=2 \
-numa node,memdev=ram3,nodeid=3 \
-object memory-backend-ram,id=ram4,size=1G \
-device pc-dimm,id=dimm1,memdev=ram4,node=0 \
-append "console=ttyAMA0 root=/dev/vda rw acpi=force"
 
root@ubuntu:~# numactl -H
available: 4 nodes (0-3)
node 0 cpus: 0
node 0 size: 1067 MB
node 0 free: 1015 MB
node 1 cpus: 1
node 1 size: 44 MB
node 1 free: 5 MB
node 2 cpus: 2
node 2 size: 63 MB
node 2 free: 24 MB
node 3 cpus: 3
node 3 size: 63 MB
node 3 free: 31 MB
node distances:
node   0   1   2   3 
  0:  10  20  20  20 
  1:  20  10  20  20 
  2:  20  20  10  20 
  3:  20  20  20  10 
root@ubuntu:~#

I am not sure why "make check" hangs with 64M size!. 
Please take a look and let me know if I missed anything obvious here.

Thanks,
Shameer


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event
  2019-07-22 14:15     ` Shameerali Kolothum Thodi
@ 2019-07-23 12:46       ` Igor Mammedov
  0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2019-07-23 12:46 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 Mon, 22 Jul 2019 14:15:17 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> > -----Original Message-----
> > From: Qemu-devel
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 18 July 2019 14:03
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> > shannon.zhaosl@gmail.com; ard.biesheuvel@linaro.org;
> > 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 v7 09/10] hw/arm: Use GED for
> > system_powerdown event
> > 
> > On Tue, 16 Jul 2019 16:38:15 +0100
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >   
> > > Use GED for system_powerdown event instead of GPIO for ACPI.
> > > Guest boot with DT still uses GPIO.  
> > 
> > 
> > I'd hate to keep ACPI GPIO around but taking in account migration
> > wouldn't this patch break ACPI GPIO based button on 4.0 and older where
> > GED is not available and guest was booted as ACPI one and then rebooted on
> > new QEMU?  
> 
> Hmm..That looks like a valid case unfortunately :(. I will keep the GPIO then.
yep, keep it for old machine types. New ones should use GED.

> 
> Thanks,
> Shameer
>  
> >   
> > > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > > ---
> > >  hw/arm/virt-acpi-build.c | 37 +------------------------------------
> > >  hw/arm/virt.c            |  6 +++---
> > >  2 files changed, 4 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > > index 75657caa36..9178ca8e40 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)
> > >  {
> > > @@ -328,37 +327,6 @@ static void acpi_dsdt_add_pci(Aml *scope, const  
> > MemMapEntry *memmap,  
> > >      aml_append(scope, dev);
> > >  }
> > >
> > > -static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry  
> > *gpio_memmap,  
> > > -                                           uint32_t gpio_irq)
> > > -{
> > > -    Aml *dev = aml_device("GPO0");
> > > -    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
> > > -    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> > > -    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> > > -
> > > -    Aml *crs = aml_resource_template();
> > > -    aml_append(crs, aml_memory32_fixed(gpio_memmap->base,  
> > gpio_memmap->size,  
> > > -                                       AML_READ_WRITE));
> > > -    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL,  
> > AML_ACTIVE_HIGH,  
> > > -                                  AML_EXCLUSIVE, &gpio_irq, 1));
> > > -    aml_append(dev, aml_name_decl("_CRS", crs));
> > > -
> > > -    Aml *aei = aml_resource_template();
> > > -    /* Pin 3 for power button */
> > > -    const uint32_t pin_list[1] = {3};
> > > -    aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE,  
> > AML_ACTIVE_HIGH,  
> > > -                                 AML_EXCLUSIVE, AML_PULL_UP, 0,  
> > pin_list, 1,  
> > > -                                 "GPO0", NULL, 0));
> > > -    aml_append(dev, aml_name_decl("_AEI", aei));
> > > -
> > > -    /* _E03 is handle for power button */
> > > -    Aml *method = aml_method("_E03", 0, AML_NOTSERIALIZED);
> > > -    aml_append(method,  
> > aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),  
> > > -                                  aml_int(0x80)));
> > > -    aml_append(dev, method);
> > > -    aml_append(scope, dev);
> > > -}
> > > -
> > >  static void acpi_dsdt_add_power_button(Aml *scope)
> > >  {
> > >      Aml *dev = aml_device(ACPI_POWER_BUTTON_DEVICE);
> > > @@ -739,9 +707,8 @@ 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) {
> > > +        acpi_dsdt_add_power_button(scope);
> > >          build_ged_aml(scope, "\\_SB."GED_DEVICE,
> > >                        HOTPLUG_HANDLER(vms->acpi_dev),
> > >                        irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE,  
> > AML_SYSTEM_MEMORY);  
> > > @@ -752,8 +719,6 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,  
> > VirtMachineState *vms)  
> > >                                   AML_SYSTEM_MEMORY);
> > >      }
> > >
> > > -    acpi_dsdt_add_power_button(scope);
> > > -
> > >      aml_append(dsdt, scope);
> > >
> > >      /* copy AML table into ACPI tables blob and patch header there */
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index ffcccc05ad..6e55c25c5f 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",
> > > @@ -1718,10 +1718,10 @@ static void machvirt_init(MachineState  
> > *machine)  
> > >
> > >      create_pcie(vms, pic);
> > >
> > > -    create_gpio(vms, pic);
> > > -
> > >      if (!vmc->no_acpi_dev && aarch64 && firmware_loaded &&  
> > acpi_enabled) {  
> > >          vms->acpi_dev = create_acpi_ged(vms, pic);
> > > +    } else {
> > > +        create_gpio(vms, pic);
> > >      }
> > >
> > >      /* Create mmio transports, so the user can create virtio backends  



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes
  2019-07-22 14:51     ` Shameerali Kolothum Thodi
@ 2019-07-23 12:58       ` Igor Mammedov
  0 siblings, 0 replies; 25+ messages in thread
From: Igor Mammedov @ 2019-07-23 12:58 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 Mon, 22 Jul 2019 14:51:59 +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: 18 July 2019 14:13
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: peter.maydell@linaro.org; sameo@linux.intel.com;
> > shannon.zhaosl@gmail.com; ard.biesheuvel@linaro.org;
> > 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 v7 10/10] tests: Update DSDT ACPI
> > table for arm/virt board with PCDIMM related changes
> > 
> > On Tue, 16 Jul 2019 16:38:16 +0100
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> >   
> > > From: Eric Auger <eric.auger@redhat.com>
> > >
> > > PCDIMM hotplug addition updated the DSDT. Update the reference table.  
> > 
> > it's not correct process. series should be merged through Michael's pci branch
> > and see
> > commit ab50f22309a17c772c51931940596e707c200739 (mst/pci)
> > Author: Michael S. Tsirkin <mst@redhat.com>
> > Date:   Tue May 21 17:38:47 2019 -0400
> > 
> >     bios-tables-test: add diff allowed list
> > 
> > how to request table update.  
> 
> Ok. Just to confirm, this means I can probably add the below diff to patch #6 and
> remove this patch(10/10) from the series. 

you can use commit 4a4418369d6 as an example

> 
> 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",
> 
> > 
> > Another thing:
> > bios-tables-test has test_acpi_tcg_dimm_pxm() test case,
> > pls make use of it to test arm/virt variant  
> 
> I had a go with this, but has found an issue with this.
> 
> This is what I added in order to run the dimm_pxm test.
> 
> - - 8- -
> 
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index a356ac3489..79af4f4874 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -871,6 +871,36 @@ static void test_acpi_piix4_tcg_dimm_pxm(void)
>      test_acpi_tcg_dimm_pxm(MACHINE_PC);
>  }
>  
> +static void test_acpi_virt_tcg_dimm_pxm(void)
> +{
> +    test_data data = {
> +        .machine = "virt",
> +        .accel = "tcg",
> +        .uefi_fl1 = "pc-bios/edk2-aarch64-code.fd",
> +        .uefi_fl2 = "pc-bios/edk2-arm-vars.fd",
> +        .cd = "tests/data/uefi-boot-images/bios-tables-test.aarch64.iso.qcow2",
> +        .ram_start = 0x40000000ULL,
> +        .scan_len = 128ULL * 1024 * 1024,
> +    };
> +
> +    data.variant = ".dimmpxm";
> +    test_acpi_one(" -cpu cortex-a57"
> +                  " -smp 4"
> +                  " -m 512M,slots=3,maxmem=2G"
> +                  " -object memory-backend-ram,id=ram0,size=128M"
> +                  " -object memory-backend-ram,id=ram1,size=128M"
> +                  " -object memory-backend-ram,id=ram2,size=128M"
> +                  " -object memory-backend-ram,id=ram3,size=128M"
> +                  " -numa node,memdev=ram0,nodeid=0"
> +                  " -numa node,memdev=ram1,nodeid=1"
> +                  " -numa node,memdev=ram2,nodeid=2"
> +                  " -numa node,memdev=ram3,nodeid=3"
> +                  " -object memory-backend-ram,id=ram4,size=1G"
> +                  " -device pc-dimm,id=dimm0,memdev=ram4,node=0",
> +                  &data);
> +    free_test_data(&data);
> +}
> +
>  static void test_acpi_virt_tcg(void)
>  {
>      test_data data = {
> @@ -917,6 +947,7 @@ int main(int argc, char *argv[])
>          qtest_add_func("acpi/q35/dimmpxm", test_acpi_q35_tcg_dimm_pxm);
>      } else if (strcmp(arch, "aarch64") == 0) {
>          qtest_add_func("acpi/virt", test_acpi_virt_tcg);
> +        qtest_add_func("acpi/virt/dimmpxm", test_acpi_virt_tcg_dimm_pxm);
>      }
>      ret = g_test_run();
>      boot_sector_cleanup(disk);
> 
> - - 8- -
> 
> Then used the script to generate the acpi tables and "make check" runs fine.
> 
> But when I changed the memory configuration to,
> 
> test_acpi_one(" -cpu cortex-a57"
>             " -smp 4"
>             " -m 256M,slots=3,maxmem=2G"
>             " -object memory-backend-ram,id=ram0,size=64M"
>             " -object memory-backend-ram,id=ram1,size=64M"
>             " -object memory-backend-ram,id=ram2,size=64M"
>             " -object memory-backend-ram,id=ram3,size=64M"
>             " -numa node,memdev=ram0,nodeid=0"
>             " -numa node,memdev=ram1,nodeid=1"
>             " -numa node,memdev=ram2,nodeid=2"
>             " -numa node,memdev=ram3,nodeid=3"
>             " -object memory-backend-ram,id=ram4,size=1G"
>             " -device pc-dimm,id=dimm0,memdev=ram4,node=0",
>             &data);
> 
> "make check" gets stuck at,
> 
>   CC      tests/bios-tables-test.o
>   LINK    tests/bios-tables-test
>   TEST    check-qtest-aarch64: tests/numa-test
>   TEST    check-qtest-aarch64: tests/boot-serial-test
>   TEST    check-qtest-aarch64: tests/migration-test
>   TEST    check-qtest-aarch64: tests/bios-tables-test -->stuck here.
> 
> But if I use the same memory configuration and run the qemu guest independently
> it boots fine.
> 
> ./qemu-system-aarch64 -machine virt,accel=tcg \
> -cpu cortex-a57 \
> -nographic -smp 4 \
> -bios QEMU_EFI_Release.fd \
> -kernel Image_5.2rc2 \
> -drive if=none,file=ubuntu-est-5.0,id=fs \
> -device virtio-blk-device,drive=fs \
> -m 256M,slots=3,maxmem=2G \
> -object memory-backend-ram,id=ram0,size=64M \
> -object memory-backend-ram,id=ram1,size=64M \
> -object memory-backend-ram,id=ram2,size=64M \
> -object memory-backend-ram,id=ram3,size=64M \
> -numa node,memdev=ram0,nodeid=0 \
> -numa node,memdev=ram1,nodeid=1 \
> -numa node,memdev=ram2,nodeid=2 \
> -numa node,memdev=ram3,nodeid=3 \
> -object memory-backend-ram,id=ram4,size=1G \
> -device pc-dimm,id=dimm1,memdev=ram4,node=0 \
> -append "console=ttyAMA0 root=/dev/vda rw acpi=force"
>  
> root@ubuntu:~# numactl -H
> available: 4 nodes (0-3)
> node 0 cpus: 0
> node 0 size: 1067 MB
> node 0 free: 1015 MB
> node 1 cpus: 1
> node 1 size: 44 MB
> node 1 free: 5 MB
> node 2 cpus: 2
> node 2 size: 63 MB
> node 2 free: 24 MB
> node 3 cpus: 3
> node 3 size: 63 MB
> node 3 free: 31 MB
> node distances:
> node   0   1   2   3 
>   0:  10  20  20  20 
>   1:  20  10  20  20 
>   2:  20  20  10  20 
>   3:  20  20  20  10 
> root@ubuntu:~#
> 
> I am not sure why "make check" hangs with 64M size!. 
> Please take a look and let me know if I missed anything obvious here.

I'd try to see what tests/bios-tables-test does when it's stuck.
(I'd guess that it can't find ACPI tables)

also pay attention to .scan_len so it would cover all RAM


> 
> Thanks,
> Shameer



^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2019-07-23 12:58 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-16 15:38 [Qemu-devel] [PATCH-for-4.2 v7 00/10] ARM virt: ACPI memory hotplug support Shameer Kolothum
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 01/10] hw/acpi: Make ACPI IO address space configurable Shameer Kolothum
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 02/10] hw/acpi: Do not create memory hotplug method when handler is not defined Shameer Kolothum
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 03/10] hw/acpi: Add ACPI Generic Event Device Support Shameer Kolothum
2019-07-17 14:32   ` Igor Mammedov
2019-07-18 10:52     ` Shameerali Kolothum Thodi
2019-07-18 12:30       ` Igor Mammedov
2019-07-22 14:13         ` Shameerali Kolothum Thodi
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 04/10] hw/arm/virt: Add memory hotplug framework Shameer Kolothum
2019-07-17 14:40   ` Igor Mammedov
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 05/10] hw/arm/virt: Add 4.2 machine type Shameer Kolothum
2019-07-17 14:51   ` Igor Mammedov
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 06/10] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot Shameer Kolothum
2019-07-18 12:41   ` Igor Mammedov
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 07/10] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT Shameer Kolothum
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 08/10] hw/acpi: Add system power down support to GED Shameer Kolothum
2019-07-18 13:07   ` Igor Mammedov
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 09/10] hw/arm: Use GED for system_powerdown event Shameer Kolothum
2019-07-18 13:03   ` Igor Mammedov
2019-07-22 14:15     ` Shameerali Kolothum Thodi
2019-07-23 12:46       ` Igor Mammedov
2019-07-16 15:38 ` [Qemu-devel] [PATCH-for-4.2 v7 10/10] tests: Update DSDT ACPI table for arm/virt board with PCDIMM related changes Shameer Kolothum
2019-07-18 13:13   ` Igor Mammedov
2019-07-22 14:51     ` Shameerali Kolothum Thodi
2019-07-23 12:58       ` Igor Mammedov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.