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

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

[Matthew Rosato]

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@linux.ibm.com>
---
 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;
 	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;
 	fib->fmt0.isc = gisc;
 
 	/* Save some guest fib values in the host for later use */
-- 
2.53.0

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

[Niklas Schnelle]

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@linux.ibm.com>
> ---
>  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));

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

Thanks,
Niklas

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@linux.ibm.com>
>> ---
>>  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

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

[Niklas Schnelle]

On Fri, 2026-04-24 at 16:09 -0400, Matthew Rosato wrote:
> 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@linux.ibm.com>
> > > ---
> > >  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.

No strong feelings and I do agree about doing the calculation the same
as in zpci_set_airq(). Your variant also is understood without knowing
that aift->sbv->vector is an unsigned long*. So it's kind of a wash to
me.

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

Makes sense and great work! Feel free to add mu:

Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>

Thanks,
Niklas

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

[Christian Borntraeger]

Am 24.04.26 um 21:39 schrieb Matthew Rosato:
> 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@linux.ibm.com>

thanks applied.