From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46636) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bgkO3-00064k-3B for qemu-devel@nongnu.org; Sun, 04 Sep 2016 23:15:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bgkO0-0001f1-9m for qemu-devel@nongnu.org; Sun, 04 Sep 2016 23:15:54 -0400 Date: Mon, 5 Sep 2016 12:58:35 +1000 From: David Gibson Message-ID: <20160905025835.GF2587@voom.fritz.box> References: <1472661255-20160-1-git-send-email-clg@kaod.org> <1472661255-20160-3-git-send-email-clg@kaod.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="NGIwU0kFl1Z1A3An" Content-Disposition: inline In-Reply-To: <1472661255-20160-3-git-send-email-clg@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 --NGIwU0kFl1Z1A3An Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > Signed-off-by: C=E9dric Le Goater > --- >=20 > 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 >=20 > hw/ppc/pnv.c | 154 +++++++++++++++++++++++++++++++++++++++++++++= ++++++ > include/hw/ppc/pnv.h | 71 ++++++++++++++++++++++++ > 2 files changed, 225 insertions(+) >=20 > 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 *machine) > exit(1); > } > } > + > + /* Create the processor chips */ > + chip_typename =3D g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_= 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_ab= ort); > + object_property_set_bool(chip, true, "realized", &error_abort); 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. > + pnv->chips[i] =3D PNV_CHIP(chip); > + } > + g_free(chip_typename); > +} > + > +static void pnv_chip_power8nvl_realize(PnvChip *chip, Error **errp) > +{ > + ; > +} I don't think you should need to define an empty realize function. Or is this just a placeholder for things future patches will add? > + > +static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > +{ > + 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; With the new chip class per cpu class, does this chip_type field serve any purpose any more? > + k->chip_f000f =3D 0x120d304980000000ull; A comment somewhere explaining what this cryptic value is would be nice. > + dc->desc =3D "PowerNV Chip POWER8NVL"; > +} > + > +static const TypeInfo pnv_chip_power8nvl_info =3D { > + .name =3D TYPE_PNV_CHIP_POWER8NVL, > + .parent =3D TYPE_PNV_CHIP, > + .instance_size =3D sizeof(PnvChipPower8NVL), > + .class_init =3D pnv_chip_power8nvl_class_init, > +}; > + > +static void pnv_chip_power8_realize(PnvChip *chip, Error **errp) > +{ > + ; > +} > + > +static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + PnvChipClass *k =3D PNV_CHIP_CLASS(klass); > + > + k->realize =3D pnv_chip_power8_realize; > + k->cpu_model =3D "POWER8"; > + k->chip_type =3D PNV_CHIP_P8; > + k->chip_f000f =3D 0x220ea04980000000ull; > + dc->desc =3D "PowerNV Chip POWER8"; > +} It might be worth using some macros to compactify the definition of each of the chip variants. > + > +static const TypeInfo pnv_chip_power8_info =3D { > + .name =3D TYPE_PNV_CHIP_POWER8, > + .parent =3D TYPE_PNV_CHIP, > + .instance_size =3D sizeof(PnvChipPower8), > + .class_init =3D pnv_chip_power8_class_init, > +}; > + > +static void pnv_chip_power8e_realize(PnvChip *chip, Error **errp) > +{ > + ; > +} > + > +static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + PnvChipClass *k =3D PNV_CHIP_CLASS(klass); > + > + k->realize =3D pnv_chip_power8e_realize; > + k->cpu_model =3D "POWER8E"; > + k->chip_type =3D PNV_CHIP_P8E; > + k->chip_f000f =3D 0x221ef04980000000ull; > + dc->desc =3D "PowerNV Chip POWER8E"; > +} > + > +static const TypeInfo pnv_chip_power8e_info =3D { > + .name =3D TYPE_PNV_CHIP_POWER8E, > + .parent =3D TYPE_PNV_CHIP, > + .instance_size =3D sizeof(PnvChipPower8e), > + .class_init =3D pnv_chip_power8e_class_init, > +}; > + > +static void pnv_chip_realize(DeviceState *dev, Error **errp) > +{ > + PnvChip *chip =3D PNV_CHIP(dev); > + PnvChipClass *pcc =3D PNV_CHIP_GET_CLASS(chip); > + > + pcc->realize(chip, errp); > +} > + > +static Property pnv_chip_properties[] =3D { > + DEFINE_PROP_UINT32("chip-id", PnvChip, chip_id, 0), > + DEFINE_PROP_END_OF_LIST(), > +}; > + > +static void pnv_chip_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + > + dc->realize =3D pnv_chip_realize; > + dc->props =3D pnv_chip_properties; > + dc->desc =3D "PowerNV Chip"; > + } > + > +static const TypeInfo pnv_chip_info =3D { > + .name =3D TYPE_PNV_CHIP, > + .parent =3D TYPE_SYS_BUS_DEVICE, > + .class_init =3D pnv_chip_class_init, > + .class_size =3D sizeof(PnvChipClass), > + .abstract =3D true, > +}; > + > +static char *pnv_get_num_chips(Object *obj, Error **errp) > +{ > + return g_strdup_printf("%d", POWERNV_MACHINE(obj)->num_chips); > +} > + > +static void pnv_set_num_chips(Object *obj, const char *value, Error **er= rp) > +{ > + PnvMachineState *pnv =3D POWERNV_MACHINE(obj); > + int num_chips; > + > + if (sscanf(value, "%d", &num_chips) !=3D 1) { > + error_setg(errp, "invalid num_chips property: '%s'", value); > + } > + > + /* > + * FIXME: should we decide on how many chips we can create based > + * on #cores and Venice vs. Murano vs. Naples chip type etc..., > + */ > + pnv->num_chips =3D num_chips; > +} > + > +static void powernv_machine_initfn(Object *obj) > +{ > + PnvMachineState *pnv =3D POWERNV_MACHINE(obj); > + pnv->num_chips =3D 1; > + > + object_property_add_str(obj, "num-chips", pnv_get_num_chips, > + pnv_set_num_chips, NULL); > + object_property_set_description(obj, "num-chips", > + "Specifies the number of processor c= hips", > + NULL); > } > =20 > static void powernv_machine_class_init(ObjectClass *oc, void *data) > @@ -233,12 +382,17 @@ static const TypeInfo powernv_machine_info =3D { > .name =3D TYPE_POWERNV_MACHINE, > .parent =3D TYPE_MACHINE, > .instance_size =3D sizeof(PnvMachineState), > + .instance_init =3D powernv_machine_initfn, > .class_init =3D powernv_machine_class_init, > }; > =20 > static void powernv_machine_register_types(void) > { > type_register_static(&powernv_machine_info); > + type_register_static(&pnv_chip_info); > + type_register_static(&pnv_chip_power8e_info); > + type_register_static(&pnv_chip_power8_info); > + type_register_static(&pnv_chip_power8nvl_info); > } > =20 > type_init(powernv_machine_register_types) > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index 31a57ed7f465..1f32573dedff 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -20,6 +20,74 @@ > #define _PPC_PNV_H > =20 > #include "hw/boards.h" > +#include "hw/sysbus.h" > + > +#define TYPE_PNV_CHIP "powernv-chip" > +#define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP) > +#define PNV_CHIP_CLASS(klass) \ > + OBJECT_CLASS_CHECK(PnvChipClass, (klass), TYPE_PNV_CHIP) > +#define PNV_CHIP_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(PnvChipClass, (obj), TYPE_PNV_CHIP) > + > +typedef enum PnvChipType { > + PNV_CHIP_P8E, /* AKA Murano (default) */ > + PNV_CHIP_P8, /* AKA Venice */ > + PNV_CHIP_P8NVL, /* AKA Naples */ > +} PnvChipType; > + > +typedef struct PnvChip { > + /*< private >*/ > + SysBusDevice parent_obj; > + > + /*< public >*/ > + uint32_t chip_id; > +} PnvChip; > + > +typedef struct PnvChipClass { > + /*< private >*/ > + SysBusDeviceClass parent_class; > + /*< public >*/ > + const char *cpu_model; > + PnvChipType chip_type; > + uint64_t chip_f000f; > + > + void (*realize)(PnvChip *dev, Error **errp); > +} PnvChipClass; > + > +#define TYPE_PNV_CHIP "powernv-chip" > + > +#define TYPE_PNV_CHIP_POWER8E "powernv-chip-POWER8E" > +#define PNV_CHIP_POWER8E(obj) \ > + OBJECT_CHECK(PnvChipPower8e, (obj), TYPE_PNV_CHIP_POWER8E) > + > +typedef struct PnvChipPower8e { > + PnvChip pnv_chip; > +} PnvChipPower8e; > + > +#define TYPE_PNV_CHIP_POWER8 "powernv-chip-POWER8" > +#define PNV_CHIP_POWER8(obj) \ > + OBJECT_CHECK(PnvChipPower8, (obj), TYPE_PNV_CHIP_POWER8) > + > +typedef struct PnvChipPower8 { > + PnvChip pnv_chip; > +} PnvChipPower8; > + > +#define TYPE_PNV_CHIP_POWER8NVL "powernv-chip-POWER8NVL" > +#define PNV_CHIP_POWER8NVL(obj) \ > + OBJECT_CHECK(PnvChipPower8NVL, (obj), TYPE_PNV_CHIP_POWER8NVL) > + > +typedef struct PnvChipPower8NVL { > + PnvChip pnv_chip; > +} PnvChipPower8NVL; > + > +/* > + * This generates a HW chip id depending on an index: > + * > + * 0x0, 0x1, 0x10, 0x11, 0x20, 0x21, ... > + * > + * Is this correct ? > + */ > +#define CHIP_HWID(i) ((((i) & 0x3e) << 3) | ((i) & 0x1)) > =20 > #define TYPE_POWERNV_MACHINE MACHINE_TYPE_NAME("powernv") > #define POWERNV_MACHINE(obj) \ > @@ -32,6 +100,9 @@ typedef struct PnvMachineState { > uint32_t initrd_base; > long initrd_size; > hwaddr fdt_addr; > + > + uint32_t num_chips; > + PnvChip **chips; > } PnvMachineState; > =20 > #endif /* _PPC_PNV_H */ --=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 --NGIwU0kFl1Z1A3An Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJXzN9aAAoJEGw4ysog2bOSKKoQAMtvas58g0XoYHQE9g08FHyG 8ODKPY+CSZhS+3GU0VoXBPyoOG/6WAqb8ZLG9cR1APObuy2jaUreUxmnhYPvPD1E Li/4oZKAYa6YEw3Trton1OmMqeUv+YP5U46PmsuAacl3c+8ay95YHPprh0e6PTNq FD8y3m1gisQ7MBJc+P8zw8SlqjgQtsnYAbPvHV+p25OOaLqj26K+RF8S7lVzSoUg dw+7tjaJg84ofQIn/q2aeUCwHuDPluMLQpIcksmKNviACzKKofakpinJlZCRaBgd G0qjcA+8gZdyahiVOqbn9O3FlXCJGGmPzUdBvp91o6c636iSBjCg5KV/1iehsKgy j9m0nmk2NDQIKQaGQnQfcapcKX3G1BgCqOPp0IzJBeK6BOkZmL/SLdRHlAB9jq1t +dqbt26Ms2g+ceobW+Q0Bhm9wHeXj5+3ij3CFK1WFWztQ3UuvgQyGl1rrXDYejhc 9TmBEiHoLfgraV3xu4s9aY8EDTUkQfO0VIpyR50NJLw8+lclsguyeuKkRcJbfocU ZJNVmxTYVXiPWgK0e+Zk3b21QRQX8C0wg7ZYRcjthwNZkHK/TU679oUoEDpi5gXg uEN03Zo+ZBoqRJ/+NtoD0Ukyxti8p2RNWn6gnO7HZ0Q+O4vrS3kCEpl+OBf66Scf 7j4aAnC7L0TWSTg/qXlv =JzDD -----END PGP SIGNATURE----- --NGIwU0kFl1Z1A3An--