On 28/08/17 19:18, Christoffer Dall wrote:

On Mon, Jul 31, 2017 at 06:26:24PM +0100, Marc Zyngier wrote:

quoted

The current implementation of MOVALL doesn't allow us to call
into the core ITS code as we hold a number of spinlocks.

Let's try a method used in other parts of the code, were we copy
the intids of the candicate interrupts, and then do whatever
we need to do with them outside of the critical section.

This allows us to move the interrupts one by one, at the expense
of a bit of CPU time. Who cares? MOVALL is such a stupid command
anyway...

Signed-off-by: Marc Zyngier <redacted>
---
 virt/kvm/arm/vgic/vgic-its.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 2c065c970ba0..65cc77fde609 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c

@@ -1147,11 +1147,12 @@ static int vgic_its_cmd_handle_invall(struct kvm *kvm, struct vgic_its *its,
 static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
 				      u64 *its_cmd)
 {
-	struct vgic_dist *dist = &kvm->arch.vgic;
 	u32 target1_addr = its_cmd_get_target_addr(its_cmd);
 	u32 target2_addr = its_cmd_mask_field(its_cmd, 3, 16, 32);
 	struct kvm_vcpu *vcpu1, *vcpu2;
 	struct vgic_irq *irq;
+	u32 *intids;
+	int irq_count, i;
 
 	if (target1_addr >= atomic_read(&kvm->online_vcpus) ||
 	    target2_addr >= atomic_read(&kvm->online_vcpus))

@@ -1163,19 +1164,31 @@ static int vgic_its_cmd_handle_movall(struct kvm *kvm, struct vgic_its *its,
 	vcpu1 = kvm_get_vcpu(kvm, target1_addr);
 	vcpu2 = kvm_get_vcpu(kvm, target2_addr);
 
-	spin_lock(&dist->lpi_list_lock);
+	irq_count = vgic_copy_lpi_list(vcpu1, &intids);
+	if (irq_count < 0)
+		return irq_count;
 
-	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
-		spin_lock(&irq->irq_lock);
+	for (i = 0; i < irq_count; i++) {
+		irq = vgic_get_irq(kvm, NULL, intids[i]);
+		if (!irq)
+			continue;

Getting irq == NULL means that we've removed this LPI since
vgic_copy_lpi_list, right?  Can this really happen while we hold the its
mutex?

A disappearing LPI can only be the result of a DISCARD, which cannot
happen, as we indeed hold the ITS lock.

Also, we don't check this in its_sync_lpi_pending_table which would
indicate that we either have a bug there or are being overly careful
here (or should change the continue to BUG).

Let's aim for consistency. I'll drop this test.

quoted

 
 		if (irq->target_vcpu == vcpu1)
 			irq->target_vcpu = vcpu2;
 
-		spin_unlock(&irq->irq_lock);

Is it safe to modify target_vcpu without holding the irq_lock?

Unintentional regression. I'll fix that. But I wonder if there is an
actual point in testing testing the target_vcpu here. Since we hold the
ITS lock, we're damn sure that the affinity can't be changed, right?

Thanks,

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