From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56618) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLQf8-0005vc-0H for qemu-devel@nongnu.org; Fri, 08 Jul 2016 03:57:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLQf5-0008BX-L7 for qemu-devel@nongnu.org; Fri, 08 Jul 2016 03:57:24 -0400 Date: Fri, 8 Jul 2016 17:59:07 +1000 From: David Gibson Message-ID: <20160708075907.GS14675@voom.fritz.box> References: <1467903025-13383-1-git-send-email-bharata@linux.vnet.ibm.com> <1467903025-13383-4-git-send-email-bharata@linux.vnet.ibm.com> <20160707181131.3d9926fc@bahia.lan> <20160708052533.GN14675@voom.fritz.box> <20160708094647.6267415c@bahia.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="zGX6P/PxmcOG18lb" Content-Disposition: inline In-Reply-To: <20160708094647.6267415c@bahia.lan> Subject: Re: [Qemu-devel] [RFC PATCH v2 3/5] spapr: Set stable_cpu_id for threads of CPU cores List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Bharata B Rao , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, imammedo@redhat.com, nikunj@linux.vnet.ibm.com, pbonzini@redhat.com --zGX6P/PxmcOG18lb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 08, 2016 at 09:46:47AM +0200, Greg Kurz wrote: > On Fri, 8 Jul 2016 15:25:33 +1000 > David Gibson wrote: >=20 > > On Thu, Jul 07, 2016 at 06:11:31PM +0200, Greg Kurz wrote: > > > On Thu, 7 Jul 2016 20:20:23 +0530 > > > Bharata B Rao wrote: > > > =20 > > > > Conditonally set stable_cpu_id for CPU threads that are created as = part > > > > of spapr CPU cores. The use of stable_cpu_id is enabled for pseries= -2.7 > > > > onwards. > > > > =20 > > >=20 > > > The last sentence is a bit confusing since the enablement actually ha= ppens > > > in patch 5/5. > > > =20 > > > > Signed-off-by: Bharata B Rao > > > > --- > > > > hw/ppc/spapr_cpu_core.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > >=20 > > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > > > index b104778..0ec3513 100644 > > > > --- a/hw/ppc/spapr_cpu_core.c > > > > +++ b/hw/ppc/spapr_cpu_core.c > > > > @@ -293,8 +293,15 @@ static void spapr_cpu_core_realize(DeviceState= *dev, Error **errp) > > > > for (i =3D 0; i < cc->nr_threads; i++) { > > > > char id[32]; > > > > obj =3D sc->threads + i * size; > > > > + CPUState *cs; > > > > =20 > > > > object_initialize(obj, size, typename); > > > > + cs =3D CPU(obj); > > > > + > > > > + /* Use core_id (which is actually cpu_dt_id) as stable CPU= id */ =20 > > >=20 > > > It isn't what I had in mind. More something like below: > > >=20 > > > In ppc_spapr_init(): > > >=20 > > > for (i =3D 0; i < spapr_max_cores; i++) { > > > spapr->cores[i]->stable_id =3D i * smp_threads; > > > } > > >=20 > > >=20 > > > In spapr_cpu_core_realize(): > > >=20 > > > for (j =3D 0; j < cc->nr_threads; j++) { > > > stable_cpu_id =3D cc->stable_id + j; > > > } > > >=20 > > > So we need to introduce cc->stable_id. =20 > >=20 > > No, we don't. Cores have had a stable ID since they were introduced. > >=20 >=20 > I agree core_dt_id is stable but it is a DT concept. There is no core_dt_id. There's just core-id, which is machine assigned (via the query hotpluggable cpus interface) and stable. > static void ppc_spapr_init(MachineState *machine) > { > [...] > for (i =3D 0; i < spapr_max_cores; i++) { > int core_dt_id =3D i * smt; =2E.uh, ok, except for that poorly named variable. But that's because this is in the machine type, and it knows it's going to use the same ids to give to the core object and to put in the device tree. > [...] > object_property_set_int(core, core_dt_id, CPU_CORE_PROP_C= ORE_ID, > &error_fatal); >=20 > This patch produces stable_cpu_id in the [0...smt * smp_cores) range. I f= ind it > awkward it depends on the host setup. True. Possibly we should set these as i * (maximum plausible number of threads). The gotcha is that currently we're using the same "dt_id" to control KVM's cpu id and that in turn controls the SMT level. That's a poor interface on the kernel side (my bad), but we have to live with it now. However we could de-couple that KVM id from the core-id. It'd no doubt cause some complications with kvm-xics, but we can probably handle it. > I'm suggesting we introduce cc->stable_id to be able to compute a simple > stable_cpu_id in the range [0...max_cpus), like x86 and ARM. I really don't see what properties this is supposed to have that are different from the existing core-id. >=20 > > Instead we should be setting the thread stable ids based on the core > > stable id. > >=20 > > > I think stable_cpu_id is the prerequisite to compute both cpu_dt_id a= nd instance_id. > > >=20 > > > Makes sense ? > > > =20 > > > > + if (cs->has_stable_cpu_id) { > > > > + cs->stable_cpu_id =3D cc->core_id + i; > > > > + } > > > > snprintf(id, sizeof(id), "thread[%d]", i); > > > > object_property_add_child(OBJECT(sc), id, obj, &local_err); > > > > if (local_err) { =20 > > > =20 > >=20 >=20 --=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 --zGX6P/PxmcOG18lb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXf11LAAoJEGw4ysog2bOSwN4P/jKEOAEwIwic1Oe+gYh7thp4 wl/XaMY/FDsB2uA7PHYVVVZwKzt+rW3XCqnOeD5w1k4StX0at87Jy8rviYWrTzw1 9nS1hSLmkM8xAzRDzKYjrAHzSQLgckkSyF98+LVU8pRocXLBExVMak8VG6GaFl1O ly/5KtGk6mtfUiiL8vdH+Bv2cH88i74DlcOCxpQKE16ttMbrIAqyVYXIPKRxU1lr pzFaFQCZB1AlgBRLmiiHtEGvg3GyPU1qwwBMt/o1ZoCyGG+Wt6tFYwSFRKXcAVTA R1o84WXedvqsQ7Zpn3CbY+hd0SAcIObAt65Z9TZNqAP0PUhCZ/F84odlPonTdvG2 QKMQ40aU94w4amT12wFghQbMLv3oKrj/gxkPfd5GceNpfBR5jWv058+RJjU5Hm94 9RL9Gv8OWzUNX6h0JtmchLXfZJeMk+K9Ex1fk6L18ujsuRhSqlTUE11DTEctXPQ6 t7Rt9+pfZs4VA902vgtPVBt25wONkh/gkcpOi4Q9nvJAUjuU0UY4Z96LN398Gg5a kshLPwEGv7qjeypPQsPFWk/cuaVWnjN88GlE1nz2qbzR9G1oCd68INPrdg1uiedp 3GGgxnr0PSE0xTchsI50vtwPFm7Rt+tDwMZ1LYAOORr4jvQthl10ZGAmZr1efpQk B9ljVdPsbeqj6GmIrB2u =2ioQ -----END PGP SIGNATURE----- --zGX6P/PxmcOG18lb--