Re: [PATCH 07/13] RFC migration: icp/server is a mess

[Nicholas Piggin]

On Fri Oct 20, 2023 at 7:39 AM AEST, Greg Kurz wrote:
> On Thu, 19 Oct 2023 21:08:25 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
> > Current code does:
> > - register pre_2_10_vmstate_dummy_icp with "icp/server" and instance
> >   dependinfg on cpu number
> > - for newer machines, it register vmstate_icp with "icp/server" name
> >   and instance 0
> > - now it unregisters "icp/server" for the 1st instance.
> > 
> > This is wrong at many levels:
> > - we shouldn't have two VMSTATEDescriptions with the same name
> > - In case this is the only solution that we can came with, it needs to
> >   be:
> >   * register pre_2_10_vmstate_dummy_icp
> >   * unregister pre_2_10_vmstate_dummy_icp
> >   * register real vmstate_icp
> > 
> > As the initialization of this machine is already complex enough, I
> > need help from PPC maintainers to fix this.
> > 
> > Volunteers?
> > 
> > CC: Cedric Le Goater <clg@kaod.org>
> > CC: Daniel Henrique Barboza <danielhb413@gmail.com>
> > CC: David Gibson <david@gibson.dropbear.id.au>
> > CC: Greg Kurz <groug@kaod.org>
> > 
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  hw/ppc/spapr.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index cb840676d3..8531d13492 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -143,7 +143,12 @@ static bool pre_2_10_vmstate_dummy_icp_needed(void 
> > *opaque)
> >  }
> >  
> >  static const VMStateDescription pre_2_10_vmstate_dummy_icp = {
> > -    .name = "icp/server",
> > +    /*
> > +     * Hack ahead.  We can't have two devices with the same name and
> > +     * instance id.  So I rename this to pass make check.
> > +     * Real help from people who knows the hardware is needed.
> > +     */
> > +    .name = "pre-2.10-icp/server",
> >      .version_id = 1,
> >      .minimum_version_id = 1,
> >      .needed = pre_2_10_vmstate_dummy_icp_needed,
>
> I guess this fix is acceptable as well and a lot simpler than
> reverting the hack actually. Outcome is the same : drop
> compat with pseries-2.9 and older.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>

So the reason we can't have duplicate names registered, aside from it
surely going bad if we actually send or receive a stream at the point
they are registered, is the duplcate check introduced in patch 9? But
before that, this hack does seem to actually work because the duplicate
is unregistered right away.

If I understand the workaround, there is an asymmetry in the migration
sequence in that receiving an unexpected object would cause a failure,
but going from newer to older would just skip some "expected" objects
and that didn't cause a problem. So you only have to deal with ignoring
the unexpected ones going form older to newer.

Side question, is it possible to flag the problem of *not* receiving
an object that you did expect? That might be a source of bugs too.

Anyway, I wonder if we could fix this spapr problem by adding a special
case wild card instance matcher to ignore it? It's still a bit hacky
but maybe a bit nicer. I don't mind deprecating the machine soon if
you want to clear the wildcard hack away soon, but it would be nice to
separate the deprecation and removal from the fix, if possible.

This patch is not tested but hopefully helps illustrate the idea.

Thanks,
Nick

diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1a31fb7293..8ce03edefa 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -1205,6 +1205,7 @@ int vmstate_save_state_v(QEMUFile *f, const 
VMStateDescription *vmsd,
 bool vmstate_save_needed(const VMStateDescription *vmsd, void *opaque);
 
 #define  VMSTATE_INSTANCE_ID_ANY  -1
+#define  VMSTATE_INSTANCE_ID_WILD -2
 
 /* Returns: 0 on success, -1 on failure */
 int vmstate_register_with_alias_id(VMStateIf *obj, uint32_t instance_id,
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index cb840676d3..2418899dd4 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -155,16 +155,10 @@ static const VMStateDescription 
pre_2_10_vmstate_dummy_icp = {
     },
 };
 
-static void pre_2_10_vmstate_register_dummy_icp(int i)
+static void pre_2_10_vmstate_register_dummy_icp(void)
 {
-    vmstate_register(NULL, i, &pre_2_10_vmstate_dummy_icp,
-                     (void *)(uintptr_t) i);
-}
-
-static void pre_2_10_vmstate_unregister_dummy_icp(int i)
-{
-    vmstate_unregister(NULL, &pre_2_10_vmstate_dummy_icp,
-                       (void *)(uintptr_t) i);
+    vmstate_register(NULL, VMSTATE_INSTANCE_ID_WILD,
+                     &pre_2_10_vmstate_dummy_icp, NULL);
 }
 
 int spapr_max_server_number(SpaprMachineState *spapr)
@@ -2665,12 +2659,10 @@ static void spapr_init_cpus(SpaprMachineState *spapr)
     }
 
     if (smc->pre_2_10_has_unused_icps) {
-        for (i = 0; i < spapr_max_server_number(spapr); i++) {
-            /* Dummy entries get deregistered when real ICPState objects
-             * are registered during CPU core hotplug.
-             */
-            pre_2_10_vmstate_register_dummy_icp(i);
-        }
+        /* Dummy entries get deregistered when real ICPState objects
+         * are registered during CPU core hotplug.
+         */
+        pre_2_10_vmstate_register_dummy_icp();
     }
 
     for (i = 0; i < possible_cpus->len; i++) {
@@ -3873,21 +3865,9 @@ void spapr_core_release(DeviceState *dev)
 static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev)
 {
     MachineState *ms = MACHINE(hotplug_dev);
-    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
     CPUCore *cc = CPU_CORE(dev);
     CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
 
-    if (smc->pre_2_10_has_unused_icps) {
-        SpaprCpuCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
-        int i;
-
-        for (i = 0; i < cc->nr_threads; i++) {
-            CPUState *cs = CPU(sc->threads[i]);
-
-            pre_2_10_vmstate_register_dummy_icp(cs->cpu_index);
-        }
-    }
-
     assert(core_slot);
     core_slot->cpu = NULL;
     qdev_unrealize(dev);
@@ -3968,10 +3948,8 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, 
DeviceState *dev)
 {
     SpaprMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
     MachineClass *mc = MACHINE_GET_CLASS(spapr);
-    SpaprMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
     SpaprCpuCore *core = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(dev);
-    CPUState *cs;
     SpaprDrc *drc;
     CPUArchId *core_slot;
     int index;
@@ -4018,13 +3996,6 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, 
DeviceState *dev)
                            &error_abort);
         }
     }
-
-    if (smc->pre_2_10_has_unused_icps) {
-        for (i = 0; i < cc->nr_threads; i++) {
-            cs = CPU(core->threads[i]);
-            pre_2_10_vmstate_unregister_dummy_icp(cs->cpu_index);
-        }
-    }
 }
 
 static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
diff --git a/migration/savevm.c b/migration/savevm.c
index 497ce02bd7..f33449e208 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -989,6 +989,10 @@ static int vmstate_save(QEMUFile *f, SaveStateEntry *se, 
JSONWriter *vmdesc)
         trace_savevm_section_skip(se->idstr, se->section_id);
         return 0;
     }
+    if (se->instance_id == VMSTATE_INSTANCE_ID_WILD) {
+        warn_report("Wildcard vmstate entry must set needed=false");
+        return 0;
+    }
 
     trace_savevm_section_start(se->idstr, se->section_id);
     save_section_header(f, se, QEMU_VM_SECTION_FULL);
@@ -1731,13 +1735,16 @@ int qemu_save_device_state(QEMUFile *f)
 
 static SaveStateEntry *find_se(const char *idstr, uint32_t instance_id)
 {
+    SaveStateEntry *se_wild = NULL;
     SaveStateEntry *se;
 
     QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
-        if (!strcmp(se->idstr, idstr) &&
-            (instance_id == se->instance_id ||
-             instance_id == se->alias_id))
-            return se;
+        if (!strcmp(se->idstr, idstr)) {
+            if (instance_id == se->instance_id || instance_id == se->alias_id)
+                return se;
+            if (se->instance_id == VMSTATE_INSTANCE_ID_WILD)
+                se_wild = se;
+        }
         /* Migrating from an older version? */
         if (strstr(se->idstr, idstr) && se->compat) {
             if (!strcmp(se->compat->idstr, idstr) &&
@@ -1746,7 +1753,7 @@ static SaveStateEntry *find_se(const char *idstr, 
uint32_t instance_id)
                 return se;
         }
     }
-    return NULL;
+    return se_wild;
 }
 
 enum LoadVMExitCodes {