[v3,02/10] ppc/pnv: add a PnvChip object

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.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---

 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(-)

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 <clg@kaod.org>
> ---
> 
>  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?
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.

> +
> +    /*
> +     * 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()?

> +    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.

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))
>  
>  #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

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 <clg@kaod.org>
>> ---
>>
>>  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
>