Re: [PATCH v8 03/10] xen/arm: inflight irqs during migration

[Ian Campbell <Ian.Campbell@citrix.com>]

On Thu, 2014-07-10 at 19:13 +0100, Stefano Stabellini wrote:
> We need to take special care when migrating irqs that are already
> inflight from one vcpu to another. See "The effect of changes to an
> GICD_ITARGETSR", part of chapter 4.3.12 of the ARM Generic Interrupt
> Controller Architecture Specification.
> 
> The main issue from the Xen point of view is that the lr_pending and
> inflight lists are per-vcpu. The lock we take to protect them is also
> per-vcpu.
> 
> In order to avoid issues, if the irq is still lr_pending, we can
> immediately move it to the new vcpu for injection.
> 
> Otherwise if it is in a GICH_LR register, set a new flag
> GIC_IRQ_GUEST_MIGRATING, so that we can recognize when we receive an irq
> while the previous one is still inflight (given that we are only dealing
> with hardware interrupts here, it just means that its LR hasn't been
> cleared yet on the old vcpu).  If GIC_IRQ_GUEST_MIGRATING is set, we
> only set GIC_IRQ_GUEST_QUEUED and interrupt the old vcpu. To know which
> one is the old vcpu, we introduce a new field to pending_irq, called
> vcpu_migrate_from.
> When clearing the LR on the old vcpu, we take special care of injecting
> the interrupt into the new vcpu. To do that we need to release the old
> vcpu lock before taking the new vcpu lock.

I still think this is an awful lot of complexity and scaffolding for
something which is rare on the scale of things and which could be almost
trivially handled by requesting a maintenance interrupt for one EOI and
completing the move at that point.

In order to avoid a simple maint interrupt you are adding code to the
normal interrupt path and a potential SGI back to another processor (and
I hope I'm misreading this but it looks like an SGI back again to finish
off?). That's got to be way more costly to the first interrupt on the
new VCPU than the cost of a maintenance IRQ on the old one.

I think avoiding maintenance interrupts in general is a worthy goal, but
there are times when they are the most appropriate mechanism.

> @@ -344,6 +385,21 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>      }
>  
>      set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> +    vcpu_migrate_from = n->vcpu_migrate_from;
> +    /* update QUEUED before MIGRATING */
> +    smp_wmb();
> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
> +    {
> +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +
> +        /* The old vcpu must have EOIed the SGI but not cleared the LR.
> +         * Give it a kick. */

You mean SPI I think.

Ian.