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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 12 Dec 2013, Ian Campbell wrote:
> 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@xxxxxxxxxxxxx>
> > 
> > ---
> > 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?

You got the test wrong.
Assuming irq != evtchn_irq, it would be:

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

So the guest would loose a potential future interrupt notification. On
the other hand the guest hasn't acked the interrupt yet, so it is not
supposed to be able to receive a second interrupt notification at this
time. Moreover consider that a second interrupt at this time can only
happen with hardware edge-trigger interrupts. This race exists even on
bare metal between the second interrupt and the guest ACKing the first.
I think it should be OK.


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

Still assuming irq != evtchn_irq, the other case would be:

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

so the guest would get one notification more from the hardware.
It can only happen with hardware edge-trigger interrupts.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel