Re: [Qemu-devel] [PATCH 5/5] spapr_cpu_core: simplify spapr_cpu_core_realize()

[Greg Kurz <groug@kaod.org>]

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

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]);
> >   
> 


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