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

[Balamuruhan S <bala24@linux.ibm.com>]

On Tue, Sep 10, 2019 at 01:00:54PM +0200, Cédric Le Goater wrote:
> >>> +
> >>> +    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,

Thanks Cedric, I was not aware of how to use it,

> 
> [ ... ] 
> 
> >>> +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);

sure, :+1:

> 
> [ ... ] 
> 
> >>> +
> >>> +/* P9 Pstate table */
> >>> +
> >>
> >> no version ? 
> > 
> > PNV9_OCC_PSTATE_MAJOR_VERSION is the P9 pstate version.
> 
> why isn't it in the switch statement below ? 

it is there in the switch statement,

::
+    case PNV9_OCC_PSTATE_MAJOR_VERSION:
+        return 0x90;
::

> 
> [ ... ] 
>  
> >>> +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

will make the change as suggested by removing the prefix.

-- Bala

> 
> Thanks,
> 
> C. 
>