Re: [Qemu-ppc] [PATCH v4 02/20] ppc/pnv: add a PnvChip object

[Cédric Le Goater]

On 10/07/2016 06:26 AM, David Gibson wrote:
> On Mon, Oct 03, 2016 at 09:24:38AM +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.
>>
>> Signed-off-by: Cédric Le Goater <address@hidden>
> 
> Reviewed-by: David Gibson <address@hidden>
> 
> I do have one small suggested change below, but it's not that important.
> 
>> ---
>>
>>  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.
>>  This is getting big, maybe should move the code to hw/ppc/pnv_chip.c ?
>>
>>  Changes since v3:
>>
>>  - removed PnvChipPower* types
>>  - removed realize ops of PnvChip
>>  - replaced scanf by qemu_strtoul
>>
>>  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         | 194 
>> +++++++++++++++++++++++++++++++++++++++++++++++++--
>>  include/hw/ppc/pnv.h |  61 ++++++++++++++++
>>  2 files changed, 250 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 02fc4826baa4..08f72dbdca97 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)));
>> @@ -115,11 +126,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;
>>  }
>>  
>> @@ -147,6 +157,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)) {
>> @@ -191,6 +203,172 @@ 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(PnvChip),
>> +    .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(PnvChip),
>> +    .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(PnvChip),
>> +    .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(PnvChip),
>> +    .class_init    = pnv_chip_power9_class_init,
>> +};
>> +
>> +static void pnv_chip_realize(DeviceState *dev, Error **errp)
>> +{
>> +    /* left purposely empty */
>> +}
>> +
>> +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);
>> +    unsigned long num_chips;
>> +
>> +    if (qemu_strtoul(value, NULL, 10, &num_chips) < 0) {
>> +        error_setg(errp, "invalid num_chips property: '%s'", value);
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * TODO: should we decide on how many chips we can create based
>> +     * on #cores and Venice vs. Murano vs. Naples chip type etc...,
>> +     */
>> +    if (num_chips < 1 || num_chips > 4) {
>> +        error_setg(errp, "invalid number of chips: '%s'", value);
>> +        return;
>> +    }
>> +
>> +    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);
> 
> So I recently discovered that nearly everything done with
> object_property_add_*() should usually be done with
> object_class_property_add_*() - basically anything where the property
> exists for every instance of the class.  Yes, practically everything
> in qemu already gets this wrong.  Still, if you get a chance to rework
> this, might as well not make this one more.

Sure I will in v5.

Thanks,

C. 

>>  }
>>  
>>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>> @@ -212,12 +390,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..da543ed81636 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -20,6 +20,64 @@
>>  #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;
>> +} PnvChipClass;
>> +
>> +#define TYPE_PNV_CHIP_POWER8E TYPE_PNV_CHIP "-POWER8E"
>> +#define PNV_CHIP_POWER8E(obj) \
>> +    OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP_POWER8E)
>> +
>> +#define TYPE_PNV_CHIP_POWER8 TYPE_PNV_CHIP "-POWER8"
>> +#define PNV_CHIP_POWER8(obj) \
>> +    OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP_POWER8)
>> +
>> +#define TYPE_PNV_CHIP_POWER8NVL TYPE_PNV_CHIP "-POWER8NVL"
>> +#define PNV_CHIP_POWER8NVL(obj) \
>> +    OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP_POWER8NVL)
>> +
>> +#define TYPE_PNV_CHIP_POWER9 TYPE_PNV_CHIP "-POWER9"
>> +#define PNV_CHIP_POWER9(obj) \
>> +    OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP_POWER9)
>> +
>> +/*
>> + * This generates a HW chip id depending on an index:
>> + *
>> + *    0x0, 0x1, 0x10, 0x11
>> + *
>> + * 4 chips should be the maximum
>> + */
>> +#define CHIP_HWID(i) ((((i) & 0x3e) << 3) | ((i) & 0x1))
>>  
>>  #define TYPE_POWERNV_MACHINE       MACHINE_TYPE_NAME("powernv")
>>  #define POWERNV_MACHINE(obj) \
>> @@ -31,6 +89,9 @@ typedef struct PnvMachineState {
>>  
>>      uint32_t initrd_base;
>>      long initrd_size;
>> +
>> +    uint32_t  num_chips;
>> +    PnvChip   **chips;
>>  } PnvMachineState;
>>  
>>  #define POWERNV_FDT_ADDR                0x01000000
>