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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 24 Mar 2014, Stefano Stabellini wrote:
> 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@xxxxxxxxxxxxx>
> > > > > 
> > > > > ---
> > > > > 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.

Actually there is no need: like you wrote inflight is a small list and
the number of pending irqs that don't fit in any LRs is going to be even
smaller. We can just iterate in reverse from the end of the list every
time.

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