* [PATCH v7 0/2] xen/arm: remove race conditions in irq migration @ 2017-04-05 20:28 Stefano Stabellini 2017-04-05 20:28 ` [PATCH v7 1/2] arm: remove irq from inflight, then change physical affinity Stefano Stabellini 0 siblings, 1 reply; 4+ messages in thread From: Stefano Stabellini @ 2017-04-05 20:28 UTC (permalink / raw) To: julien.grall; +Cc: xen-devel, sstabellini Hi all, this patch series removes three race conditions affecting the current code base. The first race condition is between gic_update_one_lr and vgic_vcpu_inject_irq: as soon as gic_update_one_lr calls irq_set_affinity a new interrupt could be injected in the new pcpu, eventually vgic_vcpu_inject_irq is called which manipulates the inflight list. The first patch solves this race by adding a barrier in gic_update_one_lr. This patch was suggested by Julien. The second race condition happens when gic_update_one_lr runs simultaneously with vgic_store_itargetsr and vgic_migrate_irq. Setting the new target is done after calling vgic_migrate_irq, which means that gic_update_one_lr could end up setting the physical affinity to the one of the old pcpu. The third race condition happens again between gic_update_one_lr and vgic_migrate_irq: when GIC_IRQ_GUEST_MIGRATING is already set and vgic_migrate_irq is called again, it will take a different vgic lock from the one that gic_update_one_lr is taking. The second patch addressed the last two issues by refusing any irq migration requests while one request is already in-progress and not yet completed. For your reference, it is not possible to take the p->desc lock from gic_update_one_lr, because the correct lock ordering is p->desc lock, then vgic lock. Changes in v7: - add reviewed-by - code style fix - s/%d/%u - gdprintk/gprintk Changes in v6: - smp_mb/smb_wmb - refuse nested irq migration requests instead of trying to handle them Stefano Stabellini (2): arm: remove irq from inflight, then change physical affinity vgic: refuse irq migration when one is already in progress xen/arch/arm/gic.c | 10 +++++++++- xen/arch/arm/vgic-v2.c | 7 +++---- xen/arch/arm/vgic-v3.c | 7 ++++--- xen/arch/arm/vgic.c | 14 +++++++++----- xen/include/asm-arm/vgic.h | 2 +- 5 files changed, 26 insertions(+), 14 deletions(-) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v7 1/2] arm: remove irq from inflight, then change physical affinity 2017-04-05 20:28 [PATCH v7 0/2] xen/arm: remove race conditions in irq migration Stefano Stabellini @ 2017-04-05 20:28 ` Stefano Stabellini 2017-04-05 20:28 ` [PATCH v7 2/2] vgic: refuse irq migration when one is already in progress Stefano Stabellini 0 siblings, 1 reply; 4+ messages in thread From: Stefano Stabellini @ 2017-04-05 20:28 UTC (permalink / raw) To: julien.grall; +Cc: xen-devel, sstabellini This patch fixes a potential race that could happen when gic_update_one_lr and vgic_vcpu_inject_irq run simultaneously. When GIC_IRQ_GUEST_MIGRATING is set, we must make sure that the irq has been removed from inflight before changing physical affinity, to avoid concurrent accesses to p->inflight, as vgic_vcpu_inject_irq will take a different vcpu lock. Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> Reviewed-by: Julien Grall <julien.grall@arm.com> --- xen/arch/arm/gic.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 9522c6c..2455d8f 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -503,6 +503,13 @@ static void gic_update_one_lr(struct vcpu *v, int i) gic_raise_guest_irq(v, irq, p->priority); else { list_del_init(&p->inflight); + /* + * Remove from inflight, then change physical affinity. It + * makes sure that when a new interrupt is received on the + * next pcpu, inflight is already cleared. No concurrent + * accesses to inflight. + */ + smp_wmb(); if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) { struct vcpu *v_target = vgic_get_target_vcpu(v, irq); -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v7 2/2] vgic: refuse irq migration when one is already in progress 2017-04-05 20:28 ` [PATCH v7 1/2] arm: remove irq from inflight, then change physical affinity Stefano Stabellini @ 2017-04-05 20:28 ` Stefano Stabellini 2017-04-06 23:12 ` Julien Grall 0 siblings, 1 reply; 4+ messages in thread From: Stefano Stabellini @ 2017-04-05 20:28 UTC (permalink / raw) To: julien.grall; +Cc: xen-devel, sstabellini When an irq migration is already in progress, but not yet completed (GIC_IRQ_GUEST_MIGRATING is set), refuse any other irq migration requests for the same irq. This patch implements this approach by returning success or failure from vgic_migrate_irq, and avoiding irq target changes on failure. It prints a warning in case the irq migration fails. It also moves the clear_bit of GIC_IRQ_GUEST_MIGRATING to after the physical irq affinity has been changed so that all operations regarding irq migration are completed. Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> --- xen/arch/arm/gic.c | 3 ++- xen/arch/arm/vgic-v2.c | 7 +++---- xen/arch/arm/vgic-v3.c | 7 ++++--- xen/arch/arm/vgic.c | 14 +++++++++----- xen/include/asm-arm/vgic.h | 2 +- 5 files changed, 19 insertions(+), 14 deletions(-) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 2455d8f..f943931 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -510,10 +510,11 @@ static void gic_update_one_lr(struct vcpu *v, int i) * accesses to inflight. */ smp_wmb(); - if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) + if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) { struct vcpu *v_target = vgic_get_target_vcpu(v, irq); irq_set_affinity(p->desc, cpumask_of(v_target->processor)); + clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status); } } } diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c index 0674f7b..dc9f95b 100644 --- a/xen/arch/arm/vgic-v2.c +++ b/xen/arch/arm/vgic-v2.c @@ -156,12 +156,11 @@ static void vgic_store_itargetsr(struct domain *d, struct vgic_irq_rank *rank, /* Only migrate the vIRQ if the target vCPU has changed */ if ( new_target != old_target ) { - vgic_migrate_irq(d->vcpu[old_target], + if ( vgic_migrate_irq(d->vcpu[old_target], d->vcpu[new_target], - virq); + virq) ) + write_atomic(&rank->vcpu[offset], new_target); } - - write_atomic(&rank->vcpu[offset], new_target); } } diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c index 0679e76..1e9890b 100644 --- a/xen/arch/arm/vgic-v3.c +++ b/xen/arch/arm/vgic-v3.c @@ -151,9 +151,10 @@ static void vgic_store_irouter(struct domain *d, struct vgic_irq_rank *rank, /* Only migrate the IRQ if the target vCPU has changed */ if ( new_vcpu != old_vcpu ) - vgic_migrate_irq(old_vcpu, new_vcpu, virq); - - write_atomic(&rank->vcpu[offset], new_vcpu->vcpu_id); + { + if ( vgic_migrate_irq(old_vcpu, new_vcpu, virq) ) + write_atomic(&rank->vcpu[offset], new_vcpu->vcpu_id); + } } static inline bool vgic_reg64_check_access(struct hsr_dabt dabt) diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 67d75a6..83569b0 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -237,18 +237,21 @@ static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq) return priority; } -void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) +bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) { unsigned long flags; struct pending_irq *p = irq_to_pending(old, irq); /* nothing to do for virtual interrupts */ if ( p->desc == NULL ) - return; + return true; /* migration already in progress, no need to do anything */ if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) ) - return; + { + gprintk(XENLOG_WARNING, "irq %u migration failed: requested while in progress\n", irq); + return false; + } perfc_incr(vgic_irq_migrates); @@ -258,7 +261,7 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) { irq_set_affinity(p->desc, cpumask_of(new->processor)); spin_unlock_irqrestore(&old->arch.vgic.lock, flags); - return; + return true; } /* If the IRQ is still lr_pending, re-inject it to the new vcpu */ if ( !list_empty(&p->lr_queue) ) @@ -269,7 +272,7 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) irq_set_affinity(p->desc, cpumask_of(new->processor)); spin_unlock_irqrestore(&old->arch.vgic.lock, flags); vgic_vcpu_inject_irq(new, irq); - return; + return true; } /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING * and wait for the EOI */ @@ -277,6 +280,7 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status); spin_unlock_irqrestore(&old->arch.vgic.lock, flags); + return true; } void arch_move_irqs(struct vcpu *v) diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h index 894c3f1..544867a 100644 --- a/xen/include/asm-arm/vgic.h +++ b/xen/include/asm-arm/vgic.h @@ -314,7 +314,7 @@ extern int vcpu_vgic_free(struct vcpu *v); extern bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int virq, const struct sgi_target *target); -extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq); +extern bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq); /* Reserve a specific guest vIRQ */ extern bool vgic_reserve_virq(struct domain *d, unsigned int virq); -- 1.9.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v7 2/2] vgic: refuse irq migration when one is already in progress 2017-04-05 20:28 ` [PATCH v7 2/2] vgic: refuse irq migration when one is already in progress Stefano Stabellini @ 2017-04-06 23:12 ` Julien Grall 0 siblings, 0 replies; 4+ messages in thread From: Julien Grall @ 2017-04-06 23:12 UTC (permalink / raw) To: Stefano Stabellini; +Cc: xen-devel Hi Stefano, On 04/05/2017 09:28 PM, Stefano Stabellini wrote: > When an irq migration is already in progress, but not yet completed > (GIC_IRQ_GUEST_MIGRATING is set), refuse any other irq migration > requests for the same irq. > > This patch implements this approach by returning success or failure from > vgic_migrate_irq, and avoiding irq target changes on failure. It prints > a warning in case the irq migration fails. > > It also moves the clear_bit of GIC_IRQ_GUEST_MIGRATING to after the > physical irq affinity has been changed so that all operations regarding > irq migration are completed. > > Signed-off-by: Stefano Stabellini <sstabellini@kernel.org> Reviewed-by: Julien Grall <julien.grall@arm.com> Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-06 23:12 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-04-05 20:28 [PATCH v7 0/2] xen/arm: remove race conditions in irq migration Stefano Stabellini 2017-04-05 20:28 ` [PATCH v7 1/2] arm: remove irq from inflight, then change physical affinity Stefano Stabellini 2017-04-05 20:28 ` [PATCH v7 2/2] vgic: refuse irq migration when one is already in progress Stefano Stabellini 2017-04-06 23:12 ` Julien Grall
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.