| Message ID | 20231113201236.30668-6-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add architecture agnostic code to support vCPU Hotplug | expand |
On 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); >
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);