On Wed, 23 Jan 2019 11:32:48 +0100 David Hildenbrand <da...@redhat.com> wrote:
> On 23.01.19 11:26, Thomas Huth wrote: > > On 2019-01-22 13:51, David Hildenbrand wrote: > >> 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. Fix assignment. > >> > >> While at it > >> - Add a comment why we have to reassign durign every reset (which I > > > > s/durign/during/ > > > >> found to be supprising) > > > > s/supprising/surprising/ > > > >> - Drop setting the PCI_SUBORDINATE_BUS temporarily to 0xff. As we are > >> setting it via a DFS and not via a BFS (as discussed e.g. in [1]), this > >> is not necessary. The last number when we return is the highest > >> number. > > > > I think that explanation is slightly wrong / misleading. It's not about > > DFS vs. BFS, it's about guest code vs. QEMU code. If you do a bridge > > setup from the guest side, you've got to set the subordinate bus number > > to 0xff to make sure that the bridge forwards all config space accesses > > to the attached devices while you're scanning the devices that are > > attached to the bridge. > > But this code is not running in the guest, it is running in QEMU, and so > > it can access the config space of the attached devices directly via > > pci_default_write_config() - the write do not need to pass the parent > > bridge in this case, and thus the subordinate bus number in the bridge > > is ignored for the config space write access. > > Indeed, I phrased that better in the spapr/pci patch I sent, What about > this: > > " > 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. > > Also add a comment why we have to reassign during every reset (which I > found to be surprising. > > Please note that hotplugging of bridges is in general still broken, will > be fixed next. > " > > > > >> Please note that hotplugging of bridges is in general still broken, will > >> be fixed next. > >> > >> [1] http://www.science.unitn.it/~fiorella/guidelinux/tlk/node76.html > >> > >> Signed-off-by: David Hildenbrand <da...@redhat.com> > >> --- > >> hw/s390x/s390-pci-bus.c | 13 ++++++++----- > >> 1 file changed, 8 insertions(+), 5 deletions(-) > > > > With the commit message fixed: > > @Conny can you fix up when applying if there are no other comments? Sure, can do. Waiting for zpci maintainer ack :) > > > > > Reviewed-by: Thomas Huth <th...@redhat.com> > > > > Thanks Thomas! > >
