Re: [Qemu-ppc] [PATCH] spapr_cpu_core: vmstate_[un]register per-CPU data from (un)realizefn

[Michael Roth]

Quoting David Gibson (2018-08-08 19:23:35)
> On Wed, Aug 08, 2018 at 09:29:19PM +0530, Bharata B Rao wrote:
> > VMStateDescription vmstate_spapr_cpu_state was added by commit
> > b94020268e0b6 (spapr_cpu_core: migrate per-CPU data) to migrate per-CPU
> > data with the required vmstate registration and unregistration calls.
> > However the unregistration is being done only from vcpu creation error path
> > and not from CPU delete path.
> > 
> > This causes migration to fail with the following error if migration is
> > attempted after a CPU unplug like this:
> > Unknown savevm section or instance 'spapr_cpu' 16
> > Additionally this leaves the source VM unresponsive after migration failure.
> > 
> > Fix this by ensuring the vmstate_unregister happens during CPU removal.
> > Fixing this becomes easier when vmstate (un)registration calls are moved to
> > vcpu (un)realize functions which is what this patch does.
> > 
> > Fixes: https://bugs.launchpad.net/qemu/+bug/1785972
> > Reported-by: Satheesh Rajendran <address@hidden>
> > Signed-off-by: Bharata B Rao <address@hidden>
> 
> Applied to ppc-for-3.1.
> 
> Unfortunately, despite being a clear regression, I think it's too late
> for 3.0 :(.
> 
> Mike, can you queue this for 3.0.1 as too, thanks?

Sure, will do.

> 
> > ---
> >  hw/ppc/spapr_cpu_core.c | 62 
> > +++++++++++++++++++++++++------------------------
> >  1 file changed, 32 insertions(+), 30 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 993759db47..bb88a3ce4e 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -113,26 +113,6 @@ const char *spapr_get_cpu_core_type(const char 
> > *cpu_type)
> >      return object_class_get_name(oc);
> >  }
> >  
> > -static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> > -{
> > -    qemu_unregister_reset(spapr_cpu_reset, cpu);
> > -    object_unparent(cpu->intc);
> > -    cpu_remove_sync(CPU(cpu));
> > -    object_unparent(OBJECT(cpu));
> > -}
> > -
> > -static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
> > -{
> > -    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > -    CPUCore *cc = CPU_CORE(dev);
> > -    int i;
> > -
> > -    for (i = 0; i < cc->nr_threads; i++) {
> > -        spapr_unrealize_vcpu(sc->threads[i]);
> > -    }
> > -    g_free(sc->threads);
> > -}
> > -
> >  static bool slb_shadow_needed(void *opaque)
> >  {
> >      sPAPRCPUState *spapr_cpu = opaque;
> > @@ -207,10 +187,34 @@ static const VMStateDescription 
> > vmstate_spapr_cpu_state = {
> >      }
> >  };
> >  
> > +static void spapr_unrealize_vcpu(PowerPCCPU *cpu, sPAPRCPUCore *sc)
> > +{
> > +    if (!sc->pre_3_0_migration) {
> > +        vmstate_unregister(NULL, &vmstate_spapr_cpu_state, 
> > cpu->machine_data);
> > +    }
> > +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> > +    object_unparent(cpu->intc);
> > +    cpu_remove_sync(CPU(cpu));
> > +    object_unparent(OBJECT(cpu));
> > +}
> > +
> > +static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > +    CPUCore *cc = CPU_CORE(dev);
> > +    int i;
> > +
> > +    for (i = 0; i < cc->nr_threads; i++) {
> > +        spapr_unrealize_vcpu(sc->threads[i], sc);
> > +    }
> > +    g_free(sc->threads);
> > +}
> > +
> >  static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > -                               Error **errp)
> > +                               sPAPRCPUCore *sc, Error **errp)
> >  {
> >      CPUPPCState *env = &cpu->env;
> > +    CPUState *cs = CPU(cpu);
> >      Error *local_err = NULL;
> >  
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> > @@ -233,6 +237,11 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, 
> > sPAPRMachineState *spapr,
> >          goto error_unregister;
> >      }
> >  
> > +    if (!sc->pre_3_0_migration) {
> > +        vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> > +                         cpu->machine_data);
> > +    }
> > +
> >      return;
> >  
> >  error_unregister:
> > @@ -272,10 +281,6 @@ static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, 
> > int i, Error **errp)
> >      }
> >  
> >      cpu->machine_data = g_new0(sPAPRCPUState, 1);
> > -    if (!sc->pre_3_0_migration) {
> > -        vmstate_register(NULL, cs->cpu_index, &vmstate_spapr_cpu_state,
> > -                         cpu->machine_data);
> > -    }
> >  
> >      object_unref(obj);
> >      return cpu;
> > @@ -290,9 +295,6 @@ static void spapr_delete_vcpu(PowerPCCPU *cpu, 
> > sPAPRCPUCore *sc)
> >  {
> >      sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> >  
> > -    if (!sc->pre_3_0_migration) {
> > -        vmstate_unregister(NULL, &vmstate_spapr_cpu_state, 
> > cpu->machine_data);
> > -    }
> >      cpu->machine_data = NULL;
> >      g_free(spapr_cpu);
> >      object_unparent(OBJECT(cpu));
> > @@ -325,7 +327,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, 
> > Error **errp)
> >      }
> >  
> >      for (j = 0; j < cc->nr_threads; j++) {
> > -        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> > +        spapr_realize_vcpu(sc->threads[j], spapr, sc, &local_err);
> >          if (local_err) {
> >              goto err_unrealize;
> >          }
> > @@ -334,7 +336,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, 
> > Error **errp)
> >  
> >  err_unrealize:
> >      while (--j >= 0) {
> > -        spapr_unrealize_vcpu(sc->threads[j]);
> > +        spapr_unrealize_vcpu(sc->threads[j], sc);
> >      }
> >  err:
> >      while (--i >= 0) {
> 
> -- 
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson