From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Stabellini Subject: Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities Date: Mon, 24 Mar 2014 16:06:14 +0000 Message-ID: References: <1395232325-19226-10-git-send-email-stefano.stabellini@eu.citrix.com> <1395409349.19839.76.camel@kazak.uk.xensource.com> <1395662736.6294.8.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: julien.grall@citrix.com, jtd@galois.com, xen-devel@lists.xensource.com, Ian Campbell List-Id: xen-devel@lists.xenproject.org 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 > > > > > > > > > > --- > > > > > 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.