Hi Mika,
On Fri, Nov 18, 2022 at 10:57:12AM +0200, Mika Westerberg wrote:
> On Thu, Nov 17, 2022 at 05:10:34PM -0600, Bjorn Helgaas wrote:
> > On Mon, Nov 14, 2022 at 01:59:52PM +0200, Mika Westerberg wrote:
> > > PCIe switch upstream port may be one of the functions of a multifunction
> > > device.
> >
> > I don't think this is specific to PCIe, is it? Can't we have a
> > multi-function device where one function is a conventional PCI bridge?
> > Actually, I don't think "multi-function" is relevant at all -- you
> > iterate over all the devices on the bus below. For PCIe, that likely
> > means multiple functions of the same device, but it could be separate
> > devices in conventional PCI.
>
> Yes it can be but I was trying to explain the problem we encountered and
> that's related to PCIe.
>
> I can leave this out if you think it is better that way.
Not necessarily, I'm just hoping this change is generic enough for all
PCI and PCIe topologies.
> > > The resource distribution code does not take this into account
> > > properly and therefore it expands the upstream port resource windows too
> > > much, not leaving space for the other functions (in the multifunction
> > > device)
> >
> > I guess the window expansion here is done by adjust_bridge_window()?
>
> Yes but the resources are distributed in
> pci_bus_distribute_available_resources().
Yep, sounds good, I was just confirming my understanding of the code.
The main point of this patch is to *reduce* the size of the windows to
leave room for peers of the bridge, so I had to look a bit to figure
out where they got expanded.
> > > if (hotplug_bridges + normal_bridges == 1) {
> > > - dev = list_first_entry(&bus->devices, struct pci_dev, bus_list);
> > > - if (dev->subordinate)
> > > - pci_bus_distribute_available_resources(dev->subordinate,
> > > - add_list, io, mmio, mmio_pref);
> > > + /* Upstream port must be the first */
> > > + bridge = list_first_entry(&bus->devices, struct pci_dev, bus_list);
> > > + if (!bridge->subordinate)
> > > + return;
> > > +
> > > + /*
> > > + * It is possible to have switch upstream port as a part of a
> > > + * multifunction device. For this reason reduce the space
> > > + * available for distribution by the amount required by the
> > > + * peers of the upstream port.
> > > + */
> > > + list_for_each_entry(dev, &bus->devices, bus_list) {
> >
> > It seems like maybe we ought to do this regardless of how many bridges
> > there are on the bus. Don't we always want to assign space to devices
> > on this bus before distributing the leftovers to downstream buses?
>
> Yes we do.
>
> > E.g., maybe this should be done before the adjust_bridge_window()
> > calls?
>
> With the current code it is clear that we deal with the upstream port.
> At least in PCIe it is not allowed to have anything else than downstream
> ports on that internal bus so the only case we would need to do this is
> the switch upstream port.
>
> Let me know if you still want me to move this before adjust_bridge_window()
> I can do that in v3. Probably needs a comment too.
Hmm, I don't know exactly how to do this, but I don't think this code
should be PCIe-specific, which I think means it shouldn't depend on
how many bridges are on the bus.
I guess the existing assumption that a bridge must be the first device
on the bus is a hidden assumption that this is PCIe. That might be a
mistake from the past.
I haven't tried it, but I wonder if we could reproduce the same
problem in a conventional PCI topology with qemu.
Bjorn
