diff mbox

[Xen-devel,v7,11/12] xen/arm: gic_events_need_delivery and irq priorities

Message ID 1396969969-18973-11-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini April 8, 2014, 3:12 p.m. UTC
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.
Introduce GIC_IRQ_GUEST_ACTIVE to track which one is the currently
active guest irq.
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 v7:
- fix locking for the list_empty case in gic_restore_pending_irqs;
- add in code comment;
- gic_events_need_delivery: break out of the loop as soon as we find the
active irq as inflight_irqs is ordered by priority;
- gic_events_need_delivery: break out of the loop if p->priority is
lower than mask_priority as inflight_irqs is ordered by priority;
- use find_next_zero_bit instead of find_first_zero_bit;
- in gic_restore_pending_irqs remember the last position of the inner
loop search and continue from there;
- in gic_restore_pending_irqs use a priority check to get out of the
inner loop.

Changes in v5:
- improve in code comments;
- use list_for_each_entry_reverse instead of writing my own list walker.

Changes in v4:
- in gic_events_need_delivery go through inflight_irqs and only consider
enabled irqs.
---
 xen/arch/arm/gic.c           |   84 ++++++++++++++++++++++++++++++++++++++----
 xen/include/asm-arm/domain.h |    5 ++-
 xen/include/asm-arm/gic.h    |    3 ++
 3 files changed, 82 insertions(+), 10 deletions(-)

Comments

Ian Campbell April 23, 2014, 1:31 p.m. UTC | #1
On Tue, 2014-04-08 at 16:12 +0100, 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.
> Introduce GIC_IRQ_GUEST_ACTIVE to track which one is the currently
> active guest irq.

There can be multiple such interrupt, can't there? In which case "which
ones are the currently active guest irqs" or "which IRQs are currently
active" or something.

> 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 v7:
> - fix locking for the list_empty case in gic_restore_pending_irqs;
> - add in code comment;
> - gic_events_need_delivery: break out of the loop as soon as we find the
> active irq as inflight_irqs is ordered by priority;
> - gic_events_need_delivery: break out of the loop if p->priority is
> lower than mask_priority as inflight_irqs is ordered by priority;
> - use find_next_zero_bit instead of find_first_zero_bit;
> - in gic_restore_pending_irqs remember the last position of the inner
> loop search and continue from there;
> - in gic_restore_pending_irqs use a priority check to get out of the
> inner loop.
> 
> Changes in v5:
> - improve in code comments;
> - use list_for_each_entry_reverse instead of writing my own list walker.
> 
> Changes in v4:
> - in gic_events_need_delivery go through inflight_irqs and only consider
> enabled irqs.
> ---
>  xen/arch/arm/gic.c           |   84 ++++++++++++++++++++++++++++++++++++++----
>  xen/include/asm-arm/domain.h |    5 ++-
>  xen/include/asm-arm/gic.h    |    3 ++
>  3 files changed, 82 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index e6e6f1a..9295ccf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -721,6 +721,7 @@ static void gic_update_one_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) &&
> @@ -735,6 +736,7 @@ static void gic_update_one_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 = GIC_INVALID_LR;
>          if ( test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
>                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> @@ -763,22 +765,53 @@ 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;
> +    struct list_head *inflight_r;
>      unsigned long flags;
>  
> +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
> +    if ( list_empty(&v->arch.vgic.lr_pending) )
> +        goto out;
> +
> +    inflight_r = &v->arch.vgic.inflight_irqs;
>      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;
> +        i = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, i);

While you are rewriting this then renaming the varialbe i to "lr" would
be a lot clearer.

> +        if ( i >= nr_lrs )
> +        {
> +            /* No more free LRs: find a lower priority irq to evict */
> +            list_for_each_entry_reverse( p_r, inflight_r, inflight )
> +            {
> +                inflight_r = &p_r->inflight;
> +                if ( p_r->priority == p->priority )
> +                    goto out;
> +                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
> +                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> +                    goto found;
> +            }

Please can you add a comment here:
		/* We didn't find a victim this time, and we won't next time, so quit */

> +            goto out;
> +
> +found:
> +            i = p_r->lr;
> +            p_r->lr = GIC_INVALID_LR;
> +            set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
> +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
> +            gic_add_to_lr_pending(v, p_r);
> +        }
>  
> -        spin_lock_irqsave(&v->arch.vgic.lock, flags);
>          gic_set_lr(i, p, GICH_LR_PENDING);
>          list_del_init(&p->lr_queue);
>          set_bit(i, &this_cpu(lr_mask));
> -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +
> +        lrs--;
> +        if ( lrs == 0 )
> +            break;
>      }
>  
> +out:
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>  }
>  
>  void gic_clear_pending_irqs(struct vcpu *v)
> @@ -794,8 +827,43 @@ void gic_clear_pending_irqs(struct vcpu *v)
>  
>  int gic_events_need_delivery(void)
>  {
> -    return (!list_empty(&current->arch.vgic.lr_pending) ||
> -            this_cpu(lr_mask));
> +    int mask_priority, lrs = nr_lrs;
> +    int max_priority = 0xff, active_priority = 0xff;
> +    struct vcpu *v = current;
> +    struct pending_irq *p;
> +    unsigned long flags;
> +
> +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;

mask_priority is a bit meaningless (I know the docs call it
priority_mask), but its the vcpus current priority, right?

Also, by adding a << 3 here then I think the rest of the logic reads
more easily, because you are then using the priority directly, and
ignoring the fact that VMCR has a limited precision. Whereas with the
shift >> 3 at the comparison sights you kind of have to think about it
in each case.

> +
> +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
> +    /* TODO: We order the guest irqs by priority, but we don't change
> +     * the priority of host irqs. */
> +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> +    {
> +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> +        {
> +            if ( p->priority < active_priority )
> +                active_priority = p->priority;
> +            break;
> +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> +            if ( p->priority < max_priority )
> +                max_priority = p->priority;
> +        }
> +        if ( (p->priority >> 3) >= mask_priority )
> +            break;

This lrs-- stuff needs a comment. I think along the lines of only the
first nr_lrs interrupt need to be considered because XXX.

Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
entries on it (say) of which the first 5 happen to be masked, don't we
want to keep going until we've looked at more than the first 4 entries
or something? Or should this decrement be conditional on ENABLE and/or
ACTIVE perhaps?

You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
haven't yet seen an active one then don't you know that max_priority <
active_priority? (this might be best discussed around a whiteboard...)


> +        lrs--;
> +        if ( lrs == 0 )
> +            break;
> +    }
> +
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +
> +    if ( max_priority < active_priority &&
> +         (max_priority >> 3) < mask_priority )
> +        return 1;
> +    else
> +        return 0;
>  }
>  
>  void gic_inject(void)

Ian
Stefano Stabellini May 8, 2014, 6:37 p.m. UTC | #2
On Wed, 23 Apr 2014, Ian Campbell wrote:
> On Tue, 2014-04-08 at 16:12 +0100, 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.
> > Introduce GIC_IRQ_GUEST_ACTIVE to track which one is the currently
> > active guest irq.
> 
> There can be multiple such interrupt, can't there? In which case "which
> ones are the currently active guest irqs" or "which IRQs are currently
> active" or something.
> 
> > 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 v7:
> > - fix locking for the list_empty case in gic_restore_pending_irqs;
> > - add in code comment;
> > - gic_events_need_delivery: break out of the loop as soon as we find the
> > active irq as inflight_irqs is ordered by priority;
> > - gic_events_need_delivery: break out of the loop if p->priority is
> > lower than mask_priority as inflight_irqs is ordered by priority;
> > - use find_next_zero_bit instead of find_first_zero_bit;
> > - in gic_restore_pending_irqs remember the last position of the inner
> > loop search and continue from there;
> > - in gic_restore_pending_irqs use a priority check to get out of the
> > inner loop.
> > 
> > Changes in v5:
> > - improve in code comments;
> > - use list_for_each_entry_reverse instead of writing my own list walker.
> > 
> > Changes in v4:
> > - in gic_events_need_delivery go through inflight_irqs and only consider
> > enabled irqs.
> > ---
> >  xen/arch/arm/gic.c           |   84 ++++++++++++++++++++++++++++++++++++++----
> >  xen/include/asm-arm/domain.h |    5 ++-
> >  xen/include/asm-arm/gic.h    |    3 ++
> >  3 files changed, 82 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index e6e6f1a..9295ccf 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -721,6 +721,7 @@ static void gic_update_one_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) &&
> > @@ -735,6 +736,7 @@ static void gic_update_one_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 = GIC_INVALID_LR;
> >          if ( test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
> >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > @@ -763,22 +765,53 @@ 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;
> > +    struct list_head *inflight_r;
> >      unsigned long flags;
> >  
> > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > +
> > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > +        goto out;
> > +
> > +    inflight_r = &v->arch.vgic.inflight_irqs;
> >      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;
> > +        i = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, i);
> 
> While you are rewriting this then renaming the varialbe i to "lr" would
> be a lot clearer.
> 
> > +        if ( i >= nr_lrs )
> > +        {
> > +            /* No more free LRs: find a lower priority irq to evict */
> > +            list_for_each_entry_reverse( p_r, inflight_r, inflight )
> > +            {
> > +                inflight_r = &p_r->inflight;
> > +                if ( p_r->priority == p->priority )
> > +                    goto out;
> > +                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
> > +                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> > +                    goto found;
> > +            }
> 
> Please can you add a comment here:
> 		/* We didn't find a victim this time, and we won't next time, so quit */
> 
> > +            goto out;
> > +
> > +found:
> > +            i = p_r->lr;
> > +            p_r->lr = GIC_INVALID_LR;
> > +            set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
> > +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
> > +            gic_add_to_lr_pending(v, p_r);
> > +        }
> >  
> > -        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >          gic_set_lr(i, p, GICH_LR_PENDING);
> >          list_del_init(&p->lr_queue);
> >          set_bit(i, &this_cpu(lr_mask));
> > -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +
> > +        lrs--;
> > +        if ( lrs == 0 )
> > +            break;
> >      }
> >  
> > +out:
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >  }
> >  
> >  void gic_clear_pending_irqs(struct vcpu *v)
> > @@ -794,8 +827,43 @@ void gic_clear_pending_irqs(struct vcpu *v)
> >  
> >  int gic_events_need_delivery(void)
> >  {
> > -    return (!list_empty(&current->arch.vgic.lr_pending) ||
> > -            this_cpu(lr_mask));
> > +    int mask_priority, lrs = nr_lrs;
> > +    int max_priority = 0xff, active_priority = 0xff;
> > +    struct vcpu *v = current;
> > +    struct pending_irq *p;
> > +    unsigned long flags;
> > +
> > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> 
> mask_priority is a bit meaningless (I know the docs call it
> priority_mask), but its the vcpus current priority, right?

It is the minimum priority that an interrupt needs to have for the cpu
to be interrupted by the gic.


> Also, by adding a << 3 here then I think the rest of the logic reads
> more easily, because you are then using the priority directly, and
> ignoring the fact that VMCR has a limited precision. Whereas with the
> shift >> 3 at the comparison sights you kind of have to think about it
> in each case.
>
> > +
> > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > +
> > +    /* TODO: We order the guest irqs by priority, but we don't change
> > +     * the priority of host irqs. */
> > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > +    {
> > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > +        {
> > +            if ( p->priority < active_priority )
> > +                active_priority = p->priority;
> > +            break;
> > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > +            if ( p->priority < max_priority )
> > +                max_priority = p->priority;
> > +        }
> > +        if ( (p->priority >> 3) >= mask_priority )
> > +            break;
> 
> This lrs-- stuff needs a comment. I think along the lines of only the
> first nr_lrs interrupt need to be considered because XXX.
> 
> Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
> entries on it (say) of which the first 5 happen to be masked, don't we
> want to keep going until we've looked at more than the first 4 entries
> or something? Or should this decrement be conditional on ENABLE and/or
> ACTIVE perhaps?

If we have 4 lrs and inflight_irqs has 10 entries on it (say) of which
the first 5 happen to be masked, we want to keep going until we've
looked at more than the first 4 entries but we certainly cannot swap
more than 4 entries.


> You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> haven't yet seen an active one then don't you know that max_priority <
> active_priority? (this might be best discussed around a whiteboard...)
>
> > +        lrs--;
> > +        if ( lrs == 0 )
> > +            break;
> > +    }
> > +
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +
> > +    if ( max_priority < active_priority &&
> > +         (max_priority >> 3) < mask_priority )
> > +        return 1;
> > +    else
> > +        return 0;
> >  }
> >  
> >  void gic_inject(void)
> 
> Ian
>
Ian Campbell May 9, 2014, 8:37 a.m. UTC | #3
On Thu, 2014-05-08 at 19:37 +0100, Stefano Stabellini wrote:
> On Wed, 23 Apr 2014, Ian Campbell wrote:
> > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> > 
> > mask_priority is a bit meaningless (I know the docs call it
> > priority_mask), but its the vcpus current priority, right?
> 
> It is the minimum priority that an interrupt needs to have for the cpu
> to be interrupted by the gic.

OK, I think you are stating the same thing as me but from a different
angle.
> 
> 
> > Also, by adding a << 3 here then I think the rest of the logic reads
> > more easily, because you are then using the priority directly, and
> > ignoring the fact that VMCR has a limited precision. Whereas with the
> > shift >> 3 at the comparison sights you kind of have to think about it
> > in each case.
> >
> > > +
> > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > +
> > > +    /* TODO: We order the guest irqs by priority, but we don't change
> > > +     * the priority of host irqs. */
> > > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > > +    {
> > > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > +        {
> > > +            if ( p->priority < active_priority )
> > > +                active_priority = p->priority;
> > > +            break;
> > > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > > +            if ( p->priority < max_priority )
> > > +                max_priority = p->priority;
> > > +        }
> > > +        if ( (p->priority >> 3) >= mask_priority )
> > > +            break;
> > 
> > This lrs-- stuff needs a comment. I think along the lines of only the
> > first nr_lrs interrupt need to be considered because XXX.
> > 
> > Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
> > entries on it (say) of which the first 5 happen to be masked, don't we
> > want to keep going until we've looked at more than the first 4 entries
> > or something? Or should this decrement be conditional on ENABLE and/or
> > ACTIVE perhaps?
> 
> If we have 4 lrs and inflight_irqs has 10 entries on it (say) of which
> the first 5 happen to be masked, we want to keep going until we've
> looked at more than the first 4 entries but we certainly cannot swap
> more than 4 entries.

I think what is confusing me is that I don't see where the lrs-- is
skipped for a masked interrupt. So aren't you counting down the lrs
variable even for the first 5 which happen to be masked?

> > You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> > exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> > haven't yet seen an active one then don't you know that max_priority <
> > active_priority? (this might be best discussed around a whiteboard...)
> >
> > > +        lrs--;
> > > +        if ( lrs == 0 )
> > > +            break;
> > > +    }
> > > +
> > > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > > +
> > > +    if ( max_priority < active_priority &&
> > > +         (max_priority >> 3) < mask_priority )
> > > +        return 1;
> > > +    else
> > > +        return 0;
> > >  }
> > >  
> > >  void gic_inject(void)
> > 
> > Ian
> >
Stefano Stabellini May 11, 2014, 2:13 p.m. UTC | #4
> > On Wed, 23 Apr 2014, Ian Campbell wrote:
> > > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > > > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> > > 
> > > mask_priority is a bit meaningless (I know the docs call it
> > > priority_mask), but its the vcpus current priority, right?
> > 
> > It is the minimum priority that an interrupt needs to have for the cpu
> > to be interrupted by the gic.
> 
> OK, I think you are stating the same thing as me but from a different
> angle.
> > 
> > 
> > > Also, by adding a << 3 here then I think the rest of the logic reads
> > > more easily, because you are then using the priority directly, and
> > > ignoring the fact that VMCR has a limited precision. Whereas with the
> > > shift >> 3 at the comparison sights you kind of have to think about it
> > > in each case.
> > >
> > > > +
> > > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > > +
> > > > +    /* TODO: We order the guest irqs by priority, but we don't change
> > > > +     * the priority of host irqs. */
> > > > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > > > +    {
> > > > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > > +        {
> > > > +            if ( p->priority < active_priority )
> > > > +                active_priority = p->priority;
> > > > +            break;
> > > > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > > > +            if ( p->priority < max_priority )
> > > > +                max_priority = p->priority;
> > > > +        }
> > > > +        if ( (p->priority >> 3) >= mask_priority )
> > > > +            break;
> > > 
> > > This lrs-- stuff needs a comment. I think along the lines of only the
> > > first nr_lrs interrupt need to be considered because XXX.
> > > 
> > > Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
> > > entries on it (say) of which the first 5 happen to be masked, don't we
> > > want to keep going until we've looked at more than the first 4 entries
> > > or something? Or should this decrement be conditional on ENABLE and/or
> > > ACTIVE perhaps?
> > 
> > If we have 4 lrs and inflight_irqs has 10 entries on it (say) of which
> > the first 5 happen to be masked, we want to keep going until we've
> > looked at more than the first 4 entries but we certainly cannot swap
> > more than 4 entries.
> 
> I think what is confusing me is that I don't see where the lrs-- is
> skipped for a masked interrupt. So aren't you counting down the lrs
> variable even for the first 5 which happen to be masked?

At the moment when the guest disables an irq, we remove it from the
lr_pending queue but we don't remove it from the inflight queue. So if
the irq has already been added to an LR register, the guest is going to
receive a notification still.

This patch doesn't change this behaviour: the eviction code in
gic_restore_pending_irqs doesn't distinguish between masked and unmasked
irqs, it treats them the same way, simply going by priority.
Consistently in gic_events_need_delivery, we only analyze the first
nr_lrs irqs by priority, regardless if they are masked or unmasked.

To answer your original question: no, we don't need to keep going past
the first 4 irqs in inflight_irqs, even if they are all masked.

Admittedly this behaviour could be improved, but it might be best to fix
it in a consequent patch series.


> > > You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> > > exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> > > haven't yet seen an active one then don't you know that max_priority <
> > > active_priority? (this might be best discussed around a whiteboard...)
> > >
> > > > +        lrs--;
> > > > +        if ( lrs == 0 )
> > > > +            break;
> > > > +    }
> > > > +
> > > > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > > > +
> > > > +    if ( max_priority < active_priority &&
> > > > +         (max_priority >> 3) < mask_priority )
> > > > +        return 1;
> > > > +    else
> > > > +        return 0;
> > > >  }
> > > >  
> > > >  void gic_inject(void)
> > > 
> > > Ian
> > > 
> 
>
Stefano Stabellini May 11, 2014, 4:50 p.m. UTC | #5
On Wed, 23 Apr 2014, Ian Campbell wrote:
> On Tue, 2014-04-08 at 16:12 +0100, 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.
> > Introduce GIC_IRQ_GUEST_ACTIVE to track which one is the currently
> > active guest irq.
> 
> There can be multiple such interrupt, can't there? In which case "which
> ones are the currently active guest irqs" or "which IRQs are currently
> active" or something.
> 
> > 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 v7:
> > - fix locking for the list_empty case in gic_restore_pending_irqs;
> > - add in code comment;
> > - gic_events_need_delivery: break out of the loop as soon as we find the
> > active irq as inflight_irqs is ordered by priority;
> > - gic_events_need_delivery: break out of the loop if p->priority is
> > lower than mask_priority as inflight_irqs is ordered by priority;
> > - use find_next_zero_bit instead of find_first_zero_bit;
> > - in gic_restore_pending_irqs remember the last position of the inner
> > loop search and continue from there;
> > - in gic_restore_pending_irqs use a priority check to get out of the
> > inner loop.
> > 
> > Changes in v5:
> > - improve in code comments;
> > - use list_for_each_entry_reverse instead of writing my own list walker.
> > 
> > Changes in v4:
> > - in gic_events_need_delivery go through inflight_irqs and only consider
> > enabled irqs.
> > ---
> >  xen/arch/arm/gic.c           |   84 ++++++++++++++++++++++++++++++++++++++----
> >  xen/include/asm-arm/domain.h |    5 ++-
> >  xen/include/asm-arm/gic.h    |    3 ++
> >  3 files changed, 82 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index e6e6f1a..9295ccf 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -721,6 +721,7 @@ static void gic_update_one_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) &&
> > @@ -735,6 +736,7 @@ static void gic_update_one_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 = GIC_INVALID_LR;
> >          if ( test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
> >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > @@ -763,22 +765,53 @@ 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;
> > +    struct list_head *inflight_r;
> >      unsigned long flags;
> >  
> > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > +
> > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > +        goto out;
> > +
> > +    inflight_r = &v->arch.vgic.inflight_irqs;
> >      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;
> > +        i = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, i);
> 
> While you are rewriting this then renaming the varialbe i to "lr" would
> be a lot clearer.
> 
> > +        if ( i >= nr_lrs )
> > +        {
> > +            /* No more free LRs: find a lower priority irq to evict */
> > +            list_for_each_entry_reverse( p_r, inflight_r, inflight )
> > +            {
> > +                inflight_r = &p_r->inflight;
> > +                if ( p_r->priority == p->priority )
> > +                    goto out;
> > +                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
> > +                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> > +                    goto found;
> > +            }
> 
> Please can you add a comment here:
> 		/* We didn't find a victim this time, and we won't next time, so quit */
> 
> > +            goto out;
> > +
> > +found:
> > +            i = p_r->lr;
> > +            p_r->lr = GIC_INVALID_LR;
> > +            set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
> > +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
> > +            gic_add_to_lr_pending(v, p_r);
> > +        }
> >  
> > -        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >          gic_set_lr(i, p, GICH_LR_PENDING);
> >          list_del_init(&p->lr_queue);
> >          set_bit(i, &this_cpu(lr_mask));
> > -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +
> > +        lrs--;
> > +        if ( lrs == 0 )
> > +            break;
> >      }
> >  
> > +out:
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >  }
> >  
> >  void gic_clear_pending_irqs(struct vcpu *v)
> > @@ -794,8 +827,43 @@ void gic_clear_pending_irqs(struct vcpu *v)
> >  
> >  int gic_events_need_delivery(void)
> >  {
> > -    return (!list_empty(&current->arch.vgic.lr_pending) ||
> > -            this_cpu(lr_mask));
> > +    int mask_priority, lrs = nr_lrs;
> > +    int max_priority = 0xff, active_priority = 0xff;
> > +    struct vcpu *v = current;
> > +    struct pending_irq *p;
> > +    unsigned long flags;
> > +
> > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> 
> mask_priority is a bit meaningless (I know the docs call it
> priority_mask), but its the vcpus current priority, right?
> 
> Also, by adding a << 3 here then I think the rest of the logic reads
> more easily, because you are then using the priority directly, and
> ignoring the fact that VMCR has a limited precision. Whereas with the
> shift >> 3 at the comparison sights you kind of have to think about it
> in each case.
> 
> > +
> > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > +
> > +    /* TODO: We order the guest irqs by priority, but we don't change
> > +     * the priority of host irqs. */
> > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > +    {
> > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > +        {
> > +            if ( p->priority < active_priority )
> > +                active_priority = p->priority;
> > +            break;
> > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > +            if ( p->priority < max_priority )
> > +                max_priority = p->priority;
> > +        }
> > +        if ( (p->priority >> 3) >= mask_priority )
> > +            break;
> 
> You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> haven't yet seen an active one then don't you know that max_priority <
> active_priority? (this might be best discussed around a whiteboard...)

That would not be correct: if the current irq (p) has the same priority
as the active irq but it has been evaluated first, then if we break
immediately we would be led to think that there is an irq that needs to
be injected but actually there isn't one.
diff mbox

Patch

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e6e6f1a..9295ccf 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -721,6 +721,7 @@  static void gic_update_one_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) &&
@@ -735,6 +736,7 @@  static void gic_update_one_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 = GIC_INVALID_LR;
         if ( test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
@@ -763,22 +765,53 @@  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;
+    struct list_head *inflight_r;
     unsigned long flags;
 
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    if ( list_empty(&v->arch.vgic.lr_pending) )
+        goto out;
+
+    inflight_r = &v->arch.vgic.inflight_irqs;
     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;
+        i = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, i);
+        if ( i >= nr_lrs )
+        {
+            /* No more free LRs: find a lower priority irq to evict */
+            list_for_each_entry_reverse( p_r, inflight_r, inflight )
+            {
+                inflight_r = &p_r->inflight;
+                if ( p_r->priority == p->priority )
+                    goto out;
+                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
+                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
+                    goto found;
+            }
+            goto out;
+
+found:
+            i = p_r->lr;
+            p_r->lr = GIC_INVALID_LR;
+            set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
+            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
+            gic_add_to_lr_pending(v, p_r);
+        }
 
-        spin_lock_irqsave(&v->arch.vgic.lock, flags);
         gic_set_lr(i, p, GICH_LR_PENDING);
         list_del_init(&p->lr_queue);
         set_bit(i, &this_cpu(lr_mask));
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+
+        lrs--;
+        if ( lrs == 0 )
+            break;
     }
 
+out:
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
 void gic_clear_pending_irqs(struct vcpu *v)
@@ -794,8 +827,43 @@  void gic_clear_pending_irqs(struct vcpu *v)
 
 int gic_events_need_delivery(void)
 {
-    return (!list_empty(&current->arch.vgic.lr_pending) ||
-            this_cpu(lr_mask));
+    int mask_priority, lrs = nr_lrs;
+    int max_priority = 0xff, active_priority = 0xff;
+    struct vcpu *v = current;
+    struct pending_irq *p;
+    unsigned long flags;
+
+    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    /* TODO: We order the guest irqs by priority, but we don't change
+     * the priority of host irqs. */
+    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
+    {
+        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
+        {
+            if ( p->priority < active_priority )
+                active_priority = p->priority;
+            break;
+        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
+            if ( p->priority < max_priority )
+                max_priority = p->priority;
+        }
+        if ( (p->priority >> 3) >= mask_priority )
+            break;
+        lrs--;
+        if ( lrs == 0 )
+            break;
+    }
+
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+
+    if ( max_priority < active_priority &&
+         (max_priority >> 3) < mask_priority )
+        return 1;
+    else
+        return 0;
 }
 
 void gic_inject(void)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 71f563f..75cc2f3 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -56,8 +56,9 @@  struct pending_irq
      *
      */
 #define GIC_IRQ_GUEST_QUEUED   0
-#define GIC_IRQ_GUEST_VISIBLE  1
-#define GIC_IRQ_GUEST_ENABLED  2
+#define GIC_IRQ_GUEST_ACTIVE   1
+#define GIC_IRQ_GUEST_VISIBLE  2
+#define GIC_IRQ_GUEST_ENABLED  3
     unsigned long status;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
     int irq;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 5a9dc77..5d8f7f1 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -129,6 +129,9 @@ 
 #define GICH_LR_CPUID_SHIFT     9
 #define GICH_VTR_NRLRGS         0x3f
 
+#define GICH_VMCR_PRIORITY_MASK   0x1f
+#define GICH_VMCR_PRIORITY_SHIFT  27
+
 /*
  * The minimum GICC_BPR is required to be in the range 0-3. We set
  * GICC_BPR to 0 but we must expect that it might be 3. This means we