| Message ID | 1472661255-20160-3-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Wed, 31 Aug 2016 18:34:10 +0200 Cédric Le Goater <clg@kaod.org> 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édric Le Goater <clg@kaod.org> > --- > > Changes since v1: > > - 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. > > Maybe this object deserves its own file hw/ppc/pnv_chip.c ? > > 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; > > /* 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 = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); > + You should check that the CPU model is supported by this machine (with object_class_by_name()) and error out if it is not, or... > + pnv->chips = g_new0(PnvChip *, pnv->num_chips); > + for (i = 0; i < pnv->num_chips; i++) { > + Object *chip = object_new(chip_typename); ... this call to object_new() will abort. > + object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_abort); > + object_property_set_bool(chip, true, "realized", &error_abort); > + pnv->chips[i] = PNV_CHIP(chip); > + } > + g_free(chip_typename); > +} > + > +static void pnv_chip_power8nvl_realize(PnvChip *chip, Error **errp) > +{ > + ; > +} > + > +static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > +{ > + DeviceClass *dc = DEVICE_CLASS(klass); > + PnvChipClass *k = PNV_CHIP_CLASS(klass); > + > + k->realize = pnv_chip_power8nvl_realize; > + k->cpu_model = "POWER8NVL"; > + k->chip_type = PNV_CHIP_P8NVL; > + k->chip_f000f = 0x120d304980000000ull; > + dc->desc = "PowerNV Chip POWER8NVL"; > +} > + > +static const TypeInfo pnv_chip_power8nvl_info = { > + .name = TYPE_PNV_CHIP_POWER8NVL, > + .parent = TYPE_PNV_CHIP, > + .instance_size = sizeof(PnvChipPower8NVL), > + .class_init = 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 = DEVICE_CLASS(klass); > + PnvChipClass *k = PNV_CHIP_CLASS(klass); > + > + k->realize = pnv_chip_power8_realize; > + k->cpu_model = "POWER8"; > + k->chip_type = PNV_CHIP_P8; > + k->chip_f000f = 0x220ea04980000000ull; > + dc->desc = "PowerNV Chip POWER8"; > +} > + > +static const TypeInfo pnv_chip_power8_info = { > + .name = TYPE_PNV_CHIP_POWER8, > + .parent = TYPE_PNV_CHIP, > + .instance_size = sizeof(PnvChipPower8), > + .class_init = 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 = DEVICE_CLASS(klass); > + PnvChipClass *k = PNV_CHIP_CLASS(klass); > + > + k->realize = pnv_chip_power8e_realize; > + k->cpu_model = "POWER8E"; > + k->chip_type = PNV_CHIP_P8E; > + k->chip_f000f = 0x221ef04980000000ull; > + dc->desc = "PowerNV Chip POWER8E"; > +} > + > +static const TypeInfo pnv_chip_power8e_info = { > + .name = TYPE_PNV_CHIP_POWER8E, > + .parent = TYPE_PNV_CHIP, > + .instance_size = sizeof(PnvChipPower8e), > + .class_init = pnv_chip_power8e_class_init, > +}; > + > +static void pnv_chip_realize(DeviceState *dev, Error **errp) > +{ > + PnvChip *chip = PNV_CHIP(dev); > + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); > + > + pcc->realize(chip, errp); > +} > + > +static Property pnv_chip_properties[] = { > + 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 = DEVICE_CLASS(klass); > + > + dc->realize = pnv_chip_realize; > + dc->props = pnv_chip_properties; > + dc->desc = "PowerNV Chip"; > + } > + > +static const TypeInfo pnv_chip_info = { > + .name = TYPE_PNV_CHIP, > + .parent = TYPE_SYS_BUS_DEVICE, > + .class_init = pnv_chip_class_init, > + .class_size = sizeof(PnvChipClass), > + .abstract = 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 = POWERNV_MACHINE(obj); > + int num_chips; > + > + if (sscanf(value, "%d", &num_chips) != 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 = num_chips; > +} > + > +static void powernv_machine_initfn(Object *obj) > +{ > + PnvMachineState *pnv = POWERNV_MACHINE(obj); > + pnv->num_chips = 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 chips", > + NULL); > } > > static void powernv_machine_class_init(ObjectClass *oc, void *data) > @@ -233,12 +382,17 @@ static const TypeInfo powernv_machine_info = { > .name = TYPE_POWERNV_MACHINE, > .parent = TYPE_MACHINE, > .instance_size = sizeof(PnvMachineState), > + .instance_init = powernv_machine_initfn, > .class_init = powernv_machine_class_init, > }; > > 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_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 > > #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" > + This macro is already defined above. > +#define TYPE_PNV_CHIP_POWER8E "powernv-chip-POWER8E" What about this ? #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; Why Power8e here and... > + > +#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; ... Power8NVL there ? > + > +/* > + * 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)) > > #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; > > #endif /* _PPC_PNV_H */
On 09/01/2016 07:21 PM, Greg Kurz wrote: > On Wed, 31 Aug 2016 18:34:10 +0200 > Cédric Le Goater <clg@kaod.org> 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édric Le Goater <clg@kaod.org> >> --- >> >> Changes since v1: >> >> - 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. >> >> Maybe this object deserves its own file hw/ppc/pnv_chip.c ? >> >> 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; >> >> /* 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 = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); >> + > > You should check that the CPU model is supported by this machine (with > object_class_by_name()) and error out if it is not, or... > >> + pnv->chips = g_new0(PnvChip *, pnv->num_chips); >> + for (i = 0; i < pnv->num_chips; i++) { >> + Object *chip = object_new(chip_typename); > > ... this call to object_new() will abort. > >> + object_property_set_int(chip, CHIP_HWID(i), "chip-id", &error_abort); >> + object_property_set_bool(chip, true, "realized", &error_abort); >> + pnv->chips[i] = PNV_CHIP(chip); >> + } >> + g_free(chip_typename); >> +} >> + >> +static void pnv_chip_power8nvl_realize(PnvChip *chip, Error **errp) >> +{ >> + ; >> +} >> + >> +static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + PnvChipClass *k = PNV_CHIP_CLASS(klass); >> + >> + k->realize = pnv_chip_power8nvl_realize; >> + k->cpu_model = "POWER8NVL"; >> + k->chip_type = PNV_CHIP_P8NVL; >> + k->chip_f000f = 0x120d304980000000ull; >> + dc->desc = "PowerNV Chip POWER8NVL"; >> +} >> + >> +static const TypeInfo pnv_chip_power8nvl_info = { >> + .name = TYPE_PNV_CHIP_POWER8NVL, >> + .parent = TYPE_PNV_CHIP, >> + .instance_size = sizeof(PnvChipPower8NVL), >> + .class_init = 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 = DEVICE_CLASS(klass); >> + PnvChipClass *k = PNV_CHIP_CLASS(klass); >> + >> + k->realize = pnv_chip_power8_realize; >> + k->cpu_model = "POWER8"; >> + k->chip_type = PNV_CHIP_P8; >> + k->chip_f000f = 0x220ea04980000000ull; >> + dc->desc = "PowerNV Chip POWER8"; >> +} >> + >> +static const TypeInfo pnv_chip_power8_info = { >> + .name = TYPE_PNV_CHIP_POWER8, >> + .parent = TYPE_PNV_CHIP, >> + .instance_size = sizeof(PnvChipPower8), >> + .class_init = 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 = DEVICE_CLASS(klass); >> + PnvChipClass *k = PNV_CHIP_CLASS(klass); >> + >> + k->realize = pnv_chip_power8e_realize; >> + k->cpu_model = "POWER8E"; >> + k->chip_type = PNV_CHIP_P8E; >> + k->chip_f000f = 0x221ef04980000000ull; >> + dc->desc = "PowerNV Chip POWER8E"; >> +} >> + >> +static const TypeInfo pnv_chip_power8e_info = { >> + .name = TYPE_PNV_CHIP_POWER8E, >> + .parent = TYPE_PNV_CHIP, >> + .instance_size = sizeof(PnvChipPower8e), >> + .class_init = pnv_chip_power8e_class_init, >> +}; >> + >> +static void pnv_chip_realize(DeviceState *dev, Error **errp) >> +{ >> + PnvChip *chip = PNV_CHIP(dev); >> + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); >> + >> + pcc->realize(chip, errp); >> +} >> + >> +static Property pnv_chip_properties[] = { >> + 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 = DEVICE_CLASS(klass); >> + >> + dc->realize = pnv_chip_realize; >> + dc->props = pnv_chip_properties; >> + dc->desc = "PowerNV Chip"; >> + } >> + >> +static const TypeInfo pnv_chip_info = { >> + .name = TYPE_PNV_CHIP, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .class_init = pnv_chip_class_init, >> + .class_size = sizeof(PnvChipClass), >> + .abstract = 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 = POWERNV_MACHINE(obj); >> + int num_chips; >> + >> + if (sscanf(value, "%d", &num_chips) != 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 = num_chips; >> +} >> + >> +static void powernv_machine_initfn(Object *obj) >> +{ >> + PnvMachineState *pnv = POWERNV_MACHINE(obj); >> + pnv->num_chips = 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 chips", >> + NULL); >> } >> >> static void powernv_machine_class_init(ObjectClass *oc, void *data) >> @@ -233,12 +382,17 @@ static const TypeInfo powernv_machine_info = { >> .name = TYPE_POWERNV_MACHINE, >> .parent = TYPE_MACHINE, >> .instance_size = sizeof(PnvMachineState), >> + .instance_init = powernv_machine_initfn, >> .class_init = powernv_machine_class_init, >> }; >> >> 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_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 >> >> #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" >> + > > This macro is already defined above. > > >> +#define TYPE_PNV_CHIP_POWER8E "powernv-chip-POWER8E" > > What about this ? > > #define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-POWER8E" I thought of doing that and forgot :) >> +#define PNV_CHIP_POWER8E(obj) \ >> + OBJECT_CHECK(PnvChipPower8e, (obj), TYPE_PNV_CHIP_POWER8E) >> + >> +typedef struct PnvChipPower8e { >> + PnvChip pnv_chip; >> +} PnvChipPower8e; > > Why Power8e here and... yes. I will change the names to be in sync with the cpu names and will probably do the same with enum PnvChipType. Thanks, C. >> + >> +#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; > > ... Power8NVL there ? > >> + >> +/* >> + * 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)) >> >> #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; >> >> #endif /* _PPC_PNV_H */ >
On Wed, Aug 31, 2016 at 06:34:10PM +0200, Cédric 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édric Le Goater <clg@kaod.org> > --- > > Changes since v1: > > - 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. > > Maybe this object deserves its own file hw/ppc/pnv_chip.c ? > > 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; > > /* 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 = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); > + > + pnv->chips = g_new0(PnvChip *, pnv->num_chips); > + for (i = 0; i < pnv->num_chips; i++) { > + Object *chip = 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); 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] = 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 = DEVICE_CLASS(klass); > + PnvChipClass *k = PNV_CHIP_CLASS(klass); > + > + k->realize = pnv_chip_power8nvl_realize; > + k->cpu_model = "POWER8NVL"; > + k->chip_type = PNV_CHIP_P8NVL; With the new chip class per cpu class, does this chip_type field serve any purpose any more? > + k->chip_f000f = 0x120d304980000000ull; A comment somewhere explaining what this cryptic value is would be nice. > + dc->desc = "PowerNV Chip POWER8NVL"; > +} > + > +static const TypeInfo pnv_chip_power8nvl_info = { > + .name = TYPE_PNV_CHIP_POWER8NVL, > + .parent = TYPE_PNV_CHIP, > + .instance_size = sizeof(PnvChipPower8NVL), > + .class_init = 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 = DEVICE_CLASS(klass); > + PnvChipClass *k = PNV_CHIP_CLASS(klass); > + > + k->realize = pnv_chip_power8_realize; > + k->cpu_model = "POWER8"; > + k->chip_type = PNV_CHIP_P8; > + k->chip_f000f = 0x220ea04980000000ull; > + dc->desc = "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 = { > + .name = TYPE_PNV_CHIP_POWER8, > + .parent = TYPE_PNV_CHIP, > + .instance_size = sizeof(PnvChipPower8), > + .class_init = 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 = DEVICE_CLASS(klass); > + PnvChipClass *k = PNV_CHIP_CLASS(klass); > + > + k->realize = pnv_chip_power8e_realize; > + k->cpu_model = "POWER8E"; > + k->chip_type = PNV_CHIP_P8E; > + k->chip_f000f = 0x221ef04980000000ull; > + dc->desc = "PowerNV Chip POWER8E"; > +} > + > +static const TypeInfo pnv_chip_power8e_info = { > + .name = TYPE_PNV_CHIP_POWER8E, > + .parent = TYPE_PNV_CHIP, > + .instance_size = sizeof(PnvChipPower8e), > + .class_init = pnv_chip_power8e_class_init, > +}; > + > +static void pnv_chip_realize(DeviceState *dev, Error **errp) > +{ > + PnvChip *chip = PNV_CHIP(dev); > + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); > + > + pcc->realize(chip, errp); > +} > + > +static Property pnv_chip_properties[] = { > + 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 = DEVICE_CLASS(klass); > + > + dc->realize = pnv_chip_realize; > + dc->props = pnv_chip_properties; > + dc->desc = "PowerNV Chip"; > + } > + > +static const TypeInfo pnv_chip_info = { > + .name = TYPE_PNV_CHIP, > + .parent = TYPE_SYS_BUS_DEVICE, > + .class_init = pnv_chip_class_init, > + .class_size = sizeof(PnvChipClass), > + .abstract = 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 = POWERNV_MACHINE(obj); > + int num_chips; > + > + if (sscanf(value, "%d", &num_chips) != 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 = num_chips; > +} > + > +static void powernv_machine_initfn(Object *obj) > +{ > + PnvMachineState *pnv = POWERNV_MACHINE(obj); > + pnv->num_chips = 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 chips", > + NULL); > } > > static void powernv_machine_class_init(ObjectClass *oc, void *data) > @@ -233,12 +382,17 @@ static const TypeInfo powernv_machine_info = { > .name = TYPE_POWERNV_MACHINE, > .parent = TYPE_MACHINE, > .instance_size = sizeof(PnvMachineState), > + .instance_init = powernv_machine_initfn, > .class_init = powernv_machine_class_init, > }; > > 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_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 > > #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)) > > #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; > > #endif /* _PPC_PNV_H */
On Mon, 2016-09-05 at 12:58 +1000, David Gibson wrote: > > With the new chip class per cpu class, does this chip_type field > serve > any purpose any more? > > > + k->chip_f000f = 0x120d304980000000ull; > > A comment somewhere explaining what this cryptic value is would be > nice. It's snapshot from an actual chip ;-) Some of the fields we know about such as the actual chip "type" and DD version but some are obscure even to us :-) Cheers, Ben. `
On 09/05/2016 08:59 AM, Benjamin Herrenschmidt wrote: > On Mon, 2016-09-05 at 12:58 +1000, David Gibson wrote: >> >> With the new chip class per cpu class, does this chip_type field >> serve >> any purpose any more? >> >>> + k->chip_f000f = 0x120d304980000000ull; >> >> A comment somewhere explaining what this cryptic value is would be >> nice. > > It's snapshot from an actual chip ;-) Some of the fields we know about > such as the actual chip "type" and DD version but some are obscure even > to us :-) yeah. I have not found a clear definition of all the bits. I will try to make a macro with what I can collect from the specs and the code. Cheers, C.
On Mon, Sep 05, 2016 at 04:59:23PM +1000, Benjamin Herrenschmidt wrote: > On Mon, 2016-09-05 at 12:58 +1000, David Gibson wrote: > > > > With the new chip class per cpu class, does this chip_type field > > serve > > any purpose any more? > > > > > + k->chip_f000f = 0x120d304980000000ull; > > > > A comment somewhere explaining what this cryptic value is would be > > nice. > > It's snapshot from an actual chip ;-) Some of the fields we know about > such as the actual chip "type" and DD version but some are obscure even > to us :-) Sorry, I actually meant what does "chip_f000f" mean, rather than what does the value inside there mean.
On 09/05/2016 04:58 AM, David Gibson wrote: > On Wed, Aug 31, 2016 at 06:34:10PM +0200, Cédric 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édric Le Goater <clg@kaod.org> >> --- >> >> Changes since v1: >> >> - 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. >> >> Maybe this object deserves its own file hw/ppc/pnv_chip.c ? >> >> 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; >> >> /* 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 = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); >> + >> + pnv->chips = g_new0(PnvChip *, pnv->num_chips); >> + for (i = 0; i < pnv->num_chips; i++) { >> + Object *chip = 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); > > 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. yes. > >> + pnv->chips[i] = 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? 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 I maintain. P9 will use a xive object when available and not a xics. I think this is when the real big difference will show up. So I should make realize() optional. >> + >> +static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(klass); >> + PnvChipClass *k = PNV_CHIP_CLASS(klass); >> + >> + k->realize = pnv_chip_power8nvl_realize; >> + k->cpu_model = "POWER8NVL"; >> + k->chip_type = PNV_CHIP_P8NVL; > > With the new chip class per cpu class, does this chip_type field serve > any purpose any more? It does look a bit redundant, I agree. But it is useful for xscom address translation (P9 is a little different), for xscom devices in general, for core id to pir conversion. It also does for lpc_irq support, which applies to P8NVL (and upper I suppose). Let's keep it for the moment as it serves its purpose, which is to handle small differences without too much cost. If this is going too far, I will propose a set of ops per chip type. >> + k->chip_f000f = 0x120d304980000000ull; > > A comment somewhere explaining what this cryptic value is would be nice. yeah. I will make a macro of it with the bits I know about :/ >> + dc->desc = "PowerNV Chip POWER8NVL"; >> +} >> + >> +static const TypeInfo pnv_chip_power8nvl_info = { >> + .name = TYPE_PNV_CHIP_POWER8NVL, >> + .parent = TYPE_PNV_CHIP, >> + .instance_size = sizeof(PnvChipPower8NVL), >> + .class_init = 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 = DEVICE_CLASS(klass); >> + PnvChipClass *k = PNV_CHIP_CLASS(klass); >> + >> + k->realize = pnv_chip_power8_realize; >> + k->cpu_model = "POWER8"; >> + k->chip_type = PNV_CHIP_P8; >> + k->chip_f000f = 0x220ea04980000000ull; >> + dc->desc = "PowerNV Chip POWER8"; >> +} > > It might be worth using some macros to compactify the definition of > each of the chip variants. yes. we have four of them now. It is time do so. Thanks, C. >> + >> +static const TypeInfo pnv_chip_power8_info = { >> + .name = TYPE_PNV_CHIP_POWER8, >> + .parent = TYPE_PNV_CHIP, >> + .instance_size = sizeof(PnvChipPower8), >> + .class_init = 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 = DEVICE_CLASS(klass); >> + PnvChipClass *k = PNV_CHIP_CLASS(klass); >> + >> + k->realize = pnv_chip_power8e_realize; >> + k->cpu_model = "POWER8E"; >> + k->chip_type = PNV_CHIP_P8E; >> + k->chip_f000f = 0x221ef04980000000ull; >> + dc->desc = "PowerNV Chip POWER8E"; >> +} >> + >> +static const TypeInfo pnv_chip_power8e_info = { >> + .name = TYPE_PNV_CHIP_POWER8E, >> + .parent = TYPE_PNV_CHIP, >> + .instance_size = sizeof(PnvChipPower8e), >> + .class_init = pnv_chip_power8e_class_init, >> +}; >> + >> +static void pnv_chip_realize(DeviceState *dev, Error **errp) >> +{ >> + PnvChip *chip = PNV_CHIP(dev); >> + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); >> + >> + pcc->realize(chip, errp); >> +} >> + >> +static Property pnv_chip_properties[] = { >> + 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 = DEVICE_CLASS(klass); >> + >> + dc->realize = pnv_chip_realize; >> + dc->props = pnv_chip_properties; >> + dc->desc = "PowerNV Chip"; >> + } >> + >> +static const TypeInfo pnv_chip_info = { >> + .name = TYPE_PNV_CHIP, >> + .parent = TYPE_SYS_BUS_DEVICE, >> + .class_init = pnv_chip_class_init, >> + .class_size = sizeof(PnvChipClass), >> + .abstract = 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 = POWERNV_MACHINE(obj); >> + int num_chips; >> + >> + if (sscanf(value, "%d", &num_chips) != 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 = num_chips; >> +} >> + >> +static void powernv_machine_initfn(Object *obj) >> +{ >> + PnvMachineState *pnv = POWERNV_MACHINE(obj); >> + pnv->num_chips = 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 chips", >> + NULL); >> } >> >> static void powernv_machine_class_init(ObjectClass *oc, void *data) >> @@ -233,12 +382,17 @@ static const TypeInfo powernv_machine_info = { >> .name = TYPE_POWERNV_MACHINE, >> .parent = TYPE_MACHINE, >> .instance_size = sizeof(PnvMachineState), >> + .instance_init = powernv_machine_initfn, >> .class_init = powernv_machine_class_init, >> }; >> >> 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_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 >> >> #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)) >> >> #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; >> >> #endif /* _PPC_PNV_H */ >
On Mon, 2016-09-05 at 09:41 +0200, Cédric Le Goater wrote: > yeah. I have not found a clear definition of all the bits. > > I will try to make a macro with what I can collect from the > specs and the code. It's the CFAM stuff, there's some doco internally but nothing releasable publicly... Cheers, Ben.
On 09/05/2016 09:41 AM, David Gibson wrote: > On Mon, Sep 05, 2016 at 04:59:23PM +1000, Benjamin Herrenschmidt wrote: >> On Mon, 2016-09-05 at 12:58 +1000, David Gibson wrote: >>> >>> With the new chip class per cpu class, does this chip_type field >>> serve >>> any purpose any more? >>> >>>> + k->chip_f000f = 0x120d304980000000ull; >>> >>> A comment somewhere explaining what this cryptic value is would be >>> nice. >> >> It's snapshot from an actual chip ;-) Some of the fields we know about >> such as the actual chip "type" and DD version but some are obscure even >> to us :-) > > Sorry, I actually meant what does "chip_f000f" mean, rather than what > does the value inside there mean. f000f is the xscom address of the register containing this CFAM chip id. I will add a chip_cfam_id attribute to the chip object. I guess it makes more sense. Other chips (like the Centaur) have such an ID. C.
On Mon, Sep 05, 2016 at 06:28:10PM +1000, Benjamin Herrenschmidt wrote: > On Mon, 2016-09-05 at 09:41 +0200, Cédric Le Goater wrote: > > yeah. I have not found a clear definition of all the bits. > > > > I will try to make a macro with what I can collect from the > > specs and the code. > > It's the CFAM stuff, there's some doco internally but nothing > releasable publicly... Right... My point is that I want a name (and/or coment) saying what the relevance of this value is to qemu. Why do we need this cryptic value sitting here?
On Mon, Sep 05, 2016 at 11:10:05AM +0200, Cédric Le Goater wrote: > On 09/05/2016 09:41 AM, David Gibson wrote: > > On Mon, Sep 05, 2016 at 04:59:23PM +1000, Benjamin Herrenschmidt wrote: > >> On Mon, 2016-09-05 at 12:58 +1000, David Gibson wrote: > >>> > >>> With the new chip class per cpu class, does this chip_type field > >>> serve > >>> any purpose any more? > >>> > >>>> + k->chip_f000f = 0x120d304980000000ull; > >>> > >>> A comment somewhere explaining what this cryptic value is would be > >>> nice. > >> > >> It's snapshot from an actual chip ;-) Some of the fields we know about > >> such as the actual chip "type" and DD version but some are obscure even > >> to us :-) > > > > Sorry, I actually meant what does "chip_f000f" mean, rather than what > > does the value inside there mean. > > f000f is the xscom address of the register containing this CFAM > chip id. I will add a chip_cfam_id attribute to the chip object. > I guess it makes more sense. > > Other chips (like the Centaur) have such an ID. Ok.
On Mon, Sep 05, 2016 at 09:56:03AM +0200, Cédric Le Goater wrote: > On 09/05/2016 04:58 AM, David Gibson wrote: > > On Wed, Aug 31, 2016 at 06:34:10PM +0200, Cédric 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édric Le Goater <clg@kaod.org> > >> --- > >> > >> Changes since v1: > >> > >> - 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. > >> > >> Maybe this object deserves its own file hw/ppc/pnv_chip.c ? > >> > >> 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; > >> > >> /* 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 = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); > >> + > >> + pnv->chips = g_new0(PnvChip *, pnv->num_chips); > >> + for (i = 0; i < pnv->num_chips; i++) { > >> + Object *chip = 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); > > > > 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. > > yes. > > > > >> + pnv->chips[i] = 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? > > 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 > I maintain. > > P9 will use a xive object when available and not a xics. I think > this is when the real big difference will show up. So I should > make realize() optional. > > >> + > >> +static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > >> +{ > >> + DeviceClass *dc = DEVICE_CLASS(klass); > >> + PnvChipClass *k = PNV_CHIP_CLASS(klass); > >> + > >> + k->realize = pnv_chip_power8nvl_realize; > >> + k->cpu_model = "POWER8NVL"; > >> + k->chip_type = PNV_CHIP_P8NVL; > > > > With the new chip class per cpu class, does this chip_type field serve > > any purpose any more? > > It does look a bit redundant, I agree. But it is useful for xscom > address translation (P9 is a little different), for xscom devices > in general, for core id to pir conversion. It also does for lpc_irq > support, which applies to P8NVL (and upper I suppose). > > Let's keep it for the moment as it serves its purpose, which is > to handle small differences without too much cost. If this is > going too far, I will propose a set of ops per chip type. Ok, fair enough.
On 09/06/2016 02:49 AM, David Gibson wrote: > On Mon, Sep 05, 2016 at 06:28:10PM +1000, Benjamin Herrenschmidt wrote: >> On Mon, 2016-09-05 at 09:41 +0200, Cédric Le Goater wrote: >>> yeah. I have not found a clear definition of all the bits. >>> >>> I will try to make a macro with what I can collect from the >>> specs and the code. >> >> It's the CFAM stuff, there's some doco internally but nothing >> releasable publicly... > > Right... > > My point is that I want a name (and/or coment) saying what the > relevance of this value is to qemu. Why do we need this cryptic value > sitting here? OK. I will add that. It is mostly important for the FW to identify the chip model. PVR is for the CPU. Thanks, C.
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; /* 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 = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); + + pnv->chips = g_new0(PnvChip *, pnv->num_chips); + for (i = 0; i < pnv->num_chips; i++) { + Object *chip = 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); + pnv->chips[i] = PNV_CHIP(chip); + } + g_free(chip_typename); +} + +static void pnv_chip_power8nvl_realize(PnvChip *chip, Error **errp) +{ + ; +} + +static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + PnvChipClass *k = PNV_CHIP_CLASS(klass); + + k->realize = pnv_chip_power8nvl_realize; + k->cpu_model = "POWER8NVL"; + k->chip_type = PNV_CHIP_P8NVL; + k->chip_f000f = 0x120d304980000000ull; + dc->desc = "PowerNV Chip POWER8NVL"; +} + +static const TypeInfo pnv_chip_power8nvl_info = { + .name = TYPE_PNV_CHIP_POWER8NVL, + .parent = TYPE_PNV_CHIP, + .instance_size = sizeof(PnvChipPower8NVL), + .class_init = 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 = DEVICE_CLASS(klass); + PnvChipClass *k = PNV_CHIP_CLASS(klass); + + k->realize = pnv_chip_power8_realize; + k->cpu_model = "POWER8"; + k->chip_type = PNV_CHIP_P8; + k->chip_f000f = 0x220ea04980000000ull; + dc->desc = "PowerNV Chip POWER8"; +} + +static const TypeInfo pnv_chip_power8_info = { + .name = TYPE_PNV_CHIP_POWER8, + .parent = TYPE_PNV_CHIP, + .instance_size = sizeof(PnvChipPower8), + .class_init = 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 = DEVICE_CLASS(klass); + PnvChipClass *k = PNV_CHIP_CLASS(klass); + + k->realize = pnv_chip_power8e_realize; + k->cpu_model = "POWER8E"; + k->chip_type = PNV_CHIP_P8E; + k->chip_f000f = 0x221ef04980000000ull; + dc->desc = "PowerNV Chip POWER8E"; +} + +static const TypeInfo pnv_chip_power8e_info = { + .name = TYPE_PNV_CHIP_POWER8E, + .parent = TYPE_PNV_CHIP, + .instance_size = sizeof(PnvChipPower8e), + .class_init = pnv_chip_power8e_class_init, +}; + +static void pnv_chip_realize(DeviceState *dev, Error **errp) +{ + PnvChip *chip = PNV_CHIP(dev); + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip); + + pcc->realize(chip, errp); +} + +static Property pnv_chip_properties[] = { + 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 = DEVICE_CLASS(klass); + + dc->realize = pnv_chip_realize; + dc->props = pnv_chip_properties; + dc->desc = "PowerNV Chip"; + } + +static const TypeInfo pnv_chip_info = { + .name = TYPE_PNV_CHIP, + .parent = TYPE_SYS_BUS_DEVICE, + .class_init = pnv_chip_class_init, + .class_size = sizeof(PnvChipClass), + .abstract = 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 = POWERNV_MACHINE(obj); + int num_chips; + + if (sscanf(value, "%d", &num_chips) != 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 = num_chips; +} + +static void powernv_machine_initfn(Object *obj) +{ + PnvMachineState *pnv = POWERNV_MACHINE(obj); + pnv->num_chips = 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 chips", + NULL); } static void powernv_machine_class_init(ObjectClass *oc, void *data) @@ -233,12 +382,17 @@ static const TypeInfo powernv_machine_info = { .name = TYPE_POWERNV_MACHINE, .parent = TYPE_MACHINE, .instance_size = sizeof(PnvMachineState), + .instance_init = powernv_machine_initfn, .class_init = powernv_machine_class_init, }; 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_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 #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)) #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; #endif /* _PPC_PNV_H */
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édric Le Goater <clg@kaod.org> --- Changes since v1: - 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. Maybe this object deserves its own file hw/ppc/pnv_chip.c ? hw/ppc/pnv.c | 154 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/ppc/pnv.h | 71 ++++++++++++++++++++++++ 2 files changed, 225 insertions(+)