Hi Marc,
can you have a quick look below?
On 18/05/16 14:01, Christoffer Dall wrote:
>> +
>> +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,
>
> 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.
>
>> + 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 direct
>
>
> Thanks,
> -Christoffer
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
