RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
> From: Zhao Liu
> Sent: Monday, May 6, 2024 10:06 AM
>
> Hi Salil,
>
> On Fri, May 03, 2024 at 07:59:32PM +, Salil Mehta wrote:
> > Date: Fri, 3 May 2024 19:59:32 +
> > From: Salil Mehta
> > Subject: RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to
> > support vCPU Hotplug
> >
> > Hello,
> >
> > Sorry, I missed this earlier.
> >
> > > From: Zhao Liu
> > > Sent: Wednesday, March 13, 2024 6:14 AM
> > > To: Salil Mehta
> > >
> > > Hi Salil,
> > >
> > > It seems my comment [1] in v7 was missed, but I still hit the same
> > > issue. Pls let me paste the previous comment here again.
> > >
> > > [1]: https://lore.kernel.org/qemu- devel/zxcqp32ggifvu...@intel.com/
> >
> > Yes, I have this in my mind.
> >
> > >
> > > [snip]
> > >
> > > > @@ -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);
> > > > +cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> > > > +&s->cpuhp_state, 0);
> > > > }
> > > >
> > >
> > > I find this cpu_hotplug_hw_init() can still cause qtest errors (for
> > > v8) on x86 platforms as you mentioned in v6:
> > >
> https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-820f4a254...@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6af3
> > >
> > > IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that
> > > is inherited from its parent x86 machine.
> > >
> > > The above error is because device-introspect-test sets the none-machine:
> > >
> > > # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-
> > > 3094820.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-
> > > 3094820.qmp,id=char0 -mon chardev=char0,mode=control -display none
> > > - audio none -nodefaults -machine none -accel qtest
> > >
> > > So what about just checking mc->possible_cpu_arch_ids instead of an
> > > assert in cpu_hotplug_hw_init()?
> > >
> > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
> > > 4b24a2500361..303f1f1f57bc 100644
> > > --- a/hw/acpi/cpu.c
> > > +++ b/hw/acpi/cpu.c
> > > @@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as,
> > > Object *owner,
> > > const CPUArchIdList *id_list;
> > > int i;
> > >
> > > -assert(mc->possible_cpu_arch_ids);
> > > +if (!mc->possible_cpu_arch_ids) {
> > > +return;
> > > +}
> > > +
> >
> >
> > Yes, we can do this with some debug print or trace maybe.
>
> Here it is just to return early without touching mc->possible_cpu_arch_ids().
> If you adopt this workaround, then in the meantime I suggest adding a
> comment to this "if" to clarify that it is for compatibility with certain
> machines that do not implement mc->possible_cpu_arch_ids().
sure.
>
> > > id_list = mc->possible_cpu_arch_ids(machine);
> > > state->dev_count = id_list->len;
> > > state->devs = g_new0(typeof(*state->devs), state->dev_count);
> > >
> > > This check seems to be acceptable in the general code path? Not all
> > > machines have possible_cpu_arch_ids, after all.
> >
> > True. BTW, have you tested this with Qtest?
>
> Yes, by "make check" on x86 platform. This workaround can help us pass the
> x86 tests.
thanks.
Best
Salil.
Re: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
Hi Salil,
On Fri, May 03, 2024 at 07:59:32PM +, Salil Mehta wrote:
> Date: Fri, 3 May 2024 19:59:32 +
> From: Salil Mehta
> Subject: RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support
> vCPU Hotplug
>
> Hello,
>
> Sorry, I missed this earlier.
>
> > From: Zhao Liu
> > Sent: Wednesday, March 13, 2024 6:14 AM
> > To: Salil Mehta
> >
> > Hi Salil,
> >
> > It seems my comment [1] in v7 was missed, but I still hit the same issue.
> > Pls
> > let me paste the previous comment here again.
> >
> > [1]: https://lore.kernel.org/qemu-devel/zxcqp32ggifvu...@intel.com/
>
> Yes, I have this in my mind.
>
> >
> > [snip]
> >
> > > @@ -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);
> > > +cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> > > +&s->cpuhp_state, 0);
> > > }
> > >
> >
> > I find this cpu_hotplug_hw_init() can still cause qtest errors (for v8) on
> > x86
> > platforms as you mentioned in v6:
> > https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-
> > 820f4a254...@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6a
> > f3
> >
> > IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that is
> > inherited from its parent x86 machine.
> >
> > The above error is because device-introspect-test sets the none-machine:
> >
> > # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-
> > 3094820.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-
> > 3094820.qmp,id=char0 -mon chardev=char0,mode=control -display none -
> > audio none -nodefaults -machine none -accel qtest
> >
> > So what about just checking mc->possible_cpu_arch_ids instead of an
> > assert in cpu_hotplug_hw_init()?
> >
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
> > 4b24a2500361..303f1f1f57bc 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as,
> > Object *owner,
> > const CPUArchIdList *id_list;
> > int i;
> >
> > -assert(mc->possible_cpu_arch_ids);
> > +if (!mc->possible_cpu_arch_ids) {
> > +return;
> > +}
> > +
>
>
> Yes, we can do this with some debug print or trace maybe.
Here it is just to return early without touching mc->possible_cpu_arch_ids().
If you adopt this workaround, then in the meantime I suggest adding a comment
to this "if" to clarify that it is for compatibility with certain machines
that do not implement mc->possible_cpu_arch_ids().
> > id_list = mc->possible_cpu_arch_ids(machine);
> > state->dev_count = id_list->len;
> > state->devs = g_new0(typeof(*state->devs), state->dev_count);
> >
> > This check seems to be acceptable in the general code path? Not all
> > machines have possible_cpu_arch_ids, after all.
>
> True. BTW, have you tested this with Qtest?
Yes, by "make check" on x86 platform. This workaround can help us pass
the x86 tests.
Regards,
Zhao
RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
Hi Vishnu,
> From: Vishnu Pajjuri
> Sent: Thursday, April 4, 2024 3:01 PM
> To: Salil Mehta ; qemu-devel@nongnu.org;
> qemu-...@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:zhukeqi...@huawei.com
>> Signed-off-by: Keqian Zhu mailto:zhukeqi...@huawei.com
>> Signed-off-by: Salil Mehta mailto:salil.me...@huawei.com
>> Reviewed-by: Jonathan Cameron mailto:jonathan.came...@huawei.com
>> Reviewed-by: Gavin Shan mailto:gs...@redhat.com
>> Reviewed-by: David Hildenbrand mailto:da...@redhat.com
>> Reviewed-by: Shaoqin Huang mailto:shahu...@redhat.com
>> Tested-by: Vishnu Pajjuri mailto:vis...@os.amperecomputing.com
>> Tested-by: Xianglai Li mailto:lixiang...@loongson.cn
>> Tested-by: Miguel Luis mailto:miguel.l...@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_STATU
RE: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
Hello,
Sorry, I missed this earlier.
> From: Zhao Liu
> Sent: Wednesday, March 13, 2024 6:14 AM
> To: Salil Mehta
>
> Hi Salil,
>
> It seems my comment [1] in v7 was missed, but I still hit the same issue. Pls
> let me paste the previous comment here again.
>
> [1]: https://lore.kernel.org/qemu-devel/zxcqp32ggifvu...@intel.com/
Yes, I have this in my mind.
>
> [snip]
>
> > @@ -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);
> > +cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> > +&s->cpuhp_state, 0);
> > }
> >
>
> I find this cpu_hotplug_hw_init() can still cause qtest errors (for v8) on
> x86
> platforms as you mentioned in v6:
> https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-
> 820f4a254...@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6a
> f3
>
> IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that is
> inherited from its parent x86 machine.
>
> The above error is because device-introspect-test sets the none-machine:
>
> # starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-
> 3094820.sock -qtest-log /dev/null -chardev socket,path=/tmp/qtest-
> 3094820.qmp,id=char0 -mon chardev=char0,mode=control -display none -
> audio none -nodefaults -machine none -accel qtest
>
> So what about just checking mc->possible_cpu_arch_ids instead of an
> assert in cpu_hotplug_hw_init()?
>
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index
> 4b24a2500361..303f1f1f57bc 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as,
> Object *owner,
> const CPUArchIdList *id_list;
> int i;
>
> -assert(mc->possible_cpu_arch_ids);
> +if (!mc->possible_cpu_arch_ids) {
> +return;
> +}
> +
Yes, we can do this with some debug print or trace maybe.
> id_list = mc->possible_cpu_arch_ids(machine);
> state->dev_count = id_list->len;
> state->devs = g_new0(typeof(*state->devs), state->dev_count);
>
> This check seems to be acceptable in the general code path? Not all
> machines have possible_cpu_arch_ids, after all.
True. BTW, have you tested this with Qtest?
Thanks
Salil.
Re: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
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
Signed-off-by: Keqian Zhu
Signed-off-by: Salil Mehta
Reviewed-by: Jonathan Cameron
Reviewed-by: Gavin Shan
Reviewed-by: David Hildenbrand
Reviewed-by: Shaoqin Huang
Tested-by: Vishnu Pajjuri
Tested-by: Xianglai Li
Tested-by: Miguel Luis
---
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_DEVIC
Re: [PATCH V8 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
Hi Salil,
It seems my comment [1] in v7 was missed, but I still hit the same
issue. Pls let me paste the previous comment here again.
[1]: https://lore.kernel.org/qemu-devel/zxcqp32ggifvu...@intel.com/
[snip]
> @@ -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);
> +cpu_hotplug_hw_init(&s->container_cpuhp, OBJECT(dev),
> +&s->cpuhp_state, 0);
> }
>
I find this cpu_hotplug_hw_init() can still cause qtest errors (for v8)
on x86 platforms as you mentioned in v6:
https://lore.kernel.org/qemu-devel/15e70616-6abb-63a4-17d0-820f4a254...@opnsrc.net/T/#m108f102b2fe92b7dd7218f2f942f7b233a9d6af3
IIUC, microvm machine has its own 'possible_cpus_arch_ids' and that is
inherited from its parent x86 machine.
The above error is because device-introspect-test sets the none-machine:
# starting QEMU: exec ./qemu-system-i386 -qtest unix:/tmp/qtest-3094820.sock
-qtest-log /dev/null -chardev socket,path=/tmp/qtest-3094820.qmp,id=char0 -mon
chardev=char0,mode=control -display none -audio none -nodefaults -machine none
-accel qtest
So what about just checking mc->possible_cpu_arch_ids instead of an
assert in cpu_hotplug_hw_init()?
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 4b24a2500361..303f1f1f57bc 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -221,7 +221,10 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
const CPUArchIdList *id_list;
int i;
-assert(mc->possible_cpu_arch_ids);
+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);
This check seems to be acceptable in the general code path? Not all machines
have possible_cpu_arch_ids, after all.
Thanks,
Zhao
