| Message ID | 1395232325-19226-10-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 03/19/2014 12:32 PM, 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> I'd like a comment somewhere in the code to explain the priority is only for the guest and doesn't reflect the priority set on the host. So people won't think we are handling correctly priority at the host level. Regards,
On Wed, 2014-03-19 at 14:00 +0000, Julien Grall wrote:
> So people won't think we are handling correctly priority at the host level.
I think we mostly handle host level interrupt priorities OK, don't we?
Ian.
On 03/20/2014 11:03 AM, Ian Campbell wrote: > On Wed, 2014-03-19 at 14:00 +0000, Julien Grall wrote: >> So people won't think we are handling correctly priority at the host level. > > I think we mostly handle host level interrupt priorities OK, don't we? We handle host level interrupt priorities but we don't propagate the guest priorities. Regards,
On Thu, 2014-03-20 at 15:10 +0000, Julien Grall wrote: > On 03/20/2014 11:03 AM, Ian Campbell wrote: > > On Wed, 2014-03-19 at 14:00 +0000, Julien Grall wrote: > >> So people won't think we are handling correctly priority at the host level. > > > > I think we mostly handle host level interrupt priorities OK, don't we? > > We handle host level interrupt priorities but we don't propagate the > guest priorities. Right, that is indeed a missing piece of the puzzle. Ian.
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? > + if ( &p_r->inflight == p->inflight.next ) > + goto out; > + } > + i = p_r->lr; > + p_r->lr = nr_lrs; > + set_bit(GIC_IRQ_GUEST_PENDING, &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)
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. 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. > > + if ( &p_r->inflight == p->inflight.next ) > > + goto out; > > + } > > + i = p_r->lr; > > + p_r->lr = nr_lrs; > > + set_bit(GIC_IRQ_GUEST_PENDING, &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) > >
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. > 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. Ian.
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.
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@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. 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.
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); 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); + if ( &p_r->inflight == p->inflight.next ) + goto out; + } + i = p_r->lr; + p_r->lr = nr_lrs; + set_bit(GIC_IRQ_GUEST_PENDING, &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) @@ -779,8 +806,38 @@ void gic_clear_pending_irqs(struct vcpu *v) int gic_events_need_delivery(void) { - return (!list_empty(¤t->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); + + 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; + } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) { + if ( p->priority < max_priority ) + max_priority = p->priority; + } + 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 517128e..fe20fd2 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_PENDING 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; uint8_t lr; struct irq_desc *desc; /* only set it the irq corresponds to a physical 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
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(-)