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