Re: [PATCH V2 05/10] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change

[Gavin Shan]

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>

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