Re: [PATCH 2/3] s390/pci: Use 32-bit fh outside of inline asm

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 10/7/26 10:46 AM, Gerd Bayer wrote:
> On Wed, 2026-10-07 at 11:19 +0000, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] s390/pci: infinite loop in zpci_refresh_trans on CC=2
>> --
>>
>> commit 1c8706bb963f0d551da7d8535f804ca9f9b4305a
>> Author: Gerd Bayer <gbayer@linux.ibm.com>
>>
>> s390/pci: Use 32-bit fh outside of inline asm
>>     
>> zpci_refresh_trans() declared the PCI function handle as a u64 type just
>> because the actual RPCIT machine instruction required that in a 64bit
>> register, while the function handle is architected as a 32bit type and
>> defined as such in the zpci_dev struct.
>>
>>> diff --git a/arch/s390/pci/pci_insn.c b/arch/s390/pci/pci_insn.c
>>> index 2ba787e2304b5..053afb3e7a990 100644
>>> --- a/arch/s390/pci/pci_insn.c
>>> +++ b/arch/s390/pci/pci_insn.c
>>> @@ -117,13 +117,14 @@ static inline u8 __rpcit(u64 fn, u64 addr, u64 range, u8 *status)
>>>  	return CC_TRANSFORM(cc);
>>>  }
>>>  
>>> -int zpci_refresh_trans(u64 fn, u64 addr, u64 range)
>>> +int zpci_refresh_trans(u32 fh, u64 addr, u64 range)
>>>  {
>>> +	u64 req = ZPCI_CREATE_REQ(fh, 0, 0);
>>>  	bool retried = false;
>>>  	u8 cc, status;
>>>  
>>>  	do {
>>> -		cc = __rpcit(fn, addr, range, &status);
>>> +		cc = __rpcit(req, addr, range, &status);
>>>  		if (cc == 2) {
>>>  			udelay(ZPCI_INSN_BUSY_DELAY);
>>
>> [Severity: High]
>> Does this retry loop risk causing an infinite loop or CPU stall when handling
>> large DMA spaces?
>>

This should really have been flagged as a pre-existing issue vs a new
regression, it is not a behavior introduced (or changed) by your patch.

>> When the s390x RPCIT instruction sets CC=2, it indicates that it cannot complete
>> the operation in the allotted time. It then updates the address and range
>> registers to reflect the remaining uncompleted portion.
> 
> I've re-read the relevant ISA documentation a couple times now. I don't
> find any mention of address and range being modified after RPCIT was
> executed - regardless of condition code or status.
> 
> Maybe @Niklas or @Matt have an idea where this allegation of "CC=2
> means partial completion and one should retry with an updated
> iova/range" comes from?

FWIW, it's not coming from QEMU -- we don't even have a CC=2 return case
for RPCIT today, and we never update the iova/range on return.

Perhaps, lacking access to the ISA documentation, it made an assumption
that the range would be updated to shorten the next RPCIT as this would
allow us to converge towards a smaller and smaller range each attempt to
potentially not hit the busy condition eventually - sounds logical anyway?

But I also find nothing that indicates the range will/should be updated
on CC=2 (or otherwise) from RPCIT; so it does just sound like a bad
assumption.

> 
>> However, in __rpcit(), addr_range is passed using an input-only constraint ("d"
>> instead of "+d"), and the arguments are passed by value:
>>
>> arch/s390/pci/pci_insn.c:__rpcit() {
>> 	union register_pair addr_range = {.even = addr, .odd = range};
>> 	...
>> 		: [addr_range] "d" (addr_range.pair)
>> }
>>
>> Because the hardware's partial progress updates to addr and range are discarded,
>> this loop continually re-invokes __rpcit() with the unmodified original
>> addr and range.

That matches your observation:  we indeed aren't taking updates to the
range in __rpcit().  But as you say: there aren't supposed to be any.

>>
>> If scanning the initial portion consistently exhausts the hardware time slice
>> (such as during IOMMU TLB flushes on devices with large DMA spaces in
>> zpci_refresh_all()), will the operation fail to advance and loop infinitely?
> 

That concern sounds valid at face value; if we tried something and it
couldn't be completed in time, from a linux perspective we are simply
trying it again without changing anything and hoping everything works
for the best this time.

> I see that all the PCI related instructions in this file may retry
> indefinitely (with delay) on CC=2. However, the architecture guarantees
> that at some point the instruction will end with any of the other
> condition codes.

I think that is the rub.  Sashiko couldn't possibly know that such an
architecture guarantee exists.

I suspect the question is also a bit theoretical: what happens if the
range is so big that it's impossible for the RPCIT to process it before
the CC2 trigger point due simply to how big the range provided is?  Then
you're guaranteed to hit it every time unless you 'chip away' at it by
shrinking the range after each attempt.

I would assume/hope that the SDMA-EDMA range limitation we impose on the
aperture size already ensures that it is easily possible to handle a
RPCIT from SDMA thru EDMA without tripping CC=2.
So then the CC=2 case becomes purely situational vs a guaranteed result
even for the largest possible RPCIT request.  That already makes it far
more reasonable to simply try it again.

So: assuming that architecture guarantee stands (we know RPCIT can
possibly complete for the largest possible range SDMA-EDMA, and we have
some architected guarantee that the loop will be broken otherwise e.g.
architecture says it won't keep giving us CC2s forever) then it sounds
like nothing to fix, but maybe worth a comment in a future patch that
explains these guarantees, based on what you find in the ISA.

I suppose we can consider whether linux should have its own redundancy
here or not in addition to those guarantees.  That sounds well beyond
the scope of this series and goes back to what I mentioned at the start:
 this is pre-existing bevahior and should not hold up this patch either way.

Thanks,
Matt