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

[Igor Mammedov]

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