Re: [PATCH] KVM: s390: pci: Fix aisb calculation

[Matthew Rosato]

On 4/24/26 3:55 PM, Niklas Schnelle wrote:
> On Fri, 2026-04-24 at 15:39 -0400, Matthew Rosato wrote:
>> The current implementation of aisb calculation will erroneously index
>> via an unsigned long * as well as multiply by 8B for every 64-bits in
>> the offset; only one or the other is required.  This throws off aisb
>> calculations once the number of devices exceeds 64, and can result
>> in out-of-bounds access as well as failure to indicate summary bits
>> associated with those devices in guests.
>>
>> Fix this by converting to a physical address before applying the
>> offset, as is already done in arch/s390/pci/pci_irq.c.
>>
>> Fixes: 3c5a1b6f0a18 ("KVM: s390: pci: provide routines for enabling/disabling interrupt forwarding")
>> Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
>> ---
>>  arch/s390/kvm/pci.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>> index 86d93e8dddae..338171f9371b 100644
>> --- a/arch/s390/kvm/pci.c
>> +++ b/arch/s390/kvm/pci.c
>> @@ -166,7 +166,7 @@ static int kvm_zpci_set_airq(struct zpci_dev *zdev)
>>  	fib.fmt0.noi = airq_iv_end(zdev->aibv);
>>  	fib.fmt0.aibv = virt_to_phys(zdev->aibv->vector);
>>  	fib.fmt0.aibvo = 0;
>> -	fib.fmt0.aisb = virt_to_phys(aift->sbv->vector + (zdev->aisb / 64) * 8);
>> +	fib.fmt0.aisb = virt_to_phys(aift->sbv->vector) + (zdev->aisb / 64) * 8;
> 
> As you state, one or the other would work, wouldn't it be easy to read
> like below:
> 
>          fib.fmt0.aisb = virt_to_phys(aift->sbv->vector + (zdev->aisb / 64));

My rationale was to match exactly what zpci_set_airq() in
arch/s390/pci/pci_irq.c is doing (I tested that code at the same time to
ensure it produced the expected result as a comparison to the code
changed by this patch).

I'd rather all 3 places stay the same unless you have a strong opinion
on it.  I think the reason they ended up different in the first place is
because this code and the pci_irq.c code were updated independently for
V!=R.

> 
>>  	fib.fmt0.aisbo = zdev->aisb & 63;
>>  	fib.gd = zdev->gisa;
>>  
>> @@ -309,7 +309,7 @@ static int kvm_s390_pci_aif_enable(struct zpci_dev *zdev, struct zpci_fib *fib,
>>  
>>  	/* Update guest FIB for re-issue */
>>  	fib->fmt0.aisbo = zdev->aisb & 63;
>> -	fib->fmt0.aisb = virt_to_phys(aift->sbv->vector + (zdev->aisb / 64) * 8);
>> +	fib->fmt0.aisb = virt_to_phys(aift->sbv->vector) + (zdev->aisb / 64) * 8;
> 
> Same argument as above.
> 
>>  	fib->fmt0.isc = gisc;
>>  
>>  	/* Save some guest fib values in the host for later use */
> 
> Ouch, good catch! Was this kind of a follow up to the issue someone
> found upstream with a wrong calculation in the gait? Or did you just
> notice things break with more than 64 devices?

A follow-up of sorts.  Sashiko gave various outputs to that gait patch
that were unrelated to it; I'm looking into those things and it led me
to at least this issue which I did verify is a real bug.

Thanks,
Matt