All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] spapr_cpu_core: vmstate_[un]register per-CPU data from (un)realizefn
@ 2018-08-08 15:59 Bharata B Rao
  2018-08-09  0:23 ` David Gibson
  0 siblings, 1 reply; 3+ messages in thread
From: Bharata B Rao @ 2018-08-08 15:59 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-ppc, david, groug, sathnaga, Bharata B Rao

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 <sathnaga@linux.vnet.ibm.com>
Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
---
 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) {
-- 
2.14.3

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr_cpu_core: vmstate_[un]register per-CPU data from (un)realizefn
  2018-08-08 15:59 [Qemu-devel] [PATCH] spapr_cpu_core: vmstate_[un]register per-CPU data from (un)realizefn Bharata B Rao
@ 2018-08-09  0:23 ` David Gibson
  2018-08-09 12:34   ` Michael Roth
  0 siblings, 1 reply; 3+ messages in thread
From: David Gibson @ 2018-08-09  0:23 UTC (permalink / raw)
  To: Bharata B Rao; +Cc: qemu-devel, qemu-ppc, groug, sathnaga, mdroth

[-- Attachment #1: Type: text/plain, Size: 5559 bytes --]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr_cpu_core: vmstate_[un]register per-CPU data from (un)realizefn
  2018-08-09  0:23 ` David Gibson
@ 2018-08-09 12:34   ` Michael Roth
  0 siblings, 0 replies; 3+ messages in thread
From: Michael Roth @ 2018-08-09 12:34 UTC (permalink / raw)
  To: Bharata B Rao, David Gibson
  Cc: qemu-devel, qemu-ppc, groug, sathnaga, qemu-stable

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-09 12:35 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-08 15:59 [Qemu-devel] [PATCH] spapr_cpu_core: vmstate_[un]register per-CPU data from (un)realizefn Bharata B Rao
2018-08-09  0:23 ` David Gibson
2018-08-09 12:34   ` Michael Roth

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.