Re: [PULL 04/41] pc: Start with modern CPU hotplug interface by default

[Peter Maydell <peter.maydell@linaro.org>]

On Thu, 12 Feb 2026 at 14:44, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> From: Zhao Liu <zhao1.liu@intel.com>
>
> 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).

Hi. This commit seems (according to git bisect) to have broken
"savevm" for the MIPS malta board (which uses piix4): it now
segfaults when you run the command from the monitor. Repro:

$ qemu-img create -f qcow2 dummy.qcow2 32M

Build QEMU with
'--target-list=mips-linux-user,mips64-linux-user,mipsel-linux-user,mipsn32-linux-user,mipsn32el-linux-user,mips-softmmu,mipsel-softmmu,mips64-softmmu,mips64el-softmmu'.

$ ./build/mips/qemu-system-mipsel -nographic -drive
if=none,format=qcow2,file=dummy.qcow2
[Type "C-a c" to get the "(qemu)" monitor prompt)]
(qemu) savevm foo

Backtrace from doing this under gdb:

#0  0x0000555555df7d4d in vmsd_can_compress (field=0x5555564f78a0
<__compound_literal.3>) at ../../migration/vmstate.c:339
#1  0x0000555555df7dbb in vmsd_desc_field_start
    (vmsd=0x555556431ba0 <vmstate_cpuhp_state>, vmdesc=0x555556918690,
field=0x5555564f78a0 <__compound_literal.3>, i=0, max=1) at
../../migration/vmstate.c:362
#2  0x0000555555df85a7 in vmstate_save_state_v
    (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
opaque=0x555556c9aac0, vmdesc=0x555556918690, version_id=1,
errp=0x7fffffffc948) at ../../migration/vmstate.c:528
#3  0x0000555555df8032 in vmstate_save_state
    (f=0x555556b5a0c0, vmsd=0x555556431ba0 <vmstate_cpuhp_state>,
opaque=0x555556c9aac0, vmdesc_id=0x555556918690, errp=0x7fffffffc948)
at ../../migration/vmstate.c:427
#4  0x0000555555df8f83 in vmstate_subsection_save
    (f=0x555556b5a0c0, vmsd=0x555556431c40 <vmstate_acpi>,
opaque=0x555556c9aac0, vmdesc=0x555556918690, errp=0x7fffffffc948)
    at ../../migration/vmstate.c:695
#5  0x0000555555df88a0 in vmstate_save_state_v
    (f=0x555556b5a0c0, vmsd=0x555556431c40 <vmstate_acpi>,
opaque=0x555556c9aac0, vmdesc=0x555556918690, version_id=3,
errp=0x7fffffffc948) at ../../migration/vmstate.c:582
#6  0x0000555555df8032 in vmstate_save_state
    (f=0x555556b5a0c0, vmsd=0x555556431c40 <vmstate_acpi>,
opaque=0x555556c9aac0, vmdesc_id=0x555556918690, errp=0x7fffffffc948)
at ../../migration/vmstate.c:427
#7  0x0000555555bd379e in vmstate_save (f=0x555556b5a0c0,
se=0x555557090ff0, vmdesc=0x555556918690, errp=0x7fffffffc948)
    at ../../migration/savevm.c:1054
#8  0x0000555555bd5133 in qemu_savevm_state_non_iterable
(f=0x555556b5a0c0, errp=0x7fffffffc948)
    at ../../migration/savevm.c:1726
#9  0x0000555555bd5226 in qemu_savevm_state_complete_precopy
(s=0x5555567a1d00) at ../../migration/savevm.c:1753
#10 0x0000555555bd5615 in qemu_savevm_state (f=0x555556b5a0c0,
errp=0x7fffffffcbf0) at ../../migration/savevm.c:1855
#11 0x0000555555bd8b83 in save_snapshot
    (name=0x555557497150 "foo", overwrite=true, vmstate=0x0,
has_devices=false, devices=0x0, errp=0x7fffffffcbf0)
    at ../../migration/savevm.c:3267
#12 0x0000555555ba9a62 in hmp_savevm (mon=0x55555691d950,
qdict=0x5555574973a0) at ../../migration/migration-hmp-cmds.c:481

The problem is that the vmstate_cpu_hotplug VMStateDescription
that is being used has a NULL "fields" (it is the "stub" one).
This used to be OK because the "piix4_pm/cpuhp" vmstate had
a "needed" function that would return false for Malta, so we
never tried to use the stub code.

What's the intention here ? Shouldn't we still have a "needed"
function that returns "false" for the cases where we would be
using the stub version of vmstate_cpu_hotplug ? Otherwise we
would be changing the migration data. We can permit a compat
break for MIPS boards, but the commit message suggests that
this wasn't intended to be a compat break.

In this thread Fabiano suggests a fix which is probably OK
if we want to take the "break migration compat" route:
https://lore.kernel.org/qemu-devel/CAFEAcA9BzSp9F8yv4eZv5eeUM4JXSy9T-QnRr_UbJZpNmhtHyw@mail.gmail.com/T/#m6f4a672ddfa8a0a34dfdf9b57bb04e529ab5c9a4


>  include/hw/acpi/cpu.h          |  1 -
>  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 -
>  6 files changed, 7 insertions(+), 50 deletions(-)
>
> diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h
> index 557219d2c63..2809dd8a911 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;
> diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
> index 6f1ae79edbf..d63ca83c1bc 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 2b3b493c014..54590129c69 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 19d4d4be932..0eda692084d 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 19c62362e31..cdd72cbcaa0 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 54ac94e17d3..3e34bedcd6f 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;
>
> --
> 2.52.0

thanks
-- PMM