From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49807) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmLvF-0003R3-Sj for qemu-devel@nongnu.org; Tue, 20 Sep 2016 10:21:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmLvB-0003fu-HT for qemu-devel@nongnu.org; Tue, 20 Sep 2016 10:21:20 -0400 Date: Tue, 20 Sep 2016 23:57:18 +1000 From: David Gibson Message-ID: <20160920135718.GL20488@umbus> References: <1473943560-14846-1-git-send-email-clg@kaod.org> <1473943560-14846-4-git-send-email-clg@kaod.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="82U+nxpba3czjIX9" Content-Disposition: inline In-Reply-To: <1473943560-14846-4-git-send-email-clg@kaod.org> Subject: Re: [Qemu-devel] [PATCH v3 03/10] ppc/pnv: add a core mask to PnvChip 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 --82U+nxpba3czjIX9 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Sep 15, 2016 at 02:45:53PM +0200, C=E9dric Le Goater wrote: > This will be used to build real HW ids for the cores and enforce some > limits on the available cores per chip. >=20 > Signed-off-by: C=E9dric Le Goater > --- >=20 > Changes since v2 : >=20 > - added POWER9 support > - removed cores_max=20 > - introduces a pnv_chip_core_sanitize() helper to check the core > ids_mask and the maximum number of cores >=20 > hw/ppc/pnv.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++= ++++++ > include/hw/ppc/pnv.h | 4 ++++ > 2 files changed, 70 insertions(+) >=20 > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 2aa5be56c8dc..ec7dd6ac5ea1 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -226,11 +226,44 @@ static void ppc_powernv_init(MachineState *machine) > snprintf(chip_name, sizeof(chip_name), "chip[%d]", CHIP_HWID(i)); > object_property_add_child(OBJECT(pnv), chip_name, chip, &error_f= atal); > object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_fa= tal); > + object_property_set_int(chip, smp_cores, "nr-cores", &error_fata= l); > + /* > + * We could customize cores_mask for the chip here. May be > + * using a powernv machine property, like 'num-chips'. Let the > + * chip choose the default for now. > + */ > + object_property_set_int(chip, 0x0, "cores-mask", &error_fatal); If you're selecting the default, why set it at all? > object_property_set_bool(chip, true, "realized", &error_fatal); > } > g_free(chip_typename); > } > =20 > +/* Allowed core identifiers on a POWER8 Processor Chip : > + * > + * > + * EX1 - Venice only > + * EX2 - Venice only > + * EX3 - Venice only > + * EX4 > + * EX5 > + * EX6 > + * > + * EX9 - Venice only > + * EX10 - Venice only > + * EX11 - Venice only > + * EX12 > + * EX13 > + * EX14 > + * > + */ > +#define POWER8E_CORE_MASK (0x7070ull) > +#define POWER8_CORE_MASK (0x7e7eull) > + > +/* > + * POWER9 has 24 cores, ids starting at 0x20 > + */ > +#define POWER9_CORE_MASK (0xffffff00000000ull) > + > static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc =3D DEVICE_CLASS(klass); > @@ -239,6 +272,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *= klass, void *data) > k->cpu_model =3D "POWER8E"; > k->chip_type =3D PNV_CHIP_POWER8E; > k->chip_cfam_id =3D 0x221ef04980000000ull; /* P8 Murano DD2.1 */ > + k->cores_mask =3D POWER8E_CORE_MASK; > dc->desc =3D "PowerNV Chip POWER8E"; > } > =20 > @@ -257,6 +291,7 @@ static void pnv_chip_power8_class_init(ObjectClass *k= lass, void *data) > k->cpu_model =3D "POWER8"; > k->chip_type =3D PNV_CHIP_POWER8; > k->chip_cfam_id =3D 0x220ea04980000000ull; /* P8 Venice DD2.0 */ > + k->cores_mask =3D POWER8_CORE_MASK; > dc->desc =3D "PowerNV Chip POWER8"; > } > =20 > @@ -275,6 +310,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass= *klass, void *data) > k->cpu_model =3D "POWER8NVL"; > k->chip_type =3D PNV_CHIP_POWER8NVL; > k->chip_cfam_id =3D 0x120d304980000000ull; /* P8 Naples DD1.0 */ > + k->cores_mask =3D POWER8_CORE_MASK; > dc->desc =3D "PowerNV Chip POWER8NVL"; > } > =20 > @@ -293,6 +329,7 @@ static void pnv_chip_power9_class_init(ObjectClass *k= lass, void *data) > k->cpu_model =3D "POWER9"; > k->chip_type =3D PNV_CHIP_POWER9; > k->chip_cfam_id =3D 0x100d104980000000ull; /* P9 Nimbus DD1.0 */ > + k->cores_mask =3D POWER9_CORE_MASK; > dc->desc =3D "PowerNV Chip POWER9"; > } > =20 > @@ -303,11 +340,38 @@ static const TypeInfo pnv_chip_power9_info =3D { > .class_init =3D pnv_chip_power9_class_init, > }; > =20 > +static void pnv_chip_core_sanitize(PnvChip *chip) > +{ > + PnvChipClass *pcc =3D PNV_CHIP_GET_CLASS(chip); > + int cores_max =3D hweight_long(pcc->cores_mask); > + > + if (chip->nr_cores > cores_max) { > + error_report("warning: too many cores for chip ! Limiting to %d", > + cores_max); > + chip->nr_cores =3D cores_max; This is called from realize() which takes an errp argument. It would be better to pass that in and actually report the error up the chain here, rather than assuming a warning is the right answer. Also.. shouldn't you actually check nr_cores against the chip local mask instead of the class one? > + } > + > + /* no custom mask for this chip, let's use the default one from > + * the chip class */ > + if (!chip->cores_mask) { > + chip->cores_mask =3D pcc->cores_mask; > + } > + > + /* filter alien core ids ! some are reserved */ > + if ((chip->cores_mask & pcc->cores_mask) !=3D chip->cores_mask) { > + error_report("warning: invalid core mask for chip !"); Likewise here you should propagate the error. > + } > + chip->cores_mask &=3D pcc->cores_mask; > +} > + > static void pnv_chip_realize(DeviceState *dev, Error **errp) > { > PnvChip *chip =3D PNV_CHIP(dev); > PnvChipClass *pcc =3D PNV_CHIP_GET_CLASS(chip); > =20 > + /* Early checks on the core settings */ > + pnv_chip_core_sanitize(chip); > + > if (pcc->realize) { > pcc->realize(chip, errp); > } > @@ -315,6 +379,8 @@ static void pnv_chip_realize(DeviceState *dev, Error = **errp) > =20 > static Property pnv_chip_properties[] =3D { > DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0), > + DEFINE_PROP_UINT32("nr-cores", PnvChip, nr_cores, 1), > + DEFINE_PROP_UINT64("cores-mask", PnvChip, cores_mask, 0x0), > DEFINE_PROP_END_OF_LIST(), > }; > =20 > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index 6e6628edcf6a..cfc32586320f 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -42,6 +42,9 @@ typedef struct PnvChip { > =20 > /*< public >*/ > uint32_t chip_id; > + > + uint32_t nr_cores; > + uint64_t cores_mask; > } PnvChip; > =20 > typedef struct PnvChipClass { > @@ -52,6 +55,7 @@ typedef struct PnvChipClass { > const char *cpu_model; > PnvChipType chip_type; > uint64_t chip_cfam_id; > + uint64_t cores_mask; > =20 > void (*realize)(PnvChip *dev, Error **errp); > } PnvChipClass; --=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 --82U+nxpba3czjIX9 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX4UA+AAoJEGw4ysog2bOSFrsP/jZfDCSLL9RFHASkm/ISh9pM YSBPa661n1z+yykOYWgB3I7UtRTjx4ltRs2+VoLhNXBCn1cO743UIvhlFj/KF4Oz B9MaC6ZpQUUZ2MCcKJwQUNsNfgp0MIh877SejRRq34zogsGDjsIiBy6Lx+nMbhJQ wFZxHUgeYGhqF5VtYJDRmB5D2upEYQfYgfzfUntN5HogO3pmVs6ZEq3yGKgAftng 2+YNkRHkhf80aETRz1sHV9bliEbAKa13nj3veQQJ2XibRZiO8zDGa5tH7g7+oI9D W6dcTbmCGyjFufPblClTlmIE0CQO79R17ww0zk0VVlJSE7GINMp1R16DoIPUo6nO FCUux7LMzJWTrCHsG/kRXhDZycgEuHBLcJvQSeNmrN2U+MyIfLFHQWmkWtBRiVxU rmHjUJcOLP19PXDj5j3fhEQuTEEhKFsc8KZwOk+K0+Blrxu0VNXegsbPVmHEdzd2 uqE6z7zLMiHaBCHwtce3MaLH79hVpPUdJYF69dVzEWnxh5KPrFqyECol2sZ4sdUJ ODQ07IKPjR51nxeSx2wrB0eklo8B01hDVvmAAr5afpz/hT+mou9z9zclO6WBf/ut zhG0ihjWu/BZXnClV1cCe/AA+o4pbR4YZyNqwDFfmuCEieLYzNEAvoJlLfmH6Z1I imxvcjiWflK+d31olhoJ =vVw/ -----END PGP SIGNATURE----- --82U+nxpba3czjIX9--