| Message ID | 1491957874-31600-21-git-send-email-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 12/04/17 01:44, Andre Przywara wrote: > 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. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic.c | 17 ++++++++++++++++- > xen/arch/arm/vgic-v3-its.c | 5 +++++ > xen/include/asm-arm/vgic.h | 1 + > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index d752352..e8c3202 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -373,6 +373,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); > @@ -510,7 +512,17 @@ static void gic_update_one_lr(struct vcpu *v, int i) > } > else if ( lr_val.state & GICH_LR_PENDING ) > { > - int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); > + int q __attribute__ ((unused)); > + > + if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) > + { > + gic_hw_ops->clear_lr(i); > + clear_bit(i, &this_cpu(lr_mask)); > + > + return; > + } > + > + q = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); > #ifdef GIC_DEBUG > if ( q ) > gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is already pending in LR%d\n", > @@ -522,6 +534,9 @@ static void gic_update_one_lr(struct vcpu *v, int i) > gic_hw_ops->clear_lr(i); > clear_bit(i, &this_cpu(lr_mask)); > > + if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) > + return; > + > if ( p->desc != NULL ) > clear_bit(_IRQ_INPROGRESS, &p->desc->status); > clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index b7e61b2..0765810 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -618,6 +618,11 @@ static int its_handle_mapti(struct virt_its *its, uint64_t *cmdptr) > goto out_remove_host_entry; > > pirq->lpi_vcpu_id = vcpu->vcpu_id; > + /* > + * Mark this LPI as new, so any older (now unmapped) LPI in any LR > + * can be easily recognised as such. > + */ > + set_bit(GIC_IRQ_GUEST_PRISTINE_LPI, pirq->status); Arrgh, so I somehow managed to introduce that typo after testing, but before sending (note to self: don't send stuff at 1:44 am), sorry for that! Of course it must be "..., &pirq->status);" Just compile-tested this series (with that typo fixed) again: every commit compiles without warnings for arm64 (w/ and w/o ITS configured) and arm32. Also successfully boot-tested on the model with and without ITS support. Cheers, Andre. > /* > * Now insert the pending_irq into the domain's LPI tree, so that > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index 02732db..b1a7525 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -66,6 +66,7 @@ struct pending_irq > #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; >
Hi Andre, TBH, I would have expected this patch to be split in two: - Introduction of the flag before patch #19 - Set flag in patch #19 This would have make easier to review the implementation of MAPTI. On 12/04/17 01:44, Andre Przywara wrote: > 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. From the GICv3 spec (8.4.6 in ARM IHI 0069C): "Behavior is UNPREDICTABLE if two or more List Registers specify the same vINTID when - ICH_LR<n>_EL2.State == 01. - ICH_LR<n>_EL2.State == 10. - ICH_LR<n>_EL2.State == 11. " The LPI does not have active bit and we remap the vLPI to another pLPI. So I think we might end up to have the same vLPI twice in the LR if the new and old vLPI was routed to the same vCPU which is UNPREDICABLE. Another case if both pending_irq are targeting a different vCPU. In this case, the interrupt may be injected simultaneously to different vCPU. I am not sure if it is valid... > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic.c | 17 ++++++++++++++++- > xen/arch/arm/vgic-v3-its.c | 5 +++++ > xen/include/asm-arm/vgic.h | 1 + > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index d752352..e8c3202 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -373,6 +373,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); > + This placement looks wrong, gic_set_lr is called by vgic_vcpu_inject_irq but you don't know if the LRs have been cleared yet so you could end up with the vIRQ in 2 different LRs which is UNPREDICTABLE. > gic_hw_ops->update_lr(lr, p, state); > > set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); > @@ -510,7 +512,17 @@ static void gic_update_one_lr(struct vcpu *v, int i) > } > else if ( lr_val.state & GICH_LR_PENDING ) > { > - int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); > + int q __attribute__ ((unused)); > + > + if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) > + { > + gic_hw_ops->clear_lr(i); > + clear_bit(i, &this_cpu(lr_mask)); > + > + return; > + } > + > + q = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); > #ifdef GIC_DEBUG > if ( q ) > gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is already pending in LR%d\n", > @@ -522,6 +534,9 @@ static void gic_update_one_lr(struct vcpu *v, int i) > gic_hw_ops->clear_lr(i); > clear_bit(i, &this_cpu(lr_mask)); > > + if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) > + return; > + > if ( p->desc != NULL ) > clear_bit(_IRQ_INPROGRESS, &p->desc->status); > clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c > index b7e61b2..0765810 100644 > --- a/xen/arch/arm/vgic-v3-its.c > +++ b/xen/arch/arm/vgic-v3-its.c > @@ -618,6 +618,11 @@ static int its_handle_mapti(struct virt_its *its, uint64_t *cmdptr) > goto out_remove_host_entry; > > pirq->lpi_vcpu_id = vcpu->vcpu_id; > + /* > + * Mark this LPI as new, so any older (now unmapped) LPI in any LR > + * can be easily recognised as such. > + */ > + set_bit(GIC_IRQ_GUEST_PRISTINE_LPI, pirq->status); > > /* > * Now insert the pending_irq into the domain's LPI tree, so that > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h > index 02732db..b1a7525 100644 > --- a/xen/include/asm-arm/vgic.h > +++ b/xen/include/asm-arm/vgic.h > @@ -66,6 +66,7 @@ struct pending_irq > #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 Please document this new flag... > unsigned long status; > struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */ > unsigned int irq; > Cheers,
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index d752352..e8c3202 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -373,6 +373,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); @@ -510,7 +512,17 @@ static void gic_update_one_lr(struct vcpu *v, int i) } else if ( lr_val.state & GICH_LR_PENDING ) { - int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); + int q __attribute__ ((unused)); + + if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) + { + gic_hw_ops->clear_lr(i); + clear_bit(i, &this_cpu(lr_mask)); + + return; + } + + q = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); #ifdef GIC_DEBUG if ( q ) gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is already pending in LR%d\n", @@ -522,6 +534,9 @@ static void gic_update_one_lr(struct vcpu *v, int i) gic_hw_ops->clear_lr(i); clear_bit(i, &this_cpu(lr_mask)); + if ( test_and_clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) ) + return; + if ( p->desc != NULL ) clear_bit(_IRQ_INPROGRESS, &p->desc->status); clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c index b7e61b2..0765810 100644 --- a/xen/arch/arm/vgic-v3-its.c +++ b/xen/arch/arm/vgic-v3-its.c @@ -618,6 +618,11 @@ static int its_handle_mapti(struct virt_its *its, uint64_t *cmdptr) goto out_remove_host_entry; pirq->lpi_vcpu_id = vcpu->vcpu_id; + /* + * Mark this LPI as new, so any older (now unmapped) LPI in any LR + * can be easily recognised as such. + */ + set_bit(GIC_IRQ_GUEST_PRISTINE_LPI, pirq->status); /* * Now insert the pending_irq into the domain's LPI tree, so that diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 02732db..b1a7525 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -66,6 +66,7 @@ struct pending_irq #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. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic.c | 17 ++++++++++++++++- xen/arch/arm/vgic-v3-its.c | 5 +++++ xen/include/asm-arm/vgic.h | 1 + 3 files changed, 22 insertions(+), 1 deletion(-)