* [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.