RE: [PATCH V6 5/9] hw/acpi: Update CPUs AML with cpu-(ctrl)dev change

[Salil Mehta]

Hi Igor,
Sorry, I missed this as well.

> From: Igor Mammedov <imammedo@redhat.com>
> Sent: Friday, October 27, 2023 2:47 PM
> To: Salil Mehta <salil.mehta@huawei.com>; mst@redhat.com
> 
> On Fri, 13 Oct 2023 11:51:25 +0100
> Salil Mehta <salil.mehta@huawei.com> 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
>                                ^^^^
> being placed in PCI0 context is no the reason why resource was described as 
> IO.
> being IO is probably historical thing (as legacy hp was implemented as IO)


I just meant that because it is IO port based therefore existing CPUs AML code
would evaluate to a system resource which describes IO Port address.



> _CRS could have been at _SB level as motherboard resource but, in that case
> we would need to carve out hole in PCI's _CRS  explicitly to exclude it.
> Hence it was placed in PCI0 context as a hack that helps to avoid us that.
> Perhaps it also applies to other targets.

Sure.

> > 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.
> 
> Also x86 should be able to switch to and work with MMIO region
> (I think QEMU wise IO and MMIO are the same)
> and we can just use MMIO, likely without any compat machinery.
> 
> aka. existing/running/migrated x86 guests will use IO instructions to access 
> region
> (since CRS they have read, says it is IO), while new VMs will access region 
> as MMIO.


Ok got it. But I guess existing CPUs AML code for ctrl-dev did not have this
change. Or did I miss anything?

 
> I might be wrong though,
>   Michael?
> 
> > On x86, Legacy 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 Legacy CPU Hotplug. Hence, make AML for
> > GPE.2 event handler conditional.
> 
> x86 has support for Legacy and Modern CPU hotplug (the later is enabled at 
> runtime).
> And both use GPE for event delivery, so above statement is not entirely 
> correct/confusing.

Ok yes. I will rephrase part of it.


Thanks
Salil.

> > 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>
> > ---
> >  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 4b24a25003..596b6d9d81 100644
> > --- a/hw/acpi/cpu.c
> > +++ b/hw/acpi/cpu.c
> > @@ -339,9 +339,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;
> > @@ -366,13 +367,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,
> > @@ -696,9 +703,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 3f2b27cf75..f9f31f9db5 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1550,7 +1550,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 bc901660fb..b521a4e0de 100644
> > --- a/include/hw/acpi/cpu.h
> > +++ b/include/hw/acpi/cpu.h
> > @@ -60,9 +60,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);
> >