Re: [PATCH v2 03/10] ppc/pnv: Add a Power11 Pnv11Chip, and a Power11 Machine

["Cédric Le Goater" <clg@kaod.org>]

On 4/26/24 19:34, Aditya Gupta wrote:
> Hello Cédric,
> 
>>>
>>> <...snip...>
>>>
>>> - * Multi processor support for POWER8, POWER8NVL and POWER9.
>>> + * Multi processor support for POWER8, POWER8NVL, POWER9, POWER10 and Power11.
>>
>> POWER10 -> Power10. Don't ask me why.
> 
> Sure, got it !
> 
>>
>>>     * XSCOM, serial communication sideband bus to configure chiplets.
>>>     * Simple LPC Controller.
>>>     * Processor Service Interface (PSI) Controller.
>>> - * Interrupt Controller, XICS (POWER8) and XIVE (POWER9) and XIVE2 (Power10).
>>> + * Interrupt Controller, XICS (POWER8) and XIVE (POWER9) and XIVE2 (Power10 &
>>> +   Power11).
>>>     * POWER8 PHB3 PCIe Host bridge and POWER9 PHB4 PCIe Host bridge.
>>>     * Simple OCC is an on-chip micro-controller used for power management tasks.
>>>     * iBT device to handle BMC communication, with the internal BMC simulator
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index 74e7908e5ffb..06e272f3bdd3 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -431,13 +431,27 @@ static const uint8_t pa_features_31[] = { 74, 0,
>>>    static void pnv_chip_power10_dt_populate(PnvChip *chip, void *fdt)
>>>    {
>>> -    static const char compat[] = "ibm,power10-xscom\0ibm,xscom";
>>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>> +    static const char compat_p10[] = "ibm,power10-xscom\0ibm,xscom";
>>> +    static const char compat_p11[] = "ibm,power11-xscom\0ibm,xscom";
>>> +
>>> +    PnvChipType chip_type = pcc->chip_type;
>>> +    const char *compat;
>>> +    int compat_len;
>>>        int i;
>>> +    if (chip_type == PNV_TYPE_POWER11) {
>>> +        compat = compat_p11;
>>> +        compat_len = sizeof(compat_p11);
>>> +    } else {
>>> +        compat = compat_p10;
>>> +        compat_len = sizeof(compat_p10);
>>> +    }
>>
>> please introduce a pnv_chip_power11_dt_populate() routine instead.
> 
> Okay.
> 
>>
>>>        pnv_dt_xscom(chip, fdt, 0,
>>>                     cpu_to_be64(PNV10_XSCOM_BASE(chip)),
>>>                     cpu_to_be64(PNV10_XSCOM_SIZE),
>>> -                 compat, sizeof(compat));
>>> +                 compat, compat_len);
>>>        for (i = 0; i < chip->nr_cores; i++) {
>>>            PnvCore *pnv_core = chip->cores[i];
>>> @@ -1288,6 +1302,8 @@ static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu,
>>>    #define POWER10_CORE_MASK  (0xffffffffffffffull)
>>> +#define POWER11_CORE_MASK  (0xffffffffffffffull)
>>> +
>>>    static void pnv_chip_power8_instance_init(Object *obj)
>>>    {
>>>        Pnv8Chip *chip8 = PNV8_CHIP(obj);
>>> @@ -1838,6 +1854,7 @@ static void pnv_chip_power10_instance_init(Object *obj)
>>>    static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp)
>>>    {
>>>        PnvChip *chip = PNV_CHIP(chip10);
>>> +    PnvChipClass *chip_class = PNV_CHIP_GET_CLASS(chip);
>>>        int i;
>>>        chip10->nr_quads = DIV_ROUND_UP(chip->nr_cores, 4);
>>> @@ -1846,7 +1863,11 @@ static void pnv_chip_power10_quad_realize(Pnv10Chip *chip10, Error **errp)
>>>        for (i = 0; i < chip10->nr_quads; i++) {
>>>            PnvQuad *eq = &chip10->quads[i];
>>> -        pnv_chip_quad_realize_one(chip, eq, chip->cores[i * 4],
>>> +        if (chip_class->chip_type == PNV_TYPE_POWER11)
>>> +            pnv_chip_quad_realize_one(chip, eq, chip->cores[i * 4],
>>> +                                  PNV_QUAD_TYPE_NAME("power11"));
>>> +        else
>>> +            pnv_chip_quad_realize_one(chip, eq, chip->cores[i * 4],
>>>                                      PNV_QUAD_TYPE_NAME("power10"));
>>
>>
>> Please change introduce a new pnv_chip_quad_realize() routine taking an
>> extra type_name argument.
> 
> Sure, can do it. But as it's called from 'pnv_chip_power10_realize',
> might require a 'pnv_chip_power11_realize' function also, so it can pass
> type_name as "power11" vs "power10".

Do the Power11 and Power10 processors have the same XSCOM and MMIO
address spaces ?

> Will do it.
> 
>>
>>
>>>            pnv_xscom_add_subregion(chip, PNV10_XSCOM_EQ_BASE(eq->quad_id),
>>> @@ -2116,6 +2137,35 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data)
>>>                                        &k->parent_realize);
>>>    }
>>> +static void pnv_chip_power11_class_init(ObjectClass *klass, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>> +    PnvChipClass *k = PNV_CHIP_CLASS(klass);
>>> +
>>> +    static const int i2c_ports_per_engine[PNV10_CHIP_MAX_I2C] = {14, 14, 2, 16};
>>> +
>>> +    k->chip_cfam_id = 0x120da04900008000ull; /* P11 (with NX) */
>>> +    k->chip_type = PNV_TYPE_POWER11;
>>> +    k->cores_mask = POWER11_CORE_MASK;
>>> +    k->chip_pir = pnv_chip_pir_p10;
>>> +    k->intc_create = pnv_chip_power10_intc_create;
>>> +    k->intc_reset = pnv_chip_power10_intc_reset;
>>> +    k->intc_destroy = pnv_chip_power10_intc_destroy;
>>> +    k->intc_print_info = pnv_chip_power10_intc_print_info;
>>> +    k->isa_create = pnv_chip_power10_isa_create;
>>> +    k->dt_populate = pnv_chip_power10_dt_populate;
>>> +    k->pic_print_info = pnv_chip_power10_pic_print_info;
>>> +    k->xscom_core_base = pnv_chip_power10_xscom_core_base;
>>> +    k->xscom_pcba = pnv_chip_power10_xscom_pcba;
>>> +    dc->desc = "PowerNV Chip POWER11";
>>> +    k->num_pecs = PNV10_CHIP_MAX_PEC;
>>> +    k->i2c_num_engines = PNV10_CHIP_MAX_I2C;
>>> +    k->i2c_ports_per_engine = i2c_ports_per_engine;
>>> +
>>> +    device_class_set_parent_realize(dc, pnv_chip_power10_realize,
>>> +                                    &k->parent_realize);
>>> +}
>>> +
>>>    static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>>>    {
>>>        PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>> @@ -2510,6 +2560,22 @@ static void pnv_machine_p10_rainier_class_init(ObjectClass *oc, void *data)
>>>        pmc->i2c_init = pnv_rainier_i2c_init;
>>>    }
>>> +static void pnv_machine_power11_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    MachineClass *mc = MACHINE_CLASS(oc);
>>> +    PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc);
>>> +    static const char compat[] = "qemu,powernv11\0ibm,powernv";
>>> +
>>> +    /* do power10_class_init as p11 core is same as p10 */
>>> +    pnv_machine_p10_common_class_init(oc, data);
>>> +
>>> +    mc->desc = "IBM PowerNV (Non-Virtualized) POWER11";
>>> +    mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power11");
>>
>> This should be using a versionned CPU "power11_vx.y".
> 
> Okay.
> 
>>
>>> +
>>> +    pmc->compat = compat;
>>> +    pmc->compat_size = sizeof(compat);
>>> +}
>>> +
>>>    static bool pnv_machine_get_hb(Object *obj, Error **errp)
>>>    {
>>>        PnvMachineState *pnv = PNV_MACHINE(obj);
>>> @@ -2613,7 +2679,23 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>>>            .parent        = TYPE_PNV10_CHIP,          \
>>>        }
>>> +#define DEFINE_PNV11_CHIP_TYPE(type, class_initfn) \
>>> +    {                                              \
>>> +        .name          = type,                     \
>>> +        .class_init    = class_initfn,             \
>>> +        .parent        = TYPE_PNV11_CHIP,          \
>>> +    }
>>> +
>>>    static const TypeInfo types[] = {
>>> +    {
>>> +        .name          = MACHINE_TYPE_NAME("powernv11"),
>>> +        .parent        = TYPE_PNV_MACHINE,
>>> +        .class_init    = pnv_machine_power11_class_init,
>>> +        .interfaces = (InterfaceInfo[]) {
>>> +            { TYPE_XIVE_FABRIC },
>>> +            { },
>>> +        },
>>> +    },
>>>        {
>>>            .name          = MACHINE_TYPE_NAME("powernv10-rainier"),
>>>            .parent        = MACHINE_TYPE_NAME("powernv10"),
>>> @@ -2668,6 +2750,16 @@ static const TypeInfo types[] = {
>>>            .abstract      = true,
>>>        },
>>> +    /*
>>> +     * P11 chip and variants
>>> +     */
>>> +    {
>>> +        .name          = TYPE_PNV11_CHIP,
>>> +        .parent        = TYPE_PNV10_CHIP,
>>> +        .instance_size = sizeof(Pnv11Chip),
>>> +    },
>>> +    DEFINE_PNV11_CHIP_TYPE(TYPE_PNV_CHIP_POWER11, pnv_chip_power11_class_init),
>>> +
>>>        /*
>>>         * P10 chip and variants
>>>         */
>>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
>>> index f40ab721d6fc..4522655793da 100644
>>> --- a/hw/ppc/pnv_core.c
>>> +++ b/hw/ppc/pnv_core.c
>>> @@ -402,7 +402,16 @@ static const TypeInfo pnv_core_infos[] = {
>>>        DEFINE_PNV_CORE_TYPE(power8, "power8_v2.0"),
>>>        DEFINE_PNV_CORE_TYPE(power8, "power8nvl_v1.0"),
>>>        DEFINE_PNV_CORE_TYPE(power9, "power9_v2.2"),
>>> -    DEFINE_PNV_CORE_TYPE(power10, "power10_v2.0"),
>>> +    {
>>> +        .parent = TYPE_PNV_CORE,
>>> +        .name = PNV_CORE_TYPE_NAME("power10_v2.0"),
>>> +        .class_init = pnv_core_power10_class_init,
>>> +        .class_base_init = pnv_core_power10_class_init,
>>> +    },
>>
>> hmm, what is this change ?
> 
> I expanded the macro, to add '.class_base_init', as .class_base_init
> will be run by QEMU during initialisation of any child classes also. And
> I wanted to let QEMU call 'pnv_core_power10_class_init' when it
> initialises 'power11' type.
> 
>>
>>
>>> +    {
>>> +        .parent = PNV_CORE_TYPE_NAME("power10_v2.0"),
>>> +        .name = PNV_CORE_TYPE_NAME("power11"),
>>> +    }
>>
>> nope. please use DEFINE_PNV_CORE_TYPE()
> 
> Okay, then, should I have a 'pnv_core_power11_class_init' call
> 'pnv_core_power10_class_init' ?

yes.


> 
>>
>>>    };
>>>    DEFINE_TYPES(pnv_core_infos)
>>> @@ -633,6 +642,11 @@ static const TypeInfo pnv_quad_infos[] = {
>>>            .parent = TYPE_PNV_QUAD,
>>>            .name = PNV_QUAD_TYPE_NAME("power10"),
>>>            .class_init = pnv_quad_power10_class_init,
>>> +        .class_base_init = pnv_quad_power10_class_init,
>>> +    },
>>> +    {
>>> +        .parent = PNV_QUAD_TYPE_NAME("power10"),
>>> +        .name = PNV_QUAD_TYPE_NAME("power11"),
>>
>> hmm, I'd rather introduce pnv_quad_power11_class_init.
> 
> Same, in that case, I can have a 'pnv_quad_power11_class_init' which
> just calls 'pnv_quad_power10_class_init'.

yes.


Thanks,

C.


> 
> Any comments ?
> 
>>> <...snip...>
>>>
>>> +struct Pnv11Chip {
>>> +    /*< private >*/
>>> +    Pnv10Chip      parent_obj;
>>
>> Can't you use a typedef instead ?
> 
> Yes, makes sense. Thanks Cédric.
> 
> - Aditya Gupta
> 
>>
>> Thanks,
>>
>> C.
>>
>>
>>> +};
>>> +
>>>    struct PnvChipClass {
>>>        /*< private >*/
>>>        SysBusDeviceClass parent_class;
>>