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

Farhan Ali
2025-09-17T17:22:56Z

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