Re: [PATCH v3 07/10] ppc/pnv: move PHB4 related XSCOM init to phb4_realize()

[Daniel Henrique Barboza]

On 1/10/22 12:57, Cédric Le Goater wrote:
> On 1/10/22 15:33, Daniel Henrique Barboza wrote:
> > Before enabling pnv-phb4 user creatable devices we need to handle PHB4
> > specific code in pnv_pec_stk_realize().
> >
> > The 'stack->phb_regs_mr' PHB4 passthrough XSCOM initialization relies on
> > 'stack->phb' being not NULL. Moving 'stack->phb_regs_mr' region_init()
> > and add_subregion() to phb4_realize() time is a natural thing to do and
> > it'll spare us from checking 'phb->stack != NULL' in stk_realize() when
> > user creatable pnv-phb4s are implemented.
> >
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >   hw/pci-host/pnv_phb4.c     | 27 +++++++++++++++++++++++++++
> >   hw/pci-host/pnv_phb4_pec.c | 10 ----------
> >   2 files changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/pci-host/pnv_phb4.c b/hw/pci-host/pnv_phb4.c
> > index 1a7395772f..152911a285 100644
> > --- a/hw/pci-host/pnv_phb4.c
> > +++ b/hw/pci-host/pnv_phb4.c
> > @@ -1194,6 +1194,31 @@ void pnv_phb4_set_stack_phb_props(PnvPhb4PecStack *stack,
> >                                &error_abort);
> >   }
> > +static void pnv_phb4_init_xscom_passthrough(PnvPHB4 *phb)
> > +{
> > +    PnvPhb4PecState *pec;
> > +    PnvPhb4PecClass *pecc;
> > +    uint32_t pec_pci_base;
> > +    char name[64];
> > +
> > +    assert(phb->stack);
> > +
> > +    pec = phb->stack->pec;
> > +    pecc = PNV_PHB4_PEC_GET_CLASS(pec);
> > +    pec_pci_base = pecc->xscom_pci_base(pec);
> > +
> > +    /* PHB pass-through */
> > +    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
> > +             pec->chip_id, pec->index, phb->stack->stack_no);
> > +    pnv_xscom_region_init(&phb->stack->phb_regs_mr, OBJECT(phb),
> > +                          &pnv_phb4_xscom_ops, phb, name, 0x40);
> > +
> > +    pnv_xscom_add_subregion(pec->chip,
> > +                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> > +                            0x40 * phb->stack->stack_no,
> > +                            &phb->stack->phb_regs_mr);
> > +}
> > +
> >   static void pnv_phb4_instance_init(Object *obj)
> >   {
> >       PnvPHB4 *phb = PNV_PHB4(obj);
> > @@ -1223,6 +1248,8 @@ static void pnv_phb4_realize(DeviceState *dev, Error 
> > **errp)
> >       memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb4_reg_ops, phb,
> >                             name, 0x2000);
> > +    pnv_phb4_init_xscom_passthrough(phb);
> > +
> >       /*
> >        * PHB4 doesn't support IO space. However, qemu gets very upset if
> >        * we don't have an IO region to anchor IO BARs onto so we just
> > diff --git a/hw/pci-host/pnv_phb4_pec.c b/hw/pci-host/pnv_phb4_pec.c
> > index 042dc0b775..5e02a51f04 100644
> > --- a/hw/pci-host/pnv_phb4_pec.c
> > +++ b/hw/pci-host/pnv_phb4_pec.c
> > @@ -571,12 +571,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error 
> > **errp)
> >                             &pnv_pec_stk_pci_xscom_ops, stack, name,
> >                             PHB4_PEC_PCI_STK_REGS_COUNT);
> > -    /* PHB pass-through */
> > -    snprintf(name, sizeof(name), "xscom-pec-%d.%d-pci-stack-%d-phb",
> > -             pec->chip_id, pec->index, stack->stack_no);
> > -    pnv_xscom_region_init(&stack->phb_regs_mr, OBJECT(stack->phb),
> > -                          &pnv_phb4_xscom_ops, stack->phb, name, 0x40);
> > -
> >       if (stack->phb && !sysbus_realize(SYS_BUS_DEVICE(stack->phb), errp)) {
> >           return;
> >       }
> > @@ -591,10 +585,6 @@ static void pnv_pec_stk_realize(DeviceState *dev, Error 
> > **errp)
> >       pnv_xscom_add_subregion(chip,
> >                               pec_pci_base + 0x40 * (stack->stack_no + 1),
> >                               &stack->pci_regs_mr);
> > -    pnv_xscom_add_subregion(chip,
> > -                            pec_pci_base + PNV9_XSCOM_PEC_PCI_STK0 +
> > -                            0x40 * stack->stack_no,
> > -                            &stack->phb_regs_mr);
> >   }
>
>
> Can't we simply move the XSCOM init and mapping under phb4 realize routine ?

There's a lot of code to be moved when doing that but in theory we can. It would
simplify some things (e.g. we wouldn't need to check stack->phb != NULL in
phb4_update_maps() like we need to do in the next patch).

I also got cold feet about the repercussions of creating a stack without any 
XSCOM
mapped on it, but perhaps this is the right time to see if that's possible and, 
if not,
try to understand why.


Thanks,


Daniel



> Thanks,
>
> C.
>
>
> >   static Property pnv_pec_stk_properties[] = {