Re: [PATCH v5 03/28] pc: Start with modern CPU hotplug interface by default

[Igor Mammedov <imammedo@redhat.com>]

On Wed,  3 Dec 2025 00:28:10 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> From: Igor Mammedov <imammedo@redhat.com>
^^^
given you resplit original patch, it's better to replace this with you,
keeping my SoB is sufficient

> 
> For compatibility reasons PC/Q35 will start with legacy CPU hotplug
> interface by default but with new CPU hotplug AML code since 2.7
> machine type (in commit 679dd1a957df ("pc: use new CPU hotplug interface
> since 2.7 machine type")). In that way, legacy firmware that doesn't use
> QEMU generated ACPI tables was able to continue using legacy CPU hotplug
> interface.
> 
> While later machine types, with firmware supporting QEMU provided ACPI
> tables, generate new CPU hotplug AML, which will switch to new CPU
> hotplug interface when guest OS executes its _INI method on ACPI tables
> loading.
> 
> Since 2.6 machine type is now gone, and consider that the legacy BIOS
> (based on QEMU ACPI prior to v2.7) should be no longer in use, previous
> compatibility requirements are no longer necessary. So initialize
> 'modern' hotplug directly from the very beginning for PC/Q35 machines
> with cpu_hotplug_hw_init(), and drop _INIT method.
> 
> Additionally, remove the checks and settings around cpu_hotplug_legacy
> in cpuhp VMState (for piix4 & ich9), to eliminate the risk of
> segmentation faults, as gpe_cpu no longer has the opportunity to be
> initialized. This is safe because all hotplug now start with the modern
> way, and it's impossible to switch to legacy way at runtime (even the
> "cpu-hotplug-legacy" properties does not allow it either).
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>

tested ping pong cross version (master vs master+this patch) migration
with 10.1 machine type, nothing is broken, hence

Acked-by: Igor Mammedov <imammedo@redhat.com>

> ---
> Changes since v4:
>  * New patch split off from Igor's v5 [*].
> 
> [*]: https://lore.kernel.org/qemu-devel/20251031142825.179239-1-imammedo@redhat.com/
> ---
>  hw/acpi/cpu.c                  | 10 ----------
>  hw/acpi/ich9.c                 | 22 +++-------------------
>  hw/acpi/piix4.c                | 21 +++------------------
>  hw/i386/acpi-build.c           |  2 +-
>  hw/loongarch/virt-acpi-build.c |  1 -
>  include/hw/acpi/cpu.h          |  1 -
>  6 files changed, 7 insertions(+), 50 deletions(-)
> 
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 6f1ae79edbf3..d63ca83c1bcd 100644
> --- a/hw/acpi/cpu.c
> +++ b/hw/acpi/cpu.c
> @@ -408,16 +408,6 @@ void build_cpus_aml(Aml *table, MachineState *machine, CPUHotplugFeatures opts,
>          aml_append(field, aml_reserved_field(4 * 8));
>          aml_append(field, aml_named_field(CPU_DATA, 32));
>          aml_append(cpu_ctrl_dev, field);
> -
> -        if (opts.has_legacy_cphp) {
> -            method = aml_method("_INI", 0, AML_SERIALIZED);
> -            /* switch off legacy CPU hotplug HW and use new one,
> -             * on reboot system is in new mode and writing 0
> -             * in CPU_SELECTOR selects BSP, which is NOP at
> -             * the time _INI is called */
> -            aml_append(method, aml_store(zero, aml_name(CPU_SELECTOR)));
> -            aml_append(cpu_ctrl_dev, method);
> -        }
>      }
>      aml_append(sb_scope, cpu_ctrl_dev);
>  
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 2b3b493c014b..54590129c695 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -183,26 +183,10 @@ static const VMStateDescription vmstate_tco_io_state = {
>      }
>  };
>  
> -static bool vmstate_test_use_cpuhp(void *opaque)
> -{
> -    ICH9LPCPMRegs *s = opaque;
> -    return !s->cpu_hotplug_legacy;
> -}
> -
> -static int vmstate_cpuhp_pre_load(void *opaque)
> -{
> -    ICH9LPCPMRegs *s = opaque;
> -    Object *obj = OBJECT(s->gpe_cpu.device);
> -    object_property_set_bool(obj, "cpu-hotplug-legacy", false, &error_abort);
> -    return 0;
> -}
> -
>  static const VMStateDescription vmstate_cpuhp_state = {
>      .name = "ich9_pm/cpuhp",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .needed = vmstate_test_use_cpuhp,
> -    .pre_load = vmstate_cpuhp_pre_load,
>      .fields = (const VMStateField[]) {
>          VMSTATE_CPU_HOTPLUG(cpuhp_state, ICH9LPCPMRegs),
>          VMSTATE_END_OF_LIST()
> @@ -338,8 +322,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm, qemu_irq sci_irq)
>      pm->powerdown_notifier.notify = pm_powerdown_req;
>      qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>  
> -    legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> -        OBJECT(lpc_pci), &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
> +    cpu_hotplug_hw_init(pci_address_space_io(lpc_pci),
> +        OBJECT(lpc_pci), &pm->cpuhp_state, ICH9_CPU_HOTPLUG_IO_BASE);
>  
>      acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
>                               &pm->acpi_memory_hotplug,
> @@ -419,7 +403,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
>  {
>      static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
>      pm->acpi_memory_hotplug.is_enabled = true;
> -    pm->cpu_hotplug_legacy = true;
> +    pm->cpu_hotplug_legacy = false;
>      pm->disable_s3 = 0;
>      pm->disable_s4 = 0;
>      pm->s4_val = 2;
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 7a18f18dda21..a7a29b0d09a9 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -195,25 +195,10 @@ static const VMStateDescription vmstate_memhp_state = {
>      }
>  };
>  
> -static bool vmstate_test_use_cpuhp(void *opaque)
> -{
> -    PIIX4PMState *s = opaque;
> -    return !s->cpu_hotplug_legacy;
> -}
> -
> -static int vmstate_cpuhp_pre_load(void *opaque)
> -{
> -    Object *obj = OBJECT(opaque);
> -    object_property_set_bool(obj, "cpu-hotplug-legacy", false, &error_abort);
> -    return 0;
> -}
> -
>  static const VMStateDescription vmstate_cpuhp_state = {
>      .name = "piix4_pm/cpuhp",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .needed = vmstate_test_use_cpuhp,
> -    .pre_load = vmstate_cpuhp_pre_load,
>      .fields = (const VMStateField[]) {
>          VMSTATE_CPU_HOTPLUG(cpuhp_state, PIIX4PMState),
>          VMSTATE_END_OF_LIST()
> @@ -573,12 +558,12 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
>          qbus_set_hotplug_handler(BUS(pci_get_bus(PCI_DEVICE(s))), OBJECT(s));
>      }
>  
> -    s->cpu_hotplug_legacy = true;
> +    s->cpu_hotplug_legacy = false;
>      object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
>                               piix4_get_cpu_hotplug_legacy,
>                               piix4_set_cpu_hotplug_legacy);
> -    legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe_cpu,
> -                                 PIIX4_CPU_HOTPLUG_IO_BASE);
> +    cpu_hotplug_hw_init(parent, OBJECT(s), &s->cpuhp_state,
> +                        PIIX4_CPU_HOTPLUG_IO_BASE);
>  
>      if (s->acpi_memory_hotplug.is_enabled) {
>          acpi_memory_hotplug_init(parent, OBJECT(s), &s->acpi_memory_hotplug,
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 9446a9f862ca..23147ddc25e7 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -964,7 +964,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker,
>          build_legacy_cpu_hotplug_aml(dsdt, machine, pm->cpu_hp_io_base);
>      } else {
>          CPUHotplugFeatures opts = {
> -            .acpi_1_compatible = true, .has_legacy_cphp = true,
> +            .acpi_1_compatible = true,
>              .smi_path = pm->smi_on_cpuhp ? "\\_SB.PCI0.SMI0.SMIC" : NULL,
>              .fw_unplugs_cpu = pm->smi_on_cpu_unplug,
>          };
> diff --git a/hw/loongarch/virt-acpi-build.c b/hw/loongarch/virt-acpi-build.c
> index 3694c9827f04..8d01c8e3de87 100644
> --- a/hw/loongarch/virt-acpi-build.c
> +++ b/hw/loongarch/virt-acpi-build.c
> @@ -369,7 +369,6 @@ build_la_ged_aml(Aml *dsdt, MachineState *machine)
>  
>      if (event & ACPI_GED_CPU_HOTPLUG_EVT) {
>          opts.acpi_1_compatible = false;
> -        opts.has_legacy_cphp = false;
>          opts.fw_unplugs_cpu = false;
>          opts.smi_path = NULL;
>  
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index 32654dc274fd..2cb0ca4f3dce 100644
> --- a/include/hw/acpi/cpu.h
> +++ b/include/hw/acpi/cpu.h
> @@ -54,7 +54,6 @@ void cpu_hotplug_hw_init(MemoryRegion *as, Object *owner,
>  
>  typedef struct CPUHotplugFeatures {
>      bool acpi_1_compatible;
> -    bool has_legacy_cphp;
>      bool fw_unplugs_cpu;
>      const char *smi_path;
>  } CPUHotplugFeatures;