On Thu, Jan 24, 2019 at 01:48:36PM +1100, Alexey Kardashevskiy wrote: > > > On 23/01/2019 19:24, David Hildenbrand wrote: > > While looking at the s390x implementation, looks like spapr has a > > similar BUG when building the topology. > > > > The primary bus number corresponds always to the bus number of the > > bus the bridge is attached to. > > > > Right now, if we have two bridges attached to the same bus (e.g. root > > bus) this is however not the case. The first bridge will have primary > > bus 0, the second bridge primary bus 1, which is wrong. Fix the assignment. > > > > While at it, drop setting the PCI_SUBORDINATE_BUS temporarily to 0xff. > > Setting it temporarily to that value (as discussed e.g. in [1]), is > > only relevant for a running system that probes the buses. The value is > > effectively unused for us just doing a DFS. > > What is DFS? > > > > > [1] http://www.science.unitn.it/~fiorella/guidelinux/tlk/node76.html > > > > Note: Is hotplug of bridges supported? I can't find where the topology > > is fixed up when hotplugging a PCI bridge. (e.g. bus numbers assigned > > and PCI_SUBORDINATE_BUS of path to the root updated). But maybe we are > > excluding bridges or this is not necessary for some reason. > > > > Signed-off-by: David Hildenbrand <da...@redhat.com> > > > Reviewed-by: Alexey Kardashevskiy <a...@ozlabs.ru>
Applied, thanks.
>
>
> > ---
> > hw/ppc/spapr_pci.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index b74f2632ec..5cdc98513d 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -2030,8 +2030,6 @@ static void spapr_phb_pci_enumerate_bridge(PCIBus
> > *bus, PCIDevice *pdev,
> > void *opaque)
> > {
> > unsigned int *bus_no = opaque;
> > - unsigned int primary = *bus_no;
> > - unsigned int subordinate = 0xff;
> > PCIBus *sec_bus = NULL;
> >
> > if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
> > @@ -2040,7 +2038,7 @@ static void spapr_phb_pci_enumerate_bridge(PCIBus
> > *bus, PCIDevice *pdev,
> > }
> >
> > (*bus_no)++;
> > - pci_default_write_config(pdev, PCI_PRIMARY_BUS, primary, 1);
> > + pci_default_write_config(pdev, PCI_PRIMARY_BUS, pci_dev_bus_num(pdev),
> > 1);
> > pci_default_write_config(pdev, PCI_SECONDARY_BUS, *bus_no, 1);
> > pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
> >
> > @@ -2049,7 +2047,6 @@ static void spapr_phb_pci_enumerate_bridge(PCIBus
> > *bus, PCIDevice *pdev,
> > return;
> > }
> >
> > - pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, subordinate, 1);
> > pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> > spapr_phb_pci_enumerate_bridge, bus_no);
> > pci_default_write_config(pdev, PCI_SUBORDINATE_BUS, *bus_no, 1);
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
signature.asc
Description: PGP signature
