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

[Salil Mehta]

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.