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

Niklas Schnelle
2025-09-17T14:48:14Z

On Thu, 2025-09-11 at 11:33 -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.
>

Nit: I'd prefer referring to functions with e.g.
pcibios_resource_to_bus() to make them easier to distinguish. Same also
below.

> Implement an architecture specific pcibios_resource_to_bus function to
> correctly translate PCI BAR resource address to bus address for s390.

Nit: I'd use the plural "addresses" above as we're dealing with a whole
range.

> 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 | 73 +++++++++++++++++++++++++++++++++++++++
> drivers/pci/host-bridge.c | 4 +--
> 2 files changed, 75 insertions(+), 2 deletions(-)
>
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index cd6676c2d602..5baeb5f6f674 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -264,6 +264,79 @@ 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) {
> + zbar = &zdev->bars[j];
> + break;
> + }
> + }
> +
> + if (zbar) {
> + /* only MMIO is supported */

Should the code that sets zbar check IORESOURCE_MEM on the res->flags
to ensure the above comment? Though zpci_setup_bus_resources() only
creates IORESOURCE_MEM resources so this would only be relevant if
someone uses a resource from some other source.

> + 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].val && !zdev->bars[j].size)
> + continue;

Shouldn't the above be '||'? I think both a 0 size and an unset bars
value would indicate invalid. zpci_setup_bus_resources() only checks 0
size so I think that would be enoug, no?

> +
> + /* 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;
> + }
> + }
> + }
> +}
> +
>

Overall the code makes sense to me. I think this hasn't caused issues
so far only because firmware has usually already set up the BAR
addresses for us.

Thanks,
Niklas