Re: [Qemu-devel] [PATCH 2/3] ppc/pnv: add a PnvChip object

[Cédric Le Goater]

On 08/05/2016 11:44 AM, Benjamin Herrenschmidt wrote:
> On Fri, 2016-08-05 at 11:15 +0200, Cédric Le Goater wrote:
>> This is is an abstraction of a P8 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.
>>
>> We start with an empty PnvChip which we will grow in the subsequent
>> patches with controllers required to run the system..
> 
> We should create a subclass PnvChipP8 which we instanciate for now
> since P9 is around the corner and will be a bit different

Yes. I gave it a try on a new 2.8 branch, check it here : 

        
https://github.com/legoater/qemu/commit/60a6206c2b31d897d1af2ea3641870c2cc0e8c41

It defines a PnvChip base class with a realize routine in which 
we can handle specific attributes of child classes like the 
PnvChipPower8. I will send it in v2 for review when David has 
taken a look at v1.

All the controllers are still under the base class, so all looks
good. But when the PnvChipPower9 comes in play, we will need to 
redispatch and see how it fits the purpose of supporting multiple
Chip models.

The core initialization should be ok but building the device 
tree might be a bit of a burden if we have to 'cast' in the chip 
type we need. We will see.


So what would be the big differences with what we have today ?   

Thanks,

C.
 
> Cheers,
> Ben.
> 
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>>  hw/ppc/pnv.c         | 47
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/ppc/pnv.h | 15 +++++++++++++++
>>  2 files changed, 62 insertions(+)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 3bb6a240c25b..a680780e9dea 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -185,6 +185,7 @@ static void ppc_powernv_init(MachineState
>> *machine)
>>      sPowerNVMachineState *pnv = POWERNV_MACHINE(machine);
>>      long fw_size;
>>      char *filename;
>> +    int i;
>>  
>>      if (ram_size < (1 * G_BYTE)) {
>>          error_report("Warning: skiboot may not work with < 1GB of
>> RAM");
>> @@ -236,6 +237,23 @@ static void ppc_powernv_init(MachineState
>> *machine)
>>              pnv->initrd_base = 0;
>>              pnv->initrd_size = 0;
>>      }
>> +
>> +    /* Create PowerNV chips
>> +     *
>> +     * FIXME: We should decide how many chips to create based on
>> +     * #cores and Venice vs. Murano vs. Naples chip type etc..., for
>> +     * now, just create one chip, with all the cores.
>> +     */
>> +    pnv->num_chips = 1;
>> +
>> +    pnv->chips = g_new0(PnvChip, pnv->num_chips);
>> +    for (i = 0; i < pnv->num_chips; i++) {
>> +        PnvChip *chip = &pnv->chips[i];
>> +
>> +        object_initialize(chip, sizeof(*chip), TYPE_PNV_CHIP);
>> +        object_property_set_int(OBJECT(chip), i, "chip-id",
>> &error_abort);
>> +        object_property_set_bool(OBJECT(chip), true, "realized",
>> &error_abort);
>> +    }
>>  }
>>  
>>  static void powernv_machine_class_init(ObjectClass *oc, void *data)
>> @@ -274,10 +292,39 @@ static const TypeInfo powernv_machine_2_8_info
>> = {
>>      .class_init    = powernv_machine_2_8_class_init,
>>  };
>>  
>> +
>> +static void pnv_chip_realize(DeviceState *dev, Error **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,
>> +    .instance_size = sizeof(PnvChip),
>> +    .class_init    = pnv_chip_class_init,
>> +};
>> +
>> +
>>  static void powernv_machine_register_types(void)
>>  {
>>      type_register_static(&powernv_machine_info);
>>      type_register_static(&powernv_machine_2_8_info);
>> +    type_register_static(&pnv_chip_info);
>>  }
>>  
>>  type_init(powernv_machine_register_types)
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 2990f691672d..6907dc9e5c3d 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -20,6 +20,18 @@
>>  #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)
>> +
>> +typedef struct PnvChip {
>> +    /*< private >*/
>> +    SysBusDevice parent_obj;
>> +
>> +    /*< public >*/
>> +    uint32_t     chip_id;
>> +} PnvChip;
>>  
>>  #define TYPE_POWERNV_MACHINE      "powernv-machine"
>>  #define POWERNV_MACHINE(obj) \
>> @@ -31,6 +43,9 @@ typedef struct sPowerNVMachineState {
>>  
>>      uint32_t initrd_base;
>>      long initrd_size;
>> +
>> +    uint32_t  num_chips;
>> +    PnvChip   *chips;
>>  } sPowerNVMachineState;
>>  
>>  #endif /* _PPC_PNV_H */