KVM: arm64: Wake-up from WFI when iqrchip is in userspace

It appears that there is nothing in the wake-up path that evaluates whether the in-kernel interrupts are pending unless we have a vgic.

This means that the userspace irqchip support has been broken for about four years, and nobody noticed. It was also broken before as we wouldn't wake-up on a PMU interrupt, but hey, who cares...

It is probably time to remove the feature altogether, because it was a terrible idea 10 years ago, and it still is.

Fixes b57de4ffd7c6d ("KVM: arm64: Simplify kvm_cpu_has_pending_timer()")

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/arm.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kvm/arm.c +0 -0

index 176cbe8baad30..8bb2c7422cc8b 100644

arch/arm64/kvm/arm.c +1 -1

+
 	return ((irq_lines || kvm_vgic_vcpu_pending_irq(v))
 		&& !kvm_arm_vcpu_stopped(v) && !v->arch.pause);
 }
--

2.47.3

Quoted 22 lines · Marc Zyngier

It appears that there is nothing in the wake-up path that evaluates whether the in-kernel interrupts are pending unless we have a vgic.

This means that the userspace irqchip support has been broken for about four years, and nobody noticed. It was also broken before as we wouldn't wake-up on a PMU interrupt, but hey, who cares...

It is probably time to remove the feature altogether, because it was a terrible idea 10 years ago, and it still is.

Fixes b57de4ffd7c6d ("KVM: arm64: Simplify kvm_cpu_has_pending_timer()")

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/arm.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kvm/arm.c +0 -0

index 176cbe8baad30..8bb2c7422cc8b 100644

arch/arm64/kvm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) · line 826

Quoted 3 lines

arch/arm64/kvm/arm.c +0 -0

@@ -824,6 +824,10 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
 	bool irq_lines = *vcpu_hcr(v) & (HCR_VI | HCR_VF | HCR_VSE);
 

Hi Marc,

arch/arm64/kvm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) · line 829

Quoted 3 lines

arch/arm64/kvm/arm.c +3 -0

+	irq_lines |= (!irqchip_in_kernel(v->kvm) &&
+		      (kvm_timer_should_notify_user(v) ||
+		       kvm_pmu_should_notify_user(v)));

How about a new helper like 'kvm_should_notify_us_irqchip()' ? We can replace the same part at beginning of kvm_vcpu_exit_request() and here w/ unlikely().

Quoted 30 lines · Yao Yuan

Quoted 22 lines · Marc Zyngier

It appears that there is nothing in the wake-up path that evaluates whether the in-kernel interrupts are pending unless we have a vgic.

This means that the userspace irqchip support has been broken for about four years, and nobody noticed. It was also broken before as we wouldn't wake-up on a PMU interrupt, but hey, who cares...

It is probably time to remove the feature altogether, because it was a terrible idea 10 years ago, and it still is.

Fixes b57de4ffd7c6d ("KVM: arm64: Simplify kvm_cpu_has_pending_timer()")

Signed-off-by Marc Zyngier <[email protected]>

---
 arch/arm64/kvm/arm.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kvm/arm.c +0 -0

index 176cbe8baad30..8bb2c7422cc8b 100644

arch/arm64/kvm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) · line 826

Quoted 3 lines

arch/arm64/kvm/arm.c +0 -0

@@ -824,6 +824,10 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *v)
 {
 	bool irq_lines = *vcpu_hcr(v) & (HCR_VI | HCR_VF | HCR_VSE);
 

Hi Marc,

Quoted 7 lines

arch/arm64/kvm/arm.c int kvm_arch_vcpu_runnable(struct kvm_vcpu *v) · line 829

Quoted 3 lines

arch/arm64/kvm/arm.c +3 -0

+	irq_lines |= (!irqchip_in_kernel(v->kvm) &&
+		      (kvm_timer_should_notify_user(v) ||
+		       kvm_pmu_should_notify_user(v)));

How about a new helper like 'kvm_should_notify_us_irqchip()' ? We can replace the same part at beginning of kvm_vcpu_exit_request() and here w/ unlikely().

I'd rather not introduce a helper, for two reasons:

- this needs to be backported all the way to 5.19, because that's how far it has been broken. So keeping it small and localised is far better than introducing a helper that will make the backport less obvious.

- I have patches to remove the other calls to kvm_*_notify_user() as a simplification of this utterly stupid feature.

Finally, and while I agree that this could take an unlikely() qualifier, a much better course of action would be to have a separate patch that moves the qualifier to the predicate itself.

Thanks,

	M.

--
Jazz isn't dead. It just smells funny.

It appears that there is nothing in the wake-up path that evaluates whether the in-kernel interrupts are pending unless we have a vgic.

This means that the userspace irqchip support has been broken for about four years, and nobody noticed. It was also broken before as we wouldn't wake-up on a PMU interrupt, but hey, who cares...

[...]