| Message ID | 1396969969-18973-11-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
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(¤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; 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
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(¤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; > > 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 >
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 > >
> > 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 > > > > >
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(¤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; > > 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 --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(¤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); + + /* 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
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(-)