Re: [Qemu-devel] [PATCH v1 3/3] hw/ppc/pnv_homer: add PowerNV homer device model

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

>>> +
>>> +    object_initialize_child(obj, "homer",  &chip9->homer, sizeof(chip9->homer),
>>> +                            TYPE_PNV9_HOMER, &error_abort, NULL);
>>> +    object_property_add_const_link(OBJECT(&chip9->homer), "chip", obj,
>>> +                                   &error_abort);
>>
>> Does HOMER need the chip ? It is not used but we might want to in the 
>> core_max_array() ? 
> 
> sorry, no it is not required, I will remove it.

It seems you will need the chip in core_max_array(). I would keep it. 
See below,

[ ... ] 

>>> +static bool core_max_array(void *opaque, hwaddr addr)

Please change the 'void *opaque' function parameter in 'PnvHOMER *homer'

>>> +{
>>> +    PnvHOMER *homer = PNV_HOMER(opaque);
>>> +    PnvHOMERClass *homer_class = PNV_HOMER_GET_CLASS(homer);
>>> +
>>> +    MachineState *ms = MACHINE(qdev_get_machine());
>>
>> Do you need the whole machine or only the chip ?  
> 
> yes, I see it is for active cores in the chip, I can use `nr_cores`
> defined in PnvChip.


If you keep the QOM link above, you can grab it in the realize handler of
the HOMER model with :  

    Object *obj;
    Error *local_err = NULL;

    obj = object_property_get_link(OBJECT(dev), "chip", &local_err);
    if (!obj) {
        error_propagate(errp, local_err);
        error_prepend(errp, "required link 'chip' not found: ");
        return;
    }

    homer->chip = PNV_CHIP(obj);

[ ... ] 

>>> +
>>> +/* P9 Pstate table */
>>> +
>>
>> no version ? 
> 
> PNV9_OCC_PSTATE_MAJOR_VERSION is the P9 pstate version.

why isn't it in the switch statement below ? 

[ ... ] 
 
>>> +typedef struct PnvHOMER {
>>> +    DeviceState parent;
>>> +
>>> +    MemoryRegion homer_regs;
>>
>> the homer_ prefix is not useful.
> 
> okay, I will change it to `hregs`.

I would just remove the prefix

Thanks,

C.