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

From: Niklas Schnelle <[email-masked]>
Date: Thu, 02 Oct 2025 20:14:52 +0200

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 <[email-masked]>
> > > ---
> > >  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 <[email-masked]>
> 
> 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

---

From: Bjorn Helgaas <[email-masked]>
Date: Thu, 2 Oct 2025 12:00:13 -0500

Bjorn

---

From: Ilpo Järvinen <[email-masked]>
Date: Thu, 2 Oct 2025 20:16:42 +0300 (EEST)

> would be to defer that commit until we can do it and the s390 change

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.

---

From: Niklas Schnelle <[email-masked]>
Date: Thu, 02 Oct 2025 14:58:45 +0200

Thanks,
Niklas

---

From: Benjamin Block <[email-masked]>
Date: Thu, 2 Oct 2025 11:16:13 +0200

On Wed, Oct 01, 2025 at 10:12:03AM -0700, Farhan Ali wrote:
> 
> On 10/1/2025 8:15 AM, Benjamin Block wrote:
> > On Wed, Sep 24, 2025 at 10:16:19AM -0700, Farhan Ali wrote:
> >> @@ -1792,6 +1798,14 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
> >>   int pci_save_state(struct pci_dev *dev)
> >>   {
> >>   	int i;
> >> +	u32 val;
> >> +
> >> +	pci_read_config_dword(dev, PCI_COMMAND, &val);
> >> +	if (PCI_POSSIBLE_ERROR(val)) {
> >> +		pci_warn(dev, "Device config space inaccessible, will only be partially restored\n");
> >> +		return -EIO;
> >
> > Should it set `dev->state_saved` to `false`, to be on the save side?
> > Not sure whether we run a risk of restoring an old, outdated state otherwise.
> 
> AFAIU if the state_saved flag was set to true then any state that we 
> have saved should be valid and should be okay to be restored from. We 
> just want to avoid saving any invalid data.

Hmm, so I dug a bit more, and I see
void pci_restore_state(struct pci_dev *dev) {}
has `dev->state_saved = false` at the end, so I guess if a device is put into
suspend, and then later woken again, the flag gets reset every time.

And then there is also code like this:

	static pci_ers_result_t e1000_io_slot_reset(struct pci_dev *pdev)
	{
		...
		err = pci_enable_device_mem(pdev);
		if (err) {
			...
		} else {
			pdev->state_saved = true;
			pci_restore_state(pdev);

I don't know..

But I see Alex suggested this before.

> >> +	}
> >> +
> >>   	/* XXX: 100% dword access ok here? */
> >>   	for (i = 0; i < 16; i++) {
> >>   		pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
> >> @@ -1854,6 +1868,14 @@ static void pci_restore_config_space_range(struct pci_dev *pdev,
> >>   
> >>   static void pci_restore_config_space(struct pci_dev *pdev)
> >>   {
> >> +	if (!pdev->state_saved) {
> >> +		pci_warn(pdev, "No saved config space, restoring BARs\n");
> >> +		pci_restore_bars(pdev);
> >> +		pci_write_config_word(pdev, PCI_COMMAND,
> >> +				PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
> >
> > Is this really something that ought to be universally enabled? I thought this
> > depends on whether attached resources are IO and/or MEM?
> >
> > 	int pci_enable_resources(struct pci_dev *dev, int mask)
> > 	{
> > 		...
> > 		pci_dev_for_each_resource(dev, r, i) {
> > 			...
> > 			if (r->flags & IORESOURCE_IO)
> > 				cmd |= PCI_COMMAND_IO;
> > 			if (r->flags & IORESOURCE_MEM)
> > 				cmd |= PCI_COMMAND_MEMORY;
> > 		}
> > 		...
> > 	}
> >
> > Also IIRC, especially on s390, we never have IO resources?
> >
> > 	int zpci_setup_bus_resources(struct zpci_dev *zdev)
> > 	{
> > 		...
> > 		for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > 			...
> > 			/* only MMIO is supported */
> > 			flags = IORESOURCE_MEM;
> > 			if (zdev->bars[i].val & 8)
> > 				flags |= IORESOURCE_PREFETCH;
> > 			if (zdev->bars[i].val & 4)
> > 				flags |= IORESOURCE_MEM_64;
> > 			...
> > 		}
> > 		...
> > 	}
> >
> > So I guess this would have to have some form of the same logic as in
> > `pci_enable_resources()`, after restoring the BARs.
> >
> > Or am I missing something?
> 
> As per my understanding of the spec, setting both I/O Space and Memory 
> Space should be safe. The spec also mentions if a function doesn't 
> support IO/Memory space access it could hardwire the bit to zero. We 
> could add the logic to iterate through all the resources and set the 
> bits accordingly, but in this case trying a best effort restoration it 
> should be fine?
> 
> Also I didn't see any issues testing on s390x with the NVMe, RoCE and 
> NETD devices, but I could have missed something.

Well, just taking a coarse look at how some other PCI device drivers use the
Command register (this being non-s390 specific after all); some of them base
decisions on whether either/or these flags are set in config space. Now that
this sets both flags, this might have surprising side-effects.
    On the other hand, iterating over the resources might not even be enough
with some device-drivers, since they base their decision on whether to enable
either/or on other knowledge.
    So I don't know. Enabling both just in case might be a good compromise.

-- 
Best Regards, Benjamin Block        /        Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH    /   https://www.ibm.com/privacy
Vors. Aufs.-R.: Wolfgang Wendt         /        Geschäftsführung: David Faller
Sitz der Ges.: Böblingen     /    Registergericht: AmtsG Stuttgart, HRB 243294

---

From: Farhan Ali <[email-masked]>
Date: Wed, 1 Oct 2025 11:01:47 -0700

On 10/1/2025 9:04 AM, Benjamin Block wrote:
> On Thu, Sep 25, 2025 at 12:54:07PM +0200, Niklas Schnelle wrote:
>>> +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;
>> When we don't find a BAR matching the resource this would become the
>> region used. I'm not sure what a better value would be if we don't find
>> a match though and that should hopefully not happen in sensible uses.
>> Also this would keep the existing behavior so seems fine.
> I was wondering the same things, but I guess it matches what happens elsewhere
> as well, if no match is found
>
> 	void __weak pcibios_resource_to_bus(struct pci_bus *bus, struct pci_bus_region *region,
> 				     struct resource *res)
> 	{
> 		...
> 		resource_size_t offset = 0;
>
> 		resource_list_for_each_entry(window, &bridge->windows) {
> 			if (resource_contains(window->res, res)) {
> 				offset = window->offset;
> 				break;
> 			}
> 		}
>
> 		region->start = res->start - offset;
> 		region->end = res->end - offset;
> 	}
>
> So I guess that is fine.
>
> The thing I'm also unclear on is whether it is OK to "throw out" this whole
> logic about `resource_contains(window->res, res)` here and
> `region_contains(&bus_region, region)` in the other original function?
> I mean, the original function don't search for perfect matches, but also
> matches where are contained in a given resource/region, which is different
> from what we do here. Are we OK with not doing that at all?

I had thought about doing the range check, similar to 
resource_contains/region_contains rather than doing exact checks. But I 
think the way we expose the topology of the devices, the offset in our 
(s390x) case is zero. So I thought it should be safe to just doing exact 
match and might help us catch any issues if its not exact.

Thanks

Farhan

---

From: Farhan Ali <[email-masked]>
Date: Wed, 1 Oct 2025 10:12:03 -0700

Thanks

Farhan

>> +		return;