Re: [PATCH v3 1/3] KVM: s390: Improve floating IRQ injection behavior

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect bitmask computation for floating I/O interrupts causes thundering herd performance regressions or lost wakeups.
--

--- Patch [1]: [PATCH v3 1/3] KVM: s390: Improve floating IRQ injection behavior ---
commit 8a0b82ce7a926dc2db73a2448ac2bcc105213707
Author: Janosch Frank <[email protected]>

KVM: s390: Improve floating IRQ injection behavior

This commit improves floating IRQ injection by checking if a VCPU has pending
IRQs instead of blindly waking up the last VCPU that went to sleep. This aims
to prevent deadlocks when a VCPU is sleeping without an open mask.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 8f24bcd1a6d36..3af060ec5be8e 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
[ ... ]
> @@ -1915,49 +1915,93 @@ static int __inject_io(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  	return 0;
>  }
>  
> +static u64 inti_to_irq_pend_mask(u64 type, int isc)
> +{
> +	switch (type) {
> +	case KVM_S390_MCHK:
> +		/* Only repressible machine checks are floating */
> +		return BIT(IRQ_PEND_MCHK_REP);
> +	case KVM_S390_INT_VIRTIO:
> +		return BIT(IRQ_PEND_VIRTIO);
> +	case KVM_S390_INT_SERVICE:
> +		return BIT(IRQ_PEND_EXT_SERVICE) |
> +		       BIT(IRQ_PEND_EXT_SERVICE_EV);
> +	case KVM_S390_INT_PFAULT_DONE:
> +		return BIT(IRQ_PEND_PFAULT_DONE);
> +	case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
> +		return isc_to_irq_type(isc);

[Severity: High]
Does this correctly return a bitmask? The other cases return BIT() masks, but
this returns a raw integer resulting from isc_to_irq_type().

Could this cause `deliverable_irqs(dst_vcpu) & irq_pend_mask` to check the
wrong bits or evaluate to 0 later in __floating_irq_kick(), leading to skipped
eligible VCPUs and a thundering herd where all VCPUs are woken up?

[ ... ]
> @@ -1974,6 +2018,8 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  		rc = __inject_pfault_done(kvm, inti);
>  		break;
>  	case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
> +		/* Grab isc here since __inject_io() might free inti */
> +		isc = isc_to_irq_type(int_word_to_isc(inti->io.io_int_word));

[Severity: High]
Are we applying isc_to_irq_type() twice for I/O interrupts?

This converts the ISC to an IRQ bit index before assigning it to `isc`. Later,
__floating_irq_kick() passes it to inti_to_irq_pend_mask(), which calls
isc_to_irq_type() a second time on the same value.

Since isc_to_irq_type() is implemented as `IRQ_PEND_IO_ISC_0 - isc`, calling it
twice will reverse the calculation and yield the original ISC value instead
of the expected IRQ bit index.

>  		rc = __inject_io(kvm, inti);
>  		break;
>  	default:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1