Re: [PATCH v2 1/2] PCI: Take multifunction devices into account when distributing resources

[Bjorn Helgaas <helgaas@kernel.org>]

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