| Message ID | 20231013105129.25648-6-salil.mehta@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add architecture agnostic code to support vCPU Hotplug | expand |
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) _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. > 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. 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. > > 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); >
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); > >
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);