From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c078T-0000SN-Bh for qemu-devel@nongnu.org; Fri, 28 Oct 2016 09:23:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c078P-00088m-C1 for qemu-devel@nongnu.org; Fri, 28 Oct 2016 09:23:53 -0400 Received: from 6.mo6.mail-out.ovh.net ([87.98.177.69]:36361) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c078P-00085u-5b for qemu-devel@nongnu.org; Fri, 28 Oct 2016 09:23:49 -0400 Received: from player786.ha.ovh.net (b7.ovh.net [213.186.33.57]) by mo6.mail-out.ovh.net (Postfix) with ESMTP id 93B7C568CF for ; Fri, 28 Oct 2016 15:23:47 +0200 (CEST) Date: Fri, 28 Oct 2016 15:23:40 +0200 From: Greg Kurz Message-ID: <20161028152340.0ffd7782@bahia> In-Reply-To: <20161028111412.GF19349@umbus.fritz.box> References: <1477620301-24822-1-git-send-email-david@gibson.dropbear.id.au> <20161028113243.0a4e92d4@bahia> <20161028111412.GF19349@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; boundary="Sig_/24Axc+7WxbsV_UyWK..4lfM"; protocol="application/pgp-signature" Subject: Re: [Qemu-devel] [RFC] powernv: CPU compatibility modes don't make sense for powernv List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: clg@kaod.org, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --Sig_/24Axc+7WxbsV_UyWK..4lfM Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Fri, 28 Oct 2016 22:14:12 +1100 David Gibson wrote: > On Fri, Oct 28, 2016 at 11:32:43AM +0200, Greg Kurz wrote: > > On Fri, 28 Oct 2016 13:05:01 +1100 > > David Gibson wrote: > > =20 > > > powernv has some code (derived from the spapr equivalent) used in dev= ice > > > tree generation which depends on the CPU's compatibility mode / logic= al > > > PVR. However, compatibility modes don't make sense on powernv - at l= east > > > not as a property controlled by the host - because the guest in power= nv > > > has full hypervisor level access to the virtual system, and so owns t= he > > > PCR (Processor Compatibility Register) which implements compatiblity = modes. > > > =20 > >=20 > > This makes a lot of sense indeed. > > =20 > > > Signed-off-by: David Gibson > > > --- > > > hw/ppc/pnv.c | 6 +----- > > > 1 file changed, 1 insertion(+), 5 deletions(-) > > >=20 > > > Hi C=C3=A9dric, I'd appreciate it if you can double check my reasoning > > > here. This patch gets powernv out of the way of a bunch of > > > compatibility mode cleanups I have in the works. > > >=20 > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > > > index 82276e0..6af3424 100644 > > > --- a/hw/ppc/pnv.c > > > +++ b/hw/ppc/pnv.c > > > @@ -110,7 +110,7 @@ static void powernv_create_core_node(PnvChip *chi= p, PnvCore *pc, void *fdt) > > > CPUState *cs =3D CPU(DEVICE(pc->threads)); > > > DeviceClass *dc =3D DEVICE_GET_CLASS(cs); > > > PowerPCCPU *cpu =3D POWERPC_CPU(cs); > > > - int smt_threads =3D ppc_get_compat_smt_threads(cpu); > > > + int smt_threads =3D CPU_CORE(pc)->nr_threads; =20 > >=20 > > Not speaking of the PCR, this has also another effect since=20 > > ppc_get_compat_smt_threads() also take the KVM smt capability > > into account: > >=20 > > int ret =3D MIN(cs->nr_threads, kvmppc_smt_threads()); > >=20 > > Shouldn't you do the same here ? =20 >=20 > Actually, no, though possible that needs mention in the commit > message. I'm pretty sure there are checks elsewhere that ensure > nr_threads <=3D kvmppc_smt_threads(). If not, having more virtual > threads than kvmppc allows certainly can't work, so we're no more > broken than before. >=20 Indeed, there's only one in ppc_cpu_realizefn() and I'm not sure all vCPUs need to perform this same check. But you're right, the check does not belong here either, it should probably be done in the machine init function. > Plus, kvmppc_smt_threads() is really only relevant for KVM HV which > doesn't, and can't support the powernv machine type anyway. >=20 kvmppc_smt_threads() is actually ill-named since it is used to know the number of threads supported by the CPU accelerator, which, I agree, is 1 for anything but KVM HV at the present time. Maybe it could change the day we support the emulation of multiple threads. So in the end, this patch looks the way to go. Reviewed-by: Greg Kurz --Sig_/24Axc+7WxbsV_UyWK..4lfM Content-Type: application/pgp-signature Content-Description: OpenPGP digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iEUEARECAAYFAlgTUVwACgkQAvw66wEB28Kf8gCYw5DVGAYOA/sQ9PHlUcLoUEvB SQCfZTB2K6Zug2xFEyzvb9iSc50SWX4= =f+hh -----END PGP SIGNATURE----- --Sig_/24Axc+7WxbsV_UyWK..4lfM--