On Mon, 8 Jul 2024 05:26:00 +0000
Salil Mehta <salil.me...@opnsrc.net> wrote:
> On 06/07/2024 14:35, Igor Mammedov wrote:
> > On Fri, 7 Jun 2024 12:56:46 +0100
> > Salil Mehta <salil.me...@huawei.com> wrote:
> >
> >> 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.
> > ack for above change
> Thanks
> >
> >
> > but below part is one too many different changes withing 1 patch.
> > anyways, GPE part probably won't be needed if you follow suggestion made
> > on previous patch.
>
> The change mentioned in the earlier patches might end up creating
>
> noise for this patch-set as one will have to touch the Memory Hotplug
>
> part as well. I'm willing to do that change but I think it is a noise for
>
> this patch-set, really.
you don't have to touch memory hotplug,
but fixing it up (as a separate patch of cause) to be consistent
with cpu hotplug would be nice.
>
> >
> >> 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 <zhukeqi...@huawei.com>
> >> Signed-off-by: Keqian Zhu <zhukeqi...@huawei.com>
> >> Signed-off-by: Salil Mehta <salil.me...@huawei.com>
> >> Reviewed-by: Gavin Shan <gs...@redhat.com>
> >> Tested-by: Vishnu Pajjuri <vis...@os.amperecomputing.com>
> >> Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
> >> Tested-by: Xianglai Li <lixiang...@loongson.cn>
> >> Tested-by: Miguel Luis <miguel.l...@oracle.com>
> >> Reviewed-by: Shaoqin Huang <shahu...@redhat.com>
> >> Tested-by: Zhao Liu <zhao1....@intel.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 af2b6655d2..4c63514b16 100644
> >> --- a/hw/acpi/cpu.c
> >> +++ b/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)
> >> + const char *event_handler_method,
> >> + AmlRegionSpace rs)
> >> {
> >> Aml *ifctx;
> >> Aml *field;
> >> @@ -370,13 +371,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 {
> > else
> > if (rs == yours type)
> >> + aml_append(crs, aml_memory32_fixed(base_addr,
> >> + ACPI_CPU_HOTPLUG_REG_LEN, AML_READ_WRITE));
> >> + }
> > else assert on not supported input
>
> Sure, no problem. I can incorporate the change.
>
> Thanks, Salil.
>
> >
> >> +
> >> 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,
> >> @@ -700,9 +707,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 53f804ac16..b73b136605 100644
> >> --- a/hw/i386/acpi-build.c
> >> +++ b/hw/i386/acpi-build.c
> >> @@ -1537,7 +1537,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 e6e1a9ef59..48cded697c 100644
> >> --- a/include/hw/acpi/cpu.h
> >> +++ b/include/hw/acpi/cpu.h
> >> @@ -61,9 +61,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);
> >>
>
