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 <sathn...@linux.vnet.ibm.com>
> > Signed-off-by: Bharata B Rao <bhar...@linux.ibm.com>
>
> 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
