Stefano Stabellini
2013-Feb-15 18:49 UTC
[PATCH 2/4] xen/arm: do not use is_running to decide whether we can write directly to the LR registers
During context switch is_running is set for the next vcpu before the
gic state is actually saved.
This leads to possible nasty races when interrupts need to be injected
after is_running is set to the next vcpu but before the currently
running gic state has been saved from the previous vcpu.
Introduce a new gic_running internal variable to precisely determine
which one is the vcpu currently using the gic.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
xen/arch/arm/gic.c | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 0ecc0f1..88f2d3a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -53,6 +53,7 @@ static irq_desc_t irq_desc[NR_IRQS];
static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
static DEFINE_PER_CPU(uint64_t, lr_mask);
static gic_callback_fn_t gic_callbacks[NR_IRQS];
+static struct vcpu *gic_running;
unsigned nr_lrs;
@@ -70,6 +71,7 @@ void gic_save_state(struct vcpu *v)
for ( i=0; i<nr_lrs; i++)
v->arch.gic_lr[i] = GICH[GICH_LR + i];
v->arch.lr_mask = this_cpu(lr_mask);
+ gic_running = NULL;
spin_unlock_irq(&gic.lock);
/* Disable until next VCPU scheduled */
GICH[GICH_HCR] = 0;
@@ -81,12 +83,16 @@ void gic_restore_state(struct vcpu *v)
int i;
if ( is_idle_vcpu(v) )
+ {
+ gic_running = v;
return;
+ }
spin_lock_irq(&gic.lock);
this_cpu(lr_mask) = v->arch.lr_mask;
for ( i=0; i<nr_lrs; i++)
GICH[GICH_LR + i] = v->arch.gic_lr[i];
+ gic_running = v;
spin_unlock_irq(&gic.lock);
GICH[GICH_HCR] = GICH_HCR_EN;
isb();
@@ -481,7 +487,7 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int
virtual_irq,
spin_lock_irqsave(&gic.lock, flags);
- if ( v->is_running &&
list_empty(&v->arch.vgic.lr_pending) )
+ if ( v == gic_running &&
list_empty(&v->arch.vgic.lr_pending) )
{
i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
if (i < nr_lrs) {
--
1.7.2.5
Stefano Stabellini
2013-Feb-15 19:50 UTC
Re: [PATCH 2/4] xen/arm: do not use is_running to decide whether we can write directly to the LR registers
On Fri, 15 Feb 2013, Stefano Stabellini wrote:> During context switch is_running is set for the next vcpu before the > gic state is actually saved. > This leads to possible nasty races when interrupts need to be injected > after is_running is set to the next vcpu but before the currently > running gic state has been saved from the previous vcpu. > Introduce a new gic_running internal variable to precisely determine > which one is the vcpu currently using the gic. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>Although the description of the problem is accurate, the fix doesn''t take SMP into consideration. Probably gic_running needs to be a per_cpu variable. In any case, I''ll rework and resent the patch.> xen/arch/arm/gic.c | 8 +++++++- > 1 files changed, 7 insertions(+), 1 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index 0ecc0f1..88f2d3a 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -53,6 +53,7 @@ static irq_desc_t irq_desc[NR_IRQS]; > static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc); > static DEFINE_PER_CPU(uint64_t, lr_mask); > static gic_callback_fn_t gic_callbacks[NR_IRQS]; > +static struct vcpu *gic_running; > > unsigned nr_lrs; > > @@ -70,6 +71,7 @@ void gic_save_state(struct vcpu *v) > for ( i=0; i<nr_lrs; i++) > v->arch.gic_lr[i] = GICH[GICH_LR + i]; > v->arch.lr_mask = this_cpu(lr_mask); > + gic_running = NULL; > spin_unlock_irq(&gic.lock); > /* Disable until next VCPU scheduled */ > GICH[GICH_HCR] = 0; > @@ -81,12 +83,16 @@ void gic_restore_state(struct vcpu *v) > int i; > > if ( is_idle_vcpu(v) ) > + { > + gic_running = v; > return; > + } > > spin_lock_irq(&gic.lock); > this_cpu(lr_mask) = v->arch.lr_mask; > for ( i=0; i<nr_lrs; i++) > GICH[GICH_LR + i] = v->arch.gic_lr[i]; > + gic_running = v; > spin_unlock_irq(&gic.lock); > GICH[GICH_HCR] = GICH_HCR_EN; > isb(); > @@ -481,7 +487,7 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq, > > spin_lock_irqsave(&gic.lock, flags); > > - if ( v->is_running && list_empty(&v->arch.vgic.lr_pending) ) > + if ( v == gic_running && list_empty(&v->arch.vgic.lr_pending) ) > { > i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs); > if (i < nr_lrs) { > -- > 1.7.2.5 >