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

Salil Mehta via posted 8 patches 5 months, 2 weeks ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Richard Henderson <richard.henderson@linaro.org>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>
There is a newer version of this series
[PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
Posted by Salil Mehta via 5 months, 2 weeks ago
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
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));
+                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;
-- 
2.34.1
Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
Posted by Igor Mammedov 4 months, 3 weeks ago
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


> +                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;
Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
Posted by Salil Mehta 4 months, 2 weeks ago
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;

Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
Posted by Igor Mammedov 4 months, 2 weeks ago
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;  
> 
Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
Posted by Salil Mehta 4 months, 2 weeks ago
On 11/07/2024 08:34, Igor Mammedov wrote:
> 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>

[...]

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


I see. Perhaps, I failed to realise the semantics in the AML
sense i.e. exporting a call to a method belonging to one scope
to another (\\_SB.CPUS to \\_SB.GED). Yes, with above change
we can avoid calling \\_SB.CPUS.CSCN method direclty from
within GED _EVT. Sorry for the noise.

I will do the change.

Thanks


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

ok. thanks for the clarification.

Best regards
Salil.
Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
Posted by Salil Mehta 4 months, 2 weeks ago
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.
ok
>
>> 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.

I understand your point but I've followed what was already existing.

For example,

build_dsdt()

{

        [...]

     acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
                        (irqmap[VIRT_UART] + ARM_SPI_BASE));
     if (vmc->acpi_expose_flash) {
         acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
     }
     fw_cfg_acpi_dsdt_add(scope, &memmap[VIRT_FW_CFG]);
     virtio_acpi_dsdt_add(scope, memmap[VIRT_MMIO].base, 
memmap[VIRT_MMIO].size,
                          (irqmap[VIRT_MMIO] + ARM_SPI_BASE),
                          0, NUM_VIRTIO_TRANSPORTS);
     acpi_dsdt_add_pci(scope, memmap, irqmap[VIRT_PCIE] + ARM_SPI_BASE, 
vms);
     if (vms->acpi_dev) {
         build_ged_aml(scope, "\\_SB."GED_DEVICE,
                       HOTPLUG_HANDLER(vms->acpi_dev),
                       irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, 
AML_SYSTEM_MEMORY,
                       memmap[VIRT_ACPI_GED].base);
     } else {
         acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
                            (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
     }

       [...]

}

Refactoring all of this code will create a noise in this patch-set.
> 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
>
Although. I agree with your suggested change but I think this should be

carried in another patch-set.

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

Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
Posted by Igor Mammedov 4 months, 2 weeks ago
On Mon, 8 Jul 2024 05:21:06 +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.  
> ok
> >  
> >> 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.  
> 
> I understand your point but I've followed what was already existing.
> 
> For example,
> 
> build_dsdt()
> 
> {
> 
>         [...]
> 
>      acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
>                         (irqmap[VIRT_UART] + ARM_SPI_BASE));
>      if (vmc->acpi_expose_flash) {
>          acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
>      }
>      fw_cfg_acpi_dsdt_add(scope, &memmap[VIRT_FW_CFG]);
>      virtio_acpi_dsdt_add(scope, memmap[VIRT_MMIO].base, 
> memmap[VIRT_MMIO].size,
>                           (irqmap[VIRT_MMIO] + ARM_SPI_BASE),
>                           0, NUM_VIRTIO_TRANSPORTS);
>      acpi_dsdt_add_pci(scope, memmap, irqmap[VIRT_PCIE] + ARM_SPI_BASE, 
> vms);
>      if (vms->acpi_dev) {
>          build_ged_aml(scope, "\\_SB."GED_DEVICE,
>                        HOTPLUG_HANDLER(vms->acpi_dev),
>                        irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, 
> AML_SYSTEM_MEMORY,
>                        memmap[VIRT_ACPI_GED].base);
>      } else {
>          acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>                             (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
>      }
> 
>        [...]
> 
> }
> 
> Refactoring all of this code will create a noise in this patch-set.
> > 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
> >  
> Although. I agree with your suggested change but I think this should be
> 
> carried in another patch-set.

I have to disagree with another patch-set on top as it introduces
unnecessary code changes, wich 'another patch-set' will put back.
(talking specifically about CPU hotplug)

Not to mention doing as it was suggested should reduce size of
your series and overall complexity.

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