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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Tue, Nov 08, 2022 at 03:11:30PM -0600, Bjorn Helgaas wrote:
> On Thu, Nov 03, 2022 at 12:32:53PM +0200, Mika Westerberg wrote:
> > It is possible to have PCIe switch upstream port a multifunction device.
> 
> I can't quite parse this.  I guess the point is that a Switch Upstream
> Port may be one of the functions of a multifunction device?

Yes.

> > 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)
> > and this leads to an issue that Jonathan reported. He runs QEMU with
> > the following topoology (QEMU parameters):
> > 
> >  -device pcie-root-port,port=0,id=root_port13,chassis=0,slot=2	\
> >  -device x3130-upstream,id=sw1,bus=root_port13,multifunction=on	\
> >  -device e1000,bus=root_port13,addr=0.1 			\
> >  -device xio3130-downstream,id=fun1,bus=sw1,chassis=0,slot=3	\
> >  -device e1000,bus=fun1
> > 
> > The first e1000 NIC here is another function in the switch upstream
> > port. This leads to following errors:
> > 
> >   pci 0000:00:04.0: bridge window [mem 0x10200000-0x103fffff] to [bus 02-04]
> >   pci 0000:02:00.0: bridge window [mem 0x10200000-0x103fffff] to [bus 03-04]
> >   pci 0000:02:00.1: BAR 0: failed to assign [mem size 0x00020000]
> >   e1000 0000:02:00.1: can't ioremap BAR 0: [??? 0x00000000 flags 0x0]
> > 
> > Fix this by taking into account the possible multifunction devices when
> > uptream port resources are distributed.
> 
> Can you include the link to Jonathan's report?

Sure I will.

> > Reported-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> > Hi,
> > 
> > This is the formal patch that resulted from the discussion here:
> > 
> > https://lore.kernel.org/linux-pci/20220905080232.36087-5-mika.westerberg@linux.intel.com/T/#m724289d0ee0c1ae07628744c283116e60efaeaf1
> > 
> > Only change from that version is that we loop through all resources of
> > the multifunction device.
> > 
> >  drivers/pci/setup-bus.c | 63 ++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 59 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > index b4096598dbcb..c8787b187ee4 100644
> > --- a/drivers/pci/setup-bus.c
> > +++ b/drivers/pci/setup-bus.c
> > @@ -1830,10 +1830,65 @@ static void pci_bus_distribute_available_resources(struct pci_bus *bus,
> >  	 * bridges below.
> >  	 */
> >  	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 */
> 
> Do you have any citation or reasoning for this handy?  We had this
> assumption before, and it seems true that an Upstream Port must be
> Function 0 because a variety of Link-related things have to be in
> Function 0, e.g., ARI ASPM Control, ARI Clock PM, Autonomous Width
> Disable, Flit Mode Disable, LTR Enable, OBFF Enable, etc.  But those
> are all pretty oblique.
> 
> I guess it's better to have the comment than not, but is the sort of
> assertion that makes one wonder why it is true.

Unfortunately I was not able to find such reference from the PCIe spec :(

> > +		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
> > +		 * resources occupied by the other functions before
> > +		 * distributing the rest.
> 
> The space consumed by the peer functions of the Switch Upstream Port
> is determined by their BAR sizes, so I don't think we actually reduce
> that.
> 
> I *think* the point here is to reduce the space available for
> distribution by the amount required by the peers of the Switch
> Upstream Port, right?  I.e., "mmio" is the amount of space we have to
> distribute, and before splitting it across devices on the secondary
> bus, we need to save out the space required for peers on the primary
> bus.

Yes, I will update the comment accordingly.

> > +		 */
> > +		list_for_each_entry(dev, &bus->devices, bus_list) {
> > +			int i;
> > +
> > +			if (dev == bridge)
> > +				continue;
> > +
> > +			/*
> > +			 * It should be multifunction but if not stop
> > +			 * the distribution and bail out.
> > +			 */
> > +			if (!dev->multifunction)
> > +				return;
> 
> Why do we bother with this?  If there are multiple devices on the bus,
> don't we want to consider them all, regardless of whether
> dev->multifunction is set?  It seems like a gratuitous check.

Agreed, I will remove it.

> 
> > +			for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> > +				const struct resource *dev_res = &dev->resource[i];
> > +				resource_size_t dev_sz;
> > +				struct resource *b_res;
> > +
> > +				if (dev_res->flags & IORESOURCE_IO) {
> > +					b_res = &io;
> > +				} else if (dev_res->flags & IORESOURCE_MEM) {
> > +					if (dev_res->flags & IORESOURCE_PREFETCH)
> > +						b_res = &mmio_pref;
> > +					else
> > +						b_res = &mmio;
> > +				} else {
> > +					continue;
> > +				}
> > +
> > +				/* Size aligned to bridge window */
> > +				align = pci_resource_alignment(bridge, b_res);
> > +				dev_sz = ALIGN(resource_size(dev_res), align);
> > +
> > +				pci_dbg(dev, "%pR aligned to %llx\n", dev_res,
> 
> %#llx to avoid confusion and match other output.

OK.