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

[Farhan Ali <alifm@linux.ibm.com>]

On 9/17/2025 7:48 AM, Niklas Schnelle wrote:
> 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.

I don't think it hurts to add the check. I don't think we support any 
PCI devices on the platform with IORESOURCE_IO.


>
>> +			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?

Right, architecturally both size 0 and unset BAR value would indicate 
invalid and this check was meant for that. But I think just changing 
this to !zdev->bars[j].size should also be enough, as we already handle 
the 64bit BAR case below. Will change this.

Thanks Farhan

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