Handle forwarded level-triggered interrupts

From: Christoffer Dall <[email protected]>

We are about to distinguish between userspace accesses and mmio traps for a number of the mmio handlers. When the requester vcpu is NULL, it mens we are handling a userspace acccess.

Factor out the functionality to get the request vcpu into its own function, mostly so we have a common place to document the semantics of the return value.

Also take the chance to move the functionality outside of holding a spinlock and instead explicitly disable and enable preemption. This supports PREEMPT_RT kernels as well.

Acked-by Marc Zyngier <[email protected]>

Reviewed-by Andre Przywara <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 virt/kvm/arm/vgic/vgic-mmio.c | 44 +++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

virt/kvm/arm/vgic/vgic-mmio.c +0 -0

index deb51ee16a3d..747b0a3b4784 100644

virt/kvm/arm/vgic/vgic-mmio.c +25 -16

+ * access.
+ *
+ * We can disable preemption locally around accessing the per-CPU variable,
+ * and use the resolved vcpu pointer after enabling preemption again, because
+ * even if the current thread is migrated to another CPU, reading the per-CPU
+ * value later will give us the same value as we update the per-CPU variable
+ * in the preempt notifier handlers.
+ */
+static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
+{
+	struct kvm_vcpu *vcpu;
+
+	preempt_disable();
+	vcpu = kvm_arm_get_running_vcpu();
+	preempt_enable();
+	return vcpu;
+}
+
 void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
 			      gpa_t addr, unsigned int len,
 			      unsigned long val)

@@ -184,24 +205,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
 static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 				    bool new_active_state)
 {
-	struct kvm_vcpu *requester_vcpu;
 	unsigned long flags;
-	spin_lock_irqsave(&irq->irq_lock, flags);
+	struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
 
-	/*
-	 * The vcpu parameter here can mean multiple things depending on how
-	 * this function is called; when handling a trap from the kernel it
-	 * depends on the GIC version, and these functions are also called as
-	 * part of save/restore from userspace.
-	 *
-	 * Therefore, we have to figure out the requester in a reliable way.
-	 *
-	 * When accessing VGIC state from user space, the requester_vcpu is
-	 * NULL, which is fine, because we guarantee that no VCPUs are running
-	 * when accessing VGIC state from user space so irq->vcpu->cpu is
-	 * always -1.
-	 */
-	requester_vcpu = kvm_arm_get_running_vcpu();
+	spin_lock_irqsave(&irq->irq_lock, flags);
 
 	/*
 	 * If this virtual IRQ was written into a list register, we

@@ -213,6 +220,11 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 	 * vgic_change_active_prepare)  and still has to sync back this IRQ,
 	 * so we release and re-acquire the spin_lock to let the other thread
 	 * sync back the IRQ.
+	 *
+	 * When accessing VGIC state from user space, requester_vcpu is
+	 * NULL, which is fine, because we guarantee that no VCPUs are running
+	 * when accessing VGIC state from user space so irq->vcpu->cpu is
+	 * always -1.
 	 */
 	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
 	       irq->vcpu != requester_vcpu && /* Current thread is not the VCPU thread */

Signature · 1 line

2.14.2

Quoted 5 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

We are about to distinguish between userspace accesses and mmio traps for a number of the mmio handlers. When the requester vcpu is NULL, it mens we are handling a userspace acccess.

Typo: means?

Quoted 23 lines

Factor out the functionality to get the request vcpu into its own function, mostly so we have a common place to document the semantics of the return value.

Also take the chance to move the functionality outside of holding a spinlock and instead explicitly disable and enable preemption. This supports PREEMPT_RT kernels as well.

Acked-by Marc Zyngier <[email protected]>

Reviewed-by Andre Przywara <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 virt/kvm/arm/vgic/vgic-mmio.c | 44 +++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

virt/kvm/arm/vgic/vgic-mmio.c +0 -0

index deb51ee16a3d..747b0a3b4784 100644
@@ -122,6 +122,27 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
 	return value;
 }
 

virt/kvm/arm/vgic/vgic-mmio.c unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu, · line 127

Quoted 3 lines

virt/kvm/arm/vgic/vgic-mmio.c +3 -0

+/*
+ * This function will return the VCPU that performed the MMIO access and
+ * trapped from twithin the VM, and will return NULL if this is a userspace

Typo: from within?

Quoted 46 lines

virt/kvm/arm/vgic/vgic-mmio.c +20 -16

+ * access.
+ *
+ * We can disable preemption locally around accessing the per-CPU variable,
+ * and use the resolved vcpu pointer after enabling preemption again, because
+ * even if the current thread is migrated to another CPU, reading the per-CPU
+ * value later will give us the same value as we update the per-CPU variable
+ * in the preempt notifier handlers.
+ */
+static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
+{
+	struct kvm_vcpu *vcpu;
+
+	preempt_disable();
+	vcpu = kvm_arm_get_running_vcpu();
+	preempt_enable();
+	return vcpu;
+}
+
 void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
 			      gpa_t addr, unsigned int len,
 			      unsigned long val)

@@ -184,24 +205,10 @@ unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
 static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 				    bool new_active_state)
 {
-	struct kvm_vcpu *requester_vcpu;
 	unsigned long flags;
-	spin_lock_irqsave(&irq->irq_lock, flags);
+	struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();
 
-	/*
-	 * The vcpu parameter here can mean multiple things depending on how
-	 * this function is called; when handling a trap from the kernel it
-	 * depends on the GIC version, and these functions are also called as
-	 * part of save/restore from userspace.
-	 *
-	 * Therefore, we have to figure out the requester in a reliable way.
-	 *
-	 * When accessing VGIC state from user space, the requester_vcpu is
-	 * NULL, which is fine, because we guarantee that no VCPUs are running
-	 * when accessing VGIC state from user space so irq->vcpu->cpu is
-	 * always -1.
-	 */
-	requester_vcpu = kvm_arm_get_running_vcpu();
+	spin_lock_irqsave(&irq->irq_lock, flags);
 

Quoted 16 lines

virt/kvm/arm/vgic/vgic-mmio.c +5 -0

 	/*
 	 * If this virtual IRQ was written into a list register, we

@@ -213,6 +220,11 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 	 * vgic_change_active_prepare)  and still has to sync back this IRQ,
 	 * so we release and re-acquire the spin_lock to let the other thread
 	 * sync back the IRQ.
+	 *
+	 * When accessing VGIC state from user space, requester_vcpu is
+	 * NULL, which is fine, because we guarantee that no VCPUs are running
+	 * when accessing VGIC state from user space so irq->vcpu->cpu is
+	 * always -1.
 	 */
 	while (irq->vcpu && /* IRQ may have state in an LR somewhere */
 	       irq->vcpu != requester_vcpu && /* Current thread is not the VCPU thread */

Signature · 1 line

2.14.2

From: Christoffer Dall <[email protected]>

For mapped IRQs (with the HW bit set in the LR) we have to follow some rules of the architecture. One of these rules is that VM must not be allowed to deactivate a virtual interrupt with the HW bit set unless the physical interrupt is also active.

This works fine when injecting mapped interrupts, because we leave it up to the injector to either set EOImode==1 or manually set the active state of the physical interrupt.

However, the guest can set virtual interrupt to be pending or active by writing to the virtual distributor, which could lead to deactivating a virtual interrupt with the HW bit set without the physical interrupt being active.

We could set the physical interrupt to active whenever we are about to enter the VM with a HW interrupt either pending or active, but that would be really slow, especially on GICv2. So we take the long way around and do the hard work when needed, which is expected to be extremely rare.

When the VM sets the pending state for a HW interrupt on the virtual distributor we set the active state on the physical distributor, because the virtual interrupt can become active and then the guest can deactivate it.

When the VM clears the pending state we also clear it on the physical side, because the injector might otherwise raise the interrupt. We also clear the physical active state when the virtual interrupt is not active, since otherwise a SPEND/CPEND sequence from the guest would prevent signaling of future interrupts.

Changing the state of mapped interrupts from userspace is not supported, and it's expected that userspace unmaps devices from VFIO before attempting to set the interrupt state, because the interrupt state is driven by hardware.

Reviewed-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
 virt/kvm/arm/vgic/vgic.c      |  7 +++++
 virt/kvm/arm/vgic/vgic.h      |  1 +
 3 files changed, 73 insertions(+), 6 deletions(-)

virt/kvm/arm/vgic/vgic-mmio.c +1 -0

index 747b0a3b4784..8d173d99a7a4 100644
@@ -16,6 +16,7 @@
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <kvm/iodev.h>
+#include <kvm/arm_arch_timer.h>
 #include <kvm/arm_vgic.h>
 
 #include "vgic.h"

virt/kvm/arm/vgic/vgic-mmio.c +45 -3

+static void vgic_hw_irq_spending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+				 bool is_uaccess)
+{
+	if (is_uaccess)
+		return;
+
+	irq->pending_latch = true;
+	vgic_irq_set_phys_active(irq, true);
+}
+
 void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
 			      gpa_t addr, unsigned int len,
 			      unsigned long val)
 {
+	bool is_uaccess = !vgic_get_mmio_requester_vcpu();
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
 	unsigned long flags;

@@ -155,17 +168,45 @@ void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
 		struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
 
 		spin_lock_irqsave(&irq->irq_lock, flags);
-		irq->pending_latch = true;
-
+		if (irq->hw)
+			vgic_hw_irq_spending(vcpu, irq, is_uaccess);
+		else
+			irq->pending_latch = true;
 		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
 		vgic_put_irq(vcpu->kvm, irq);
 	}
 }
 
+/* Must be called with irq->irq_lock held */
+static void vgic_hw_irq_cpending(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
+				 bool is_uaccess)
+{
+	if (is_uaccess)
+		return;
+
+	irq->pending_latch = false;
+
+	/*
+	 * We don't want the guest to effectively mask the physical
+	 * interrupt by doing a write to SPENDR followed by a write to
+	 * CPENDR for HW interrupts, so we clear the active state on
+	 * the physical side if the virtual interrupt is not active.
+	 * This may lead to taking an additional interrupt on the
+	 * host, but that should not be a problem as the worst that
+	 * can happen is an additional vgic injection.  We also clear
+	 * the pending state to maintain proper semantics for edge HW
+	 * interrupts.
+	 */
+	vgic_irq_set_phys_pending(irq, false);
+	if (!irq->active)
+		vgic_irq_set_phys_active(irq, false);
+}
+
 void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
 			      gpa_t addr, unsigned int len,
 			      unsigned long val)
 {
+	bool is_uaccess = !vgic_get_mmio_requester_vcpu();
 	u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
 	int i;
 	unsigned long flags;

@@ -175,7 +216,10 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
 
 		spin_lock_irqsave(&irq->irq_lock, flags);
 
-		irq->pending_latch = false;
+		if (irq->hw)
+			vgic_hw_irq_cpending(vcpu, irq, is_uaccess);
+		else
+			irq->pending_latch = false;
 
 		spin_unlock_irqrestore(&irq->irq_lock, flags);
 		vgic_put_irq(vcpu->kvm, irq);

virt/kvm/arm/vgic/vgic-mmio.c +8 -3

+
 static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
-				    bool new_active_state)
+				    bool active)
 {
 	unsigned long flags;
 	struct kvm_vcpu *requester_vcpu = vgic_get_mmio_requester_vcpu();

@@ -231,8 +286,12 @@ static void vgic_mmio_change_active(struct kvm_vcpu *vcpu, struct vgic_irq *irq,
 	       irq->vcpu->cpu != -1) /* VCPU thread is running */
 		cond_resched_lock(&irq->irq_lock);
 
-	irq->active = new_active_state;
-	if (new_active_state)
+	if (irq->hw)
+		vgic_hw_irq_change_active(vcpu, irq, active, !requester_vcpu);
+	else
+		irq->active = active;
+
+	if (irq->active)
 		vgic_queue_irq_unlock(vcpu->kvm, irq, flags);
 	else
 		spin_unlock_irqrestore(&irq->irq_lock, flags);

virt/kvm/arm/vgic/vgic.c +7 -0

index eadabb249d2a..f4c92fae9cd3 100644
@@ -144,6 +144,13 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq)
 	kfree(irq);
 }
 
+void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending)
+{
+	WARN_ON(irq_set_irqchip_state(irq->host_irq,
+				      IRQCHIP_STATE_PENDING,
+				      pending));
+}
+
 /* Get the input level of a mapped IRQ directly from the physical GIC */
 bool vgic_get_phys_line_level(struct vgic_irq *irq)
 {

virt/kvm/arm/vgic/vgic.h +1 -0

index d0787983a357..12c37b89f7a3 100644
@@ -146,6 +146,7 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu,
 			      u32 intid);
 void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq);
 bool vgic_get_phys_line_level(struct vgic_irq *irq);
+void vgic_irq_set_phys_pending(struct vgic_irq *irq, bool pending);
 void vgic_irq_set_phys_active(struct vgic_irq *irq, bool active);
 bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq,
 			   unsigned long flags);

Signature · 1 line

2.14.2

Quoted 62 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

For mapped IRQs (with the HW bit set in the LR) we have to follow some rules of the architecture. One of these rules is that VM must not be allowed to deactivate a virtual interrupt with the HW bit set unless the physical interrupt is also active.

This works fine when injecting mapped interrupts, because we leave it up to the injector to either set EOImode==1 or manually set the active state of the physical interrupt.

However, the guest can set virtual interrupt to be pending or active by writing to the virtual distributor, which could lead to deactivating a virtual interrupt with the HW bit set without the physical interrupt being active.

We could set the physical interrupt to active whenever we are about to enter the VM with a HW interrupt either pending or active, but that would be really slow, especially on GICv2. So we take the long way around and do the hard work when needed, which is expected to be extremely rare.

When the VM sets the pending state for a HW interrupt on the virtual distributor we set the active state on the physical distributor, because the virtual interrupt can become active and then the guest can deactivate it.

When the VM clears the pending state we also clear it on the physical side, because the injector might otherwise raise the interrupt. We also clear the physical active state when the virtual interrupt is not active, since otherwise a SPEND/CPEND sequence from the guest would prevent signaling of future interrupts.

Changing the state of mapped interrupts from userspace is not supported, and it's expected that userspace unmaps devices from VFIO before attempting to set the interrupt state, because the interrupt state is driven by hardware.

Reviewed-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
 virt/kvm/arm/vgic/vgic.c      |  7 +++++
 virt/kvm/arm/vgic/vgic.h      |  1 +
 3 files changed, 73 insertions(+), 6 deletions(-)

virt/kvm/arm/vgic/vgic-mmio.c +1 -0

index 747b0a3b4784..8d173d99a7a4 100644
@@ -16,6 +16,7 @@
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <kvm/iodev.h>
+#include <kvm/arm_arch_timer.h>
 #include <kvm/arm_vgic.h>
 
 #include "vgic.h"

@@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
 	return vcpu;
 }
 

virt/kvm/arm/vgic/vgic-mmio.c static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void) · line 147

Quoted 1 line

virt/kvm/arm/vgic/vgic-mmio.c +1 -0

+/* Must be called with irq->irq_lock held */

Instead of enforcing this rule in comment, you can enforce it in code:

BUG_ON(!spin_is_locked(irq->irq_lock))

Quoted 63 lines · Yury Norov

Quoted 62 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

For mapped IRQs (with the HW bit set in the LR) we have to follow some rules of the architecture. One of these rules is that VM must not be allowed to deactivate a virtual interrupt with the HW bit set unless the physical interrupt is also active.

This works fine when injecting mapped interrupts, because we leave it up to the injector to either set EOImode==1 or manually set the active state of the physical interrupt.

However, the guest can set virtual interrupt to be pending or active by writing to the virtual distributor, which could lead to deactivating a virtual interrupt with the HW bit set without the physical interrupt being active.

We could set the physical interrupt to active whenever we are about to enter the VM with a HW interrupt either pending or active, but that would be really slow, especially on GICv2. So we take the long way around and do the hard work when needed, which is expected to be extremely rare.

When the VM sets the pending state for a HW interrupt on the virtual distributor we set the active state on the physical distributor, because the virtual interrupt can become active and then the guest can deactivate it.

When the VM clears the pending state we also clear it on the physical side, because the injector might otherwise raise the interrupt. We also clear the physical active state when the virtual interrupt is not active, since otherwise a SPEND/CPEND sequence from the guest would prevent signaling of future interrupts.

Changing the state of mapped interrupts from userspace is not supported, and it's expected that userspace unmaps devices from VFIO before attempting to set the interrupt state, because the interrupt state is driven by hardware.

Reviewed-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
 virt/kvm/arm/vgic/vgic.c      |  7 +++++
 virt/kvm/arm/vgic/vgic.h      |  1 +
 3 files changed, 73 insertions(+), 6 deletions(-)

virt/kvm/arm/vgic/vgic-mmio.c +1 -0

index 747b0a3b4784..8d173d99a7a4 100644
@@ -16,6 +16,7 @@
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <kvm/iodev.h>
+#include <kvm/arm_arch_timer.h>
 #include <kvm/arm_vgic.h>
 
 #include "vgic.h"

@@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
 	return vcpu;
 }
 

Quoted 5 lines

virt/kvm/arm/vgic/vgic-mmio.c static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void) · line 147

Quoted 1 line

virt/kvm/arm/vgic/vgic-mmio.c +1 -0

+/* Must be called with irq->irq_lock held */

Instead of enforcing this rule in comment, you can enforce it in code:

BUG_ON(!spin_is_locked(irq->irq_lock))

Are we going to litter the kernel with such assertions? I don't think that's a valuable thing to do.

Quoted 64 lines · Marc Zyngier

Quoted 63 lines · Yury Norov

Quoted 62 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

For mapped IRQs (with the HW bit set in the LR) we have to follow some rules of the architecture. One of these rules is that VM must not be allowed to deactivate a virtual interrupt with the HW bit set unless the physical interrupt is also active.

This works fine when injecting mapped interrupts, because we leave it up to the injector to either set EOImode==1 or manually set the active state of the physical interrupt.

However, the guest can set virtual interrupt to be pending or active by writing to the virtual distributor, which could lead to deactivating a virtual interrupt with the HW bit set without the physical interrupt being active.

We could set the physical interrupt to active whenever we are about to enter the VM with a HW interrupt either pending or active, but that would be really slow, especially on GICv2. So we take the long way around and do the hard work when needed, which is expected to be extremely rare.

When the VM sets the pending state for a HW interrupt on the virtual distributor we set the active state on the physical distributor, because the virtual interrupt can become active and then the guest can deactivate it.

When the VM clears the pending state we also clear it on the physical side, because the injector might otherwise raise the interrupt. We also clear the physical active state when the virtual interrupt is not active, since otherwise a SPEND/CPEND sequence from the guest would prevent signaling of future interrupts.

Changing the state of mapped interrupts from userspace is not supported, and it's expected that userspace unmaps devices from VFIO before attempting to set the interrupt state, because the interrupt state is driven by hardware.

Reviewed-by Marc Zyngier <[email protected]>

Signed-off-by Christoffer Dall <[email protected]>

---
 virt/kvm/arm/vgic/vgic-mmio.c | 71 +++++++++++++++++++++++++++++++++++++++----
 virt/kvm/arm/vgic/vgic.c      |  7 +++++
 virt/kvm/arm/vgic/vgic.h      |  1 +
 3 files changed, 73 insertions(+), 6 deletions(-)

virt/kvm/arm/vgic/vgic-mmio.c +1 -0

index 747b0a3b4784..8d173d99a7a4 100644
@@ -16,6 +16,7 @@
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
 #include <kvm/iodev.h>
+#include <kvm/arm_arch_timer.h>
 #include <kvm/arm_vgic.h>
 
 #include "vgic.h"

@@ -143,10 +144,22 @@ static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void)
 	return vcpu;
 }
 

Quoted 8 lines

Quoted 5 lines

virt/kvm/arm/vgic/vgic-mmio.c static struct kvm_vcpu *vgic_get_mmio_requester_vcpu(void) · line 147

Quoted 1 line

virt/kvm/arm/vgic/vgic-mmio.c +1 -0

+/* Must be called with irq->irq_lock held */

Instead of enforcing this rule in comment, you can enforce it in code:

BUG_ON(!spin_is_locked(irq->irq_lock))

Are we going to litter the kernel with such assertions? I don't think that's a valuable thing to do.

That's what I agree - BUG-ifiyng is somewhat debugging technique and should be avoided in release code. But as I can see, in kvm code BUG()s are widely used:

$ find . -name "*.c" | xargs grep -w 'BUG\|BUG_ON' | grep kvm | wc -l
155

So I tuned my littering detector. :)

In this patchset new BUG()s are added in patches 4 and 6. In patch 6 BUG() has meaning of TODO:

(quoted diff) +5 -0

+       if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+               timer = vcpu_vtimer(vcpu);
+       else
+               BUG(); /* We only map the vtimer so far */
+

Which is far from original purpose of BUG().

If you think that BUG() is not OK in this case (and I agree with it), I think they should be also removed from patches 4 and 6. 6 - for sure.

From: Christoffer Dall <[email protected]>

The VGIC can now support the life-cycle of mapped level-triggered interrupts, and we no longer have to read back the timer state on every exit from the VM if we had an asserted timer interrupt signal, because the VGIC already knows if we hit the unlikely case where the guest disables the timer without ACKing the virtual timer interrupt.

This means we rework a bit of the code to factor out the functionality to snapshot the timer state from vtimer_save_state(), and we can reuse this functionality in the sync path when we have an irqchip in userspace, and also to support our implementation of the get_input_level() function for the timer.

This change also means that we can no longer rely on the timer's view of the interrupt line to set the active state, because we no longer maintain this state for mapped interrupts when exiting from the guest. Instead, we only set the active state if the virtual interrupt is active, and otherwise we simply let the timer fire again and raise the virtual interrupt from the ISR.

Signed-off-by Christoffer Dall <[email protected]>

---
 include/kvm/arm_arch_timer.h |  2 ++
 virt/kvm/arm/arch_timer.c    | 75 +++++++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 39 deletions(-)

include/kvm/arm_arch_timer.h +2 -0

index 01ee473517e2..f57f795d704c 100644
@@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
 
 void kvm_timer_init_vhe(void);
 
+bool kvm_arch_timer_get_input_level(int vintid);
+
 #define vcpu_vtimer(v)	(&(v)->arch.timer_cpu.vtimer)
 #define vcpu_ptimer(v)	(&(v)->arch.timer_cpu.ptimer)
 

include/kvm/arm_arch_timer.h +5 -0

+bool kvm_arch_timer_get_input_level(int vintid)
+{
+	struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
+	struct arch_timer_context *timer;
+

int kvm_timer_enable(struct kvm_vcpu *vcpu)
{

include/kvm/arm_arch_timer.h +1 -1

 	struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;

@@ -841,7 +838,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
 	}
 
 	ret = kvm_vgic_map_phys_irq(vcpu, host_vtimer_irq, vtimer->irq.irq,
-				    NULL);
+				    kvm_arch_timer_get_input_level);
 	if (ret)
 		return ret;
 

Signature · 1 line

2.14.2

Quoted 40 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

The VGIC can now support the life-cycle of mapped level-triggered interrupts, and we no longer have to read back the timer state on every exit from the VM if we had an asserted timer interrupt signal, because the VGIC already knows if we hit the unlikely case where the guest disables the timer without ACKing the virtual timer interrupt.

This means we rework a bit of the code to factor out the functionality to snapshot the timer state from vtimer_save_state(), and we can reuse this functionality in the sync path when we have an irqchip in userspace, and also to support our implementation of the get_input_level() function for the timer.

This change also means that we can no longer rely on the timer's view of the interrupt line to set the active state, because we no longer maintain this state for mapped interrupts when exiting from the guest. Instead, we only set the active state if the virtual interrupt is active, and otherwise we simply let the timer fire again and raise the virtual interrupt from the ISR.

Signed-off-by Christoffer Dall <[email protected]>

---
 include/kvm/arm_arch_timer.h |  2 ++
 virt/kvm/arm/arch_timer.c    | 75 +++++++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 39 deletions(-)

include/kvm/arm_arch_timer.h +2 -0

index 01ee473517e2..f57f795d704c 100644
@@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
 
 void kvm_timer_init_vhe(void);
 
+bool kvm_arch_timer_get_input_level(int vintid);
+
 #define vcpu_vtimer(v)	(&(v)->arch.timer_cpu.vtimer)
 #define vcpu_ptimer(v)	(&(v)->arch.timer_cpu.ptimer)
 

[...]

Quoted 5 lines

include/kvm/arm_arch_timer.h +5 -0

+bool kvm_arch_timer_get_input_level(int vintid)
+{
+	struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
+	struct arch_timer_context *timer;
+

include/kvm/arm_arch_timer.h

Quoted 10 lines

include/kvm/arm_arch_timer.h +10 -0

+	if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+		timer = vcpu_vtimer(vcpu);
+	else
+		BUG(); /* We only map the vtimer so far */
+
+	if (timer->loaded)
+		__timer_snapshot_state(timer);
+
+	return kvm_timer_should_fire(timer);
+}

I think it worth to reword to highlight your intention about BUG, and save 1 call to vcpu_vtimer()

bool kvm_arch_timer_get_input_level(int vintid)

{
	struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
	struct arch_timer_context *timer = vcpu_vtimer(vcpu);

        /* We only map the vtimer so far */
	BUG_ON(vintid != timer->irq.irq)

	if (timer->loaded)
		__timer_snapshot_state(timer);

	return kvm_timer_should_fire(timer);
}

Quoted 64 lines · Yury Norov

Quoted 40 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

The VGIC can now support the life-cycle of mapped level-triggered interrupts, and we no longer have to read back the timer state on every exit from the VM if we had an asserted timer interrupt signal, because the VGIC already knows if we hit the unlikely case where the guest disables the timer without ACKing the virtual timer interrupt.

This means we rework a bit of the code to factor out the functionality to snapshot the timer state from vtimer_save_state(), and we can reuse this functionality in the sync path when we have an irqchip in userspace, and also to support our implementation of the get_input_level() function for the timer.

This change also means that we can no longer rely on the timer's view of the interrupt line to set the active state, because we no longer maintain this state for mapped interrupts when exiting from the guest. Instead, we only set the active state if the virtual interrupt is active, and otherwise we simply let the timer fire again and raise the virtual interrupt from the ISR.

Signed-off-by Christoffer Dall <[email protected]>

---
 include/kvm/arm_arch_timer.h |  2 ++
 virt/kvm/arm/arch_timer.c    | 75 +++++++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 39 deletions(-)

include/kvm/arm_arch_timer.h +2 -0

index 01ee473517e2..f57f795d704c 100644
@@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
 
 void kvm_timer_init_vhe(void);
 
+bool kvm_arch_timer_get_input_level(int vintid);
+
 #define vcpu_vtimer(v)	(&(v)->arch.timer_cpu.vtimer)
 #define vcpu_ptimer(v)	(&(v)->arch.timer_cpu.ptimer)
 

[...]

Quoted 5 lines

include/kvm/arm_arch_timer.h +5 -0

+bool kvm_arch_timer_get_input_level(int vintid)
+{
+	struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
+	struct arch_timer_context *timer;
+

include/kvm/arm_arch_timer.h

Quoted 10 lines

include/kvm/arm_arch_timer.h +10 -0

+	if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+		timer = vcpu_vtimer(vcpu);
+	else
+		BUG() We only map the vtimer so far */
+
+	if (timer->loaded)
+		__timer_snapshot_state(timer);
+
+	return kvm_timer_should_fire(timer);
+}

I think it worth to reword to highlight your intention about BUG, and save 1 call to vcpu_vtimer()

bool kvm_arch_timer_get_input_level(int vintid)

Quoted 13 lines

{
	struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
	struct arch_timer_context *timer = vcpu_vtimer(vcpu);

        /* We only map the vtimer so far */
	BUG_ON(vintid != timer->irq.irq)

	if (timer->loaded)
		__timer_snapshot_state(timer);

	return kvm_timer_should_fire(timer);
}

Besides the incredible bikesheding nature of your comments, I disagree. The current code suggests where to add functionality once we move to using the physical timer hardware to drive an EL1 physical timer on VHE systems, and is purposely written this way.

I'm sure we have real bugs and real issues in the code, perhaps you could spend your energy looking for those, and if you cannot find any, then provide a reviewed-by instead of these pointless cosmetic adjustments.

Quoted 84 lines · Christoffer Dall

Quoted 64 lines · Yury Norov

Quoted 40 lines · Christoffer Dall

From: Christoffer Dall <[email protected]>

The VGIC can now support the life-cycle of mapped level-triggered interrupts, and we no longer have to read back the timer state on every exit from the VM if we had an asserted timer interrupt signal, because the VGIC already knows if we hit the unlikely case where the guest disables the timer without ACKing the virtual timer interrupt.

This means we rework a bit of the code to factor out the functionality to snapshot the timer state from vtimer_save_state(), and we can reuse this functionality in the sync path when we have an irqchip in userspace, and also to support our implementation of the get_input_level() function for the timer.

This change also means that we can no longer rely on the timer's view of the interrupt line to set the active state, because we no longer maintain this state for mapped interrupts when exiting from the guest. Instead, we only set the active state if the virtual interrupt is active, and otherwise we simply let the timer fire again and raise the virtual interrupt from the ISR.

Signed-off-by Christoffer Dall <[email protected]>

---
 include/kvm/arm_arch_timer.h |  2 ++
 virt/kvm/arm/arch_timer.c    | 75 +++++++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 39 deletions(-)

include/kvm/arm_arch_timer.h +2 -0

index 01ee473517e2..f57f795d704c 100644
@@ -90,6 +90,8 @@ void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
 
 void kvm_timer_init_vhe(void);
 
+bool kvm_arch_timer_get_input_level(int vintid);
+
 #define vcpu_vtimer(v)	(&(v)->arch.timer_cpu.vtimer)
 #define vcpu_ptimer(v)	(&(v)->arch.timer_cpu.ptimer)
 

[...]

Quoted 5 lines

include/kvm/arm_arch_timer.h +5 -0

+bool kvm_arch_timer_get_input_level(int vintid)
+{
+	struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
+	struct arch_timer_context *timer;
+

include/kvm/arm_arch_timer.h

Quoted 10 lines

include/kvm/arm_arch_timer.h +10 -0

+	if (vintid == vcpu_vtimer(vcpu)->irq.irq)
+		timer = vcpu_vtimer(vcpu);
+	else
+		BUG() We only map the vtimer so far */
+
+	if (timer->loaded)
+		__timer_snapshot_state(timer);
+
+	return kvm_timer_should_fire(timer);
+}

I think it worth to reword to highlight your intention about BUG, and save 1 call to vcpu_vtimer()

bool kvm_arch_timer_get_input_level(int vintid)

Quoted 13 lines

{
	struct kvm_vcpu *vcpu = kvm_arm_get_running_vcpu();
	struct arch_timer_context *timer = vcpu_vtimer(vcpu);

        /* We only map the vtimer so far */
	BUG_ON(vintid != timer->irq.irq)

	if (timer->loaded)
		__timer_snapshot_state(timer);

	return kvm_timer_should_fire(timer);
}

Besides the incredible bikesheding nature of your comments, I disagree. The current code suggests where to add functionality once we move to using the physical timer hardware to drive an EL1 physical timer on VHE systems, and is purposely written this way.

Quoted 4 lines

I'm sure we have real bugs and real issues in the code, perhaps you could spend your energy looking for those, and if you cannot find any, then provide a reviewed-by instead of these pointless cosmetic adjustments.

OK. I understood. Let me elaborate.

0. As you say in patch 0, This series is based on 4.15-rc, so I decided that the code above is assumed to be release version. You may change it in future, or may not, but the code will exist as is in mainline kernel for some time, right?

1. BUG() is needed to kill system, and it really kills it. This is wrong to kill system in else-branch of some minor helper due to unimplemented feature. You should use pr_err() or WARN_ON() instead. The best - return reasonable error and do everything possible on upper levels to keep system alive. What's really wrong in my comment is that I didn't suggest you switch to WARN_ON().

2. Calling the same function with the same argument twice in code path is also an issue. Besides the nasty feeling of that code, it might be dangerous. The most obvious scenario is when it returns different values due to change of internal state. I realize that vcpu_vtimer() is just a pointer dereference. But it may bite you painfully as well. I know it for sure because it bitten me once. Consider racing scenario in this patch:
https://patchwork.kernel.org/patch/9974327/

It may never become the problem, or may become one day. But fix is clear and obvious - why not taking it?

3. I will be really happy to provide tested-by and reviewed-by. But for doing that I need to actually test and review. It would be extremely helpful if you share your testing modules and scripts. I have access to several Cavium machines and can do before/after measurements. Right now I have few tests, but kvm is very complex system, and I'm not sure I measure what I'm actually going to. Guidance from KVM experts is what really lacks.