diff mbox

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

Message ID 1407237989-27654-10-git-send-email-stefano.stabellini@eu.citrix.com
State Accepted
Commit 50d8fe8fcbab2440cfeeb65c4765868398652473
Headers show

Commit Message

Stefano Stabellini Aug. 5, 2014, 11:26 a.m. UTC
This way we don't need to take the desc->lock in order to access
desc->status in many of the gic and vgic functions.

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 v10:
- add in-code comment;
- fix _IRQF_SHARED renaming.

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 +++++++++++++++++----------------
 xen/include/xen/irq.h |    5 +++++
 4 files changed, 27 insertions(+), 21 deletions(-)

Comments

Julien Grall Aug. 5, 2014, 12:35 p.m. UTC | #1
Hi Stefano,

On 08/05/2014 12:26 PM, Stefano Stabellini wrote:
> This way we don't need to take the desc->lock in order to access
> desc->status in many of the gic and vgic functions.
> 
> 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>
Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,
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..25ecf1d 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
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 96d818f..ffb5932 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -76,6 +76,11 @@  struct msi_desc;
  * This is the "IRQ descriptor", which contains various information
  * about the irq, including what kind of hardware handling it has,
  * whether it is disabled etc etc.
+ *
+ * Note: on ARMv8 we can use normal bit manipulation functions to access
+ * the status field because struct irq_desc contains pointers, therefore
+ * the alignment of the struct is at least 8 bytes and status is the
+ * first field.
  */
 typedef struct irq_desc {
     unsigned int status;        /* IRQ status */