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

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 <[email protected]>

this looks too complicated for a fix. Now what is the semantic of this?
This is used for migration purposes, and the doc says:

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)