| Message ID | 20170526173540.10066-25-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 05/26/2017 06:35 PM, Andre Przywara wrote: > @@ -441,6 +443,40 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq) > #endif > } > > +/* > + * Find an unused LR to insert an IRQ into, starting with the LR given > + * by @lr. If this new interrupt is a PRISTINE LPI, scan the other LRs to > + * avoid inserting the same IRQ twice. This situation can occur when an > + * event gets discarded while the LPI is in an LR, and a new LPI with the > + * same number gets mapped quickly afterwards. > + */ > +static unsigned int gic_find_unused_lr(struct vcpu *v, > + struct pending_irq *p, > + unsigned int lr) > +{ > + unsigned int nr_lrs = gic_hw_ops->info->nr_lrs; > + unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask); > + struct gic_lr lr_val; > + > + ASSERT(spin_is_locked(&v->arch.vgic.lock)); > + > + if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) Stefano suggested to put an unlikely and ... > @@ -479,8 +516,14 @@ static void gic_update_one_lr(struct vcpu *v, int i) > gic_hw_ops->read_lr(i, &lr_val); > irq = lr_val.virq; > p = irq_to_pending(v, irq); > - /* An LPI might have been unmapped, in which case we just clean up here. */ > - if ( unlikely(!p) ) > + /* > + * An LPI might have been unmapped, in which case we just clean up here. > + * If that LPI is marked as PRISTINE, the information in the LR is bogus, > + * as it belongs to a previous, already unmapped LPI. So we discard it > + * here as well. > + */ > + if ( unlikely(!p) || > + test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) ... I think we should put one here too. With that, I think the patch looks good. I will let Stefano confirm he is happy with that too. Cheers,
On Fri, 2 Jun 2017, Julien Grall wrote: > Hi Andre, > > On 05/26/2017 06:35 PM, Andre Przywara wrote: > > @@ -441,6 +443,40 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned > > int virtual_irq) > > #endif > > } > > +/* > > + * Find an unused LR to insert an IRQ into, starting with the LR given > > + * by @lr. If this new interrupt is a PRISTINE LPI, scan the other LRs to > > + * avoid inserting the same IRQ twice. This situation can occur when an > > + * event gets discarded while the LPI is in an LR, and a new LPI with the > > + * same number gets mapped quickly afterwards. > > + */ > > +static unsigned int gic_find_unused_lr(struct vcpu *v, > > + struct pending_irq *p, > > + unsigned int lr) > > +{ > > + unsigned int nr_lrs = gic_hw_ops->info->nr_lrs; > > + unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask); > > + struct gic_lr lr_val; > > + > > + ASSERT(spin_is_locked(&v->arch.vgic.lock)); > > + > > + if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) > > Stefano suggested to put an unlikely and ... > > > @@ -479,8 +516,14 @@ static void gic_update_one_lr(struct vcpu *v, int i) > > gic_hw_ops->read_lr(i, &lr_val); > > irq = lr_val.virq; > > p = irq_to_pending(v, irq); > > - /* An LPI might have been unmapped, in which case we just clean up > > here. */ > > - if ( unlikely(!p) ) > > + /* > > + * An LPI might have been unmapped, in which case we just clean up > > here. > > + * If that LPI is marked as PRISTINE, the information in the LR is > > bogus, > > + * as it belongs to a previous, already unmapped LPI. So we discard it > > + * here as well. > > + */ > > + if ( unlikely(!p) || > > + test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) > > ... I think we should put one here too. > > With that, I think the patch looks good. I will let Stefano confirm he is > happy with that too. Yes, I think it is OK
Hi Andre, On 26/05/17 18:35, Andre Przywara wrote: > +/* > + * Find an unused LR to insert an IRQ into, starting with the LR given > + * by @lr. If this new interrupt is a PRISTINE LPI, scan the other LRs to > + * avoid inserting the same IRQ twice. This situation can occur when an > + * event gets discarded while the LPI is in an LR, and a new LPI with the > + * same number gets mapped quickly afterwards. > + */ > +static unsigned int gic_find_unused_lr(struct vcpu *v, > + struct pending_irq *p, > + unsigned int lr) > +{ > + unsigned int nr_lrs = gic_hw_ops->info->nr_lrs; > + unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask); > + struct gic_lr lr_val; > + > + ASSERT(spin_is_locked(&v->arch.vgic.lock)); > + > + if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) > + { > + unsigned int used_lr = 0; > + > + while ( (used_lr = find_next_bit(lr_mask, nr_lrs, used_lr)) < nr_lrs ) This loop is incorrect. find_next_bit will find the next set bit start at the offset used_lr. So if used_lr is set and does match the virq, this will turned into an infinite loop. I would use the macro for_each_set_bit (see xen/bitops.h) here that will handle the problem for you: for_each_set_bit( used_lr, lr_mask, nr_lrs ) { .... } Cheers,
Hi, On 08/06/17 10:45, Julien Grall wrote: > Hi Andre, > > On 26/05/17 18:35, Andre Przywara wrote: >> +/* >> + * Find an unused LR to insert an IRQ into, starting with the LR given >> + * by @lr. If this new interrupt is a PRISTINE LPI, scan the other >> LRs to >> + * avoid inserting the same IRQ twice. This situation can occur when an >> + * event gets discarded while the LPI is in an LR, and a new LPI with >> the >> + * same number gets mapped quickly afterwards. >> + */ >> +static unsigned int gic_find_unused_lr(struct vcpu *v, >> + struct pending_irq *p, >> + unsigned int lr) >> +{ >> + unsigned int nr_lrs = gic_hw_ops->info->nr_lrs; >> + unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask); >> + struct gic_lr lr_val; >> + >> + ASSERT(spin_is_locked(&v->arch.vgic.lock)); >> + >> + if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) >> + { >> + unsigned int used_lr = 0; >> + >> + while ( (used_lr = find_next_bit(lr_mask, nr_lrs, used_lr)) < >> nr_lrs ) > > This loop is incorrect. find_next_bit will find the next set bit start > at the offset used_lr. So if used_lr is set and does match the virq, > this will turned into an infinite loop. Thanks for catching this! Fixed as hinted below. Cheers, Andre. > I would use the macro for_each_set_bit (see xen/bitops.h) here that will > handle the problem for you: > > for_each_set_bit( used_lr, lr_mask, nr_lrs ) > { > .... > } > > Cheers, >
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 9e32103..da5ba86 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -375,6 +375,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p, { ASSERT(!local_irq_is_enabled()); + clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status); + gic_hw_ops->update_lr(lr, p, state); set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); @@ -441,6 +443,40 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq) #endif } +/* + * Find an unused LR to insert an IRQ into, starting with the LR given + * by @lr. If this new interrupt is a PRISTINE LPI, scan the other LRs to + * avoid inserting the same IRQ twice. This situation can occur when an + * event gets discarded while the LPI is in an LR, and a new LPI with the + * same number gets mapped quickly afterwards. + */ +static unsigned int gic_find_unused_lr(struct vcpu *v, + struct pending_irq *p, + unsigned int lr) +{ + unsigned int nr_lrs = gic_hw_ops->info->nr_lrs; + unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask); + struct gic_lr lr_val; + + ASSERT(spin_is_locked(&v->arch.vgic.lock)); + + if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) + { + unsigned int used_lr = 0; + + while ( (used_lr = find_next_bit(lr_mask, nr_lrs, used_lr)) < nr_lrs ) + { + gic_hw_ops->read_lr(used_lr, &lr_val); + if ( lr_val.virq == p->irq ) + return used_lr; + } + } + + lr = find_next_zero_bit(lr_mask, nr_lrs, lr); + + return lr; +} + void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq, unsigned int priority) { @@ -456,7 +492,8 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq, if ( v == current && list_empty(&v->arch.vgic.lr_pending) ) { - i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs); + i = gic_find_unused_lr(v, p, 0); + if (i < nr_lrs) { set_bit(i, &this_cpu(lr_mask)); gic_set_lr(i, p, GICH_LR_PENDING); @@ -479,8 +516,14 @@ static void gic_update_one_lr(struct vcpu *v, int i) gic_hw_ops->read_lr(i, &lr_val); irq = lr_val.virq; p = irq_to_pending(v, irq); - /* An LPI might have been unmapped, in which case we just clean up here. */ - if ( unlikely(!p) ) + /* + * An LPI might have been unmapped, in which case we just clean up here. + * If that LPI is marked as PRISTINE, the information in the LR is bogus, + * as it belongs to a previous, already unmapped LPI. So we discard it + * here as well. + */ + if ( unlikely(!p) || + test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) { ASSERT(is_lpi(irq)); @@ -590,7 +633,7 @@ static void gic_restore_pending_irqs(struct vcpu *v) inflight_r = &v->arch.vgic.inflight_irqs; list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue ) { - lr = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, lr); + lr = gic_find_unused_lr(v, p, lr); if ( lr >= nr_lrs ) { /* No more free LRs: find a lower priority irq to evict */ diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 02732db..3fc4ceb 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -60,12 +60,18 @@ struct pending_irq * vcpu while it is still inflight and on an GICH_LR register on the * old vcpu. * + * GIC_IRQ_GUEST_PRISTINE_LPI: the IRQ is a newly mapped LPI, which + * has never been in an LR before. This means that any trace of an + * LPI with the same number in an LR must be from an older LPI, which + * has been unmapped before. + * */ #define GIC_IRQ_GUEST_QUEUED 0 #define GIC_IRQ_GUEST_ACTIVE 1 #define GIC_IRQ_GUEST_VISIBLE 2 #define GIC_IRQ_GUEST_ENABLED 3 #define GIC_IRQ_GUEST_MIGRATING 4 +#define GIC_IRQ_GUEST_PRISTINE_LPI 5 unsigned long status; struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */ unsigned int irq;
When LPIs get unmapped by a guest, they might still be in some LR of some VCPU. Nevertheless we remove the corresponding pending_irq (possibly freeing it), and detect this case (irq_to_pending() returns NULL) when the LR gets cleaned up later. However a *new* LPI may get mapped with the same number while the old LPI is *still* in some LR. To avoid getting the wrong state, we mark every newly mapped LPI as PRISTINE, which means: has never been in an LR before. If we detect the LPI in an LR anyway, it must have been an older one, which we can simply retire. Before inserting such a PRISTINE LPI into an LR, we must make sure that it's not already in another LR, as the architecture forbids two interrupts with the same virtual IRQ number on one CPU. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic.c | 51 ++++++++++++++++++++++++++++++++++++++++++---- xen/include/asm-arm/vgic.h | 6 ++++++ 2 files changed, 53 insertions(+), 4 deletions(-)