[PATCH v9 00/10] gic and vgic fixes and improvements

[PATCH v9 00/10] gic and vgic fixes and improvements

[Stefano Stabellini]

Hi all,
this patch series is a collection of three previously sent patch series
to fix bugs in the gic/vgic and implement irq migration on arm.
They have been grouped together under Ian's suggestion.

The last versions of the series, sent separately, were:
<alpine.DEB.2.02.1407031546180.11722@kaball.uk.xensource.com>
<alpine.DEB.2.02.1406131216260.13771@kaball.uk.xensource.com>
<alpine.DEB.2.02.1406241906040.19982@kaball.uk.xensource.com>


Changes in v9:
- move vgic_get_target_vcpu declaration to vgic.h;
- move _vgic_get_target_vcpu to vgic-v2.c and name it
vgic_v2_get_target_vcpu;
- introduce get_target_vcpu to vgic_ops;
- simplify the code to deal with inflight irqs while migrating irqs;
- move arch_move_irqs declaration to irq.h;
- use an arch hook to remove workaround to inject evtchn_irq on irq
enable;
- add explicit flags parameter to vgic_lock_rank and vgic_unlock_rank;
- do not rename IRQF_SHARED to IRQ_SHARED.


Stefano Stabellini (10):
      xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs
      xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier
      xen/arm: inflight irqs during migration
      xen/arm: support irq delivery to vcpu > 0
      xen/arm: physical irq follow virtual irq
      xen: introduce sched_move_irqs
      xen: remove workaround to inject evtchn_irq on irq enable
      xen/arm: take the rank lock before accessing ipriority
      xen: introduce bit access macros for the IRQ line status flags
      xen/arm: make accesses to desc->status flags atomic

 xen/arch/arm/gic-v2.c       |   19 ++++--
 xen/arch/arm/gic.c          |   21 +++++--
 xen/arch/arm/irq.c          |   44 +++++++------
 xen/arch/arm/vgic-v2.c      |  137 +++++++++++++++++++++++++++++-----------
 xen/arch/arm/vgic.c         |  147 +++++++++++++++++++++++++++++++++++--------
 xen/common/domain.c         |    1 +
 xen/common/schedule.c       |   12 +++-
 xen/include/asm-arm/event.h |    2 +
 xen/include/asm-arm/irq.h   |    3 +
 xen/include/asm-arm/vgic.h  |   16 ++++-
 xen/include/asm-x86/event.h |    2 +
 xen/include/asm-x86/irq.h   |    2 +
 xen/include/xen/irq.h       |   27 +++++---
 13 files changed, 326 insertions(+), 107 deletions(-)

[PATCH v9 01/10] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

vgic_enable_irqs should enable irq delivery to the vcpu specified by
GICD_ITARGETSR, rather than the vcpu that wrote to GICD_ISENABLER.
Similarly vgic_disable_irqs should use the target vcpu specified by
itarget to disable irqs.

itargets can be set to a mask but vgic_get_target_vcpu always returns
the lower vcpu in the mask.

Correctly initialize itargets for SPIs.

Ignore bits in GICD_ITARGETSR corresponding to invalid vcpus.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---

Changes in v9:
- move vgic_get_target_vcpu declaration to vgic.h;
- move _vgic_get_target_vcpu to vgic-v2.c and name it
vgic_v2_get_target_vcpu;
- introduce get_target_vcpu to vgic_ops.

Changes in v8:
- rebase on ab78724fc5628318b172b4344f7280621a151e1b.

Changes in v7:
- add ASSERT to _vgic_get_target_vcpu;
- add comment to vgic_distr_mmio_write.

Changes in v6:
- add assert and bug_on;
- add in-code comments;
- move additional check on itargets writing from the following patch to
this patch;
- sizeof(itargets) instead of 8*sizeof(itargets[0]);
- remove the unneeded cast of &target for find_first_bit.

Changes in v5:
- improve in-code comments;
- use vgic_rank_irq;
- use bit masks to write-ignore GICD_ITARGETSR;
- introduce an version of vgic_get_target_vcpu that doesn't take the
rank lock;
- keep the rank lock while enabling/disabling irqs;
- use find_first_bit instead of find_next_bit;
- check for zero writes to GICD_ITARGETSR.

Changes in v4:
- remove assert that could allow a guest to crash Xen;
- add itargets validation to vgic_distr_mmio_write;
- export vgic_get_target_vcpu.

Changes in v3:
- add assert in get_target_vcpu;
- rename get_target_vcpu to vgic_get_target_vcpu.

Changes in v2:
- refactor the common code in get_target_vcpu;
- unify PPI and SPI paths;
- correctly initialize itargets for SPI;
- use byte_read.
---
 xen/arch/arm/vgic-v2.c     |   42 ++++++++++++++++++++++++++++++++++++++----
 xen/arch/arm/vgic.c        |   43 ++++++++++++++++++++++++++++++++++---------
 xen/include/asm-arm/vgic.h |    5 +++++
 3 files changed, 77 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 2102e43..63d4f65 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -298,12 +298,12 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         vgic_lock_rank(v, rank);
         tr = rank->ienable;
         rank->ienable |= *r;
-        vgic_unlock_rank(v, rank);
         /* The virtual irq is derived from register offset.
          * The register difference is word difference. So divide by 2(DABT_WORD)
          * to get Virtual irq number */
         vgic_enable_irqs(v, (*r) & (~tr),
                          (gicd_reg - GICD_ISENABLER) >> DABT_WORD);
+        vgic_unlock_rank(v, rank);
         return 1;
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -313,12 +313,12 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         vgic_lock_rank(v, rank);
         tr = rank->ienable;
         rank->ienable &= ~*r;
-        vgic_unlock_rank(v, rank);
         /* The virtual irq is derived from register offset.
          * The register difference is word difference. So divide by 2(DABT_WORD)
          * to get  Virtual irq number */
         vgic_disable_irqs(v, (*r) & tr,
                          (gicd_reg - GICD_ICENABLER) >> DABT_WORD);
+        vgic_unlock_rank(v, rank);
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -359,13 +359,29 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
         if ( rank == NULL) goto write_ignore;
+        /* 8-bit vcpu mask for this domain */
+        BUG_ON(v->domain->max_vcpus > 8);
+        tr = (1 << v->domain->max_vcpus) - 1;
+        if ( dabt.size == 2 )
+            tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
+        else
+            tr = (tr << (8 * (gicd_reg & 0x3)));
+        tr &= *r;
+        /* ignore zero writes */
+        if ( !tr )
+            goto write_ignore;
+        /* For word reads ignore writes where any single byte is zero */
+        if ( dabt.size == 2 &&
+            !((tr & 0xff) && (tr & (0xff << 8)) &&
+             (tr & (0xff << 16)) && (tr & (0xff << 24))))
+            goto write_ignore;
         vgic_lock_rank(v, rank);
         if ( dabt.size == DABT_WORD )
             rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
-                                          DABT_WORD)] = *r;
+                                          DABT_WORD)] = tr;
         else
             vgic_byte_write(&rank->itargets[REG_RANK_INDEX(8,
-                       gicd_reg - GICD_ITARGETSR, DABT_WORD)], *r, gicd_reg);
+                       gicd_reg - GICD_ITARGETSR, DABT_WORD)], tr, gicd_reg);
         vgic_unlock_rank(v, rank);
         return 1;
 
@@ -460,6 +476,23 @@ static const struct mmio_handler_ops vgic_v2_distr_mmio_handler = {
     .write_handler = vgic_v2_distr_mmio_write,
 };
 
+static struct vcpu *vgic_v2_get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+    unsigned long target;
+    struct vcpu *v_target;
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+    ASSERT(spin_is_locked(&rank->lock));
+
+    target = vgic_byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
+    /* 1-N SPI should be delivered as pending to all the vcpus in the
+     * mask, but here we just return the first vcpu for simplicity and
+     * because it would be too slow to do otherwise. */
+    target = find_first_bit(&target, 8);
+    ASSERT(target >= 0 && target < v->domain->max_vcpus);
+    v_target = v->domain->vcpu[target];
+    return v_target;
+}
+
 static int vgic_v2_vcpu_init(struct vcpu *v)
 {
     int i;
@@ -487,6 +520,7 @@ static int vgic_v2_domain_init(struct domain *d)
 static const struct vgic_ops vgic_v2_ops = {
     .vcpu_init   = vgic_v2_vcpu_init,
     .domain_init = vgic_v2_domain_init,
+    .get_target_vcpu = vgic_v2_get_target_vcpu,
 };
 
 int vgic_v2_init(struct domain *d)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 1948316..ebfec83 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -48,7 +48,7 @@ struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n,
         return NULL;
 }
 
-static struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
+struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
 {
     return vgic_rank_offset(v, 8, irq, DABT_WORD);
 }
@@ -96,7 +96,13 @@ int domain_vgic_init(struct domain *d)
         INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
     }
     for (i=0; i<DOMAIN_NR_RANKS(d); i++)
+    {
         spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
+        /* By default deliver to CPU0 */
+        memset(d->arch.vgic.shared_irqs[i].itargets,
+               0x1,
+               sizeof(d->arch.vgic.shared_irqs[i].itargets));
+    }
 
     d->arch.vgic.handler->domain_init(d);
 
@@ -146,19 +152,35 @@ int vcpu_vgic_free(struct vcpu *v)
     return 0;
 }
 
+/* takes the rank lock */
+struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+    struct domain *d = v->domain;
+    struct vcpu *v_target;
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+
+    vgic_lock_rank(v, rank);
+    v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+    vgic_unlock_rank(v, rank);
+    return v_target;
+}
+
 void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
+    struct domain *d = v->domain;
     const unsigned long mask = r;
     struct pending_irq *p;
     unsigned int irq;
     unsigned long flags;
     int i = 0;
+    struct vcpu *v_target;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
-        p = irq_to_pending(v, irq);
+        v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+        p = irq_to_pending(v_target, irq);
         clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        gic_remove_from_queues(v, irq);
+        gic_remove_from_queues(v_target, irq);
         if ( p->desc != NULL )
         {
             spin_lock_irqsave(&p->desc->lock, flags);
@@ -171,29 +193,32 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 
 void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
 {
+    struct domain *d = v->domain;
     const unsigned long mask = r;
     struct pending_irq *p;
     unsigned int irq;
     unsigned long flags;
     int i = 0;
+    struct vcpu *v_target;
 
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
-        p = irq_to_pending(v, irq);
+        v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
+        p = irq_to_pending(v_target, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
         /* We need to force the first injection of evtchn_irq because
          * evtchn_upcall_pending is already set by common code on vcpu
          * creation. */
-        if ( irq == v->domain->arch.evtchn_irq &&
+        if ( irq == v_target->domain->arch.evtchn_irq &&
              vcpu_info(current, evtchn_upcall_pending) &&
              list_empty(&p->inflight) )
-            vgic_vcpu_inject_irq(v, irq);
+            vgic_vcpu_inject_irq(v_target, irq);
         else {
             unsigned long flags;
-            spin_lock_irqsave(&v->arch.vgic.lock, flags);
+            spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
             if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-                gic_raise_guest_irq(v, irq, p->priority);
-            spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+                gic_raise_guest_irq(v_target, irq, p->priority);
+            spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
         }
         if ( p->desc != NULL )
         {
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 19eed7e..81a3eef 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -91,6 +91,9 @@ struct vgic_ops {
     int (*vcpu_init)(struct vcpu *v);
     /* Domain specific initialization of vGIC */
     int (*domain_init)(struct domain *d);
+    /* Get the target vcpu for a given virq. The rank lock is already taken
+     * when calling this. */
+    struct vcpu *(*get_target_vcpu)(struct vcpu *v, unsigned int irq);
 };
 
 /* Number of ranks of interrupt registers for a domain */
@@ -151,10 +154,12 @@ enum gic_sgi_mode;
 extern int domain_vgic_init(struct domain *d);
 extern void domain_vgic_free(struct domain *d);
 extern int vcpu_vgic_init(struct vcpu *v);
+extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
 extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, int s);
+extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
 extern void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n);
 extern void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n);
 extern void register_vgic_ops(struct domain *d, const struct vgic_ops *ops);
-- 
1.7.10.4

[PATCH v9 02/10] xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier

[Stefano Stabellini]

It makes the code cleaner, especially with the following patches.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/vgic.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index ebfec83..8255e96 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -321,13 +321,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
-    if ( !list_empty(&n->inflight) )
-    {
-        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
-        gic_raise_inflight_irq(v, irq);
-        goto out;
-    }
-
     /* vcpu offline */
     if ( test_bit(_VPF_down, &v->pause_flags) )
     {
@@ -335,10 +328,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
         return;
     }
 
+    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
+
+    if ( !list_empty(&n->inflight) )
+    {
+        gic_raise_inflight_irq(v, irq);
+        goto out;
+    }
+ 
     priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, irq, DABT_WORD)], 0, irq & 0x3);
 
     n->irq = irq;
-    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
     n->priority = priority;
 
     /* the irq is enabled */
-- 
1.7.10.4

Re: [PATCH v9 02/10] xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier

[Julien Grall]

Hi Stefano,

On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> It makes the code cleaner, especially with the following patches.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Julien Grall <julien.grall@linaro.org>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/vgic.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index ebfec83..8255e96 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -321,13 +321,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>  
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>  
> -    if ( !list_empty(&n->inflight) )
> -    {
> -        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> -        gic_raise_inflight_irq(v, irq);
> -        goto out;
> -    }
> -
>      /* vcpu offline */
>      if ( test_bit(_VPF_down, &v->pause_flags) )
>      {
> @@ -335,10 +328,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>          return;
>      }
>  
> +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> +
> +    if ( !list_empty(&n->inflight) )
> +    {
> +        gic_raise_inflight_irq(v, irq);
> +        goto out;
> +    }
> + 

NIT: The line above contains a space. It should be empty.

Regards,

-- 
Julien Grall

Re: [PATCH v9 02/10] xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier

[Stefano Stabellini]

On Mon, 28 Jul 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> > It makes the code cleaner, especially with the following patches.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Acked-by: Julien Grall <julien.grall@linaro.org>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/vgic.c |   16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index ebfec83..8255e96 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -321,13 +321,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
> >  
> >      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >  
> > -    if ( !list_empty(&n->inflight) )
> > -    {
> > -        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> > -        gic_raise_inflight_irq(v, irq);
> > -        goto out;
> > -    }
> > -
> >      /* vcpu offline */
> >      if ( test_bit(_VPF_down, &v->pause_flags) )
> >      {
> > @@ -335,10 +328,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
> >          return;
> >      }
> >  
> > +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> > +
> > +    if ( !list_empty(&n->inflight) )
> > +    {
> > +        gic_raise_inflight_irq(v, irq);
> > +        goto out;
> > +    }
> > + 
> 
> NIT: The line above contains a space. It should be empty.

fixed

[PATCH v9 03/10] xen/arm: inflight irqs during migration

[Stefano Stabellini]

We need to take special care when migrating irqs that are already
inflight from one vcpu to another. See "The effect of changes to an
GICD_ITARGETSR", part of chapter 4.3.12 of the ARM Generic Interrupt
Controller Architecture Specification.

The main issue from the Xen point of view is that the lr_pending and
inflight lists are per-vcpu. The lock we take to protect them is also
per-vcpu.

In order to avoid issues, if the irq is still lr_pending, we can
immediately move it to the new vcpu for injection.

Otherwise if it is in a GICH_LR register, set a new flag
GIC_IRQ_GUEST_MIGRATING.  If GIC_IRQ_GUEST_MIGRATING is set we'll change
the affinity of the physical irq when clearing the LR (in a following
commit). Therefore if the irq is inflight in an LR when the guest writes
to itarget, we wait until the LR is cleared before changing physical
irq affinity. This guarantees that the old vcpu is going to be
interrupted and clear the LR, either because it has been descheduled
after EOIing the interrupt or because it is interrupted by the second
physical irq coming.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v9:
- simplify the code: rely on "physical irq follow virtual irq" to ensure
that physical irqs are injected into the right pcpu.

Changes in v8:
- rebase on ab78724fc5628318b172b4344f7280621a151e1b;
- rename target to new_target to avoid conflicts.

Changes in v7:
- move the _VPF_down check before setting GIC_IRQ_GUEST_QUEUED;
- fix comments;
- rename trl to target;
- introduce vcpu_migrate_from;
- do not kick new vcpu on MIGRATING, kick the old vcpu instead;
- separate moving GIC_IRQ_GUEST_QUEUED earlier into a different patch.

Changes in v6:
- remove unnecessary casts to (const unsigned long *) to the arguments
of find_first_bit and find_next_bit;
- instroduce a corresponding smb_rmb call;
- deal with migrating an irq that is inflight and still lr_pending;
- replace the dsb with smb_wmb and smb_rmb, use them to ensure the order
of accesses to GIC_IRQ_GUEST_QUEUED and GIC_IRQ_GUEST_MIGRATING;

Changes in v5:
- pass unsigned long to find_next_bit for alignment on aarch64;
- call vgic_get_target_vcpu instead of gic_add_to_lr_pending to add the
irq in the right inflight queue;
- add barrier and bit tests to make sure that vgic_migrate_irq and
gic_update_one_lr can run simultaneously on different cpus without
issues;
- rework the loop to identify the new vcpu when ITARGETSR is written;
- use find_first_bit instead of find_next_bit.
---
 xen/arch/arm/gic.c         |    8 ++++++--
 xen/arch/arm/vgic-v2.c     |   43 ++++++++++++++++++++++++++++++++++---------
 xen/arch/arm/vgic.c        |   37 +++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vgic.h |    6 ++++++
 4 files changed, 83 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 256d9cf..3e75fc5 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -33,6 +33,7 @@
 #include <asm/device.h>
 #include <asm/io.h>
 #include <asm/gic.h>
+#include <asm/vgic.h>
 
 static void gic_restore_pending_irqs(struct vcpu *v);
 
@@ -375,10 +376,13 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         p->lr = GIC_INVALID_LR;
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
-             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
+             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
             gic_raise_guest_irq(v, irq, p->priority);
-        else
+        else {
             list_del_init(&p->inflight);
+            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 63d4f65..f9e357c 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -356,34 +356,59 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         goto write_ignore;
 
     case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
+    {
+        /* unsigned long needed for find_next_bit */
+        unsigned long target;
+        int i;
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
         if ( rank == NULL) goto write_ignore;
         /* 8-bit vcpu mask for this domain */
         BUG_ON(v->domain->max_vcpus > 8);
-        tr = (1 << v->domain->max_vcpus) - 1;
+        target = (1 << v->domain->max_vcpus) - 1;
         if ( dabt.size == 2 )
-            tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
+            target = target | (target << 8) | (target << 16) | (target << 24);
         else
-            tr = (tr << (8 * (gicd_reg & 0x3)));
-        tr &= *r;
+            target = (target << (8 * (gicd_reg & 0x3)));
+        target &= *r;
         /* ignore zero writes */
-        if ( !tr )
+        if ( !target )
             goto write_ignore;
         /* For word reads ignore writes where any single byte is zero */
         if ( dabt.size == 2 &&
-            !((tr & 0xff) && (tr & (0xff << 8)) &&
-             (tr & (0xff << 16)) && (tr & (0xff << 24))))
+            !((target & 0xff) && (target & (0xff << 8)) &&
+             (target & (0xff << 16)) && (target & (0xff << 24))))
             goto write_ignore;
         vgic_lock_rank(v, rank);
+        i = 0;
+        while ( (i = find_next_bit(&target, 32, i)) < 32 )
+        {
+            unsigned int irq, new_target, old_target;
+            unsigned long old_target_mask;
+            struct vcpu *v_target, *v_old;
+
+            new_target = i % 8;
+            old_target_mask = vgic_byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR, DABT_WORD)], 0, i/8);
+            old_target = find_first_bit(&old_target_mask, 8);
+
+            if ( new_target != old_target )
+            {
+                irq = gicd_reg + (i / 8);
+                v_target = v->domain->vcpu[new_target];
+                v_old = v->domain->vcpu[old_target];
+                vgic_migrate_irq(v_old, v_target, irq);
+            }
+            i += 8 - new_target;
+        }
         if ( dabt.size == DABT_WORD )
             rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
-                                          DABT_WORD)] = tr;
+                                          DABT_WORD)] = target;
         else
             vgic_byte_write(&rank->itargets[REG_RANK_INDEX(8,
-                       gicd_reg - GICD_ITARGETSR, DABT_WORD)], tr, gicd_reg);
+                       gicd_reg - GICD_ITARGETSR, DABT_WORD)], target, gicd_reg);
         vgic_unlock_rank(v, rank);
         return 1;
+    }
 
     case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8255e96..5f45cd2 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -165,6 +165,43 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
     return v_target;
 }
 
+void 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;
+
+    /* migration already in progress, no need to do anything */
+    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+        return;
+
+    spin_lock_irqsave(&old->arch.vgic.lock, flags);
+
+    if ( list_empty(&p->inflight) )
+    {
+        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+        return;
+    }
+    /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
+    if ( !list_empty(&p->lr_queue) )
+    {
+        list_del_init(&p->lr_queue);
+        list_del_init(&p->inflight);
+        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+        vgic_vcpu_inject_irq(new, irq);
+        return;
+    }
+    /* if the IRQ is in a GICH_LR register, set GIC_IRQ_GUEST_MIGRATING
+     * and wait for the EOI */
+    if ( !list_empty(&p->inflight) )
+        set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+
+    spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+}
+
 void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     struct domain *d = v->domain;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 81a3eef..434a625 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -55,11 +55,16 @@ struct pending_irq
      * GIC_IRQ_GUEST_ENABLED: the guest IRQ is enabled at the VGICD
      * level (GICD_ICENABLER/GICD_ISENABLER).
      *
+     * GIC_IRQ_GUEST_MIGRATING: the irq is being migrated to a different
+     * vcpu while it is still inflight and on an GICH_LR register on the
+     * old vcpu.
+     *
      */
 #define GIC_IRQ_GUEST_QUEUED   0
 #define GIC_IRQ_GUEST_ACTIVE   1
 #define GIC_IRQ_GUEST_VISIBLE  2
 #define GIC_IRQ_GUEST_ENABLED  3
+#define GIC_IRQ_GUEST_MIGRATING   4
     unsigned long status;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
     int irq;
@@ -169,6 +174,7 @@ extern int vcpu_vgic_free(struct vcpu *v);
 extern int vgic_to_sgi(struct vcpu *v, register_t sgir,
                        enum gic_sgi_mode irqmode, int virq,
                        unsigned long vcpu_mask);
+extern void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq);
 #endif /* __ASM_ARM_VGIC_H__ */
 
 /*
-- 
1.7.10.4

[PATCH v9 04/10] xen/arm: support irq delivery to vcpu > 0

[Stefano Stabellini]

Use vgic_get_target_vcpu to retrieve the target vcpu from do_IRQ.
Remove in-code comments about missing implementation of SGI delivery to
vcpus other than 0.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---

Changes in v7:
- improve in-code comment;
- use gic_number_lines in assert.

Changes in v6:
- add in-code comments;
- assert that the guest irq is an SPI.

Changes in v4:
- the mask in gic_route_irq_to_guest is a physical cpu mask, treat it as
such;
- export vgic_get_target_vcpu in a previous patch.
---
 xen/arch/arm/gic.c         |    3 ++-
 xen/arch/arm/irq.c         |    5 +++--
 xen/arch/arm/vgic.c        |   11 +++++++++++
 xen/include/asm-arm/vgic.h |    1 +
 4 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3e75fc5..f5c7c91 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -137,7 +137,8 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
 
     gic_set_irq_properties(desc, cpumask_of(smp_processor_id()), GIC_PRI_IRQ);
 
-    /* TODO: do not assume delivery to vcpu0 */
+    /* Use vcpu0 to retrieve the pending_irq struct. Given that we only
+     * route SPIs to guests, it doesn't make any difference. */
     p = irq_to_pending(d->vcpu[0], desc->irq);
     p->desc = desc;
 }
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 3a8acbf..49ca467 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -198,8 +198,9 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         desc->status |= IRQ_INPROGRESS;
         desc->arch.eoi_cpu = smp_processor_id();
 
-        /* XXX: inject irq into all guest vcpus */
-        vgic_vcpu_inject_irq(d->vcpu[0], irq);
+        /* the irq cannot be a PPI, we only support delivery of SPIs to
+         * guests */
+        vgic_vcpu_inject_spi(d, irq);
         goto out_no_end;
     }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5f45cd2..02e5985 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -400,6 +400,17 @@ out:
         smp_send_event_check_mask(cpumask_of(v->processor));
 }
 
+void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq)
+{
+    struct vcpu *v;
+
+    /* the IRQ needs to be an SPI */
+    ASSERT(irq >= 32 && irq <= gic_number_lines());
+
+    v = vgic_get_target_vcpu(d->vcpu[0], irq);
+    vgic_vcpu_inject_irq(v, irq);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 434a625..9b1db04 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -161,6 +161,7 @@ extern void domain_vgic_free(struct domain *d);
 extern int vcpu_vgic_init(struct vcpu *v);
 extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
 extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq);
+extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, int s);
-- 
1.7.10.4

Re: [PATCH v9 04/10] xen/arm: support irq delivery to vcpu > 0

[Julien Grall]

Hi Stefano,

On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5f45cd2..02e5985 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -400,6 +400,17 @@ out:
>          smp_send_event_check_mask(cpumask_of(v->processor));
>  }
>  
> +void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq)

I know I've already acked this patch... I was looking to the code, and I
found the name of this function odd.

Actually we are injecting the spis to the domain d, not a specific VCPU.
The correct name would be vgic_domain_inject_spi.

Anyway, I'm perfectly fine to send a follow-up later.

Regards,

-- 
Julien Grall

[PATCH v9 05/10] xen/arm: physical irq follow virtual irq

[Stefano Stabellini]

Migrate physical irqs to the same physical cpu that is running the vcpu
expected to receive the irqs. That is done when enabling irqs, when the
guest writes to GICD_ITARGETSR and when Xen migrates a vcpu to a
different pcpu.

In case of virq migration, if the virq is inflight and in a GICH_LR
register already, delay migrating the corresponding physical irq until
the virq is EOIed by the guest and the MIGRATING flag has been cleared.
This way we make sure that the pcpu running the old vcpu gets
interrupted with a new irq of the same kind, clearing the GICH_LR sooner.

Introduce a new arch specific function, arch_move_irqs, that is empty on
x86 and implements the vgic irq migration code on ARM.
arch_move_irqs is going to be called by from sched.c.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>

---

Changes in v9:
- move arch_move_irqs declaration to irq.h.

Changes in v7:
- remove checks at the top of gic_irq_set_affinity, add assert instead;
- move irq_set_affinity to irq.c;
- delay setting the affinity of the physical irq when the virq is
MIGRATING until the virq is EOIed by the guest;
- do not set the affinity of MIGRATING irqs from arch_move_irqs.

Changes in v6:
- use vgic_get_target_vcpu instead of _vgic_get_target_vcpu in
arch_move_irqs.

Changes in v5:
- prettify vgic_move_irqs;
- rename vgic_move_irqs to arch_move_irqs;
- introduce helper function irq_set_affinity.
---
 xen/arch/arm/gic-v2.c     |   15 +++++++++++++--
 xen/arch/arm/gic.c        |    6 +++++-
 xen/arch/arm/irq.c        |    6 ++++++
 xen/arch/arm/vgic.c       |   21 +++++++++++++++++++++
 xen/include/asm-arm/irq.h |    3 +++
 xen/include/asm-x86/irq.h |    2 ++
 6 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 1305542..da60a41 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -569,9 +569,20 @@ static void gicv2_guest_irq_end(struct irq_desc *desc)
     /* Deactivation happens in maintenance interrupt / via GICV */
 }
 
-static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
+static void gicv2_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
 {
-    BUG();
+    unsigned int mask;
+
+    ASSERT(!cpumask_empty(cpu_mask));
+
+    spin_lock(&gicv2.lock);
+
+    mask = gicv2_cpu_mask(cpu_mask);
+
+    /* Set target CPU mask (RAZ/WI on uniprocessor) */
+    writeb_gicd(mask, GICD_ITARGETSR + desc->irq);
+
+    spin_unlock(&gicv2.lock);
 }
 
 /* XXX different for level vs edge */
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index f5c7c91..2aa9500 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -382,7 +382,11 @@ 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);
-            clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+            if ( test_and_clear_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));
+            }
         }
     }
 }
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 49ca467..7150c7a 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -134,6 +134,12 @@ static inline struct domain *irq_get_domain(struct irq_desc *desc)
     return desc->action->dev_id;
 }
 
+void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
+{
+    if ( desc != NULL )
+        desc->handler->set_affinity(desc, cpu_mask);
+}
+
 int request_irq(unsigned int irq, unsigned int irqflags,
                 void (*handler)(int, void *, struct cpu_user_regs *),
                 const char *devname, void *dev_id)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 02e5985..299ae7e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -182,6 +182,7 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
 
     if ( list_empty(&p->inflight) )
     {
+        irq_set_affinity(p->desc, cpumask_of(new->processor));
         spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
         return;
     }
@@ -190,6 +191,7 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     {
         list_del_init(&p->lr_queue);
         list_del_init(&p->inflight);
+        irq_set_affinity(p->desc, cpumask_of(new->processor));
         spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
         vgic_vcpu_inject_irq(new, irq);
         return;
@@ -202,6 +204,24 @@ void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
 }
 
+void arch_move_irqs(struct vcpu *v)
+{
+    const cpumask_t *cpu_mask = cpumask_of(v->processor);
+    struct domain *d = v->domain;
+    struct pending_irq *p;
+    struct vcpu *v_target;
+    int i;
+
+    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
+    {
+        v_target = vgic_get_target_vcpu(v, i);
+        p = irq_to_pending(v_target, i);
+
+        if ( v_target == v && !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
+            irq_set_affinity(p->desc, cpu_mask);
+    }
+}
+
 void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     struct domain *d = v->domain;
@@ -259,6 +279,7 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         }
         if ( p->desc != NULL )
         {
+            irq_set_affinity(p->desc, cpumask_of(v_target->processor));
             spin_lock_irqsave(&p->desc->lock, flags);
             p->desc->handler->enable(p->desc);
             spin_unlock_irqrestore(&p->desc->lock, flags);
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index e567f71..e877334 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -42,12 +42,15 @@ void init_secondary_IRQ(void);
 
 int route_irq_to_guest(struct domain *d, unsigned int irq,
                        const char *devname);
+void arch_move_irqs(struct vcpu *v);
 
 /* Set IRQ type for an SPI */
 int irq_set_spi_type(unsigned int spi, unsigned int type);
 
 int platform_get_irq(const struct dt_device_node *device, int index);
 
+void irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
diff --git a/xen/include/asm-x86/irq.h b/xen/include/asm-x86/irq.h
index 9066d38..d3c55f3 100644
--- a/xen/include/asm-x86/irq.h
+++ b/xen/include/asm-x86/irq.h
@@ -197,4 +197,6 @@ void cleanup_domain_irq_mapping(struct domain *);
 
 bool_t cpu_has_pending_apic_eoi(void);
 
+static inline void arch_move_irqs(struct vcpu *v) { }
+
 #endif /* _ASM_HW_IRQ_H */
-- 
1.7.10.4

Re: [PATCH v9 05/10] xen/arm: physical irq follow virtual irq

[Julien Grall]

Hi stefano,

On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> +void arch_move_irqs(struct vcpu *v)
> +{
> +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
> +    struct domain *d = v->domain;
> +    struct pending_irq *p;
> +    struct vcpu *v_target;
> +    int i;
> +
> +    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )

Sorry, I didn't spot this error until now.

For the VGIC nr_lines contains the number of *SPIs* rather on the GIC
structure it's the number of IRQs... the name is very confusing. I have
a patch to rename nr_lines into nr_spis, along with adding a macro
vgic_number_lines.

I plan to send it which my device passthrough patch series. As the patch
may help you. It may be better if you carry the patch.

Regards,

-- 
Julien Grall

Re: [PATCH v9 05/10] xen/arm: physical irq follow virtual irq

[Stefano Stabellini]

On Mon, 28 Jul 2014, Julien Grall wrote:
> Hi stefano,
> 
> On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> > +void arch_move_irqs(struct vcpu *v)
> > +{
> > +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
> > +    struct domain *d = v->domain;
> > +    struct pending_irq *p;
> > +    struct vcpu *v_target;
> > +    int i;
> > +
> > +    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
> 
> Sorry, I didn't spot this error until now.
> 
> For the VGIC nr_lines contains the number of *SPIs* rather on the GIC
> structure it's the number of IRQs... the name is very confusing. I have
> a patch to rename nr_lines into nr_spis, along with adding a macro
> vgic_number_lines.

I couldn't parse this sentence.
I guess you are saying that vgic.nr_lines doesn't represent the number
of spis?


> I plan to send it which my device passthrough patch series. As the patch
> may help you. It may be better if you carry the patch.

Please append it here so I can have a look.

Re: [PATCH v9 05/10] xen/arm: physical irq follow virtual irq

[Julien Grall]

On 07/28/2014 05:20 PM, Stefano Stabellini wrote:
> On Mon, 28 Jul 2014, Julien Grall wrote:
>> Hi stefano,
>>
>> On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
>>> +void arch_move_irqs(struct vcpu *v)
>>> +{
>>> +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
>>> +    struct domain *d = v->domain;
>>> +    struct pending_irq *p;
>>> +    struct vcpu *v_target;
>>> +    int i;
>>> +
>>> +    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
>>
>> Sorry, I didn't spot this error until now.
>>
>> For the VGIC nr_lines contains the number of *SPIs* rather on the GIC
>> structure it's the number of IRQs... the name is very confusing. I have
>> a patch to rename nr_lines into nr_spis, along with adding a macro
>> vgic_number_lines.
> 
> I couldn't parse this sentence.

Sorry it was not very clear.

> I guess you are saying that vgic.nr_lines doesn't represent the number
> of spis?

Yes. In the VGIC structure nr_lines = number of SPIs.

On GIC structure nr_lines = number of IRQs.

>> I plan to send it which my device passthrough patch series. As the patch
>> may help you. It may be better if you carry the patch.
> 
> Please append it here so I can have a look.

commit 534bdbbbd65b10f2780898bda2db5cdfc892dc34
Author: Julien Grall <julien.grall@linaro.org>
Date:   Fri Jul 18 12:25:18 2014 +0100

    xen/arm: vgic: Rename nr_lines into nr_spis
    
    The field nr_lines in the arch_domain vgic structure contains the number of
    SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
    code, where it means the number of IRQs. This can lead to coding error.
    
    Also introduce vgic_nr_lines to get the number of IRQ handled by the emulated
    GIC.
    
    Signed-off-by: Julien Grall <julien.grall@linaro.org>

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 19b2167..ee4f6ca 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -432,8 +432,6 @@ static int gicv2v_setup(struct domain *d)
         d->arch.vgic.cbase = GUEST_GICC_BASE;
     }
 
-    d->arch.vgic.nr_lines = 0;
-
     /*
      * Map the gic virtual cpu interface in the gic cpu interface
      * region of the guest.
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index ae31dbf..fbda349 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -54,7 +54,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         /* No secure world support for guests. */
         vgic_lock(v);
         *r = ( (v->domain->max_vcpus << 5) & GICD_TYPE_CPUS )
-            |( ((v->domain->arch.vgic.nr_lines / 32)) & GICD_TYPE_LINES );
+            |( ((v->domain->arch.vgic.nr_spis / 32)) & GICD_TYPE_LINES );
         vgic_unlock(v);
         return 1;
     case GICD_IIDR:
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 9f7ed4d..3204131 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -59,13 +59,10 @@ int domain_vgic_init(struct domain *d)

 
     d->arch.vgic.ctlr = 0;
 
-    /* Currently nr_lines in vgic and gic doesn't have the same meanings
-     * Here nr_lines = number of SPIs
-     */
     if ( is_hardware_domain(d) )
-        d->arch.vgic.nr_lines = gic_number_lines() - 32;
+        d->arch.vgic.nr_spis = gic_number_lines() - 32;
     else
-        d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
+        d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */
 
     switch ( gic_hw_version() )
     {
@@ -83,11 +80,11 @@ int domain_vgic_init(struct domain *d)
         return -ENOMEM;
 
     d->arch.vgic.pending_irqs =
-        xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
+        xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
     if ( d->arch.vgic.pending_irqs == NULL )
         return -ENOMEM;
 
-    for (i=0; i<d->arch.vgic.nr_lines; i++)
+    for (i=0; i<d->arch.vgic.nr_spis; i++)
     {
         INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
         INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
@@ -230,7 +227,7 @@ void arch_move_irqs(struct vcpu *v)
     struct vcpu *v_target;
     int i;
 
-    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
+    for ( i = 32; i < d->arch.vgic.nr_spis; i++ )
     {
         v_target = vgic_get_target_vcpu(v, i);
         p = irq_to_pending(v_target, i);
@@ -354,7 +351,7 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
 struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
 {
     struct pending_irq *n;
-    /* Pending irqs allocation strategy: the first vgic.nr_lines irqs
+    /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
      * are used for SPIs; the rests are used for per cpu irqs */
     if ( irq < 32 )
         n = &v->arch.vgic.pending_irqs[irq];
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 32d0554..5719fe5 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -89,7 +89,7 @@ struct arch_domain
          */
         spinlock_t lock;
         int ctlr;
-        int nr_lines; /* Number of SPIs */
+        int nr_spis; /* Number of SPIs */
         struct vgic_irq_rank *shared_irqs;
         /*
          * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 5d6a8ad..b94de8c 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -101,7 +101,7 @@ struct vgic_ops {
 };
 
 /* Number of ranks of interrupt registers for a domain */
-#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
+#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
 
 #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
 #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
@@ -155,6 +155,8 @@ enum gic_sgi_mode;
  */
 #define REG_RANK_INDEX(b, n, s) ((((n) >> s) & ((b)-1)) % 32)
 
+#define vgic_num_irqs(d)        ((d)->arch.vgic.nr_spis + 32)
+
 extern int domain_vgic_init(struct domain *d);
 extern void domain_vgic_free(struct domain *d);
 extern int vcpu_vgic_init(struct vcpu *v);

Regards,


-- 
Julien Grall

Re: [PATCH v9 05/10] xen/arm: physical irq follow virtual irq

[Stefano Stabellini]

On Mon, 28 Jul 2014, Julien Grall wrote:
> On 07/28/2014 05:20 PM, Stefano Stabellini wrote:
> > On Mon, 28 Jul 2014, Julien Grall wrote:
> >> Hi stefano,
> >>
> >> On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> >>> +void arch_move_irqs(struct vcpu *v)
> >>> +{
> >>> +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
> >>> +    struct domain *d = v->domain;
> >>> +    struct pending_irq *p;
> >>> +    struct vcpu *v_target;
> >>> +    int i;
> >>> +
> >>> +    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
> >>
> >> Sorry, I didn't spot this error until now.
> >>
> >> For the VGIC nr_lines contains the number of *SPIs* rather on the GIC
> >> structure it's the number of IRQs... the name is very confusing. I have
> >> a patch to rename nr_lines into nr_spis, along with adding a macro
> >> vgic_number_lines.
> > 
> > I couldn't parse this sentence.
> 
> Sorry it was not very clear.
> 
> > I guess you are saying that vgic.nr_lines doesn't represent the number
> > of spis?
> 
> Yes. In the VGIC structure nr_lines = number of SPIs.
> 
> On GIC structure nr_lines = number of IRQs.

Wait a second. If in the vgic struct, nr_lines = number of SPIs, then
the code in this patch is correct as it is, isn't it?



> >> I plan to send it which my device passthrough patch series. As the patch
> >> may help you. It may be better if you carry the patch.
> > 
> > Please append it here so I can have a look.
> 
> commit 534bdbbbd65b10f2780898bda2db5cdfc892dc34
> Author: Julien Grall <julien.grall@linaro.org>
> Date:   Fri Jul 18 12:25:18 2014 +0100
> 
>     xen/arm: vgic: Rename nr_lines into nr_spis
>     
>     The field nr_lines in the arch_domain vgic structure contains the number of
>     SPIs for the emulated GIC. Using the nr_lines make confusion with the GIC
>     code, where it means the number of IRQs. This can lead to coding error.
>     
>     Also introduce vgic_nr_lines to get the number of IRQ handled by the emulated
>     GIC.
>     
>     Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 19b2167..ee4f6ca 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -432,8 +432,6 @@ static int gicv2v_setup(struct domain *d)
>          d->arch.vgic.cbase = GUEST_GICC_BASE;
>      }
>  
> -    d->arch.vgic.nr_lines = 0;
> -
>      /*
>       * Map the gic virtual cpu interface in the gic cpu interface
>       * region of the guest.
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index ae31dbf..fbda349 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -54,7 +54,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
>          /* No secure world support for guests. */
>          vgic_lock(v);
>          *r = ( (v->domain->max_vcpus << 5) & GICD_TYPE_CPUS )
> -            |( ((v->domain->arch.vgic.nr_lines / 32)) & GICD_TYPE_LINES );
> +            |( ((v->domain->arch.vgic.nr_spis / 32)) & GICD_TYPE_LINES );
>          vgic_unlock(v);
>          return 1;
>      case GICD_IIDR:
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 9f7ed4d..3204131 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -59,13 +59,10 @@ int domain_vgic_init(struct domain *d)
> 
>  
>      d->arch.vgic.ctlr = 0;
>  
> -    /* Currently nr_lines in vgic and gic doesn't have the same meanings
> -     * Here nr_lines = number of SPIs
> -     */
>      if ( is_hardware_domain(d) )
> -        d->arch.vgic.nr_lines = gic_number_lines() - 32;
> +        d->arch.vgic.nr_spis = gic_number_lines() - 32;
>      else
> -        d->arch.vgic.nr_lines = 0; /* We don't need SPIs for the guest */
> +        d->arch.vgic.nr_spis = 0; /* We don't need SPIs for the guest */
>  
>      switch ( gic_hw_version() )
>      {
> @@ -83,11 +80,11 @@ int domain_vgic_init(struct domain *d)
>          return -ENOMEM;
>  
>      d->arch.vgic.pending_irqs =
> -        xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
> +        xzalloc_array(struct pending_irq, d->arch.vgic.nr_spis);
>      if ( d->arch.vgic.pending_irqs == NULL )
>          return -ENOMEM;
>  
> -    for (i=0; i<d->arch.vgic.nr_lines; i++)
> +    for (i=0; i<d->arch.vgic.nr_spis; i++)
>      {
>          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
>          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
> @@ -230,7 +227,7 @@ void arch_move_irqs(struct vcpu *v)
>      struct vcpu *v_target;
>      int i;
>  
> -    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
> +    for ( i = 32; i < d->arch.vgic.nr_spis; i++ )
>      {
>          v_target = vgic_get_target_vcpu(v, i);
>          p = irq_to_pending(v_target, i);
> @@ -354,7 +351,7 @@ int vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode, int
>  struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
>  {
>      struct pending_irq *n;
> -    /* Pending irqs allocation strategy: the first vgic.nr_lines irqs
> +    /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
>       * are used for SPIs; the rests are used for per cpu irqs */
>      if ( irq < 32 )
>          n = &v->arch.vgic.pending_irqs[irq];
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 32d0554..5719fe5 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -89,7 +89,7 @@ struct arch_domain
>           */
>          spinlock_t lock;
>          int ctlr;
> -        int nr_lines; /* Number of SPIs */
> +        int nr_spis; /* Number of SPIs */
>          struct vgic_irq_rank *shared_irqs;
>          /*
>           * SPIs are domain global, SGIs and PPIs are per-VCPU and stored in
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index 5d6a8ad..b94de8c 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -101,7 +101,7 @@ struct vgic_ops {
>  };
>  
>  /* Number of ranks of interrupt registers for a domain */
> -#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_lines+31)/32)
> +#define DOMAIN_NR_RANKS(d) (((d)->arch.vgic.nr_spis+31)/32)
>  
>  #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
>  #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
> @@ -155,6 +155,8 @@ enum gic_sgi_mode;
>   */
>  #define REG_RANK_INDEX(b, n, s) ((((n) >> s) & ((b)-1)) % 32)
>  
> +#define vgic_num_irqs(d)        ((d)->arch.vgic.nr_spis + 32)
> +
>  extern int domain_vgic_init(struct domain *d);
>  extern void domain_vgic_free(struct domain *d);
>  extern int vcpu_vgic_init(struct vcpu *v);
> 
> Regards,
> 
> 
> -- 
> Julien Grall
> 

Re: [PATCH v9 05/10] xen/arm: physical irq follow virtual irq

[Julien Grall]

Hi Stefano,

On 01/08/14 18:22, Stefano Stabellini wrote:
> On Mon, 28 Jul 2014, Julien Grall wrote:
>> On 07/28/2014 05:20 PM, Stefano Stabellini wrote:
>>> On Mon, 28 Jul 2014, Julien Grall wrote:
>>>> Hi stefano,
>>>>
>>>> On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
>>>>> +void arch_move_irqs(struct vcpu *v)
>>>>> +{
>>>>> +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
>>>>> +    struct domain *d = v->domain;
>>>>> +    struct pending_irq *p;
>>>>> +    struct vcpu *v_target;
>>>>> +    int i;
>>>>> +
>>>>> +    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
>>>>
>>>> Sorry, I didn't spot this error until now.
>>>>
>>>> For the VGIC nr_lines contains the number of *SPIs* rather on the GIC
>>>> structure it's the number of IRQs... the name is very confusing. I have
>>>> a patch to rename nr_lines into nr_spis, along with adding a macro
>>>> vgic_number_lines.
>>>
>>> I couldn't parse this sentence.
>>
>> Sorry it was not very clear.
>>
>>> I guess you are saying that vgic.nr_lines doesn't represent the number
>>> of spis?
>>
>> Yes. In the VGIC structure nr_lines = number of SPIs.
>>
>> On GIC structure nr_lines = number of IRQs.
>
> Wait a second. If in the vgic struct, nr_lines = number of SPIs, then
> the code in this patch is correct as it is, isn't it?

No because your loop starts at 32 and you check against vgic.nr_lines.

The loop should be:

for ( i = 32; i < (d->arch.vgic.nr_lines + 32); i++ )

Otherwise you will forget to migrate the last 32 SPIs.

Regards,

-- 
Julien Grall

Re: [PATCH v9 05/10] xen/arm: physical irq follow virtual irq

[Stefano Stabellini]

On Fri, 1 Aug 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 01/08/14 18:22, Stefano Stabellini wrote:
> > On Mon, 28 Jul 2014, Julien Grall wrote:
> > > On 07/28/2014 05:20 PM, Stefano Stabellini wrote:
> > > > On Mon, 28 Jul 2014, Julien Grall wrote:
> > > > > Hi stefano,
> > > > > 
> > > > > On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> > > > > > +void arch_move_irqs(struct vcpu *v)
> > > > > > +{
> > > > > > +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
> > > > > > +    struct domain *d = v->domain;
> > > > > > +    struct pending_irq *p;
> > > > > > +    struct vcpu *v_target;
> > > > > > +    int i;
> > > > > > +
> > > > > > +    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
> > > > > 
> > > > > Sorry, I didn't spot this error until now.
> > > > > 
> > > > > For the VGIC nr_lines contains the number of *SPIs* rather on the GIC
> > > > > structure it's the number of IRQs... the name is very confusing. I
> > > > > have
> > > > > a patch to rename nr_lines into nr_spis, along with adding a macro
> > > > > vgic_number_lines.
> > > > 
> > > > I couldn't parse this sentence.
> > > 
> > > Sorry it was not very clear.
> > > 
> > > > I guess you are saying that vgic.nr_lines doesn't represent the number
> > > > of spis?
> > > 
> > > Yes. In the VGIC structure nr_lines = number of SPIs.
> > > 
> > > On GIC structure nr_lines = number of IRQs.
> > 
> > Wait a second. If in the vgic struct, nr_lines = number of SPIs, then
> > the code in this patch is correct as it is, isn't it?
> 
> No because your loop starts at 32 and you check against vgic.nr_lines.
> 
> The loop should be:
> 
> for ( i = 32; i < (d->arch.vgic.nr_lines + 32); i++ )
> 
> Otherwise you will forget to migrate the last 32 SPIs.

Sorry, obviously I didn't think it through.

For simplicity I think it is best if I just make this change.

If you are going to base your series on this one, you might as well
cleanup this line too in your patch?

Re: [PATCH v9 05/10] xen/arm: physical irq follow virtual irq

[Julien Grall]

On 01/08/14 18:58, Stefano Stabellini wrote:
> On Fri, 1 Aug 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 01/08/14 18:22, Stefano Stabellini wrote:
>>> On Mon, 28 Jul 2014, Julien Grall wrote:
>>>> On 07/28/2014 05:20 PM, Stefano Stabellini wrote:
>>>>> On Mon, 28 Jul 2014, Julien Grall wrote:
>>>>>> Hi stefano,
>>>>>>
>>>>>> On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
>>>>>>> +void arch_move_irqs(struct vcpu *v)
>>>>>>> +{
>>>>>>> +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
>>>>>>> +    struct domain *d = v->domain;
>>>>>>> +    struct pending_irq *p;
>>>>>>> +    struct vcpu *v_target;
>>>>>>> +    int i;
>>>>>>> +
>>>>>>> +    for ( i = 32; i < d->arch.vgic.nr_lines; i++ )
>>>>>>
>>>>>> Sorry, I didn't spot this error until now.
>>>>>>
>>>>>> For the VGIC nr_lines contains the number of *SPIs* rather on the GIC
>>>>>> structure it's the number of IRQs... the name is very confusing. I
>>>>>> have
>>>>>> a patch to rename nr_lines into nr_spis, along with adding a macro
>>>>>> vgic_number_lines.
>>>>>
>>>>> I couldn't parse this sentence.
>>>>
>>>> Sorry it was not very clear.
>>>>
>>>>> I guess you are saying that vgic.nr_lines doesn't represent the number
>>>>> of spis?
>>>>
>>>> Yes. In the VGIC structure nr_lines = number of SPIs.
>>>>
>>>> On GIC structure nr_lines = number of IRQs.
>>>
>>> Wait a second. If in the vgic struct, nr_lines = number of SPIs, then
>>> the code in this patch is correct as it is, isn't it?
>>
>> No because your loop starts at 32 and you check against vgic.nr_lines.
>>
>> The loop should be:
>>
>> for ( i = 32; i < (d->arch.vgic.nr_lines + 32); i++ )
>>
>> Otherwise you will forget to migrate the last 32 SPIs.
>
> Sorry, obviously I didn't think it through.
>
> For simplicity I think it is best if I just make this change.
>
> If you are going to base your series on this one, you might as well
> cleanup this line too in your patch?

It's fine for me.

Regards,

-- 
Julien Grall

[PATCH v9 06/10] xen: introduce sched_move_irqs

[Stefano Stabellini]

Introduce sched_move_irqs: it calls arch_move_irqs and
evtchn_move_pirqs.
Replace calls to evtchn_move_pirqs with calls to sched_move_irqs.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by:  George Dunlap <george.dunlap@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
CC: jbeulich@suse.com
CC: tim@xen.org
CC: keir.xen@gmail.com
---
 xen/common/schedule.c |   12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index e9eb0bc..8c1561f 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -226,6 +226,12 @@ int sched_init_vcpu(struct vcpu *v, unsigned int processor)
     return 0;
 }
 
+static void sched_move_irqs(struct vcpu *v)
+{
+    arch_move_irqs(v);
+    evtchn_move_pirqs(v);
+}
+
 int sched_move_domain(struct domain *d, struct cpupool *c)
 {
     struct vcpu *v;
@@ -301,7 +307,7 @@ int sched_move_domain(struct domain *d, struct cpupool *c)
 
         v->sched_priv = vcpu_priv[v->vcpu_id];
         if ( !d->is_dying )
-            evtchn_move_pirqs(v);
+            sched_move_irqs(v);
 
         new_p = cpumask_cycle(new_p, c->cpu_valid);
 
@@ -528,7 +534,7 @@ static void vcpu_migrate(struct vcpu *v)
     spin_unlock_irqrestore(old_lock, flags);
 
     if ( old_cpu != new_cpu )
-        evtchn_move_pirqs(v);
+        sched_move_irqs(v);
 
     /* Wake on new CPU. */
     vcpu_wake(v);
@@ -1251,7 +1257,7 @@ static void schedule(void)
     stop_timer(&prev->periodic_timer);
 
     if ( next_slice.migrated )
-        evtchn_move_pirqs(next);
+        sched_move_irqs(next);
 
     vcpu_periodic_timer_work(next);
 
-- 
1.7.10.4

[PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Stefano Stabellini]

evtchn_upcall_pending is already set by common code at vcpu creation,
therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
Currently we do that from vgic_enable_irqs as a workaround.

Do this properly by introducing an appropriate arch specific hook:
arch_evtchn_inject. arch_evtchn_inject is called by map_vcpu_info to
inject the evtchn irq into the guest. On ARM is implemented by calling
vgic_vcpu_inject_irq, on x86 is unneeded.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: JBeulich@suse.com
---

Changes in v9:
- use an arch hook.

Changes in v2:
- coding style fix;
- add comment;
- return an error if arch_set_info_guest is called without VGCF_online.
---
 xen/arch/arm/vgic.c         |   23 +++++++++--------------
 xen/common/domain.c         |    1 +
 xen/include/asm-arm/event.h |    2 ++
 xen/include/asm-x86/event.h |    2 ++
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 299ae7e..474eebd 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -263,20 +263,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
         p = irq_to_pending(v_target, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        /* We need to force the first injection of evtchn_irq because
-         * evtchn_upcall_pending is already set by common code on vcpu
-         * creation. */
-        if ( irq == v_target->domain->arch.evtchn_irq &&
-             vcpu_info(current, evtchn_upcall_pending) &&
-             list_empty(&p->inflight) )
-            vgic_vcpu_inject_irq(v_target, irq);
-        else {
-            unsigned long flags;
-            spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
-            if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-                gic_raise_guest_irq(v_target, irq, p->priority);
-            spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
-        }
+        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+        if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+            gic_raise_guest_irq(v_target, irq, p->priority);
+        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
         if ( p->desc != NULL )
         {
             irq_set_affinity(p->desc, cpumask_of(v_target->processor));
@@ -432,6 +422,11 @@ void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq)
     vgic_vcpu_inject_irq(v, irq);
 }
 
+void arch_evtchn_inject(struct vcpu *v)
+{
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/common/domain.c b/xen/common/domain.c
index cd64aea..05d0049 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1058,6 +1058,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
     vcpu_info(v, evtchn_upcall_pending) = 1;
     for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
         set_bit(i, &vcpu_info(v, evtchn_pending_sel));
+    arch_evtchn_inject(v);
 
     return 0;
 }
diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
index 5330dfe..8c77427 100644
--- a/xen/include/asm-arm/event.h
+++ b/xen/include/asm-arm/event.h
@@ -56,6 +56,8 @@ static inline int arch_virq_is_global(int virq)
     return 1;
 }
 
+void arch_evtchn_inject(struct vcpu *v);
+
 #endif
 /*
  * Local variables:
diff --git a/xen/include/asm-x86/event.h b/xen/include/asm-x86/event.h
index a82062e..3c1a9d1 100644
--- a/xen/include/asm-x86/event.h
+++ b/xen/include/asm-x86/event.h
@@ -44,4 +44,6 @@ static inline int arch_virq_is_global(uint32_t virq)
     return 1;
 }
 
+static inline void arch_evtchn_inject(struct vcpu *v) { }
+
 #endif
-- 
1.7.10.4

Re: [PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Jan Beulich]

>>> On 24.07.14 at 19:33, <stefano.stabellini@eu.citrix.com> wrote:
> evtchn_upcall_pending is already set by common code at vcpu creation,
> therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
> Currently we do that from vgic_enable_irqs as a workaround.
> 
> Do this properly by introducing an appropriate arch specific hook:
> arch_evtchn_inject. arch_evtchn_inject is called by map_vcpu_info to
> inject the evtchn irq into the guest. On ARM is implemented by calling
> vgic_vcpu_inject_irq, on x86 is unneeded.

>From a mechanical pov this has my ack, but I still don't see why
ARM needs what x86 (even for HVM) appears to get along fine
without.

Jan

Re: [PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Stefano Stabellini]

On Fri, 25 Jul 2014, Jan Beulich wrote:
> >>> On 24.07.14 at 19:33, <stefano.stabellini@eu.citrix.com> wrote:
> > evtchn_upcall_pending is already set by common code at vcpu creation,
> > therefore on ARM we also need to call vgic_vcpu_inject_irq for it.
> > Currently we do that from vgic_enable_irqs as a workaround.
> > 
> > Do this properly by introducing an appropriate arch specific hook:
> > arch_evtchn_inject. arch_evtchn_inject is called by map_vcpu_info to
> > inject the evtchn irq into the guest. On ARM is implemented by calling
> > vgic_vcpu_inject_irq, on x86 is unneeded.
> 
> >From a mechanical pov this has my ack,
    
Thanks.


> but I still don't see why ARM needs what x86 (even for HVM) appears to
> get along fine without.

Good question.
x86 PV guests have in xen_irq_enable:

if (unlikely(vcpu->evtchn_upcall_pending))
        xen_force_evtchn_callback();

Also xen_irq_enable_direct calls check_events.

I suspect that PV on HVM guests that get events via gsi interrupts get
away without it because they only call VCPUOP_register_vcpu_info on
secondary cpus.

Re: [PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Jan Beulich]

>>> On 01.08.14 at 19:00, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 25 Jul 2014, Jan Beulich wrote:
>> but I still don't see why ARM needs what x86 (even for HVM) appears to
>> get along fine without.
> 
> Good question.
> x86 PV guests have in xen_irq_enable:
> 
> if (unlikely(vcpu->evtchn_upcall_pending))
>         xen_force_evtchn_callback();
> 
> Also xen_irq_enable_direct calls check_events.
> 
> I suspect that PV on HVM guests that get events via gsi interrupts get
> away without it because they only call VCPUOP_register_vcpu_info on
> secondary cpus.

But my pointer was specifically to pure HVM guests (among all the
other possible kinds)...

Jan

Re: [PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Stefano Stabellini]

On Mon, 4 Aug 2014, Jan Beulich wrote:
> >>> On 01.08.14 at 19:00, <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 25 Jul 2014, Jan Beulich wrote:
> >> but I still don't see why ARM needs what x86 (even for HVM) appears to
> >> get along fine without.
> > 
> > Good question.
> > x86 PV guests have in xen_irq_enable:
> > 
> > if (unlikely(vcpu->evtchn_upcall_pending))
> >         xen_force_evtchn_callback();
> > 
> > Also xen_irq_enable_direct calls check_events.
> > 
> > I suspect that PV on HVM guests that get events via gsi interrupts get
> > away without it because they only call VCPUOP_register_vcpu_info on
> > secondary cpus.
> 
> But my pointer was specifically to pure HVM guests (among all the
> other possible kinds)...

Pure HVM guests don't map the vcpu info struct so they don't have this
problem. After all they don't have event channels.
More advanced forms of PV on HVM guests have vector callbacks that need
no emulation at the vioapic/vlapic level.
So that leaves us with old style PV on HVM guests, that receive event
notifications via legacy interrupts, but only to the first vcpu. This is
the case I am talking about.

Am I missing anything?

Re: [PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Jan Beulich]

>>> On 04.08.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 4 Aug 2014, Jan Beulich wrote:
>> >>> On 01.08.14 at 19:00, <stefano.stabellini@eu.citrix.com> wrote:
>> > On Fri, 25 Jul 2014, Jan Beulich wrote:
>> >> but I still don't see why ARM needs what x86 (even for HVM) appears to
>> >> get along fine without.
>> > 
>> > Good question.
>> > x86 PV guests have in xen_irq_enable:
>> > 
>> > if (unlikely(vcpu->evtchn_upcall_pending))
>> >         xen_force_evtchn_callback();
>> > 
>> > Also xen_irq_enable_direct calls check_events.
>> > 
>> > I suspect that PV on HVM guests that get events via gsi interrupts get
>> > away without it because they only call VCPUOP_register_vcpu_info on
>> > secondary cpus.
>> 
>> But my pointer was specifically to pure HVM guests (among all the
>> other possible kinds)...
> 
> Pure HVM guests don't map the vcpu info struct so they don't have this
> problem. After all they don't have event channels.
> More advanced forms of PV on HVM guests have vector callbacks that need
> no emulation at the vioapic/vlapic level.
> So that leaves us with old style PV on HVM guests, that receive event
> notifications via legacy interrupts, but only to the first vcpu. This is
> the case I am talking about.
> 
> Am I missing anything?

No, you're right. So coming back to your suspicion above: Nothing
prevents a HVM guest to also call VCPUOP_register_vcpu_info on
the boot CPU (and in fact such an asymmetry would seem pretty
odd); old-style HVM guests with PV drivers (built from
unmodified_drivers/) don't call VCPUOP_register_vcpu_info at all.
But in the end if what you say is true there would be a case where
x86 is also broken, just that there doesn't appear to be a kernel
utilizing this case. Since especially for HVM guests we shouldn't be
making assumptions in the hypervisor on guest behavior, shouldn't
your patch at least try to address that case then at once?

Jan

Re: [PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Stefano Stabellini]

On Mon, 4 Aug 2014, Jan Beulich wrote:
> >>> On 04.08.14 at 12:02, <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 4 Aug 2014, Jan Beulich wrote:
> >> >>> On 01.08.14 at 19:00, <stefano.stabellini@eu.citrix.com> wrote:
> >> > On Fri, 25 Jul 2014, Jan Beulich wrote:
> >> >> but I still don't see why ARM needs what x86 (even for HVM) appears to
> >> >> get along fine without.
> >> > 
> >> > Good question.
> >> > x86 PV guests have in xen_irq_enable:
> >> > 
> >> > if (unlikely(vcpu->evtchn_upcall_pending))
> >> >         xen_force_evtchn_callback();
> >> > 
> >> > Also xen_irq_enable_direct calls check_events.
> >> > 
> >> > I suspect that PV on HVM guests that get events via gsi interrupts get
> >> > away without it because they only call VCPUOP_register_vcpu_info on
> >> > secondary cpus.
> >> 
> >> But my pointer was specifically to pure HVM guests (among all the
> >> other possible kinds)...
> > 
> > Pure HVM guests don't map the vcpu info struct so they don't have this
> > problem. After all they don't have event channels.
> > More advanced forms of PV on HVM guests have vector callbacks that need
> > no emulation at the vioapic/vlapic level.
> > So that leaves us with old style PV on HVM guests, that receive event
> > notifications via legacy interrupts, but only to the first vcpu. This is
> > the case I am talking about.
> > 
> > Am I missing anything?
> 
> No, you're right. So coming back to your suspicion above: Nothing
> prevents a HVM guest to also call VCPUOP_register_vcpu_info on
> the boot CPU (and in fact such an asymmetry would seem pretty
> odd); old-style HVM guests with PV drivers (built from
> unmodified_drivers/) don't call VCPUOP_register_vcpu_info at all.
> But in the end if what you say is true there would be a case where
> x86 is also broken, just that there doesn't appear to be a kernel
> utilizing this case. Since especially for HVM guests we shouldn't be
> making assumptions in the hypervisor on guest behavior, shouldn't
> your patch at least try to address that case then at once?

The most logical thing to do would be to implement arch_evtchn_inject on
x86 as:

void arch_evtchn_inject(struct vcpu *v)
{
    if ( has_hvm_container_vcpu(v) )
        hvm_assert_evtchn_irq(v);
}

however it is very difficult to test because:
- the !xen_have_vector_callback code path doesn't work properly on a
modern Linux kernel;
- going all the way back to 2.6.37, !xen_have_vector_callback works but
then calling xen_vcpu_setup on vcpu0 doesn't work anyway. I don't know
exactly why but I don't think that the reason has anything to do with
the problem we are discussing here. In fact simply calling on vcpu0 an
hypercall that only sets evtchn_upcall_pending and then calls
arch_evtchn_inject works as espected.

I know we are not just dealing with Linux guests, but given all this I
am not sure how useful would actually be to provide the implementation
of arch_evtchn_inject on x86.  What do you think?

Re: [PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Jan Beulich]

>>> On 04.08.14 at 22:29, <stefano.stabellini@eu.citrix.com> wrote:
> On Mon, 4 Aug 2014, Jan Beulich wrote:
>> No, you're right. So coming back to your suspicion above: Nothing
>> prevents a HVM guest to also call VCPUOP_register_vcpu_info on
>> the boot CPU (and in fact such an asymmetry would seem pretty
>> odd); old-style HVM guests with PV drivers (built from
>> unmodified_drivers/) don't call VCPUOP_register_vcpu_info at all.
>> But in the end if what you say is true there would be a case where
>> x86 is also broken, just that there doesn't appear to be a kernel
>> utilizing this case. Since especially for HVM guests we shouldn't be
>> making assumptions in the hypervisor on guest behavior, shouldn't
>> your patch at least try to address that case then at once?
> 
> The most logical thing to do would be to implement arch_evtchn_inject on
> x86 as:
> 
> void arch_evtchn_inject(struct vcpu *v)
> {
>     if ( has_hvm_container_vcpu(v) )
>         hvm_assert_evtchn_irq(v);
> }
> 
> however it is very difficult to test because:
> - the !xen_have_vector_callback code path doesn't work properly on a
> modern Linux kernel;
> - going all the way back to 2.6.37, !xen_have_vector_callback works but
> then calling xen_vcpu_setup on vcpu0 doesn't work anyway. I don't know
> exactly why but I don't think that the reason has anything to do with
> the problem we are discussing here. In fact simply calling on vcpu0 an
> hypercall that only sets evtchn_upcall_pending and then calls
> arch_evtchn_inject works as espected.
> 
> I know we are not just dealing with Linux guests, but given all this I
> am not sure how useful would actually be to provide the implementation
> of arch_evtchn_inject on x86.  What do you think?

I think having the implementation you suggest above is in any
event better than just an empty one. And with that I would also
suggest that its declaration be moved to a common header.

Jan

Re: [PATCH v9 07/10] xen: remove workaround to inject evtchn_irq on irq enable

[Stefano Stabellini]

On Tue, 5 Aug 2014, Jan Beulich wrote:
> >>> On 04.08.14 at 22:29, <stefano.stabellini@eu.citrix.com> wrote:
> > On Mon, 4 Aug 2014, Jan Beulich wrote:
> >> No, you're right. So coming back to your suspicion above: Nothing
> >> prevents a HVM guest to also call VCPUOP_register_vcpu_info on
> >> the boot CPU (and in fact such an asymmetry would seem pretty
> >> odd); old-style HVM guests with PV drivers (built from
> >> unmodified_drivers/) don't call VCPUOP_register_vcpu_info at all.
> >> But in the end if what you say is true there would be a case where
> >> x86 is also broken, just that there doesn't appear to be a kernel
> >> utilizing this case. Since especially for HVM guests we shouldn't be
> >> making assumptions in the hypervisor on guest behavior, shouldn't
> >> your patch at least try to address that case then at once?
> > 
> > The most logical thing to do would be to implement arch_evtchn_inject on
> > x86 as:
> > 
> > void arch_evtchn_inject(struct vcpu *v)
> > {
> >     if ( has_hvm_container_vcpu(v) )
> >         hvm_assert_evtchn_irq(v);
> > }
> > 
> > however it is very difficult to test because:
> > - the !xen_have_vector_callback code path doesn't work properly on a
> > modern Linux kernel;
> > - going all the way back to 2.6.37, !xen_have_vector_callback works but
> > then calling xen_vcpu_setup on vcpu0 doesn't work anyway. I don't know
> > exactly why but I don't think that the reason has anything to do with
> > the problem we are discussing here. In fact simply calling on vcpu0 an
> > hypercall that only sets evtchn_upcall_pending and then calls
> > arch_evtchn_inject works as espected.
> > 
> > I know we are not just dealing with Linux guests, but given all this I
> > am not sure how useful would actually be to provide the implementation
> > of arch_evtchn_inject on x86.  What do you think?
> 
> I think having the implementation you suggest above is in any
> event better than just an empty one. And with that I would also
> suggest that its declaration be moved to a common header.

OK

[PATCH v9 08/10] xen/arm: take the rank lock before accessing ipriority

[Stefano Stabellini]

Currently we read ipriority from vgic_vcpu_inject_irq without taking the
rank lock. Fix that by taking the rank lock and reading ipriority at the
beginning of the function.

As vgic_vcpu_inject_irq is called from the irq.c upon receiving an
interrupt, we need to change the implementation of vgic_lock/unlock_rank
to spin_lock_irqsave to make it safe in irq context.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v9:
- add explicit flags paramter to vgic_lock_rank and vgic_unlock_rank.

Changes in v2:
- rebased on ab78724fc5628318b172b4344f7280621a151e1b;
- remove warning on changing priority of active irqs.
---
 xen/arch/arm/vgic-v2.c     |   74 +++++++++++++++++++++++---------------------
 xen/arch/arm/vgic.c        |   10 ++++--
 xen/include/asm-arm/vgic.h |    4 +--
 3 files changed, 47 insertions(+), 41 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index f9e357c..8b21a13 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -39,6 +39,7 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
     register_t *r = select_user_reg(regs, dabt.reg);
     struct vgic_irq_rank *rank;
     int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
+    unsigned long flags;
 
     switch ( gicd_reg )
     {
@@ -77,54 +78,54 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISENABLER, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = rank->ienable;
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICENABLER, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = rank->ienable;
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISPENDR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ICPENDR ... GICD_ICPENDRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = vgic_byte_read(rank->ipend, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ISACTIVER ... GICD_ISACTIVERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = rank->iactive;
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ICACTIVER ... GICD_ICACTIVERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = rank->iactive;
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ITARGETSR ... GICD_ITARGETSRN:
@@ -132,12 +133,12 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
 
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR,
                                            DABT_WORD)];
         if ( dabt.size == DABT_BYTE )
             *r = vgic_byte_read(*r, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
@@ -145,21 +146,21 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
 
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
                                             DABT_WORD)];
         if ( dabt.size == DABT_BYTE )
             *r = vgic_byte_read(*r, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ICFGR ... GICD_ICFGRN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)];
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_NSACR ... GICD_NSACRN:
@@ -176,18 +177,18 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_CPENDSGIR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = vgic_byte_read(rank->pendsgi, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_SPENDSGIR ... GICD_SPENDSGIRN:
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_SPENDSGIR, DABT_WORD);
         if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         *r = vgic_byte_read(rank->pendsgi, dabt.sign, gicd_reg);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     /* Implementation defined -- read as zero */
@@ -269,6 +270,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     struct vgic_irq_rank *rank;
     int gicd_reg = (int)(info->gpa - v->domain->arch.vgic.dbase);
     uint32_t tr;
+    unsigned long flags;
 
     switch ( gicd_reg )
     {
@@ -295,7 +297,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISENABLER, DABT_WORD);
         if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         tr = rank->ienable;
         rank->ienable |= *r;
         /* The virtual irq is derived from register offset.
@@ -303,14 +305,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
          * to get Virtual irq number */
         vgic_enable_irqs(v, (*r) & (~tr),
                          (gicd_reg - GICD_ISENABLER) >> DABT_WORD);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICENABLER, DABT_WORD);
         if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         tr = rank->ienable;
         rank->ienable &= ~*r;
         /* The virtual irq is derived from register offset.
@@ -318,7 +320,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
          * to get  Virtual irq number */
         vgic_disable_irqs(v, (*r) & tr,
                          (gicd_reg - GICD_ICENABLER) >> DABT_WORD);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -337,18 +339,18 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER, DABT_WORD);
         if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         rank->iactive &= ~*r;
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ICACTIVER ... GICD_ICACTIVERN:
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER, DABT_WORD);
         if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         rank->iactive &= ~*r;
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ITARGETSR ... GICD_ITARGETSR + 7:
@@ -379,7 +381,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
             !((target & 0xff) && (target & (0xff << 8)) &&
              (target & (0xff << 16)) && (target & (0xff << 24))))
             goto write_ignore;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         i = 0;
         while ( (i = find_next_bit(&target, 32, i)) < 32 )
         {
@@ -406,7 +408,7 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         else
             vgic_byte_write(&rank->itargets[REG_RANK_INDEX(8,
                        gicd_reg - GICD_ITARGETSR, DABT_WORD)], target, gicd_reg);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
     }
 
@@ -414,14 +416,14 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
         if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         if ( dabt.size == DABT_WORD )
             rank->ipriority[REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR,
                                            DABT_WORD)] = *r;
         else
             vgic_byte_write(&rank->ipriority[REG_RANK_INDEX(8,
                         gicd_reg - GICD_IPRIORITYR, DABT_WORD)], *r, gicd_reg);
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_ICFGR: /* SGIs */
@@ -433,9 +435,9 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
         if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank);
+        vgic_lock_rank(v, rank, flags);
         rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, DABT_WORD)] = *r;
-        vgic_unlock_rank(v, rank);
+        vgic_unlock_rank(v, rank, flags);
         return 1;
 
     case GICD_NSACR ... GICD_NSACRN:
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 474eebd..a3df18d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -158,10 +158,11 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
     struct domain *d = v->domain;
     struct vcpu *v_target;
     struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+    unsigned long flags;
 
-    vgic_lock_rank(v, rank);
+    vgic_lock_rank(v, rank, flags);
     v_target = d->arch.vgic.handler->get_target_vcpu(v, irq);
-    vgic_unlock_rank(v, rank);
+    vgic_unlock_rank(v, rank, flags);
     return v_target;
 }
 
@@ -367,6 +368,10 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
     unsigned long flags;
     bool_t running;
 
+    vgic_lock_rank(v, rank, flags);
+    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, irq, DABT_WORD)], 0, irq & 0x3);
+    vgic_unlock_rank(v, rank, flags);
+
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
     /* vcpu offline */
@@ -384,7 +389,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
         goto out;
     }
  
-    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, irq, DABT_WORD)], 0, irq & 0x3);
 
     n->irq = irq;
     n->priority = priority;
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 9b1db04..338ba03 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -107,8 +107,8 @@ struct vgic_ops {
 #define vgic_lock(v)   spin_lock_irq(&(v)->domain->arch.vgic.lock)
 #define vgic_unlock(v) spin_unlock_irq(&(v)->domain->arch.vgic.lock)
 
-#define vgic_lock_rank(v, r) spin_lock(&(r)->lock)
-#define vgic_unlock_rank(v, r) spin_unlock(&(r)->lock)
+#define vgic_lock_rank(v, r, flags)   spin_lock_irqsave(&(r)->lock, flags)
+#define vgic_unlock_rank(v, r, flags) spin_unlock_irqrestore(&(r)->lock, flags)
 
 /*
  * Rank containing GICD_<FOO><n> for GICD_<FOO> with
-- 
1.7.10.4

Re: [PATCH v9 08/10] xen/arm: take the rank lock before accessing ipriority

[Julien Grall]

Hi Stefano,

On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> Currently we read ipriority from vgic_vcpu_inject_irq without taking the
> rank lock. Fix that by taking the rank lock and reading ipriority at the
> beginning of the function.
> 
> As vgic_vcpu_inject_irq is called from the irq.c upon receiving an
> interrupt, we need to change the implementation of vgic_lock/unlock_rank
> to spin_lock_irqsave to make it safe in irq context.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Which series between GICv3 and this one will go first? I suspect this
one, so Vijay will have to rebase his VGICv3 patch on yours.

[..]

> @@ -367,6 +368,10 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>      unsigned long flags;
>      bool_t running;
>  
> +    vgic_lock_rank(v, rank, flags);
> +    priority = vgic_byte_read(rank->ipriority[REG_RANK_INDEX(8, irq, DABT_WORD)], 0, irq & 0x3);

This line is longer than 80 characters. I would split it, but I'm not
sure if it will be readable.

Anyway:

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

[PATCH v9 09/10] xen: introduce bit access macros for the IRQ line status flags

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>

---

Changes in v9:
- do not rename IRQF_SHARED to IRQ_SHARED.
---
 xen/include/xen/irq.h |   27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index 40c0f3f..96d818f 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -22,15 +22,24 @@ struct irqaction {
 /*
  * IRQ line status.
  */
-#define IRQ_INPROGRESS    (1u<<0) /* IRQ handler active - do not enter! */
-#define IRQ_DISABLED      (1u<<1) /* IRQ disabled - do not enter! */
-#define IRQ_PENDING       (1u<<2) /* IRQ pending - replay on enable */
-#define IRQ_REPLAY        (1u<<3) /* IRQ has been replayed but not acked yet */
-#define IRQ_GUEST         (1u<<4) /* IRQ is handled by guest OS(es) */
-#define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
-#define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
-#define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */
-#define IRQF_SHARED       (1<<8)  /* IRQ is shared */
+#define _IRQ_INPROGRESS         0 /* IRQ handler active - do not enter! */
+#define _IRQ_DISABLED           1 /* IRQ disabled - do not enter! */
+#define _IRQ_PENDING            2 /* IRQ pending - replay on enable */
+#define _IRQ_REPLAY             3 /* IRQ has been replayed but not acked yet */
+#define _IRQ_GUEST              4 /* IRQ is handled by guest OS(es) */
+#define _IRQ_MOVE_PENDING       5 /* IRQ is migrating to another CPUs */
+#define _IRQ_PER_CPU            6 /* IRQ is per CPU */
+#define _IRQ_GUEST_EOI_PENDING  7 /* IRQ was disabled, pending a guest EOI */
+#define _IRQF_SHARED            8 /* IRQ is shared */
+#define IRQ_INPROGRESS          (1u<<_IRQ_INPROGRESS)
+#define IRQ_DISABLED            (1u<<_IRQ_DISABLED)
+#define IRQ_PENDING             (1u<<_IRQ_PENDING)
+#define IRQ_REPLAY              (1u<<_IRQ_REPLAY)
+#define IRQ_GUEST               (1u<<_IRQ_GUEST)
+#define IRQ_MOVE_PENDING        (1u<<_IRQ_MOVE_PENDING)
+#define IRQ_PER_CPU             (1u<<_IRQ_PER_CPU)
+#define IRQ_GUEST_EOI_PENDING   (1u<<_IRQ_GUEST_EOI_PENDING)
+#define IRQF_SHARED             (1u<<_IRQF_SHARED)
 
 /* Special IRQ numbers. */
 #define AUTO_ASSIGN_IRQ         (-1)
-- 
1.7.10.4

Re: [PATCH v9 09/10] xen: introduce bit access macros for the IRQ line status flags

[Julien Grall]

Hi Stefano,

On 07/24/2014 06:33 PM, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>

FWIW,

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

> ---
> 
> Changes in v9:
> - do not rename IRQF_SHARED to IRQ_SHARED.
> ---
>  xen/include/xen/irq.h |   27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index 40c0f3f..96d818f 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -22,15 +22,24 @@ struct irqaction {
>  /*
>   * IRQ line status.
>   */
> -#define IRQ_INPROGRESS    (1u<<0) /* IRQ handler active - do not enter! */
> -#define IRQ_DISABLED      (1u<<1) /* IRQ disabled - do not enter! */
> -#define IRQ_PENDING       (1u<<2) /* IRQ pending - replay on enable */
> -#define IRQ_REPLAY        (1u<<3) /* IRQ has been replayed but not acked yet */
> -#define IRQ_GUEST         (1u<<4) /* IRQ is handled by guest OS(es) */
> -#define IRQ_MOVE_PENDING  (1u<<5) /* IRQ is migrating to another CPUs */
> -#define IRQ_PER_CPU       (1u<<6) /* IRQ is per CPU */
> -#define IRQ_GUEST_EOI_PENDING (1u<<7) /* IRQ was disabled, pending a guest EOI */
> -#define IRQF_SHARED       (1<<8)  /* IRQ is shared */
> +#define _IRQ_INPROGRESS         0 /* IRQ handler active - do not enter! */
> +#define _IRQ_DISABLED           1 /* IRQ disabled - do not enter! */
> +#define _IRQ_PENDING            2 /* IRQ pending - replay on enable */
> +#define _IRQ_REPLAY             3 /* IRQ has been replayed but not acked yet */
> +#define _IRQ_GUEST              4 /* IRQ is handled by guest OS(es) */
> +#define _IRQ_MOVE_PENDING       5 /* IRQ is migrating to another CPUs */
> +#define _IRQ_PER_CPU            6 /* IRQ is per CPU */
> +#define _IRQ_GUEST_EOI_PENDING  7 /* IRQ was disabled, pending a guest EOI */
> +#define _IRQF_SHARED            8 /* IRQ is shared */
> +#define IRQ_INPROGRESS          (1u<<_IRQ_INPROGRESS)
> +#define IRQ_DISABLED            (1u<<_IRQ_DISABLED)
> +#define IRQ_PENDING             (1u<<_IRQ_PENDING)
> +#define IRQ_REPLAY              (1u<<_IRQ_REPLAY)
> +#define IRQ_GUEST               (1u<<_IRQ_GUEST)
> +#define IRQ_MOVE_PENDING        (1u<<_IRQ_MOVE_PENDING)
> +#define IRQ_PER_CPU             (1u<<_IRQ_PER_CPU)
> +#define IRQ_GUEST_EOI_PENDING   (1u<<_IRQ_GUEST_EOI_PENDING)
> +#define IRQF_SHARED             (1u<<_IRQF_SHARED)
>  
>  /* Special IRQ numbers. */
>  #define AUTO_ASSIGN_IRQ         (-1)
> 


-- 
Julien Grall

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

[Stefano Stabellini]

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(-)

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
-- 
1.7.10.4

Re: [PATCH v9 10/10] xen/arm: make accesses to desc->status flags atomic

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH v9 10/10] xen/arm: make accesses to desc->status flags atomic

[Stefano Stabellini]

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.

Re: [PATCH v9 10/10] xen/arm: make accesses to desc->status flags atomic

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH v9 10/10] xen/arm: make accesses to desc->status flags atomic

[Stefano Stabellini]

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!