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 <bhar...@linux.vnet.ibm.com>
> > > ---
> > > 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
pgp_s1SQc1dk7.pgp
Description: PGP signature
