From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36997) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmcD2-0007Bu-DY for qemu-devel@nongnu.org; Wed, 21 Sep 2016 03:44:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmcCx-0007IT-7m for qemu-devel@nongnu.org; Wed, 21 Sep 2016 03:44:47 -0400 Received: from 1.mo69.mail-out.ovh.net ([178.33.251.173]:53161) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmcCw-0007Hw-RM for qemu-devel@nongnu.org; Wed, 21 Sep 2016 03:44:43 -0400 Received: from player779.ha.ovh.net (b9.ovh.net [213.186.33.59]) by mo69.mail-out.ovh.net (Postfix) with ESMTP id 2A2DCFFBBCA for ; Wed, 21 Sep 2016 09:44:40 +0200 (CEST) References: <1473943560-14846-1-git-send-email-clg@kaod.org> <1473943560-14846-3-git-send-email-clg@kaod.org> <20160920135014.GK20488@umbus> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <74f0ea55-e77d-e9c2-d61b-79fb64ad589b@kaod.org> Date: Wed, 21 Sep 2016 09:44:36 +0200 MIME-Version: 1.0 In-Reply-To: <20160920135014.GK20488@umbus> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable 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: David Gibson Cc: qemu-ppc@nongnu.org, Benjamin Herrenschmidt , qemu-devel@nongnu.org On 09/20/2016 03:50 PM, David Gibson wrote: > 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. >> >> 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. >> >> 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= . >=20 > Thanks for the above - that's basically exactly the sort of > description I was looking for. >=20 >> Signed-off-by: C=E9dric Le Goater >> --- >> >> 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 >> >> Changes since v2: >> >> - 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 >> >> 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(-) >> >> 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,= int 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 t= o >> + * 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 = *pnv, >> &end_prop, sizeof(end_prop)))); >> } >> =20 >> - /* Put all the memory in one node on chip 0 until we find a way t= o >> - * 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 *machi= ne) >> 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->c= pu_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)->= name); >> + 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, &erro= r_fatal); >> + object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error= _fatal); >> + object_property_set_bool(chip, true, "realized", &error_fatal= ); >> + } >> + g_free(chip_typename); >> +} >> + >> +static void pnv_chip_power8e_class_init(ObjectClass *klass, void *dat= a) >> +{ >> + 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 *d= ata) >> +{ >> + 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); >=20 > Is there actually a need for this PnvChipClass::realize callback? There is no real need for the moment. I guess I will remove it in v4, we can add it back later if required. > Couldn't the class_inits just direclty set DeviceClass::realize? >=20 >> + } >> +} >> + >> +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 *= *errp) >> +{ >> + 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); >> + } >=20 > As a rule, I'd recommend strtol() instead of sscanf() - less > variations in the semantics by platform. ok. >> + >> + /* >> + * 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); >=20 > Although come to that, why not use object_property_add_uint32_ptr() or > similar instead of property_add_str()? I think I tried that already and got trapped by the fact that pnv->num_ch= ips=20 needed to be static. I will take a closer look because it makes sense. >> + object_property_set_description(obj, "num-chips", >> + "Specifies the number of processo= r chips", >> + 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; >=20 > If you're not adding any fields you don't need to define an actual > type for the subclass. this is true. I was planning to add fields but the need did not arise, I will remove the PnvChipPower* types. We can readd them when needed, possibly when we introduce a really incompatible device model that=20 needs more than an PnvChipClass to handle differences. > 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. yes. I got a bad failure because of that recently. I will recheck all of it after the above removal. > I suspect you're going to want P8 family and P9 family intermediate > classes fairly early on. yes. the core definitions depends on it.=20 Thanks, C. >> + >> +#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