On Wed, Sep 5, 2012 at 1:02 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> Recomputing the whole GIC state when injecting a single interrupt
> is a tiny bit silly.
>
> Instead, just computing the new state for this particular IRQ saves
> a bit of processing and makes things noticeably faster.
noticeably how?
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> arch/arm/include/asm/kvm_vgic.h | 1 +
> arch/arm/kvm/vgic.c | 69 ++++++++++++++++++++++++++++++-----------
> 2 files changed, 52 insertions(+), 18 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
> index 633e697..1f7fd32 100644
> --- a/arch/arm/include/asm/kvm_vgic.h
> +++ b/arch/arm/include/asm/kvm_vgic.h
> @@ -183,6 +183,7 @@ struct vgic_dist {
> u8 irq_sgi_sources[VGIC_MAX_CPUS][16];
>
> /* Target CPU for each IRQ */
> + u8 irq_spi_cpu[VGIC_NR_SHARED_IRQS];
> struct vgic_bitmap irq_spi_target[VGIC_MAX_CPUS];
>
> /* Bitmap indicating which CPU has something pending */
> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
> index 5177792..5f32772 100644
> --- a/arch/arm/kvm/vgic.c
> +++ b/arch/arm/kvm/vgic.c
> @@ -47,6 +47,9 @@
> * registers, stored on each vcpu. We only keep one bit of
> * information per interrupt, making sure that only one vcpu can
> * accept the interrupt.
> + * The same is true when injecting an interrupt, except that we only
> + * consider a single interrupt at a time. The irq_spi_cpu array
> + * contains the target CPU for each SPI.
> *
> * The handling of level interrupts adds some extra complexity. We
> * need to track when the interrupt has been EOIed, so we can sample
> @@ -313,6 +316,7 @@ static void vgic_set_target_reg(struct kvm *kvm, u32 val, int irq)
> int shift = i * 8;
> target = ffs((val >> shift) & 0xffU);
> target = target ? (target - 1) : 0;
> + dist->irq_spi_cpu[irq] = target;
> kvm_for_each_vcpu(c, vcpu, kvm) {
> bmap = vgic_bitmap_get_shared_map(&dist->irq_spi_target[c]);
> if (c == target)
> @@ -866,23 +870,19 @@ static void vgic_kick_vcpus(struct kvm *kvm)
> }
> }
>
> -int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, const struct kvm_irq_level *irq)
> +static bool vgic_update_irq_state(struct kvm *kvm, int cpuid,
> + const struct kvm_irq_level *irq)
> {
> struct vgic_dist *dist = &kvm->arch.vgic;
> - int nrcpus = atomic_read(&kvm->online_vcpus);
> - int is_edge, state;
> + struct kvm_vcpu *vcpu;
> + int is_edge, state, level;
> + int pend, enabled;
> unsigned long flags;
> - bool updated_state = false;
>
> - if (cpuid >= nrcpus)
> - return -EINVAL;
> -
> - /* Only PPIs or SPIs */
> - if (irq->irq >= VGIC_NR_IRQS || irq->irq < 16)
> - return -EINVAL;
> + level = !!irq->level;
>
> - kvm_debug("Inject IRQ%d\n", irq->irq);
> spin_lock_irqsave(&dist->lock, flags);
> +
> is_edge = vgic_irq_is_edge(dist, irq->irq);
> state = vgic_bitmap_get_irq_val(&dist->irq_state, cpuid, irq->irq);
>
> @@ -891,16 +891,49 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, const struct kvm_irq_level *
> * - level triggered and we change level
> * - edge triggered and we have a rising edge
> */
> - if ((!is_edge && (state ^ !!irq->level)) ||
> - (is_edge && !state && irq->level)) {
> - vgic_bitmap_set_irq_val(&dist->irq_state, cpuid,
> - irq->irq, !!irq->level);
> - vgic_update_state(kvm);
> - updated_state = true;
> + if (!((!is_edge && (state ^ level)) ||
> + (is_edge && !state && level))) {
> + spin_unlock_irqrestore(&dist->lock, flags);
> + return false;
> + }
wow, this one really hurts now, I'll probably have a go with my good
old friend De Morgan here and try to get rid of the double negation
and fit the comment a little better.
> +
> + vgic_bitmap_set_irq_val(&dist->irq_state, cpuid, irq->irq, level);
> +
> + enabled = vgic_bitmap_get_irq_val(&dist->irq_enabled, cpuid, irq->irq);
> + pend = level && enabled;
> +
> + if (irq->irq >= 32) {
> + cpuid = dist->irq_spi_cpu[irq->irq - 32];
> + pend &= vgic_bitmap_get_irq_val(&dist->irq_spi_target[cpuid],
> + 0, irq->irq);
> }
> +
> + kvm_debug("Inject IRQ%d level %d CPU%d\n", irq->irq, level, cpuid);
> +
> + vcpu = kvm_get_vcpu(kvm, cpuid);
> + if (pend) {
> + set_bit(irq->irq, vcpu->arch.vgic_cpu.pending);
> + set_bit(cpuid, &dist->irq_pending_on_cpu);
> + } else
> + clear_bit(irq->irq, vcpu->arch.vgic_cpu.pending);
> +
> spin_unlock_irqrestore(&dist->lock, flags);
>
> - if (updated_state)
> + return true;
> +}
> +
> +int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, const struct kvm_irq_level *irq)
> +{
> + int nrcpus = atomic_read(&kvm->online_vcpus);
> +
> + if (cpuid >= nrcpus)
> + return -EINVAL;
> +
> + /* Only PPIs or SPIs */
> + if (irq->irq >= VGIC_NR_IRQS || irq->irq < 16)
> + return -EINVAL;
> +
> + if (vgic_update_irq_state(kvm, cpuid, irq))
> vgic_kick_vcpus(kvm);
>
> return 0;
> --
> 1.7.12
>
this looks reasonable, I'll apply it on top of the irq rework and
change it so it fits.
I assume you've tested this as usual?
thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
