[PATCH V13 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change

Salil Mehta via posted 8 patches 5 months, 2 weeks ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Yanan Wang <wangyanan55@huawei.com>, Richard Henderson <richard.henderson@linaro.org>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>
There is a newer version of this series
[PATCH V13 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change
Posted by Salil Mehta via 5 months, 2 weeks ago
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.

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 <zhukeqian1@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
Reviewed-by: Gavin Shan <gshan@redhat.com>
Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: Xianglai Li <lixianglai@loongson.cn>
Tested-by: Miguel Luis <miguel.luis@oracle.com>
Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
Tested-by: Zhao Liu <zhao1.liu@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 {
+            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,
@@ -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);
 
-- 
2.34.1
Re: [PATCH V13 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change
Posted by Igor Mammedov 4 months, 3 weeks ago
On Fri, 7 Jun 2024 12:56:46 +0100
Salil Mehta <salil.mehta@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


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.
 
> 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 <zhukeqian1@huawei.com>
> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> Reviewed-by: Gavin Shan <gshan@redhat.com>
> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Tested-by: Xianglai Li <lixianglai@loongson.cn>
> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> Tested-by: Zhao Liu <zhao1.liu@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

> +
>          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);
>
Re: [PATCH V13 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change
Posted by Salil Mehta 4 months, 2 weeks ago
On 06/07/2024 14:35, Igor Mammedov wrote:
> On Fri, 7 Jun 2024 12:56:46 +0100
> Salil Mehta <salil.mehta@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.

>   
>> 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 <zhukeqian1@huawei.com>
>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
>> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Tested-by: Xianglai Li <lixianglai@loongson.cn>
>> Tested-by: Miguel Luis <miguel.luis@oracle.com>
>> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
>> Tested-by: Zhao Liu <zhao1.liu@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);
>>
Re: [PATCH V13 5/8] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change
Posted by Igor Mammedov 4 months, 2 weeks ago
On Mon, 8 Jul 2024 05:26:00 +0000
Salil Mehta <salil.mehta@opnsrc.net> wrote:

> On 06/07/2024 14:35, Igor Mammedov wrote:
> > On Fri, 7 Jun 2024 12:56:46 +0100
> > Salil Mehta <salil.mehta@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 <zhukeqian1@huawei.com>
> >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> >> Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
> >> Reviewed-by: Gavin Shan <gshan@redhat.com>
> >> Tested-by: Vishnu Pajjuri <vishnu@os.amperecomputing.com>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >> Tested-by: Xianglai Li <lixianglai@loongson.cn>
> >> Tested-by: Miguel Luis <miguel.luis@oracle.com>
> >> Reviewed-by: Shaoqin Huang <shahuang@redhat.com>
> >> Tested-by: Zhao Liu <zhao1.liu@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);
> >>     
>