On Fri, May 06, 2016 at 11:45:39AM +0100, Andre Przywara wrote:
> The active register handlers are shared between the v2 and v3
> emulation, so their implementation goes into vgic-mmio.c, to be
> easily referenced from the v3 emulation as well later.
> Since activation/deactivation of an interrupt may happen entirely
> in the guest without it ever exiting, we need some extra logic to
> properly track the active state.
> Putting it on an ap_list on activation is similar to the normal case
> handled by vgic_queue_irq_unlock(), but differs in some details that
> make a separate implementation worthwhile.
>
> Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
> ---
> Changelog RFC..v1:
> - handling queueing in write handler
> - remove IRQ lock from read handler
>
> Changelog v1 .. v2:
> - adapt to new MMIO framework
>
> virt/kvm/arm/vgic/vgic-mmio-v2.c | 4 +-
> virt/kvm/arm/vgic/vgic-mmio.c | 120
> +++++++++++++++++++++++++++++++++++++++
> virt/kvm/arm/vgic/vgic-mmio.h | 10 ++++
> 3 files changed, 132 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index 4b87e0a..054b52d 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -80,9 +80,9 @@ static const struct vgic_register_region
> vgic_v2_dist_registers[] = {
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR,
> vgic_mmio_read_pending, vgic_mmio_write_cpending, 1),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1),
> + vgic_mmio_read_active, vgic_mmio_write_sactive, 1),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_CLEAR,
> - vgic_mmio_read_raz, vgic_mmio_write_wi, 1),
> + vgic_mmio_read_active, vgic_mmio_write_cactive, 1),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PRI,
> vgic_mmio_read_raz, vgic_mmio_write_wi, 8),
> REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_TARGET,
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.c b/virt/kvm/arm/vgic/vgic-mmio.c
> index 4df1af7..dbf683e 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio.c
> @@ -162,6 +162,126 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> }
> }
>
> +unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + u32 intid = (addr & 0x7f) * 8;
> + u32 value = 0;
> + int i;
> +
> + /* Loop over all IRQs affected by this read */
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + if (irq->active)
> + value |= (1U << i);
> + }
> +
> + return extract_bytes(value, addr & 3, len);
> +}
> +
> +void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u32 intid = (addr & 0x7f) * 8;
> + 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);
> +
> + irq->active = false;
> +
> + /*
> + * Christoffer wrote:
> + * The question is what to do if the vcpu for this irq is
> + * running and the LR there has the active bit set, then we'll
> + * overwrite this change when we fold the LR state back into
> + * the vgic_irq struct.
> + *
> + * Since I expect this to be extremely rare, one option is to
> + * force irq->vcpu to exit (if non-null) and then do you
> + * thing here after you've confirm it has exited while holding
> + * some lock preventing it from re-entering again.
> + * Slightly crazy.
> + *
> + * The alternative is to put a big fat comment nothing that
> + * this is non-supported bad race, and wait until someone
> + * submits a bug report relating to this...
> + */
I thought I wrote a patch for this?
> +
> + spin_unlock(&irq->irq_lock);
> + }
> +}
> +
> +void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val)
> +{
> + u32 intid = (addr & 0x7f) * 8;
> + 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);
> +
> + /* As this is a special case, we can't use the
> + * vgic_queue_irq_unlock() function to put this on a VCPU.
> + * So deal with this here explicitly unless the IRQs was
> + * already active, it was on a VCPU before or there is no
> + * target VCPU assigned at the moment.
> + */
> + if (irq->active || irq->vcpu || !irq->target_vcpu) {
> + irq->active = true;
> +
> + spin_unlock(&irq->irq_lock);
> + continue;
> + }
> +
> + spin_unlock(&irq->irq_lock);
> +retry:
> + vcpu = irq->target_vcpu;
> +
> + spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
> + spin_lock(&irq->irq_lock);
> +
> + /*
> + * Recheck after dropping the IRQ lock to see if we should
> + * still care about queueing it.
> + */
> + if (irq->active || irq->vcpu) {
> + irq->active = true;
> +
> + spin_unlock(&irq->irq_lock);
> + spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> +
> + continue;
> + }
> +
> + /* Did the target VCPU change while we had the lock dropped? */
> + if (vcpu != irq->target_vcpu) {
> + spin_unlock(&irq->irq_lock);
> + spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> +
> + goto retry;
> + }
> +
> + /* Now queue the IRQ to the VCPU's ap_list. */
> + list_add_tail(&irq->ap_list, &vcpu->arch.vgic_cpu.ap_list_head);
> + irq->vcpu = vcpu;
> +
> + irq->active = true;
> +
> + spin_unlock(&irq->irq_lock);
> + spin_unlock(&vcpu->arch.vgic_cpu.ap_list_lock);
> +
> + kvm_vcpu_kick(vcpu);
quite frankly, this looks very complicated and I really think it's worth
reusing the queue function after seeing this (sorry I haven't been able
to before).
-Christoffer
> + }
> +}
> +
> static int match_region(const void *key, const void *elt)
> {
> const unsigned int offset = (unsigned long)key;
> diff --git a/virt/kvm/arm/vgic/vgic-mmio.h b/virt/kvm/arm/vgic/vgic-mmio.h
> index d4fc029..fa875dc 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio.h
> +++ b/virt/kvm/arm/vgic/vgic-mmio.h
> @@ -96,6 +96,16 @@ void vgic_mmio_write_cpending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val);
>
> +unsigned long vgic_mmio_read_active(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len);
> +
> +void vgic_mmio_write_cactive(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val);
> +
> +void vgic_mmio_write_sactive(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len,
> + unsigned long val);
>
> unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
>
> --
> 2.7.3
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
