From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49949) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmLvk-0003vX-PL for qemu-devel@nongnu.org; Tue, 20 Sep 2016 10:22:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmLvf-00040C-76 for qemu-devel@nongnu.org; Tue, 20 Sep 2016 10:21:51 -0400 Date: Tue, 20 Sep 2016 23:50:14 +1000 From: David Gibson Message-ID: <20160920135014.GK20488@umbus> References: <1473943560-14846-1-git-send-email-clg@kaod.org> <1473943560-14846-3-git-send-email-clg@kaod.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Gnkm4uF329L9IN0W" Content-Disposition: inline In-Reply-To: <1473943560-14846-3-git-send-email-clg@kaod.org> Subject: Re: [Qemu-devel] [PATCH v3 02/10] 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 --Gnkm4uF329L9IN0W Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Sep 15, 2016 at 02:45:52PM +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 inits 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 > The Chip CFAM (Common FRU Access Module) ID gives the model of the > chip and its version number. It is generally the first thing firmwares > fetch, available at XSCOM PCB address 0xf000f, to start initialization. Thanks for the above - that's basically exactly the sort of description I was looking for. > Signed-off-by: C=E9dric Le Goater > --- >=20 > chip_type could possibly be removed or calculated from the attribute > chip_cfam_id. Let's keep it for now and see how the patchset evolves. > Maybe this object deserves its own file hw/ppc/pnv_chip.c ?=20 >=20 > Changes since v2: >=20 > - forced a POWER8 cpu model if none is specified and check that a > PnvChip type exist for it > - did some renaming to be consistent with the cpu model names > - added POWER9 chip > - removed empty realize op > - renamed atribute chip_f000f in chip_cfam_id > - used error_fatal instead of error_abort when setting the chip > properties > - introduced a powernv_populate_chip() routine >=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 > hw/ppc/pnv.c | 192 +++++++++++++++++++++++++++++++++++++++++++++= ++++-- > include/hw/ppc/pnv.h | 79 +++++++++++++++++++++ > 2 files changed, 266 insertions(+), 5 deletions(-) >=20 > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index ee78422b2eae..2aa5be56c8dc 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -74,6 +74,16 @@ static void powernv_populate_memory_node(void *fdt, in= t chip_id, hwaddr start, > _FDT((fdt_setprop_cell(fdt, off, "ibm,chip-id", chip_id))); > } > =20 > +static void powernv_populate_chip(PnvChip *chip, void *fdt) > +{ > + /* Put all the memory in one node on chip 0 until we find a way to > + * specify different ranges for each chip > + */ > + if (chip->chip_id =3D=3D 0) { > + powernv_populate_memory_node(fdt, chip->chip_id, 0, ram_size); > + } > +} > + > static void *powernv_create_fdt(PnvMachineState *pnv, > const char *kernel_cmdline) > { > @@ -81,6 +91,7 @@ static void *powernv_create_fdt(PnvMachineState *pnv, > char *buf; > const char plat_compat[] =3D "qemu,powernv\0ibm,powernv"; > int off; > + int i; > =20 > fdt =3D g_malloc0(FDT_MAX_SIZE); > _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE))); > @@ -117,11 +128,10 @@ static void *powernv_create_fdt(PnvMachineState *pn= v, > &end_prop, sizeof(end_prop)))); > } > =20 > - /* Put all the memory in one node on chip 0 until we find a way to > - * specify different ranges for each chip > - */ > - powernv_populate_memory_node(fdt, 0, 0, ram_size); > - > + /* Populate device tree for each chip */ > + for (i =3D 0; i < pnv->num_chips; i++) { > + powernv_populate_chip(pnv->chips[i], fdt); > + } > return fdt; > } > =20 > @@ -146,6 +156,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)) { > @@ -190,6 +202,170 @@ static void ppc_powernv_init(MachineState *machine) > exit(1); > } > } > + > + /* We need some cpu model to instantiate the PnvChip class */ > + if (machine->cpu_model =3D=3D NULL) { > + machine->cpu_model =3D "POWER8"; > + } > + > + /* Create the processor chips */ > + chip_typename =3D g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_= model); > + if (!object_class_by_name(chip_typename)) { > + error_report("qemu: invalid CPU model '%s' for %s machine", > + machine->cpu_model, MACHINE_GET_CLASS(machine)->nam= e); > + exit(1); > + } > + > + pnv->chips =3D g_new0(PnvChip *, pnv->num_chips); > + for (i =3D 0; i < pnv->num_chips; i++) { > + char chip_name[32]; > + Object *chip =3D object_new(chip_typename); > + > + pnv->chips[i] =3D PNV_CHIP(chip); > + > + 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_bool(chip, true, "realized", &error_fatal); > + } > + g_free(chip_typename); > +} > + > +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->cpu_model =3D "POWER8E"; > + k->chip_type =3D PNV_CHIP_POWER8E; > + k->chip_cfam_id =3D 0x221ef04980000000ull; /* P8 Murano DD2.1 */ > + 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_power8_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + PnvChipClass *k =3D PNV_CHIP_CLASS(klass); > + > + k->cpu_model =3D "POWER8"; > + k->chip_type =3D PNV_CHIP_POWER8; > + k->chip_cfam_id =3D 0x220ea04980000000ull; /* P8 Venice DD2.0 */ > + dc->desc =3D "PowerNV Chip POWER8"; > +} > + > +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_power8nvl_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + PnvChipClass *k =3D PNV_CHIP_CLASS(klass); > + > + k->cpu_model =3D "POWER8NVL"; > + k->chip_type =3D PNV_CHIP_POWER8NVL; > + k->chip_cfam_id =3D 0x120d304980000000ull; /* P8 Naples DD1.0 */ > + 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_power9_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc =3D DEVICE_CLASS(klass); > + PnvChipClass *k =3D PNV_CHIP_CLASS(klass); > + > + k->cpu_model =3D "POWER9"; > + k->chip_type =3D PNV_CHIP_POWER9; > + k->chip_cfam_id =3D 0x100d104980000000ull; /* P9 Nimbus DD1.0 */ > + dc->desc =3D "PowerNV Chip POWER9"; > +} > + > +static const TypeInfo pnv_chip_power9_info =3D { > + .name =3D TYPE_PNV_CHIP_POWER9, > + .parent =3D TYPE_PNV_CHIP, > + .instance_size =3D sizeof(PnvChipPower9), > + .class_init =3D pnv_chip_power9_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); > + > + if (pcc->realize) { > + pcc->realize(chip, errp); Is there actually a need for this PnvChipClass::realize callback? Couldn't the class_inits just direclty set DeviceClass::realize? > + } > +} > + > +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); > + } As a rule, I'd recommend strtol() instead of sscanf() - less variations in the semantics by platform. > + > + /* > + * 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); Although come to that, why not use object_property_add_uint32_ptr() or similar instead of property_add_str()? > + 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) > @@ -211,12 +387,18 @@ 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); > + type_register_static(&pnv_chip_power9_info); > } > =20 > type_init(powernv_machine_register_types) > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index c8a73bc74267..6e6628edcf6a 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -20,6 +20,82 @@ > #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_POWER8E, /* AKA Murano (default) */ > + PNV_CHIP_POWER8, /* AKA Venice */ > + PNV_CHIP_POWER8NVL, /* AKA Naples */ > + PNV_CHIP_POWER9, /* AKA Nimbus */ > +} 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_cfam_id; > + > + void (*realize)(PnvChip *dev, Error **errp); > +} PnvChipClass; > + > +#define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-POWER8E" > +#define PNV_CHIP_POWER8E(obj) \ > + OBJECT_CHECK(PnvChipPower8E, (obj), TYPE_PNV_CHIP_POWER8E) > + > +typedef struct PnvChipPower8E { > + PnvChip pnv_chip; > +} PnvChipPower8E; If you're not adding any fields you don't need to define an actual type for the subclass. And if you *are* defining a type, then you need to make sure you set the instance size in the type definition to match it. I suspect you're going to want P8 family and P9 family intermediate classes fairly early on. > + > +#define TYPE_PNV_CHIP_POWER8 TYPE_PNV_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 TYPE_PNV_CHIP "-POWER8NVL" > +#define PNV_CHIP_POWER8NVL(obj) \ > + OBJECT_CHECK(PnvChipPower8NVL, (obj), TYPE_PNV_CHIP_POWER8NVL) > + > +typedef struct PnvChipPower8NVL { > + PnvChip pnv_chip; > +} PnvChipPower8NVL; > + > +#define TYPE_PNV_CHIP_POWER9 TYPE_PNV_CHIP "-POWER9" > +#define PNV_CHIP_POWER9(obj) \ > + OBJECT_CHECK(PnvChipPower9, (obj), TYPE_PNV_CHIP_POWER9) > + > +typedef struct PnvChipPower9 { > + PnvChip pnv_chip; > +} PnvChipPower9; > + > +/* > + * 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) \ > @@ -31,6 +107,9 @@ typedef struct PnvMachineState { > =20 > uint32_t initrd_base; > long initrd_size; > + > + uint32_t num_chips; > + PnvChip **chips; > } PnvMachineState; > =20 > #define POWERNV_FDT_ADDR 0x01000000 --=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 --Gnkm4uF329L9IN0W Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX4T6TAAoJEGw4ysog2bOSHUEQANQ8oPLe27EpYZ4ZutPNWTcR j0ilbP5eTrEmk+QVO11j4rKCTP1fdDHjyIrq2ey3qYKgS4aAfvmgvJuSy0sRPTb+ eJat31Bmi7r3wlGdOSWOQ3UwEyf/8QTkuTLECc26swWgPYBz5+MTC2OavDoRwzmN 3hyzT6c7khOmBLTMLv1tn7t+dnPVRgfl+MdnPRmPAtzRvJwkuAphK3TIobFFjQf1 f/QG4TDvuXAOvbczKleYRlP+cfseXAr24YT3Z36pMhJLez/mekloWUfpUehiJ928 p+NLN6PxKDn5Oa3CnBJrsb+IOIe3M4uU5jZFBXGAuvgkZi8E6Gek5AZBOpFQdVAR 9EyHQoOCGY69j1mb2xBXdj2cNwQcEXw4Tt3Bu6wATfeLSzBOJZ1Zxyc3EDNAKYVJ CK4vC/ckskxti4VCjLD5VVMClhvyZXs0eqVG9kiNgq6yeAkMyPP0gbzlSWPeHUQw PU6/p5z2Fvs4n/OMJmrOGdgW5U1EM7DPjRT+u4sGBWmDUCJQuTG9LG0/QKjtUn7Z V/jmmCcxkAzt2fIawgfs0X6gMKJTFx84doGskZcaxWbex5JwJ8dKyApvHW8QAbIi wifKaHmo2d9jdhJEvUuwZvThddWMEmx4ZfhkrFcyGzqZbWFXB1en64hTrPAqnXi3 LGRXIh265/Mwfsh4tjkg =b488 -----END PGP SIGNATURE----- --Gnkm4uF329L9IN0W--