[PATCH v4 27/56] KVM: arm/arm64: vgic-new: Add ACTIVE registers handlers
From: andre.przywara@arm.com (Andre Przywara)
Date: 2016-05-19 10:12:55
Also in:
kvm, kvmarm
Hi Marc, can you have a quick look below? On 18/05/16 14:01, Christoffer Dall wrote:
quoted
+ +void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + int i; + + kvm_arm_halt_guest(vcpu->kvm); + for_each_set_bit(i, &val, len * 8) { + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); + + spin_lock(&irq->irq_lock); + /* + * If this virtual IRQ was written into a list register, we + * have to make sure the CPU that runs the VCPU thread has + * synced back LR state to the struct vgic_irq. We can only + * know this for sure, when either this irq is not assigned to + * anyone's AP list anymore, or the VCPU thread is not + * running on any CPUs. + * + * In the opposite case, we know the VCPU thread may be on its + * way back from the guest 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. + */ + while (irq->vcpu && /* IRQ may have state in an LR somewhere */ + irq->vcpu->cpu != -1) /* VCPU thread is running */ + cond_resched_lock(&irq->irq_lock); + + irq->active = false; + spin_unlock(&irq->irq_lock); + } + kvm_arm_resume_guest(vcpu->kvm); +} + +void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu, + gpa_t addr, unsigned int len, + unsigned long val) +{ + u32 intid = VGIC_ADDR_TO_INTID(addr, 1); + int i; + + for_each_set_bit(i, &val, len * 8) { + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i); + + spin_lock(&irq->irq_lock); + + /* + * If the IRQ was already active or it was on a VCPU before + * or there is no target VCPU assigned at the moment, then + * just proceed. + */ + if (irq->active || irq->vcpu || !irq->target_vcpu) {why is it that we don't care if this IRQ is on a LR, for example being just pending and not active, and we thereby loose this active state when the vcpu syncs back the state?
Mmmh, good question. I don't remember anymore why this irq->vcpu check was added. Marc, can you confirm that this is OK to be removed? I think it's an optimization anyway. Thanks, Andre,
quoted hunk ↗ jump to hunk
We care for the case where we clear the active state, but not when we set it. Perhaps we discussed this in the past, but now I've forgotten, so that should at least be documented somehow.quoted
+ irq->active = true; + + spin_unlock(&irq->irq_lock); + continue; + } + + irq->active = true; + vgic_queue_irq_unlock(vcpu->kvm, irq);this won't work just yet, but I think all that's needed to make it work is this patchlet:diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c index c22f7e2..27204f22 100644 --- a/virt/kvm/arm/vgic/vgic.c +++ b/virt/kvm/arm/vgic/vgic.c@@ -81,7 +81,7 @@ static struct kvm_vcpu *vgic_target_oracle(struct vgic_irq *irq) /* If the interrupt is active, it must stay on the current vcpu */ if (irq->active) - return irq->vcpu; + return irq->vcpu ? : irq->target_vcpu; /* * If the IRQ is not active but enabled and pending, we should directThanks, -Christoffer