[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan
|
From: |
Igor Mammedov |
|
Subject: |
Re: [PATCH V13 4/8] hw/acpi: Update GED _EVT method AML with CPU scan |
|
Date: |
Thu, 11 Jul 2024 10:34:03 +0200 |
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;
>