diff mbox

[Xen-devel,v9,10/10] xen/arm: make accesses to desc->status flags atomic

Message ID 1406223192-26267-10-git-send-email-stefano.stabellini@eu.citrix.com
State New
Headers show

Commit Message

Stefano Stabellini July 24, 2014, 5:33 p.m. UTC 
Using *_bit manipulation functions on desc->status is safe on arm64:
status is an unsigned int but is the first field of a struct that
contains pointers, therefore the alignement of the struct is at least 8
bytes.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Changes in v2:
- rebase on ab78724fc5628318b172b4344f7280621a151e1b.
---
 xen/arch/arm/gic-v2.c |    4 ++--
 xen/arch/arm/gic.c    |    6 +++---
 xen/arch/arm/irq.c    |   33 +++++++++++++++++----------------
 3 files changed, 22 insertions(+), 21 deletions(-)

Comments

Julien Grall July 25, 2014, 12:40 p.m. UTC | #1 
Hi Stefano,

On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> Using *_bit manipulation functions on desc->status is safe on arm64:
> status is an unsigned int but is the first field of a struct that
> contains pointers, therefore the alignement of the struct is at least 8
> bytes.

I would add a comment in the structure irq_desc in include/common/irq.h.
To explain this assumption.

It would avoid people breaking the structure by mistake in the future.

Also I would explain a bit more why we need to use atomic while most of
the access are protected by desc->lock.

[..]

> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> index 7150c7a..0097349 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -126,7 +126,7 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
>  {
>      ASSERT(spin_is_locked(&desc->lock));
>  
> -    if ( !(desc->status & IRQ_GUEST) )
> +    if ( !test_bit(_IRQ_GUEST, &desc->status) )
>          return dom_xen;
>  
>      ASSERT(desc->action != NULL);
> @@ -195,13 +195,13 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>          goto out;
>      }
>  
> -    if ( desc->status & IRQ_GUEST )
> +    if ( test_bit(_IRQ_GUEST, &desc->status) )
>      {
>          struct domain *d = irq_get_domain(desc);
>  
>          desc->handler->end(desc);
>  
> -        desc->status |= IRQ_INPROGRESS;
> +        set_bit(_IRQ_INPROGRESS, &desc->status);
>          desc->arch.eoi_cpu = smp_processor_id();
>  
>          /* the irq cannot be a PPI, we only support delivery of SPIs to
> @@ -210,22 +210,23 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
>          goto out_no_end;
>      }
>  
> -    desc->status |= IRQ_PENDING;
> +    set_bit(_IRQ_PENDING, &desc->status);
>  
>      /*
>       * Since we set PENDING, if another processor is handling a different
>       * instance of this same irq, the other processor will take care of it.
>       */
> -    if ( desc->status & (IRQ_DISABLED | IRQ_INPROGRESS) )
> +    if ( test_bit(_IRQ_DISABLED, &desc->status) ||
> +         test_bit(_IRQ_INPROGRESS, &desc->status) )

You replaced 1 access to desc->status by 2 accesses. I think it may have
a race condition when the desc->lock it's not taken (such as in
gic_update_one_lr).

At the same time, this will never happen as a guest IRQ will never
execute this code.

If it happens for IRQ routed to Xen, the interrupt will be EOIed and
fire again as soon as we leave the IRQ exception mode.

So I think we are fine for now.

Regards,
Stefano Stabellini July 28, 2014, 4:31 p.m. UTC | #2 
On Fri, 25 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> > Using *_bit manipulation functions on desc->status is safe on arm64:
> > status is an unsigned int but is the first field of a struct that
> > contains pointers, therefore the alignement of the struct is at least 8
> > bytes.
> 
> I would add a comment in the structure irq_desc in include/common/irq.h.
> To explain this assumption.
> 
> It would avoid people breaking the structure by mistake in the future.

I can do that


> Also I would explain a bit more why we need to use atomic while most of
> the access are protected by desc->lock.

OK


> [..]
> 
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index 7150c7a..0097349 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -126,7 +126,7 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
> >  {
> >      ASSERT(spin_is_locked(&desc->lock));
> >  
> > -    if ( !(desc->status & IRQ_GUEST) )
> > +    if ( !test_bit(_IRQ_GUEST, &desc->status) )
> >          return dom_xen;
> >  
> >      ASSERT(desc->action != NULL);
> > @@ -195,13 +195,13 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
> >          goto out;
> >      }
> >  
> > -    if ( desc->status & IRQ_GUEST )
> > +    if ( test_bit(_IRQ_GUEST, &desc->status) )
> >      {
> >          struct domain *d = irq_get_domain(desc);
> >  
> >          desc->handler->end(desc);
> >  
> > -        desc->status |= IRQ_INPROGRESS;
> > +        set_bit(_IRQ_INPROGRESS, &desc->status);
> >          desc->arch.eoi_cpu = smp_processor_id();
> >  
> >          /* the irq cannot be a PPI, we only support delivery of SPIs to
> > @@ -210,22 +210,23 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
> >          goto out_no_end;
> >      }
> >  
> > -    desc->status |= IRQ_PENDING;
> > +    set_bit(_IRQ_PENDING, &desc->status);
> >  
> >      /*
> >       * Since we set PENDING, if another processor is handling a different
> >       * instance of this same irq, the other processor will take care of it.
> >       */
> > -    if ( desc->status & (IRQ_DISABLED | IRQ_INPROGRESS) )
> > +    if ( test_bit(_IRQ_DISABLED, &desc->status) ||
> > +         test_bit(_IRQ_INPROGRESS, &desc->status) )
> 
> You replaced 1 access to desc->status by 2 accesses. I think it may have
> a race condition when the desc->lock it's not taken (such as in
> gic_update_one_lr).

It wasn't guaranteed to be atomic before either.


> At the same time, this will never happen as a guest IRQ will never
> execute this code.
> 
> If it happens for IRQ routed to Xen, the interrupt will be EOIed and
> fire again as soon as we leave the IRQ exception mode.
> 
> So I think we are fine for now.

I think so too.
Julien Grall July 28, 2014, 5:14 p.m. UTC | #3 
Hi Stefano,

On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> @@ -305,13 +306,13 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
>       *  - if the IRQ is marked as shared
>       *  - dev_id is not NULL when IRQF_SHARED is set
>       */
> -    if ( desc->action != NULL && (!(desc->status & IRQF_SHARED) || !shared) )
> +    if ( desc->action != NULL && (!test_bit(_IRQF_SHARED, &desc->status) || !shared) )
>          return -EINVAL;
>      if ( shared && new->dev_id == NULL )
>          return -EINVAL;
>  
>      if ( shared )
> -        desc->status |= IRQF_SHARED;
> +        set_bit(IRQF_SHARED, &desc->status);

Shouldn't it be _IRQF_SHARED?

Regards,
Stefano Stabellini July 29, 2014, 10:25 a.m. UTC | #4 
On Mon, 28 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> > @@ -305,13 +306,13 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
> >       *  - if the IRQ is marked as shared
> >       *  - dev_id is not NULL when IRQF_SHARED is set
> >       */
> > -    if ( desc->action != NULL && (!(desc->status & IRQF_SHARED) || !shared) )
> > +    if ( desc->action != NULL && (!test_bit(_IRQF_SHARED, &desc->status) || !shared) )
> >          return -EINVAL;
> >      if ( shared && new->dev_id == NULL )
> >          return -EINVAL;
> >  
> >      if ( shared )
> > -        desc->status |= IRQF_SHARED;
> > +        set_bit(IRQF_SHARED, &desc->status);
> 
> Shouldn't it be _IRQF_SHARED?

well spotted! thanks!
diff mbox

Patch

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index da60a41..78ad4de 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -516,7 +516,7 @@  static void gicv2_irq_enable(struct irq_desc *desc)
     ASSERT(spin_is_locked(&desc->lock));
 
     spin_lock_irqsave(&gicv2.lock, flags);
-    desc->status &= ~IRQ_DISABLED;
+    clear_bit(_IRQ_DISABLED, &desc->status);
     dsb(sy);
     /* Enable routing */
     writel_gicd((1u << (irq % 32)), GICD_ISENABLER + (irq / 32) * 4);
@@ -533,7 +533,7 @@  static void gicv2_irq_disable(struct irq_desc *desc)
     spin_lock_irqsave(&gicv2.lock, flags);
     /* Disable routing */
     writel_gicd(1u << (irq % 32), GICD_ICENABLER + (irq / 32) * 4);
-    desc->status |= IRQ_DISABLED;
+    set_bit(_IRQ_DISABLED, &desc->status);
     spin_unlock_irqrestore(&gicv2.lock, flags);
 }
 
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 2aa9500..6611ba0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -115,7 +115,7 @@  void gic_route_irq_to_xen(struct irq_desc *desc, const cpumask_t *cpu_mask,
 {
     ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
     ASSERT(desc->irq < gic_number_lines());/* Can't route interrupts that don't exist */
-    ASSERT(desc->status & IRQ_DISABLED);
+    ASSERT(test_bit(_IRQ_DISABLED, &desc->status));
     ASSERT(spin_is_locked(&desc->lock));
 
     desc->handler = gic_hw_ops->gic_host_irq_type;
@@ -133,7 +133,7 @@  void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
     ASSERT(spin_is_locked(&desc->lock));
 
     desc->handler = gic_hw_ops->gic_guest_irq_type;
-    desc->status |= IRQ_GUEST;
+    set_bit(_IRQ_GUEST, &desc->status);
 
     gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
 
@@ -369,7 +369,7 @@  static void gic_update_one_lr(struct vcpu *v, int i)
 
         if ( p->desc != NULL )
         {
-            p->desc->status &= ~IRQ_INPROGRESS;
+            clear_bit(_IRQ_INPROGRESS, &p->desc->status);
             if ( platform_has_quirk(PLATFORM_QUIRK_GUEST_PIRQ_NEED_EOI) )
                 gic_hw_ops->deactivate_irq(p->desc);
         }
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 7150c7a..0097349 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -126,7 +126,7 @@  static inline struct domain *irq_get_domain(struct irq_desc *desc)
 {
     ASSERT(spin_is_locked(&desc->lock));
 
-    if ( !(desc->status & IRQ_GUEST) )
+    if ( !test_bit(_IRQ_GUEST, &desc->status) )
         return dom_xen;
 
     ASSERT(desc->action != NULL);
@@ -195,13 +195,13 @@  void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         goto out;
     }
 
-    if ( desc->status & IRQ_GUEST )
+    if ( test_bit(_IRQ_GUEST, &desc->status) )
     {
         struct domain *d = irq_get_domain(desc);
 
         desc->handler->end(desc);
 
-        desc->status |= IRQ_INPROGRESS;
+        set_bit(_IRQ_INPROGRESS, &desc->status);
         desc->arch.eoi_cpu = smp_processor_id();
 
         /* the irq cannot be a PPI, we only support delivery of SPIs to
@@ -210,22 +210,23 @@  void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         goto out_no_end;
     }
 
-    desc->status |= IRQ_PENDING;
+    set_bit(_IRQ_PENDING, &desc->status);
 
     /*
      * Since we set PENDING, if another processor is handling a different
      * instance of this same irq, the other processor will take care of it.
      */
-    if ( desc->status & (IRQ_DISABLED | IRQ_INPROGRESS) )
+    if ( test_bit(_IRQ_DISABLED, &desc->status) ||
+         test_bit(_IRQ_INPROGRESS, &desc->status) )
         goto out;
 
-    desc->status |= IRQ_INPROGRESS;
+    set_bit(_IRQ_INPROGRESS, &desc->status);
 
-    while ( desc->status & IRQ_PENDING )
+    while ( test_bit(_IRQ_PENDING, &desc->status) )
     {
         struct irqaction *action;
 
-        desc->status &= ~IRQ_PENDING;
+        clear_bit(_IRQ_PENDING, &desc->status);
         action = desc->action;
 
         spin_unlock_irq(&desc->lock);
@@ -239,7 +240,7 @@  void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         spin_lock_irq(&desc->lock);
     }
 
-    desc->status &= ~IRQ_INPROGRESS;
+    clear_bit(_IRQ_INPROGRESS, &desc->status);
 
 out:
     desc->handler->end(desc);
@@ -282,13 +283,13 @@  void release_irq(unsigned int irq, const void *dev_id)
     if ( !desc->action )
     {
         desc->handler->shutdown(desc);
-        desc->status &= ~IRQ_GUEST;
+        clear_bit(_IRQ_GUEST, &desc->status);
     }
 
     spin_unlock_irqrestore(&desc->lock,flags);
 
     /* Wait to make sure it's not being used on another CPU */
-    do { smp_mb(); } while ( desc->status & IRQ_INPROGRESS );
+    do { smp_mb(); } while ( test_bit(_IRQ_INPROGRESS, &desc->status) );
 
     if ( action->free_on_release )
         xfree(action);
@@ -305,13 +306,13 @@  static int __setup_irq(struct irq_desc *desc, unsigned int irqflags,
      *  - if the IRQ is marked as shared
      *  - dev_id is not NULL when IRQF_SHARED is set
      */
-    if ( desc->action != NULL && (!(desc->status & IRQF_SHARED) || !shared) )
+    if ( desc->action != NULL && (!test_bit(_IRQF_SHARED, &desc->status) || !shared) )
         return -EINVAL;
     if ( shared && new->dev_id == NULL )
         return -EINVAL;
 
     if ( shared )
-        desc->status |= IRQF_SHARED;
+        set_bit(IRQF_SHARED, &desc->status);
 
     new->next = desc->action;
     dsb(ish);
@@ -332,7 +333,7 @@  int setup_irq(unsigned int irq, unsigned int irqflags, struct irqaction *new)
 
     spin_lock_irqsave(&desc->lock, flags);
 
-    if ( desc->status & IRQ_GUEST )
+    if ( test_bit(_IRQ_GUEST, &desc->status) )
     {
         struct domain *d = irq_get_domain(desc);
 
@@ -396,10 +397,10 @@  int route_irq_to_guest(struct domain *d, unsigned int irq,
     {
         struct domain *ad = irq_get_domain(desc);
 
-        if ( (desc->status & IRQ_GUEST) && d == ad )
+        if ( test_bit(_IRQ_GUEST, &desc->status) && d == ad )
             goto out;
 
-        if ( desc->status & IRQ_GUEST )
+        if ( test_bit(_IRQ_GUEST, &desc->status) )
             printk(XENLOG_ERR "ERROR: IRQ %u is already used by domain %u\n",
                    irq, ad->domain_id);
         else