RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug

[Salil Mehta <salil.mehta@huawei.com>]

Hi Vishnu,

>  From: Vishnu Pajjuri <vishnu@amperemail.onmicrosoft.com> 
>  Sent: Thursday, April 4, 2024 3:01 PM
>  To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; qemu-arm@nongnu.org
>  
>  Hi Salil,
>>  On 12-03-2024 07:29, Salil Mehta 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.
>>  
>>  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 mailto:zhukeqian1@huawei.com
>>  Signed-off-by: Keqian Zhu mailto:zhukeqian1@huawei.com
>>  Signed-off-by: Salil Mehta mailto:salil.mehta@huawei.com
>>  Reviewed-by: Jonathan Cameron mailto:Jonathan.Cameron@huawei.com
>>  Reviewed-by: Gavin Shan mailto:gshan@redhat.com
>>  Reviewed-by: David Hildenbrand mailto:david@redhat.com
>>  Reviewed-by: Shaoqin Huang mailto:shahuang@redhat.com
>>  Tested-by: Vishnu Pajjuri mailto:vishnu@os.amperecomputing.com
>>  Tested-by: Xianglai Li mailto:lixianglai@loongson.cn
>>  Tested-by: Miguel Luis mailto:miguel.luis@oracle.com
>>  ---
>>   hw/acpi/acpi-cpu-hotplug-stub.c>    |  6 ++++++
>>   hw/acpi/generic_event_device.c>     | 17 +++++++++++++++++
>>   include/hw/acpi/generic_event_device.h |  4 ++++
>>   3 files changed, 27 insertions(+)
>>  
>>  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..35a71505a5 100644
>>  --- a/hw/acpi/generic_event_device.c
>>  +++ b/hw/acpi/generic_event_device.c
>>  @@ -12,6 +12,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qapi/error.h"
>>   #include "hw/acpi/acpi.h"
>>  +#include "hw/acpi/cpu.h"
>>   #include "hw/acpi/generic_event_device.h"
>>   #include "hw/irq.h"
>>   #include "hw/mem/pc-dimm.h"
>>  @@ -25,6 +26,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 +236,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 +252,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 +267,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 +280,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 +295,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);
>>  @@ -400,6 +411,12 @@ static void acpi_ged_initfn(Object *obj)
>>       memory_region_init_io(&ged_st->regs, obj, &ged_regs_ops, ged_st,
>>             TYPE_ACPI_GED "-regs", ACPI_GED_REG_COUNT);
>>       sysbus_init_mmio(sbd, &ged_st->regs);
>>  +
>>  +    memory_region_init(&s->container_cpuhp, OBJECT(dev), "cpuhp container",
>>  +           ACPI_CPU_HOTPLUG_REG_LEN);
>>  +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->container_cpuhp);
>  same sbd can be used here instead of SYS_BUS_DEVICE(dev).


    Yes, we can.
	

>>  +    cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>>  +        &s->cpuhp_state, 0);
>>   }
>>   
>>   static void acpi_ged_class_init(ObjectClass *class, void *data)
>>  diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
>>  index ba84ce0214..90fc41cbb8 100644
>>  --- a/include/hw/acpi/generic_event_device.h
>>  +++ b/include/hw/acpi/generic_event_device.h
>>  @@ -60,6 +60,7 @@
>>   #define HW_ACPI_GENERIC_EVENT_DEVICE_H
>>   
>>   #include "hw/sysbus.h"
>>  +#include "hw/acpi/cpu_hotplug.h"
>>   #include "hw/acpi/memory_hotplug.h"
>>   #include "hw/acpi/ghes.h"
>>   #include "qom/object.h"
>>  @@ -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;
>>  Otherwise, Looks good to me.  Feel free to add
>>  Reviewed-by: "Vishnu Pajjuri" mailto:vishnu@os.amperecomputing.com

Thanks.


>>  Regards,
>>  -Vishnu