| Message ID | 1402409240-28114-12-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 2014-06-10 at 15:07 +0100, Stefano Stabellini wrote: > Introduce GIC_IRQ_GUEST_ACTIVE to track which irqs are currently > active in the guest. > > gic_events_need_delivery should only return positive if an outstanding > pending irq has an higher group priority than the currently active group > priotity and the priority mask. > Read GICH_APR to find the active group priority. > Read GICH_VMCR to find the priority mask. > Find the highest priority non-active enabled irq by going through the > inflight list. > > 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 v9: > - gic_events_need_delivery: read GICH_APR to find the active group > priority; > - gic_events_need_delivery: simplify the search loop. Thanks, I can actually grok this version ;-) Acked this + #9. AIUI there is no relationship here with "xen/arm: make accesses to desc->status flags atomic" which fixes an issue which already present before this series. There applied (this series, not the atomic flags one, yet). thanks. Ian.
On Tue, 2014-06-10 at 15:07 +0100, Stefano Stabellini wrote: > Introduce GIC_IRQ_GUEST_ACTIVE to track which irqs are currently > active in the guest. > > gic_events_need_delivery should only return positive if an outstanding > pending irq has an higher group priority than the currently active group > priotity and the priority mask. > Read GICH_APR to find the active group priority. > Read GICH_VMCR to find the priority mask. > Find the highest priority non-active enabled irq by going through the > inflight list. > > 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 v9: > - gic_events_need_delivery: read GICH_APR to find the active group > priority; > - gic_events_need_delivery: simplify the search loop. Thanks, I can actually grok this version ;-) Acked this + #9. AIUI there is no relationship here with "xen/arm: make accesses to desc->status flags atomic" which fixes an issue which already present before this series. There applied (this series, not the atomic flags one, yet). thanks. Ian.
On 06/18/2014 11:36 AM, Ian Campbell wrote: > AIUI there is no relationship here with "xen/arm: make accesses to > desc->status flags atomic" which fixes an issue which already present > before this series. The error was not really there before. We were "safe" because the flags were cleared before EOIing the IRQ. Here, we are clearing after EOIing (it has been made by the guest). So this race condition can happen more often. Regards,
On Wed, 18 Jun 2014, Julien Grall wrote: > On 06/18/2014 11:36 AM, Ian Campbell wrote: > > AIUI there is no relationship here with "xen/arm: make accesses to > > desc->status flags atomic" which fixes an issue which already present > > before this series. > > The error was not really there before. We were "safe" because the flags > were cleared before EOIing the IRQ. > > Here, we are clearing after EOIing (it has been made by the guest). So > this race condition can happen more often. However considering that when IRQ_GUEST is set, IRQ_INPROGRESS is only read by irq.c, I think the window for errors is extremely small. That doesn't mean we should expose ourselves to risks, so I still recommend we apply "make accesses to desc->status flags atomic".
On 06/18/2014 01:51 PM, Stefano Stabellini wrote: > On Wed, 18 Jun 2014, Julien Grall wrote: >> On 06/18/2014 11:36 AM, Ian Campbell wrote: >>> AIUI there is no relationship here with "xen/arm: make accesses to >>> desc->status flags atomic" which fixes an issue which already present >>> before this series. >> >> The error was not really there before. We were "safe" because the flags >> were cleared before EOIing the IRQ. >> >> Here, we are clearing after EOIing (it has been made by the guest). So >> this race condition can happen more often. > > However considering that when IRQ_GUEST is set, IRQ_INPROGRESS is only > read by irq.c, I think the window for errors is extremely small. > > That doesn't mean we should expose ourselves to risks, so I still > recommend we apply "make accesses to desc->status flags atomic". FYI, with my upcoming device passthrough series, I rely of IRQ_INPROGRESS in release_irq to know if we need to EOI an IRQ routed to the guest. So this series is definitely necessary, otherwise we loose the interrupt for ever. Regards,
On Wed, 2014-06-18 at 13:51 +0100, Stefano Stabellini wrote: > On Wed, 18 Jun 2014, Julien Grall wrote: > > On 06/18/2014 11:36 AM, Ian Campbell wrote: > > > AIUI there is no relationship here with "xen/arm: make accesses to > > > desc->status flags atomic" which fixes an issue which already present > > > before this series. > > > > The error was not really there before. We were "safe" because the flags > > were cleared before EOIing the IRQ. > > > > Here, we are clearing after EOIing (it has been made by the guest). So > > this race condition can happen more often. > > However considering that when IRQ_GUEST is set, IRQ_INPROGRESS is only > read by irq.c, I think the window for errors is extremely small. > > That doesn't mean we should expose ourselves to risks, so I still > recommend we apply "make accesses to desc->status flags atomic". Agreed, I'll see how sufficiently acked it is when I get that far through my queue. Ian.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 3728182..5e502df 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -650,6 +650,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); if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) && test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) ) { @@ -673,6 +674,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_ENABLED, &p->status) && test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) ) @@ -706,20 +708,55 @@ void gic_clear_lrs(struct vcpu *v) static void gic_restore_pending_irqs(struct vcpu *v) { - int i; - struct pending_irq *p, *t; + int lr = 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; + lr = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, lr); + if ( lr >= nr_lrs ) + { + /* No more free LRs: find a lower priority irq to evict */ + list_for_each_entry_reverse( p_r, inflight_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; + } + /* We didn't find a victim this time, and we won't next + * time, so quit */ + goto out; + +found: + lr = 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); + inflight_r = &p_r->inflight; + } - gic_set_lr(i, p, GICH_LR_PENDING); + gic_set_lr(lr, p, GICH_LR_PENDING); list_del_init(&p->lr_queue); - set_bit(i, &this_cpu(lr_mask)); + set_bit(lr, &this_cpu(lr_mask)); + + /* We can only evict nr_lrs entries */ + lrs--; + if ( lrs == 0 ) + break; } + +out: spin_unlock_irqrestore(&v->arch.vgic.lock, flags); } @@ -736,8 +773,40 @@ 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)); + struct vcpu *v = current; + struct pending_irq *p; + unsigned long flags; + const unsigned long apr = GICH[GICH_APR]; + int mask_priority; + int active_priority; + int rc = 0; + + mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK; + active_priority = find_next_bit(&apr, 32, 0); + + 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. */ + + /* find the first enabled non-active irq, the queue is already + * ordered by priority */ + list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight ) + { + if ( GIC_PRI_TO_GUEST(p->priority) >= mask_priority ) + goto out; + if ( GIC_PRI_TO_GUEST(p->priority) >= active_priority ) + goto out; + if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) + { + rc = 1; + goto out; + } + } + +out: + spin_unlock_irqrestore(&v->arch.vgic.lock, flags); + return rc; } void gic_inject(void) diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h index 59ce196..d689675 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 aede45c..bf6fb1e 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
Introduce GIC_IRQ_GUEST_ACTIVE to track which irqs are currently active in the guest. gic_events_need_delivery should only return positive if an outstanding pending irq has an higher group priority than the currently active group priotity and the priority mask. Read GICH_APR to find the active group priority. Read GICH_VMCR to find the priority mask. Find the highest priority non-active enabled irq by going through the inflight list. 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 v9: - gic_events_need_delivery: read GICH_APR to find the active group priority; - gic_events_need_delivery: simplify the search loop. Changes in v8: - in gic_restore_pending_irqs rename i to lr; - add in code comments in gic_restore_pending_irqs and gic_events_need_delivery; - add << 3 to mask_priority. 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 | 85 ++++++++++++++++++++++++++++++++++++++---- xen/include/asm-arm/domain.h | 5 ++- xen/include/asm-arm/gic.h | 3 ++ 3 files changed, 83 insertions(+), 10 deletions(-)