Re: [PATCH v2 08/16] ppc/pnv: user created pnv-phb for powernv9

[Frederic Barrat]

On 03/06/2022 23:00, Daniel Henrique Barboza wrote:
> > >   static void pnv_phb4_realize(DeviceState *dev, Error **errp)
> > >   {
> > >       PnvPHB4 *phb = PNV_PHB4(dev);
> > > +    PnvMachineState *pnv = PNV_MACHINE(qdev_get_machine());
> > > +    PnvChip *chip = pnv_get_chip(pnv, phb->chip_id);
> > >       XiveSource *xsrc = &phb->xsrc;
> > > +    BusState *s;
> > > +    Error *local_err = NULL;
> > >       int nr_irqs;
> > >       char name[32];
> > > +    if (!chip) {
> > > +        error_setg(errp, "invalid chip id: %d", phb->chip_id);
> > > +        return;
> > > +    }
> > > +
> > > +    /* User created PHBs need to be assigned to a PEC */
> > > +    if (!phb->pec) {
> > > +        phb->pec = pnv_phb4_get_pec(chip, phb, &local_err);
> > > +        if (local_err) {
> > > +            error_propagate(errp, local_err);
> > > +            return;
> > > +        }
> > > +    }
> > > +
> > > +    /* Reparent the PHB to the chip to build the device tree */
> > > +    pnv_chip_parent_fixup(chip, OBJECT(phb->phb_base), phb->phb_id);
> >
> >
> > Didn't you mean to do that only for the user-created case? 
>
> It's done for both because the default generated PHBs are being created 
> loosely
> from the chip starting on 3f4c369ea63e ("ppc/pnv: make PECs create and 
> realize
> PHB4s"). We had to amend the code after that commit to fix the QOM 
> hierarchy
> of these devices. 6e7b96750359 ("ppc/pnv: fix default PHB4 QOM 
> hierarchy") has
> more details.
>
>
> > And why not attaching the PHB to the PEC instead of the chip, like in 
> > pnv_pec_default_phb_realize() ? I think it makes more sense to keep 
> > the chip->PEC->PHB parenting in the qom tree (and incidentally, that's 
> > where I'd prefer to have the phb3 model move to).
>
> I can only speculate since I didn't participate in the initial design. My
> educated guess is that we didn't want to show PECs in qtree, hence we
> made the PHB a child of the chip instead. I'll also guess that this can be
> due to how PHB3 worked and we just copied the existing design.


I don't believe that's correct though. As Cedric replied, the PECs show 
up in the qom tree on P9/P10, with chip->PEC->PHB, in that order. And I 
think that's fine, that's the model we should tend to (and which would 
require a fix on P8, since there we have chip->phb->pbcq, which is 
backwards. The pbcq object is the equivalent of the PEC).

So I think we should keep the qom relationship as it currently is on 
P9/P10, since it looks good. On P8, we can keep the current status for 
now and, as discussed privately, have another series later to clean 
things up.

  Fred