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

[Niklas Schnelle <schnelle@linux.ibm.com>]

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