| Message ID | 20211220155448.1233-3-pali@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [pciutils,1/4] lspci: Show 16/32/64 bit width for address ranges behind bridge | expand |
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. Martin
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
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?
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. Does this patch fix a problem? I'm not clear on what the benefit is. Bjorn
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? But in any case changes in kernel does not help lspci/libpci which is running on existing (unmodified) kernel. > Does this patch fix a problem? I'm not clear on what the benefit is. > > Bjorn 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.
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
On Thursday 20 January 2022 15:02:12 Bjorn Helgaas wrote: > 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 Usage is in patch 4/4. lspci with this change and also with patch 4/4 can distinguish between states: 1) PCI-to-PCI bridge does not support IO and 2) PCI-to-PCI bridge has enabled IO forwarding range set to 0x0000-0x0fff. Both these two states have IO_BASE and IO_LIMIT registers set to zeros. lspci currently decodes IO_BASE=IO_LIMIT=0x00 as IO forwarding range is enabled and set to range 0x0000-0x0fff.
On Thursday 20 January 2022 15:02:12 Bjorn Helgaas wrote: > 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. Hello Bjorn, I would like to remind this older issue. Did you have a time to audit usage of IORESOURCE_WINDOW? Some flag for resource forwarding windows in PCI-to-PCI bridges would really help userspace application to distinguish between IO/MEM BARs an IO/MEM forwarding windows.
On Fri, Nov 11, 2022 at 09:09:45PM +0100, Pali Rohár wrote: > On Thursday 20 January 2022 15:02:12 Bjorn Helgaas wrote: > > On Thu, Jan 20, 2022 at 09:45:05PM +0100, Pali Rohár wrote: [trimmed material; beginning of thread is at https://lore.kernel.org/r/20211220155448.1233-3-pali@kernel.org] > > > 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. > > Hello Bjorn, I would like to remind this older issue. Did you have a time > to audit usage of IORESOURCE_WINDOW? Some flag for resource forwarding > windows in PCI-to-PCI bridges would really help userspace application to > distinguish between IO/MEM BARs an IO/MEM forwarding windows. I had forgotten all about this issue. IIUC, the ultimate goal here is to help lspci distinguish between an I/O window that's disabled and one that's enabled at [io 0x0000-0x0fff]. I have not done the research to see whether it would be safe to set IORESOURCE_WINDOW for PCI-to-PCI bridge windows. I'm sorry if I left the impression that I intended to do that. I would welcome your help to do that. Bjorn
On Friday 11 November 2022 15:05:55 Bjorn Helgaas wrote: > On Fri, Nov 11, 2022 at 09:09:45PM +0100, Pali Rohár wrote: > > On Thursday 20 January 2022 15:02:12 Bjorn Helgaas wrote: > > > On Thu, Jan 20, 2022 at 09:45:05PM +0100, Pali Rohár wrote: > > [trimmed material; beginning of thread is at > https://lore.kernel.org/r/20211220155448.1233-3-pali@kernel.org] > > > > > 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. > > > > Hello Bjorn, I would like to remind this older issue. Did you have a time > > to audit usage of IORESOURCE_WINDOW? Some flag for resource forwarding > > windows in PCI-to-PCI bridges would really help userspace application to > > distinguish between IO/MEM BARs an IO/MEM forwarding windows. > > I had forgotten all about this issue. IIUC, the ultimate goal here > is to help lspci distinguish between an I/O window that's disabled and > one that's enabled at [io 0x0000-0x0fff]. > > I have not done the research to see whether it would be safe to set > IORESOURCE_WINDOW for PCI-to-PCI bridge windows. I'm sorry if I left > the impression that I intended to do that. I would welcome your help > to do that. > > Bjorn Ok, do you have some resources or other information at which I should look? I just do not know where to start or what to check for that research. I looked into kernel sources and the only places where is code checking for IORESOURCE_WINDOW is ACPI related: arch/arm64/kernel/pci.c and drivers/pnp/resource.c. And I do not fully understand how is ACPI connected with PCI resources at this level. Other places which check (lib/vsprintf.c and drivers/pnp/interface.c) just use it for printf-formats.
On Fri, Nov 11, 2022 at 10:48:16PM +0100, Pali Rohár wrote: > On Friday 11 November 2022 15:05:55 Bjorn Helgaas wrote: > > On Fri, Nov 11, 2022 at 09:09:45PM +0100, Pali Rohár wrote: > > > On Thursday 20 January 2022 15:02:12 Bjorn Helgaas wrote: > > > > On Thu, Jan 20, 2022 at 09:45:05PM +0100, Pali Rohár wrote: > > > > [trimmed material; beginning of thread is at > > https://lore.kernel.org/r/20211220155448.1233-3-pali@kernel.org] > > > > > > > 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. > > > > > > Hello Bjorn, I would like to remind this older issue. Did you have a time > > > to audit usage of IORESOURCE_WINDOW? Some flag for resource forwarding > > > windows in PCI-to-PCI bridges would really help userspace application to > > > distinguish between IO/MEM BARs an IO/MEM forwarding windows. > > > > I had forgotten all about this issue. IIUC, the ultimate goal here > > is to help lspci distinguish between an I/O window that's disabled and > > one that's enabled at [io 0x0000-0x0fff]. > > > > I have not done the research to see whether it would be safe to set > > IORESOURCE_WINDOW for PCI-to-PCI bridge windows. I'm sorry if I left > > the impression that I intended to do that. I would welcome your help > > to do that. > > Ok, do you have some resources or other information at which I should > look? I just do not know where to start or what to check for that > research. > > I looked into kernel sources and the only places where is code checking > for IORESOURCE_WINDOW is ACPI related: arch/arm64/kernel/pci.c and > drivers/pnp/resource.c. And I do not fully understand how is ACPI > connected with PCI resources at this level. Other places which check > (lib/vsprintf.c and drivers/pnp/interface.c) just use it for > printf-formats. Yeah, that's the kind of thing I have in mind. I can't remember if I had any specific concern. Bjorn
diff --git a/lib/access.c b/lib/access.c index b257849a685e..a33e067e2389 100644 --- a/lib/access.c +++ b/lib/access.c @@ -189,7 +189,7 @@ pci_reset_properties(struct pci_dev *d) } int -pci_fill_info_v35(struct pci_dev *d, int flags) +pci_fill_info_v38(struct pci_dev *d, int flags) { unsigned int uflags = flags; if (uflags & PCI_FILL_RESCAN) @@ -203,19 +203,21 @@ pci_fill_info_v35(struct pci_dev *d, int flags) } /* In version 3.1, pci_fill_info got new flags => versioned alias */ -/* In versions 3.2, 3.3, 3.4 and 3.5, the same has happened */ -STATIC_ALIAS(int pci_fill_info(struct pci_dev *d, int flags), pci_fill_info_v35(d, flags)); -DEFINE_ALIAS(int pci_fill_info_v30(struct pci_dev *d, int flags), pci_fill_info_v35); -DEFINE_ALIAS(int pci_fill_info_v31(struct pci_dev *d, int flags), pci_fill_info_v35); -DEFINE_ALIAS(int pci_fill_info_v32(struct pci_dev *d, int flags), pci_fill_info_v35); -DEFINE_ALIAS(int pci_fill_info_v33(struct pci_dev *d, int flags), pci_fill_info_v35); -DEFINE_ALIAS(int pci_fill_info_v34(struct pci_dev *d, int flags), pci_fill_info_v35); +/* In versions 3.2, 3.3, 3.4, 3.5 and 3.8, the same has happened */ +STATIC_ALIAS(int pci_fill_info(struct pci_dev *d, int flags), pci_fill_info_v38(d, flags)); +DEFINE_ALIAS(int pci_fill_info_v30(struct pci_dev *d, int flags), pci_fill_info_v38); +DEFINE_ALIAS(int pci_fill_info_v31(struct pci_dev *d, int flags), pci_fill_info_v38); +DEFINE_ALIAS(int pci_fill_info_v32(struct pci_dev *d, int flags), pci_fill_info_v38); +DEFINE_ALIAS(int pci_fill_info_v33(struct pci_dev *d, int flags), pci_fill_info_v38); +DEFINE_ALIAS(int pci_fill_info_v34(struct pci_dev *d, int flags), pci_fill_info_v38); +DEFINE_ALIAS(int pci_fill_info_v35(struct pci_dev *d, int flags), pci_fill_info_v38); SYMBOL_VERSION(pci_fill_info_v30, pci_fill_info@LIBPCI_3.0); SYMBOL_VERSION(pci_fill_info_v31, pci_fill_info@LIBPCI_3.1); SYMBOL_VERSION(pci_fill_info_v32, pci_fill_info@LIBPCI_3.2); SYMBOL_VERSION(pci_fill_info_v33, pci_fill_info@LIBPCI_3.3); SYMBOL_VERSION(pci_fill_info_v34, pci_fill_info@LIBPCI_3.4); -SYMBOL_VERSION(pci_fill_info_v35, pci_fill_info@@LIBPCI_3.5); +SYMBOL_VERSION(pci_fill_info_v35, pci_fill_info@LIBPCI_3.5); +SYMBOL_VERSION(pci_fill_info_v38, pci_fill_info@@LIBPCI_3.8); void pci_setup_cache(struct pci_dev *d, byte *cache, int len) diff --git a/lib/caps.c b/lib/caps.c index c3b918059fe1..70a41b836e31 100644 --- a/lib/caps.c +++ b/lib/caps.c @@ -129,7 +129,7 @@ pci_find_cap_nr(struct pci_dev *d, unsigned int id, unsigned int type, unsigned int target = (cap_number ? *cap_number : 0); unsigned int index = 0; - pci_fill_info_v35(d, ((type == PCI_CAP_NORMAL) ? PCI_FILL_CAPS : PCI_FILL_EXT_CAPS)); + pci_fill_info_v38(d, ((type == PCI_CAP_NORMAL) ? PCI_FILL_CAPS : PCI_FILL_EXT_CAPS)); for (c=d->first_cap; c; c=c->next) { diff --git a/lib/filter.c b/lib/filter.c index 573fb2810363..195f813193c4 100644 --- a/lib/filter.c +++ b/lib/filter.c @@ -129,7 +129,7 @@ pci_filter_match_v33(struct pci_filter *f, struct pci_dev *d) return 0; if (f->device >= 0 || f->vendor >= 0) { - pci_fill_info_v35(d, PCI_FILL_IDENT); + pci_fill_info_v38(d, PCI_FILL_IDENT); if ((f->device >= 0 && f->device != d->device_id) || (f->vendor >= 0 && f->vendor != d->vendor_id)) return 0; diff --git a/lib/internal.h b/lib/internal.h index 17c27e194a29..4df8dd70c2c6 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -74,6 +74,7 @@ int pci_fill_info_v32(struct pci_dev *, int flags) VERSIONED_ABI; int pci_fill_info_v33(struct pci_dev *, int flags) VERSIONED_ABI; int pci_fill_info_v34(struct pci_dev *, int flags) VERSIONED_ABI; int pci_fill_info_v35(struct pci_dev *, int flags) VERSIONED_ABI; +int pci_fill_info_v38(struct pci_dev *, int flags) VERSIONED_ABI; struct pci_property { struct pci_property *next; diff --git a/lib/libpci.ver b/lib/libpci.ver index e20c3f581c71..73f7fa71e357 100644 --- a/lib/libpci.ver +++ b/lib/libpci.ver @@ -82,3 +82,8 @@ LIBPCI_3.7 { global: pci_find_cap_nr; }; + +LIBPCI_3.8 { + global: + pci_fill_info; +}; diff --git a/lib/pci.h b/lib/pci.h index 814247691086..0ec7f211ca24 100644 --- a/lib/pci.h +++ b/lib/pci.h @@ -142,6 +142,9 @@ struct pci_dev { pciaddr_t flags[6]; /* PCI_IORESOURCE_* flags for regions */ pciaddr_t rom_flags; /* PCI_IORESOURCE_* flags for expansion ROM */ int domain; /* PCI domain (host bridge) */ + pciaddr_t bridge_base_addr[4]; /* Bridge base addresses (without flags) */ + pciaddr_t bridge_size[4]; /* Bridge sizes */ + pciaddr_t bridge_flags[4]; /* PCI_IORESOURCE_* flags for bridge addresses */ /* Fields used internally */ struct pci_access *access; @@ -205,6 +208,7 @@ char *pci_get_string_property(struct pci_dev *d, u32 prop) PCI_ABI; #define PCI_FILL_IO_FLAGS 0x1000 #define PCI_FILL_DT_NODE 0x2000 /* Device tree node */ #define PCI_FILL_IOMMU_GROUP 0x4000 +#define PCI_FILL_BRIDGE_BASES 0x8000 #define PCI_FILL_RESCAN 0x00010000 void pci_setup_cache(struct pci_dev *, u8 *cache, int len) PCI_ABI; diff --git a/lib/sysfs.c b/lib/sysfs.c index fb6424105e84..7c157a2688ad 100644 --- a/lib/sysfs.c +++ b/lib/sysfs.c @@ -148,19 +148,22 @@ sysfs_get_value(struct pci_dev *d, char *object, int mandatory) return -1; } -static void +static unsigned int sysfs_get_resources(struct pci_dev *d) { struct pci_access *a = d->access; char namebuf[OBJNAMELEN], buf[256]; + struct { pciaddr_t flags, base_addr, size; } lines[10]; + unsigned int done; FILE *file; int i; + done = 0; sysfs_obj_name(d, "resource", namebuf); file = fopen(namebuf, "r"); if (!file) a->error("Cannot open %s: %s", namebuf, strerror(errno)); - for (i = 0; i < 7; i++) + for (i = 0; i < 7+6+4+1; i++) { unsigned long long start, end, size, flags; if (!fgets(buf, sizeof(buf), file)) @@ -177,16 +180,50 @@ sysfs_get_resources(struct pci_dev *d) flags &= PCI_ADDR_FLAG_MASK; d->base_addr[i] = start | flags; d->size[i] = size; + done |= PCI_FILL_BASES | PCI_FILL_SIZES | PCI_FILL_IO_FLAGS; } - else + else if (i == 6) { d->rom_flags = flags; flags &= PCI_ADDR_FLAG_MASK; d->rom_base_addr = start | flags; d->rom_size = size; + done |= PCI_FILL_ROM_BASE; } + 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) + { + int offset = (i == 7+6+4) ? 6 : 0; + for (i = 0; i < 4; i++) + { + d->bridge_flags[i] = lines[offset+i].flags; + d->bridge_base_addr[i] = lines[offset+i].base_addr; + d->bridge_size[i] = lines[offset+i].size; + } + done |= PCI_FILL_BRIDGE_BASES; } fclose(file); + return done; } static void sysfs_scan(struct pci_access *a) @@ -316,10 +353,9 @@ sysfs_fill_info(struct pci_dev *d, unsigned int flags) d->irq = sysfs_get_value(d, "irq", 1); done |= PCI_FILL_IRQ; } - if (flags & (PCI_FILL_BASES | PCI_FILL_ROM_BASE | PCI_FILL_SIZES | PCI_FILL_IO_FLAGS)) + if (flags & (PCI_FILL_BASES | PCI_FILL_ROM_BASE | PCI_FILL_SIZES | PCI_FILL_IO_FLAGS | PCI_FILL_BRIDGE_BASES)) { - sysfs_get_resources(d); - done |= PCI_FILL_BASES | PCI_FILL_ROM_BASE | PCI_FILL_SIZES | PCI_FILL_IO_FLAGS; + done |= sysfs_get_resources(d); } }