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

[Zhao Liu <zhao1.liu@intel.com>]

+Philippe & Aurelien

(And seeking Igor's input)

> 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.

Thanks, this is my bad - I didn't realize it would break Malta. The
"needed" method looks necessary, but previously making the "stub" one
depend on cpu_hotplug_legacy=true also sounds a bit not proper.

Maybe in the needed() method we should also check whether the board/
machine supports CPU hotplug: check mc->has_hotpluggable_cpus. IIUC,
Malta doesn't support this.

> 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

I think Fabiano's patch is a clean solution. Or I think there's another
option: we can bring needed() back but check mc->has_hotpluggable_cpus
instead, like acpi-ged/cpuhp did ().

Igor, what do you think?

---
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index bbb1bd60a206..5c7dfb2c69db 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -184,10 +184,18 @@ static const VMStateDescription vmstate_tco_io_state = {
     }
 };

+static bool cpuhp_needed(void *opaque)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+
+    return mc->has_hotpluggable_cpus;
+}
+
 static const VMStateDescription vmstate_cpuhp_state = {
     .name = "ich9_pm/cpuhp",
     .version_id = 1,
     .minimum_version_id = 1,
+    .needed = cpuhp_needed,
     .fields = (const VMStateField[]) {
         VMSTATE_CPU_HOTPLUG(cpuhp_state, ICH9LPCPMRegs),
         VMSTATE_END_OF_LIST()
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 43860d122780..9b7f50c7afac 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -195,10 +195,18 @@ static const VMStateDescription vmstate_memhp_state = {
     }
 };

+static bool cpuhp_needed(void *opaque)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
+
+    return mc->has_hotpluggable_cpus;
+}
+
 static const VMStateDescription vmstate_cpuhp_state = {
     .name = "piix4_pm/cpuhp",
     .version_id = 1,
     .minimum_version_id = 1,
+    .needed = cpuhp_needed,
     .fields = (const VMStateField[]) {
         VMSTATE_CPU_HOTPLUG(cpuhp_state, PIIX4PMState),
         VMSTATE_END_OF_LIST()