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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 11 Jul 2024 03:29:40 +0000
Salil Mehta <salil.mehta@opnsrc.net> wrote:

> 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

I'm not convinced by above arguments yet (perhaps I don't see a problem
you are observing), so I'd like to keep cpu hotplug isolated/not exposed
as much as possible unless it's proven that it will not work as is.
(see for more below) 
 
> 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.
I just don't get why you have to call CPU Scan explicitly from arm/virt
side. (maybe my suggestion was lost in translation/was not clear enough)
Let's pretend that hw/i386/acpi-build.c is an arm/virt code and demo
what I was suggesting.


diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 2d6e91b124..33addb6275 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -117,6 +117,9 @@ void build_ged_aml(Aml *table, const char *name, HotplugHandler *hotplug_dev,
                            aml_notify(aml_name("\\_SB.NVDR"),
                                       aml_int(0x80)));
                 break;
+            case ACPI_GED_CPU_HOTPLUG_EVT:
+                aml_append(if_ctx, aml_call0("\\_SB.GED.CPEV"));
+                break
             default:
                 /*
                  * Please make sure all the events in ged_supported_events[]
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f4e366f64f..8b4f422652 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1536,7 +1536,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
             .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", "\\_SB.GED.CPEV");
     }
 
this way build_cpus_aml() will create and populate with scan
\\_SB.GED.CPEV method.

which is then called by 
GED._EVT()
    ...
    if cpuhotplug
          \\_SB.GED.CPEV()
    ...

there is no multiplexing of cpuhp event in build_cpus_aml,
the only event multiplexer is GED._EVT() as it should be.
And you don't have to expose cpu hotplug internals to any
other place[s].

PS:
For legacy cphp handling acpi/cpu.c has only _INI method
that is created when opts.has_legacy_cphp is true.
we should be able to get rid of it when 2.6 machine type is removed.
But ARM [or anything else] don't have to be aware of it
if you use static initializer like it's done in hw/i386/acpi-build.c
and just ignore non relevant fields.



> 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;  
>