Re: [PATCH v3 04/10] pnv_phb4_pec.c: remove stack 'phb-id' alias

[Cédric Le Goater]

On 1/10/22 17:27, Daniel Henrique Barboza wrote:
> On 1/10/22 12:49, Cédric Le Goater wrote:
> > On 1/10/22 15:33, Daniel Henrique Barboza wrote:
> > > This alias is a indirect way of setting stack->phb->index. Since we have
> > > access to a valid stack->phb (for default_enabled() at least - next
> > > patch will deal with it accordingly) we can directly set the phb 'index'
> > > attribute.
> > >
> > > Let's also take the opportunity to explain why we're having to deal with
> > > stack->phb attributes during pec_realize().
> > >
> > > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > > ---
> > >   hw/pci-host/pnv_phb4_pec.c | 12 ++++++++++--
> > >   1 file changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> > > index 1f264d0a9c..417fac4cef 100644
> > > --- a/hw/pci-host/pnv_phb4_pec.c
> > > +++ b/hw/pci-host/pnv_phb4_pec.c
> > > @@ -395,8 +395,17 @@ static void pnv_pec_realize(DeviceState *dev, Error **errp)
> > >           int phb_id = pnv_phb4_pec_get_phb_id(pec, i);
> > >           object_property_set_int(stk_obj, "stack-no", i, &error_abort);
> > > -        object_property_set_int(stk_obj, "phb-id", phb_id, &error_abort);
> > >           object_property_set_link(stk_obj, "pec", OBJECT(pec), &error_abort);
> > > +
> > > +        /*
> > > +         * stack->phb->index is dependent on the position the
> > > +         * stack occupies in pec->stacks[]. We have this information
> > > +         * available here via the 'i' iterator so it's convenient to
> > > +         * do it now.
> > > +         */
> > > +        object_property_set_int(OBJECT(&stack->phb), "index", phb_id,
> > > +                                &error_abort);
> >
> > I don't like the fact that we are exposing ->phb under the PEC model.
> > It looks like this is going to be a problem afterwards when defaults
> > are disabled.
> >
> > We should move the setting of the PHB ID under pnv_pec_stk_realize()
> > before the PHB is realized and compute the id with :
> >
> >         int phb_id = pnv_phb4_pec_get_phb_id(pec, stack->stack_no);
>
>
> Oh, if stack->stack_no is the stack index of the pec->stacks[] array then we 
> should
> instead move all this stuff into phb4_realize().

The properties are generally set by the parent object and for
user created PHB4 devices, the user will set the PHB4 "index"
property which we will use to do a lookup of the owning stack.
It's similar to chip-id.

Thanks,

C.