[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 02/10] ppc/pnv: add a PnvChip object
From: |
Cédric Le Goater |
Subject: |
Re: [Qemu-devel] [PATCH v3 02/10] ppc/pnv: add a PnvChip object |
Date: |
Wed, 21 Sep 2016 09:44:36 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 |
On 09/20/2016 03:50 PM, David Gibson wrote:
> On Thu, Sep 15, 2016 at 02:45:52PM +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 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.
>
> Thanks for the above - that's basically exactly the sort of
> description I was looking for.
>
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>
>> 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 ?
>>
>> 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:
>>
>> - 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.
>>
>> 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)));
>> }
>>
>> +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 == 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[] = "qemu,powernv\0ibm,powernv";
>> int off;
>> + int i;
>>
>> fdt = 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))));
>> }
>>
>> - /* 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 = 0; i < pnv->num_chips; i++) {
>> + powernv_populate_chip(pnv->chips[i], fdt);
>> + }
>> return fdt;
>> }
>>
>> @@ -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;
>>
>> /* 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 == NULL) {
>> + machine->cpu_model = "POWER8";
>> + }
>> +
>> + /* Create the processor chips */
>> + chip_typename = 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)->name);
>> + exit(1);
>> + }
>> +
>> + pnv->chips = g_new0(PnvChip *, pnv->num_chips);
>> + for (i = 0; i < pnv->num_chips; i++) {
>> + char chip_name[32];
>> + Object *chip = object_new(chip_typename);
>> +
>> + pnv->chips[i] = PNV_CHIP(chip);
>> +
>> + snprintf(chip_name, sizeof(chip_name), "chip[%d]", CHIP_HWID(i));
>> + object_property_add_child(OBJECT(pnv), chip_name, chip,
>> &error_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 *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + PnvChipClass *k = PNV_CHIP_CLASS(klass);
>> +
>> + k->cpu_model = "POWER8E";
>> + k->chip_type = PNV_CHIP_POWER8E;
>> + k->chip_cfam_id = 0x221ef04980000000ull; /* P8 Murano DD2.1 */
>> + 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_power8_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + PnvChipClass *k = PNV_CHIP_CLASS(klass);
>> +
>> + k->cpu_model = "POWER8";
>> + k->chip_type = PNV_CHIP_POWER8;
>> + k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>> + 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_power8nvl_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + PnvChipClass *k = PNV_CHIP_CLASS(klass);
>> +
>> + k->cpu_model = "POWER8NVL";
>> + k->chip_type = PNV_CHIP_POWER8NVL;
>> + k->chip_cfam_id = 0x120d304980000000ull; /* P8 Naples DD1.0 */
>> + 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_power9_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> + PnvChipClass *k = PNV_CHIP_CLASS(klass);
>> +
>> + k->cpu_model = "POWER9";
>> + k->chip_type = PNV_CHIP_POWER9;
>> + k->chip_cfam_id = 0x100d104980000000ull; /* P9 Nimbus DD1.0 */
>> + dc->desc = "PowerNV Chip POWER9";
>> +}
>> +
>> +static const TypeInfo pnv_chip_power9_info = {
>> + .name = TYPE_PNV_CHIP_POWER9,
>> + .parent = TYPE_PNV_CHIP,
>> + .instance_size = sizeof(PnvChipPower9),
>> + .class_init = pnv_chip_power9_class_init,
>> +};
>> +
>> +static void pnv_chip_realize(DeviceState *dev, Error **errp)
>> +{
>> + PnvChip *chip = PNV_CHIP(dev);
>> + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> +
>> + if (pcc->realize) {
>> + pcc->realize(chip, errp);
>
> 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?
>
>> + }
>> +}
>> +
>> +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);
>> + }
>
> 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 = 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);
>
> 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_chips
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 processor
>> chips",
>> + NULL);
>> }
>>
>> static void powernv_machine_class_init(ObjectClass *oc, void *data)
>> @@ -211,12 +387,18 @@ 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_register_static(&pnv_chip_power9_info);
>> }
>>
>> 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
>>
>> #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.
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
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.
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))
>>
>> #define TYPE_POWERNV_MACHINE MACHINE_TYPE_NAME("powernv")
>> #define POWERNV_MACHINE(obj) \
>> @@ -31,6 +107,9 @@ typedef struct PnvMachineState {
>>
>> uint32_t initrd_base;
>> long initrd_size;
>> +
>> + uint32_t num_chips;
>> + PnvChip **chips;
>> } PnvMachineState;
>>
>> #define POWERNV_FDT_ADDR 0x01000000
>
- [Qemu-devel] [PATCH v3 00/10] ppc/pnv: loading skiboot and booting the kernel, Cédric Le Goater, 2016/09/15
- [Qemu-devel] [PATCH v3 01/10] ppc/pnv: add skeleton PowerNV platform, Cédric Le Goater, 2016/09/15
- [Qemu-devel] [PATCH v3 02/10] ppc/pnv: add a PnvChip object, Cédric Le Goater, 2016/09/15
- [Qemu-devel] [PATCH v3 03/10] ppc/pnv: add a core mask to PnvChip, Cédric Le Goater, 2016/09/15
- [Qemu-devel] [PATCH v3 04/10] ppc/pnv: add a PIR handler to PnvChip, Cédric Le Goater, 2016/09/15
- [Qemu-devel] [PATCH v3 05/10] ppc/pnv: add a PnvCore object, Cédric Le Goater, 2016/09/15