Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan

[Salil Mehta]

Hi Igor,


On 06/07/2024 14:28, Igor Mammedov wrote:
> On Fri, 7 Jun 2024 12:56:45 +0100
> Salil Mehta <salil.mehta@huawei.com> wrote:
>
> > OSPM evaluates _EVT method to map the event. The CPU hotplug event eventually
> > results in start of the CPU scan. Scan figures out the CPU and the kind of
> > event(plug/unplug) and notifies it back to the guest. Update the GED AML _EVT
> > method with the call to \\_SB.CPUS.CSCN
> >
> > Also, macro CPU_SCAN_METHOD might be referred in other places like during GED
> > intialization so it makes sense to have its definition placed in some common
> > header file like cpu_hotplug.h. But doing this can cause compilation break
> > because of the conflicting macro definitions present in cpu.c and cpu_hotplug.c
> one of the reasons is that you reusing legacy hw/acpi/cpu_hotplug.h,
> see below for suggestion.
>
> > and because both these files get compiled due to historic reasons of x86 world
> > i.e. decision to use legacy(GPE.2)/modern(GED) CPU hotplug interface happens
> > during runtime [1]. To mitigate above, for now, declare a new common macro
> > ACPI_CPU_SCAN_METHOD for CPU scan method instead.
> > (This needs a separate discussion later on for clean-up)
> >
> > Reference:
> > [1] 
> > https://lore.kernel.org/qemu-devel/1463496205-251412-24-git-send-email-imammedo@redhat.com/
> >
> > 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>
> > 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: Shaoqin Huang <shahuang@redhat.com>
> > Tested-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   hw/acpi/cpu.c                  | 2 +-
> >   hw/acpi/generic_event_device.c | 4 ++++
> >   include/hw/acpi/cpu_hotplug.h  | 2 ++
> >   3 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> > index 473b37ba88..af2b6655d2 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -327,7 +327,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
> >   #define CPUHP_RES_DEVICE  "PRES"
> >   #define CPU_LOCK          "CPLK"
> >   #define CPU_STS_METHOD    "CSTA"
> > -#define CPU_SCAN_METHOD   "CSCN"
> > +#define CPU_SCAN_METHOD   ACPI_CPU_SCAN_METHOD
> >   #define CPU_NOTIFY_METHOD "CTFY"
> >   #define CPU_EJECT_METHOD  "CEJ0"
> >   #define CPU_OST_METHOD    "COST"
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index 54d3b4bf9d..63226b0040 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -109,6 +109,10 @@ void build_ged_aml(Aml *table, const char *name, 
> > HotplugHandler *hotplug_dev,
> >                   aml_append(if_ctx, aml_call0(MEMORY_DEVICES_CONTAINER "."
> >                                                MEMORY_SLOT_SCAN_METHOD));
> >                   break;
> > +            case ACPI_GED_CPU_HOTPLUG_EVT:
> > +                aml_append(if_ctx, aml_call0(ACPI_CPU_CONTAINER "."
> > +                                             ACPI_CPU_SCAN_METHOD));
> I don't particularly like exposing cpu hotplug internals for outside code
> and then making that code do plumbing hoping that nothing will explode
> in the future.
>
> build_cpus_aml() takes event_handler_method to create a method that
> can be called by platform. What I suggest is to call that method here
> instead of trying to expose CPU hotplug internals and manually building
> call path here.
> aka:
>    build_cpus_aml(event_handler_method = PATH_TO_GED_DEVICE.CSCN)
> and then call here
>    aml_append(if_ctx, aml_call0(CSCN));
> which will call  CSCN in GED scope, that was be populated by
> build_cpus_aml() to do cpu scan properly without need to expose
> cpu hotplug internal names and then trying to fixup conflicts caused by that.
>
> PS:
> we should do the same for memory hotplug, we see in context above

In the x86 world and ARM, two different types of event handling 
mechanisms are
used: one based on GPE and the other on GED. The latter has its own 
placeholder
within the generic_event_device.c file, which also multiplexes other GED 
events.
Multiplexing AMLs for two different types of handlers into 
build_cpus_aml() seems
to be a fundamental mistake here. For CPU handling, this should also not 
be done
because x86 deals with legacy hotplug and modern hotplug differently but 
still
uses the same GPE-based event handling for both. Moreover, which type of 
handler
should be used depends upon the context from which build_cpus_aml() is 
called
and it would be ugly to demultiplex these inside the build_cpus_aml() code.

Therefore, my suggestion is:
1. Keep the CPU’s _EVT handling code in the generic_event_device.c as it is.
2. Pull out the event handler method and CPU scan call-related 
initialization entirely
    out of the CPU’s AML code (i.e., both in build_cpus_aml() and 
build_legacy_cpu_hotplug_aml()).
3. Remove the input parameter of event_handler in build_cpus_aml().
4. Create a separate function like build_gpe_aml() and use this function 
in the
     following places:

File: hw/i386/acpi-build.c

build_dsdt()
{
   [...]
    scope =  aml_scope("_GPE");
   [...]
    aml_append(dsdt, scope);

    if (pcmc->legacy_cpu_hotplug) {
        build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
    } else {
        CPUHotplugFeatures opts = {
            .acpi_1_compatible = true, .has_legacy_cphp = true,
            .smi_path = pm->smi_on_cpuhp ? "\\_SB.PCI0.SMI0.SMIC" : NULL,
            .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
        };
        build_cpus_aml(dsdt, machine, opts, pc_madt_cpu_entry,
-                       pm->cpu_hp_io_base, "\\_SB.PCI0", "\\_GPE._E02");
+                      pm->cpu_hp_io_base, "\\_SB.PCI0");
+      build_gpe_aml(...,, ,"\\_GPE._E02")

    }
   [...]
}

File: hw/acpi/cpu.c

@@ -343,9 +343,10 @@ const VMStateDescription vmstate_cpu_hotplug = {
 #define CPU_FW_EJECT_EVENT "CEJF"

 void build_cpus_aml(Aml *table, MachineState *machine, 
CPUHotplugFeatures opts,
-                    build_madt_cpu_fn build_madt_cpu, hwaddr io_base,
+                    build_madt_cpu_fn build_madt_cpu, hwaddr base_addr,
                     const char *res_root,
-                    const char *event_handler_method)
+                    AmlRegionSpace rs)
 {
     Aml *ifctx;
     Aml *field;


That said, if you still wish you proceed with your suggestions I can go
ahead and do it but please understand that I'll have to put a check
to avoid adding call to CPU Scan for am/virt platform since we would
want to add that call as part of the GED/AML code. This is unnecessary
and would look ugly.


Thanks
Salil.


> > +                break;
> >               case ACPI_GED_PWR_DOWN_EVT:
> >                   aml_append(if_ctx,
> >                              aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
> > diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
> > index 48b291e45e..ef631750b4 100644
> > --- a/include/hw/acpi/cpu_hotplug.h
> > +++ b/include/hw/acpi/cpu_hotplug.h
> > @@ -20,6 +20,8 @@
> >   #include "hw/acpi/cpu.h"
> >   
> >   #define ACPI_CPU_HOTPLUG_REG_LEN 12
> > +#define ACPI_CPU_SCAN_METHOD "CSCN"
> > +#define ACPI_CPU_CONTAINER "\\_SB.CPUS"
> >   
> >   typedef struct AcpiCpuHotplug {
> >       Object *device;