[PATCH 0/2] VGIC fixes for 3.9-rc1

[PATCH 0/2] VGIC fixes for 3.9-rc1

[Marc Zyngier]

I've just realised that these patches have never been on LAKML, and
only to kvm-arm. It is probably better to repost them here before they
get pulled in.

The first patch is just a cleanup, and has very little consequence on
the behaviour of the VGIC. The second one is more serious, and results
in lost interrupts under heavy load.

Patches are against mainline as of today, and tested on both TC2 and
arm64 model.

Cheers,

	M.

Marc Zyngier (2):
  ARM: KVM: vgic: force EOIed LRs to the empty state
  ARM: KVM: vgic: take distributor lock on sync_hwstate path

 arch/arm/kvm/vgic.c | 35 ++++++++++++++---------------------
 1 file changed, 14 insertions(+), 21 deletions(-)

-- 
1.7.12.4

[PATCH 1/2] ARM: KVM: vgic: force EOIed LRs to the empty state

[Marc Zyngier]

The VGIC doesn't guarantee that an EOIed LR that has been configured
to generate a maintenance interrupt will appear as empty.

While the code recovers from this situation, it is better to clean
the LR and flag it as empty so it can be quickly recycled.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/vgic.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
index c9a1731..76ea1aa 100644
--- a/arch/arm/kvm/vgic.c
+++ b/arch/arm/kvm/vgic.c
@@ -883,8 +883,7 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 			  lr, irq, vgic_cpu->vgic_lr[lr]);
 		BUG_ON(!test_bit(lr, vgic_cpu->lr_used));
 		vgic_cpu->vgic_lr[lr] |= GICH_LR_PENDING_BIT;
-
-		goto out;
+		return true;
 	}
 
 	/* Try to use another LR for this interrupt */
@@ -898,7 +897,6 @@ static bool vgic_queue_irq(struct kvm_vcpu *vcpu, u8 sgi_source_id, int irq)
 	vgic_cpu->vgic_irq_lr_map[irq] = lr;
 	set_bit(lr, vgic_cpu->lr_used);
 
-out:
 	if (!vgic_irq_is_edge(vcpu, irq))
 		vgic_cpu->vgic_lr[lr] |= GICH_LR_EOI;
 
@@ -1054,6 +1052,13 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 			} else {
 				vgic_cpu_irq_clear(vcpu, irq);
 			}
+
+			/*
+			 * Despite being EOIed, the LR may not have
+			 * been marked as empty.
+			 */
+			set_bit(lr, (unsigned long *)vgic_cpu->vgic_elrsr);
+			vgic_cpu->vgic_lr[lr] &= ~GICH_LR_ACTIVE_BIT;
 		}
 	}
 
-- 
1.7.12.4

[PATCH 2/2] ARM: KVM: vgic: take distributor lock on sync_hwstate path

[Marc Zyngier]

Now that the maintenance interrupt handling is actually out of the
handler itself, the code becomes quite racy as we can get preempted
while we process the state.

Wrapping this code around the distributor lock ensures that we're not
preempted and relatively race-free.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/vgic.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
index 76ea1aa..0e4cfe1 100644
--- a/arch/arm/kvm/vgic.c
+++ b/arch/arm/kvm/vgic.c
@@ -1016,21 +1016,6 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 
 	kvm_debug("MISR = %08x\n", vgic_cpu->vgic_misr);
 
-	/*
-	 * We do not need to take the distributor lock here, since the only
-	 * action we perform is clearing the irq_active_bit for an EOIed
-	 * level interrupt.  There is a potential race with
-	 * the queuing of an interrupt in __kvm_vgic_flush_hwstate(), where we
-	 * check if the interrupt is already active. Two possibilities:
-	 *
-	 * - The queuing is occurring on the same vcpu: cannot happen,
-	 *   as we're already in the context of this vcpu, and
-	 *   executing the handler
-	 * - The interrupt has been migrated to another vcpu, and we
-	 *   ignore this interrupt for this run. Big deal. It is still
-	 *   pending though, and will get considered when this vcpu
-	 *   exits.
-	 */
 	if (vgic_cpu->vgic_misr & GICH_MISR_EOI) {
 		/*
 		 * Some level interrupts have been EOIed. Clear their
@@ -1069,9 +1054,8 @@ static bool vgic_process_maintenance(struct kvm_vcpu *vcpu)
 }
 
 /*
- * Sync back the VGIC state after a guest run. We do not really touch
- * the distributor here (the irq_pending_on_cpu bit is safe to set),
- * so there is no need for taking its lock.
+ * Sync back the VGIC state after a guest run. The distributor lock is
+ * needed so we don't get preempted in the middle of the state processing.
  */
 static void __kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 {
@@ -1117,10 +1101,14 @@ void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
 
 void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
 {
+	struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
+
 	if (!irqchip_in_kernel(vcpu->kvm))
 		return;
 
+	spin_lock(&dist->lock);
 	__kvm_vgic_sync_hwstate(vcpu);
+	spin_unlock(&dist->lock);
 }
 
 int kvm_vgic_vcpu_pending_irq(struct kvm_vcpu *vcpu)
-- 
1.7.12.4

[PATCH 0/2] VGIC fixes for 3.9-rc1

[Will Deacon]

On Fri, Feb 22, 2013 at 06:40:44PM +0000, Marc Zyngier wrote:
> I've just realised that these patches have never been on LAKML, and
> only to kvm-arm. It is probably better to repost them here before they
> get pulled in.
> 
> The first patch is just a cleanup, and has very little consequence on
> the behaviour of the VGIC. The second one is more serious, and results
> in lost interrupts under heavy load.
> 
> Patches are against mainline as of today, and tested on both TC2 and
> arm64 model.

I'm probably turning up late to this party, but after wrapping my head
around the vgic once again:

  Reviewed-by: Will Deacon <will.deacon@arm.com>

for these two patches.

Will

[PATCH 0/2] VGIC fixes for 3.9-rc1

[Marc Zyngier]

On 25/02/13 13:49, Will Deacon wrote:
> On Fri, Feb 22, 2013 at 06:40:44PM +0000, Marc Zyngier wrote:
>> I've just realised that these patches have never been on LAKML, and
>> only to kvm-arm. It is probably better to repost them here before they
>> get pulled in.
>>
>> The first patch is just a cleanup, and has very little consequence on
>> the behaviour of the VGIC. The second one is more serious, and results
>> in lost interrupts under heavy load.
>>
>> Patches are against mainline as of today, and tested on both TC2 and
>> arm64 model.
> 
> I'm probably turning up late to this party, but after wrapping my head
> around the vgic once again:
> 
>   Reviewed-by: Will Deacon <will.deacon@arm.com>
> 
> for these two patches.

Thanks Will. I have some paracetamol if you need any... ;-)

	M.
-- 
Jazz is not dead. It just smells funny...