[PATCH V15 3/7] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

Salil Mehta via posted 7 patches 4 months, 1 week ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Richard Henderson <richard.henderson@linaro.org>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>
There is a newer version of this series
[PATCH V15 3/7] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
Posted by Salil Mehta via 4 months, 1 week ago
ACPI GED (as described in the ACPI 6.4 spec) uses an interrupt listed in the
_CRS object of GED to intimate OSPM about an event. Later then demultiplexes the
notified event by evaluating ACPI _EVT method to know the type of event. Use
ACPI GED to also notify the guest kernel about any CPU hot(un)plug events.

Note, GED interface is used by many hotplug events like memory hotplug, NVDIMM
hotplug and non-hotplug events like system power down event. Each of these can
be selected using a bit in the 32 bit GED IO interface. A bit has been reserved
for the CPU hotplug event.

ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
stub to avoid compilation break.

Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Tested-by: Zhao Liu <zhao1.liu@intel.com>
Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
---
 docs/specs/acpi_hw_reduced_hotplug.rst |  3 ++-
 hw/acpi/acpi-cpu-hotplug-stub.c        |  6 ++++++
 hw/acpi/generic_event_device.c         | 24 ++++++++++++++++++++++++
 include/hw/acpi/generic_event_device.h |  4 ++++
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
index 0bd3f9399f..3acd6fcd8b 100644
--- a/docs/specs/acpi_hw_reduced_hotplug.rst
+++ b/docs/specs/acpi_hw_reduced_hotplug.rst
@@ -64,7 +64,8 @@ GED IO interface (4 byte access)
        0: Memory hotplug event
        1: System power down event
        2: NVDIMM hotplug event
-    3-31: Reserved
+       3: CPU hotplug event
+    4-31: Reserved
 
 **write_access:**
 
diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
index 3fc4b14c26..c6c61bb9cd 100644
--- a/hw/acpi/acpi-cpu-hotplug-stub.c
+++ b/hw/acpi/acpi-cpu-hotplug-stub.c
@@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
     return;
 }
 
+void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
+                         CPUHotplugState *state, hwaddr base_addr)
+{
+    return;
+}
+
 void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
 {
     return;
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 2d6e91b124..1b31d633ba 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -25,6 +25,7 @@ static const uint32_t ged_supported_events[] = {
     ACPI_GED_MEM_HOTPLUG_EVT,
     ACPI_GED_PWR_DOWN_EVT,
     ACPI_GED_NVDIMM_HOTPLUG_EVT,
+    ACPI_GED_CPU_HOTPLUG_EVT,
 };
 
 /*
@@ -234,6 +235,8 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
         } else {
             acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
         }
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "virt: device plug request for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -248,6 +251,8 @@ static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,
     if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
                        !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {
         acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug request for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -261,6 +266,8 @@ static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,
 
     if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
         acpi_memory_unplug_cb(&s->memhp_state, dev, errp);
+    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
+        acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
     } else {
         error_setg(errp, "acpi: device unplug for unsupported device"
                    " type: %s", object_get_typename(OBJECT(dev)));
@@ -272,6 +279,7 @@ static void acpi_ged_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
     AcpiGedState *s = ACPI_GED(adev);
 
     acpi_memory_ospm_status(&s->memhp_state, list);
+    acpi_cpu_ospm_status(&s->cpuhp_state, list);
 }
 
 static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
@@ -286,6 +294,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
         sel = ACPI_GED_PWR_DOWN_EVT;
     } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
         sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
+    } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
+        sel = ACPI_GED_CPU_HOTPLUG_EVT;
     } else {
         /* Unknown event. Return without generating interrupt. */
         warn_report("GED: Unsupported event %d. No irq injected", ev);
@@ -371,6 +381,19 @@ static const VMStateDescription vmstate_acpi_ged = {
     }
 };
 
+static void acpi_ged_realize(DeviceState *dev, Error **errp)
+{
+    AcpiGedState *s = ACPI_GED(dev);
+    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+    /* initialize CPU Hotplug related regions */
+    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
+                       ACPI_CPU_HOTPLUG_REG_LEN);
+    sysbus_init_mmio(sbd, &s->container_cpuhp);
+    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
+                        &s->cpuhp_state, 0);
+}
+
 static void acpi_ged_initfn(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -411,6 +434,7 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)
     dc->desc = "ACPI Generic Event Device";
     device_class_set_props(dc, acpi_ged_properties);
     dc->vmsd = &vmstate_acpi_ged;
+    dc->realize = acpi_ged_realize;
 
     hc->plug = acpi_ged_device_plug_cb;
     hc->unplug_request = acpi_ged_unplug_request_cb;
diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index ba84ce0214..e091ac2108 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -62,6 +62,7 @@
 #include "hw/sysbus.h"
 #include "hw/acpi/memory_hotplug.h"
 #include "hw/acpi/ghes.h"
+#include "hw/acpi/cpu.h"
 #include "qom/object.h"
 
 #define ACPI_POWER_BUTTON_DEVICE "PWRB"
@@ -95,6 +96,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
 #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
 #define ACPI_GED_PWR_DOWN_EVT      0x2
 #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
+#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
 
 typedef struct GEDState {
     MemoryRegion evt;
@@ -106,6 +108,8 @@ struct AcpiGedState {
     SysBusDevice parent_obj;
     MemHotplugState memhp_state;
     MemoryRegion container_memhp;
+    CPUHotplugState cpuhp_state;
+    MemoryRegion container_cpuhp;
     GEDState ged_state;
     uint32_t ged_event_bitmap;
     qemu_irq irq;
-- 
2.34.1
Re: [PATCH V15 3/7] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
Posted by Igor Mammedov 4 months, 1 week ago
On Sat, 13 Jul 2024 19:25:12 +0100
Salil Mehta <salil.mehta@huawei.com> wrote:

> ACPI GED (as described in the ACPI 6.4 spec) uses an interrupt listed in the
> _CRS object of GED to intimate OSPM about an event. Later then demultiplexes the
> notified event by evaluating ACPI _EVT method to know the type of event. Use
> ACPI GED to also notify the guest kernel about any CPU hot(un)plug events.
> 
> Note, GED interface is used by many hotplug events like memory hotplug, NVDIMM
> hotplug and non-hotplug events like system power down event. Each of these can
> be selected using a bit in the 32 bit GED IO interface. A bit has been reserved
> for the CPU hotplug event.

> ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG
> support has been enabled for particular architecture. Add cpu_hotplug_hw_init()
> stub to avoid compilation break.

so any target (and machines in it) that has ACPI_CPU_HOTPLUG enabled will use have all CPU hotplug
machinery builtin which is fine.

However any machine that uses GED but do not opt-in into CPU hotplug,
will still have CPU hotplug registers/memory regions enabled/mapped. 

It's not much concern for upstream as migration from new to older QEMU
is not supported, however it will break migration downstream (arm/virt) as
new QEMU will try to migrate memory regions/state that do not exists
in older QEMU. Se below for suggestion.

> 
> Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Tested-by: Xianglai Li <lixianglai@loongson.cn>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Tested-by: Zhao Liu <zhao1.liu@intel.com>
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>  docs/specs/acpi_hw_reduced_hotplug.rst |  3 ++-
>  hw/acpi/acpi-cpu-hotplug-stub.c        |  6 ++++++
>  hw/acpi/generic_event_device.c         | 24 ++++++++++++++++++++++++
>  include/hw/acpi/generic_event_device.h |  4 ++++
>  4 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst b/docs/specs/acpi_hw_reduced_hotplug.rst
> index 0bd3f9399f..3acd6fcd8b 100644
> --- a/docs/specs/acpi_hw_reduced_hotplug.rst
> +++ b/docs/specs/acpi_hw_reduced_hotplug.rst
> @@ -64,7 +64,8 @@ GED IO interface (4 byte access)
>         0: Memory hotplug event
>         1: System power down event
>         2: NVDIMM hotplug event
> -    3-31: Reserved
> +       3: CPU hotplug event
> +    4-31: Reserved
>  
>  **write_access:**
>  
> diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
> index 3fc4b14c26..c6c61bb9cd 100644
> --- a/hw/acpi/acpi-cpu-hotplug-stub.c
> +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> @@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>      return;
>  }
>  
> +void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> +                         CPUHotplugState *state, hwaddr base_addr)
> +{
> +    return;
> +}
> +
>  void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list)
>  {
>      return;
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 2d6e91b124..1b31d633ba 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -25,6 +25,7 @@ static const uint32_t ged_supported_events[] = {
>      ACPI_GED_MEM_HOTPLUG_EVT,
>      ACPI_GED_PWR_DOWN_EVT,
>      ACPI_GED_NVDIMM_HOTPLUG_EVT,
> +    ACPI_GED_CPU_HOTPLUG_EVT,
>  };
>  
>  /*
> @@ -234,6 +235,8 @@ static void acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
>          } else {
>              acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev, errp);
>          }
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>      } else {
>          error_setg(errp, "virt: device plug request for unsupported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -248,6 +251,8 @@ static void acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,
>      if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>                         !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {
>          acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>      } else {
>          error_setg(errp, "acpi: device unplug request for unsupported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -261,6 +266,8 @@ static void acpi_ged_unplug_cb(HotplugHandler *hotplug_dev,
>  
>      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>          acpi_memory_unplug_cb(&s->memhp_state, dev, errp);
> +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
> +        acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
>      } else {
>          error_setg(errp, "acpi: device unplug for unsupported device"
>                     " type: %s", object_get_typename(OBJECT(dev)));
> @@ -272,6 +279,7 @@ static void acpi_ged_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
>      AcpiGedState *s = ACPI_GED(adev);
>  
>      acpi_memory_ospm_status(&s->memhp_state, list);
> +    acpi_cpu_ospm_status(&s->cpuhp_state, list);
>  }
>  
>  static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
> @@ -286,6 +294,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>          sel = ACPI_GED_PWR_DOWN_EVT;
>      } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
>          sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
> +    } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
> +        sel = ACPI_GED_CPU_HOTPLUG_EVT;
>      } else {
>          /* Unknown event. Return without generating interrupt. */
>          warn_report("GED: Unsupported event %d. No irq injected", ev);
> @@ -371,6 +381,19 @@ static const VMStateDescription vmstate_acpi_ged = {
>      }
>  };
>  
> +static void acpi_ged_realize(DeviceState *dev, Error **errp)
> +{
> +    AcpiGedState *s = ACPI_GED(dev);
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +

> +    /* initialize CPU Hotplug related regions */
> +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
> +                       ACPI_CPU_HOTPLUG_REG_LEN);
> +    sysbus_init_mmio(sbd, &s->container_cpuhp);
> +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> +                        &s->cpuhp_state, 0);

how about making it conditional on supported events
see hw/arm/virt.c: create_acpi_ged()
    if (ms->ram_slots) {                                                         
        event |= ACPI_GED_MEM_HOTPLUG_EVT;                                       
    }                                                                            
                                                                                 
    if (ms->nvdimms_state->is_enabled) {                                         
        event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;                                    
    } 

so cpu hotplug would have similar section
and ged realize could use ged-event to enable/disable
cpuhp feature registers and hwinit.

> +}
> +
>  static void acpi_ged_initfn(Object *obj)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -411,6 +434,7 @@ static void acpi_ged_class_init(ObjectClass *class, void *data)
>      dc->desc = "ACPI Generic Event Device";
>      device_class_set_props(dc, acpi_ged_properties);
>      dc->vmsd = &vmstate_acpi_ged;
> +    dc->realize = acpi_ged_realize;
>  
>      hc->plug = acpi_ged_device_plug_cb;
>      hc->unplug_request = acpi_ged_unplug_request_cb;
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index ba84ce0214..e091ac2108 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -62,6 +62,7 @@
>  #include "hw/sysbus.h"
>  #include "hw/acpi/memory_hotplug.h"
>  #include "hw/acpi/ghes.h"
> +#include "hw/acpi/cpu.h"
>  #include "qom/object.h"
>  
>  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> @@ -95,6 +96,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>  #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
>  #define ACPI_GED_PWR_DOWN_EVT      0x2
>  #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
> +#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
>  
>  typedef struct GEDState {
>      MemoryRegion evt;
> @@ -106,6 +108,8 @@ struct AcpiGedState {
>      SysBusDevice parent_obj;
>      MemHotplugState memhp_state;
>      MemoryRegion container_memhp;
> +    CPUHotplugState cpuhp_state;
> +    MemoryRegion container_cpuhp;
>      GEDState ged_state;
>      uint32_t ged_event_bitmap;
>      qemu_irq irq;
RE: [PATCH V15 3/7] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
Posted by Salil Mehta via 4 months, 1 week ago
Hi Igor,

Thanks for the feedback.

>  From: qemu-arm-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
>  arm-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
>  Mammedov
>  Sent: Monday, July 15, 2024 2:12 PM
>  To: Salil Mehta <salil.mehta@huawei.com>
>  
>  On Sat, 13 Jul 2024 19:25:12 +0100
>  Salil Mehta <salil.mehta@huawei.com> wrote:
>  
>  > ACPI GED (as described in the ACPI 6.4 spec) uses an interrupt listed
>  > in the _CRS object of GED to intimate OSPM about an event. Later then
>  > demultiplexes the notified event by evaluating ACPI _EVT method to
>  > know the type of event. Use ACPI GED to also notify the guest kernel
>  about any CPU hot(un)plug events.
>  >
>  > Note, GED interface is used by many hotplug events like memory
>  > hotplug, NVDIMM hotplug and non-hotplug events like system power
>  down
>  > event. Each of these can be selected using a bit in the 32 bit GED IO
>  > interface. A bit has been reserved for the CPU hotplug event.
>  
>  > ACPI CPU hotplug related initialization should only happen if
>  > ACPI_CPU_HOTPLUG support has been enabled for particular
>  architecture.
>  > Add cpu_hotplug_hw_init() stub to avoid compilation break.
>  
>  so any target (and machines in it) that has ACPI_CPU_HOTPLUG enabled will
>  use have all CPU hotplug machinery builtin which is fine.
>  
>  However any machine that uses GED but do not opt-in into CPU hotplug, will
>  still have CPU hotplug registers/memory regions enabled/mapped.
>  
>  It's not much concern for upstream as migration from new to older QEMU is
>  not supported, however it will break migration downstream (arm/virt) as
>  new QEMU will try to migrate memory regions/state that do not exists in
>  older QEMU. Se below for suggestion.


Sorry. I missed this one. Yes. agreed.


>  > Co-developed-by: Keqian Zhu <zhukeqian1@huawei.com>
>  > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>  > Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>  > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>  > Reviewed-by: Gavin Shan <gshan@redhat.com>
>  > Reviewed-by: David Hildenbrand <david@redhat.com>
>  > Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>  > Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > Tested-by: Xianglai Li <lixianglai@loongson.cn>
>  > Tested-by: Miguel Luis <miguel.luis@oracle.com>
>  > Reviewed-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>  > Tested-by: Zhao Liu <zhao1.liu@intel.com>
>  > Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
>  > ---
>  >  docs/specs/acpi_hw_reduced_hotplug.rst |  3 ++-
>  >  hw/acpi/acpi-cpu-hotplug-stub.c        |  6 ++++++
>  >  hw/acpi/generic_event_device.c         | 24 ++++++++++++++++++++++++
>  >  include/hw/acpi/generic_event_device.h |  4 ++++
>  >  4 files changed, 36 insertions(+), 1 deletion(-)
>  >
>  > diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst
>  > b/docs/specs/acpi_hw_reduced_hotplug.rst
>  > index 0bd3f9399f..3acd6fcd8b 100644
>  > --- a/docs/specs/acpi_hw_reduced_hotplug.rst
>  > +++ b/docs/specs/acpi_hw_reduced_hotplug.rst
>  > @@ -64,7 +64,8 @@ GED IO interface (4 byte access)
>  >         0: Memory hotplug event
>  >         1: System power down event
>  >         2: NVDIMM hotplug event
>  > -    3-31: Reserved
>  > +       3: CPU hotplug event
>  > +    4-31: Reserved
>  >
>  >  **write_access:**
>  >
>  > diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c
>  > b/hw/acpi/acpi-cpu-hotplug-stub.c index 3fc4b14c26..c6c61bb9cd 100644
>  > --- a/hw/acpi/acpi-cpu-hotplug-stub.c
>  > +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
>  > @@ -19,6 +19,12 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion
>  *parent, Object *owner,
>  >      return;
>  >  }
>  >
>  > +void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
>  > +                         CPUHotplugState *state, hwaddr base_addr) {
>  > +    return;
>  > +}
>  > +
>  >  void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList
>  > ***list)  {
>  >      return;
>  > diff --git a/hw/acpi/generic_event_device.c
>  > b/hw/acpi/generic_event_device.c index 2d6e91b124..1b31d633ba
>  100644
>  > --- a/hw/acpi/generic_event_device.c
>  > +++ b/hw/acpi/generic_event_device.c
>  > @@ -25,6 +25,7 @@ static const uint32_t ged_supported_events[] = {
>  >      ACPI_GED_MEM_HOTPLUG_EVT,
>  >      ACPI_GED_PWR_DOWN_EVT,
>  >      ACPI_GED_NVDIMM_HOTPLUG_EVT,
>  > +    ACPI_GED_CPU_HOTPLUG_EVT,
>  >  };
>  >
>  >  /*
>  > @@ -234,6 +235,8 @@ static void
>  acpi_ged_device_plug_cb(HotplugHandler *hotplug_dev,
>  >          } else {
>  >              acpi_memory_plug_cb(hotplug_dev, &s->memhp_state, dev,
>  errp);
>  >          }
>  > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>  > +        acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>  >      } else {
>  >          error_setg(errp, "virt: device plug request for unsupported device"
>  >                     " type: %s", object_get_typename(OBJECT(dev)));
>  > @@ -248,6 +251,8 @@ static void
>  acpi_ged_unplug_request_cb(HotplugHandler *hotplug_dev,
>  >      if ((object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM) &&
>  >                         !(object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)))) {
>  >          acpi_memory_unplug_request_cb(hotplug_dev, &s->memhp_state,
>  > dev, errp);
>  > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>  > +        acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev,
>  > + errp);
>  >      } else {
>  >          error_setg(errp, "acpi: device unplug request for unsupported
>  device"
>  >                     " type: %s", object_get_typename(OBJECT(dev)));
>  > @@ -261,6 +266,8 @@ static void acpi_ged_unplug_cb(HotplugHandler
>  > *hotplug_dev,
>  >
>  >      if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
>  >          acpi_memory_unplug_cb(&s->memhp_state, dev, errp);
>  > +    } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>  > +        acpi_cpu_unplug_cb(&s->cpuhp_state, dev, errp);
>  >      } else {
>  >          error_setg(errp, "acpi: device unplug for unsupported device"
>  >                     " type: %s", object_get_typename(OBJECT(dev)));
>  > @@ -272,6 +279,7 @@ static void acpi_ged_ospm_status(AcpiDeviceIf
>  *adev, ACPIOSTInfoList ***list)
>  >      AcpiGedState *s = ACPI_GED(adev);
>  >
>  >      acpi_memory_ospm_status(&s->memhp_state, list);
>  > +    acpi_cpu_ospm_status(&s->cpuhp_state, list);
>  >  }
>  >
>  >  static void acpi_ged_send_event(AcpiDeviceIf *adev,
>  > AcpiEventStatusBits ev) @@ -286,6 +294,8 @@ static void
>  acpi_ged_send_event(AcpiDeviceIf *adev, AcpiEventStatusBits ev)
>  >          sel = ACPI_GED_PWR_DOWN_EVT;
>  >      } else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
>  >          sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
>  > +    } else if (ev & ACPI_CPU_HOTPLUG_STATUS) {
>  > +        sel = ACPI_GED_CPU_HOTPLUG_EVT;
>  >      } else {
>  >          /* Unknown event. Return without generating interrupt. */
>  >          warn_report("GED: Unsupported event %d. No irq injected",
>  > ev); @@ -371,6 +381,19 @@ static const VMStateDescription
>  vmstate_acpi_ged = {
>  >      }
>  >  };
>  >
>  > +static void acpi_ged_realize(DeviceState *dev, Error **errp) {
>  > +    AcpiGedState *s = ACPI_GED(dev);
>  > +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>  > +
>  
>  > +    /* initialize CPU Hotplug related regions */
>  > +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp
>  container",
>  > +                       ACPI_CPU_HOTPLUG_REG_LEN);
>  > +    sysbus_init_mmio(sbd, &s->container_cpuhp);
>  > +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>  > +                        &s->cpuhp_state, 0);
>  
>  how about making it conditional on supported events see hw/arm/virt.c:
>  create_acpi_ged()
>      if (ms->ram_slots) {
>          event |= ACPI_GED_MEM_HOTPLUG_EVT;
>      }
>  
>      if (ms->nvdimms_state->is_enabled) {
>          event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
>      }
>  
>  so cpu hotplug would have similar section and ged realize could use ged-
>  event to enable/disable cpuhp feature registers and hwinit.


Got it, thanks for pointing out. I will fix this and share V16 by COB today itself.


Best regards
Salil.


>  
>  > +}
>  > +
>  >  static void acpi_ged_initfn(Object *obj)  {
>  >      DeviceState *dev = DEVICE(obj);
>  > @@ -411,6 +434,7 @@ static void acpi_ged_class_init(ObjectClass *class,
>  void *data)
>  >      dc->desc = "ACPI Generic Event Device";
>  >      device_class_set_props(dc, acpi_ged_properties);
>  >      dc->vmsd = &vmstate_acpi_ged;
>  > +    dc->realize = acpi_ged_realize;
>  >
>  >      hc->plug = acpi_ged_device_plug_cb;
>  >      hc->unplug_request = acpi_ged_unplug_request_cb; diff --git
>  > a/include/hw/acpi/generic_event_device.h
>  > b/include/hw/acpi/generic_event_device.h
>  > index ba84ce0214..e091ac2108 100644
>  > --- a/include/hw/acpi/generic_event_device.h
>  > +++ b/include/hw/acpi/generic_event_device.h
>  > @@ -62,6 +62,7 @@
>  >  #include "hw/sysbus.h"
>  >  #include "hw/acpi/memory_hotplug.h"
>  >  #include "hw/acpi/ghes.h"
>  > +#include "hw/acpi/cpu.h"
>  >  #include "qom/object.h"
>  >
>  >  #define ACPI_POWER_BUTTON_DEVICE "PWRB"
>  > @@ -95,6 +96,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState,
>  ACPI_GED)
>  >  #define ACPI_GED_MEM_HOTPLUG_EVT   0x1
>  >  #define ACPI_GED_PWR_DOWN_EVT      0x2
>  >  #define ACPI_GED_NVDIMM_HOTPLUG_EVT 0x4
>  > +#define ACPI_GED_CPU_HOTPLUG_EVT    0x8
>  >
>  >  typedef struct GEDState {
>  >      MemoryRegion evt;
>  > @@ -106,6 +108,8 @@ struct AcpiGedState {
>  >      SysBusDevice parent_obj;
>  >      MemHotplugState memhp_state;
>  >      MemoryRegion container_memhp;
>  > +    CPUHotplugState cpuhp_state;
>  > +    MemoryRegion container_cpuhp;
>  >      GEDState ged_state;
>  >      uint32_t ged_event_bitmap;
>  >      qemu_irq irq;
>