Re: [Qemu-devel] [PATCH v3 02/10] ppc/pnv: add a PnvChip object

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 14591 bytes --]

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

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]