Re: [PATCH v1 03/11] KVM: s390: Fix get_all_floating_irqs()

[Claudio Imbrenda <imbrenda@linux.ibm.com>]

On Wed, 12 Aug 2026 09:11:11 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 11.08.26 um 17:56 schrieb Claudio Imbrenda:
> > When attempting to report all pending floating interrupt to userspace,
> > the GISA IPM bits are atomically tested and cleared, and the
> > corresponding interrupt description is written in the output buffer. If
> > the output buffer is too small, an error is returned to userspace, but
> > the GISA IPM bits are now lost.
> > 
> > Fix by moving the GISA test at the end of the function, and keeping
> > track of which bits have been cleared. In case of error, set the bits
> > again, so they are not lost.
> > 
> > Fixes: 24160af6cb28 ("KVM: s390: add GISA interrupts to FLIC ioctl interface")
> > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com>  
> 
> this looks too complicated for a fix. Now what is the semantic of this?
> This is used for migration purposes, and the doc says:

that's exactly what I did after I read the documentation more carefully

> 
> Documentation/virt/kvm/devices/s390_flic.rst
> 
>    KVM_DEV_FLIC_GET_ALL_IRQS
>      Copies all floating interrupts into a buffer provided by userspace.
> [...]
>      All interrupts remain pending, i.e. are not deleted from the list of
>      currently pending interrupts.
> [...]
> 
> So even the success case is wrong. Why not simply add a new helper that
> reads the GISA without clearing the bits?
> 
> static inline int gisa_test_ipm_gisc(struct kvm_s390_gisa *gisa, u32 gisc)
> {
>        return test_bit_inv(IPM_BIT_OFFSET + gisc, (unsigned long *) gisa);
> }
> 
> 
> 
> 
> > ---
> >   arch/s390/kvm/interrupt.c | 93 ++++++++++++++++++---------------------
> >   1 file changed, 44 insertions(+), 49 deletions(-)
> > 
> > diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> > index 6b3f97a7513b..30963e05e0e6 100644
> > --- a/arch/s390/kvm/interrupt.c
> > +++ b/arch/s390/kvm/interrupt.c
> > @@ -2211,15 +2211,14 @@ void kvm_s390_clear_float_irqs(struct kvm *kvm)
> >   static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
> >   {
> >   	struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
> > +	struct kvm_s390_irq *buf __free(kvfree) = NULL;
> >   	struct kvm_s390_interrupt_info *inti;
> >   	struct kvm_s390_float_interrupt *fi;
> > -	struct kvm_s390_irq *buf;
> >   	struct kvm_s390_irq *irq;
> > +	unsigned int tmp = 0;
> >   	int max_irqs;
> > -	int ret = 0;
> >   	int n = 0;
> >   	int i;
> > -	unsigned long flags;
> >   
> >   	if (len > KVM_S390_FLIC_MAX_BUFFER || len == 0)
> >   		return -EINVAL;
> > @@ -2235,14 +2234,48 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
> >   
> >   	max_irqs = len / sizeof(struct kvm_s390_irq);
> >   
> > +	fi = &kvm->arch.float_int;
> > +	scoped_guard(spinlock_irqsave, &fi->lock) {
> > +		for (i = 0; i < FIRQ_LIST_COUNT; i++) {
> > +			list_for_each_entry(inti, &fi->lists[i], list) {
> > +				/* signal userspace to try again */
> > +				if (n == max_irqs)
> > +					return -ENOMEM;
> > +				inti_to_irq(inti, &buf[n]);
> > +				n++;
> > +			}
> > +		}
> > +		if (test_bit(IRQ_PEND_EXT_SERVICE, &fi->pending_irqs) ||
> > +		    test_bit(IRQ_PEND_EXT_SERVICE_EV, &fi->pending_irqs)) {
> > +			/* signal userspace to try again */
> > +			if (n == max_irqs)
> > +				return -ENOMEM;
> > +			irq = (struct kvm_s390_irq *)&buf[n];
> > +			irq->type = KVM_S390_INT_SERVICE;
> > +			irq->u.ext = fi->srv_signal;
> > +			n++;
> > +		}
> > +		if (test_bit(IRQ_PEND_MCHK_REP, &fi->pending_irqs)) {
> > +			/* signal userspace to try again */
> > +			if (n == max_irqs)
> > +				return -ENOMEM;
> > +			irq = (struct kvm_s390_irq *)&buf[n];
> > +			irq->type = KVM_S390_MCHK;
> > +			irq->u.mchk = fi->mchk;
> > +			n++;
> > +		}
> > +	}
> >   	if (gi->origin && gisa_get_ipm(gi->origin)) {
> >   		for (i = 0; i <= MAX_ISC; i++) {
> >   			if (n == max_irqs) {
> > +				/* restore removed bits if returning failure */
> > +				__atomic_or(tmp, (void *)&gi->origin->ipm);
> >   				/* signal userspace to try again */
> > -				ret = -ENOMEM;
> > -				goto out_nolock;
> > +				return -ENOMEM;
> >   			}
> >   			if (gisa_tac_ipm_gisc(gi->origin, i)) {
> > +				/* set aside the bits we cleared */
> > +				tmp |= 1 << (31 - i);
> >   				irq = (struct kvm_s390_irq *) &buf[n];
> >   				irq->type = KVM_S390_INT_IO(1, 0, 0, 0);
> >   				irq->u.io.io_int_word = isc_to_int_word(i);
> > @@ -2250,53 +2283,15 @@ static int get_all_floating_irqs(struct kvm *kvm, u8 __user *usrbuf, u64 len)
> >   			}
> >   		}
> >   	}
> > -	fi = &kvm->arch.float_int;
> > -	spin_lock_irqsave(&fi->lock, flags);
> > -	for (i = 0; i < FIRQ_LIST_COUNT; i++) {
> > -		list_for_each_entry(inti, &fi->lists[i], list) {
> > -			if (n == max_irqs) {
> > -				/* signal userspace to try again */
> > -				ret = -ENOMEM;
> > -				goto out;
> > -			}
> > -			inti_to_irq(inti, &buf[n]);
> > -			n++;
> > -		}
> > -	}
> > -	if (test_bit(IRQ_PEND_EXT_SERVICE, &fi->pending_irqs) ||
> > -	    test_bit(IRQ_PEND_EXT_SERVICE_EV, &fi->pending_irqs)) {
> > -		if (n == max_irqs) {
> > -			/* signal userspace to try again */
> > -			ret = -ENOMEM;
> > -			goto out;
> > -		}
> > -		irq = (struct kvm_s390_irq *) &buf[n];
> > -		irq->type = KVM_S390_INT_SERVICE;
> > -		irq->u.ext = fi->srv_signal;
> > -		n++;
> > -	}
> > -	if (test_bit(IRQ_PEND_MCHK_REP, &fi->pending_irqs)) {
> > -		if (n == max_irqs) {
> > -				/* signal userspace to try again */
> > -				ret = -ENOMEM;
> > -				goto out;
> > -		}
> > -		irq = (struct kvm_s390_irq *) &buf[n];
> > -		irq->type = KVM_S390_MCHK;
> > -		irq->u.mchk = fi->mchk;
> > -		n++;
> > -}
> >   
> > -out:
> > -	spin_unlock_irqrestore(&fi->lock, flags);
> > -out_nolock:
> > -	if (!ret && n > 0) {
> > -		if (copy_to_user(usrbuf, buf, sizeof(struct kvm_s390_irq) * n))
> > -			ret = -EFAULT;
> > +	if (n > 0 && copy_to_user(usrbuf, buf, sizeof(struct kvm_s390_irq) * n)) {
> > +		/* restore removed bits if returning failure */
> > +		if (tmp)
> > +			__atomic_or(tmp, (void *)&gi->origin->ipm);
> > +		return -EFAULT;
> >   	}
> > -	vfree(buf);
> >   
> > -	return ret < 0 ? ret : n;
> > +	return n;
> >   }
> >   
> >   static int flic_ais_mode_get_all(struct kvm *kvm, struct kvm_device_attr *attr)  
>