> From: Gavin Shan <gs...@redhat.com> > Sent: Tuesday, October 3, 2023 1:09 AM > To: Salil Mehta <salil.me...@huawei.com>; qemu-devel@nongnu.org; qemu- > a...@nongnu.org > Cc: m...@kernel.org; jean-phili...@linaro.org; Jonathan Cameron > <jonathan.came...@huawei.com>; lpieral...@kernel.org; > peter.mayd...@linaro.org; richard.hender...@linaro.org; > imamm...@redhat.com; andrew.jo...@linux.dev; da...@redhat.com; > phi...@linaro.org; eric.au...@redhat.com; oliver.up...@linux.dev; > pbonz...@redhat.com; m...@redhat.com; w...@kernel.org; raf...@kernel.org; > alex.ben...@linaro.org; li...@armlinux.org.uk; > dar...@os.amperecomputing.com; il...@os.amperecomputing.com; > vis...@os.amperecomputing.com; karl.heub...@oracle.com; > miguel.l...@oracle.com; salil.me...@opnsrc.net; zhukeqian > <zhukeqi...@huawei.com>; wangxiongfeng (C) <wangxiongfe...@huawei.com>; > wangyanan (Y) <wangyana...@huawei.com>; jiakern...@gmail.com; > maob...@loongson.cn; lixiang...@loongson.cn; Linuxarm <linux...@huawei.com> > Subject: Re: [PATCH V2 05/10] hw/acpi: Update CPUs AML with cpu-(ctrl)dev > change > > On 9/30/23 10:19, Salil Mehta wrote: > > CPUs Control device(\\_SB.PCI0) register interface for the x86 arch is > > based on > > PCI and is IO port based and hence 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. > > > > This cpus AML code change updates the existing inerface of the build cpus > > AML > > function to accept both IO/MEMORY type regions and update the _CRS object > > correspondingly. > > > > 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> > > --- > > hw/acpi/cpu.c | 23 ++++++++++++++++------- > > hw/i386/acpi-build.c | 2 +- > > include/hw/acpi/cpu.h | 5 +++-- > > 3 files changed, 20 insertions(+), 10 deletions(-) > > > > With commit log improved to address Jonathan's comments why > @event_handler_method > won't be needed on aarch64: > > Reviewed-by: Gavin Shan <gs...@redhat.com>
Thanks Salil. > > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > > index 45defdc0e2..66a71660ec 100644 > > --- a/hw/acpi/cpu.c > > +++ b/hw/acpi/cpu.c > > @@ -338,9 +338,10 @@ const VMStateDescription vmstate_cpu_hotplug = { > > #define CPU_FW_EJECT_EVENT "CEJF" > > > > void build_cpus_aml(Aml *table, MachineState *machine, > CPUHotplugFeatures opts, > > - hwaddr io_base, > > + hwaddr base_addr, > > const char *res_root, > > - const char *event_handler_method) > > + const char *event_handler_method, > > + AmlRegionSpace rs) > > { > > Aml *ifctx; > > Aml *field; > > @@ -367,13 +368,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, > > @@ -699,9 +706,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 4d2d40bab5..611d3d044d 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -1550,7 +1550,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > .fw_unplugs_cpu = pm->smi_on_cpu_unplug, > > }; > > build_cpus_aml(dsdt, machine, opts, pm->cpu_hp_io_base, > > - "\\_SB.PCI0", "\\_GPE._E02"); > > + "\\_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 999caaf510..b87ebfdf4b 100644 > > --- a/include/hw/acpi/cpu.h > > +++ b/include/hw/acpi/cpu.h > > @@ -56,9 +56,10 @@ typedef struct CPUHotplugFeatures { > > } CPUHotplugFeatures; > > > > void build_cpus_aml(Aml *table, MachineState *machine, > CPUHotplugFeatures opts, > > - hwaddr io_base, > > + 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); > > > > Thanks, > Gavin