Re: [PATCH v4 2/6] xen/arm: track the state of guest IRQs

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

On Wed, 2013-12-11 at 19:07 +0000, Stefano Stabellini wrote:
> Introduce a status field in struct pending_irq. Valid states are
> GUEST_PENDING, GUEST_VISIBLE and GUEST_ENABLED and they are not mutually
> exclusive.  See the in-code comment for an explanation of the states and
> how they are used.
> Use atomic operations to set and clear the status bits. Note that
> setting GIC_IRQ_GUEST_VISIBLE and clearing GIC_IRQ_GUEST_PENDING can be
> done in two separate operations as the underlying pending status is
> actually only cleared on the LR after the guest ACKs the interrupts.
> Until that happens it's not possible to receive another interrupt.
> 
> The main effect of this patch is that an IRQ can be set to GUEST_PENDING
> while it is being serviced by the guest. In maintenance_interrupt we
> check whether GUEST_PENDING is set and if it is we add the irq back into
> the lr_pending queue so that it's going to be reinjected one more time,
> if the interrupt is still enabled at the vgicd level.
> If it is not, it is going to be injected as soon as the guest renables
> the interrupt.
> 
> One exception is evtchn_irq: in that case we don't want to
> set the GIC_IRQ_GUEST_PENDING bit if it is already GUEST_VISIBLE,
> because as part of the event handling loop, the guest would realize that
> new events are present even without a new notification.
> Also we already have a way to figure out exactly when we do need to
> inject a second notification if vgic_vcpu_inject_irq is called after the
> end of the guest event handling loop and before the guest EOIs the
> interrupt (see db453468d92369e7182663fb13e14d83ec4ce456 "arm: vgic: fix
> race between evtchn upcall and evtchnop_send").
> 
> In maintenance_interrupt only stop injecting interrupts if no new
> interrupts have been added to the LRs.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> Changes in v2:
> - use atomic operations to modify the pending_irq status bits, remove
> the now unnecessary locks;
> - make status unsigned long;
> - in maintenance_interrupt only stops injecting interrupts if no new
> interrupts have been added to the LRs;
> - add a state to keep track whether the guest irq is enabled at the
> vgicd level;
> 
> Changes in v3:
> - do not set the GUEST_PENDING bit for evtchn_irq if the irq is already
> guest visible.
> 
> Changes in v4:
> - move set_bit _GIC_IRQ_GUEST_VISIBLE and clear_bit
> _GIC_IRQ_GUEST_PENDING to gic_set_lr;
> - turn set_int into a bool_t;
> - remove raw GIC_IRQ_GUEST values;
> - in maintenance_interrupt if the irq is still PENDING, add it back into
> the lr_pending queue instead of immediately reinjecting it.
> ---
>  xen/arch/arm/gic.c           |   54 ++++++++++++++++++++++++++++++++----------
>  xen/arch/arm/vgic.c          |   17 ++++++++++---
>  xen/include/asm-arm/domain.h |   37 +++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 5f7a548..19e0ae4 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -615,6 +615,7 @@ static inline void gic_set_lr(int lr, unsigned int virtual_irq,
>          unsigned int state, unsigned int priority)
>  {
>      int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> +    struct pending_irq *p = irq_to_pending(current, virtual_irq);
>  
>      BUG_ON(lr >= nr_lrs);
>      BUG_ON(lr < 0);
> @@ -624,6 +625,9 @@ static inline void gic_set_lr(int lr, unsigned int virtual_irq,
>          maintenance_int |
>          ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
>          ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +
> +    set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> +    clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);

So WRT races in vgic_vcpu_inject_irq we were discussing on v3. The above
vs:

+        if ( (irq != current->domain->arch.evtchn_irq) ||
+             (!test_bit(_GIC_IRQ_GUEST_VISIBLE, &n->status)) )
+            set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);

If one cpu (A) is in gic_set_lr while another (B) is in
vgic_vcpu_inject_irq, is there any ordering which fails to do the right
thing.

        A -- set VISIBLE
        B -- test VISIBLE
        B -- set PENDING
        A -- clear PENDING

Doesn't seem right?

        B -- test VISIBLE
        A -- set VISIBLE
        A -- clear PENDING
        B -- doesn't set PENDING
        
Seems OK.

        A -- set VISIBLE
    B -- test VISIBLE
    A -- clear PENDING
    B -- set PENDING

OK?

        B -- test VISIBLE
    A -- set VISIBLE
        B -- set PENDING
        A -- clear PENDING
   
Wrong?

If A clear's pending first does that work out right every time?

Ian.