diff mbox series

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

Message ID d40ea40fe8290160f95a79515bebc20c4fbfe48f.1689837093.git.lixianglai@loongson.cn
State New
Headers show
Series Adds CPU hot-plug support to Loongarch | expand

Commit Message

Xianglai Li July 20, 2023, 7:15 a.m. UTC 
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
function to accept both IO/MEMORY type regions and update the _CRS object
correspondingly.

Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
Cc: Song Gao <gaosong@loongson.cn>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Ani Sinha <anisinha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: Yanan Wang <wangyanan55@huawei.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: xianglai li <lixianglai@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(-)

Comments

Igor Mammedov July 28, 2023, 11:55 a.m. UTC | #1 
On Thu, 20 Jul 2023 15:15:07 +0800
xianglai li <lixianglai@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
> 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

> 
> Cc: Xiaojuan Yang <yangxiaojuan@loongson.cn>
> Cc: Song Gao <gaosong@loongson.cn>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Ani Sinha <anisinha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
> Cc: Yanan Wang <wangyanan55@huawei.com>
> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: xianglai li <lixianglai@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), ...

>  
>          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);
>
Xianglai Li Aug. 1, 2023, 8:16 a.m. UTC | #2 
Hi, Igor Mammedov :

On 7/28/23 7:55 PM, Igor Mammedov wrote:
> On Thu, 20 Jul 2023 15:15:07 +0800
> xianglai li <lixianglai@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 <yangxiaojuan@loongson.cn>
>> Cc: Song Gao <gaosong@loongson.cn>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Ani Sinha <anisinha@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <richard.henderson@linaro.org>
>> Cc: Eduardo Habkost <eduardo@habkost.net>
>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>> Cc: "Philippe Mathieu-Daudé" <philmd@linaro.org>
>> Cc: Yanan Wang <wangyanan55@huawei.com>
>> Cc: "Daniel P. Berrangé" <berrange@redhat.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Signed-off-by: xianglai li <lixianglai@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.
diff mbox series

Patch

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));
+        }
 
         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);