Re: [Qemu-devel] [RFC PATCH v2 05/23] spapr: Reorganize CPU dt generation code

[David Gibson]

On Wed, Mar 25, 2015 at 01:56:17PM +0530, Bharata B Rao wrote:
> On Wed, Mar 25, 2015 at 12:36:38PM +1100, David Gibson wrote:
> > On Mon, Mar 23, 2015 at 07:05:46PM +0530, Bharata B Rao wrote:
> > > Reorganize CPU device tree generation code so that it be reused from
> > > hotplug path. CPU dt entries are now generated from spapr_finalize_fdt()
> > > instead of spapr_create_fdt_skel().
> > > 
> > > Signed-off-by: Bharata B Rao <address@hidden>
> > > ---
> > >  hw/ppc/spapr.c | 288 
> > > ++++++++++++++++++++++++++++++---------------------------
> > >  1 file changed, 154 insertions(+), 134 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 36ff754..1a25cc0 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -206,24 +206,50 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int 
> > > offset, PowerPCCPU *cpu,
> > >      return ret;
> > >  }
> > >  
> > > +static int spapr_fixup_cpu_numa_smt_dt(void *fdt, int offset, CPUState 
> > > *cs,
> > > +                                        sPAPREnvironment *spapr)
> > > +{
> > > +    int ret;
> > > +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > +    int index = ppc_get_vcpu_dt_id(cpu);
> > > +    uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> > > +    uint32_t associativity[] = {cpu_to_be32(0x5),
> > > +                                cpu_to_be32(0x0),
> > > +                                cpu_to_be32(0x0),
> > > +                                cpu_to_be32(0x0),
> > > +                                cpu_to_be32(cs->numa_node),
> > > +                                cpu_to_be32(index)};
> > > +
> > > +    /* Advertise NUMA via ibm,associativity */
> > > +    if (nb_numa_nodes > 1) {
> > > +        ret = fdt_setprop(fdt, offset, "ibm,associativity", 
> > > associativity,
> > > +                          sizeof(associativity));
> > > +        if (ret < 0) {
> > > +            return ret;
> > > +        }
> > > +    }
> > > +
> > > +    ret = fdt_setprop(fdt, offset, "ibm,pft-size",
> > > +                      pft_size_prop, sizeof(pft_size_prop));
> > > +    if (ret < 0) {
> > > +        return ret;
> > > +    }
> > 
> > The pft-size property isn't actually related to NUMA, so it doesn't
> > really belong in this function.
> 
> Right, let me find an appropriate place to set this.

The top level cpu_fixup function sounds right to me.

> 
> > 
> > > +    return spapr_fixup_cpu_smt_dt(fdt, offset, cpu,
> > > +                                 ppc_get_compat_smt_threads(cpu));
> > 
> > Likewise calling the smt fixup function from the numa fixup function
> > just seems odd to me; just be explicit and call the two sequentially.
> 
> It's just poor naming, I meant numa-and-smt. So essentially it is
> fixing up NUMA and SMT related properties.

I realise that it's NUMA and SMT properties - but "NUMA & SMT" seems a
very arbitrary distinction.  It's not clear why that's a useful subset
of things to be handled by their own function.

> > Overall this seems an odd way to split things.  Why not just make a
> > spapr_fixup_one_cpu_dt() function, or similar, which should do all the
> > necessary pieces.
> 
> Just one routine to setup everything related to CPU DT will not work
> because some parts (NUMA and SMT bits) can be potentially fixed up
> later as part of ibm,client-architecture-support call. This is the
> reason why I have split up the reorg in this manner. Anyway will take
> another stab at this and see if I can improve this.

Ok.

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

pgpHb6XXbXJPn.pgp
Description: PGP signature