Xen devel - Feb 2013 - [PATCH v2 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.

Use current instead of is_running to check which one is the currently
running vcpu: set_current is called right before __context_switch and
schedule_tail with interrupt disabled.

Re-enabled interrupts after ctxt_switch_from, so that all the context
switch saving functions don''t have to worry about receiving interrupts
while saving state.


Changes in v2:
- rework the patch to run ctxt_switch_from with interrupt disabled,
rather than introducing a gic_running internal variable.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/domain.c |    5 ++---
 xen/arch/arm/gic.c    |    4 +---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e37ec54..40979e2 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -168,11 +168,10 @@ static void ctxt_switch_to(struct vcpu *n)
 
 static void schedule_tail(struct vcpu *prev)
 {
-    /* Re-enable interrupts before restoring state which may fault. */
-    local_irq_enable();
-
     ctxt_switch_from(prev);
 
+    local_irq_enable();
+
     /* TODO
        update_runstate_area(current);
     */
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index ac1f939..2d0b052 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -65,11 +65,9 @@ void gic_save_state(struct vcpu *v)
 {
     int i;
 
-    spin_lock_irq(&gic.lock);
     for ( i=0; i<nr_lrs; i++)
         v->arch.gic_lr[i] = GICH[GICH_LR + i];
     v->arch.lr_mask = this_cpu(lr_mask);
-    spin_unlock_irq(&gic.lock);
     /* Disable until next VCPU scheduled */
     GICH[GICH_HCR] = 0;
     isb();
@@ -480,7 +478,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 == current && 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

On Mon, 2013-02-18 at 16:02 +0000, 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.
> 
> Use current instead of is_running to check which one is the currently
> running vcpu: set_current is called right before __context_switch and
> schedule_tail with interrupt disabled.
> 
> Re-enabled interrupts after ctxt_switch_from, so that all the context
> switch saving functions don''t have to worry about receiving
interrupts
> while saving state.
> 
> 
> Changes in v2:
> - rework the patch to run ctxt_switch_from with interrupt disabled,
> rather than introducing a gic_running internal variable.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/domain.c |    5 ++---
>  xen/arch/arm/gic.c    |    4 +---
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index e37ec54..40979e2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -168,11 +168,10 @@ static void ctxt_switch_to(struct vcpu *n)
>  
>  static void schedule_tail(struct vcpu *prev)
>  {
> -    /* Re-enable interrupts before restoring state which may fault. */
> -    local_irq_enable();
> -
>      ctxt_switch_from(prev);
>  
> +    local_irq_enable();
> +
>      /* TODO
>         update_runstate_area(current);
>      */
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index ac1f939..2d0b052 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -65,11 +65,9 @@ void gic_save_state(struct vcpu *v)
>  {
>      int i;
>  
> -    spin_lock_irq(&gic.lock);
>      for ( i=0; i<nr_lrs; i++)
>          v->arch.gic_lr[i] = GICH[GICH_LR + i];
>      v->arch.lr_mask = this_cpu(lr_mask);
> -    spin_unlock_irq(&gic.lock);
Why does this lock become unnecessary? Just because interrupts are now
disabled around this call and it only accesses v->foo which cannot be
accessed simultaneously on some other pCPU (e.g. one which is trying to
inject an event to this vCPU)?

Would be good to describe in the changelog or perhaps a comment. Maybe
even ASSERT(irqs_disbled())
>      /* Disable until next VCPU scheduled */
>      GICH[GICH_HCR] = 0;
>      isb();
> @@ -480,7 +478,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 == current &&
list_empty(&v->arch.vgic.lr_pending) )
>      {
>          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
>          if (i < nr_lrs) {

On Wed, 10 Apr 2013, Ian Campbell wrote:
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index ac1f939..2d0b052 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -65,11 +65,9 @@ void gic_save_state(struct vcpu *v)
> >  {
> >      int i;
> >  
> > -    spin_lock_irq(&gic.lock);
> >      for ( i=0; i<nr_lrs; i++)
> >          v->arch.gic_lr[i] = GICH[GICH_LR + i];
> >      v->arch.lr_mask = this_cpu(lr_mask);
> > -    spin_unlock_irq(&gic.lock);
> 
> Why does this lock become unnecessary? Just because interrupts are now
> disabled around this call and it only accesses v->foo which cannot be
> accessed simultaneously on some other pCPU (e.g. one which is trying to
> inject an event to this vCPU)?
That''s right.
> Would be good to describe in the changelog or perhaps a comment. Maybe
> even ASSERT(irqs_disbled())
Good idea.