CPUs Control device(\\_SB.PCI0) register interface for the x86 arch is IO port
based and existing CPUs AML code assumes _CRS objects would evaluate to a system
resource which describes IO Port address. But on ARM arch CPUs control
device(\\_SB.PRES) register interface is memory-mapped hence _CRS object should
evaluate to system resource which describes memory-mapped base address. Update
build CPUs AML function to accept both IO/MEMORY region spaces and accordingly
update the _CRS object.
On x86, CPU Hotplug uses Generic ACPI GPE Block Bit 2 (GPE.2) event handler to
notify OSPM about any CPU hot(un)plug events. Latest CPU Hotplug is based on
ACPI Generic Event Device framework and uses ACPI GED device for the same. Not
all architectures support GPE based CPU Hotplug event handler. Hence, make AML
for GPE.2 event handler conditional.
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: Gavin Shan <gshan@redhat.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
---
hw/acpi/cpu.c | 23 ++++++++++++++++-------
hw/i386/acpi-build.c | 3 ++-
include/hw/acpi/cpu.h | 5 +++--
3 files changed, 21 insertions(+), 10 deletions(-)
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index de1f9295dc..5b0eaad1c5 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -339,9 +339,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)
+ const char *event_handler_method,
+ AmlRegionSpace rs)
{
Aml *ifctx;
Aml *field;
@@ -366,13 +367,19 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
aml_append(cpu_ctrl_dev, aml_mutex(CPU_LOCK, 0));
crs = aml_resource_template();
- aml_append(crs, aml_io(AML_DECODE16, io_base, io_base, 1,
+ if (rs == AML_SYSTEM_IO) {
+ aml_append(crs, aml_io(AML_DECODE16, base_addr, base_addr, 1,
ACPI_CPU_HOTPLUG_REG_LEN));
+ } else {
+ aml_append(crs, aml_memory32_fixed(base_addr,
+ ACPI_CPU_HOTPLUG_REG_LEN, AML_READ_WRITE));
+ }
+
aml_append(cpu_ctrl_dev, aml_name_decl("_CRS", crs));
/* declare CPU hotplug MMIO region with related access fields */
aml_append(cpu_ctrl_dev,
- aml_operation_region("PRST", AML_SYSTEM_IO, aml_int(io_base),
+ aml_operation_region("PRST", rs, aml_int(base_addr),
ACPI_CPU_HOTPLUG_REG_LEN));
field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK,
@@ -696,9 +703,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
aml_append(sb_scope, cpus_dev);
aml_append(table, sb_scope);
- method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
- aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
- aml_append(table, method);
+ if (event_handler_method) {
+ method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
+ aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
+ aml_append(table, method);
+ }
g_free(cphp_res_path);
}
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 3f2b27cf75..f9f31f9db5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1550,7 +1550,8 @@ 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", "\\_GPE._E02",
+ AML_SYSTEM_IO);
}
if (pcms->memhp_io_base && nr_mem) {
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
index bc901660fb..b521a4e0de 100644
--- a/include/hw/acpi/cpu.h
+++ b/include/hw/acpi/cpu.h
@@ -60,9 +60,10 @@ typedef void (*build_madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
GArray *entry, bool force_enabled);
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);
+ const char *event_handler_method,
+ AmlRegionSpace rs);
void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list);
--
2.34.1On Mon, 13 Nov 2023 20:12:33 +0000
Salil Mehta <salil.mehta@huawei.com> wrote:
wrt subj: 'cpu-(ctrl)dev change' doesn't make any sense to me, pls rephrase and be more specific.
> CPUs Control device(\\_SB.PCI0) register interface for the x86 arch is IO port
> based and existing CPUs AML code assumes _CRS objects would evaluate to a system
> resource which describes IO Port address. But on ARM arch CPUs control
> device(\\_SB.PRES) register interface is memory-mapped hence _CRS object should
> evaluate to system resource which describes memory-mapped base address. Update
> build CPUs AML function to accept both IO/MEMORY region spaces and accordingly
> update the _CRS object.
> On x86, CPU Hotplug uses Generic ACPI GPE Block Bit 2 (GPE.2) event handler to
> notify OSPM about any CPU hot(un)plug events. Latest CPU Hotplug is based on
^^^ latest supported ATM is x86
and not based on GED vvvv, suggest to drop sentence or actually whole paragraph.
> ACPI Generic Event Device framework and uses ACPI GED device for the same. Not
> all architectures support GPE based CPU Hotplug event handler. Hence, make AML
> for GPE.2 event handler conditional.
this is the 2nd different thing the patch does, pls split that into another,
for more on that see related hunk.
>
> 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: Gavin Shan <gshan@redhat.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Xianglai Li <lixianglai@loongson.cn>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> ---
> hw/acpi/cpu.c | 23 ++++++++++++++++-------
> hw/i386/acpi-build.c | 3 ++-
> include/hw/acpi/cpu.h | 5 +++--
> 3 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index de1f9295dc..5b0eaad1c5 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -339,9 +339,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)
> + const char *event_handler_method,
> + AmlRegionSpace rs)
> {
> Aml *ifctx;
> Aml *field;
> @@ -366,13 +367,19 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> aml_append(cpu_ctrl_dev, aml_mutex(CPU_LOCK, 0));
>
> crs = aml_resource_template();
> - aml_append(crs, aml_io(AML_DECODE16, io_base, io_base, 1,
> + if (rs == AML_SYSTEM_IO) {
> + aml_append(crs, aml_io(AML_DECODE16, base_addr, base_addr, 1,
> ACPI_CPU_HOTPLUG_REG_LEN));
> + } else {
> + aml_append(crs, aml_memory32_fixed(base_addr,
> + ACPI_CPU_HOTPLUG_REG_LEN, AML_READ_WRITE));
> + }
> +
> aml_append(cpu_ctrl_dev, aml_name_decl("_CRS", crs));
>
> /* declare CPU hotplug MMIO region with related access fields */
> aml_append(cpu_ctrl_dev,
> - aml_operation_region("PRST", AML_SYSTEM_IO, aml_int(io_base),
> + aml_operation_region("PRST", rs, aml_int(base_addr),
> ACPI_CPU_HOTPLUG_REG_LEN));
>
> field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK,
> @@ -696,9 +703,11 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
> aml_append(sb_scope, cpus_dev);
> aml_append(table, sb_scope);
>
> - method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
> - aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
> - aml_append(table, method);
> + if (event_handler_method) {
> + method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
> + aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
> + aml_append(table, method);
> + }
indeed, injecting a method won't work with GED (and well it's hackish to begin with).
Suggest to get rid of it and make event source initiate the call to CPU_SCAN_METHOD.
Something like (similar to what you do in 4/8): (maybe make it part of 4/8)
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 011d2c6c2d..ed35f2390f 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -341,7 +341,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
build_madt_cpu_fn build_madt_cpu, hwaddr io_base,
const char *res_root,
- const char *event_handler_method)
+ )
{
Aml *ifctx;
Aml *field;
@@ -696,9 +696,5 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
aml_append(sb_scope, cpus_dev);
aml_append(table, sb_scope);
- method = aml_method(event_handler_method, 0, AML_NOTSERIALIZED);
- aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
- aml_append(table, method);
-
g_free(cphp_res_path);
}
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 80db183b78..e740e402bf 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1856,6 +1856,8 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
aml_append(method, aml_release(aml_name("\\_SB.PCI0.BLCK")));
}
aml_append(scope, method);
+ if (!pcmc->legacy_cpu_hotplug) {
...
+ aml_append(method, aml_call0("\\_SB.CPUS." CPU_SCAN_METHOD));
...
}
}
aml_append(dsdt, scope);
}
>
> g_free(cphp_res_path);
> }
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 3f2b27cf75..f9f31f9db5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -1550,7 +1550,8 @@ 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", "\\_GPE._E02",
> + AML_SYSTEM_IO);
> }
>
> if (pcms->memhp_io_base && nr_mem) {
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index bc901660fb..b521a4e0de 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -60,9 +60,10 @@ typedef void (*build_madt_cpu_fn)(int uid, const CPUArchIdList *apic_ids,
> GArray *entry, bool force_enabled);
>
> 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);
> + const char *event_handler_method,
> + AmlRegionSpace rs);
>
> void acpi_cpu_ospm_status(CPUHotplugState *cpu_st, ACPIOSTInfoList ***list);
>
© 2016 - 2024 Red Hat, Inc.