[PATCH v4 04/10] s390/pci: Add architecture specific resource/bus address translation

Farhan Ali posted 10 patches 4 months, 2 weeks ago
There is a newer version of this series
[PATCH v4 04/10] s390/pci: Add architecture specific resource/bus address translation
Posted by Farhan Ali 4 months, 2 weeks ago
On s390 today we overwrite the PCI BAR resource address to either an
artificial cookie address or MIO address. However this address is different
from the bus address of the BARs programmed by firmware. The artificial
cookie address was created to index into an array of function handles
(zpci_iomap_start). The MIO (mapped I/O) addresses are provided by firmware
but maybe different from the bus address. This creates an issue when trying
to convert the BAR resource address to bus address using the generic
pcibios_resource_to_bus().

Implement an architecture specific pcibios_resource_to_bus() function to
correctly translate PCI BAR resource addresses to bus addresses for s390.
Similarly add architecture specific pcibios_bus_to_resource function to do
the reverse translation.

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---
 arch/s390/pci/pci.c       | 74 +++++++++++++++++++++++++++++++++++++++
 drivers/pci/host-bridge.c |  4 +--
 2 files changed, 76 insertions(+), 2 deletions(-)

diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index cd6676c2d602..3992ba44e1cf 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -264,6 +264,80 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
 	return 0;
 }
 
+void pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region,
+			     struct resource *res)
+{
+	struct zpci_bus *zbus = bus->sysdata;
+	struct zpci_bar_struct *zbar;
+	struct zpci_dev *zdev;
+
+	region->start = res->start;
+	region->end = res->end;
+
+	for (int i = 0; i < ZPCI_FUNCTIONS_PER_BUS; i++) {
+		int j = 0;
+
+		zbar = NULL;
+		zdev = zbus->function[i];
+		if (!zdev)
+			continue;
+
+		for (j = 0; j < PCI_STD_NUM_BARS; j++) {
+			if (zdev->bars[j].res->start == res->start &&
+			    zdev->bars[j].res->end == res->end &&
+			    res->flags & IORESOURCE_MEM) {
+				zbar = &zdev->bars[j];
+				break;
+			}
+		}
+
+		if (zbar) {
+			/* only MMIO is supported */
+			region->start = zbar->val & PCI_BASE_ADDRESS_MEM_MASK;
+			if (zbar->val & PCI_BASE_ADDRESS_MEM_TYPE_64)
+				region->start |= (u64)zdev->bars[j + 1].val << 32;
+
+			region->end = region->start + (1UL << zbar->size) - 1;
+			return;
+		}
+	}
+}
+
+void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+			     struct pci_bus_region *region)
+{
+	struct zpci_bus *zbus = bus->sysdata;
+	struct zpci_dev *zdev;
+	resource_size_t start, end;
+
+	res->start = region->start;
+	res->end = region->end;
+
+	for (int i = 0; i < ZPCI_FUNCTIONS_PER_BUS; i++) {
+		zdev = zbus->function[i];
+		if (!zdev || !zdev->has_resources)
+			continue;
+
+		for (int j = 0; j < PCI_STD_NUM_BARS; j++) {
+			if (!zdev->bars[j].size)
+				continue;
+
+			/* only MMIO is supported */
+			start = zdev->bars[j].val & PCI_BASE_ADDRESS_MEM_MASK;
+			if (zdev->bars[j].val & PCI_BASE_ADDRESS_MEM_TYPE_64)
+				start |= (u64)zdev->bars[j + 1].val << 32;
+
+			end = start + (1UL << zdev->bars[j].size) - 1;
+
+			if (start == region->start && end == region->end) {
+				res->start = zdev->bars[j].res->start;
+				res->end = zdev->bars[j].res->end;
+				return;
+			}
+		}
+	}
+}
+
 void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size,
 			   pgprot_t prot)
 {
diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index afa50b446567..56d62afb3afe 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -48,7 +48,7 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
 }
 EXPORT_SYMBOL_GPL(pci_set_host_bridge_release);
 
-void pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region,
+void __weak pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region,
 			     struct resource *res)
 {
 	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
@@ -73,7 +73,7 @@ static bool region_contains(struct pci_bus_region *region1,
 	return region1->start <= region2->start && region1->end >= region2->end;
 }
 
-void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
+void __weak pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
 			     struct pci_bus_region *region)
 {
 	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
-- 
2.43.0
Re: [PATCH v4 04/10] s390/pci: Add architecture specific resource/bus address translation
Posted by Niklas Schnelle 4 months, 1 week ago
On Wed, 2025-09-24 at 10:16 -0700, Farhan Ali wrote:
> On s390 today we overwrite the PCI BAR resource address to either an
> artificial cookie address or MIO address. However this address is different
> from the bus address of the BARs programmed by firmware. The artificial
> cookie address was created to index into an array of function handles
> (zpci_iomap_start). The MIO (mapped I/O) addresses are provided by firmware
> but maybe different from the bus address. This creates an issue when trying
> to convert the BAR resource address to bus address using the generic
> pcibios_resource_to_bus().
> 
> Implement an architecture specific pcibios_resource_to_bus() function to
> correctly translate PCI BAR resource addresses to bus addresses for s390.
> Similarly add architecture specific pcibios_bus_to_resource function to do
> the reverse translation.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
>  arch/s390/pci/pci.c       | 74 +++++++++++++++++++++++++++++++++++++++
>  drivers/pci/host-bridge.c |  4 +--
>  2 files changed, 76 insertions(+), 2 deletions(-)
> 

@Bjorn, interesting new development. This actually fixes a current
linux-next breakage for us. In linux-next commit 06b77d5647a4 ("PCI:
Mark resources IORESOURCE_UNSET when outside bridge windows") from Ilpo
(added) breaks PCI on s390 because the check he added in
__pci_read_base() doesn't find the resource because the BAR address
does not match our MIO / address cookie addresses. With this patch
added however the pcibios_bus_to_resource() in __pci_read_base()
converts  the region correctly and then Ilpo's check works. I was
looking at this code quite intensely today wondering about Benjamin's
comment if we do need to check for containment rather than exact match.
I concluded that I think it is fine as is and was about to give my R-b
before Gerd had tracked down the linux-next issue and I found that this
fixes it.

So now I wonder if we might want to pick this one already to fix the
linux-next regression? Either way I'd like to add my:

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>

Thanks,
Niklas
Re: [PATCH v4 04/10] s390/pci: Add architecture specific resource/bus address translation
Posted by Bjorn Helgaas 4 months, 1 week ago
On Thu, Oct 02, 2025 at 02:58:45PM +0200, Niklas Schnelle wrote:
> On Wed, 2025-09-24 at 10:16 -0700, Farhan Ali wrote:
> > On s390 today we overwrite the PCI BAR resource address to either an
> > artificial cookie address or MIO address. However this address is different
> > from the bus address of the BARs programmed by firmware. The artificial
> > cookie address was created to index into an array of function handles
> > (zpci_iomap_start). The MIO (mapped I/O) addresses are provided by firmware
> > but maybe different from the bus address. This creates an issue when trying
> > to convert the BAR resource address to bus address using the generic
> > pcibios_resource_to_bus().
> > 
> > Implement an architecture specific pcibios_resource_to_bus() function to
> > correctly translate PCI BAR resource addresses to bus addresses for s390.
> > Similarly add architecture specific pcibios_bus_to_resource function to do
> > the reverse translation.
> > 
> > Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> > ---
> >  arch/s390/pci/pci.c       | 74 +++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/host-bridge.c |  4 +--
> >  2 files changed, 76 insertions(+), 2 deletions(-)
> > 
> 
> @Bjorn, interesting new development. This actually fixes a current
> linux-next breakage for us. In linux-next commit 06b77d5647a4 ("PCI:
> Mark resources IORESOURCE_UNSET when outside bridge windows") from Ilpo
> (added) breaks PCI on s390 because the check he added in
> __pci_read_base() doesn't find the resource because the BAR address
> does not match our MIO / address cookie addresses. With this patch
> added however the pcibios_bus_to_resource() in __pci_read_base()
> converts  the region correctly and then Ilpo's check works. I was
> looking at this code quite intensely today wondering about Benjamin's
> comment if we do need to check for containment rather than exact match.
> I concluded that I think it is fine as is and was about to give my R-b
> before Gerd had tracked down the linux-next issue and I found that this
> fixes it.
> 
> So now I wonder if we might want to pick this one already to fix the
> linux-next regression? Either way I'd like to add my:
> 
> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>

Hmmm, thanks for the report.  I'm about ready to send the pull
request, and I hate to include something that is known to break s390
and would require a fix before v6.18.  At the same time, I hate to add
non-trivial code, including more weak functions, this late in the
window.

06b77d5647a4 ("PCI: Mark resources IORESOURCE_UNSET when outside
bridge windows") fixes some bogus messages, but I'm not sure that it's
actually a functional change.  So maybe the simplest at this point
would be to defer that commit until we can do it and the s390 change
together.

Bjorn
Re: [PATCH v4 04/10] s390/pci: Add architecture specific resource/bus address translation
Posted by Niklas Schnelle 4 months, 1 week ago
On Thu, 2025-10-02 at 12:00 -0500, Bjorn Helgaas wrote:
> On Thu, Oct 02, 2025 at 02:58:45PM +0200, Niklas Schnelle wrote:
> > On Wed, 2025-09-24 at 10:16 -0700, Farhan Ali wrote:
> > > On s390 today we overwrite the PCI BAR resource address to either an
> > > artificial cookie address or MIO address. However this address is different
> > > from the bus address of the BARs programmed by firmware. The artificial
> > > cookie address was created to index into an array of function handles
> > > (zpci_iomap_start). The MIO (mapped I/O) addresses are provided by firmware
> > > but maybe different from the bus address. This creates an issue when trying
> > > to convert the BAR resource address to bus address using the generic
> > > pcibios_resource_to_bus().
> > > 
> > > Implement an architecture specific pcibios_resource_to_bus() function to
> > > correctly translate PCI BAR resource addresses to bus addresses for s390.
> > > Similarly add architecture specific pcibios_bus_to_resource function to do
> > > the reverse translation.
> > > 
> > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> > > ---
> > >  arch/s390/pci/pci.c       | 74 +++++++++++++++++++++++++++++++++++++++
> > >  drivers/pci/host-bridge.c |  4 +--
> > >  2 files changed, 76 insertions(+), 2 deletions(-)
> > > 
> > 
> > @Bjorn, interesting new development. This actually fixes a current
> > linux-next breakage for us. In linux-next commit 06b77d5647a4 ("PCI:
> > Mark resources IORESOURCE_UNSET when outside bridge windows") from Ilpo
> > (added) breaks PCI on s390 because the check he added in
> > __pci_read_base() doesn't find the resource because the BAR address
> > does not match our MIO / address cookie addresses. With this patch
> > added however the pcibios_bus_to_resource() in __pci_read_base()
> > converts  the region correctly and then Ilpo's check works. I was
> > looking at this code quite intensely today wondering about Benjamin's
> > comment if we do need to check for containment rather than exact match.
> > I concluded that I think it is fine as is and was about to give my R-b
> > before Gerd had tracked down the linux-next issue and I found that this
> > fixes it.
> > 
> > So now I wonder if we might want to pick this one already to fix the
> > linux-next regression? Either way I'd like to add my:
> > 
> > Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> 
> Hmmm, thanks for the report.  I'm about ready to send the pull
> request, and I hate to include something that is known to break s390
> and would require a fix before v6.18.  At the same time, I hate to add
> non-trivial code, including more weak functions, this late in the
> window.
> 
> 06b77d5647a4 ("PCI: Mark resources IORESOURCE_UNSET when outside
> bridge windows") fixes some bogus messages, but I'm not sure that it's
> actually a functional change.  So maybe the simplest at this point
> would be to defer that commit until we can do it and the s390 change
> together.
> 
> Bjorn

Yeah that makes a lot of sense. I agree this change is not completely
trivial. Might have been a little enthusiastic with it reviving PCI
support from the dead on linux-next. If the other patch is not an
important fix and Ilpo seems to agree, then simply reverting it is the
safe solution.

Thanks,
Niklas
Re: [PATCH v4 04/10] s390/pci: Add architecture specific resource/bus address translation
Posted by Ilpo Järvinen 4 months, 1 week ago
On Thu, 2 Oct 2025, Bjorn Helgaas wrote:

> On Thu, Oct 02, 2025 at 02:58:45PM +0200, Niklas Schnelle wrote:
> > On Wed, 2025-09-24 at 10:16 -0700, Farhan Ali wrote:
> > > On s390 today we overwrite the PCI BAR resource address to either an
> > > artificial cookie address or MIO address. However this address is different
> > > from the bus address of the BARs programmed by firmware. The artificial
> > > cookie address was created to index into an array of function handles
> > > (zpci_iomap_start). The MIO (mapped I/O) addresses are provided by firmware
> > > but maybe different from the bus address. This creates an issue when trying
> > > to convert the BAR resource address to bus address using the generic
> > > pcibios_resource_to_bus().
> > > 
> > > Implement an architecture specific pcibios_resource_to_bus() function to
> > > correctly translate PCI BAR resource addresses to bus addresses for s390.
> > > Similarly add architecture specific pcibios_bus_to_resource function to do
> > > the reverse translation.
> > > 
> > > Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> > > ---
> > >  arch/s390/pci/pci.c       | 74 +++++++++++++++++++++++++++++++++++++++
> > >  drivers/pci/host-bridge.c |  4 +--
> > >  2 files changed, 76 insertions(+), 2 deletions(-)
> > > 
> > 
> > @Bjorn, interesting new development. This actually fixes a current
> > linux-next breakage for us. In linux-next commit 06b77d5647a4 ("PCI:
> > Mark resources IORESOURCE_UNSET when outside bridge windows") from Ilpo
> > (added) breaks PCI on s390 because the check he added in
> > __pci_read_base() doesn't find the resource because the BAR address
> > does not match our MIO / address cookie addresses. With this patch
> > added however the pcibios_bus_to_resource() in __pci_read_base()
> > converts  the region correctly and then Ilpo's check works. I was
> > looking at this code quite intensely today wondering about Benjamin's
> > comment if we do need to check for containment rather than exact match.
> > I concluded that I think it is fine as is and was about to give my R-b
> > before Gerd had tracked down the linux-next issue and I found that this
> > fixes it.
> > 
> > So now I wonder if we might want to pick this one already to fix the
> > linux-next regression? Either way I'd like to add my:
> > 
> > Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> 
> Hmmm, thanks for the report.  I'm about ready to send the pull
> request, and I hate to include something that is known to break s390
> and would require a fix before v6.18.  At the same time, I hate to add
> non-trivial code, including more weak functions, this late in the
> window.
> 
> 06b77d5647a4 ("PCI: Mark resources IORESOURCE_UNSET when outside
> bridge windows") fixes some bogus messages, but I'm not sure that it's
> actually a functional change.  So maybe the simplest at this point
> would be to defer that commit until we can do it and the s390 change
> together.

Hi,

I didn't notice any issues because of the conflict messages, but then, I 
didn't look very deeply into what those pnp things were as it seemed bug 
in PCI core we want to fix anyway.

Deferring the commit 06b77d5647a4 would be prudent as there seems to be 
another problem in Geert's case discussed in the other thread. Even this 
short time in next has already served us well by exposing things that need 
fixing so better to wait until we've known things resolved.

-- 
 i.