Re: [PATCH pciutils 3/4] libpci: Add support for filling bridge resources

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Jan 20, 2022 at 09:45:05PM +0100, Pali Rohár wrote:
> On Thursday 20 January 2022 14:33:52 Bjorn Helgaas wrote:
> > On Fri, Dec 31, 2021 at 11:27:48PM +0100, Pali Rohár wrote:
> > > On Sunday 26 December 2021 23:20:27 Pali Rohár wrote:
> > > > On Sunday 26 December 2021 23:13:11 Martin Mareš wrote:
> > > > > Hi!
> > > > > 
> > > > > > +      else if (i < 7+6+4)
> > > > > > +        {
> > > > > > +          /*
> > > > > > +           * If kernel was compiled without CONFIG_PCI_IOV option then after
> > > > > > +           * the ROM line for configured bridge device (that which had set
> > > > > > +           * subordinary bus number to non-zero value) are four additional lines
> > > > > > +           * which describe resources behind bridge. For PCI-to-PCI bridges they
> > > > > > +           * are: IO, MEM, PREFMEM and empty. For CardBus bridges they are: IO0,
> > > > > > +           * IO1, MEM0 and MEM1. For unconfigured bridges and other devices
> > > > > > +           * there is no additional line after the ROM line. If kernel was
> > > > > > +           * compiled with CONFIG_PCI_IOV option then after the ROM line and
> > > > > > +           * before the first bridge resource line are six additional lines
> > > > > > +           * which describe IOV resources. Read all remaining lines in resource
> > > > > > +           * file and based on the number of remaining lines (0, 4, 6, 10) parse
> > > > > > +           * resources behind bridge.
> > > > > > +           */
> > > > > > +          lines[i-7].flags = flags;
> > > > > > +          lines[i-7].base_addr = start;
> > > > > > +          lines[i-7].size = size;
> > > > > > +        }
> > > > > > +    }
> > > > > > +  if (i == 7+4 || i == 7+6+4)
> > > > > 
> > > > > This looks crazy: is there any other way how to tell what the
> > > > > bridge entries mean?  Checking the number of entries looks very
> > > > > brittle.
> > > > 
> > > > I do not know any other way. Just for reference, here is a link to
> > > > the function resource_show() and DEVICE_COUNT_RESOURCE enum:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/pci-sysfs.c?h=v5.15#n136
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/pci.h?h=v5.15#n94
> > > 
> > > I have also checked flags and there is no indication if resource is
> > > assigned on bridge as BAR or is forwarded behind the bridge.
> > > 
> > > Bjorn, Krzysztof: any idea if something better than checking number
> > > of entries in "resource" node can be used to determinate type of
> > > entry at specified line in "resource" node?
> > 
> > That *is* crazy.  I'm sorry that resource_show() works that way, and
> > that it gives no clue to identify BAR vs ROM vs IOV BAR vs CB window
> > vs regular bridge window.
> > 
> > It's conceivable that we could add "io_window" and "mem_window" files
> > or something similar.
> 
> Meanwhile I found out that in linux/ioport.h file is IORESOURCE_WINDOW
> constant with comment /* forwarded by bridge */
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/ioport.h?h=v5.15#n56
> 
> But apparently it is not set for resources behind PCI bridges and
> therefore it is not available in column of "resources" sysfs file.
> 
> So maybe instead of adding new sysfs files, it would be better way to
> implement this flag and export it in flags column of "resources" file
> for every row which belongs to resources behind bridges?

I looked at that, too.  Today we only set IORESOURCE_WINDOW for host
bridge windows.  Maybe it could be set for PCI-to-PCI bridge windows,
too.  Would have to audit users to make sure it wouldn't break
anything.

> But in any case changes in kernel does not help lspci/libpci which is
> running on existing (unmodified) kernel.

Of course.

> > Does this patch fix a problem?  I'm not clear on what the benefit is.
> 
> My patch for libpci fixes it, but via counting number of rows in
> "resources" sysfs file... which is crazy. But I do not see any other
> option how to do it via currently available kernel APIs.

The current subject and commit log are:

  libpci: Add support for filling bridge resources

  Extend libpci API and ABI to fill bridge resources from sysfs.

That doesn't give a reason why Martin should include this patch.  Does
it fix a problem?  Does it help lspci show more information?  If so,
what is the difference in output?

Bjorn