diff mbox series

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

Message ID 549c9a9dcbc1592ea79496f7b3ab234f366adeba.1721731723.git.mst@redhat.com
State New
Headers show
Series [PULL,v2,01/61] hw/virtio/virtio-crypto: Fix op_code assignment in virtio_crypto_create_asym_session | expand

Commit Message

Michael S. Tsirkin July 23, 2024, 10:59 a.m. UTC 
From: Salil Mehta <salil.mehta@huawei.com>

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.mehta@huawei.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>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Message-Id: <20240716111502.202344-5-salil.mehta@huawei.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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 <salil.mehta@huawei.com>
> 
> 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.mehta@huawei.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>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> Message-Id: <20240716111502.202344-5-salil.mehta@huawei.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>   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 <maobibo@loongson.cn> 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 <salil.mehta@huawei.com>
> > 
> > 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/20240613233639.202896-13-salil.mehta@huawei.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>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> > Message-Id: <20240716111502.202344-5-salil.mehta@huawei.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >   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),
> >   
>
Bibo Mao Oct. 15, 2024, 1:20 a.m. UTC | #3 
Hi Salil,

On 2024/10/15 上午3:59, Salil Mehta wrote:
> Hi Bibo,
> 
>>   From: maobibo <maobibo@loongson.cn>
>>   Sent: Monday, October 14, 2024 9:53 AM
>>   To: qemu-devel@nongnu.org; Salil Mehta <salil.mehta@huawei.com>
>>   Cc: Michael S. Tsirkin <mst@redhat.com>; Peter Maydell
>>   <peter.maydell@linaro.org>; Salil Mehta <salil.mehta@huawei.com>;
>>   zhukeqian <zhukeqian1@huawei.com>; Jonathan Cameron
>>   <jonathan.cameron@huawei.com>; Gavin Shan <gshan@redhat.com>;
>>   Vishnu Pajjuri <vishnu@os.amperecomputing.com>; Xianglai Li
>>   <lixianglai@loongson.cn>; Miguel Luis <miguel.luis@oracle.com>; Shaoqin
>>   Huang <shahuang@redhat.com>; Zhao Liu <zhao1.liu@intel.com>; Igor
>>   Mammedov <imammedo@redhat.com>; Ani Sinha <anisinha@redhat.com>
>>   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.mehta@huawei.com/
> 
> 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 <salil.mehta@huawei.com>
>>   >
>>   > 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
>>   > a@huawei.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>
>>   > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>>   > Message-Id: <20240716111502.202344-5-salil.mehta@huawei.com>
>>   > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>>   > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>   > ---
>>   >   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 | #4 
On Mon, 14 Oct 2024 20:05:58 +0000
Salil Mehta <salil.mehta@huawei.com> wrote:

> Hi Igor,
> 
> >  From: qemu-devel-bounces+salil.mehta=huawei.com@nongnu.org <qemu-
> >  devel-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of Igor
> >  Mammedov
> >  Sent: Monday, October 14, 2024 10:38 AM
> >  
> >  On Mon, 14 Oct 2024 16:52:55 +0800
> >  maobibo <maobibo@loongson.cn> 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 <salil.mehta@huawei.com>
> >  > >
> >  > > 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/20240706162845.3baf5568@imammedo.users.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/

> 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.mehta@huawei.com/
> 
> Many thanks!
> 
> 
> Best regards
> Salil.
>
Igor Mammedov Oct. 15, 2024, 2:34 p.m. UTC | #5 
On Tue, 15 Oct 2024 09:41:24 +0000
Salil Mehta <salil.mehta@huawei.com> wrote:

> HI Igor,
> 
> >  From: Igor Mammedov <imammedo@redhat.com>
> >  Sent: Tuesday, October 15, 2024 10:31 AM
> >  To: Salil Mehta <salil.mehta@huawei.com>
> >  Cc: maobibo <maobibo@loongson.cn>; qemu-devel@nongnu.org; Michael
> >  S. Tsirkin <mst@redhat.com>; Peter Maydell <peter.maydell@linaro.org>;
> >  zhukeqian <zhukeqian1@huawei.com>; Jonathan Cameron
> >  <jonathan.cameron@huawei.com>; Gavin Shan <gshan@redhat.com>;
> >  Vishnu Pajjuri <vishnu@os.amperecomputing.com>; Xianglai Li
> >  <lixianglai@loongson.cn>; Miguel Luis <miguel.luis@oracle.com>; Shaoqin
> >  Huang <shahuang@redhat.com>; Zhao Liu <zhao1.liu@intel.com>; Ani Sinha
> >  <anisinha@redhat.com>
> >  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 <salil.mehta@huawei.com> wrote:
> >    
> >  > Hi Igor,
> >  >  
> >  > >  From: qemu-devel-bounces+salil.mehta=huawei.com@nongnu.org  
> >  <qemu-  
> >  > > devel-bounces+salil.mehta=huawei.com@nongnu.org> On Behalf Of  
> >  Igor  
> >  > > Mammedov
> >  > >  Sent: Monday, October 14, 2024 10:38 AM
> >  > >
> >  > >  On Mon, 14 Oct 2024 16:52:55 +0800
> >  > >  maobibo <maobibo@loongson.cn> 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 <salil.mehta@huawei.com>  > >  > > 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/20240706162845.3baf5568@imammedo.us  
> >  > 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
> >  > a@huawei.com/
> >  >
> >  > Many thanks!
> >  >
> >  >
> >  > Best regards
> >  > Salil.
> >  >  
> >    
>
diff mbox series

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),