Re: [Xen-devel] [PATCH v3 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: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?

Yes. Also I can't see another way around this.


> > 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?

Yes, because the test at the beginning of gic_inject will cover that
case.


> If VISIBLE is set right after the test, what happens?
 
It is OK because it means that the guest is getting another interrupt
notification anyway. The guest OS would lose the second interrupt even
on native: until the guest ACKs the interrupt is not supposed to be able
to receive another interrupt.
We are actually allowing the guest to receive more interrupt
notifications than it would be capable if it was running bare metal
by clearing PENDING before the guest does it.

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