Hi Marc, Some time ago, you suggested that we should communicate a change of the IRQ state via vgic_queue_irq_unlock [1], which needs to include dropping the IRQ from the VCPU's ap_list if the IRQ is not pending or enabled but on the ap_list. And I additionally add a case where the IRQ has to be migrated to another ap_list. (maybe you forget this...) Does this patch match your thought at the time? [1] https://lore.kernel.org/patchwork/patch/1371884/ Signed-off-by: Shenming Lu <lushenming@xxxxxxxxxx> --- arch/arm64/kvm/vgic/vgic.c | 116 ++++++++++++++++++++++++------------- 1 file changed, 75 insertions(+), 41 deletions(-) diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c index 15b666200f0b..9b88d49aa439 100644 --- a/arch/arm64/kvm/vgic/vgic.c +++ b/arch/arm64/kvm/vgic/vgic.c @@ -326,8 +326,9 @@ static bool vgic_validate_injection(struct vgic_irq *irq, bool level, void *owne /* * Check whether an IRQ needs to (and can) be queued to a VCPU's ap list. - * Do the queuing if necessary, taking the right locks in the right order. - * Returns true when the IRQ was queued, false otherwise. + * Do the queuing, dropping or migrating if necessary, taking the right + * locks in the right order. Returns true when the IRQ was queued, false + * otherwise. * * Needs to be entered with the IRQ lock already held, but will return * with all locks dropped. @@ -335,49 +336,38 @@ static bool vgic_validate_injection(struct vgic_irq *irq, bool level, void *owne bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq, unsigned long flags) { + struct kvm_vcpu *target_vcpu; struct kvm_vcpu *vcpu; + bool ret = false; lockdep_assert_held(&irq->irq_lock); retry: - vcpu = vgic_target_oracle(irq); - if (irq->vcpu || !vcpu) { + target_vcpu = vgic_target_oracle(irq); + vcpu = irq->vcpu; + if (target_vcpu == vcpu) { /* - * If this IRQ is already on a VCPU's ap_list, then it - * cannot be moved or modified and there is no more work for + * If this IRQ's state is consistent with whether on + * the right ap_lsit or not, there is no more work for * us to do. - * - * Otherwise, if the irq is not pending and enabled, it does - * not need to be inserted into an ap_list and there is also - * no more work for us to do. */ raw_spin_unlock_irqrestore(&irq->irq_lock, flags); - - /* - * We have to kick the VCPU here, because we could be - * queueing an edge-triggered interrupt for which we - * get no EOI maintenance interrupt. In that case, - * while the IRQ is already on the VCPU's AP list, the - * VCPU could have EOI'ed the original interrupt and - * won't see this one until it exits for some other - * reason. - */ - if (vcpu) { - kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu); - kvm_vcpu_kick(vcpu); - } - return false; + goto out; } /* * We must unlock the irq lock to take the ap_list_lock where - * we are going to insert this new pending interrupt. + * we are going to insert/drop this IRQ. */ raw_spin_unlock_irqrestore(&irq->irq_lock, flags); /* someone can do stuff here, which we re-check below */ - raw_spin_lock_irqsave(&vcpu->arch.vgic_cpu.ap_list_lock, flags); + if (target_vcpu) + raw_spin_lock_irqsave(&target_vcpu->arch.vgic_cpu.ap_list_lock, + flags); + if (vcpu) + raw_spin_lock_irqsave(&vcpu->arch.vgic_cpu.ap_list_lock, flags); raw_spin_lock(&irq->irq_lock); /* @@ -392,30 +382,74 @@ bool vgic_queue_irq_unlock(struct kvm *kvm, struct vgic_irq *irq, * In both cases, drop the locks and retry. */ - if (unlikely(irq->vcpu || vcpu != vgic_target_oracle(irq))) { + if (unlikely(target_vcpu != vgic_target_oracle(irq) || + vcpu != irq->vcpu)) { raw_spin_unlock(&irq->irq_lock); - raw_spin_unlock_irqrestore(&vcpu->arch.vgic_cpu.ap_list_lock, - flags); + if (target_vcpu) + raw_spin_unlock_irqrestore(&target_vcpu->arch.vgic_cpu.ap_list_lock, + flags); + if (vcpu) + raw_spin_unlock_irqrestore(&vcpu->arch.vgic_cpu.ap_list_lock, + flags); raw_spin_lock_irqsave(&irq->irq_lock, flags); goto retry; } - /* - * Grab a reference to the irq to reflect the fact that it is - * now in the ap_list. - */ - vgic_get_irq_kref(irq); - list_add_tail(&irq->ap_list, &vcpu->arch.vgic_cpu.ap_list_head); - irq->vcpu = vcpu; + if (!vcpu && target_vcpu) { + /* + * Insert this new pending interrupt. + * Grab a reference to the irq to reflect the fact that + * it is now in the ap_list. + */ + vgic_get_irq_kref(irq); + list_add_tail(&irq->ap_list, + &target_vcpu->arch.vgic_cpu.ap_list_head); + irq->vcpu = target_vcpu; + ret = true; + } else if (vcpu && !target_vcpu) { + /* + * This IRQ is not pending or enabled but on the ap_list, + * drop it from the ap_list. + */ + list_del(&irq->ap_list); + irq->vcpu = NULL; + raw_spin_unlock(&irq->irq_lock); + vgic_put_irq(vcpu->kvm, irq); + raw_spin_unlock_irqrestore(&vcpu->arch.vgic_cpu.ap_list_lock, + flags); + goto out; + } else { + /* This IRQ looks like it has to be migrated. */ + list_del(&irq->ap_list); + list_add_tail(&irq->ap_list, + &target_vcpu->arch.vgic_cpu.ap_list_head); + irq->vcpu = target_vcpu; + } raw_spin_unlock(&irq->irq_lock); - raw_spin_unlock_irqrestore(&vcpu->arch.vgic_cpu.ap_list_lock, flags); + if (target_vcpu) + raw_spin_unlock_irqrestore(&target_vcpu->arch.vgic_cpu.ap_list_lock, + flags); + if (vcpu) + raw_spin_unlock_irqrestore(&vcpu->arch.vgic_cpu.ap_list_lock, flags); - kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu); - kvm_vcpu_kick(vcpu); +out: + /* + * Even for the already queuing rightly case we have + * to kick the VCPU, because we could be queueing an + * edge-triggered interrupt for which we get no EOI + * maintenance interrupt. In that case, while the IRQ + * is already on the VCPU's AP list, the VCPU could + * have EOI'ed the original interrupt and won't see + * this one until it exits for some other reason. + */ + if (target_vcpu) { + kvm_make_request(KVM_REQ_IRQ_PENDING, target_vcpu); + kvm_vcpu_kick(target_vcpu); + } - return true; + return ret; } /** -- 2.19.1 _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
