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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > 
> > ---
> > 
> > Changes in v4:
> > - merged patch #3 and #4 into a single patch.
> > 
> > Changes in v2:
> > - remove the EOI code, now unnecessary;
> > - do not assume physical IRQ == virtual IRQ;
> > - refactor gic_set_lr.
> > ---
> >  xen/arch/arm/gic.c  |  135 
> > ++++++++++++++++++++++-----------------------------
> >  xen/arch/arm/vgic.c |    3 +-
> >  2 files changed, 60 insertions(+), 78 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index dbba5d3..32d3bea 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -128,6 +130,7 @@ void gic_restore_state(struct vcpu *v)
> >      GICH[GICH_HCR] = GICH_HCR_EN;
> >      isb();
> >  
> > +    gic_clear_lrs(v);
> >      gic_restore_pending_irqs(v);
> 
> Not related to this patch exactly, but is this function badly named? It
> seems to not actually be restoring anything but is actually looking for
> any newly pending irqs which it can now inject, is that right?

Yeah, that is correct.


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


> >  }
> >  
> > @@ -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).


> Another future cleanup: I think a lot of this register frobbing code
> would be cleaner if GICH_LR_FOO_MASK was already shifted.
> 
> [...]
> > +static void gic_clear_lrs(struct vcpu *v)
> > +{
> > +    struct pending_irq *p;
> > +    int i = 0, irq;
> > +    uint32_t lr;
> > +    bool_t inflight;
> > +
> > +    ASSERT(!local_irq_is_enabled());
> > +
> > +    while ((i = find_next_bit((const long unsigned int *) 
> > &this_cpu(lr_mask),
> 
> "long unsigned int" is an odd one isn't it?
> 
> It seems like all of the other places which do bitops on lr_mask manage
> to avoid any case at all.

_find_next_bit_le requires a const unsigned long *p as first argument.
I'll make the change to const unsigned long.


> > +                              nr_lrs, i)) < nr_lrs) {
> > +        lr = GICH[GICH_LR + i];
> > +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> 
> This state means the lr is "completed" (or inactive, however you want to
> think about it)? A little helper macro lr_completed(lr) would clarify
> this without needing a comment.

This is another one of those pieces of code that are going to disappear
in the following patches. I think the final version is going to look
nicer, even without macros.


> Isn't that condition also true for an LR which was never injected? Won't
> we then try and complete a non-existent interrupt? Ah, this is protected
> by the lr_mask isn't it.

We are only iterating over lr_mask, that means that we have written to
this lr in the recent past.


> > +        {
> > +            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


> > +            {
> > +                spin_lock(&v->arch.vgic.lock);
> > +                list_del_init(&p->inflight);
> > +                spin_unlock(&v->arch.vgic.lock);
> > +            }
> > +
> > +        }
> > +
> > +        i++;
> > +    }
> > +}
> > +
> >  static void gic_restore_pending_irqs(struct vcpu *v)
> >  {
> >      int i;
> >  static void maintenance_interrupt(int irq, void *dev_id, struct 
> > cpu_user_regs *regs)
> >  {
> [...]
> 
> 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.

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