Re: [PATCH 2/8] Update CPUs AML with cpu-(ctrl)dev change

Tue, 01 Aug 2023 01:17:29 -0700 

Hi, Igor Mammedov :

On 7/28/23 7:55 PM, Igor Mammedov wrote:

On Thu, 20 Jul 2023 15:15:07 +0800
xianglai li <lixiang...@loongson.cn> 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 
LOONGARCH
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

                                                  ^^^ typo

Oh, I'm sorry for the typo.

function to accept both IO/MEMORY type regions and update the _CRS object
correspondingly.

try to reformat commit message  to less than 80 character per line

Ok! I'll modify it as suggested in the next version of patch.

Cc: Xiaojuan Yang <yangxiaoj...@loongson.cn>
Cc: Song Gao <gaos...@loongson.cn>
Cc: "Michael S. Tsirkin" <m...@redhat.com>
Cc: Igor Mammedov <imamm...@redhat.com>
Cc: Ani Sinha <anisi...@redhat.com>
Cc: Paolo Bonzini <pbonz...@redhat.com>
Cc: Richard Henderson <richard.hender...@linaro.org>
Cc: Eduardo Habkost <edua...@habkost.net>
Cc: Marcel Apfelbaum <marcel.apfelb...@gmail.com>
Cc: "Philippe Mathieu-Daudé" <phi...@linaro.org>
Cc: Yanan Wang <wangyana...@huawei.com>
Cc: "Daniel P. Berrangé" <berra...@redhat.com>
Cc: Peter Xu <pet...@redhat.com>
Cc: David Hildenbrand <da...@redhat.com>
Signed-off-by: xianglai li <lixiang...@loongson.cn>
---
  hw/acpi/cpu.c         | 30 +++++++++++++++++++++++-------
  hw/i386/acpi-build.c  |  2 +-
  include/hw/acpi/cpu.h |  5 +++--
  3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 6897c8789a..3b945a1a40 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -5,6 +5,7 @@
  #include "qapi/qapi-events-acpi.h"
  #include "trace.h"
  #include "sysemu/numa.h"
+#include "hw/acpi/cpu_hotplug.h"
#define OVMF_CPUHP_SMI_CMD 4 @@ -331,9 +332,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 mmap_io_base,
                      const char *res_root,
-                    const char *event_handler_method)
+                    const char *event_handler_method,
+                    AmlRegionSpace rs)
  {
      Aml *ifctx;
      Aml *field;
@@ -360,14 +362,28 @@ 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, mmap_io_base, mmap_io_base, 1,
                                 ACPI_CPU_HOTPLUG_REG_LEN));
+        } else {
+            aml_append(crs, aml_memory32_fixed(mmap_io_base,
+                               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),
-                                 ACPI_CPU_HOTPLUG_REG_LEN));
+        if (rs == AML_SYSTEM_IO) {
+            /* declare CPU hotplug MMIO region with related access fields */
+            aml_append(cpu_ctrl_dev,
+                aml_operation_region("PRST", AML_SYSTEM_IO,
+                                             aml_int(mmap_io_base),
+                                             ACPI_CPU_HOTPLUG_REG_LEN));
+        } else {
+            aml_append(cpu_ctrl_dev,
+                aml_operation_region("PRST", AML_SYSTEM_MEMORY,
+                                             aml_int(mmap_io_base),
+                                             ACPI_CPU_HOTPLUG_REG_LEN));
+        }


to reduce duplication, following could be better way to spell it:
  g_assert(rs == foo1 || rs == foo2)
  ... aml_operation_region("PRST", rs, aml_int(io_base), ...
Well, this suggestion makes the code look good, I'll fix it in the next patch release.
field = aml_field("PRST", AML_BYTE_ACC, AML_NOLOCK, 
                            AML_WRITE_AS_ZEROS);
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 9c74fa17ad..5d02690593 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -1548,7 +1548,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..cddea78333 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 mmap_io_base,
                      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!

xianglai.

Reply via email to