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

[David Gibson <david@gibson.dropbear.id.au>]

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