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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-12-11 at 19:00 +0000, Stefano Stabellini wrote:
> On Wed, 11 Dec 2013, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 18:50 +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.
> > 
> > Is the ordering of the set/clear important? It seems like it ought to
> > be, one of the orderings risks losing information and the other risks
> > spurious interrupts (the latter being obviously preferred).
> 
> The only case for which it might matter is physical edge-triggered IRQs
> (I have none on the platform I am testing this series on).
> If we clear GUEST_PENDING before setting GUEST_VISIBLE we slightly
> reduce the chance of loosing an interrupt. But keep in mind that the
> chance still exists: after we read GICC_IAR and before we clear
> GUEST_PENDING, if we receive another hardware interrupt it would be
> lost.

... and that is OK because? We consider them to have been coalesced I
guess?

> So I don't think that the relative order of GUEST_PENDING and
> GUEST_VISIBLE makes any meaningful difference.
> 
> 
> > > 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.
> > 
> > Is the race between checking the event bit mask and unmasking and/or
> > acking the interrupt closed somehow?
>  
> The window when the guest can loose evtchn notifications happens
> after existing the event handling loop and before EOIing the interrupt.
> We fixed it by noticing that evtchn_upcall_pending is set and trying an
> evtchn_irq re-injection right before returning to guest.
> 
> Of course we don't want to re-inject the evtchn_irq twice if it is
> already there so that's why I added the GIC_IRQ_GUEST_VISIBLE check in
> vgic_vcpu_inject_irq. The behavior in this regard should be exactly the
> same as before.
> 
> We don't want to rely on GIC_IRQ_GUEST_PENDING to inject a second
> evtchn_irq because if the guest is in the event handling loop it doesn't
> actually need a new notification.

OK, I think...

> > (We had an issue like this with PVHVM on x86 didn't we?)
> 
> It is different because in that case the source of notifications (the
> vector callback) doesn't need an EOI.

and should we ever figure out how to do that on ARM it will need a guest
visible flag to enable anyway. OK then.

> > > @@ -672,8 +673,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned 
> > > int irq, int virtual)
> > >  
> > >      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > >  
> > > -    /* vcpu offline or irq already pending */
> > > -    if (test_bit(_VPF_down, &v->pause_flags) || 
> > > !list_empty(&n->inflight))
> > > +    if ( !list_empty(&n->inflight) )
> > > +    {
> > > +        if ( (irq != current->domain->arch.evtchn_irq) ||
> > > +             (!test_bit(_GIC_IRQ_GUEST_VISIBLE, &n->status)) )
> > > +            set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
> > 
> > I worry about a race here. I think the consequence is a spurious
> > interrupt i.e. the safe option. Right? If yes then a comment to that
> > affect would be useful.
> 
> There is no race: it would only be possible for evtchn injections but
> those are completely taken care of by the in-guest evtchn loop and
> db453468d92369e7182663fb13e14d83ec4ce456.
> 
> Or maybe you mean something else that I am not seeing?

If something clears VISIBLE between the test and the set, do things work
correctly?

If VISIBLE is set right after the test, what happens?

Ian.


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