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

[Salil Mehta]

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;