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

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

[Janosch Frank]

Multiple fixes for the floating IRQ handling triggered by the firq
kvm-unit-test sometimes failing for pv vms.

Only waking up cpus which are enabled, re-kicking after the
instruction notification for pv cpus and a general redistribution of
IRQs on entry (pending guidance from performance measurements).

The floating part of floating IRQs can make it difficult to make
absolutely sure that an IRQ is actually delivered, especially if the
OS doesn't regularly have the masks open. We might miss mask
enablements if the intervention bits don't get set properly and the
kicks might end up nowhere.

v3:
 - Separated code into two patches
 - Added a patch that fixes service injections for PV vms
 - Added IRQ filtering and kick all if no cpu is enabled
 - Fixed the IO pend type translation
v2:
 - Rebase onto master
 - Added IRQ type check before delivering

Janosch Frank (3):
  KVM: s390: Improve floating IRQ injection behavior
  KVM: s390: Kick PV cpus at the right time for service irqs
  KVM: s390: Add opportunistic floating IRQ injection

 arch/s390/include/asm/kvm_host.h |   1 +
 arch/s390/kvm/intercept.c        |   9 ++
 arch/s390/kvm/interrupt.c        | 156 ++++++++++++++++++++++++++-----
 arch/s390/kvm/kvm-s390.c         |   3 +
 arch/s390/kvm/kvm-s390.h         |   2 +
 5 files changed, 150 insertions(+), 21 deletions(-)

-- 
2.53.0

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

[Janosch Frank]

Floating IRQs can be handled by any VCPU that opened its masks. The
current design does not check if the mask is open when a floating IRQ
is injected via the FLIC. It will wakeup the last VCPU that went
sleeping hoping it's the correct one.

Improve this by checking if the VCPU has pending IRQs and if not try
to find another VCPU which can take the IRQ.

This is not the final fix around this topic but we'll eventually
inject a pending IRQ. The current code can easily deadlock a VM and
with this fix we work around that.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 86 ++++++++++++++++++++++++++++++---------
 1 file changed, 66 insertions(+), 20 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 8f24bcd1a6d3..3af060ec5be8 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);
+	default:
+		return 0;
+	}
+}
+
+/*
+ * Setup intervention masks to catch running vcpus that hopefully open
+ * their masks soonish and kick sleeping vcpus to motivate them to
+ * take IRQs.
+ */
+static void vcpu_intervention_kick(struct kvm_vcpu *vcpu, u64 type)
+{
+	/* make the VCPU drop out of the SIE, or wake it up if sleeping */
+	switch (type) {
+	case KVM_S390_MCHK:
+		kvm_s390_set_cpuflags(vcpu, CPUSTAT_STOP_INT);
+		break;
+	case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
+		if (!(type & KVM_S390_INT_IO_AI_MASK &&
+		      vcpu->kvm->arch.gisa_int.origin) ||
+		      kvm_s390_pv_cpu_get_handle(vcpu))
+			kvm_s390_set_cpuflags(vcpu, CPUSTAT_IO_INT);
+		break;
+	default:
+		kvm_s390_set_cpuflags(vcpu, CPUSTAT_EXT_INT);
+		break;
+	}
+	kvm_s390_vcpu_wakeup(vcpu);
+}
+
 /*
  * Find a destination VCPU for a floating irq and kick it.
  */
-static void __floating_irq_kick(struct kvm *kvm, u64 type)
+static void __floating_irq_kick(struct kvm *kvm, u64 type, int isc)
 {
 	struct kvm_vcpu *dst_vcpu;
 	int sigcpu, online_vcpus, nr_tries = 0;
+	u64 irq_pend_mask;
+	unsigned long i;
 
 	online_vcpus = atomic_read(&kvm->online_vcpus);
 	if (!online_vcpus)
 		return;
 
+	irq_pend_mask = inti_to_irq_pend_mask(type, isc);
 	for (sigcpu = kvm->arch.float_int.last_sleep_cpu; ; sigcpu++) {
 		sigcpu %= online_vcpus;
 		dst_vcpu = kvm_get_vcpu(kvm, sigcpu);
-		if (!is_vcpu_stopped(dst_vcpu))
+		if (!is_vcpu_stopped(dst_vcpu) &&
+		    deliverable_irqs(dst_vcpu) & irq_pend_mask)
 			break;
 		/* avoid endless loops if all vcpus are stopped */
-		if (nr_tries++ >= online_vcpus)
-			return;
+		if (nr_tries++ >= online_vcpus * 2) {
+			dst_vcpu = NULL;
+			break;
+		}
 	}
 
-	/* make the VCPU drop out of the SIE, or wake it up if sleeping */
-	switch (type) {
-	case KVM_S390_MCHK:
-		kvm_s390_set_cpuflags(dst_vcpu, CPUSTAT_STOP_INT);
-		break;
-	case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
-		if (!(type & KVM_S390_INT_IO_AI_MASK &&
-		      kvm->arch.gisa_int.origin) ||
-		      kvm_s390_pv_cpu_get_handle(dst_vcpu))
-			kvm_s390_set_cpuflags(dst_vcpu, CPUSTAT_IO_INT);
-		break;
-	default:
-		kvm_s390_set_cpuflags(dst_vcpu, CPUSTAT_EXT_INT);
-		break;
+	/* Nobody was enabled, time to wake all of them */
+	if (!dst_vcpu) {
+		kvm_for_each_vcpu(i, dst_vcpu, kvm)
+			vcpu_intervention_kick(dst_vcpu, type);
+		return;
 	}
-	kvm_s390_vcpu_wakeup(dst_vcpu);
+
+	vcpu_intervention_kick(dst_vcpu, type);
 }
 
 static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 {
 	u64 type = READ_ONCE(inti->type);
+	int isc = -1;
 	int rc;
 
 	switch (type) {
@@ -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));
 		rc = __inject_io(kvm, inti);
 		break;
 	default:
@@ -1982,7 +2028,7 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 	if (rc)
 		return rc;
 
-	__floating_irq_kick(kvm, type);
+	__floating_irq_kick(kvm, type, isc);
 	return 0;
 }
 
-- 
2.53.0

[PATCH v3 2/3] KVM: s390: Kick PV cpus at the right time for service irqs

[Janosch Frank]

Service call handling is a two stage process for PV vms. First we
receive the secure instruction intercept and then the secure
instruction notification intercept.

The secure instruction intercept (104) is analogous to the non-pv
instruction intercept (4) with the difference that we're not allowed
to inject an IRQ when re-entering SIE. We have to wait for the
notification intercept (108) which tells us that we're allowed to
inject.

Unfortunately we never considered this difference and hence the IRQ
injection code will try to inject on the secure instruction intercept
where service IRQs are masked.

It's time to move injection to the instruction notification and skip
injection on the instruction interception path.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/kvm/intercept.c |  9 +++++++
 arch/s390/kvm/interrupt.c | 53 +++++++++++++++++++++++++++++++++------
 arch/s390/kvm/kvm-s390.h  |  1 +
 3 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/arch/s390/kvm/intercept.c b/arch/s390/kvm/intercept.c
index 1980df61ef30..e1de3f471ffd 100644
--- a/arch/s390/kvm/intercept.c
+++ b/arch/s390/kvm/intercept.c
@@ -536,6 +536,15 @@ static int handle_pv_sclp(struct kvm_vcpu *vcpu)
 	set_bit(IRQ_PEND_EXT_SERVICE, &fi->pending_irqs);
 	clear_bit(IRQ_PEND_EXT_SERVICE, &fi->masked_irqs);
 	spin_unlock_irqrestore(&fi->lock, flags);
+
+	/*
+	 * We missed the floating IRQ kick since we can only inject
+	 * when we end up here and not when the irq was injected via
+	 * the FLIC.
+	 *
+	 * Now that we have cleared the masking we can kick cpus.
+	 */
+	kvm_s390_pv_sclp_kick(vcpu);
 	return 0;
 }
 
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 3af060ec5be8..53ba74a749e8 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -1960,10 +1960,7 @@ static void vcpu_intervention_kick(struct kvm_vcpu *vcpu, u64 type)
 	kvm_s390_vcpu_wakeup(vcpu);
 }
 
-/*
- * Find a destination VCPU for a floating irq and kick it.
- */
-static void __floating_irq_kick(struct kvm *kvm, u64 type, int isc)
+static void kick_cpu_irq(struct kvm *kvm, u64 type, u64 parm)
 {
 	struct kvm_vcpu *dst_vcpu;
 	int sigcpu, online_vcpus, nr_tries = 0;
@@ -1974,7 +1971,7 @@ static void __floating_irq_kick(struct kvm *kvm, u64 type, int isc)
 	if (!online_vcpus)
 		return;
 
-	irq_pend_mask = inti_to_irq_pend_mask(type, isc);
+	irq_pend_mask = inti_to_irq_pend_mask(type, parm);
 	for (sigcpu = kvm->arch.float_int.last_sleep_cpu; ; sigcpu++) {
 		sigcpu %= online_vcpus;
 		dst_vcpu = kvm_get_vcpu(kvm, sigcpu);
@@ -1998,10 +1995,49 @@ static void __floating_irq_kick(struct kvm *kvm, u64 type, int isc)
 	vcpu_intervention_kick(dst_vcpu, type);
 }
 
+void kvm_s390_pv_sclp_kick(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * The cpu that called sclp likely will also take the IRQ, no
+	 * need to kick anyone.
+	 */
+	if (likely(deliverable_irqs(vcpu) & IRQ_PEND_EXT_SERVICE))
+		return;
+
+	/*
+	 * For the other cases we might have sleeping cpus with open
+	 * masks. Time to find and kick them.
+	 */
+	kick_cpu_irq(vcpu->kvm, KVM_S390_INT_SERVICE, -1);
+}
+
+/*
+ * Find a destination VCPU for a floating irq and kick it.
+ */
+static void __floating_irq_kick(struct kvm *kvm, u64 type, u64 parm)
+{
+	int prot;
+
+	mutex_lock(&kvm->lock);
+	prot = kvm_s390_pv_is_protected(kvm);
+	mutex_unlock(&kvm->lock);
+	/*
+	 * No need to kick on non-ev service IRQs for PV VMs, we're
+	 * not allowed to inject anyway. We need to wait for the sclp
+	 * instruction notification AFTER re-entry of the vcpu that
+	 * handled the instruction intercept.
+	 */
+	if (prot && type == KVM_S390_INT_SERVICE &&
+	    !(parm & SCCB_EVENT_PENDING))
+		return;
+
+	kick_cpu_irq(kvm, type, parm);
+}
+
 static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 {
 	u64 type = READ_ONCE(inti->type);
-	int isc = -1;
+	u64 parm;
 	int rc;
 
 	switch (type) {
@@ -2012,6 +2048,7 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 		rc = __inject_virtio(kvm, inti);
 		break;
 	case KVM_S390_INT_SERVICE:
+		parm = inti->ext.ext_params & SCCB_EVENT_PENDING;
 		rc = __inject_service(kvm, inti);
 		break;
 	case KVM_S390_INT_PFAULT_DONE:
@@ -2019,7 +2056,7 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *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));
+		parm = isc_to_irq_type(int_word_to_isc(inti->io.io_int_word));
 		rc = __inject_io(kvm, inti);
 		break;
 	default:
@@ -2028,7 +2065,7 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 	if (rc)
 		return rc;
 
-	__floating_irq_kick(kvm, type, isc);
+	__floating_irq_kick(kvm, type, parm);
 	return 0;
 }
 
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 6d2842fb71a3..8e886bcef4a0 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -375,6 +375,7 @@ enum hrtimer_restart kvm_s390_idle_wakeup(struct hrtimer *timer);
 int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu);
 void kvm_s390_clear_local_irqs(struct kvm_vcpu *vcpu);
 void kvm_s390_clear_float_irqs(struct kvm *kvm);
+void kvm_s390_pv_sclp_kick(struct kvm_vcpu *vcpu);
 int __must_check kvm_s390_inject_vm(struct kvm *kvm,
 				    struct kvm_s390_interrupt *s390int,
 				    struct kvm_s390_interrupt_info *inti);
-- 
2.53.0

[PATCH v3 3/3] KVM: s390: Add opportunistic floating IRQ injection

[Janosch Frank]

Try to distribute floating IRQs which haven't been taken by a VCPU yet
to sleeping VCPUs. This lowers the risk of floating IRQs not being
delivered.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/interrupt.c        | 31 +++++++++++++++++++++++++++++++
 arch/s390/kvm/kvm-s390.c         |  3 +++
 arch/s390/kvm/kvm-s390.h         |  1 +
 4 files changed, 36 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index b4182ca4435f..1d62bdd7aca5 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -467,6 +467,7 @@ struct kvm_vm_stat {
 	u64 gmap_shadow_r3_entry;
 	u64 gmap_shadow_sg_entry;
 	u64 gmap_shadow_pg_entry;
+	u64 inject_redist;
 };
 
 struct kvm_arch_memory_slot {
diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 53ba74a749e8..1ee9ab250504 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -367,6 +367,37 @@ static unsigned long deliverable_irqs(struct kvm_vcpu *vcpu)
 	return active_mask;
 }
 
+void distribute_float_irqs(struct kvm *kvm)
+{
+	struct kvm_vcpu *dst_vcpu;
+	int sigcpu, online_vcpus;
+
+	if (!READ_ONCE(kvm->arch.float_int.pending_irqs))
+		return;
+
+	online_vcpus = atomic_read(&kvm->online_vcpus);
+
+	/*
+	 * Not too worried about synchronization for idle_mask. We
+	 * might burn too many cycles but apart from that waking a
+	 * vcpu is not harmful.
+	 */
+	sigcpu = find_first_bit(kvm->arch.idle_mask, online_vcpus);
+	/* Well nobody's sleeping so someone will likely take the IRQ soon */
+	if (sigcpu == online_vcpus)
+		return;
+
+	do {
+		dst_vcpu = kvm_get_vcpu(kvm, sigcpu);
+		if (deliverable_irqs(dst_vcpu)) {
+			kvm->stat.inject_redist++;
+			kvm_s390_vcpu_wakeup(dst_vcpu);
+			break;
+		}
+		sigcpu = find_next_bit(kvm->arch.idle_mask, online_vcpus, ++sigcpu);
+	} while (sigcpu < online_vcpus);
+}
+
 static void __set_cpu_idle(struct kvm_vcpu *vcpu)
 {
 	kvm_s390_set_cpuflags(vcpu, CPUSTAT_WAIT);
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 518a69c55e85..d6d46688d980 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -86,6 +86,7 @@ const struct kvm_stats_desc kvm_vm_stats_desc[] = {
 	STATS_DESC_COUNTER(VM, gmap_shadow_r3_entry),
 	STATS_DESC_COUNTER(VM, gmap_shadow_sg_entry),
 	STATS_DESC_COUNTER(VM, gmap_shadow_pg_entry),
+	STATS_DESC_COUNTER(VM, inject_redist),
 };
 
 const struct kvm_stats_header kvm_vm_stats_header = {
@@ -4572,6 +4573,8 @@ static int vcpu_pre_run(struct kvm_vcpu *vcpu)
 		rc = kvm_s390_deliver_pending_interrupts(vcpu);
 		if (rc || guestdbg_exit_pending(vcpu))
 			return rc;
+
+		distribute_float_irqs(vcpu->kvm);
 	}
 
 	rc = kvm_s390_handle_requests(vcpu);
diff --git a/arch/s390/kvm/kvm-s390.h b/arch/s390/kvm/kvm-s390.h
index 8e886bcef4a0..7115a8f5b188 100644
--- a/arch/s390/kvm/kvm-s390.h
+++ b/arch/s390/kvm/kvm-s390.h
@@ -376,6 +376,7 @@ int __must_check kvm_s390_deliver_pending_interrupts(struct kvm_vcpu *vcpu);
 void kvm_s390_clear_local_irqs(struct kvm_vcpu *vcpu);
 void kvm_s390_clear_float_irqs(struct kvm *kvm);
 void kvm_s390_pv_sclp_kick(struct kvm_vcpu *vcpu);
+void distribute_float_irqs(struct kvm *kvm);
 int __must_check kvm_s390_inject_vm(struct kvm *kvm,
 				    struct kvm_s390_interrupt *s390int,
 				    struct kvm_s390_interrupt_info *inti);
-- 
2.53.0

Re: [PATCH v3 3/3] KVM: s390: Add opportunistic floating IRQ injection

[sashiko-bot]

> Try to distribute floating IRQs which haven't been taken by a VCPU yet
> to sleeping VCPUs. This lowers the risk of floating IRQs not being
> delivered.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819190408.247843-1-frankja@linux.ibm.com?part=3

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

[sashiko-bot]

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

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/20260819190408.247843-1-frankja@linux.ibm.com?part=1

Re: [PATCH v3 2/3] KVM: s390: Kick PV cpus at the right time for service irqs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Unconditional acquisition of a sleeping lock (`mutex_lock(&kvm->lock)`) in the floating interrupt injection hot-path causes a system BUG when called from atomic context.
- [Critical] Acquiring `kvm->lock` inside `__floating_irq_kick()` violates the KVM locking hierarchy, creating ABBA deadlocks with `vcpu->mutex` and `kvm->srcu`.
- [High] In `kvm_s390_pv_sclp_kick()`, `IRQ_PEND_EXT_SERVICE` is incorrectly used as a bitmask instead of a bit index, causing the kick condition to evaluate incorrectly.
- [High] `inti_to_irq_pend_mask()` returns an unshifted bit index instead of a bitmask for I/O interrupts and double-converts the ISC value, breaking IO floating IRQ routing.
- [Medium] The local variable `parm` is passed by value uninitialized to `__floating_irq_kick()` for several interrupt types, triggering Undefined Behavior and KMSAN warnings.
--

--- Patch [2]: [PATCH v3 2/3] KVM: s390: Kick PV cpus at the right time for service irqs ---
commit a45baf236c563050e29f20187e5498732692fe4c
Author: Janosch Frank <frankja@linux.ibm.com>

KVM: s390: Kick PV cpus at the right time for service irqs

Service call handling is a two stage process for PV vms. First we
receive the secure instruction intercept and then the secure
instruction notification intercept.

> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 3af060ec5be8e..53ba74a749e8a 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c

[ ... ]

> @@ -1998,10 +1995,49 @@ static void __floating_irq_kick(struct kvm *kvm, u64 type, int isc)
>  	vcpu_intervention_kick(dst_vcpu, type);
>  }
>  
> +void kvm_s390_pv_sclp_kick(struct kvm_vcpu *vcpu)
> +{
> +	/*
> +	 * The cpu that called sclp likely will also take the IRQ, no
> +	 * need to kick anyone.
> +	 */
> +	if (likely(deliverable_irqs(vcpu) & IRQ_PEND_EXT_SERVICE))
> +		return;

[Severity: High]
Is IRQ_PEND_EXT_SERVICE missing a BIT() macro here?

The deliverable_irqs() function returns a bitmask, but IRQ_PEND_EXT_SERVICE
is an enum value (15). Doesn't this evaluate the bitwise AND against 15
(0b1111) rather than checking bit 15? This could result in false negatives
and lost wakeups for target vCPUs waiting for service interrupts.

> +
> +	/*
> +	 * For the other cases we might have sleeping cpus with open
> +	 * masks. Time to find and kick them.
> +	 */
> +	kick_cpu_irq(vcpu->kvm, KVM_S390_INT_SERVICE, -1);
> +}
> +
> +/*
> + * Find a destination VCPU for a floating irq and kick it.
> + */
> +static void __floating_irq_kick(struct kvm *kvm, u64 type, u64 parm)
> +{
> +	int prot;
> +
> +	mutex_lock(&kvm->lock);
> +	prot = kvm_s390_pv_is_protected(kvm);
> +	mutex_unlock(&kvm->lock);

[Severity: Critical]
Can acquiring this sleeping lock cause a 'scheduling while atomic' bug?

When called via the interrupt injection path, this can execute in an atomic
context:

kvm_arch_set_irq_inatomic()
   spin_lock(&fi->ais_lock);
   kvm_s390_inject_vm()
      __inject_vm()
         __floating_irq_kick()
            mutex_lock(&kvm->lock);

[Severity: Critical]
Does taking kvm->lock here also create an ABBA deadlock?

During standard I/O interrupt reinjection:

vcpu_run()
   handle_tpi()
      kvm_s390_reinject_io_int()
         __inject_vm()
            __floating_irq_kick()

In this path, vcpu->mutex is already held by KVM_RUN, and the vCPU is inside
an SRCU read-side critical section. Acquiring kvm->lock while holding these
violates the KVM locking hierarchy.

> +	/*
> +	 * No need to kick on non-ev service IRQs for PV VMs, we're
> +	 * not allowed to inject anyway. We need to wait for the sclp
> +	 * instruction notification AFTER re-entry of the vcpu that
> +	 * handled the instruction intercept.
> +	 */
> +	if (prot && type == KVM_S390_INT_SERVICE &&
> +	    !(parm & SCCB_EVENT_PENDING))
> +		return;
> +
> +	kick_cpu_irq(kvm, type, parm);
> +}
> +
>  static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  {
>  	u64 type = READ_ONCE(inti->type);
> -	int isc = -1;
> +	u64 parm;
>  	int rc;
>  
>  	switch (type) {

[ ... ]

>  	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));
> +		parm = isc_to_irq_type(int_word_to_isc(inti->io.io_int_word));

[Severity: High]
Does this result in a double-conversion of the ISC value?

Down in inti_to_irq_pend_mask(), the switch case for I/O interrupts does:

    case KVM_S390_INT_IO_MIN...KVM_S390_INT_IO_MAX:
        return isc_to_irq_type(isc);

Since parm is already converted here and passed into inti_to_irq_pend_mask()
as the isc parameter, it looks like it gets processed through
isc_to_irq_type() a second time. It also appears to lack a BIT() macro
wrapper, returning a raw integer (0-7) instead of a bitmask.

>  		rc = __inject_io(kvm, inti);
>  		break;
>  	default:
> @@ -2028,7 +2065,7 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
>  	if (rc)
>  		return rc;
>  
> -	__floating_irq_kick(kvm, type, isc);
> +	__floating_irq_kick(kvm, type, parm);

[Severity: Medium]
Does parm get passed uninitialized here for certain interrupt types?

For cases like KVM_S390_MCHK, KVM_S390_INT_VIRTIO, and
KVM_S390_INT_PFAULT_DONE, parm is never assigned a value in the switch
statement above. Passing it by value to __floating_irq_kick() causes an
uninitialized read that triggers KMSAN warnings.

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819190408.247843-1-frankja@linux.ibm.com?part=2