KVM: arm/arm64: vgic: Disallow Active+Pending for level interrupts

It was recently reported that VFIO mediated devices, and anything
that VFIO exposes as level interrupts, do no strictly follow the
expected logic of such interrupts as it only lowers the input
line when the guest has EOId the interrupt at the GIC level, rather
than when it Acked the interrupt at the device level.

THe GIC's Active+Pending state is fundamentally incompatible with
this behaviour, as it prevents KVM from observing the EOI, and in
turn results in VFIO never dropping the line. This results in an
interrupt storm in the guest, which it really never expected.

As we cannot really change VFIO to follow the strict rules of level
signalling, let's forbid the A+P state altogether, as it is in the
end only an optimization. It ensures that we will transition via
an invalid state, which we can use to notify VFIO of the EOI.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Shunyong Yang <shunyong.yang@hxt-semitech.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index 8327ea932aeacb03d96578ef24cc01fa40f37359..45aa433f018ffdebfd05cd7d2720ed996b57dc4d 100644 (file)
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -105,12 +105,9 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 
                /*
                 * Clear soft pending state when level irqs have been acked.
-                * Always regenerate the pending state.
                 */
-               if (irq->config == VGIC_CONFIG_LEVEL) {
-                       if (!(val & GICH_LR_PENDING_BIT))
-                               irq->pending_latch = false;
-               }
+               if (irq->config == VGIC_CONFIG_LEVEL && !(val & GICH_LR_STATE))
+                       irq->pending_latch = false;
 
                /*
                 * Level-triggered mapped IRQs are special because we only
@@ -153,8 +150,35 @@ void vgic_v2_fold_lr_state(struct kvm_vcpu *vcpu)
 void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 {
        u32 val = irq->intid;
+       bool allow_pending = true;
+
+       if (irq->active)
+               val |= GICH_LR_ACTIVE_BIT;
+
+       if (irq->hw) {
+               val |= GICH_LR_HW;
+               val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
+               /*
+                * Never set pending+active on a HW interrupt, as the
+                * pending state is kept at the physical distributor
+                * level.
+                */
+               if (irq->active)
+                       allow_pending = false;
+       } else {
+               if (irq->config == VGIC_CONFIG_LEVEL) {
+                       val |= GICH_LR_EOI;
 
-       if (irq_is_pending(irq)) {
+                       /*
+                        * Software resampling doesn't work very well
+                        * if we allow P+A, so let's not do that.
+                        */
+                       if (irq->active)
+                               allow_pending = false;
+               }
+       }
+
+       if (allow_pending && irq_is_pending(irq)) {
                val |= GICH_LR_PENDING_BIT;
 
                if (irq->config == VGIC_CONFIG_EDGE)
@@ -171,24 +195,6 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
                }
        }
 
-       if (irq->active)
-               val |= GICH_LR_ACTIVE_BIT;
-
-       if (irq->hw) {
-               val |= GICH_LR_HW;
-               val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
-               /*
-                * Never set pending+active on a HW interrupt, as the
-                * pending state is kept at the physical distributor
-                * level.
-                */
-               if (irq->active && irq_is_pending(irq))
-                       val &= ~GICH_LR_PENDING_BIT;
-       } else {
-               if (irq->config == VGIC_CONFIG_LEVEL)
-                       val |= GICH_LR_EOI;
-       }
-
        /*
         * Level-triggered mapped IRQs are special because we only observe
         * rising edges as input to the VGIC.  We therefore lower the line

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index e17f61a434fa872bf68695985df1d0eebe4717aa..8195f52ae6f0906c31432ca0ac4471d4bdd3c74a 100644 (file)
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -97,12 +97,9 @@ void vgic_v3_fold_lr_state(struct kvm_vcpu *vcpu)
 
                /*
                 * Clear soft pending state when level irqs have been acked.
-                * Always regenerate the pending state.
                 */
-               if (irq->config == VGIC_CONFIG_LEVEL) {
-                       if (!(val & ICH_LR_PENDING_BIT))
-                               irq->pending_latch = false;
-               }
+               if (irq->config == VGIC_CONFIG_LEVEL && !(val & ICH_LR_STATE))
+                       irq->pending_latch = false;
 
                /*
                 * Level-triggered mapped IRQs are special because we only
@@ -136,8 +133,35 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 {
        u32 model = vcpu->kvm->arch.vgic.vgic_model;
        u64 val = irq->intid;
+       bool allow_pending = true;
+
+       if (irq->active)
+               val |= ICH_LR_ACTIVE_BIT;
+
+       if (irq->hw) {
+               val |= ICH_LR_HW;
+               val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
+               /*
+                * Never set pending+active on a HW interrupt, as the
+                * pending state is kept at the physical distributor
+                * level.
+                */
+               if (irq->active)
+                       allow_pending = false;
+       } else {
+               if (irq->config == VGIC_CONFIG_LEVEL) {
+                       val |= ICH_LR_EOI;
 
-       if (irq_is_pending(irq)) {
+                       /*
+                        * Software resampling doesn't work very well
+                        * if we allow P+A, so let's not do that.
+                        */
+                       if (irq->active)
+                               allow_pending = false;
+               }
+       }
+
+       if (allow_pending && irq_is_pending(irq)) {
                val |= ICH_LR_PENDING_BIT;
 
                if (irq->config == VGIC_CONFIG_EDGE)
@@ -155,24 +179,6 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
                }
        }
 
-       if (irq->active)
-               val |= ICH_LR_ACTIVE_BIT;
-
-       if (irq->hw) {
-               val |= ICH_LR_HW;
-               val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
-               /*
-                * Never set pending+active on a HW interrupt, as the
-                * pending state is kept at the physical distributor
-                * level.
-                */
-               if (irq->active && irq_is_pending(irq))
-                       val &= ~ICH_LR_PENDING_BIT;
-       } else {
-               if (irq->config == VGIC_CONFIG_LEVEL)
-                       val |= ICH_LR_EOI;
-       }
-
        /*
         * Level-triggered mapped IRQs are special because we only observe
         * rising edges as input to the VGIC.  We therefore lower the line