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 <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>
---
hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
hw/acpi/cpu.c | 6 +++++-
hw/acpi/generic_event_device.c | 17 +++++++++++++++++
include/hw/acpi/generic_event_device.h | 4 ++++
4 files changed, 32 insertions(+), 1 deletion(-)
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/cpu.c b/hw/acpi/cpu.c
index 69aaa563db..473b37ba88 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -221,7 +221,11 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
const CPUArchIdList *id_list;
int i;
- assert(mc->possible_cpu_arch_ids);
+ /* hotplug might not be available for all types like x86/microvm etc. */
+ if (!mc->possible_cpu_arch_ids) {
+ return;
+ }
+
id_list = mc->possible_cpu_arch_ids(machine);
state->dev_count = id_list->len;
state->devs = g_new0(typeof(*state->devs), state->dev_count);
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 2d6e91b124..54d3b4bf9d 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(sbd, &s->container_cpuhp);
+ 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;
--
2.34.1On Fri, 7 Jun 2024 12:56:44 +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.
>
> 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>
> ---
> hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
> hw/acpi/cpu.c | 6 +++++-
> hw/acpi/generic_event_device.c | 17 +++++++++++++++++
> include/hw/acpi/generic_event_device.h | 4 ++++
> 4 files changed, 32 insertions(+), 1 deletion(-)
>
> 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/cpu.c b/hw/acpi/cpu.c
> index 69aaa563db..473b37ba88 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -221,7 +221,11 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> const CPUArchIdList *id_list;
> int i;
>
> - assert(mc->possible_cpu_arch_ids);
> + /* hotplug might not be available for all types like x86/microvm etc. */
> + if (!mc->possible_cpu_arch_ids) {
> + return;
> + }
if hotplug is not supported, this function shouldn't be called at all.
[...]
> @@ -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(sbd, &s->container_cpuhp);
> + cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> + &s->cpuhp_state, 0);
suggest to move this call to realize time, and gate it on
ACPI_GED_CPU_HOTPLUG_EVT being set.
Platform that supports cpu hotplug must optin, setting ACPI_GED_CPU_HOTPLUG_EVT,
while for the rest it will be ignored.
for example: create_acpi_ged() : event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
> }
>
> 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;
On 06/07/2024 13:46, Igor Mammedov wrote:
> On Fri, 7 Jun 2024 12:56:44 +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.
>>
>> 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>
>> ---
>> hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
>> hw/acpi/cpu.c | 6 +++++-
>> hw/acpi/generic_event_device.c | 17 +++++++++++++++++
>> include/hw/acpi/generic_event_device.h | 4 ++++
>> 4 files changed, 32 insertions(+), 1 deletion(-)
>>
>> 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/cpu.c b/hw/acpi/cpu.c
>> index 69aaa563db..473b37ba88 100644
>> --- a/hw/acpi/cpu.c
>> +++ b/hw/acpi/cpu.c
>> @@ -221,7 +221,11 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
>> const CPUArchIdList *id_list;
>> int i;
>>
>> - assert(mc->possible_cpu_arch_ids);
>> + /* hotplug might not be available for all types like x86/microvm etc. */
>> + if (!mc->possible_cpu_arch_ids) {
>> + return;
>> + }
> if hotplug is not supported, this function shouldn't be called at all.
True. But none the less this gets called for Intel/microvm and causes
qtest to fail.
I think, we've had this discussion before last year as well. Please
check below:
https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-820f4a254607@opnsrc.net/
>
> [...]
>> @@ -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(sbd, &s->container_cpuhp);
>> + cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
>> + &s->cpuhp_state, 0);
> suggest to move this call to realize time, and gate it on
> ACPI_GED_CPU_HOTPLUG_EVT being set.
> Platform that supports cpu hotplug must optin, setting ACPI_GED_CPU_HOTPLUG_EVT,
> while for the rest it will be ignored.
>
> for example: create_acpi_ged() : event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
Similar case applies to the Memory hotplug as well and any cleaning here
will mean going beyond the realms of this patch-set. But I can definitely
take this activity in a separate patch-set if you wish?
Thanks
>
>> }
>>
>> 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;
On Mon, 8 Jul 2024 05:12:48 +0000
Salil Mehta <salil.mehta@opnsrc.net> wrote:
> On 06/07/2024 13:46, Igor Mammedov wrote:
> > On Fri, 7 Jun 2024 12:56:44 +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.
> >>
> >> 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>
> >> ---
> >> hw/acpi/acpi-cpu-hotplug-stub.c | 6 ++++++
> >> hw/acpi/cpu.c | 6 +++++-
> >> hw/acpi/generic_event_device.c | 17 +++++++++++++++++
> >> include/hw/acpi/generic_event_device.h | 4 ++++
> >> 4 files changed, 32 insertions(+), 1 deletion(-)
> >>
> >> 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/cpu.c b/hw/acpi/cpu.c
> >> index 69aaa563db..473b37ba88 100644
> >> --- a/hw/acpi/cpu.c
> >> +++ b/hw/acpi/cpu.c
> >> @@ -221,7 +221,11 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
> >> const CPUArchIdList *id_list;
> >> int i;
> >>
> >> - assert(mc->possible_cpu_arch_ids);
> >> + /* hotplug might not be available for all types like x86/microvm etc. */
> >> + if (!mc->possible_cpu_arch_ids) {
> >> + return;
> >> + }
> > if hotplug is not supported, this function shouldn't be called at all.
>
> True. But none the less this gets called for Intel/microvm and causes
> qtest to fail.
>
> I think, we've had this discussion before last year as well. Please
> check below:
>
> https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-820f4a254607@opnsrc.net/
And I see that I had the same objection,
'
cpu_hotplug_hw_init() should not be called at initfn time,
but rather at realize time.
'
> >
> > [...]
> >> @@ -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(sbd, &s->container_cpuhp);
> >> + cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> >> + &s->cpuhp_state, 0);
> > suggest to move this call to realize time, and gate it on
> > ACPI_GED_CPU_HOTPLUG_EVT being set.
> > Platform that supports cpu hotplug must optin, setting ACPI_GED_CPU_HOTPLUG_EVT,
> > while for the rest it will be ignored.
which I've just suggested again ^^^.
> >
> > for example: create_acpi_ged() : event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
>
> Similar case applies to the Memory hotplug as well and any cleaning here
>
> will mean going beyond the realms of this patch-set. But I can definitely
>
> take this activity in a separate patch-set if you wish?
For memory hotplug cleanup it's fine to be separate series,
but for cpu hotplug parts you are touching I'd very much prefer
done it right from the start.
That might also help to reduce code churn within this series.
>
> Thanks
>
> >
> >> }
> >>
> >> 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;
>
On Fri Jun 7, 2024 at 9:56 PM AEST, Salil Mehta wrote: > @@ -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(sbd, &s->container_cpuhp); > + cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev), > + &s->cpuhp_state, 0); > } Could the ACPI persistent presence ARM requires be a property of the ACPI device? Thanks, Nick
HI Nick, > From: Nicholas Piggin <npiggin@gmail.com> > Sent: Thursday, July 4, 2024 4:03 AM > To: Salil Mehta <salil.mehta@huawei.com>; qemu-devel@nongnu.org; > qemu-arm@nongnu.org > > On Fri Jun 7, 2024 at 9:56 PM AEST, Salil Mehta wrote: > > @@ -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(sbd, &s->container_cpuhp); > > + cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev), > > + &s->cpuhp_state, 0); > > } > > Could the ACPI persistent presence ARM requires be a property of the ACPI > device? I think it is more of a CPU property rather than a GED device? > > Thanks, > Nick
© 2016 - 2024 Red Hat, Inc.