From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35323) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaxFR-0006hd-7a for qemu-devel@nongnu.org; Wed, 25 Mar 2015 22:10:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaxFP-0003a2-Sw for qemu-devel@nongnu.org; Wed, 25 Mar 2015 22:10:17 -0400 Date: Thu, 26 Mar 2015 12:40:30 +1100 From: David Gibson Message-ID: <20150326014030.GI28039@voom.redhat.com> References: <1427117764-23008-1-git-send-email-bharata@linux.vnet.ibm.com> <1427117764-23008-6-git-send-email-bharata@linux.vnet.ibm.com> <20150325013638.GP25043@voom.fritz.box> <20150325082616.GB32581@in.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="svExV93C05KqedWb" Content-Disposition: inline In-Reply-To: <20150325082616.GB32581@in.ibm.com> Subject: Re: [Qemu-devel] [RFC PATCH v2 05/23] spapr: Reorganize CPU dt generation code List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Bharata B Rao Cc: mdroth@linux.vnet.ibm.com, agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, imammedo@redhat.com, afaerber@suse.de --svExV93C05KqedWb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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_fd= t() > > > instead of spapr_create_fdt_skel(). > > >=20 > > > Signed-off-by: Bharata B Rao > > > --- > > > hw/ppc/spapr.c | 288 ++++++++++++++++++++++++++++++-----------------= ---------- > > > 1 file changed, 154 insertions(+), 134 deletions(-) > > >=20 > > > 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, in= t offset, PowerPCCPU *cpu, > > > return ret; > > > } > > > =20 > > > +static int spapr_fixup_cpu_numa_smt_dt(void *fdt, int offset, CPUSta= te *cs, > > > + sPAPREnvironment *spapr) > > > +{ > > > + int ret; > > > + PowerPCCPU *cpu =3D POWERPC_CPU(cs); > > > + int index =3D ppc_get_vcpu_dt_id(cpu); > > > + uint32_t pft_size_prop[] =3D {0, cpu_to_be32(spapr->htab_shift)}; > > > + uint32_t associativity[] =3D {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 =3D fdt_setprop(fdt, offset, "ibm,associativity", associ= ativity, > > > + sizeof(associativity)); > > > + if (ret < 0) { > > > + return ret; > > > + } > > > + } > > > + > > > + ret =3D fdt_setprop(fdt, offset, "ibm,pft-size", > > > + pft_size_prop, sizeof(pft_size_prop)); > > > + if (ret < 0) { > > > + return ret; > > > + } > >=20 > > The pft-size property isn't actually related to NUMA, so it doesn't > > really belong in this function. >=20 > Right, let me find an appropriate place to set this. The top level cpu_fixup function sounds right to me. >=20 > >=20 > > > + return spapr_fixup_cpu_smt_dt(fdt, offset, cpu, > > > + ppc_get_compat_smt_threads(cpu)); > >=20 > > Likewise calling the smt fixup function from the numa fixup function > > just seems odd to me; just be explicit and call the two sequentially. >=20 > 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. >=20 > 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. --=20 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 --svExV93C05KqedWb Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVE2ONAAoJEGw4ysog2bOSX/gQANfZZtZeW38VbBuU6JXwoWMQ /nOx7OmPMLNIWTKmboe6/u2OMiEkHI/airbPKJ8QXan6vXoxoqkBCc7/ZIHdBMil cFiPYn/QOgi0nunFxK+H+CXL9kh/Hcb6hvSm+7LCutZ1c7yl+avj3h5KVib1/4LL 5bXa3AKySZH1oz0d3Sz8rDkeW57fSdil67i2ZbzOalVjS6pKtJc3l5Nj5Pc0Dadh 6vnIxlBcCdCL77fEP+0iDJGsjwKANSK58VPgXNBR/mRHa9NWwsVbqwDupvOm7wAf qfC4Qk0ec7iHotyW7HpHjjBHVBckx2PDDIdBR12m3A73o/0UUg//axpvhPY7S275 SALOSQITMwfb8QE+zXWu56xkGRTd2jSqEAPCkEg0BgTW7Xg9QsZabsVnVdmH2tA/ np+Hso+ZzmjgNoRuBeNp03JtMCUDx3768V0hPM04lAOTCytW7WMso6JsvRvsWjcA LHpuXh4uQzqul291Au6bPUyy1bSI1B5nXLH0tv0EdguA7vu8upM0mOv9q5RGZkja NNgQteuf24IIuW8ThZ5sHVXdl5nBOO+mClk3iOQ+VH3tBg25dhs7SIAZtI+EFVEy QzuL9Z2JBg7Fh6uuHlO+QmfEmVI/gpJpnFuUzsH2Ldt2C6evO/kx1PG2MbhtNPRO TcVcUEa6cgIAg0zHpGfb =XRKl -----END PGP SIGNATURE----- --svExV93C05KqedWb--