[PATCH v7 0/6] vgic emulation and GICD_ITARGETSR

[PATCH v7 0/6] vgic emulation and GICD_ITARGETSR

[Stefano Stabellini]

Hi all,
this patch series improves vgic emulation in relation to GICD_ITARGETSR,
and implements irq delivery to vcpus other than vcpu0.

vgic_enable_irqs and vgic_disable_irqs currently ignore the itarget
settings and just enable/disable irqs on the current vcpu. Fix their
behaviour to enable/disable irqs on the vcpu set by itarget, that is
vcpu0 for irq >= 32 initially.

Introduce a new vgic function called vgic_get_target_vcpu to retrieve
the right target vcpu (looking at itargets) and use it from do_IRQ.

Change the physical irq affinity to make physical irqs follow virtual
cpus migration.

The main changes in v7 are:
- introduce a vcpu_migrate_from field in pending_irq to keep track of
the old vcpu so that we can kick it when we receive a new interrupt if
it appears to still be inflight
- delay setting the physical irq affinity until after clearing the
GIC_IRQ_GUEST_MIGRATING flag

The full list of changes follow.


Changes in v7:
- add ASSERT to _vgic_get_target_vcpu;
- add comment to vgic_distr_mmio_write;
- 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;
- use gic_number_lines in assert;
- 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:
- rebased on f7e46156a1c006a6eb5489e0227d39229eec316d;
- add ASSERT to _vgic_get_target_vcpu;
- add BUG_ON to vgic_distr_mmio_write;
- add in-code comment to _vgic_get_target_vcpu;
- move additional check on itargets writing from the second patch to
the first patch;
- sizeof(itargets) instead of 8*sizeof(itargets[0]);
- remove the unneeded cast of &target for find_first_bit;
- remove unnecessary casts to (const unsigned long *) to the arguments
of find_first_bit and find_next_bit;
- 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;
- add in-code comments to vgic_vcpu_inject_spi and do_IRQ;
- assert that the guest irq is an SPI in vgic_vcpu_inject_spi;
- use vgic_get_target_vcpu instead of _vgic_get_target_vcpu in
arch_move_irqs;
- introduce sched_move_irqs.


Changes in v5:
- add "rename vgic_irq_rank to vgic_rank_offset";
- add "introduce vgic_rank_irq";
- 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;
- 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;
- prettify vgic_move_irqs;
- rename vgic_move_irqs to arch_move_irqs;
- introduce helper function irq_set_affinity.



Stefano Stabellini (6):
      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/arch/arm/gic-v2.c        |   17 +++-
 xen/arch/arm/gic.c           |   32 ++++++-
 xen/arch/arm/irq.c           |   11 ++-
 xen/arch/arm/vgic.c          |  209 ++++++++++++++++++++++++++++++++++++++----
 xen/common/schedule.c        |   12 ++-
 xen/include/asm-arm/domain.h |    7 ++
 xen/include/asm-arm/gic.h    |    3 +
 xen/include/asm-arm/irq.h    |    2 +
 xen/include/asm-arm/vgic.h   |    1 +
 xen/include/asm-x86/irq.h    |    2 +
 10 files changed, 266 insertions(+), 30 deletions(-)

[PATCH v7 1/6] 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 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.c       |   82 ++++++++++++++++++++++++++++++++++++++-------
 xen/include/asm-arm/gic.h |    2 ++
 2 files changed, 71 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 96bd7c1..0c8673f 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -95,7 +95,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));
+    }
 
     /*
      * We rely on gicv_setup() to initialize dbase(vGIC distributor base)
@@ -346,6 +352,36 @@ read_as_zero:
     return 1;
 }
 
+/* the rank lock is already taken */
+static struct vcpu *_vgic_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;
+}
+
+/* takes the rank lock */
+struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+    struct vcpu *v_target;
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+
+    vgic_lock_rank(v, rank);
+    v_target = _vgic_get_target_vcpu(v, irq);
+    vgic_unlock_rank(v, rank);
+    return v_target;
+}
+
 static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     const unsigned long mask = r;
@@ -353,12 +389,14 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
     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 = _vgic_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);
@@ -376,24 +414,26 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     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 = _vgic_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 )
         {
@@ -508,8 +548,8 @@ static int vgic_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);
         vgic_enable_irqs(v, (*r) & (~tr), (gicd_reg - GICD_ISENABLER) >> 2);
+        vgic_unlock_rank(v, rank);
         return 1;
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -519,8 +559,8 @@ static int vgic_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);
         vgic_disable_irqs(v, (*r) & tr, (gicd_reg - GICD_ICENABLER) >> 2);
+        vgic_unlock_rank(v, rank);
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -561,13 +601,29 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
         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 * (offset & 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 == 2 )
-            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;
+            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = tr;
         else
         {
-            tr = REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR);
-            vgic_byte_write(&rank->itargets[tr], *r, offset);
+            int ri = REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR);
+            vgic_byte_write(&rank->itargets[ri], tr, offset);
         }
         vgic_unlock_rank(v, rank);
         return 1;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ed610cb..839d053 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -317,6 +317,8 @@ struct gic_hw_operations {
 
 void register_gic_ops(const struct gic_hw_operations *ops);
 
+struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
+
 #endif /* __ASSEMBLY__ */
 #endif
 
-- 
1.7.10.4

[PATCH v7 2/6] 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>
---
 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 0c8673f..e928879 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -750,13 +750,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) )
     {
@@ -764,10 +757,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)], 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 v7 2/6] xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier

[Julien Grall]

Hi Stefano,

On 07/03/2014 05:53 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>

Regards,

-- 
Julien Grall

Re: [PATCH v7 2/6] xen/arm: move setting GIC_IRQ_GUEST_QUEUED earlier

[Ian Campbell]

On Thu, 2014-07-03 at 17:53 +0100, 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: Ian Campbell <ian.campbell@citrix.com>

But is the movement of the list_empty not relevant enough to mention in
the commit log too? AFAICT it changes behaviour wrt vcpus which are
offline (in a good way, I think)

[PATCH v7 3/6] 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, so that we can recognize when we receive an irq
while the previous one is still inflight (given that we are only dealing
with hardware interrupts here, it just means that its LR hasn't been
cleared yet on the old vcpu).  If GIC_IRQ_GUEST_MIGRATING is set, we
only set GIC_IRQ_GUEST_QUEUED and interrupt the old vcpu. To know which
one is the old vcpu, we introduce a new field to pending_irq, called
vcpu_migrate_from.
When clearing the LR on the old vcpu, we take special care of injecting
the interrupt into the new vcpu. To do that we need to release the old
vcpu lock before taking the new vcpu lock.

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

---

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           |   28 +++++++++++-
 xen/arch/arm/vgic.c          |   99 ++++++++++++++++++++++++++++++++++++++----
 xen/include/asm-arm/domain.h |    7 +++
 3 files changed, 123 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e1e27b35..b5269d4 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);
 
@@ -372,10 +373,33 @@ 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 {
+            int m, q;
             list_del_init(&p->inflight);
+
+            m = test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+            p->vcpu_migrate_from = NULL;
+            /* check MIGRATING before QUEUED */
+            smp_rmb();
+            q = test_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+            if ( m && q )
+            {
+                struct vcpu *v_target;
+
+                /* It is safe to temporarily drop the lock because we
+                 * are finished dealing with this LR. We'll take the
+                 * lock before reading the next. */
+                spin_unlock(&v->arch.vgic.lock);
+                /* vgic_get_target_vcpu takes the rank lock, ensuring
+                 * consistency with other itarget changes. */
+                v_target = vgic_get_target_vcpu(v, irq);
+                vgic_vcpu_inject_irq(v_target, irq);
+                spin_lock(&v->arch.vgic.lock);
+            }
+        }
     }
 }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index e928879..8827a77 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -382,6 +382,46 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
     return v_target;
 }
 
+static 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) )
+    {
+        p->vcpu_migrate_from = old;
+        set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+    }
+
+    spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+}
+
 static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     const unsigned long mask = r;
@@ -598,35 +638,60 @@ static int vgic_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 != 0 && dabt.size != 2 ) goto bad_width;
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
         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 * (offset & 0x3)));
-        tr &= *r;
+            target = (target << (8 * (offset & 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, target, old_target;
+            unsigned long old_target_mask;
+            struct vcpu *v_target, *v_old;
+
+            target = i % 8;
+            old_target_mask = vgic_byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
+            old_target = find_first_bit(&old_target_mask, 8);
+
+            if ( target != old_target )
+            {
+                irq = offset + (i / 8);
+                v_target = v->domain->vcpu[target];
+                v_old = v->domain->vcpu[old_target];
+                vgic_migrate_irq(v_old, v_target, irq);
+            }
+            i += 8 - target;
+        }
         if ( dabt.size == 2 )
-            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = tr;
+            rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = target;
         else
         {
             int ri = REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR);
-            vgic_byte_write(&rank->itargets[ri], tr, offset);
+            vgic_byte_write(&rank->itargets[ri], target, offset);
         }
         vgic_unlock_rank(v, rank);
         return 1;
+    }
 
     case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
@@ -747,6 +812,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
     struct pending_irq *iter, *n = irq_to_pending(v, irq);
     unsigned long flags;
     bool_t running;
+    struct vcpu *vcpu_migrate_from;
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
@@ -758,6 +824,21 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
     }
 
     set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
+    vcpu_migrate_from = n->vcpu_migrate_from;
+    /* update QUEUED before MIGRATING */
+    smp_wmb();
+    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
+    {
+        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+
+        /* The old vcpu must have EOIed the SGI but not cleared the LR.
+         * Give it a kick. */
+        running = n->vcpu_migrate_from->is_running;
+        vcpu_unblock(n->vcpu_migrate_from);
+        if ( running )
+            smp_send_event_check_mask(cpumask_of(n->vcpu_migrate_from->processor));
+        return;
+    }
 
     if ( !list_empty(&n->inflight) )
     {
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 077ac1e..70c1215 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -48,17 +48,24 @@ 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;
 #define GIC_INVALID_LR         ~(uint8_t)0
     uint8_t lr;
     uint8_t priority;
+    /* keeps track of the vcpu this irq is currently migrating from */
+    struct vcpu *vcpu_migrate_from;
     /* inflight is used to append instances of pending_irq to
      * vgic.inflight_irqs */
     struct list_head inflight;
-- 
1.7.10.4

Re: [PATCH v7 3/6] xen/arm: inflight irqs during migration

[Julien Grall]

On 07/03/2014 05:53 PM, Stefano Stabellini wrote:
>  static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
>      const unsigned long mask = r;
> @@ -598,35 +638,60 @@ static int vgic_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 != 0 && dabt.size != 2 ) goto bad_width;
>          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
>          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 * (offset & 0x3)));
> -        tr &= *r;
> +            target = (target << (8 * (offset & 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, target, old_target;

target is already defined above, and this will shadow this previous
definition. I would rename one of them to avoid coding error later.

-- 
Julien Grall

[PATCH v7 4/6] 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>

---

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 b5269d4..be97261 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -138,7 +138,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 8827a77..b4493a3 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -873,6 +873,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 92f1e86..bb02a6c 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -86,6 +86,7 @@ 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 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 int vcpu_vgic_free(struct vcpu *v);
-- 
1.7.10.4

Re: [PATCH v7 4/6] xen/arm: support irq delivery to vcpu > 0

[Julien Grall]

Hi Stefano,

On 07/03/2014 05:53 PM, Stefano Stabellini wrote:
> 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>

Regards,

-- 
Julien Grall

Re: [PATCH v7 4/6] xen/arm: support irq delivery to vcpu > 0

[Ian Campbell]

On Thu, 2014-07-03 at 17:53 +0100, Stefano Stabellini wrote:
> 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: Ian Campbell <ian.campbell@citrix.com>

[PATCH v7 5/6] 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 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     |   17 +++++++++++++++--
 xen/arch/arm/gic.c        |    1 +
 xen/arch/arm/irq.c        |    6 ++++++
 xen/arch/arm/vgic.c       |   21 +++++++++++++++++++++
 xen/include/asm-arm/gic.h |    1 +
 xen/include/asm-arm/irq.h |    2 ++
 xen/include/asm-x86/irq.h |    2 ++
 7 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
index 695c232..c3d2853 100644
--- a/xen/arch/arm/gic-v2.c
+++ b/xen/arch/arm/gic-v2.c
@@ -532,9 +532,22 @@ 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();
+    volatile unsigned char *bytereg;
+    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) */
+    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
+    bytereg[desc->irq] = mask;
+
+    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 be97261..37b08c2 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -397,6 +397,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
                 /* vgic_get_target_vcpu takes the rank lock, ensuring
                  * consistency with other itarget changes. */
                 v_target = vgic_get_target_vcpu(v, irq);
+                irq_set_affinity(p->desc, cpumask_of(v_target->processor));
                 vgic_vcpu_inject_irq(v_target, irq);
                 spin_lock(&v->arch.vgic.lock);
             }
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 b4493a3..69d3040 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -399,6 +399,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
 
     if ( list_empty(&p->inflight) )
     {
+        irq_set_affinity(p->desc, cpumask_of(new->processor));
         spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
         return;
     }
@@ -407,6 +408,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
     {
         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;
@@ -422,6 +424,24 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
     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);
+    }
+}
+
 static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     const unsigned long mask = r;
@@ -477,6 +497,7 @@ static 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/gic.h b/xen/include/asm-arm/gic.h
index 839d053..6deb4bd 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -318,6 +318,7 @@ struct gic_hw_operations {
 void register_gic_ops(const struct gic_hw_operations *ops);
 
 struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
+void arch_move_irqs(struct vcpu *v);
 
 #endif /* __ASSEMBLY__ */
 #endif
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index e567f71..dc282f0 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -48,6 +48,8 @@ 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 v7 5/6] xen/arm: physical irq follow virtual irq

[Julien Grall]

On 07/03/2014 05:53 PM, Stefano Stabellini wrote:
> diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> index 695c232..c3d2853 100644
> --- a/xen/arch/arm/gic-v2.c
> +++ b/xen/arch/arm/gic-v2.c
> @@ -532,9 +532,22 @@ 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();
> +    volatile unsigned char *bytereg;
> +    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) */
> +    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
> +    bytereg[desc->irq] = mask;

The new implemenation of GICv2 is using {read,write}* helpers. Can you
use writeb_relaxed here, please?

[..]

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index b4493a3..69d3040 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -399,6 +399,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
>  
>      if ( list_empty(&p->inflight) )
>      {
> +        irq_set_affinity(p->desc, cpumask_of(new->processor));
>          spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>          return;
>      }
> @@ -407,6 +408,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
>      {
>          list_del_init(&p->lr_queue);
>          list_del_init(&p->inflight);
> +        irq_set_affinity(p->desc, cpumask_of(new->processor));

I think this irq_set_affinity is misplaced. You forgot to handle the
case where the IRQ has been EOIed, and no IRQ has been queued.

Also, it looks like this is done a bit late. the IRQ may fire on the
previous physical CPU again at least once.

This will happen the X-Gene quirk.

Regards,

-- 
Julien Grall

Re: [PATCH v7 5/6] xen/arm: physical irq follow virtual irq

[Ian Campbell]

On Fri, 2014-07-04 at 11:51 +0100, Julien Grall wrote:
> On 07/03/2014 05:53 PM, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 695c232..c3d2853 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -532,9 +532,22 @@ 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();
> > +    volatile unsigned char *bytereg;
> > +    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) */
> > +    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
> > +    bytereg[desc->irq] = mask;
> 
> The new implemenation of GICv2 is using {read,write}* helpers. Can you
> use writeb_relaxed here, please?

Now called writeb_gicd(mask, GICD_ITARGETSR + desc->irq) in staging...

Ian.

Re: [PATCH v7 5/6] xen/arm: physical irq follow virtual irq

[Stefano Stabellini]

On Fri, 4 Jul 2014, Julien Grall wrote:
> On 07/03/2014 05:53 PM, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c
> > index 695c232..c3d2853 100644
> > --- a/xen/arch/arm/gic-v2.c
> > +++ b/xen/arch/arm/gic-v2.c
> > @@ -532,9 +532,22 @@ 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();
> > +    volatile unsigned char *bytereg;
> > +    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) */
> > +    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
> > +    bytereg[desc->irq] = mask;
> 
> The new implemenation of GICv2 is using {read,write}* helpers. Can you
> use writeb_relaxed here, please?
> 
> [..]

sure



> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index b4493a3..69d3040 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -399,6 +399,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
> >  
> >      if ( list_empty(&p->inflight) )
> >      {
> > +        irq_set_affinity(p->desc, cpumask_of(new->processor));
> >          spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> >          return;
> >      }
> > @@ -407,6 +408,7 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
> >      {
> >          list_del_init(&p->lr_queue);
> >          list_del_init(&p->inflight);
> > +        irq_set_affinity(p->desc, cpumask_of(new->processor));
> 
> I think this irq_set_affinity is misplaced. You forgot to handle the
> case where the IRQ has been EOIed, and no IRQ has been queued.

If it was EOI'ed and cleared, then it falls into the first case above.
If it was EOI'ed and not yet cleared, then it falls into the last case
below.


> Also, it looks like this is done a bit late. the IRQ may fire on the
> previous physical CPU again at least once.
>
> This will happen the X-Gene quirk.

That's not a problem, the case is handled correctly.

[PATCH v7 6/6] 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