Re: [PATCH 1/4] pnv_phb4_pec: Keep track of instantiated PHBs

[Daniel Henrique Barboza]

On 3/2/23 19:21, Philippe Mathieu-Daudé wrote:
> Hi Frederic,
>
> On 2/3/23 17:37, Frederic Barrat wrote:
> > Add an array on the PEC object to keep track of the PHBs which are
> > instantiated. The array can be sparsely populated when using
> > user-created PHBs. It will be useful for the next patch to only export
> > instantiated PHBs in the device tree.
> >
> > Signed-off-by: Frederic Barrat <fbarrat@linux.ibm.com>
> > ---
> >   hw/pci-host/pnv_phb4_pec.c     | 12 +++++++-----
> >   hw/ppc/pnv.c                   |  1 +
> >   include/hw/pci-host/pnv_phb4.h |  2 ++
> >   3 files changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> > index 43267a428f..97c06bb0a0 100644
> > --- a/hw/pci-host/pnv_phb4_pec.c
> > +++ b/hw/pci-host/pnv_phb4_pec.c
>
> > +static PnvPHB *pnv_pec_default_phb_realize(PnvPhb4PecState *pec,
> > +                                           int stack_no,
> > +                                           Error **errp)
> >   {
> >       PnvPHB *phb = PNV_PHB(qdev_new(TYPE_PNV_PHB));
> >       int phb_id = pnv_phb4_pec_get_phb_id(pec, stack_no);
> > @@ -128,8 +128,9 @@ static void pnv_pec_default_phb_realize(PnvPhb4PecState 
> > *pec,
> >                               &error_fatal);
> >       if (!sysbus_realize(SYS_BUS_DEVICE(phb), errp)) {
> > -        return;
> > +        return NULL;
> >       }
> > +    return phb;
> >   }
>
>
> > diff --git a/include/hw/pci-host/pnv_phb4.h b/include/hw/pci-host/pnv_phb4.h
> > index 28d61b96c7..0b72ef1471 100644
> > --- a/include/hw/pci-host/pnv_phb4.h
> > +++ b/include/hw/pci-host/pnv_phb4.h
> > @@ -185,6 +185,8 @@ struct PnvPhb4PecState {
> >       /* PHBs */
> >       uint32_t num_phbs;
> > +#define MAX_PHBS_PER_PEC        3
> > +    PnvPHB *phbs[MAX_PHBS_PER_PEC];
> >       PnvChip *chip;
> >   };
>
>  From QOM PoV, better would be to 'embed' the PnvPHB structure (not only
> a pointer to it), and initialize the PnvPHB instance calling
> object_initialize_child() instead of qdev_new().


We were doing something similar in this PHB controllers one year ago. The
reason we moved to host pointers instead of using embed structures was to
allow user-created phbs. There's no way of telling beforehand if the user
wants one, two or three PHBs in the PEC.


Thanks,

Daniel

> See for example the recent conversion of OHCISysBusState in commit
> 01c400ae43 ("hw/display/sm501: Embed OHCI QOM child in chipset").
>
> Regards,
>
> Phil.