Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Mon, 24 Mar 2014, Ian Campbell wrote:
> On Mon, 2014-03-24 at 12:00 +0000, Stefano Stabellini wrote:
> > On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > > gic_events_need_delivery should only return positive if an outstanding
> > > > pending irq has an higher priority than the currently active irq and the
> > > > priority mask.
> > > > Rewrite the function by going through the priority ordered inflight and
> > > > lr_queue lists.
> > > > 
> > > > In gic_restore_pending_irqs replace lower priority pending (and not
> > > > active) irqs in GICH_LRs with higher priority irqs if no more GICH_LRs
> > > > are available.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > 
> > > > ---
> > > > Changes in v4:
> > > > - in gic_events_need_delivery go through inflight_irqs and only consider
> > > > enabled irqs.
> > > > ---
> > > >  xen/arch/arm/gic.c           |   71 +++++++++++++++++++++++++++++++++++++-----
> > > >  xen/include/asm-arm/domain.h |    5 +--
> > > >  xen/include/asm-arm/gic.h    |    3 ++
> > > >  3 files changed, 70 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > > index bc9d66d..533964e 100644
> > > > --- a/xen/arch/arm/gic.c
> > > > +++ b/xen/arch/arm/gic.c
> > > > @@ -709,6 +709,7 @@ static void _gic_clear_lr(struct vcpu *v, int i)
> > > >      p = irq_to_pending(v, irq);
> > > >      if ( lr & GICH_LR_ACTIVE )
> > > >      {
> > > > +        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> > > >          /* HW interrupts cannot be ACTIVE and PENDING */
> > > >          if ( p->desc == NULL &&
> > > >               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > > > @@ -723,6 +724,7 @@ static void _gic_clear_lr(struct vcpu *v, int i)
> > > >          if ( p->desc != NULL )
> > > >              p->desc->status &= ~IRQ_INPROGRESS;
> > > >          clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > > > +        clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> > > >          p->lr = nr_lrs;
> > > >          if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> > > >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > > > @@ -750,22 +752,47 @@ void gic_clear_lrs(struct vcpu *v)
> > > >  
> > > >  static void gic_restore_pending_irqs(struct vcpu *v)
> > > >  {
> > > > -    int i;
> > > > -    struct pending_irq *p, *t;
> > > > +    int i = 0, lrs = nr_lrs;
> > > > +    struct pending_irq *p, *t, *p_r;
> > > >      unsigned long flags;
> > > >  
> > > > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > > > +        return;
> > > > +
> > > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > > +
> > > > +    p_r = list_entry(v->arch.vgic.inflight_irqs.prev,
> > > > +                         typeof(*p_r), inflight);
> > > 
> > > Is this getting the tail of the list or something else?
> > > 
> > > >      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> > > >      {
> > > >          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
> > > > -        if ( i >= nr_lrs ) return;
> > > > +        if ( i >= nr_lrs )
> > > > +        {
> > > > +            while ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) ||
> > > > +                    test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> > > > +            {
> > > > +                p_r = list_entry(p_r->inflight.prev, typeof(*p_r), inflight);
> > > 
> > > Oh, maybe this (and the thing above) is an open coded list_for_each_prev
> > > or one of its variants? e.g. list_for_each_entry_reverse?
> > 
> > Yes, it is a list_for_each_entry_reverse that starts from the current
> > p_r and stops when it finds the first entry that is
> > GIC_IRQ_GUEST_VISIBLE and GIC_IRQ_GUEST_ACTIVE.
> 
> I think this can/should be recast in terms of the regular macro then.

OK.
We'll need to introduce list_for_each_entry_continue_reverse.

> > It can also stop if the next entry that would be found going through
> > inflight in reverse is the same that we are evaluating going through
> > lr_pending in regular order.
> 
> I think it should also be possible to incorporate this behaviour.
> 
> But is it worth going backwards? I'd have though that for a long list of
> inflight interrupts the normal case would be a smallish number of
> VISIBLE+ACTIVE interrupts at the head and a potentially larger tail up
> to nr_lrs. So you would be better off searching in normal order.

I think it is worth going backward to get the lowest priority interrupt
currently being injected. Inflight is also ordered by priority.