From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51525) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bh4cN-0002IL-90 for qemu-devel@nongnu.org; Mon, 05 Sep 2016 20:52:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bh4cI-0003al-7y for qemu-devel@nongnu.org; Mon, 05 Sep 2016 20:52:02 -0400 Date: Tue, 6 Sep 2016 10:52:26 +1000 From: David Gibson Message-ID: <20160906005226.GE2900@voom.fritz.box> References: <1472661255-20160-1-git-send-email-clg@kaod.org> <1472661255-20160-3-git-send-email-clg@kaod.org> <20160905025835.GF2587@voom.fritz.box> <5ba2f048-72bb-85b9-31a4-462c99e05386@kaod.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="GxcwvYAGnODwn7V8" Content-Disposition: inline In-Reply-To: <5ba2f048-72bb-85b9-31a4-462c99e05386@kaod.org> Subject: Re: [Qemu-devel] [PATCH v2 2/7] ppc/pnv: add a PnvChip object List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: qemu-ppc@nongnu.org, Benjamin Herrenschmidt , qemu-devel@nongnu.org, Alexander Graf --GxcwvYAGnODwn7V8 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 05, 2016 at 09:56:03AM +0200, C=E9dric Le Goater wrote: > On 09/05/2016 04:58 AM, David Gibson wrote: > > On Wed, Aug 31, 2016 at 06:34:10PM +0200, C=E9dric Le Goater wrote: > >> This is is an abstraction of a POWER8 chip which is a set of cores > >> plus other 'units', like the pervasive unit, the interrupt controller, > >> the memory controller, the on-chip microcontroller, etc. The whole can > >> be seen as a socket. It depends on a cpu model and its characteristics, > >> max cores, specific init are defined in a PnvChipClass. > >> > >> We start with an near empty PnvChip with only a few cpu constants > >> which we will grow in the subsequent patches with the controllers > >> required to run the system. > >> > >> Signed-off-by: C=E9dric Le Goater > >> --- > >> > >> Changes since v1: > >> =20 > >> - introduced a PnvChipClass depending on the cpu model. It also > >> provides some chip constants used by devices, like the cpu model hw > >> id (f000f), a enum type (not sure this is useful yet), a custom > >> realize ops for customization. > >> - the num-chips property can be configured on the command line. > >> =20 > >> Maybe this object deserves its own file hw/ppc/pnv_chip.c ?=20 > >> > >> hw/ppc/pnv.c | 154 ++++++++++++++++++++++++++++++++++++++++++= +++++++++ > >> include/hw/ppc/pnv.h | 71 ++++++++++++++++++++++++ > >> 2 files changed, 225 insertions(+) > >> > >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > >> index 70413e3c5740..06051268e200 100644 > >> --- a/hw/ppc/pnv.c > >> +++ b/hw/ppc/pnv.c > >> @@ -168,6 +168,8 @@ static void ppc_powernv_init(MachineState *machine) > >> char *fw_filename; > >> long fw_size; > >> long kernel_size; > >> + int i; > >> + char *chip_typename; > >> =20 > >> /* allocate RAM */ > >> if (ram_size < (1 * G_BYTE)) { > >> @@ -212,6 +214,153 @@ static void ppc_powernv_init(MachineState *machi= ne) > >> exit(1); > >> } > >> } > >> + > >> + /* Create the processor chips */ > >> + chip_typename =3D g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->c= pu_model); > >> + > >> + pnv->chips =3D g_new0(PnvChip *, pnv->num_chips); > >> + for (i =3D 0; i < pnv->num_chips; i++) { > >> + Object *chip =3D object_new(chip_typename); > >> + object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error= _abort); > >> + object_property_set_bool(chip, true, "realized", &error_abort= ); > >=20 > > I'm guessing these could fail due to bad user supplied parameters, not > > just internal bugs. In which case this should be &error_fatal rather > > than &error_abort. >=20 > yes. >=20 > >=20 > >> + pnv->chips[i] =3D PNV_CHIP(chip); > >> + } > >> + g_free(chip_typename); > >> +} > >> + > >> +static void pnv_chip_power8nvl_realize(PnvChip *chip, Error **errp) > >> +{ > >> + ; > >> +} > >=20 > > I don't think you should need to define an empty realize function. =20 > > Or is this just a placeholder for things future patches will add? >=20 > yes that was the plan but maybe this is a little early. chip_type > proved to be useful enough for the moment in the full patchset=20 > I maintain. >=20 > P9 will use a xive object when available and not a xics. I think=20 > this is when the real big difference will show up. So I should=20 > make realize() optional. >=20 > >> + > >> +static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *d= ata) > >> +{ > >> + DeviceClass *dc =3D DEVICE_CLASS(klass); > >> + PnvChipClass *k =3D PNV_CHIP_CLASS(klass); > >> + > >> + k->realize =3D pnv_chip_power8nvl_realize; > >> + k->cpu_model =3D "POWER8NVL"; > >> + k->chip_type =3D PNV_CHIP_P8NVL; > >=20 > > With the new chip class per cpu class, does this chip_type field serve > > any purpose any more? >=20 > It does look a bit redundant, I agree. But it is useful for xscom=20 > address translation (P9 is a little different), for xscom devices=20 > in general, for core id to pir conversion. It also does for lpc_irq=20 > support, which applies to P8NVL (and upper I suppose).=20 >=20 > Let's keep it for the moment as it serves its purpose, which is=20 > to handle small differences without too much cost. If this is=20 > going too far, I will propose a set of ops per chip type. Ok, fair enough. --=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 --GxcwvYAGnODwn7V8 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJXzhNKAAoJEGw4ysog2bOScfEP+QF9GeHydRqBpKPAueP8Le4O fRMgz7RzB1YtXRq24pqbcpxgyvzKUJMIURoOOV0uwaWNFfBnGGPC6Rxw7wHRfiAX TIyqyxJABNe/ePMfjtE5w1NEYRixZDF9HjVAlyrKsQZ41263yLK9YZ0A7cGVu9g+ BhXRJsMEk+FWQGJFYZeJFoA84/QuyQc0KZY9ZH9sQfE068gyiG6/PyhxZXd4WyQ9 rLe16m9RHmsylt4XA09gvx8u6FFqbwcTK934NfWqC9Kvb2n+AlBCOMDtg3oRihxE PcDSZoiEV7Ie1Nb5lerUpq8rmT3Kn/PP+vRox1pdUwrzcAMkZ0lEobprX26zQdx1 Hce5u1SMUNteDwpcW+CT7SopuSTWAtFMY2PM7V7R5x7beNIlcK3M6F9kcr/OeH88 Su8Gcg3NtP0GyxsUBZmUJcdZO39WmVhLRduOAYrStFAvXHt78RFaZXz+v+hN7A+D greYf4IW4ZyY/DHicIbwLxA49L2dbpW87abkZm3PzS8Dhw5C1ysNbw2EFsoFGjji bbuwT/VYhXoMbwV8NoO/HDEo3nzVi831fuY0nWT29FM9hzObKHexhdBp63nWKje1 eyW1YBqcoiPiF0m0Zs3Gh5en5hwHku7sbznn22iZy40c73MpJDHy+R0J7ZoA2wVW OJ5PmvGdTnrH7WdfoIIe =bxED -----END PGP SIGNATURE----- --GxcwvYAGnODwn7V8--