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

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

Thanks,
Niklas