Re: [PATCH v3 01/12] ppc/pnv: add PHB3 bus init helper

[Daniel Henrique Barboza]

On 6/24/22 10:44, Cédric Le Goater wrote:
> (Adding people who could help making the right change)
>
> On 6/24/22 10:49, Daniel Henrique Barboza wrote:
> > The PnvPHB3 bus init consists of initializing the pci_io and pci_mmio
> > regions, registering it via pci_register_root_bus() and then setup the
> > iommu.
> >
> > We'll want to init the bus from outside pnv_phb3.c when the bus is
> > removed from the PnvPHB3 device and put into a new parent PnvPHB device.
> > The new pnv_phb3_bus_init() helper will be used by the parent to init
> > the bus when using the PHB3 backend.
> >
> > Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> > ---
> >   hw/pci-host/pnv_phb3.c         | 39 ++++++++++++++++++++--------------
> >   include/hw/pci-host/pnv_phb3.h |  1 +
> >   2 files changed, 24 insertions(+), 16 deletions(-)
> >
> > diff --git a/hw/pci-host/pnv_phb3.c b/hw/pci-host/pnv_phb3.c
> > index d58d3c1701..058cbab555 100644
> > --- a/hw/pci-host/pnv_phb3.c
> > +++ b/hw/pci-host/pnv_phb3.c
> > @@ -986,6 +986,28 @@ static void pnv_phb3_instance_init(Object *obj)
> >   }
> > +void pnv_phb3_bus_init(DeviceState *dev, PnvPHB3 *phb)
> > +{
> > +    PCIHostState *pci = PCI_HOST_BRIDGE(dev);
> > +
> > +    /*
> > +     * PHB3 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
> > +     * initialize one which we never hook up to anything
> > +     */
> > +    memory_region_init(&phb->pci_io, OBJECT(phb), "pci-io", 0x10000);
> > +    memory_region_init(&phb->pci_mmio, OBJECT(phb), "pci-mmio",
> > +                       PCI_MMIO_TOTAL_SIZE);
>
>
> Could we change the root port settings with io-reserve=0 to remove
> the IO range ?


I don't think so, and I think this is what the comment right before refers to. 
Even
with io-reserve=0 I can't remove phb->pci_io. The code breaks in the middle of
the root port realize core code, pci_bridge_initfn(), when trying to create the
bridge windows via

    br->windows = pci_bridge_region_init(br);

There's no verification of io-reserve value (via res_reserve.io) influencing the
init of those regions. What I can see related to io-reserve, for example, is 
this
piece of code from gen_rp_realize() in gen_pcie_root_port.c:


    if (!grp->res_reserve.io) {
        pci_word_test_and_clear_mask(d->wmask + PCI_COMMAND,
                                     PCI_COMMAND_IO);
        d->wmask[PCI_IO_BASE] = 0;
        d->wmask[PCI_IO_LIMIT] = 0;
    }

Of course that this piece of code does nothing to avoid the segfault described
below.


I think this might be worth a investigation later on as a follow up. For now
I'd like to focus on pnv-phb changes (including the user created support)
before the freeze.


Thanks,


Daniel

> Thanks,
>
> C.
>
>
>
> > +    pci->bus = pci_register_root_bus(dev,
> > +                                     dev->id ? dev->id : NULL,
> > +                                     pnv_phb3_set_irq, pnv_phb3_map_irq, phb,
> > +                                     &phb->pci_mmio, &phb->pci_io,
> > +                                     0, 4, TYPE_PNV_PHB3_ROOT_BUS);
> > +
> > +    pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
> > +}
> > +
> >   static void pnv_phb3_realize(DeviceState *dev, Error **errp)
> >   {
> >       PnvPHB3 *phb = PNV_PHB3(dev);
> > @@ -1035,22 +1057,7 @@ static void pnv_phb3_realize(DeviceState *dev, Error 
> > **errp)
> >       memory_region_init_io(&phb->mr_regs, OBJECT(phb), &pnv_phb3_reg_ops, phb,
> >                             "phb3-regs", 0x1000);
> > -    /*
> > -     * PHB3 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
> > -     * initialize one which we never hook up to anything
> > -     */
> > -    memory_region_init(&phb->pci_io, OBJECT(phb), "pci-io", 0x10000);
> > -    memory_region_init(&phb->pci_mmio, OBJECT(phb), "pci-mmio",
> > -                       PCI_MMIO_TOTAL_SIZE);
> > -
> > -    pci->bus = pci_register_root_bus(dev,
> > -                                     dev->id ? dev->id : NULL,
> > -                                     pnv_phb3_set_irq, pnv_phb3_map_irq, phb,
> > -                                     &phb->pci_mmio, &phb->pci_io,
> > -                                     0, 4, TYPE_PNV_PHB3_ROOT_BUS);
> > -
> > -    pci_setup_iommu(pci->bus, pnv_phb3_dma_iommu, phb);
> > +    pnv_phb3_bus_init(dev, phb);
> >       pnv_phb_attach_root_port(pci, TYPE_PNV_PHB3_ROOT_PORT,
> >                                phb->phb_id, phb->chip_id);
> > diff --git a/include/hw/pci-host/pnv_phb3.h b/include/hw/pci-host/pnv_phb3.h
> > index af6ec83cf6..1375f18fc1 100644
> > --- a/include/hw/pci-host/pnv_phb3.h
> > +++ b/include/hw/pci-host/pnv_phb3.h
> > @@ -164,5 +164,6 @@ uint64_t pnv_phb3_reg_read(void *opaque, hwaddr off, 
> > unsigned size);
> >   void pnv_phb3_reg_write(void *opaque, hwaddr off, uint64_t val, unsigned 
> > size);
> >   void pnv_phb3_update_regions(PnvPHB3 *phb);
> >   void pnv_phb3_remap_irqs(PnvPHB3 *phb);
> > +void pnv_phb3_bus_init(DeviceState *dev, PnvPHB3 *phb);
> >   #endif /* PCI_HOST_PNV_PHB3_H */