Re: [Xen-devel] [PATCH v4 03/10] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2014-03-21 at 15:55 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> > > If the irq to be injected is an hardware irq (p->desc != NULL), set
> > > GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ.
> > > 
> > > Remove the code to EOI a physical interrupt on behalf of the guest
> > > because it has become unnecessary.
> > 
> > Stupid question: there is no possibility of a h/w interrupt which for
> > one reason or another cannot be injected using these GIC facilities?
> 
> I don't think so. Nothing is written in spec about such a case.
> 
> 
> > > Introduce a new function, gic_clear_lrs, that goes over the GICH_LR
> > > registers, clear the invalid ones and free the corresponding interrupts
> > > from the inflight queue if appropriate. Add the interrupt to lr_pending
> > > if the GIC_IRQ_GUEST_PENDING is still set.
> > > 
> > > Call gic_clear_lrs from gic_restore_state and on return to guest
> > > (gic_inject).
> > > 
> > > In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu,
> > > send and SGI to it to interrupt it and force it to clear the old LRs.
> > 
> > s/and/an/
> > 
> > Is the SGI just there to cause it to flush its LRs? Does it not also
> > serve the purpose of causing the pcpu to pick up the potential new
> > interrupt?
> 
> Yes. Before this patch we were already sending an SGI to the other pcpu
> so that it would pick up the new IRQ.
> Now we also send an SGI to the other pcpu even if the IRQ is already
> inflight, so that the second pcpu can clear the LR corresponding to the
> previous injection as well as injecting the new interrupt.

Can you clarify that in the commit message please? (pretty much
inserting what you just said will do).

I assume that there is no chance we will send two SGIs?

> > Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which
> > suggests that the clear should be done there (which also seems logical).
> 
> It is just temporary: patch "call gic_clear_lrs on entry to the
> hypervisor" moves the call to gic_clear_lrs to traps.c.

I noticed that later, any reason for taking this roundabout path to the
final destination?

> > > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, 
> > > struct irqaction *new)
> > >  static inline void gic_set_lr(int lr, struct pending_irq *p,
> > >          unsigned int state)
> > >  {
> > > -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> > > +    uint32_t lr_reg;
> > >  
> > >      BUG_ON(lr >= nr_lrs);
> > >      BUG_ON(lr < 0);
> > >      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
> > >  
> > > -    GICH[GICH_LR + lr] = state |
> > > -        maintenance_int |
> > > -        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > > +    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > >          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> > > +    if ( p->desc != NULL )
> > > +        lr_reg |= GICH_LR_HW |
> > > +            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << 
> > > GICH_LR_PHYSICAL_SHIFT);
> > 
> > It seems like it would be a silicon design bug if this were ever true.
> > So BUG_ON(p->desc->irq & ~GICH_LR_PHYSICAL_MASK)?
>  
> Right, I'll remove it.
> 
> 
> > I think this should be checked at the time the interrupt is routed
> > instead of here, which gets it out of this hotter path.
> 
> Actually it cannot happen as desc->irq is initialized by init_irq_data
> in a for loop up to NR_IRQS (1024).

Excellent.

(although beware gic v3 ;-))


> > > +        {
> > > +            inflight = 0;
> > > +            GICH[GICH_LR + i] = 0;
> > > +            clear_bit(i, &this_cpu(lr_mask));
> > > +
> > > +            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > > +            spin_lock(&gic.lock);
> > > +            p = irq_to_pending(v, irq);
> > > +            if ( p->desc != NULL )
> > > +                p->desc->status &= ~IRQ_INPROGRESS;
> > > +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > > +            if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> > > +                    test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > > +            {
> > > +                inflight = 1;
> > > +                gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> > > +            }
> > > +            spin_unlock(&gic.lock);
> > 
> > Can an interrupt arrive at this point and make this interrupt "inflight"
> > again, such that the following list remove is actually wrong?
> 
> gic_clear_lrs is always called with irq disabled, see the ASSERT at the
> beginning of the function

Great.

> > 
> > Is now empty but not removed?
> 
> Yes. We want to keep receiving maintenance interrupts, but we don't need
> to do anything anymore in the handler because everything we need to do
> is done on return to guest.

I figured that out on the next patch -- a comment in any empty interrupt
handler would be very useful.

Ian.



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