Re: [Xen-devel] [PATCH v6 08/10] xen/arm: second irq injection while the first irq is still inflight

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 3 Apr 2014, Ian Campbell wrote:
> On Wed, 2014-04-02 at 16:01 +0100, Stefano Stabellini wrote:
> (picking up where I left on v5 when I realised there had been a v6)
> 
> 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 40e768e..218e3c6 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -680,6 +680,14 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int 
> > virtual_irq,
> >  {
> >      int i;
> >      unsigned long flags;
> > +    struct pending_irq *n = irq_to_pending(v, virtual_irq);
> > +
> > +    if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status))
> > +    {
> > +        if ( v == current )
> > +            gic_clear_one_lr(v, n->lr);
> 
> Maybe this is the same confusion I had with v5 in a different hat, but
> why?
> 
> If the IRQ is visible to the guest (present in an LR, either active or
> pending) and a new one comes along why would we clear the LR?

Although gic_clear_one_lr is mostly about clearing an LR once the guest
irq handling is completed (hence the name), it is smart enough to avoid
clearing the LR if the irq is still GICH_LR_ACTIVE. It is also able to
to inject a second irq while the first is already GICH_LR_ACTIVE (by
setting both GICH_LR_ACTIVE and GICH_LR_PENDING).

By calling gic_clear_one_lr here, we make sure that:

- we clear the LR of the old guest irq if it has been already EOIed
- we inject the new irq in a new LR if old guest irq has already been
  EOIed and cleared
- we inject the new irq while the old one is still GICH_LR_ACTIVE by
  setting both GICH_LR_ACTIVE and GICH_LR_PENDING
- we clear GIC_IRQ_GUEST_PENDING (no further injections) if the old irq
  is still GICH_LR_PENDING

In summary we do everything we can to handle the guest irq as soon as
possible.


> Is it so that we can reinsert it again right after? If so please could
> you add a comment saying that.
> 
> By doing thing this way don't we lose the active bit in the LR if it was
> set?
> 
> > +        list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
> > +    } else if ( n->priority != priority )
> > +        gdprintk(XENLOG_WARNING, "Changing priority of an inflight 
> > interrupt is not supported");
> 
> Would it be OK to silently ignore this? Changing the priority of an
> interrupt which isn't masked is going to be racy even on real hardware,
> i.e. you might receive one more at the old priority. I haven't checked
> what the GIC docs say about this -- it could well be unpredicatable or
> something...

I agree, I'll remove the gdprintk.

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