[PULL,v2,40/61] hw/acpi: Update GED _EVT method AML with CPU scan

Commit Message

Michael S. Tsirkin July 23, 2024, 10:59 a.m. UTC

From: Salil Mehta <[e-mail]>

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 method \\_SB.CPUS.CSCN (via \\_SB.GED.CSCN)

Architecture specific code [1] might initialize its CPUs AML code by calling
common function build_cpus_aml() like below for ARM:

build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry, memmap[VIRT_CPUHP_ACPI].base,
               "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);

[1] https://lore.kernel.org/qemu-devel/[e-mail]/

Co-developed-by: Keqian Zhu <[e-mail]>
Signed-off-by: Keqian Zhu <[e-mail]>
Signed-off-by: Salil Mehta <[e-mail]>
Reviewed-by: Jonathan Cameron <[e-mail]>
Reviewed-by: Gavin Shan <[e-mail]>
Tested-by: Vishnu Pajjuri <[e-mail]>
Tested-by: Xianglai Li <[e-mail]>
Tested-by: Miguel Luis <[e-mail]>
Reviewed-by: Shaoqin Huang <[e-mail]>
Tested-by: Zhao Liu <[e-mail]>
Reviewed-by: Igor Mammedov <[e-mail]>
Message-Id: <[e-mail]>
Reviewed-by: Michael S. Tsirkin <[e-mail]>
Signed-off-by: Michael S. Tsirkin <[e-mail]>
---
 include/hw/acpi/generic_event_device.h | 1 +
 hw/acpi/generic_event_device.c         | 3 +++
 2 files changed, 4 insertions(+)

Comments

Bibo Mao Oct. 14, 2024, 8:52 a.m. UTC | #1

Hi Salil,

When I debug cpu hotplug on LoongArch system, It reports error like this:
     ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN], 
AE_NOT_FOUND
     ACPI Error: Aborting method \_SB.GED._EVT due to previous error 
(AE_NOT_FOUND)
     acpi-ged ACPI0013:00: IRQ method execution failed


With this patch, GED CPU call method is "\\_SB.GED.CSCN", however in 
function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".
     method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
     aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
     aml_append(table, method);

It seems that CPU scanning method name is not consistent between 
function build_cpus_aml() and build_ged_aml().

Regards
Bibo Mao

On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:
> From: Salil Mehta <[e-mail]>
> 
> 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 method \\_SB.CPUS.CSCN (via \\_SB.GED.CSCN)
> 
> Architecture specific code [1] might initialize its CPUs AML code by calling
> common function build_cpus_aml() like below for ARM:
> 
> build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry, memmap[VIRT_CPUHP_ACPI].base,
>                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);
> 
> [1] https://lore.kernel.org/qemu-devel/[e-mail]/
> 
> Co-developed-by: Keqian Zhu <[e-mail]>
> Signed-off-by: Keqian Zhu <[e-mail]>
> Signed-off-by: Salil Mehta <[e-mail]>
> Reviewed-by: Jonathan Cameron <[e-mail]>
> Reviewed-by: Gavin Shan <[e-mail]>
> Tested-by: Vishnu Pajjuri <[e-mail]>
> Tested-by: Xianglai Li <[e-mail]>
> Tested-by: Miguel Luis <[e-mail]>
> Reviewed-by: Shaoqin Huang <[e-mail]>
> Tested-by: Zhao Liu <[e-mail]>
> Reviewed-by: Igor Mammedov <[e-mail]>
> Message-Id: <[e-mail]>
> Reviewed-by: Michael S. Tsirkin <[e-mail]>
> Signed-off-by: Michael S. Tsirkin <[e-mail]>
> ---
>   include/hw/acpi/generic_event_device.h | 1 +
>   hw/acpi/generic_event_device.c         | 3 +++
>   2 files changed, 4 insertions(+)
> 
> diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> index e091ac2108..40af3550b5 100644
> --- a/include/hw/acpi/generic_event_device.h
> +++ b/include/hw/acpi/generic_event_device.h
> @@ -87,6 +87,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
>   #define GED_DEVICE      "GED"
>   #define AML_GED_EVT_REG "EREG"
>   #define AML_GED_EVT_SEL "ESEL"
> +#define AML_GED_EVT_CPU_SCAN_METHOD "\\_SB.GED.CSCN"
>   
>   /*
>    * Platforms need to specify the GED event bitmap
> diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> index 4641933a0f..15b4c3ebbf 100644
> --- a/hw/acpi/generic_event_device.c
> +++ b/hw/acpi/generic_event_device.c
> @@ -108,6 +108,9 @@ 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(AML_GED_EVT_CPU_SCAN_METHOD));
> +                break;
>               case ACPI_GED_PWR_DOWN_EVT:
>                   aml_append(if_ctx,
>                              aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
>

Igor Mammedov Oct. 14, 2024, 9:37 a.m. UTC | #2

On Mon, 14 Oct 2024 16:52:55 +0800
maobibo <[e-mail]> wrote:

> Hi Salil,
> 
> When I debug cpu hotplug on LoongArch system, It reports error like this:
>      ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN], 
> AE_NOT_FOUND
>      ACPI Error: Aborting method \_SB.GED._EVT due to previous error 
> (AE_NOT_FOUND)
>      acpi-ged ACPI0013:00: IRQ method execution failed
> 
> 
> With this patch, GED CPU call method is "\\_SB.GED.CSCN", however in 
> function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".
>      method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
>      aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
>      aml_append(table, method);
> 
> It seems that CPU scanning method name is not consistent between 
> function build_cpus_aml() and build_ged_aml().
> 
> Regards
> Bibo Mao
> 
> On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:
> > From: Salil Mehta <[e-mail]>
> > 
> > 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 method \\_SB.CPUS.CSCN (via \\_SB.GED.CSCN)
> > 
> > Architecture specific code [1] might initialize its CPUs AML code by calling
> > common function build_cpus_aml() like below for ARM:
> > 
> > build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry, memmap[VIRT_CPUHP_ACPI].base,
> >                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);

it should be \\_SB.CPUS.CSCN

> > 
> > [1] https://lore.kernel.org/qemu-devel/[e-mail]/
> > 
> > Co-developed-by: Keqian Zhu <[e-mail]>
> > Signed-off-by: Keqian Zhu <[e-mail]>
> > Signed-off-by: Salil Mehta <[e-mail]>
> > Reviewed-by: Jonathan Cameron <[e-mail]>
> > Reviewed-by: Gavin Shan <[e-mail]>
> > Tested-by: Vishnu Pajjuri <[e-mail]>
> > Tested-by: Xianglai Li <[e-mail]>
> > Tested-by: Miguel Luis <[e-mail]>
> > Reviewed-by: Shaoqin Huang <[e-mail]>
> > Tested-by: Zhao Liu <[e-mail]>
> > Reviewed-by: Igor Mammedov <[e-mail]>
> > Message-Id: <[e-mail]>
> > Reviewed-by: Michael S. Tsirkin <[e-mail]>
> > Signed-off-by: Michael S. Tsirkin <[e-mail]>
> > ---
> >   include/hw/acpi/generic_event_device.h | 1 +
> >   hw/acpi/generic_event_device.c         | 3 +++
> >   2 files changed, 4 insertions(+)
> > 
> > diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
> > index e091ac2108..40af3550b5 100644
> > --- a/include/hw/acpi/generic_event_device.h
> > +++ b/include/hw/acpi/generic_event_device.h
> > @@ -87,6 +87,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
> >   #define GED_DEVICE      "GED"
> >   #define AML_GED_EVT_REG "EREG"
> >   #define AML_GED_EVT_SEL "ESEL"
> > +#define AML_GED_EVT_CPU_SCAN_METHOD "\\_SB.GED.CSCN"
> >   
> >   /*
> >    * Platforms need to specify the GED event bitmap
> > diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
> > index 4641933a0f..15b4c3ebbf 100644
> > --- a/hw/acpi/generic_event_device.c
> > +++ b/hw/acpi/generic_event_device.c
> > @@ -108,6 +108,9 @@ 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(AML_GED_EVT_CPU_SCAN_METHOD));
> > +                break;
> >               case ACPI_GED_PWR_DOWN_EVT:
> >                   aml_append(if_ctx,
> >                              aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
> >   
>

Salil Mehta Oct. 14, 2024, 7:59 p.m. UTC | #3

Hi Bibo,

>  From: maobibo <[e-mail]>
>  Sent: Monday, October 14, 2024 9:53 AM
>  To: [e-mail]; Salil Mehta <[e-mail]>
>  Cc: Michael S. Tsirkin <[e-mail]>; Peter Maydell
>  <[e-mail]>; Salil Mehta <[e-mail]>;
>  zhukeqian <[e-mail]>; Jonathan Cameron
>  <[e-mail]>; Gavin Shan <[e-mail]>;
>  Vishnu Pajjuri <[e-mail]>; Xianglai Li
>  <[e-mail]>; Miguel Luis <[e-mail]>; Shaoqin
>  Huang <[e-mail]>; Zhao Liu <[e-mail]>; Igor
>  Mammedov <[e-mail]>; Ani Sinha <[e-mail]>
>  Subject: Re: [PULL v2 40/61] hw/acpi: Update GED _EVT method AML with
>  CPU scan
>  
>  Hi Salil,
>  
>  When I debug cpu hotplug on LoongArch system, It reports error like this:
>       ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN],
>  AE_NOT_FOUND
>       ACPI Error: Aborting method \_SB.GED._EVT due to previous error
>  (AE_NOT_FOUND)
>       acpi-ged ACPI0013:00: IRQ method execution failed
>  
>  
>  With this patch, GED CPU call method is "\\_SB.GED.CSCN", however in
>  function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".
>       method = aml_method(event_handler_method, 0,
>  AML_NOTSERIALIZED);
>       aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
>       aml_append(table, method);
>  
>  It seems that CPU scanning method name is not consistent between
>  function build_cpus_aml() and build_ged_aml().


I believe your question stems from the following patch I've sent recently:

https://lore.kernel.org/qemu-devel/[e-mail]/

I’ve already proposed a fix for this issue. Does that not work for you?

Thanks
Salil.


>  
>  Regards
>  Bibo Mao
>  
>  On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:
>  > From: Salil Mehta <[e-mail]>
>  >
>  > 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 method \\_SB.CPUS.CSCN (via
>  > \\_SB.GED.CSCN)
>  >
>  > Architecture specific code [1] might initialize its CPUs AML code by
>  > calling common function build_cpus_aml() like below for ARM:
>  >
>  > build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry,
>  memmap[VIRT_CPUHP_ACPI].base,
>  >                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);
>  >
>  > [1]
>  > https://lore.kernel.org/qemu-devel/20240613233639.202896-13-salil.meht
>  > [e-mail]/
>  >
>  > Co-developed-by: Keqian Zhu <[e-mail]>
>  > Signed-off-by: Keqian Zhu <[e-mail]>
>  > Signed-off-by: Salil Mehta <[e-mail]>
>  > Reviewed-by: Jonathan Cameron <[e-mail]>
>  > Reviewed-by: Gavin Shan <[e-mail]>
>  > Tested-by: Vishnu Pajjuri <[e-mail]>
>  > Tested-by: Xianglai Li <[e-mail]>
>  > Tested-by: Miguel Luis <[e-mail]>
>  > Reviewed-by: Shaoqin Huang <[e-mail]>
>  > Tested-by: Zhao Liu <[e-mail]>
>  > Reviewed-by: Igor Mammedov <[e-mail]>
>  > Message-Id: <[e-mail]>
>  > Reviewed-by: Michael S. Tsirkin <[e-mail]>
>  > Signed-off-by: Michael S. Tsirkin <[e-mail]>
>  > ---
>  >   include/hw/acpi/generic_event_device.h | 1 +
>  >   hw/acpi/generic_event_device.c         | 3 +++
>  >   2 files changed, 4 insertions(+)
>  >
>  > diff --git a/include/hw/acpi/generic_event_device.h
>  > b/include/hw/acpi/generic_event_device.h
>  > index e091ac2108..40af3550b5 100644
>  > --- a/include/hw/acpi/generic_event_device.h
>  > +++ b/include/hw/acpi/generic_event_device.h
>  > @@ -87,6 +87,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState,
>  ACPI_GED)
>  >   #define GED_DEVICE      "GED"
>  >   #define AML_GED_EVT_REG "EREG"
>  >   #define AML_GED_EVT_SEL "ESEL"
>  > +#define AML_GED_EVT_CPU_SCAN_METHOD "\\_SB.GED.CSCN"
>  >
>  >   /*
>  >    * Platforms need to specify the GED event bitmap diff --git
>  > a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
>  > index 4641933a0f..15b4c3ebbf 100644
>  > --- a/hw/acpi/generic_event_device.c
>  > +++ b/hw/acpi/generic_event_device.c
>  > @@ -108,6 +108,9 @@ 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(AML_GED_EVT_CPU_SCAN_METHOD));
>  > +                break;
>  >               case ACPI_GED_PWR_DOWN_EVT:
>  >                   aml_append(if_ctx,
>  >
>  > aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
>  >

Salil Mehta Oct. 14, 2024, 8:05 p.m. UTC | #4

Hi Igor,

>  From: qemu-devel-bounces+salil.mehta=[e-mail] <qemu-
>  devel-bounces+salil.mehta=[e-mail]> On Behalf Of Igor
>  Mammedov
>  Sent: Monday, October 14, 2024 10:38 AM
>  
>  On Mon, 14 Oct 2024 16:52:55 +0800
>  maobibo <[e-mail]> wrote:
>  
>  > Hi Salil,
>  >
>  > When I debug cpu hotplug on LoongArch system, It reports error like this:
>  >      ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN],
>  > AE_NOT_FOUND
>  >      ACPI Error: Aborting method \_SB.GED._EVT due to previous error
>  > (AE_NOT_FOUND)
>  >      acpi-ged ACPI0013:00: IRQ method execution failed
>  >
>  >
>  > With this patch, GED CPU call method is "\\_SB.GED.CSCN", however in
>  > function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".
>  >      method = aml_method(event_handler_method, 0,
>  AML_NOTSERIALIZED);
>  >      aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
>  >      aml_append(table, method);
>  >
>  > It seems that CPU scanning method name is not consistent between
>  > function build_cpus_aml() and build_ged_aml().
>  >
>  > Regards
>  > Bibo Mao
>  >
>  > On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:
>  > > From: Salil Mehta <[e-mail]>
>  > >
>  > > 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 method \\_SB.CPUS.CSCN (via
>  > > \\_SB.GED.CSCN)
>  > >
>  > > Architecture specific code [1] might initialize its CPUs AML code by
>  > > calling common function build_cpus_aml() like below for ARM:
>  > >
>  > > build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry,
>  memmap[VIRT_CPUHP_ACPI].base,
>  > >                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);
>  
>  it should be \\_SB.CPUS.CSCN


I guess we are getting back to where we started then?

https://lore.kernel.org/qemu-devel/[e-mail]/


Excerpt from above discussion and your suggestion:
[...]

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

[...]


Solution:
I've avoided above error in different way and keeping exactly what you
suggested \_SB.PATH_TO_GED_DEVICE.CSCN i.e. \_SB.GED.CSCN
Please have a look:

https://lore.kernel.org/qemu-devel/[e-mail]/

Many thanks!


Best regards
Salil.

Bibo Mao Oct. 15, 2024, 1:20 a.m. UTC | #5

Hi Salil,

On 2024/10/15 上午3:59, Salil Mehta wrote:
> Hi Bibo,
> 
>>   From: maobibo <[e-mail]>
>>   Sent: Monday, October 14, 2024 9:53 AM
>>   To: [e-mail]; Salil Mehta <[e-mail]>
>>   Cc: Michael S. Tsirkin <[e-mail]>; Peter Maydell
>>   <[e-mail]>; Salil Mehta <[e-mail]>;
>>   zhukeqian <[e-mail]>; Jonathan Cameron
>>   <[e-mail]>; Gavin Shan <[e-mail]>;
>>   Vishnu Pajjuri <[e-mail]>; Xianglai Li
>>   <[e-mail]>; Miguel Luis <[e-mail]>; Shaoqin
>>   Huang <[e-mail]>; Zhao Liu <[e-mail]>; Igor
>>   Mammedov <[e-mail]>; Ani Sinha <[e-mail]>
>>   Subject: Re: [PULL v2 40/61] hw/acpi: Update GED _EVT method AML with
>>   CPU scan
>>   
>>   Hi Salil,
>>   
>>   When I debug cpu hotplug on LoongArch system, It reports error like this:
>>        ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN],
>>   AE_NOT_FOUND
>>        ACPI Error: Aborting method \_SB.GED._EVT due to previous error
>>   (AE_NOT_FOUND)
>>        acpi-ged ACPI0013:00: IRQ method execution failed
>>   
>>   
>>   With this patch, GED CPU call method is "\\_SB.GED.CSCN", however in
>>   function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".
>>        method = aml_method(event_handler_method, 0,
>>   AML_NOTSERIALIZED);
>>        aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
>>        aml_append(table, method);
>>   
>>   It seems that CPU scanning method name is not consistent between
>>   function build_cpus_aml() and build_ged_aml().
> 
> 
> I believe your question stems from the following patch I've sent recently:
> 
> https://lore.kernel.org/qemu-devel/[e-mail]/
> 
> I’ve already proposed a fix for this issue. Does that not work for you?
yes, it works for me if AML_GED_EVT_CPU_SCAN_METHOD is used as parameter 
in function build_cpus_aml().

Sorry for the noise.

Regards
Bibo Mao
> 
> Thanks
> Salil.
> 
> 
>>   
>>   Regards
>>   Bibo Mao
>>   
>>   On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:
>>   > From: Salil Mehta <[e-mail]>
>>   >
>>   > 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 method \\_SB.CPUS.CSCN (via
>>   > \\_SB.GED.CSCN)
>>   >
>>   > Architecture specific code [1] might initialize its CPUs AML code by
>>   > calling common function build_cpus_aml() like below for ARM:
>>   >
>>   > build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry,
>>   memmap[VIRT_CPUHP_ACPI].base,
>>   >                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);
>>   >
>>   > [1]
>>   > https://lore.kernel.org/qemu-devel/20240613233639.202896-13-salil.meht
>>   > [e-mail]/
>>   >
>>   > Co-developed-by: Keqian Zhu <[e-mail]>
>>   > Signed-off-by: Keqian Zhu <[e-mail]>
>>   > Signed-off-by: Salil Mehta <[e-mail]>
>>   > Reviewed-by: Jonathan Cameron <[e-mail]>
>>   > Reviewed-by: Gavin Shan <[e-mail]>
>>   > Tested-by: Vishnu Pajjuri <[e-mail]>
>>   > Tested-by: Xianglai Li <[e-mail]>
>>   > Tested-by: Miguel Luis <[e-mail]>
>>   > Reviewed-by: Shaoqin Huang <[e-mail]>
>>   > Tested-by: Zhao Liu <[e-mail]>
>>   > Reviewed-by: Igor Mammedov <[e-mail]>
>>   > Message-Id: <[e-mail]>
>>   > Reviewed-by: Michael S. Tsirkin <[e-mail]>
>>   > Signed-off-by: Michael S. Tsirkin <[e-mail]>
>>   > ---
>>   >   include/hw/acpi/generic_event_device.h | 1 +
>>   >   hw/acpi/generic_event_device.c         | 3 +++
>>   >   2 files changed, 4 insertions(+)
>>   >
>>   > diff --git a/include/hw/acpi/generic_event_device.h
>>   > b/include/hw/acpi/generic_event_device.h
>>   > index e091ac2108..40af3550b5 100644
>>   > --- a/include/hw/acpi/generic_event_device.h
>>   > +++ b/include/hw/acpi/generic_event_device.h
>>   > @@ -87,6 +87,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState,
>>   ACPI_GED)
>>   >   #define GED_DEVICE      "GED"
>>   >   #define AML_GED_EVT_REG "EREG"
>>   >   #define AML_GED_EVT_SEL "ESEL"
>>   > +#define AML_GED_EVT_CPU_SCAN_METHOD "\\_SB.GED.CSCN"
>>   >
>>   >   /*
>>   >    * Platforms need to specify the GED event bitmap diff --git
>>   > a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
>>   > index 4641933a0f..15b4c3ebbf 100644
>>   > --- a/hw/acpi/generic_event_device.c
>>   > +++ b/hw/acpi/generic_event_device.c
>>   > @@ -108,6 +108,9 @@ 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(AML_GED_EVT_CPU_SCAN_METHOD));
>>   > +                break;
>>   >               case ACPI_GED_PWR_DOWN_EVT:
>>   >                   aml_append(if_ctx,
>>   >
>>   > aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
>>   >
>

Igor Mammedov Oct. 15, 2024, 9:31 a.m. UTC | #6

On Mon, 14 Oct 2024 20:05:58 +0000
Salil Mehta <[e-mail]> wrote:

> Hi Igor,
> 
> >  From: qemu-devel-bounces+salil.mehta=[e-mail] <qemu-
> >  devel-bounces+salil.mehta=[e-mail]> On Behalf Of Igor
> >  Mammedov
> >  Sent: Monday, October 14, 2024 10:38 AM
> >  
> >  On Mon, 14 Oct 2024 16:52:55 +0800
> >  maobibo <[e-mail]> wrote:
> >    
> >  > Hi Salil,
> >  >
> >  > When I debug cpu hotplug on LoongArch system, It reports error like this:
> >  >      ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN],
> >  > AE_NOT_FOUND
> >  >      ACPI Error: Aborting method \_SB.GED._EVT due to previous error
> >  > (AE_NOT_FOUND)
> >  >      acpi-ged ACPI0013:00: IRQ method execution failed
> >  >
> >  >
> >  > With this patch, GED CPU call method is "\\_SB.GED.CSCN", however in
> >  > function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".
> >  >      method = aml_method(event_handler_method, 0,  
> >  AML_NOTSERIALIZED);  
> >  >      aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
> >  >      aml_append(table, method);
> >  >
> >  > It seems that CPU scanning method name is not consistent between
> >  > function build_cpus_aml() and build_ged_aml().
> >  >
> >  > Regards
> >  > Bibo Mao
> >  >
> >  > On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:  
> >  > > From: Salil Mehta <[e-mail]>
> >  > >
> >  > > 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 method \\_SB.CPUS.CSCN (via
> >  > > \\_SB.GED.CSCN)
> >  > >
> >  > > Architecture specific code [1] might initialize its CPUs AML code by
> >  > > calling common function build_cpus_aml() like below for ARM:
> >  > >
> >  > > build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry,  
> >  memmap[VIRT_CPUHP_ACPI].base,  
> >  > >                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);  
> >  
> >  it should be \\_SB.CPUS.CSCN  
> 
> 
> I guess we are getting back to where we started then?
> 
> https://lore.kernel.org/qemu-devel/[e-mail]/
> 

Indeed, CSCN in name had me confused,
perhaps it would be better to rename that something else.
maybe something like s/_SB.GED.CSCN/_SB.GED.ECPU/

> Excerpt from above discussion and your suggestion:
> [...]
> 
> 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
> 
> [...]
> 
> 
> Solution:
> I've avoided above error in different way and keeping exactly what you
> suggested \_SB.PATH_TO_GED_DEVICE.CSCN i.e. \_SB.GED.CSCN
> Please have a look:
> 
> https://lore.kernel.org/qemu-devel/[e-mail]/
> 
> Many thanks!
> 
> 
> Best regards
> Salil.
>

Salil Mehta Oct. 15, 2024, 9:34 a.m. UTC | #7

Hi Bibo,

>  From: maobibo <[e-mail]>
>  Sent: Tuesday, October 15, 2024 2:20 AM
>  To: Salil Mehta <[e-mail]>; [e-mail]
>  
>  Hi Salil,
>  
>  On 2024/10/15 上午3:59, Salil Mehta wrote:
>  > Hi Bibo,
>  >
>  >>   From: maobibo <[e-mail]>
>  >>   Sent: Monday, October 14, 2024 9:53 AM
>  >>   To: [e-mail]; Salil Mehta <[e-mail]>
>  >>   Cc: Michael S. Tsirkin <[e-mail]>; Peter Maydell
>  >>   <[e-mail]>; Salil Mehta <[e-mail]>;
>  >>   zhukeqian <[e-mail]>; Jonathan Cameron
>  >>   <[e-mail]>; Gavin Shan <[e-mail]>;
>  >>   Vishnu Pajjuri <[e-mail]>; Xianglai Li
>  >>   <[e-mail]>; Miguel Luis <[e-mail]>;
>  Shaoqin
>  >>   Huang <[e-mail]>; Zhao Liu <[e-mail]>; Igor
>  >>   Mammedov <[e-mail]>; Ani Sinha
>  <[e-mail]>
>  >>   Subject: Re: [PULL v2 40/61] hw/acpi: Update GED _EVT method AML
>  with
>  >>   CPU scan
>  >>
>  >>   Hi Salil,
>  >>
>  >>   When I debug cpu hotplug on LoongArch system, It reports error like
>  this:
>  >>        ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN],
>  >>   AE_NOT_FOUND
>  >>        ACPI Error: Aborting method \_SB.GED._EVT due to previous error
>  >>   (AE_NOT_FOUND)
>  >>        acpi-ged ACPI0013:00: IRQ method execution failed
>  >>
>  >>
>  >>   With this patch, GED CPU call method is "\\_SB.GED.CSCN", however in
>  >>   function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".
>  >>        method = aml_method(event_handler_method, 0,
>  >>   AML_NOTSERIALIZED);
>  >>        aml_append(method, aml_call0("\\_SB.CPUS."
>  CPU_SCAN_METHOD));
>  >>        aml_append(table, method);
>  >>
>  >>   It seems that CPU scanning method name is not consistent between
>  >>   function build_cpus_aml() and build_ged_aml().
>  >
>  >
>  > I believe your question stems from the following patch I've sent recently:
>  >
>  > https://lore.kernel.org/qemu-devel/20241009031815.250096-16-salil.meht
>  > [e-mail]/
>  >
>  > I’ve already proposed a fix for this issue. Does that not work for you?
>  yes, it works for me if AML_GED_EVT_CPU_SCAN_METHOD is used as
>  parameter in function build_cpus_aml().
>  
>  Sorry for the noise.


No issues. Good that it got sorted 

Salil Mehta Oct. 15, 2024, 9:41 a.m. UTC | #8

HI Igor,

>  From: Igor Mammedov <[e-mail]>
>  Sent: Tuesday, October 15, 2024 10:31 AM
>  To: Salil Mehta <[e-mail]>
>  Cc: maobibo <[e-mail]>; [e-mail]; Michael
>  S. Tsirkin <[e-mail]>; Peter Maydell <[e-mail]>;
>  zhukeqian <[e-mail]>; Jonathan Cameron
>  <[e-mail]>; Gavin Shan <[e-mail]>;
>  Vishnu Pajjuri <[e-mail]>; Xianglai Li
>  <[e-mail]>; Miguel Luis <[e-mail]>; Shaoqin
>  Huang <[e-mail]>; Zhao Liu <[e-mail]>; Ani Sinha
>  <[e-mail]>
>  Subject: Re: [PULL v2 40/61] hw/acpi: Update GED _EVT method AML with
>  CPU scan
>  
>  On Mon, 14 Oct 2024 20:05:58 +0000
>  Salil Mehta <[e-mail]> wrote:
>  
>  > Hi Igor,
>  >
>  > >  From: qemu-devel-bounces+salil.mehta=[e-mail]
>  <qemu-
>  > > devel-bounces+salil.mehta=[e-mail]> On Behalf Of
>  Igor
>  > > Mammedov
>  > >  Sent: Monday, October 14, 2024 10:38 AM
>  > >
>  > >  On Mon, 14 Oct 2024 16:52:55 +0800
>  > >  maobibo <[e-mail]> wrote:
>  > >
>  > >  > Hi Salil,
>  > >  >
>  > >  > When I debug cpu hotplug on LoongArch system, It reports error like
>  this:
>  > >  >      ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN],
>  > >  > AE_NOT_FOUND
>  > >  >      ACPI Error: Aborting method \_SB.GED._EVT due to previous error
>  > >  > (AE_NOT_FOUND)
>  > >  >      acpi-ged ACPI0013:00: IRQ method execution failed
>  > >  >
>  > >  >
>  > >  > With this patch, GED CPU call method is "\\_SB.GED.CSCN", however
>  > > in  > function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".
>  > >  >      method = aml_method(event_handler_method, 0,
>  > >  AML_NOTSERIALIZED);
>  > >  >      aml_append(method, aml_call0("\\_SB.CPUS."
>  CPU_SCAN_METHOD));
>  > >  >      aml_append(table, method);
>  > >  >
>  > >  > It seems that CPU scanning method name is not consistent between
>  > > > function build_cpus_aml() and build_ged_aml().
>  > >  >
>  > >  > Regards
>  > >  > Bibo Mao
>  > >  >
>  > >  > On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:
>  > >  > > From: Salil Mehta <[e-mail]>  > >  > > 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
>  > > method \\_SB.CPUS.CSCN (via  > > \\_SB.GED.CSCN)  > >  > >
>  > > Architecture specific code [1] might initialize its CPUs AML code by
>  > > > > calling common function build_cpus_aml() like below for ARM:
>  > >  > >
>  > >  > > build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry,
>  > > memmap[VIRT_CPUHP_ACPI].base,
>  > >  > >                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);
>  > >
>  > >  it should be \\_SB.CPUS.CSCN
>  >
>  >
>  > I guess we are getting back to where we started then?
>  >
>  > https://lore.kernel.org/qemu-
>  devel/[e-mail]
>  > ers.ipa.redhat.com/
>  >
>  
>  Indeed, CSCN in name had me confused,
>  perhaps it would be better to rename that something else.
>  maybe something like s/_SB.GED.CSCN/_SB.GED.ECPU/


Sure, we can definitely improve. But we are indeed triggering CPU Scan
here so I don’t understand how will `ECPU` be more intuitive than
`CSCN`.  what about below?

s/_SB.GED.CSCN/_SB.GED.CPUSCAN/


Thanks
Salil.

>  
>  > Excerpt from above discussion and your suggestion:
>  > [...]
>  >
>  > 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
>  >
>  > [...]
>  >
>  >
>  > Solution:
>  > I've avoided above error in different way and keeping exactly what you
>  > suggested \_SB.PATH_TO_GED_DEVICE.CSCN i.e. \_SB.GED.CSCN Please
>  have
>  > a look:
>  >
>  > https://lore.kernel.org/qemu-devel/20241009031815.250096-16-salil.meht
>  > [e-mail]/
>  >
>  > Many thanks!
>  >
>  >
>  > Best regards
>  > Salil.
>  >
>

Igor Mammedov Oct. 15, 2024, 2:34 p.m. UTC | #9

On Tue, 15 Oct 2024 09:41:24 +0000
Salil Mehta <[e-mail]> wrote:

> HI Igor,
> 
> >  From: Igor Mammedov <[e-mail]>
> >  Sent: Tuesday, October 15, 2024 10:31 AM
> >  To: Salil Mehta <[e-mail]>
> >  Cc: maobibo <[e-mail]>; [e-mail]; Michael
> >  S. Tsirkin <[e-mail]>; Peter Maydell <[e-mail]>;
> >  zhukeqian <[e-mail]>; Jonathan Cameron
> >  <[e-mail]>; Gavin Shan <[e-mail]>;
> >  Vishnu Pajjuri <[e-mail]>; Xianglai Li
> >  <[e-mail]>; Miguel Luis <[e-mail]>; Shaoqin
> >  Huang <[e-mail]>; Zhao Liu <[e-mail]>; Ani Sinha
> >  <[e-mail]>
> >  Subject: Re: [PULL v2 40/61] hw/acpi: Update GED _EVT method AML with
> >  CPU scan
> >  
> >  On Mon, 14 Oct 2024 20:05:58 +0000
> >  Salil Mehta <[e-mail]> wrote:
> >    
> >  > Hi Igor,
> >  >  
> >  > >  From: qemu-devel-bounces+salil.mehta=[e-mail]  
> >  <qemu-  
> >  > > devel-bounces+salil.mehta=[e-mail]> On Behalf Of  
> >  Igor  
> >  > > Mammedov
> >  > >  Sent: Monday, October 14, 2024 10:38 AM
> >  > >
> >  > >  On Mon, 14 Oct 2024 16:52:55 +0800
> >  > >  maobibo <[e-mail]> wrote:
> >  > >  
> >  > >  > Hi Salil,
> >  > >  >
> >  > >  > When I debug cpu hotplug on LoongArch system, It reports error like  
> >  this:  
> >  > >  >      ACPI BIOS Error (bug): Could not resolve symbol [\_SB.GED.CSCN],
> >  > >  > AE_NOT_FOUND
> >  > >  >      ACPI Error: Aborting method \_SB.GED._EVT due to previous error
> >  > >  > (AE_NOT_FOUND)
> >  > >  >      acpi-ged ACPI0013:00: IRQ method execution failed
> >  > >  >
> >  > >  >
> >  > >  > With this patch, GED CPU call method is "\\_SB.GED.CSCN", however  
> >  > > in  > function build_cpus_aml(), its method name is "\\_SB.CPUS.CSCN".  
> >  > >  >      method = aml_method(event_handler_method, 0,  
> >  > >  AML_NOTSERIALIZED);  
> >  > >  >      aml_append(method, aml_call0("\\_SB.CPUS."  
> >  CPU_SCAN_METHOD));  
> >  > >  >      aml_append(table, method);
> >  > >  >
> >  > >  > It seems that CPU scanning method name is not consistent between
> >  > > > function build_cpus_aml() and build_ged_aml().
> >  > >  >
> >  > >  > Regards
> >  > >  > Bibo Mao
> >  > >  >
> >  > >  > On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:  
> >  > >  > > From: Salil Mehta <[e-mail]>  > >  > > 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
> >  > > method \\_SB.CPUS.CSCN (via  > > \\_SB.GED.CSCN)  > >  > >
> >  > > Architecture specific code [1] might initialize its CPUs AML code by  
> >  > > > > calling common function build_cpus_aml() like below for ARM:
> >  > >  > >
> >  > >  > > build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry,  
> >  > > memmap[VIRT_CPUHP_ACPI].base,  
> >  > >  > >                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);  
> >  > >
> >  > >  it should be \\_SB.CPUS.CSCN  
> >  >
> >  >
> >  > I guess we are getting back to where we started then?
> >  >
> >  > https://lore.kernel.org/qemu-  
> >  devel/[e-mail]  
> >  > ers.ipa.redhat.com/
> >  >  
> >  
> >  Indeed, CSCN in name had me confused,
> >  perhaps it would be better to rename that something else.
> >  maybe something like s/_SB.GED.CSCN/_SB.GED.ECPU/  
> 
> 
> Sure, we can definitely improve. But we are indeed triggering CPU Scan
> here so I don’t understand how will `ECPU` be more intuitive than
> `CSCN`.  what about below?
> 
> s/_SB.GED.CSCN/_SB.GED.CPUSCAN/

ACPI name segment is limited to 4 characters only.

ECPU - Event handler for CPU
it could be something else though

the point is not confuse it with CSCN (apparently different namespace but
still it could lead to confusion as above shows )

> 
> 
> Thanks
> Salil.
> 
> >    
> >  > Excerpt from above discussion and your suggestion:
> >  > [...]
> >  >
> >  > 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
> >  >
> >  > [...]
> >  >
> >  >
> >  > Solution:
> >  > I've avoided above error in different way and keeping exactly what you
> >  > suggested \_SB.PATH_TO_GED_DEVICE.CSCN i.e. \_SB.GED.CSCN Please  
> >  have  
> >  > a look:
> >  >
> >  > https://lore.kernel.org/qemu-devel/20241009031815.250096-16-salil.meht
> >  > [e-mail]/
> >  >
> >  > Many thanks!
> >  >
> >  >
> >  > Best regards
> >  > Salil.
> >  >  
> >    
>

Salil Mehta Oct. 15, 2024, 2:42 p.m. UTC | #10

Hi Igor,

>  From: Igor Mammedov <[e-mail]>
>  Sent: Tuesday, October 15, 2024 3:34 PM
>  To: Salil Mehta <[e-mail]>
>  
>  On Tue, 15 Oct 2024 09:41:24 +0000
>  Salil Mehta <[e-mail]> wrote:
>  
>  > HI Igor,
>  >
>  > >  From: Igor Mammedov <[e-mail]>
>  > >  Sent: Tuesday, October 15, 2024 10:31 AM
>  > >  To: Salil Mehta <[e-mail]>
>  > >  Cc: maobibo <[e-mail]>; [e-mail];
>  Michael
>  > > S. Tsirkin <[e-mail]>; Peter Maydell
>  > > <[e-mail]>;  zhukeqian <[e-mail]>;
>  > > Jonathan Cameron  <[e-mail]>; Gavin Shan
>  > > <[e-mail]>;  Vishnu Pajjuri
>  <[e-mail]>;
>  > > Xianglai Li  <[e-mail]>; Miguel Luis
>  > > <[e-mail]>; Shaoqin  Huang <[e-mail]>;
>  Zhao
>  > > Liu <[e-mail]>; Ani Sinha  <[e-mail]>
>  > >  Subject: Re: [PULL v2 40/61] hw/acpi: Update GED _EVT method AML
>  > > with  CPU scan
>  > >
>  > >  On Mon, 14 Oct 2024 20:05:58 +0000
>  > >  Salil Mehta <[e-mail]> wrote:
>  > >
>  > >  > Hi Igor,
>  > >  >
>  > >  > >  From: qemu-devel-bounces+salil.mehta=[e-mail]
>  > >  <qemu-
>  > >  > > devel-bounces+salil.mehta=[e-mail]> On Behalf
>  Of
>  > > Igor  > > Mammedov  > >  Sent: Monday, October 14, 2024 10:38 AM  >
>  > > >  > >  On Mon, 14 Oct 2024 16:52:55 +0800  > >  maobibo
>  > > <[e-mail]> wrote:
>  > >  > >
>  > >  > >  > Hi Salil,
>  > >  > >  >
>  > >  > >  > When I debug cpu hotplug on LoongArch system, It reports
>  > > error like
>  > >  this:
>  > >  > >  >      ACPI BIOS Error (bug): Could not resolve symbol
>  [\_SB.GED.CSCN],
>  > >  > >  > AE_NOT_FOUND
>  > >  > >  >      ACPI Error: Aborting method \_SB.GED._EVT due to previous
>  error
>  > >  > >  > (AE_NOT_FOUND)
>  > >  > >  >      acpi-ged ACPI0013:00: IRQ method execution failed
>  > >  > >  >
>  > >  > >  >
>  > >  > >  > With this patch, GED CPU call method is "\\_SB.GED.CSCN",
>  > > however  > > in  > function build_cpus_aml(), its method name is
>  "\\_SB.CPUS.CSCN".
>  > >  > >  >      method = aml_method(event_handler_method, 0,
>  > >  > >  AML_NOTSERIALIZED);
>  > >  > >  >      aml_append(method, aml_call0("\\_SB.CPUS."
>  > >  CPU_SCAN_METHOD));
>  > >  > >  >      aml_append(table, method);
>  > >  > >  >
>  > >  > >  > It seems that CPU scanning method name is not consistent
>  > > between  > > > function build_cpus_aml() and build_ged_aml().
>  > >  > >  >
>  > >  > >  > Regards
>  > >  > >  > Bibo Mao
>  > >  > >  >
>  > >  > >  > On 2024/7/23 下午6:59, Michael S. Tsirkin wrote:
>  > >  > >  > > From: Salil Mehta <[e-mail]>  > >  > > 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  > > method \\_SB.CPUS.CSCN (via  > >
>  > > \\_SB.GED.CSCN)  > >  > >  > > Architecture specific code [1] might
>  > > initialize its CPUs AML code by  > > > > calling common function
>  build_cpus_aml() like below for ARM:
>  > >  > >  > >
>  > >  > >  > > build_cpus_aml(scope, ms, opts, xx_madt_cpu_entry,  > >
>  > > memmap[VIRT_CPUHP_ACPI].base,
>  > >  > >  > >                 "\\_SB", "\\_SB.GED.CSCN", AML_SYSTEM_MEMORY);
>  > >  > >
>  > >  > >  it should be \\_SB.CPUS.CSCN  >  >  > I guess we are getting
>  > > back to where we started then?
>  > >  >
>  > >  > https://lore.kernel.org/qemu-
>  > >  devel/[e-mail]
>  > >  > ers.ipa.redhat.com/
>  > >  >
>  > >
>  > >  Indeed, CSCN in name had me confused,  perhaps it would be better
>  > > to rename that something else.
>  > >  maybe something like s/_SB.GED.CSCN/_SB.GED.ECPU/
>  >
>  >
>  > Sure, we can definitely improve. But we are indeed triggering CPU Scan
>  > here so I don’t understand how will `ECPU` be more intuitive than
>  > `CSCN`.  what about below?
>  >
>  > s/_SB.GED.CSCN/_SB.GED.CPUSCAN/
>  
>  ACPI name segment is limited to 4 characters only.

I see.


>  
>  ECPU - Event handler for CPU
>  it could be something else though
>  
>  the point is not confuse it with CSCN (apparently different namespace but
>  still it could lead to confusion as above shows )


No problem. I'll send a patch today.

Thanks!


>  
>  >
>  >
>  > Thanks
>  > Salil.
>  >
>  > >
>  > >  > Excerpt from above discussion and your suggestion:
>  > >  > [...]
>  > >  >
>  > >  > 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
>  > > >  > [...]  >  >  > Solution:
>  > >  > I've avoided above error in different way and keeping exactly
>  > > what you  > suggested \_SB.PATH_TO_GED_DEVICE.CSCN i.e.
>  > > \_SB.GED.CSCN Please  have  > a look:
>  > >  >
>  > >  >
>  > > https://lore.kernel.org/qemu-devel/20241009031815.250096-16-salil.me
>  > > ht
>  > >  > [e-mail]/
>  > >  >
>  > >  > Many thanks!
>  > >  >
>  > >  >
>  > >  > Best regards
>  > >  > Salil.
>  > >  >
>  > >
>  >
>

Patch

diff --git a/include/hw/acpi/generic_event_device.h b/include/hw/acpi/generic_event_device.h
index e091ac2108..40af3550b5 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -87,6 +87,7 @@  OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
 #define GED_DEVICE      "GED"
 #define AML_GED_EVT_REG "EREG"
 #define AML_GED_EVT_SEL "ESEL"
+#define AML_GED_EVT_CPU_SCAN_METHOD "\\_SB.GED.CSCN"
 
 /*
  * Platforms need to specify the GED event bitmap
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 4641933a0f..15b4c3ebbf 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -108,6 +108,9 @@  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(AML_GED_EVT_CPU_SCAN_METHOD));
+                break;
             case ACPI_GED_PWR_DOWN_EVT:
                 aml_append(if_ctx,
                            aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),