[Qemu-devel] [PATCH v2 00/11] ARM virt: ACPI memory hotplug support

[Qemu-devel] [PATCH v2 00/11] ARM virt: ACPI memory hotplug support

[Shameer Kolothum]

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 series can be applied on top of Eric's branch here[4].

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] https://github.com/eauger/qemu/tree/v3.1.0-extended-memmap-v12


RFC --> v2

-Use GED device instead of GPIO for ACPI hotplug events.
-Removed NVDIMM support for now.
-Includes dropped patches from Eric's v9 series.


Eric Auger (1):
  hw/arm/virt: Add memory hotplug framework

Samuel Ortiz (3):
  hw/acpi: Do not create memory hotplug method when handler is not
    defined
  hw/arm/virt: Add virtual ACPI device
  hw/acpi: Add ACPI Generic Event Device Support

Sebastien Boeuf (1):
  hw/acpi: Move constant definitions to header files

Shameer Kolothum (6):
  hw/acpi: Make ACPI IO address space configurable
  hw/arm/virt: Add ACPI support for device memory cold-plug
  hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
  hw/arm/boot: Expose the PC-DIMM nodes in the DT
  hw/arm/virt: Add GED device configuration and build aml
  hw/arm/virt: Add GED irq routing and Enable memory hotplug

 default-configs/arm-softmmu.mak  |   4 +
 hw/acpi/Makefile.objs            |   1 +
 hw/acpi/ged.c                    | 198 +++++++++++++++++++++++++++++++++++++++
 hw/acpi/memory_hotplug.c         |  58 ++++--------
 hw/arm/Makefile.objs             |   2 +-
 hw/arm/boot.c                    |  42 +++++++++
 hw/arm/virt-acpi-build.c         |  31 ++++++
 hw/arm/virt-acpi.c               | 161 +++++++++++++++++++++++++++++++
 hw/arm/virt.c                    |  93 +++++++++++++++++-
 hw/i386/acpi-build.c             |   3 +-
 include/hw/acpi/ged.h            |  61 ++++++++++++
 include/hw/acpi/memory_hotplug.h |  31 +++++-
 include/hw/arm/virt.h            |   8 ++
 13 files changed, 649 insertions(+), 44 deletions(-)
 create mode 100644 hw/acpi/ged.c
 create mode 100644 hw/arm/virt-acpi.c
 create mode 100644 include/hw/acpi/ged.h

-- 
2.7.4

[Qemu-devel] [PATCH v2 01/11] hw/acpi: Move constant definitions to header files

[Shameer Kolothum]

From: Sebastien Boeuf <sebastien.boeuf@intel.com>

By moving the definition of memory hotplug related constants used
by ACPI for both CPU and memory, this commits allows those to be
used from  other parts of the code.

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/acpi/memory_hotplug.c         | 26 --------------------------
 include/hw/acpi/memory_hotplug.h | 26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index 921cad2..a6beb10 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -8,32 +8,6 @@
 #include "qapi/error.h"
 #include "qapi/qapi-events-misc.h"
 
-#define MEMORY_SLOTS_NUMBER          "MDNR"
-#define MEMORY_HOTPLUG_IO_REGION     "HPMR"
-#define MEMORY_SLOT_ADDR_LOW         "MRBL"
-#define MEMORY_SLOT_ADDR_HIGH        "MRBH"
-#define MEMORY_SLOT_SIZE_LOW         "MRLL"
-#define MEMORY_SLOT_SIZE_HIGH        "MRLH"
-#define MEMORY_SLOT_PROXIMITY        "MPX"
-#define MEMORY_SLOT_ENABLED          "MES"
-#define MEMORY_SLOT_INSERT_EVENT     "MINS"
-#define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
-#define MEMORY_SLOT_EJECT            "MEJ"
-#define MEMORY_SLOT_SLECTOR          "MSEL"
-#define MEMORY_SLOT_OST_EVENT        "MOEV"
-#define MEMORY_SLOT_OST_STATUS       "MOSC"
-#define MEMORY_SLOT_LOCK             "MLCK"
-#define MEMORY_SLOT_STATUS_METHOD    "MRST"
-#define MEMORY_SLOT_CRS_METHOD       "MCRS"
-#define MEMORY_SLOT_OST_METHOD       "MOST"
-#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 ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index 77c6576..fbfcbe6 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -5,6 +5,32 @@
 #include "hw/acpi/acpi.h"
 #include "hw/acpi/aml-build.h"
 
+#define MEMORY_SLOTS_NUMBER          "MDNR"
+#define MEMORY_HOTPLUG_IO_REGION     "HPMR"
+#define MEMORY_SLOT_ADDR_LOW         "MRBL"
+#define MEMORY_SLOT_ADDR_HIGH        "MRBH"
+#define MEMORY_SLOT_SIZE_LOW         "MRLL"
+#define MEMORY_SLOT_SIZE_HIGH        "MRLH"
+#define MEMORY_SLOT_PROXIMITY        "MPX"
+#define MEMORY_SLOT_ENABLED          "MES"
+#define MEMORY_SLOT_INSERT_EVENT     "MINS"
+#define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
+#define MEMORY_SLOT_EJECT            "MEJ"
+#define MEMORY_SLOT_SLECTOR          "MSEL"
+#define MEMORY_SLOT_OST_EVENT        "MOEV"
+#define MEMORY_SLOT_OST_STATUS       "MOSC"
+#define MEMORY_SLOT_LOCK             "MLCK"
+#define MEMORY_SLOT_STATUS_METHOD    "MRST"
+#define MEMORY_SLOT_CRS_METHOD       "MCRS"
+#define MEMORY_SLOT_OST_METHOD       "MOST"
+#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"
+
 /**
  * MemStatus:
  * @is_removing: the memory device in slot has been requested to be ejected.
-- 
2.7.4

[Qemu-devel] [PATCH v2 02/11] hw/acpi: Make ACPI IO address space configurable

[Shameer Kolothum]

This is in preparation for adding support for ARM64 platforms
where it doesn't use port mapped IO for ACPI IO space.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/acpi/memory_hotplug.c         | 22 ++++++++++++++--------
 hw/i386/acpi-build.c             |  3 ++-
 include/hw/acpi/memory_hotplug.h |  5 +++--
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
index a6beb10..77ff0af 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -8,7 +8,7 @@
 #include "qapi/error.h"
 #include "qapi/qapi-events-misc.h"
 
-static uint16_t memhp_io_base;
+static hwaddr memhp_io_base;
 
 static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
 {
@@ -182,7 +182,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());
 
@@ -315,7 +315,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;
@@ -338,14 +339,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 622ccb9..d9b554f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1852,7 +1852,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
         build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
                        "\\_SB.PCI0", "\\_GPE._E02");
     }
-    build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0", "\\_GPE._E03");
+    build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
+                             "\\_GPE._E03", AML_SYSTEM_IO);
 
     scope =  aml_scope("_GPE");
     {
diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
index fbfcbe6..52f9027 100644
--- a/include/hw/acpi/memory_hotplug.h
+++ b/include/hw/acpi/memory_hotplug.h
@@ -55,7 +55,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);
@@ -74,5 +74,6 @@ void acpi_memory_ospm_status(MemHotplugState *mem_st, ACPIOSTInfoList ***list);
 
 void build_memory_hotplug_aml(Aml *table, uint32_t nr_mem,
                               const char *res_root,
-                              const char *event_handler_method);
+                              const char *event_handler_method,
+                              AmlRegionSpace rs);
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH v2 03/11] hw/acpi: Do not create memory hotplug method when handler is not defined

[Shameer Kolothum]

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>
---
 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 77ff0af..b138bef 100644
--- a/hw/acpi/memory_hotplug.c
+++ b/hw/acpi/memory_hotplug.c
@@ -695,10 +695,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.7.4

[Qemu-devel] [PATCH v2 04/11] hw/arm/virt: Add virtual ACPI device

[Shameer Kolothum]

From: Samuel Ortiz <sameo@linux.intel.com>

This is to provide an acpi device interface for Arm/virt.
This will be used by Arm/Virt to add hotplug support via
ACPI GED device.

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/Makefile.objs  |   2 +-
 hw/arm/virt-acpi.c    | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h |   1 +
 3 files changed, 113 insertions(+), 1 deletion(-)
 create mode 100644 hw/arm/virt-acpi.c

diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index fa57c7c..e0db3cd 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -1,6 +1,6 @@
 obj-y += boot.o sysbus-fdt.o
 obj-$(CONFIG_ARM_VIRT) += virt.o
-obj-$(CONFIG_ACPI) += virt-acpi-build.o
+obj-$(CONFIG_ACPI) += virt-acpi-build.o virt-acpi.o
 obj-$(CONFIG_DIGIC) += digic_boards.o
 obj-$(CONFIG_EXYNOS4) += exynos4_boards.o
 obj-$(CONFIG_HIGHBANK) += highbank.o
diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
new file mode 100644
index 0000000..df8a02b
--- /dev/null
+++ b/hw/arm/virt-acpi.c
@@ -0,0 +1,111 @@
+/*
+ *
+ * Copyright (c) 2018 Intel Corporation
+ *
+ * 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.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/range.h"
+#include "qapi/error.h"
+#include "exec/address-spaces.h"
+
+#include "hw/hw.h"
+#include "hw/hotplug.h"
+#include "hw/sysbus.h"
+#include "hw/arm/virt.h"
+
+#include "hw/acpi/acpi.h"
+
+typedef struct VirtAcpiState {
+    SysBusDevice parent_obj;
+} VirtAcpiState;
+
+#define TYPE_VIRT_ACPI "virt-acpi"
+#define VIRT_ACPI(obj) \
+    OBJECT_CHECK(VirtAcpiState, (obj), TYPE_VIRT_ACPI)
+
+static const VMStateDescription vmstate_acpi = {
+    .name = "virt_acpi",
+    .version_id = 1,
+    .minimum_version_id = 1,
+};
+
+static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
+                                DeviceState *dev, Error **errp)
+{
+}
+
+static void virt_device_unplug_request_cb(HotplugHandler *hotplug_dev,
+                                          DeviceState *dev, Error **errp)
+{
+}
+
+static void virt_device_unplug_cb(HotplugHandler *hotplug_dev,
+                                  DeviceState *dev, Error **errp)
+{
+}
+
+static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
+{
+}
+
+static void virt_device_realize(DeviceState *dev, Error **errp)
+{
+}
+
+DeviceState *virt_acpi_init(void)
+{
+    return sysbus_create_simple(TYPE_VIRT_ACPI, -1, NULL);
+}
+
+static Property virt_acpi_properties[] = {
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virt_acpi_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";
+    dc->vmsd = &vmstate_acpi;
+    dc->props = virt_acpi_properties;
+    dc->realize = virt_device_realize;
+
+    hc->plug = virt_device_plug_cb;
+    hc->unplug_request = virt_device_unplug_request_cb;
+    hc->unplug = virt_device_unplug_cb;
+
+    adevc->send_event = virt_send_ged;
+}
+
+static const TypeInfo virt_acpi_info = {
+    .name          = TYPE_VIRT_ACPI,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(VirtAcpiState),
+    .class_init    = virt_acpi_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_HOTPLUG_HANDLER },
+        { TYPE_ACPI_DEVICE_IF },
+        { }
+    }
+};
+
+static void virt_acpi_register_types(void)
+{
+    type_register_static(&virt_acpi_info);
+}
+
+type_init(virt_acpi_register_types)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 507517c..6076167 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -145,6 +145,7 @@ typedef struct {
     OBJECT_CLASS_CHECK(VirtMachineClass, klass, TYPE_VIRT_MACHINE)
 
 void virt_acpi_setup(VirtMachineState *vms);
+DeviceState *virt_acpi_init(void);
 
 /* Return the number of used redistributor regions  */
 static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
-- 
2.7.4

[Qemu-devel] [PATCH v2 05/11] hw/arm/virt: Add memory hotplug framework

[Shameer Kolothum]

From: Eric Auger <eric.auger@redhat.com>

This patch adds the the memory hot-plug/hot-unplug infrastructure
in machvirt. It is still not enabled as no device memory is allocated.

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>
---
 default-configs/arm-softmmu.mak |  2 ++
 hw/arm/virt.c                   | 53 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index bd6943b..fbc4564 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -165,3 +165,5 @@ CONFIG_PCI_EXPRESS_DESIGNWARE=y
 CONFIG_STRONGARM=y
 CONFIG_HIGHBANK=y
 CONFIG_MUSICPAL=y
+CONFIG_MEM_DEVICE=y
+CONFIG_DIMM=y
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c7fb534..40a1273 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -61,6 +61,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, \
@@ -1810,6 +1812,42 @@ 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)
+{
+    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
+
+    if (dev->hotplugged) {
+        error_setg(errp, "memory hotplug is not supported");
+    }
+
+    if (is_nvdimm) {
+        error_setg(errp, "nvdimm 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);
+    Error *local_err = NULL;
+
+    pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
+
+    error_propagate(errp, local_err);
+}
+
+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)
 {
@@ -1821,12 +1859,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);
     }
 
@@ -1890,7 +1939,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;
 }
 
 static void virt_instance_init(Object *obj)
-- 
2.7.4

[Qemu-devel] [PATCH v2 06/11] hw/arm/virt: Add ACPI support for device memory cold-plug

[Shameer Kolothum]

This adds support to build the aml code so that Guest can see the
cold-plugged device memory.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 default-configs/arm-softmmu.mak |  1 +
 hw/arm/virt-acpi-build.c        |  9 +++++++++
 hw/arm/virt-acpi.c              | 23 +++++++++++++++++++++++
 hw/arm/virt.c                   | 11 +++++++++++
 include/hw/arm/virt.h           |  2 ++
 5 files changed, 46 insertions(+)

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index fbc4564..b3bac25 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -167,3 +167,4 @@ CONFIG_HIGHBANK=y
 CONFIG_MUSICPAL=y
 CONFIG_MEM_DEVICE=y
 CONFIG_DIMM=y
+CONFIG_ACPI_MEMORY_HOTPLUG=y
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index d7e2e48..87d66da 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -40,6 +40,7 @@
 #include "hw/loader.h"
 #include "hw/hw.h"
 #include "hw/acpi/aml-build.h"
+#include "hw/acpi/memory_hotplug.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
@@ -49,6 +50,13 @@
 #define ARM_SPI_BASE 32
 #define ACPI_POWER_BUTTON_DEVICE "PWRB"
 
+static void acpi_dsdt_add_memory_hotplug(Aml *scope, MachineState *ms)
+{
+    uint32_t nr_mem = ms->ram_slots;
+
+    build_memory_hotplug_aml(scope, nr_mem, "\\_SB", NULL, AML_SYSTEM_MEMORY);
+}
+
 static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
 {
     uint16_t i;
@@ -740,6 +748,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
      * the RTC ACPI device at all when using UEFI.
      */
     scope = aml_scope("\\_SB");
+    acpi_dsdt_add_memory_hotplug(scope, MACHINE(vms));
     acpi_dsdt_add_cpus(scope, vms->smp_cpus);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
index df8a02b..18ebe94 100644
--- a/hw/arm/virt-acpi.c
+++ b/hw/arm/virt-acpi.c
@@ -26,9 +26,11 @@
 #include "hw/arm/virt.h"
 
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/memory_hotplug.h"
 
 typedef struct VirtAcpiState {
     SysBusDevice parent_obj;
+    MemHotplugState memhp_state;
 } VirtAcpiState;
 
 #define TYPE_VIRT_ACPI "virt-acpi"
@@ -44,6 +46,16 @@ static const VMStateDescription vmstate_acpi = {
 static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
                                 DeviceState *dev, Error **errp)
 {
+    VirtAcpiState *s = VIRT_ACPI(hotplug_dev);
+
+    if (s->memhp_state.is_enabled &&
+        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+            acpi_memory_plug_cb(hotplug_dev, &s->memhp_state,
+                                dev, errp);
+    } else {
+        error_setg(errp, "virt: device plug request for unsupported device"
+                   " type: %s", object_get_typename(OBJECT(dev)));
+    }
 }
 
 static void virt_device_unplug_request_cb(HotplugHandler *hotplug_dev,
@@ -62,6 +74,15 @@ static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
 
 static void virt_device_realize(DeviceState *dev, Error **errp)
 {
+    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
+    const MemMapEntry *memmap = vms->memmap;
+    VirtAcpiState *s = VIRT_ACPI(dev);
+
+    if (s->memhp_state.is_enabled) {
+        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
+                                 &s->memhp_state,
+                                 memmap[VIRT_PCDIMM_ACPI].base);
+    }
 }
 
 DeviceState *virt_acpi_init(void)
@@ -70,6 +91,8 @@ DeviceState *virt_acpi_init(void)
 }
 
 static Property virt_acpi_properties[] = {
+    DEFINE_PROP_BOOL("memory-hotplug-support", VirtAcpiState,
+                     memhp_state.is_enabled, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 40a1273..9427f4f 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -133,6 +133,7 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
+    [VIRT_PCDIMM_ACPI] =        { 0x09070000, 0x00010000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -1648,6 +1649,8 @@ static void machvirt_init(MachineState *machine)
 
     create_platform_bus(vms, pic);
 
+    vms->acpi = virt_acpi_init();
+
     vms->bootinfo.ram_size = machine->ram_size;
     vms->bootinfo.kernel_filename = machine->kernel_filename;
     vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
@@ -1832,11 +1835,19 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 static void virt_memory_plug(HotplugHandler *hotplug_dev,
                              DeviceState *dev, Error **errp)
 {
+    HotplugHandlerClass *hhc;
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
     Error *local_err = NULL;
 
     pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
+    if (local_err) {
+        goto out;
+    }
+
+    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi);
+    hhc->plug(HOTPLUG_HANDLER(vms->acpi), dev, &error_abort);
 
+out:
     error_propagate(errp, local_err);
 }
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 6076167..e46a051 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -77,6 +77,7 @@ enum {
     VIRT_GPIO,
     VIRT_SECURE_UART,
     VIRT_SECURE_MEM,
+    VIRT_PCDIMM_ACPI,
     VIRT_LOWMEMMAP_LAST,
 };
 
@@ -132,6 +133,7 @@ typedef struct {
     uint32_t iommu_phandle;
     int psci_conduit;
     hwaddr highest_gpa;
+    DeviceState *acpi;
 } VirtMachineState;
 
 #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
-- 
2.7.4

[Qemu-devel] [PATCH v2 07/11] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT

[Shameer Kolothum]

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 87d66da..6cb7263 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -524,6 +524,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;
@@ -549,6 +550,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.7.4

[Qemu-devel] [PATCH v2 08/11] hw/arm/boot: Expose the PC-DIMM nodes in the DT

[Shameer Kolothum]

This patch adds memory nodes corresponding to PC-DIMM regions.

NVDIMM and ACPI_NVDIMM configs are not yet set for ARM so we
don't need to care about NVDIMM at this stage.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/boot.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index a830655..4caaf91 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -19,6 +19,7 @@
 #include "sysemu/numa.h"
 #include "hw/boards.h"
 #include "hw/loader.h"
+#include "hw/mem/memory-device.h"
 #include "elf.h"
 #include "sysemu/device_tree.h"
 #include "qemu/config-file.h"
@@ -522,6 +523,41 @@ static void fdt_add_psci_node(void *fdt)
     qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
 }
 
+static int fdt_add_hotpluggable_memory_nodes(void *fdt,
+                                             uint32_t acells, uint32_t scells) {
+    MemoryDeviceInfoList *info, *info_list = qmp_memory_device_list();
+    MemoryDeviceInfo *mi;
+    int ret = 0;
+
+    for (info = info_list; info != NULL; info = info->next) {
+        mi = info->value;
+        switch (mi->type) {
+        case MEMORY_DEVICE_INFO_KIND_DIMM:
+        {
+            PCDIMMDeviceInfo *di = mi->u.dimm.data;
+
+            ret = fdt_add_memory_node(fdt, acells, di->addr,
+                                      scells, di->size, di->node);
+            if (ret) {
+                fprintf(stderr,
+                        "couldn't add PCDIMM /memory@%"PRIx64" node\n",
+                        di->addr);
+                goto out;
+            }
+            break;
+        }
+        default:
+            fprintf(stderr, "%s memory nodes are not yet supported\n",
+                    MemoryDeviceInfoKind_str(mi->type));
+            ret = -ENOENT;
+            goto out;
+        }
+    }
+out:
+    qapi_free_MemoryDeviceInfoList(info_list);
+    return ret;
+}
+
 int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
                  hwaddr addr_limit, AddressSpace *as)
 {
@@ -621,6 +657,12 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
         }
     }
 
+    rc = fdt_add_hotpluggable_memory_nodes(fdt, acells, scells);
+    if (rc < 0) {
+        fprintf(stderr, "couldn't add hotpluggable memory nodes\n");
+        goto fail;
+    }
+
     rc = fdt_path_offset(fdt, "/chosen");
     if (rc < 0) {
         qemu_fdt_add_subnode(fdt, "/chosen");
-- 
2.7.4

[Qemu-devel] [PATCH v2 09/11] hw/acpi: Add ACPI Generic Event Device Support

[Shameer Kolothum]

From: Samuel Ortiz <sameo@linux.intel.com>

The ACPI Generic Event Device (GED) is a hardware-reduced specific
device that handles all platform events, including the hotplug ones.
This patch generate the AML code that defines GEDs.
Platforms need to specify their own GedEvent array to describe what kind
of events they want to support through GED. The build_ged_aml routine
takes a GedEvent array that maps a specific GED event to an IRQ number.
Then we use that array to build both the _CRS and the _EVT section
of the GED device.

This is in preparation for making use of GED for ARM/virt
platform and for now supports only memory hotplug.

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>
---
 hw/acpi/Makefile.objs |   1 +
 hw/acpi/ged.c         | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/ged.h |  61 ++++++++++++++++
 3 files changed, 260 insertions(+)
 create mode 100644 hw/acpi/ged.c
 create mode 100644 include/hw/acpi/ged.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 2d46e37..6cf572b 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) += ged.o
 common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
 
 common-obj-y += acpi_interface.o
diff --git a/hw/acpi/ged.c b/hw/acpi/ged.c
new file mode 100644
index 0000000..5076fbc
--- /dev/null
+++ b/hw/acpi/ged.c
@@ -0,0 +1,198 @@
+/*
+ *
+ * Copyright (c) 2018 Intel Corporation
+ *
+ * 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.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "hw/acpi/ged.h"
+
+static hwaddr ged_io_base;
+
+static uint64_t ged_read(void *opaque, hwaddr addr, unsigned size)
+{
+    uint64_t val = 0;
+    GEDState *ged_st = opaque;
+
+    switch (addr) {
+    case ACPI_GED_IRQ_SEL_OFFSET:
+        /* Read the selector value and reset it */
+        qemu_mutex_lock(&ged_st->lock);
+        val = ged_st->sel;
+        ged_st->sel = ACPI_GED_IRQ_SEL_INIT;
+        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,
+    },
+};
+
+void acpi_ged_init(MemoryRegion *as, Object *owner, GEDState *ged_st,
+                   hwaddr base_addr, uint32_t ged_irq)
+{
+
+    assert(!ged_io_base);
+
+    ged_io_base = base_addr;
+    ged_st->irq = ged_irq;
+    qemu_mutex_init(&ged_st->lock);
+    memory_region_init_io(&ged_st->io, owner, &ged_ops, ged_st,
+                          "acpi-ged-event", ACPI_GED_REG_LEN);
+    memory_region_add_subregion(as, base_addr, &ged_st->io);
+}
+
+void acpi_ged_event(GEDState *ged_st, qemu_irq *irq, uint32_t ged_irq_sel)
+{
+    /*
+     * 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 |= ged_irq_sel;
+    qemu_mutex_unlock(&ged_st->lock);
+
+    /* Trigger the event by sending an interrupt to the guest. */
+    qemu_irq_pulse(irq[ged_st->irq]);
+}
+
+static Aml *ged_event_aml(GedEvent *event)
+{
+
+    if (!event) {
+        return NULL;
+    }
+
+    switch (event->event) {
+    case GED_MEMORY_HOTPLUG:
+        /* We run a complete memory SCAN when getting a memory hotplug event */
+        return aml_call0("\\_SB.MHPC." MEMORY_SLOT_SCAN_METHOD);
+    default:
+        break;
+    }
+
+    return NULL;
+}
+
+void build_ged_aml(Aml *table, const char *name, uint32_t ged_irq,
+                   GedEvent *events, uint32_t events_size,
+                   AmlRegionSpace rs)
+{
+    Aml *crs = aml_resource_template();
+    Aml *evt, *field;
+    Aml *zero = aml_int(0);
+    Aml *dev = aml_device("%s", name);
+    Aml *irq_sel = aml_local(0);
+    Aml *isel = aml_name(AML_GED_IRQ_SEL);
+    uint32_t i;
+
+    if (!ged_io_base) {
+        return;
+    }
+
+    /* _CRS interrupt */
+    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
+                                  AML_EXCLUSIVE, &ged_irq, 1));
+    /*
+     * For each GED event we:
+     * - Add an interrupt to the CRS section.
+     * - Add a conditional block for each event, inside a while loop.
+     *   This is semantically equivalent to a switch/case implementation.
+     */
+    evt = aml_method("_EVT", 1, AML_SERIALIZED);
+    {
+        Aml *ged_aml;
+        Aml *if_ctx;
+
+        /* Local0 = ISEL */
+        aml_append(evt, aml_store(isel, irq_sel));
+
+        /*
+         * Here we want to call a method for each supported GED event type.
+         * The resulting ASL code looks like:
+         *
+         * Local0 = ISEL
+         * If ((Local0 & irq0) == irq0)
+         * {
+         *     MethodEvent0()
+         * }
+         *
+         * If ((Local0 & irq1) == irq1)
+         * {
+         *     MethodEvent1()
+         * }
+         *
+         * If ((Local0 & irq2) == irq2)
+         * {
+         *     MethodEvent2()
+         * }
+         */
+
+        for (i = 0; i < events_size; i++) {
+            ged_aml = ged_event_aml(&events[i]);
+            if (!ged_aml) {
+                continue;
+            }
+
+            /* If ((Local1 == irq))*/
+            if_ctx = aml_if(aml_equal(aml_and(irq_sel, aml_int(events[i].selector), NULL), aml_int(events[i].selector)));
+            {
+                /* AML for this specific type of event */
+                aml_append(if_ctx, ged_aml);
+            }
+
+            /*
+             * We append the first if to the while context.
+             * Other ifs will be elseifs.
+             */
+            aml_append(evt, if_ctx);
+        }
+    }
+
+    aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0013")));
+    aml_append(dev, aml_name_decl("_UID", zero));
+    aml_append(dev, aml_name_decl("_CRS", crs));
+
+    /* Append IO region */
+    aml_append(dev, aml_operation_region(AML_GED_IRQ_REG, rs,
+               aml_int(ged_io_base + ACPI_GED_IRQ_SEL_OFFSET),
+               ACPI_GED_IRQ_SEL_LEN));
+    field = aml_field(AML_GED_IRQ_REG, AML_DWORD_ACC, AML_NOLOCK,
+                      AML_WRITE_AS_ZEROS);
+    aml_append(field, aml_named_field(AML_GED_IRQ_SEL,
+                                      ACPI_GED_IRQ_SEL_LEN * 8));
+    aml_append(dev, field);
+
+    /* Append _EVT method */
+    aml_append(dev, evt);
+
+    aml_append(table, dev);
+}
diff --git a/include/hw/acpi/ged.h b/include/hw/acpi/ged.h
new file mode 100644
index 0000000..60689b0
--- /dev/null
+++ b/include/hw/acpi/ged.h
@@ -0,0 +1,61 @@
+/*
+ *
+ * Copyright (c) 2018 Intel Corporation
+ *
+ * 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.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HW_ACPI_GED_H
+#define HW_ACPI_GED_H
+
+#include "qemu/osdep.h"
+#include "exec/memory.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/memory_hotplug.h"
+
+#define ACPI_GED_IRQ_SEL_OFFSET 0x0
+#define ACPI_GED_IRQ_SEL_LEN    0x4
+#define ACPI_GED_IRQ_SEL_INIT   0x0
+#define ACPI_GED_IRQ_SEL_MEM    0x1
+#define ACPI_GED_REG_LEN        0x4
+
+#define GED_DEVICE      "GED"
+#define AML_GED_IRQ_REG "IREG"
+#define AML_GED_IRQ_SEL "ISEL"
+
+typedef struct Aml Aml;
+
+typedef enum {
+    GED_MEMORY_HOTPLUG = 1,
+} GedEventType;
+
+typedef struct GedEvent {
+    uint32_t     selector;
+    GedEventType event;
+} GedEvent;
+
+typedef struct GEDState {
+    MemoryRegion io;
+    uint32_t     sel;
+    uint32_t     irq;
+    QemuMutex    lock;
+} GEDState;
+
+void acpi_ged_init(MemoryRegion *as, Object *owner, GEDState *ged_st,
+                   hwaddr base_addr, uint32_t ged_irq);
+void acpi_ged_event(GEDState *ged_st, qemu_irq *irq, uint32_t ged_irq_sel);
+void build_ged_aml(Aml *table, const char* name, uint32_t ged_irq,
+                   GedEvent *events, uint32_t events_size,
+                   AmlRegionSpace rs);
+
+#endif
-- 
2.7.4

[Qemu-devel] [PATCH v2 10/11] hw/arm/virt: Add GED device configuration and build aml

[Shameer Kolothum]

This initializes the GED device with base memory and irq.
It also configures ged memory hotplug event and builds the
corresponding aml code.

But ged irq routing to Guest is not enabled and thus hotplug
is not yet supported.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 default-configs/arm-softmmu.mak |  1 +
 hw/arm/virt-acpi-build.c        | 13 +++++++++++++
 hw/arm/virt-acpi.c              |  4 ++++
 hw/arm/virt.c                   | 22 ++++++++++++++++++++++
 include/hw/arm/virt.h           |  4 ++++
 5 files changed, 44 insertions(+)

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index b3bac25..7c442fd 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -168,3 +168,4 @@ CONFIG_MUSICPAL=y
 CONFIG_MEM_DEVICE=y
 CONFIG_DIMM=y
 CONFIG_ACPI_MEMORY_HOTPLUG=y
+CONFIG_ACPI_HW_REDUCED=y
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 6cb7263..86f25ad 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -50,6 +50,18 @@
 #define ARM_SPI_BASE 32
 #define ACPI_POWER_BUTTON_DEVICE "PWRB"
 
+static void acpi_dsdt_add_ged(Aml *scope, VirtMachineState *vms)
+{
+    int irq =  vms->irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE;
+
+    if (!vms->ged_events || !vms->ged_events_size) {
+        return;
+    }
+
+    build_ged_aml(scope, "\\_SB."GED_DEVICE, irq, vms->ged_events,
+                  vms->ged_events_size, AML_SYSTEM_MEMORY);
+}
+
 static void acpi_dsdt_add_memory_hotplug(Aml *scope, MachineState *ms)
 {
     uint32_t nr_mem = ms->ram_slots;
@@ -758,6 +770,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
      */
     scope = aml_scope("\\_SB");
     acpi_dsdt_add_memory_hotplug(scope, MACHINE(vms));
+    acpi_dsdt_add_ged(scope, vms);
     acpi_dsdt_add_cpus(scope, vms->smp_cpus);
     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
index 18ebe94..3b55c63 100644
--- a/hw/arm/virt-acpi.c
+++ b/hw/arm/virt-acpi.c
@@ -31,6 +31,7 @@
 typedef struct VirtAcpiState {
     SysBusDevice parent_obj;
     MemHotplugState memhp_state;
+    GEDState ged_state;
 } VirtAcpiState;
 
 #define TYPE_VIRT_ACPI "virt-acpi"
@@ -76,12 +77,15 @@ static void virt_device_realize(DeviceState *dev, Error **errp)
 {
     VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
     const MemMapEntry *memmap = vms->memmap;
+    const int *irqmap = vms->irqmap;
     VirtAcpiState *s = VIRT_ACPI(dev);
 
     if (s->memhp_state.is_enabled) {
         acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
                                  &s->memhp_state,
                                  memmap[VIRT_PCDIMM_ACPI].base);
+        acpi_ged_init(get_system_memory(), OBJECT(dev), &s->ged_state,
+                      memmap[VIRT_ACPI_GED].base, irqmap[VIRT_ACPI_GED]);
     }
 }
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9427f4f..352dbb1 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -134,6 +134,7 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
     [VIRT_PCDIMM_ACPI] =        { 0x09070000, 0x00010000 },
+    [VIRT_ACPI_GED] =           { 0x09080000, 0x00010000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -169,6 +170,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 */
@@ -184,6 +186,25 @@ static const char *valid_cpus[] = {
     ARM_CPU_TYPE_NAME("max"),
 };
 
+static void virt_acpi_ged_conf(VirtMachineState *vms)
+{
+    uint8_t events_size;
+
+    /* GED events */
+    GedEvent events[] = {
+        {
+            .selector = ACPI_GED_IRQ_SEL_MEM,
+            .event    = GED_MEMORY_HOTPLUG,
+        },
+    };
+
+    events_size = ARRAY_SIZE(events);
+
+    vms->ged_events = g_malloc0(events_size * sizeof(GedEvent));
+    memcpy(vms->ged_events, events, events_size * sizeof(GedEvent));
+    vms->ged_events_size = events_size;
+}
+
 static bool cpu_type_valid(const char *cpu)
 {
     int i;
@@ -1650,6 +1671,7 @@ static void machvirt_init(MachineState *machine)
     create_platform_bus(vms, pic);
 
     vms->acpi = virt_acpi_init();
+    virt_acpi_ged_conf(vms);
 
     vms->bootinfo.ram_size = machine->ram_size;
     vms->bootinfo.kernel_filename = machine->kernel_filename;
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index e46a051..49fda81 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -37,6 +37,7 @@
 #include "hw/arm/arm.h"
 #include "sysemu/kvm.h"
 #include "hw/intc/arm_gicv3_common.h"
+#include "hw/acpi/ged.h"
 
 #define NUM_GICV2M_SPIS       64
 #define NUM_VIRTIO_TRANSPORTS 32
@@ -78,6 +79,7 @@ enum {
     VIRT_SECURE_UART,
     VIRT_SECURE_MEM,
     VIRT_PCDIMM_ACPI,
+    VIRT_ACPI_GED,
     VIRT_LOWMEMMAP_LAST,
 };
 
@@ -134,6 +136,8 @@ typedef struct {
     int psci_conduit;
     hwaddr highest_gpa;
     DeviceState *acpi;
+    GedEvent *ged_events;
+    uint32_t ged_events_size;
 } VirtMachineState;
 
 #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
-- 
2.7.4

[Qemu-devel] [PATCH v2 11/11] hw/arm/virt: Add GED irq routing and Enable memory hotplug

[Shameer Kolothum]

Adds GED irq routing to guest and enables memory hotplug.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 hw/arm/virt-acpi.c    | 27 +++++++++++++++++++++++++--
 hw/arm/virt.c         | 17 +++++++++++------
 include/hw/arm/virt.h |  3 ++-
 3 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
index 3b55c63..c5a9311 100644
--- a/hw/arm/virt-acpi.c
+++ b/hw/arm/virt-acpi.c
@@ -32,6 +32,7 @@ typedef struct VirtAcpiState {
     SysBusDevice parent_obj;
     MemHotplugState memhp_state;
     GEDState ged_state;
+    qemu_irq *gsi;
 } VirtAcpiState;
 
 #define TYPE_VIRT_ACPI "virt-acpi"
@@ -71,6 +72,21 @@ static void virt_device_unplug_cb(HotplugHandler *hotplug_dev,
 
 static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
 {
+    VirtAcpiState *s = VIRT_ACPI(adev);
+    uint32_t sel = ACPI_GED_IRQ_SEL_INIT;
+
+    if (ev & ACPI_MEMORY_HOTPLUG_STATUS) {
+        sel = ACPI_GED_IRQ_SEL_MEM;
+    } else {
+        /* Unknown event. Return without generating interrupt. */
+        return;
+    }
+
+    /*
+     * We inject the hotplug interrupt. The IRQ selector will make
+     * the difference from the ACPI table.
+     */
+    acpi_ged_event(&s->ged_state, s->gsi, sel);
 }
 
 static void virt_device_realize(DeviceState *dev, Error **errp)
@@ -89,9 +105,16 @@ static void virt_device_realize(DeviceState *dev, Error **errp)
     }
 }
 
-DeviceState *virt_acpi_init(void)
+DeviceState *virt_acpi_init(qemu_irq *gsi)
 {
-    return sysbus_create_simple(TYPE_VIRT_ACPI, -1, NULL);
+    DeviceState *dev;
+    VirtAcpiState *s;
+
+    dev = sysbus_create_simple(TYPE_VIRT_ACPI, -1, NULL);
+    s = VIRT_ACPI(dev);
+    s->gsi = gsi;
+
+    return dev;
 }
 
 static Property virt_acpi_properties[] = {
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 352dbb1..fa352c5 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -577,6 +577,12 @@ static void create_v2m(VirtMachineState *vms, qemu_irq *pic)
     fdt_add_v2m_gic_node(vms);
 }
 
+static void virt_gsi_handler(void *opaque, int n, int level)
+{
+    qemu_irq *gic_irq = opaque;
+    qemu_set_irq(gic_irq[n], level);
+}
+
 static void create_gic(VirtMachineState *vms, qemu_irq *pic)
 {
     /* We create a standalone GIC */
@@ -692,6 +698,8 @@ static void create_gic(VirtMachineState *vms, qemu_irq *pic)
         pic[i] = qdev_get_gpio_in(gicdev, i);
     }
 
+    vms->gsi = qemu_allocate_irqs(virt_gsi_handler, pic, NUM_IRQS);
+
     fdt_add_gic_node(vms);
 
     if (type == 3 && vms->its) {
@@ -1444,7 +1452,7 @@ static void machvirt_init(MachineState *machine)
     VirtMachineClass *vmc = VIRT_MACHINE_GET_CLASS(machine);
     MachineClass *mc = MACHINE_GET_CLASS(machine);
     const CPUArchIdList *possible_cpus;
-    qemu_irq pic[NUM_IRQS];
+    qemu_irq *pic;
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *secure_sysmem = NULL;
     int n, virt_max_cpus;
@@ -1640,6 +1648,7 @@ static void machvirt_init(MachineState *machine)
 
     create_flash(vms, sysmem, secure_sysmem ? secure_sysmem : sysmem);
 
+    pic = g_new0(qemu_irq, NUM_IRQS);
     create_gic(vms, pic);
 
     fdt_add_pmu_nodes(vms);
@@ -1670,7 +1679,7 @@ static void machvirt_init(MachineState *machine)
 
     create_platform_bus(vms, pic);
 
-    vms->acpi = virt_acpi_init();
+    vms->acpi = virt_acpi_init(vms->gsi);
     virt_acpi_ged_conf(vms);
 
     vms->bootinfo.ram_size = machine->ram_size;
@@ -1842,10 +1851,6 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
 {
     const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    if (dev->hotplugged) {
-        error_setg(errp, "memory hotplug is not supported");
-    }
-
     if (is_nvdimm) {
         error_setg(errp, "nvdimm is not yet supported");
         return;
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 49fda81..30ff460 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -136,6 +136,7 @@ typedef struct {
     int psci_conduit;
     hwaddr highest_gpa;
     DeviceState *acpi;
+    qemu_irq *gsi;
     GedEvent *ged_events;
     uint32_t ged_events_size;
 } VirtMachineState;
@@ -151,7 +152,7 @@ typedef struct {
     OBJECT_CLASS_CHECK(VirtMachineClass, klass, TYPE_VIRT_MACHINE)
 
 void virt_acpi_setup(VirtMachineState *vms);
-DeviceState *virt_acpi_init(void);
+DeviceState *virt_acpi_init(qemu_irq *gsi);
 
 /* Return the number of used redistributor regions  */
 static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 00/11] ARM virt: ACPI memory hotplug support

[Auger Eric]

Hi Shameer,
On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> 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
s/NVDIM/NVDIMM
> 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 series can be applied on top of Eric's branch here[4].
> 
> 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] https://github.com/eauger/qemu/tree/v3.1.0-extended-memmap-v12

You have a minor conflict wrt upstream due to

7c28b925b7  build: convert pci.mak to Kconfig

which adds CONFIG_LSI_SCSI_PCI=y in default-configs/arm-softmmu.mak

Thanks

Eric

> 
> 
> RFC --> v2
> 
> -Use GED device instead of GPIO for ACPI hotplug events.
> -Removed NVDIMM support for now.
> -Includes dropped patches from Eric's v9 series.
> 
> 
> Eric Auger (1):
>   hw/arm/virt: Add memory hotplug framework
> 
> Samuel Ortiz (3):
>   hw/acpi: Do not create memory hotplug method when handler is not
>     defined
>   hw/arm/virt: Add virtual ACPI device
>   hw/acpi: Add ACPI Generic Event Device Support
> 
> Sebastien Boeuf (1):
>   hw/acpi: Move constant definitions to header files
> 
> Shameer Kolothum (6):
>   hw/acpi: Make ACPI IO address space configurable
>   hw/arm/virt: Add ACPI support for device memory cold-plug
>   hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
>   hw/arm/boot: Expose the PC-DIMM nodes in the DT
>   hw/arm/virt: Add GED device configuration and build aml
>   hw/arm/virt: Add GED irq routing and Enable memory hotplug
> 
>  default-configs/arm-softmmu.mak  |   4 +
>  hw/acpi/Makefile.objs            |   1 +
>  hw/acpi/ged.c                    | 198 +++++++++++++++++++++++++++++++++++++++
>  hw/acpi/memory_hotplug.c         |  58 ++++--------
>  hw/arm/Makefile.objs             |   2 +-
>  hw/arm/boot.c                    |  42 +++++++++
>  hw/arm/virt-acpi-build.c         |  31 ++++++
>  hw/arm/virt-acpi.c               | 161 +++++++++++++++++++++++++++++++
>  hw/arm/virt.c                    |  93 +++++++++++++++++-
>  hw/i386/acpi-build.c             |   3 +-
>  include/hw/acpi/ged.h            |  61 ++++++++++++
>  include/hw/acpi/memory_hotplug.h |  31 +++++-
>  include/hw/arm/virt.h            |   8 ++
>  13 files changed, 649 insertions(+), 44 deletions(-)
>  create mode 100644 hw/acpi/ged.c
>  create mode 100644 hw/arm/virt-acpi.c
>  create mode 100644 include/hw/acpi/ged.h
> 

Re: [Qemu-devel] [PATCH v2 00/11] ARM virt: ACPI memory hotplug support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 08 March 2019 15:54
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com;
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sameo@linux.intel.com; sebastien.boeuf@intel.com
> Cc: Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> Subject: Re: [PATCH v2 00/11] ARM virt: ACPI memory hotplug support
> 
> Hi Shameer,
> On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> > 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
> s/NVDIM/NVDIMM
> > 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 series can be applied on top of Eric's branch here[4].
> >
> > 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] https://github.com/eauger/qemu/tree/v3.1.0-extended-memmap-v12
> 
> You have a minor conflict wrt upstream due to
> 
> 7c28b925b7  build: convert pci.mak to Kconfig
> 
> which adds CONFIG_LSI_SCSI_PCI=y in default-configs/arm-softmmu.mak

Thanks Eric. Noted

Cheers,
Shameer

> >
> >
> > RFC --> v2
> >
> > -Use GED device instead of GPIO for ACPI hotplug events.
> > -Removed NVDIMM support for now.
> > -Includes dropped patches from Eric's v9 series.
> >
> >
> > Eric Auger (1):
> >   hw/arm/virt: Add memory hotplug framework
> >
> > Samuel Ortiz (3):
> >   hw/acpi: Do not create memory hotplug method when handler is not
> >     defined
> >   hw/arm/virt: Add virtual ACPI device
> >   hw/acpi: Add ACPI Generic Event Device Support
> >
> > Sebastien Boeuf (1):
> >   hw/acpi: Move constant definitions to header files
> >
> > Shameer Kolothum (6):
> >   hw/acpi: Make ACPI IO address space configurable
> >   hw/arm/virt: Add ACPI support for device memory cold-plug
> >   hw/arm/virt-acpi-build: Add PC-DIMM in SRAT
> >   hw/arm/boot: Expose the PC-DIMM nodes in the DT
> >   hw/arm/virt: Add GED device configuration and build aml
> >   hw/arm/virt: Add GED irq routing and Enable memory hotplug
> >
> >  default-configs/arm-softmmu.mak  |   4 +
> >  hw/acpi/Makefile.objs            |   1 +
> >  hw/acpi/ged.c                    | 198
> +++++++++++++++++++++++++++++++++++++++
> >  hw/acpi/memory_hotplug.c         |  58 ++++--------
> >  hw/arm/Makefile.objs             |   2 +-
> >  hw/arm/boot.c                    |  42 +++++++++
> >  hw/arm/virt-acpi-build.c         |  31 ++++++
> >  hw/arm/virt-acpi.c               | 161
> +++++++++++++++++++++++++++++++
> >  hw/arm/virt.c                    |  93 +++++++++++++++++-
> >  hw/i386/acpi-build.c             |   3 +-
> >  include/hw/acpi/ged.h            |  61 ++++++++++++
> >  include/hw/acpi/memory_hotplug.h |  31 +++++-
> >  include/hw/arm/virt.h            |   8 ++
> >  13 files changed, 649 insertions(+), 44 deletions(-)  create mode
> > 100644 hw/acpi/ged.c  create mode 100644 hw/arm/virt-acpi.c  create
> > mode 100644 include/hw/acpi/ged.h
> >

Re: [Qemu-devel] [PATCH v2 01/11] hw/acpi: Move constant definitions to header files

[Auger Eric]

Hi,

On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> 
> By moving the definition of memory hotplug related constants used
> by ACPI for both CPU and memory, this commits allows those to be
> used from  other parts of the code.

Maybe elaborate on where you intend to use them.

> 
> Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/acpi/memory_hotplug.c         | 26 --------------------------
>  include/hw/acpi/memory_hotplug.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index 921cad2..a6beb10 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -8,32 +8,6 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-misc.h"
>  
> -#define MEMORY_SLOTS_NUMBER          "MDNR"
> -#define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> -#define MEMORY_SLOT_ADDR_LOW         "MRBL"
> -#define MEMORY_SLOT_ADDR_HIGH        "MRBH"
> -#define MEMORY_SLOT_SIZE_LOW         "MRLL"
> -#define MEMORY_SLOT_SIZE_HIGH        "MRLH"
> -#define MEMORY_SLOT_PROXIMITY        "MPX"
> -#define MEMORY_SLOT_ENABLED          "MES"
> -#define MEMORY_SLOT_INSERT_EVENT     "MINS"
> -#define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
> -#define MEMORY_SLOT_EJECT            "MEJ"
> -#define MEMORY_SLOT_SLECTOR          "MSEL"
> -#define MEMORY_SLOT_OST_EVENT        "MOEV"
> -#define MEMORY_SLOT_OST_STATUS       "MOSC"
> -#define MEMORY_SLOT_LOCK             "MLCK"
> -#define MEMORY_SLOT_STATUS_METHOD    "MRST"
> -#define MEMORY_SLOT_CRS_METHOD       "MCRS"
> -#define MEMORY_SLOT_OST_METHOD       "MOST"
> -#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"

Do we really need to expose all of them. I just can see
MEMORY_SLOT_SCAN_METHOD used in hw/acpi/ged.c? Maybe I missed some though?

Also at the beginning I tried to find some specification details for
those stuff but I failed. It would be helpful for a non specialist
reader to add a comment that helps to understand what is part of a
specification (link?) and what is arbitrarily defined.

Thanks

Eric
> -
>  static uint16_t memhp_io_base;
>  
>  static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index 77c6576..fbfcbe6 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -5,6 +5,32 @@
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/aml-build.h"
>  
> +#define MEMORY_SLOTS_NUMBER          "MDNR"
> +#define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> +#define MEMORY_SLOT_ADDR_LOW         "MRBL"
> +#define MEMORY_SLOT_ADDR_HIGH        "MRBH"
> +#define MEMORY_SLOT_SIZE_LOW         "MRLL"
> +#define MEMORY_SLOT_SIZE_HIGH        "MRLH"
> +#define MEMORY_SLOT_PROXIMITY        "MPX"
> +#define MEMORY_SLOT_ENABLED          "MES"
> +#define MEMORY_SLOT_INSERT_EVENT     "MINS"
> +#define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
> +#define MEMORY_SLOT_EJECT            "MEJ"
> +#define MEMORY_SLOT_SLECTOR          "MSEL"
> +#define MEMORY_SLOT_OST_EVENT        "MOEV"
> +#define MEMORY_SLOT_OST_STATUS       "MOSC"
> +#define MEMORY_SLOT_LOCK             "MLCK"
> +#define MEMORY_SLOT_STATUS_METHOD    "MRST"
> +#define MEMORY_SLOT_CRS_METHOD       "MCRS"
> +#define MEMORY_SLOT_OST_METHOD       "MOST"
> +#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"
> +
>  /**
>   * MemStatus:
>   * @is_removing: the memory device in slot has been requested to be ejected.
> 

Re: [Qemu-devel] [PATCH v2 02/11] hw/acpi: Make ACPI IO address space configurable

[Auger Eric]

Hi,

On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> This is in preparation for adding support for ARM64 platforms
> where it doesn't use port mapped IO for ACPI IO space.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/acpi/memory_hotplug.c         | 22 ++++++++++++++--------
>  hw/i386/acpi-build.c             |  3 ++-
>  include/hw/acpi/memory_hotplug.h |  5 +++--
>  3 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> index a6beb10..77ff0af 100644
> --- a/hw/acpi/memory_hotplug.c
> +++ b/hw/acpi/memory_hotplug.c
> @@ -8,7 +8,7 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-events-misc.h"
>  
> -static uint16_t memhp_io_base;
> +static hwaddr memhp_io_base;
>  
>  static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus *mdev)
>  {
> @@ -182,7 +182,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());
>  
> @@ -315,7 +315,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;
> @@ -338,14 +339,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 622ccb9..d9b554f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1852,7 +1852,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base,
>                         "\\_SB.PCI0", "\\_GPE._E02");
>      }
> -    build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0", "\\_GPE._E03");
> +    build_memory_hotplug_aml(dsdt, nr_mem, "\\_SB.PCI0",
> +                             "\\_GPE._E03", AML_SYSTEM_IO);
>  
>      scope =  aml_scope("_GPE");
>      {
> diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h
> index fbfcbe6..52f9027 100644
> --- a/include/hw/acpi/memory_hotplug.h
> +++ b/include/hw/acpi/memory_hotplug.h
> @@ -55,7 +55,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);
> @@ -74,5 +74,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
> 

Re: [Qemu-devel] [PATCH v2 01/11] hw/acpi: Move constant definitions to header files

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 08 March 2019 16:09
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com;
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sameo@linux.intel.com; sebastien.boeuf@intel.com
> Cc: Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> Subject: Re: [PATCH v2 01/11] hw/acpi: Move constant definitions to header
> files
> 
> Hi,
> 
> On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> > From: Sebastien Boeuf <sebastien.boeuf@intel.com>
> >
> > By moving the definition of memory hotplug related constants used
> > by ACPI for both CPU and memory, this commits allows those to be
> > used from  other parts of the code.
> 
> Maybe elaborate on where you intend to use them.

Ok.
 
> >
> > Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/acpi/memory_hotplug.c         | 26 --------------------------
> >  include/hw/acpi/memory_hotplug.h | 26 ++++++++++++++++++++++++++
> >  2 files changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c
> > index 921cad2..a6beb10 100644
> > --- a/hw/acpi/memory_hotplug.c
> > +++ b/hw/acpi/memory_hotplug.c
> > @@ -8,32 +8,6 @@
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-events-misc.h"
> >
> > -#define MEMORY_SLOTS_NUMBER          "MDNR"
> > -#define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> > -#define MEMORY_SLOT_ADDR_LOW         "MRBL"
> > -#define MEMORY_SLOT_ADDR_HIGH        "MRBH"
> > -#define MEMORY_SLOT_SIZE_LOW         "MRLL"
> > -#define MEMORY_SLOT_SIZE_HIGH        "MRLH"
> > -#define MEMORY_SLOT_PROXIMITY        "MPX"
> > -#define MEMORY_SLOT_ENABLED          "MES"
> > -#define MEMORY_SLOT_INSERT_EVENT     "MINS"
> > -#define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
> > -#define MEMORY_SLOT_EJECT            "MEJ"
> > -#define MEMORY_SLOT_SLECTOR          "MSEL"
> > -#define MEMORY_SLOT_OST_EVENT        "MOEV"
> > -#define MEMORY_SLOT_OST_STATUS       "MOSC"
> > -#define MEMORY_SLOT_LOCK             "MLCK"
> > -#define MEMORY_SLOT_STATUS_METHOD    "MRST"
> > -#define MEMORY_SLOT_CRS_METHOD       "MCRS"
> > -#define MEMORY_SLOT_OST_METHOD       "MOST"
> > -#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"
> 
> Do we really need to expose all of them. I just can see
> MEMORY_SLOT_SCAN_METHOD used in hw/acpi/ged.c? Maybe I missed some
> though?

That is right. This was taken from the Nemu approach and I thought it might be
useful in future changes. But if there is no major motivation in keeping this in header,
I may skip this patch and just move the MEMORY_SLOT_SCAN_METHOD.

> Also at the beginning I tried to find some specification details for
> those stuff but I failed. It would be helpful for a non specialist
> reader to add a comment that helps to understand what is part of a
> specification (link?) and what is arbitrarily defined.

Ok. I could only find this Nemu wiki description here
https://github.com/intel/nemu/wiki/Memory-hotplug

Not sure Qemu or ACPI has a similar documentation elsewhere.

Thanks,
Shameer

> Thanks
> 
> Eric
> > -
> >  static uint16_t memhp_io_base;
> >
> >  static ACPIOSTInfo *acpi_memory_device_status(int slot, MemStatus
> *mdev)
> > diff --git a/include/hw/acpi/memory_hotplug.h
> b/include/hw/acpi/memory_hotplug.h
> > index 77c6576..fbfcbe6 100644
> > --- a/include/hw/acpi/memory_hotplug.h
> > +++ b/include/hw/acpi/memory_hotplug.h
> > @@ -5,6 +5,32 @@
> >  #include "hw/acpi/acpi.h"
> >  #include "hw/acpi/aml-build.h"
> >
> > +#define MEMORY_SLOTS_NUMBER          "MDNR"
> > +#define MEMORY_HOTPLUG_IO_REGION     "HPMR"
> > +#define MEMORY_SLOT_ADDR_LOW         "MRBL"
> > +#define MEMORY_SLOT_ADDR_HIGH        "MRBH"
> > +#define MEMORY_SLOT_SIZE_LOW         "MRLL"
> > +#define MEMORY_SLOT_SIZE_HIGH        "MRLH"
> > +#define MEMORY_SLOT_PROXIMITY        "MPX"
> > +#define MEMORY_SLOT_ENABLED          "MES"
> > +#define MEMORY_SLOT_INSERT_EVENT     "MINS"
> > +#define MEMORY_SLOT_REMOVE_EVENT     "MRMV"
> > +#define MEMORY_SLOT_EJECT            "MEJ"
> > +#define MEMORY_SLOT_SLECTOR          "MSEL"
> > +#define MEMORY_SLOT_OST_EVENT        "MOEV"
> > +#define MEMORY_SLOT_OST_STATUS       "MOSC"
> > +#define MEMORY_SLOT_LOCK             "MLCK"
> > +#define MEMORY_SLOT_STATUS_METHOD    "MRST"
> > +#define MEMORY_SLOT_CRS_METHOD       "MCRS"
> > +#define MEMORY_SLOT_OST_METHOD       "MOST"
> > +#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"
> > +
> >  /**
> >   * MemStatus:
> >   * @is_removing: the memory device in slot has been requested to be
> ejected.
> >

Re: [Qemu-devel] [PATCH v2 04/11] hw/arm/virt: Add virtual ACPI device

[Auger Eric]

Hi Shameer,

On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> From: Samuel Ortiz <sameo@linux.intel.com>
> 
> This is to provide an acpi device interface for Arm/virt.
> This will be used by Arm/Virt to add hotplug support via
> ACPI GED device.

I think this would deserves to mention this is a skeleton or,
wouldn't it make sense to merge the virt-acpi part of "hw/arm/virt: Add
ACPI support for device memory cold-plug" inside this patch; and keep
its instantiation in machvirt in a separate patch?

> 
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/Makefile.objs  |   2 +-
>  hw/arm/virt-acpi.c    | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |   1 +
>  3 files changed, 113 insertions(+), 1 deletion(-)
>  create mode 100644 hw/arm/virt-acpi.c
> 
> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> index fa57c7c..e0db3cd 100644
> --- a/hw/arm/Makefile.objs
> +++ b/hw/arm/Makefile.objs
> @@ -1,6 +1,6 @@
>  obj-y += boot.o sysbus-fdt.o
>  obj-$(CONFIG_ARM_VIRT) += virt.o
> -obj-$(CONFIG_ACPI) += virt-acpi-build.o
> +obj-$(CONFIG_ACPI) += virt-acpi-build.o virt-acpi.o
>  obj-$(CONFIG_DIGIC) += digic_boards.o
>  obj-$(CONFIG_EXYNOS4) += exynos4_boards.o
>  obj-$(CONFIG_HIGHBANK) += highbank.o
> diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
> new file mode 100644
> index 0000000..df8a02b
> --- /dev/null
> +++ b/hw/arm/virt-acpi.c
> @@ -0,0 +1,111 @@
> +/*
> + *
> + * Copyright (c) 2018 Intel Corporation
> + *
> + * 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.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/range.h"
> +#include "qapi/error.h"
> +#include "exec/address-spaces.h"
> +
> +#include "hw/hw.h"
> +#include "hw/hotplug.h"
> +#include "hw/sysbus.h"
> +#include "hw/arm/virt.h"
> +
> +#include "hw/acpi/acpi.h"
> +
> +typedef struct VirtAcpiState {
> +    SysBusDevice parent_obj;
> +} VirtAcpiState;
> +
> +#define TYPE_VIRT_ACPI "virt-acpi"
> +#define VIRT_ACPI(obj) \
> +    OBJECT_CHECK(VirtAcpiState, (obj), TYPE_VIRT_ACPI)
> +
> +static const VMStateDescription vmstate_acpi = {
> +    .name = "virt_acpi",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +};
> +
> +static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
> +                                DeviceState *dev, Error **errp)
> +{
> +}
> +
> +static void virt_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> +                                          DeviceState *dev, Error **errp)
> +{
> +}
> +
> +static void virt_device_unplug_cb(HotplugHandler *hotplug_dev,
> +                                  DeviceState *dev, Error **errp)
> +{
> +}
> +
> +static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> +{
> +}
> +
> +static void virt_device_realize(DeviceState *dev, Error **errp)
> +{
> +}
> +
> +DeviceState *virt_acpi_init(void)
> +{
> +    return sysbus_create_simple(TYPE_VIRT_ACPI, -1, NULL);
> +}
> +
> +static Property virt_acpi_properties[] = {
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void virt_acpi_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";
> +    dc->vmsd = &vmstate_acpi;
do we really need the vmsd field as of now?
> +    dc->props = virt_acpi_properties;> +    dc->realize = virt_device_realize;
> +
> +    hc->plug = virt_device_plug_cb;
> +    hc->unplug_request = virt_device_unplug_request_cb;
> +    hc->unplug = virt_device_unplug_cb;
unplug_request and unplug still aren't implemented at the end of the
series. Maybe we can ignore them at the moment?
> +
> +    adevc->send_event = virt_send_ged;
> +}
> +
> +static const TypeInfo virt_acpi_info = {
> +    .name          = TYPE_VIRT_ACPI,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(VirtAcpiState),
> +    .class_init    = virt_acpi_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { TYPE_ACPI_DEVICE_IF },
> +        { }
> +    }
> +};
> +
> +static void virt_acpi_register_types(void)
> +{
> +    type_register_static(&virt_acpi_info);
> +}
> +
> +type_init(virt_acpi_register_types)
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 507517c..6076167 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -145,6 +145,7 @@ typedef struct {
>      OBJECT_CLASS_CHECK(VirtMachineClass, klass, TYPE_VIRT_MACHINE)
>  
>  void virt_acpi_setup(VirtMachineState *vms);
> +DeviceState *virt_acpi_init(void);
>  
>  /* Return the number of used redistributor regions  */
>  static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
> 

Thanks

Eric

Re: [Qemu-devel] [PATCH v2 06/11] hw/arm/virt: Add ACPI support for device memory cold-plug

[Auger Eric]

Hi Shameer,

On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> This adds support to build the aml code so that Guest can see the
s/Guest/the guest
> cold-plugged device memory.

Isn't the colplug part only related to acpi_dsdt_add_memory_hotplug()? I
understand the patch also adds some hotplug infra?

Can't you just keerp the dsdt additions here, move the virt-acpi changes
in the original patch and move the virt-acpi instantiation in a
subsequent patch?

Thanks

Eric
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  default-configs/arm-softmmu.mak |  1 +
>  hw/arm/virt-acpi-build.c        |  9 +++++++++
>  hw/arm/virt-acpi.c              | 23 +++++++++++++++++++++++
>  hw/arm/virt.c                   | 11 +++++++++++
>  include/hw/arm/virt.h           |  2 ++
>  5 files changed, 46 insertions(+)
> 
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index fbc4564..b3bac25 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -167,3 +167,4 @@ CONFIG_HIGHBANK=y
>  CONFIG_MUSICPAL=y
>  CONFIG_MEM_DEVICE=y
>  CONFIG_DIMM=y
> +CONFIG_ACPI_MEMORY_HOTPLUG=y
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index d7e2e48..87d66da 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -40,6 +40,7 @@
>  #include "hw/loader.h"
>  #include "hw/hw.h"
>  #include "hw/acpi/aml-build.h"
> +#include "hw/acpi/memory_hotplug.h"
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/arm/virt.h"
> @@ -49,6 +50,13 @@
>  #define ARM_SPI_BASE 32
>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>  
> +static void acpi_dsdt_add_memory_hotplug(Aml *scope, MachineState *ms)
> +{
> +    uint32_t nr_mem = ms->ram_slots;
> +
> +    build_memory_hotplug_aml(scope, nr_mem, "\\_SB", NULL, AML_SYSTEM_MEMORY);
> +}
> +
>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>  {
>      uint16_t i;
> @@ -740,6 +748,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>       * the RTC ACPI device at all when using UEFI.
>       */
>      scope = aml_scope("\\_SB");
> +    acpi_dsdt_add_memory_hotplug(scope, MACHINE(vms));
>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
> index df8a02b..18ebe94 100644
> --- a/hw/arm/virt-acpi.c
> +++ b/hw/arm/virt-acpi.c
> @@ -26,9 +26,11 @@
>  #include "hw/arm/virt.h"
>  
>  #include "hw/acpi/acpi.h"
> +#include "hw/acpi/memory_hotplug.h"
>  
>  typedef struct VirtAcpiState {
>      SysBusDevice parent_obj;
> +    MemHotplugState memhp_state;
>  } VirtAcpiState;
>  
>  #define TYPE_VIRT_ACPI "virt-acpi"
> @@ -44,6 +46,16 @@ static const VMStateDescription vmstate_acpi = {
>  static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
>                                  DeviceState *dev, Error **errp)
>  {
> +    VirtAcpiState *s = VIRT_ACPI(hotplug_dev);
> +
> +    if (s->memhp_state.is_enabled &&
> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +            acpi_memory_plug_cb(hotplug_dev, &s->memhp_state,
> +                                dev, errp);
> +    } else {
> +        error_setg(errp, "virt: device plug request for unsupported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
>  }
>  
>  static void virt_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> @@ -62,6 +74,15 @@ static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>  
>  static void virt_device_realize(DeviceState *dev, Error **errp)
>  {
> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> +    const MemMapEntry *memmap = vms->memmap;
> +    VirtAcpiState *s = VIRT_ACPI(dev);
> +
> +    if (s->memhp_state.is_enabled) {
> +        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
> +                                 &s->memhp_state,
> +                                 memmap[VIRT_PCDIMM_ACPI].base);
> +    }
>  }
>  
>  DeviceState *virt_acpi_init(void)
> @@ -70,6 +91,8 @@ DeviceState *virt_acpi_init(void)
>  }
>  
>  static Property virt_acpi_properties[] = {
> +    DEFINE_PROP_BOOL("memory-hotplug-support", VirtAcpiState,
> +                     memhp_state.is_enabled, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 40a1273..9427f4f 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -133,6 +133,7 @@ static const MemMapEntry base_memmap[] = {
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
> +    [VIRT_PCDIMM_ACPI] =        { 0x09070000, 0x00010000 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -1648,6 +1649,8 @@ static void machvirt_init(MachineState *machine)
>  
>      create_platform_bus(vms, pic);
>  
> +    vms->acpi = virt_acpi_init();
> +
>      vms->bootinfo.ram_size = machine->ram_size;
>      vms->bootinfo.kernel_filename = machine->kernel_filename;
>      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -1832,11 +1835,19 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
>                               DeviceState *dev, Error **errp)
>  {
> +    HotplugHandlerClass *hhc;
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>      Error *local_err = NULL;
>  
>      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> +    if (local_err) {
> +        goto out;
> +    }
> +
> +    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi);
> +    hhc->plug(HOTPLUG_HANDLER(vms->acpi), dev, &error_abort);
>  
> +out:
>      error_propagate(errp, local_err);
>  }
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 6076167..e46a051 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -77,6 +77,7 @@ enum {
>      VIRT_GPIO,
>      VIRT_SECURE_UART,
>      VIRT_SECURE_MEM,
> +    VIRT_PCDIMM_ACPI,
>      VIRT_LOWMEMMAP_LAST,
>  };
>  
> @@ -132,6 +133,7 @@ typedef struct {
>      uint32_t iommu_phandle;
>      int psci_conduit;
>      hwaddr highest_gpa;
> +    DeviceState *acpi;
>  } VirtMachineState;
>  
>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> 

Re: [Qemu-devel] [PATCH v2 07/11] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT

[Igor Mammedov]

On Fri, 8 Mar 2019 11:42:14 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> Generate Memory Affinity Structures for PC-DIMM ranges.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 87d66da..6cb7263 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -524,6 +524,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;
> @@ -549,6 +550,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);
> +    }
> +

do we care about 'Windows' guests?
if yes then we should drop this patch,
see commit for reasoning dbb6da8ba7e

>      build_header(linker, table_data, (void *)(table_data->data + srat_start),
>                   "SRAT", table_data->len - srat_start, 3, NULL, NULL);
>  }

Re: [Qemu-devel] [PATCH v2 08/11] hw/arm/boot: Expose the PC-DIMM nodes in the DT

[Igor Mammedov]

On Fri, 8 Mar 2019 11:42:15 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This patch adds memory nodes corresponding to PC-DIMM regions.
> 
> NVDIMM and ACPI_NVDIMM configs are not yet set for ARM so we
> don't need to care about NVDIMM at this stage.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/boot.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> index a830655..4caaf91 100644
> --- a/hw/arm/boot.c
> +++ b/hw/arm/boot.c
> @@ -19,6 +19,7 @@
>  #include "sysemu/numa.h"
>  #include "hw/boards.h"
>  #include "hw/loader.h"
> +#include "hw/mem/memory-device.h"
>  #include "elf.h"
>  #include "sysemu/device_tree.h"
>  #include "qemu/config-file.h"
> @@ -522,6 +523,41 @@ static void fdt_add_psci_node(void *fdt)
>      qemu_fdt_setprop_cell(fdt, "/psci", "migrate", migrate_fn);
>  }
>  
> +static int fdt_add_hotpluggable_memory_nodes(void *fdt,
> +                                             uint32_t acells, uint32_t scells) {
> +    MemoryDeviceInfoList *info, *info_list = qmp_memory_device_list();
> +    MemoryDeviceInfo *mi;
> +    int ret = 0;
> +
> +    for (info = info_list; info != NULL; info = info->next) {
> +        mi = info->value;
> +        switch (mi->type) {
> +        case MEMORY_DEVICE_INFO_KIND_DIMM:
> +        {
> +            PCDIMMDeviceInfo *di = mi->u.dimm.data;
> +
> +            ret = fdt_add_memory_node(fdt, acells, di->addr,
> +                                      scells, di->size, di->node);
> +            if (ret) {
> +                fprintf(stderr,
> +                        "couldn't add PCDIMM /memory@%"PRIx64" node\n",
> +                        di->addr);
> +                goto out;
> +            }
> +            break;
> +        }
> +        default:
> +            fprintf(stderr, "%s memory nodes are not yet supported\n",
> +                    MemoryDeviceInfoKind_str(mi->type));
> +            ret = -ENOENT;
> +            goto out;
> +        }
> +    }
> +out:
> +    qapi_free_MemoryDeviceInfoList(info_list);
> +    return ret;
> +}
> +
>  int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>                   hwaddr addr_limit, AddressSpace *as)
>  {
> @@ -621,6 +657,12 @@ int arm_load_dtb(hwaddr addr, const struct arm_boot_info *binfo,
>          }
>      }
>  
> +    rc = fdt_add_hotpluggable_memory_nodes(fdt, acells, scells);
According to Eric's test, guest kernel picks this up. So it probably
should be an opt-in feature to avoid conflict with ACPI definition in DSDT.

> +    if (rc < 0) {
> +        fprintf(stderr, "couldn't add hotpluggable memory nodes\n");
> +        goto fail;
> +    }
> +
>      rc = fdt_path_offset(fdt, "/chosen");
>      if (rc < 0) {
>          qemu_fdt_add_subnode(fdt, "/chosen");

Re: [Qemu-devel] [PATCH v2 04/11] hw/arm/virt: Add virtual ACPI device

[Shameerali Kolothum Thodi]

Hi Eric,

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 11 March 2019 13:24
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com;
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sameo@linux.intel.com; sebastien.boeuf@intel.com
> Cc: Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> Subject: Re: [PATCH v2 04/11] hw/arm/virt: Add virtual ACPI device
> 
> Hi Shameer,
> 
> On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> > From: Samuel Ortiz <sameo@linux.intel.com>
> >
> > This is to provide an acpi device interface for Arm/virt.
> > This will be used by Arm/Virt to add hotplug support via
> > ACPI GED device.
> 
> I think this would deserves to mention this is a skeleton or,
> wouldn't it make sense to merge the virt-acpi part of "hw/arm/virt: Add
> ACPI support for device memory cold-plug" inside this patch; and keep
> its instantiation in machvirt in a separate patch?

Ok. I will look into it.

> >
> > Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  hw/arm/Makefile.objs  |   2 +-
> >  hw/arm/virt-acpi.c    | 111
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/arm/virt.h |   1 +
> >  3 files changed, 113 insertions(+), 1 deletion(-)
> >  create mode 100644 hw/arm/virt-acpi.c
> >
> > diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> > index fa57c7c..e0db3cd 100644
> > --- a/hw/arm/Makefile.objs
> > +++ b/hw/arm/Makefile.objs
> > @@ -1,6 +1,6 @@
> >  obj-y += boot.o sysbus-fdt.o
> >  obj-$(CONFIG_ARM_VIRT) += virt.o
> > -obj-$(CONFIG_ACPI) += virt-acpi-build.o
> > +obj-$(CONFIG_ACPI) += virt-acpi-build.o virt-acpi.o
> >  obj-$(CONFIG_DIGIC) += digic_boards.o
> >  obj-$(CONFIG_EXYNOS4) += exynos4_boards.o
> >  obj-$(CONFIG_HIGHBANK) += highbank.o
> > diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
> > new file mode 100644
> > index 0000000..df8a02b
> > --- /dev/null
> > +++ b/hw/arm/virt-acpi.c
> > @@ -0,0 +1,111 @@
> > +/*
> > + *
> > + * Copyright (c) 2018 Intel Corporation
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of
> MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
> License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along
> with
> > + * this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/range.h"
> > +#include "qapi/error.h"
> > +#include "exec/address-spaces.h"
> > +
> > +#include "hw/hw.h"
> > +#include "hw/hotplug.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/arm/virt.h"
> > +
> > +#include "hw/acpi/acpi.h"
> > +
> > +typedef struct VirtAcpiState {
> > +    SysBusDevice parent_obj;
> > +} VirtAcpiState;
> > +
> > +#define TYPE_VIRT_ACPI "virt-acpi"
> > +#define VIRT_ACPI(obj) \
> > +    OBJECT_CHECK(VirtAcpiState, (obj), TYPE_VIRT_ACPI)
> > +
> > +static const VMStateDescription vmstate_acpi = {
> > +    .name = "virt_acpi",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +};
> > +
> > +static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
> > +                                DeviceState *dev, Error **errp)
> > +{
> > +}
> > +
> > +static void virt_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > +                                          DeviceState *dev, Error
> **errp)
> > +{
> > +}
> > +
> > +static void virt_device_unplug_cb(HotplugHandler *hotplug_dev,
> > +                                  DeviceState *dev, Error **errp)
> > +{
> > +}
> > +
> > +static void virt_send_ged(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> > +{
> > +}
> > +
> > +static void virt_device_realize(DeviceState *dev, Error **errp)
> > +{
> > +}
> > +
> > +DeviceState *virt_acpi_init(void)
> > +{
> > +    return sysbus_create_simple(TYPE_VIRT_ACPI, -1, NULL);
> > +}
> > +
> > +static Property virt_acpi_properties[] = {
> > +    DEFINE_PROP_END_OF_LIST(),
> > +};
> > +
> > +static void virt_acpi_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";
> > +    dc->vmsd = &vmstate_acpi;
> do we really need the vmsd field as of now?

I am not sure. Is this only relevant for migration cases only?

> > +    dc->props = virt_acpi_properties;> +    dc->realize =
> virt_device_realize;
> > +
> > +    hc->plug = virt_device_plug_cb;
> > +    hc->unplug_request = virt_device_unplug_request_cb;
> > +    hc->unplug = virt_device_unplug_cb;
> unplug_request and unplug still aren't implemented at the end of the
> series. Maybe we can ignore them at the moment?

Agreed. I will remove those.

Thanks,
Shameer

> > +
> > +    adevc->send_event = virt_send_ged;
> > +}
> > +
> > +static const TypeInfo virt_acpi_info = {
> > +    .name          = TYPE_VIRT_ACPI,
> > +    .parent        = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size = sizeof(VirtAcpiState),
> > +    .class_init    = virt_acpi_class_init,
> > +    .interfaces = (InterfaceInfo[]) {
> > +        { TYPE_HOTPLUG_HANDLER },
> > +        { TYPE_ACPI_DEVICE_IF },
> > +        { }
> > +    }
> > +};
> > +
> > +static void virt_acpi_register_types(void)
> > +{
> > +    type_register_static(&virt_acpi_info);
> > +}
> > +
> > +type_init(virt_acpi_register_types)
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 507517c..6076167 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -145,6 +145,7 @@ typedef struct {
> >      OBJECT_CLASS_CHECK(VirtMachineClass, klass,
> TYPE_VIRT_MACHINE)
> >
> >  void virt_acpi_setup(VirtMachineState *vms);
> > +DeviceState *virt_acpi_init(void);
> >
> >  /* Return the number of used redistributor regions  */
> >  static inline int virt_gicv3_redist_region_count(VirtMachineState *vms)
> >
> 
> Thanks
> 
> Eric

Re: [Qemu-devel] [PATCH v2 06/11] hw/arm/virt: Add ACPI support for device memory cold-plug

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@redhat.com]
> Sent: 11 March 2019 13:32
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com;
> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
> sameo@linux.intel.com; sebastien.boeuf@intel.com
> Cc: Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
> Subject: Re: [PATCH v2 06/11] hw/arm/virt: Add ACPI support for device
> memory cold-plug
> 
> Hi Shameer,
> 
> On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> > This adds support to build the aml code so that Guest can see the
> s/Guest/the guest
> > cold-plugged device memory.
> 
> Isn't the colplug part only related to acpi_dsdt_add_memory_hotplug()? I
> understand the patch also adds some hotplug infra?

Hmm..build_memory_hotplug_aml() doesn’t do much if io_base address
Is not initialized. So we need the acpi_memory_hotplug_init().

Also, I think acpi_memory_plug_cb() needs to be called for cold-plugged dimm
otherwise the scan wont detect it.
 
> Can't you just keerp the dsdt additions here, move the virt-acpi changes
> in the original patch and move the virt-acpi instantiation in a
> subsequent patch?

Ok, will take look if there is any scope for reordering this.

Thanks,
Shameer
 
> Thanks
> 
> Eric
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  default-configs/arm-softmmu.mak |  1 +
> >  hw/arm/virt-acpi-build.c        |  9 +++++++++
> >  hw/arm/virt-acpi.c              | 23 +++++++++++++++++++++++
> >  hw/arm/virt.c                   | 11 +++++++++++
> >  include/hw/arm/virt.h           |  2 ++
> >  5 files changed, 46 insertions(+)
> >
> > diff --git a/default-configs/arm-softmmu.mak
> b/default-configs/arm-softmmu.mak
> > index fbc4564..b3bac25 100644
> > --- a/default-configs/arm-softmmu.mak
> > +++ b/default-configs/arm-softmmu.mak
> > @@ -167,3 +167,4 @@ CONFIG_HIGHBANK=y
> >  CONFIG_MUSICPAL=y
> >  CONFIG_MEM_DEVICE=y
> >  CONFIG_DIMM=y
> > +CONFIG_ACPI_MEMORY_HOTPLUG=y
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index d7e2e48..87d66da 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -40,6 +40,7 @@
> >  #include "hw/loader.h"
> >  #include "hw/hw.h"
> >  #include "hw/acpi/aml-build.h"
> > +#include "hw/acpi/memory_hotplug.h"
> >  #include "hw/pci/pcie_host.h"
> >  #include "hw/pci/pci.h"
> >  #include "hw/arm/virt.h"
> > @@ -49,6 +50,13 @@
> >  #define ARM_SPI_BASE 32
> >  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> >
> > +static void acpi_dsdt_add_memory_hotplug(Aml *scope, MachineState
> *ms)
> > +{
> > +    uint32_t nr_mem = ms->ram_slots;
> > +
> > +    build_memory_hotplug_aml(scope, nr_mem, "\\_SB", NULL,
> AML_SYSTEM_MEMORY);
> > +}
> > +
> >  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
> >  {
> >      uint16_t i;
> > @@ -740,6 +748,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
> VirtMachineState *vms)
> >       * the RTC ACPI device at all when using UEFI.
> >       */
> >      scope = aml_scope("\\_SB");
> > +    acpi_dsdt_add_memory_hotplug(scope, MACHINE(vms));
> >      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
> >      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
> >                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> > diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
> > index df8a02b..18ebe94 100644
> > --- a/hw/arm/virt-acpi.c
> > +++ b/hw/arm/virt-acpi.c
> > @@ -26,9 +26,11 @@
> >  #include "hw/arm/virt.h"
> >
> >  #include "hw/acpi/acpi.h"
> > +#include "hw/acpi/memory_hotplug.h"
> >
> >  typedef struct VirtAcpiState {
> >      SysBusDevice parent_obj;
> > +    MemHotplugState memhp_state;
> >  } VirtAcpiState;
> >
> >  #define TYPE_VIRT_ACPI "virt-acpi"
> > @@ -44,6 +46,16 @@ static const VMStateDescription vmstate_acpi = {
> >  static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
> >                                  DeviceState *dev, Error **errp)
> >  {
> > +    VirtAcpiState *s = VIRT_ACPI(hotplug_dev);
> > +
> > +    if (s->memhp_state.is_enabled &&
> > +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > +            acpi_memory_plug_cb(hotplug_dev, &s->memhp_state,
> > +                                dev, errp);
> > +    } else {
> > +        error_setg(errp, "virt: device plug request for unsupported
> device"
> > +                   " type: %s", object_get_typename(OBJECT(dev)));
> > +    }
> >  }
> >
> >  static void virt_device_unplug_request_cb(HotplugHandler *hotplug_dev,
> > @@ -62,6 +74,15 @@ static void virt_send_ged(AcpiDeviceIf *adev,
> AcpiEventStatusBits ev)
> >
> >  static void virt_device_realize(DeviceState *dev, Error **errp)
> >  {
> > +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
> > +    const MemMapEntry *memmap = vms->memmap;
> > +    VirtAcpiState *s = VIRT_ACPI(dev);
> > +
> > +    if (s->memhp_state.is_enabled) {
> > +        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
> > +                                 &s->memhp_state,
> > +
> memmap[VIRT_PCDIMM_ACPI].base);
> > +    }
> >  }
> >
> >  DeviceState *virt_acpi_init(void)
> > @@ -70,6 +91,8 @@ DeviceState *virt_acpi_init(void)
> >  }
> >
> >  static Property virt_acpi_properties[] = {
> > +    DEFINE_PROP_BOOL("memory-hotplug-support", VirtAcpiState,
> > +                     memhp_state.is_enabled, true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 40a1273..9427f4f 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -133,6 +133,7 @@ static const MemMapEntry base_memmap[] = {
> >      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
> >      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
> >      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
> > +    [VIRT_PCDIMM_ACPI] =        { 0x09070000, 0x00010000 },
> >      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
> >      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that
> size */
> >      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> > @@ -1648,6 +1649,8 @@ static void machvirt_init(MachineState *machine)
> >
> >      create_platform_bus(vms, pic);
> >
> > +    vms->acpi = virt_acpi_init();
> > +
> >      vms->bootinfo.ram_size = machine->ram_size;
> >      vms->bootinfo.kernel_filename = machine->kernel_filename;
> >      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > @@ -1832,11 +1835,19 @@ static void
> virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >  static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >                               DeviceState *dev, Error **errp)
> >  {
> > +    HotplugHandlerClass *hhc;
> >      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >      Error *local_err = NULL;
> >
> >      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi);
> > +    hhc->plug(HOTPLUG_HANDLER(vms->acpi), dev, &error_abort);
> >
> > +out:
> >      error_propagate(errp, local_err);
> >  }
> >
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index 6076167..e46a051 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -77,6 +77,7 @@ enum {
> >      VIRT_GPIO,
> >      VIRT_SECURE_UART,
> >      VIRT_SECURE_MEM,
> > +    VIRT_PCDIMM_ACPI,
> >      VIRT_LOWMEMMAP_LAST,
> >  };
> >
> > @@ -132,6 +133,7 @@ typedef struct {
> >      uint32_t iommu_phandle;
> >      int psci_conduit;
> >      hwaddr highest_gpa;
> > +    DeviceState *acpi;
> >  } VirtMachineState;
> >
> >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> VIRT_PCIE_ECAM)
> >

Re: [Qemu-devel] [PATCH v2 06/11] hw/arm/virt: Add ACPI support for device memory cold-plug

[Auger Eric]

Hi Shameer,
On 3/11/19 6:37 PM, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Auger Eric [mailto:eric.auger@redhat.com]
>> Sent: 11 March 2019 13:32
>> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
>> qemu-devel@nongnu.org; qemu-arm@nongnu.org; imammedo@redhat.com;
>> peter.maydell@linaro.org; shannon.zhaosl@gmail.com;
>> sameo@linux.intel.com; sebastien.boeuf@intel.com
>> Cc: Linuxarm <linuxarm@huawei.com>; xuwei (O) <xuwei5@huawei.com>
>> Subject: Re: [PATCH v2 06/11] hw/arm/virt: Add ACPI support for device
>> memory cold-plug
>>
>> Hi Shameer,
>>
>> On 3/8/19 12:42 PM, Shameer Kolothum wrote:
>>> This adds support to build the aml code so that Guest can see the
>> s/Guest/the guest
>>> cold-plugged device memory.
>>
>> Isn't the colplug part only related to acpi_dsdt_add_memory_hotplug()? I
>> understand the patch also adds some hotplug infra?
> 
> Hmm..build_memory_hotplug_aml() doesn’t do much if io_base address
> Is not initialized. So we need the acpi_memory_hotplug_init().
Yes that's correct, we need it for cold-plug.
> 
> Also, I think acpi_memory_plug_cb() needs to be called for cold-plugged dimm
> otherwise the scan wont detect it.
You may be right as some MemStatus fields are updated also in coldplug case.

Worth to double check the bare minimal to make cold-plug OK in ACPI mode
(without the dt node generation in ACPI mode as suggested by Igor), and
augment the commit message with more details.

Thanks

Eric

>  
>> Can't you just keerp the dsdt additions here, move the virt-acpi changes
>> in the original patch and move the virt-acpi instantiation in a
>> subsequent patch?
> 
> Ok, will take look if there is any scope for reordering this.
> 
> Thanks,
> Shameer
>  
>> Thanks
>>
>> Eric
>>>
>>> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>>> ---
>>>  default-configs/arm-softmmu.mak |  1 +
>>>  hw/arm/virt-acpi-build.c        |  9 +++++++++
>>>  hw/arm/virt-acpi.c              | 23 +++++++++++++++++++++++
>>>  hw/arm/virt.c                   | 11 +++++++++++
>>>  include/hw/arm/virt.h           |  2 ++
>>>  5 files changed, 46 insertions(+)
>>>
>>> diff --git a/default-configs/arm-softmmu.mak
>> b/default-configs/arm-softmmu.mak
>>> index fbc4564..b3bac25 100644
>>> --- a/default-configs/arm-softmmu.mak
>>> +++ b/default-configs/arm-softmmu.mak
>>> @@ -167,3 +167,4 @@ CONFIG_HIGHBANK=y
>>>  CONFIG_MUSICPAL=y
>>>  CONFIG_MEM_DEVICE=y
>>>  CONFIG_DIMM=y
>>> +CONFIG_ACPI_MEMORY_HOTPLUG=y
>>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>>> index d7e2e48..87d66da 100644
>>> --- a/hw/arm/virt-acpi-build.c
>>> +++ b/hw/arm/virt-acpi-build.c
>>> @@ -40,6 +40,7 @@
>>>  #include "hw/loader.h"
>>>  #include "hw/hw.h"
>>>  #include "hw/acpi/aml-build.h"
>>> +#include "hw/acpi/memory_hotplug.h"
>>>  #include "hw/pci/pcie_host.h"
>>>  #include "hw/pci/pci.h"
>>>  #include "hw/arm/virt.h"
>>> @@ -49,6 +50,13 @@
>>>  #define ARM_SPI_BASE 32
>>>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>>>
>>> +static void acpi_dsdt_add_memory_hotplug(Aml *scope, MachineState
>> *ms)
>>> +{
>>> +    uint32_t nr_mem = ms->ram_slots;
>>> +
>>> +    build_memory_hotplug_aml(scope, nr_mem, "\\_SB", NULL,
>> AML_SYSTEM_MEMORY);
>>> +}
>>> +
>>>  static void acpi_dsdt_add_cpus(Aml *scope, int smp_cpus)
>>>  {
>>>      uint16_t i;
>>> @@ -740,6 +748,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>> VirtMachineState *vms)
>>>       * the RTC ACPI device at all when using UEFI.
>>>       */
>>>      scope = aml_scope("\\_SB");
>>> +    acpi_dsdt_add_memory_hotplug(scope, MACHINE(vms));
>>>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>>>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>>>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>>> diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
>>> index df8a02b..18ebe94 100644
>>> --- a/hw/arm/virt-acpi.c
>>> +++ b/hw/arm/virt-acpi.c
>>> @@ -26,9 +26,11 @@
>>>  #include "hw/arm/virt.h"
>>>
>>>  #include "hw/acpi/acpi.h"
>>> +#include "hw/acpi/memory_hotplug.h"
>>>
>>>  typedef struct VirtAcpiState {
>>>      SysBusDevice parent_obj;
>>> +    MemHotplugState memhp_state;
>>>  } VirtAcpiState;
>>>
>>>  #define TYPE_VIRT_ACPI "virt-acpi"
>>> @@ -44,6 +46,16 @@ static const VMStateDescription vmstate_acpi = {
>>>  static void virt_device_plug_cb(HotplugHandler *hotplug_dev,
>>>                                  DeviceState *dev, Error **errp)
>>>  {
>>> +    VirtAcpiState *s = VIRT_ACPI(hotplug_dev);
>>> +
>>> +    if (s->memhp_state.is_enabled &&
>>> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>>> +            acpi_memory_plug_cb(hotplug_dev, &s->memhp_state,
>>> +                                dev, errp);
>>> +    } else {
>>> +        error_setg(errp, "virt: device plug request for unsupported
>> device"
>>> +                   " type: %s", object_get_typename(OBJECT(dev)));
>>> +    }
>>>  }
>>>
>>>  static void virt_device_unplug_request_cb(HotplugHandler *hotplug_dev,
>>> @@ -62,6 +74,15 @@ static void virt_send_ged(AcpiDeviceIf *adev,
>> AcpiEventStatusBits ev)
>>>
>>>  static void virt_device_realize(DeviceState *dev, Error **errp)
>>>  {
>>> +    VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
>>> +    const MemMapEntry *memmap = vms->memmap;
>>> +    VirtAcpiState *s = VIRT_ACPI(dev);
>>> +
>>> +    if (s->memhp_state.is_enabled) {
>>> +        acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
>>> +                                 &s->memhp_state,
>>> +
>> memmap[VIRT_PCDIMM_ACPI].base);
>>> +    }
>>>  }
>>>
>>>  DeviceState *virt_acpi_init(void)
>>> @@ -70,6 +91,8 @@ DeviceState *virt_acpi_init(void)
>>>  }
>>>
>>>  static Property virt_acpi_properties[] = {
>>> +    DEFINE_PROP_BOOL("memory-hotplug-support", VirtAcpiState,
>>> +                     memhp_state.is_enabled, true),
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>>> index 40a1273..9427f4f 100644
>>> --- a/hw/arm/virt.c
>>> +++ b/hw/arm/virt.c
>>> @@ -133,6 +133,7 @@ static const MemMapEntry base_memmap[] = {
>>>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>>>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
>>>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
>>> +    [VIRT_PCDIMM_ACPI] =        { 0x09070000, 0x00010000 },
>>>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>>>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that
>> size */
>>>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
>>> @@ -1648,6 +1649,8 @@ static void machvirt_init(MachineState *machine)
>>>
>>>      create_platform_bus(vms, pic);
>>>
>>> +    vms->acpi = virt_acpi_init();
>>> +
>>>      vms->bootinfo.ram_size = machine->ram_size;
>>>      vms->bootinfo.kernel_filename = machine->kernel_filename;
>>>      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
>>> @@ -1832,11 +1835,19 @@ static void
>> virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
>>>                               DeviceState *dev, Error **errp)
>>>  {
>>> +    HotplugHandlerClass *hhc;
>>>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>>>      Error *local_err = NULL;
>>>
>>>      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), &local_err);
>>> +    if (local_err) {
>>> +        goto out;
>>> +    }
>>> +
>>> +    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi);
>>> +    hhc->plug(HOTPLUG_HANDLER(vms->acpi), dev, &error_abort);
>>>
>>> +out:
>>>      error_propagate(errp, local_err);
>>>  }
>>>
>>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>>> index 6076167..e46a051 100644
>>> --- a/include/hw/arm/virt.h
>>> +++ b/include/hw/arm/virt.h
>>> @@ -77,6 +77,7 @@ enum {
>>>      VIRT_GPIO,
>>>      VIRT_SECURE_UART,
>>>      VIRT_SECURE_MEM,
>>> +    VIRT_PCDIMM_ACPI,
>>>      VIRT_LOWMEMMAP_LAST,
>>>  };
>>>
>>> @@ -132,6 +133,7 @@ typedef struct {
>>>      uint32_t iommu_phandle;
>>>      int psci_conduit;
>>>      hwaddr highest_gpa;
>>> +    DeviceState *acpi;
>>>  } VirtMachineState;
>>>
>>>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
>> VIRT_PCIE_ECAM)
>>>

Re: [Qemu-devel] [PATCH v2 09/11] hw/acpi: Add ACPI Generic Event Device Support

[Auger Eric]

Hi,

On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> From: Samuel Ortiz <sameo@linux.intel.com>
> 
> The ACPI Generic Event Device (GED) is a hardware-reduced specific
> device that handles all platform events, including the hotplug ones.
> This patch generate the AML code that defines GEDs.
s/generate/generates
> Platforms need to specify their own GedEvent array to describe what kind
> of events they want to support through GED. The build_ged_aml routine
> takes a GedEvent array that maps a specific GED event to an IRQ number.
> Then we use that array to build both the _CRS and the _EVT section
> of the GED device.
> 
> This is in preparation for making use of GED for ARM/virt
> platform and for now supports only memory hotplug.
> 
> 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>
> ---
>  hw/acpi/Makefile.objs |   1 +
>  hw/acpi/ged.c         | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/ged.h |  61 ++++++++++++++++
>  3 files changed, 260 insertions(+)
>  create mode 100644 hw/acpi/ged.c
>  create mode 100644 include/hw/acpi/ged.h
> 
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 2d46e37..6cf572b 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) += ged.o
>  common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>  
>  common-obj-y += acpi_interface.o
> diff --git a/hw/acpi/ged.c b/hw/acpi/ged.c
> new file mode 100644
> index 0000000..5076fbc
> --- /dev/null
> +++ b/hw/acpi/ged.c
> @@ -0,0 +1,198 @@
> +/*
> + *
> + * Copyright (c) 2018 Intel Corporation
> + *
> + * 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.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "hw/acpi/ged.h"
Add a comment giving a short description of what is the GED, available
since 6.1 spec?
> +
> +static hwaddr ged_io_base;
 Couldn't we add a comment such as, "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_IRQ_SEL_OFFSET:
> +        /* Read the selector value and reset it */
> +        qemu_mutex_lock(&ged_st->lock);
> +        val = ged_st->sel;
> +        ged_st->sel = ACPI_GED_IRQ_SEL_INIT;
using 0 instead of ACPI_GED_IRQ_SEL_INIT may be clearer?
> +        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,
> +    },
> +};
> +
> +void acpi_ged_init(MemoryRegion *as, Object *owner, GEDState *ged_st,
> +                   hwaddr base_addr, uint32_t ged_irq)
Maybe directly pass the qemu_irq* and store it into the GEDState
> +{
> +
> +    assert(!ged_io_base);
> +
> +    ged_io_base = base_addr;
> +    ged_st->irq = ged_irq;
> +    qemu_mutex_init(&ged_st->lock);
> +    memory_region_init_io(&ged_st->io, owner, &ged_ops, ged_st,
> +                          "acpi-ged-event", ACPI_GED_REG_LEN);
> +    memory_region_add_subregion(as, base_addr, &ged_st->io);
> +}
> +
> +void acpi_ged_event(GEDState *ged_st, qemu_irq *irq, uint32_t ged_irq_sel)
... then you wouldn't need the irq parameter here?
> +{
> +    /*
> +     * 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 |= ged_irq_sel;
is it allowed to have several events logged simultaneously? _EVT args is
described as "EventNumber: An integer indicating the event number (GSIV
number) of the current event"
> +    qemu_mutex_unlock(&ged_st->lock);
> +
> +    /* Trigger the event by sending an interrupt to the guest. */
> +    qemu_irq_pulse(irq[ged_st->irq]);
Is it valid to consider the irq always is edge sensitive? I understand
from the spec the irq mode may be platform dependent.
> +}
> +
> +static Aml *ged_event_aml(GedEvent *event)
> +{
> +
> +    if (!event) {
> +        return NULL;
> +    }
> +
> +    switch (event->event) {
> +    case GED_MEMORY_HOTPLUG:
> +        /* We run a complete memory SCAN when getting a memory hotplug event */
> +        return aml_call0("\\_SB.MHPC." MEMORY_SLOT_SCAN_METHOD);
use MEMORY_DEVICES_CONTAINER now exposed in
include/hw/acpi/memory_hotplug.h?
> +    default:
> +        break;
> +    }
> +
> +    return NULL;
> +}
> +
> +void build_ged_aml(Aml *table, const char *name, uint32_t ged_irq,
> +                   GedEvent *events, uint32_t events_size,
> +                   AmlRegionSpace rs)
> +{
> +    Aml *crs = aml_resource_template();
> +    Aml *evt, *field;
> +    Aml *zero = aml_int(0);
> +    Aml *dev = aml_device("%s", name);
> +    Aml *irq_sel = aml_local(0);
> +    Aml *isel = aml_name(AML_GED_IRQ_SEL);
> +    uint32_t i;
> +
> +    if (!ged_io_base) {
> +        return;
> +    }
> +
> +    /* _CRS interrupt */
> +    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH,
> +                                  AML_EXCLUSIVE, &ged_irq, 1));
> +    /*
> +     * For each GED event we:
> +     * - Add an interrupt to the CRS section.
> +     * - Add a conditional block for each event, inside a while loop.
> +     *   This is semantically equivalent to a switch/case implementation.
> +     */
> +    evt = aml_method("_EVT", 1, AML_SERIALIZED);
> +    {
> +        Aml *ged_aml;
> +        Aml *if_ctx;
> +
> +        /* Local0 = ISEL */
> +        aml_append(evt, aml_store(isel, irq_sel));
> +
> +        /*
> +         * Here we want to call a method for each supported GED event type.
> +         * The resulting ASL code looks like:
> +         *
> +         * Local0 = ISEL
> +         * If ((Local0 & irq0) == irq0)
> +         * {
> +         *     MethodEvent0()
> +         * }
> +         *
> +         * If ((Local0 & irq1) == irq1)
> +         * {
> +         *     MethodEvent1()
> +         * }
> +         *
> +         * If ((Local0 & irq2) == irq2)
> +         * {
> +         *     MethodEvent2()
> +         * }
> +         */
> +
> +        for (i = 0; i < events_size; i++) {
> +            ged_aml = ged_event_aml(&events[i]);
> +            if (!ged_aml) {
> +                continue;
> +            }
> +
> +            /* If ((Local1 == irq))*/
> +            if_ctx = aml_if(aml_equal(aml_and(irq_sel, aml_int(events[i].selector), NULL), aml_int(events[i].selector)));
> +            {
> +                /* AML for this specific type of event */
> +                aml_append(if_ctx, ged_aml);
> +            }
> +
> +            /*
> +             * We append the first if to the while context.
We append the first "if" to the "while" context.
> +             * Other ifs will be elseifs.
> +             */
> +            aml_append(evt, if_ctx);
> +        }
> +    }
> +
> +    aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0013")));
> +    aml_append(dev, aml_name_decl("_UID", zero));
can't we use something different from 0 like a aml_string("GED")?
> +    aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +    /* Append IO region */
> +    aml_append(dev, aml_operation_region(AML_GED_IRQ_REG, rs,
> +               aml_int(ged_io_base + ACPI_GED_IRQ_SEL_OFFSET),
> +               ACPI_GED_IRQ_SEL_LEN));
> +    field = aml_field(AML_GED_IRQ_REG, AML_DWORD_ACC, AML_NOLOCK,
> +                      AML_WRITE_AS_ZEROS);
> +    aml_append(field, aml_named_field(AML_GED_IRQ_SEL,
> +                                      ACPI_GED_IRQ_SEL_LEN * 8));
> +    aml_append(dev, field);
> +
> +    /* Append _EVT method */
> +    aml_append(dev, evt);
> +
> +    aml_append(table, dev);
> +}
Spec says "The platform declare its support for the GED, and query
whether an OS supports it, via the _OSC method". \_SB._OSC. I fail to
see that method defined?
> diff --git a/include/hw/acpi/ged.h b/include/hw/acpi/ged.h
> new file mode 100644
> index 0000000..60689b0
> --- /dev/null
> +++ b/include/hw/acpi/ged.h
> @@ -0,0 +1,61 @@
> +/*
> + *
> + * Copyright (c) 2018 Intel Corporation
> + *
> + * 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.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_ACPI_GED_H
> +#define HW_ACPI_GED_H
> +
> +#include "qemu/osdep.h"
> +#include "exec/memory.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/memory_hotplug.h"
> +
> +#define ACPI_GED_IRQ_SEL_OFFSET 0x0
> +#define ACPI_GED_IRQ_SEL_LEN    0x4
> +#define ACPI_GED_IRQ_SEL_INIT   0x0
> +#define ACPI_GED_IRQ_SEL_MEM    0x1
> +#define ACPI_GED_REG_LEN        0x4
> +
> +#define GED_DEVICE      "GED"
> +#define AML_GED_IRQ_REG "IREG"
> +#define AML_GED_IRQ_SEL "ISEL"
> +
> +typedef struct Aml Aml;
> +
> +typedef enum {
> +    GED_MEMORY_HOTPLUG = 1,
> +} GedEventType;
> +
> +typedef struct GedEvent {
> +    uint32_t     selector;Add a comment saying this corresponds to GSIV number?
> +    GedEventType event;
> +} GedEvent;
> +
> +typedef struct GEDState {
> +    MemoryRegion io;
> +    uint32_t     sel;
> +    uint32_t     irq;
why not storing the qemu_irq* directly?
> +    QemuMutex    lock;
> +} GEDState;
> +
> +void acpi_ged_init(MemoryRegion *as, Object *owner, GEDState *ged_st,
> +                   hwaddr base_addr, uint32_t ged_irq);
> +void acpi_ged_event(GEDState *ged_st, qemu_irq *irq, uint32_t ged_irq_sel);
> +void build_ged_aml(Aml *table, const char* name, uint32_t ged_irq,
> +                   GedEvent *events, uint32_t events_size,
> +                   AmlRegionSpace rs);
> +
> +#endif
> 

Thanks

Eric

Re: [Qemu-devel] [PATCH v2 10/11] hw/arm/virt: Add GED device configuration and build aml

[Auger Eric]

Hi Shameer,

On 3/8/19 12:42 PM, Shameer Kolothum wrote:
> This initializes the GED device with base memory and irq.
> It also configures ged memory hotplug event and builds the
> corresponding aml code.
> 
> But ged irq routing to Guest is not enabled and thus hotplug
> is not yet supported.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  default-configs/arm-softmmu.mak |  1 +
>  hw/arm/virt-acpi-build.c        | 13 +++++++++++++
>  hw/arm/virt-acpi.c              |  4 ++++
>  hw/arm/virt.c                   | 22 ++++++++++++++++++++++
>  include/hw/arm/virt.h           |  4 ++++
>  5 files changed, 44 insertions(+)
> 
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index b3bac25..7c442fd 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -168,3 +168,4 @@ CONFIG_MUSICPAL=y
>  CONFIG_MEM_DEVICE=y
>  CONFIG_DIMM=y
>  CONFIG_ACPI_MEMORY_HOTPLUG=y
> +CONFIG_ACPI_HW_REDUCED=y
requires a KConfig entry now
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 6cb7263..86f25ad 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -50,6 +50,18 @@
>  #define ARM_SPI_BASE 32
>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>  
> +static void acpi_dsdt_add_ged(Aml *scope, VirtMachineState *vms)
> +{
> +    int irq =  vms->irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE;
> +
> +    if (!vms->ged_events || !vms->ged_events_size) {
> +        return;
> +    }
> +
> +    build_ged_aml(scope, "\\_SB."GED_DEVICE, irq, vms->ged_events,
> +                  vms->ged_events_size, AML_SYSTEM_MEMORY);
> +}
> +
>  static void acpi_dsdt_add_memory_hotplug(Aml *scope, MachineState *ms)
>  {
>      uint32_t nr_mem = ms->ram_slots;
> @@ -758,6 +770,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>       */
>      scope = aml_scope("\\_SB");
>      acpi_dsdt_add_memory_hotplug(scope, MACHINE(vms));
> +    acpi_dsdt_add_ged(scope, vms);
>      acpi_dsdt_add_cpus(scope, vms->smp_cpus);
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
> diff --git a/hw/arm/virt-acpi.c b/hw/arm/virt-acpi.c
> index 18ebe94..3b55c63 100644
> --- a/hw/arm/virt-acpi.c
> +++ b/hw/arm/virt-acpi.c
> @@ -31,6 +31,7 @@
>  typedef struct VirtAcpiState {
>      SysBusDevice parent_obj;
>      MemHotplugState memhp_state;
> +    GEDState ged_state;
>  } VirtAcpiState;
>  
>  #define TYPE_VIRT_ACPI "virt-acpi"
> @@ -76,12 +77,15 @@ static void virt_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(qdev_get_machine());
>      const MemMapEntry *memmap = vms->memmap;
> +    const int *irqmap = vms->irqmap;
>      VirtAcpiState *s = VIRT_ACPI(dev);
>  
>      if (s->memhp_state.is_enabled) {
>          acpi_memory_hotplug_init(get_system_memory(), OBJECT(dev),
>                                   &s->memhp_state,
>                                   memmap[VIRT_PCDIMM_ACPI].base);
> +        acpi_ged_init(get_system_memory(), OBJECT(dev), &s->ged_state,
> +                      memmap[VIRT_ACPI_GED].base, irqmap[VIRT_ACPI_GED]);
>      }
>  }
>  
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9427f4f..352dbb1 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -134,6 +134,7 @@ static const MemMapEntry base_memmap[] = {
>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
>      [VIRT_PCDIMM_ACPI] =        { 0x09070000, 0x00010000 },
> +    [VIRT_ACPI_GED] =           { 0x09080000, 0x00010000 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -169,6 +170,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 */
> @@ -184,6 +186,25 @@ static const char *valid_cpus[] = {
>      ARM_CPU_TYPE_NAME("max"),
>  };
>  
> +static void virt_acpi_ged_conf(VirtMachineState *vms)
> +{
> +    uint8_t events_size;
> +
> +    /* GED events */
> +    GedEvent events[] = {
> +        {
> +            .selector = ACPI_GED_IRQ_SEL_MEM,
> +            .event    = GED_MEMORY_HOTPLUG,
> +        },
> +    };
> +
> +    events_size = ARRAY_SIZE(events);
> +
> +    vms->ged_events = g_malloc0(events_size * sizeof(GedEvent));
> +    memcpy(vms->ged_events, events, events_size * sizeof(GedEvent));
> +    vms->ged_events_size = events_size;
> +}
can't you do the virt_acpi_ged_conf's job directly in virt_acpi_init
avoid this memcpy by using a pointer to a static const GedEvent
ged_events[]?
> +
>  static bool cpu_type_valid(const char *cpu)
>  {
>      int i;
> @@ -1650,6 +1671,7 @@ static void machvirt_init(MachineState *machine)
>      create_platform_bus(vms, pic);
>  
>      vms->acpi = virt_acpi_init();
> +    virt_acpi_ged_conf(vms);

>  
>      vms->bootinfo.ram_size = machine->ram_size;
>      vms->bootinfo.kernel_filename = machine->kernel_filename;
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index e46a051..49fda81 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -37,6 +37,7 @@
>  #include "hw/arm/arm.h"
>  #include "sysemu/kvm.h"
>  #include "hw/intc/arm_gicv3_common.h"
> +#include "hw/acpi/ged.h"
>  
>  #define NUM_GICV2M_SPIS       64
>  #define NUM_VIRTIO_TRANSPORTS 32
> @@ -78,6 +79,7 @@ enum {
>      VIRT_SECURE_UART,
>      VIRT_SECURE_MEM,
>      VIRT_PCDIMM_ACPI,
> +    VIRT_ACPI_GED,
>      VIRT_LOWMEMMAP_LAST,
>  };
>  
> @@ -134,6 +136,8 @@ typedef struct {
>      int psci_conduit;
>      hwaddr highest_gpa;
>      DeviceState *acpi;
> +    GedEvent *ged_events;
> +    uint32_t ged_events_size;
I guess we can avoid that one by using a const GedEvent *ged_events and
using ARRAY_SIZE?

Thanks

Eric
>  } VirtMachineState;
>  
>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
>