There's no real reason to create all CPUs in a first pass and to realize
them in a second pass. Merging these two loops makes the code simpler.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |   25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 0ebaf804a9bc..f52af20e0096 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -172,7 +172,8 @@ error:
     error_propagate(errp, local_err);
 }
 
-static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
+static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i,
+                                     sPAPRMachineState *spapr, Error **errp)
 {
     sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
     CPUCore *cc = CPU_CORE(sc);
@@ -201,9 +202,16 @@ static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
         goto err;
     }
 
+    spapr_realize_vcpu(cpu, spapr, &local_err);
+    if (local_err) {
+        goto err_unparent;
+    }
+
     object_unref(obj);
     return cpu;
 
+err_unparent:
+    object_unparent(obj);
 err:
     object_unref(obj);
     error_propagate(errp, local_err);
@@ -212,6 +220,7 @@ err:
 
 static void spapr_delete_vcpu(PowerPCCPU *cpu)
 {
+    spapr_unrealize_vcpu(cpu);
     object_unparent(OBJECT(cpu));
 }
 
@@ -226,7 +235,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(OBJECT(dev));
     Error *local_err = NULL;
-    int i, j;
+    int i;
 
     if (!spapr) {
         error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
@@ -235,24 +244,14 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
 
     sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
-        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
+        sc->threads[i] = spapr_create_vcpu(sc, i, spapr, &local_err);
         if (local_err) {
             goto err;
         }
     }
 
-    for (j = 0; j < cc->nr_threads; j++) {
-        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
-        if (local_err) {
-            goto err_unrealize;
-        }
-    }
     return;
 
-err_unrealize:
-    while (--j >= 0) {
-        spapr_unrealize_vcpu(sc->threads[i]);
-    }
 err:
     while (--i >= 0) {
         spapr_delete_vcpu(sc->threads[i]);

On Thu, Jun 14, 2018 at 11:51:11PM +0200, Greg Kurz wrote:
> There's no real reason to create all CPUs in a first pass and to realize
> them in a second pass. Merging these two loops makes the code simpler.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I'm a bit uncertain about this one.  It's correct at the moment, but
I'm wondering if there might be things we want to do wile the cpu
objects are constructed, but not initialized.

In fact, I thought I wanted something like that for allowing earlier
initialization of the default caps values, though in the end I found a
simpler and better approach.

So, I'm holding off on this one for the time being.

> ---
>  hw/ppc/spapr_cpu_core.c |   25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 0ebaf804a9bc..f52af20e0096 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -172,7 +172,8 @@ error:
>      error_propagate(errp, local_err);
>  }
>  
> -static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
> +static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i,
> +                                     sPAPRMachineState *spapr, Error **errp)
>  {
>      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
>      CPUCore *cc = CPU_CORE(sc);
> @@ -201,9 +202,16 @@ static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
>          goto err;
>      }
>  
> +    spapr_realize_vcpu(cpu, spapr, &local_err);
> +    if (local_err) {
> +        goto err_unparent;
> +    }
> +
>      object_unref(obj);
>      return cpu;
>  
> +err_unparent:
> +    object_unparent(obj);
>  err:
>      object_unref(obj);
>      error_propagate(errp, local_err);
> @@ -212,6 +220,7 @@ err:
>  
>  static void spapr_delete_vcpu(PowerPCCPU *cpu)
>  {
> +    spapr_unrealize_vcpu(cpu);
>      object_unparent(OBJECT(cpu));
>  }
>  
> @@ -226,7 +235,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(OBJECT(dev));
>      Error *local_err = NULL;
> -    int i, j;
> +    int i;
>  
>      if (!spapr) {
>          error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
> @@ -235,24 +244,14 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>  
>      sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> -        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
> +        sc->threads[i] = spapr_create_vcpu(sc, i, spapr, &local_err);
>          if (local_err) {
>              goto err;
>          }
>      }
>  
> -    for (j = 0; j < cc->nr_threads; j++) {
> -        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> -        if (local_err) {
> -            goto err_unrealize;
> -        }
> -    }
>      return;
>  
> -err_unrealize:
> -    while (--j >= 0) {
> -        spapr_unrealize_vcpu(sc->threads[i]);
> -    }
>  err:
>      while (--i >= 0) {
>          spapr_delete_vcpu(sc->threads[i]);
> 

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

On Fri, 15 Jun 2018 10:08:29 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 14, 2018 at 11:51:11PM +0200, Greg Kurz wrote:
> > There's no real reason to create all CPUs in a first pass and to realize
> > them in a second pass. Merging these two loops makes the code simpler.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> I'm a bit uncertain about this one.  It's correct at the moment, but
> I'm wondering if there might be things we want to do wile the cpu
> objects are constructed, but not initialized.
> 

Yeah, I thought about that also, but I couldn't find any case...

> In fact, I thought I wanted something like that for allowing earlier
> initialization of the default caps values, though in the end I found a
> simpler and better approach.
> 
> So, I'm holding off on this one for the time being.
> 

Sure.

> > ---
> >  hw/ppc/spapr_cpu_core.c |   25 ++++++++++++-------------
> >  1 file changed, 12 insertions(+), 13 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 0ebaf804a9bc..f52af20e0096 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -172,7 +172,8 @@ error:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > -static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
> > +static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i,
> > +                                     sPAPRMachineState *spapr, Error **errp)
> >  {
> >      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_GET_CLASS(sc);
> >      CPUCore *cc = CPU_CORE(sc);
> > @@ -201,9 +202,16 @@ static PowerPCCPU *spapr_create_vcpu(sPAPRCPUCore *sc, int i, Error **errp)
> >          goto err;
> >      }
> >  
> > +    spapr_realize_vcpu(cpu, spapr, &local_err);
> > +    if (local_err) {
> > +        goto err_unparent;
> > +    }
> > +
> >      object_unref(obj);
> >      return cpu;
> >  
> > +err_unparent:
> > +    object_unparent(obj);
> >  err:
> >      object_unref(obj);
> >      error_propagate(errp, local_err);
> > @@ -212,6 +220,7 @@ err:
> >  
> >  static void spapr_delete_vcpu(PowerPCCPU *cpu)
> >  {
> > +    spapr_unrealize_vcpu(cpu);
> >      object_unparent(OBJECT(cpu));
> >  }
> >  
> > @@ -226,7 +235,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> >      CPUCore *cc = CPU_CORE(OBJECT(dev));
> >      Error *local_err = NULL;
> > -    int i, j;
> > +    int i;
> >  
> >      if (!spapr) {
> >          error_setg(errp, TYPE_SPAPR_CPU_CORE " needs a pseries machine");
> > @@ -235,24 +244,14 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >  
> >      sc->threads = g_new(PowerPCCPU *, cc->nr_threads);
> >      for (i = 0; i < cc->nr_threads; i++) {
> > -        sc->threads[i] = spapr_create_vcpu(sc, i, &local_err);
> > +        sc->threads[i] = spapr_create_vcpu(sc, i, spapr, &local_err);
> >          if (local_err) {
> >              goto err;
> >          }
> >      }
> >  
> > -    for (j = 0; j < cc->nr_threads; j++) {
> > -        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> > -        if (local_err) {
> > -            goto err_unrealize;
> > -        }
> > -    }
> >      return;
> >  
> > -err_unrealize:
> > -    while (--j >= 0) {
> > -        spapr_unrealize_vcpu(sc->threads[i]);
> > -    }
> >  err:
> >      while (--i >= 0) {
> >          spapr_delete_vcpu(sc->threads[i]);
> >   
>