[PATCH v6 0/5] vgic emulation and GICD_ITARGETSR

[PATCH v6 0/5] 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.


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 (5):
      xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs
      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.c           |   46 +++++++++--
 xen/arch/arm/irq.c           |    5 +-
 xen/arch/arm/vgic.c          |  179 ++++++++++++++++++++++++++++++++++++++----
 xen/common/schedule.c        |   12 ++-
 xen/include/asm-arm/domain.h |    4 +
 xen/include/asm-arm/gic.h    |    4 +
 xen/include/asm-x86/irq.h    |    2 +
 7 files changed, 228 insertions(+), 24 deletions(-)

[PATCH v6 1/5] 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>

---


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       |   78 ++++++++++++++++++++++++++++++++++++++-------
 xen/include/asm-arm/gic.h |    2 ++
 2 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 757707e..1e1c244 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -111,7 +111,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));
+    }
     return 0;
 }
 
@@ -374,6 +380,35 @@ 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 = 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);
+    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;
@@ -381,12 +416,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);
@@ -404,24 +441,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 )
         {
@@ -536,8 +575,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);
+        vgic_unlock_rank(v, rank);
         return 1;
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -547,8 +586,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);
+        vgic_unlock_rank(v, rank);
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
@@ -589,12 +628,27 @@ 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;
+        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
             byte_write(&rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)],
-                       *r, offset);
+                       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 8e37ccf..3950554 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -226,6 +226,8 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
                   unsigned int *out_hwirq, unsigned int *out_type);
 void gic_clear_lrs(struct vcpu *v);
 
+struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq);
+
 #endif /* __ASSEMBLY__ */
 #endif
 
-- 
1.7.10.4

[PATCH v6 2/5] 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. 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.

In vgic_vcpu_inject_irq set GIC_IRQ_GUEST_QUEUED before testing
GIC_IRQ_GUEST_MIGRATING to avoid races with gic_update_one_lr.

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

---

Changes in v6:
- 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.

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           |   26 ++++++++++++--
 xen/arch/arm/vgic.c          |   78 ++++++++++++++++++++++++++++++++++++------
 xen/include/asm-arm/domain.h |    4 +++
 3 files changed, 95 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 4d2a92d..c7dda9b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -683,10 +683,32 @@ 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);
+            /* 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 1e1c244..7924629 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -409,6 +409,35 @@ 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 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 */
+    } else if ( !list_empty(&p->inflight) )
+        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;
@@ -546,6 +575,8 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     int offset = (int)(info->gpa - v->domain->arch.vgic.dbase);
     int gicd_reg = REG(offset);
     uint32_t tr;
+    unsigned long trl;
+    int i;
 
     switch ( gicd_reg )
     {
@@ -630,25 +661,45 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         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;
+        trl = (1 << v->domain->max_vcpus) - 1;
         if ( dabt.size == 2 )
-            tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
+            trl = trl | (trl << 8) | (trl << 16) | (trl << 24);
         else
-            tr = (tr << (8 * (offset & 0x3)));
-        tr &= *r;
+            trl = (trl << (8 * (offset & 0x3)));
+        trl &= *r;
         /* ignore zero writes */
-        if ( !tr )
+        if ( !trl )
             goto write_ignore;
         if ( dabt.size == 2 &&
-            !((tr & 0xff) && (tr & (0xff << 8)) &&
-             (tr & (0xff << 16)) && (tr & (0xff << 24))))
+            !((trl & 0xff) && (trl & (0xff << 8)) &&
+             (trl & (0xff << 16)) && (trl & (0xff << 24))))
             goto write_ignore;
         vgic_lock_rank(v, rank);
+        i = 0;
+        while ( (i = find_next_bit(&trl, 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 = 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)] = trl;
         else
             byte_write(&rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)],
-                       tr, offset);
+                       trl, offset);
         vgic_unlock_rank(v, rank);
         return 1;
 
@@ -786,9 +837,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
+    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
+    /* update QUEUED before MIGRATING */
+    smp_wmb();
+    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
+        goto out;
+
     if ( !list_empty(&n->inflight) )
     {
-        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
         gic_raise_inflight_irq(v, irq);
         goto out;
     }
@@ -796,6 +852,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
     /* vcpu offline */
     if ( test_bit(_VPF_down, &v->pause_flags) )
     {
+        clear_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
         spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
         return;
     }
@@ -803,7 +860,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
     priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
 
     n->irq = irq;
-    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
     n->priority = priority;
 
     /* the irq is enabled */
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 2cb6837..12b2a1d 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -55,11 +55,15 @@ 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.
+     *
      */
 #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;
-- 
1.7.10.4

[PATCH v6 3/5] 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 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        |    2 +-
 xen/arch/arm/irq.c        |    5 +++--
 xen/arch/arm/vgic.c       |   11 +++++++++++
 xen/include/asm-arm/gic.h |    1 +
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index c7dda9b..54610ce 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -292,7 +292,7 @@ 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 */
+    /* Route to vcpu0 by default */
     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 9c141bc..15b8edc 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -197,8 +197,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 7924629..b2f922c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -884,6 +884,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 <= 1019);
+
+    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/gic.h b/xen/include/asm-arm/gic.h
index 3950554..ba9ba9b 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -169,6 +169,7 @@ 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 struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 
-- 
1.7.10.4

[PATCH v6 4/5] 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.

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 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.c        |   18 ++++++++++++++++--
 xen/arch/arm/vgic.c       |   30 ++++++++++++++++++++++++++++++
 xen/include/asm-arm/gic.h |    1 +
 xen/include/asm-x86/irq.h |    2 ++
 4 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 54610ce..1965f86 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -192,9 +192,23 @@ static void gic_guest_irq_end(struct irq_desc *desc)
     /* Deactivation happens in maintenance interrupt / via GICV */
 }
 
-static void gic_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
+static void gic_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
 {
-    BUG();
+    volatile unsigned char *bytereg;
+    unsigned int mask;
+
+    if ( desc == NULL || cpumask_empty(cpu_mask) )
+        return;
+
+    spin_lock(&gic.lock);
+
+    mask = gic_cpu_mask(cpu_mask);
+
+    /* Set target CPU mask (RAZ/WI on uniprocessor) */
+    bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
+    bytereg[desc->irq] = mask;
+
+    spin_unlock(&gic.lock);
 }
 
 /* XXX different for level vs edge */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index b2f922c..5a504ad 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -438,6 +438,32 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
     spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
 }
 
+static inline void irq_set_affinity(struct irq_desc *desc,
+                                    const cpumask_t *cpu_mask)
+{
+    if ( desc != NULL )
+        desc->handler->set_affinity(desc, cpu_mask);
+}
+
+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);
+        if ( v_target == v )
+        {
+            p = irq_to_pending(v, i);
+            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;
@@ -493,6 +519,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);
@@ -681,6 +708,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
             unsigned int irq, target, old_target;
             unsigned long old_target_mask;
             struct vcpu *v_target, *v_old;
+            struct pending_irq *p;
 
             target = i % 8;
             old_target_mask = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
@@ -692,6 +720,8 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
                 v_target = v->domain->vcpu[target];
                 v_old = v->domain->vcpu[old_target];
                 vgic_migrate_irq(v_old, v_target, irq);
+                p = irq_to_pending(v_target, irq);
+                irq_set_affinity(p->desc, cpumask_of(v_target->processor));
             }
             i += 8 - target;
         }
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index ba9ba9b..dcc2f1c 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -228,6 +228,7 @@ int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
 void gic_clear_lrs(struct vcpu *v);
 
 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-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

[PATCH v6 5/5] 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>
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

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Julien Grall]

Hi Stefano,

On 06/23/2014 05:37 PM, Stefano Stabellini wrote:
> +/* 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 = 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);

I'm tempted to ask you adding an ASSERT here. Just for catching coding
error.

[..]

> @@ -536,8 +575,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);
> +        vgic_unlock_rank(v, rank);
>          return 1;
>  
>      case GICD_ICENABLER ... GICD_ICENABLERN:
> @@ -547,8 +586,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);
> +        vgic_unlock_rank(v, rank);
>          return 1;

Sorry for not having catch this earlier. I don't really like the idea to
extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
can be long to execute as it may touch the GIC distributor.

In another way, the rank lock is only taken in the distributor emulation.

Assuming we consider that distributor access may be slow:

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

Aside that, that made me think about two possible issue (not related to
this patch series) in vgic_inject_irq:
	- We don't take the rank lock to read the priority, even tho it's only
a byte access.
	- If the an interrupt is injected while it's already active, we don't
update the priority of this interrupt.

Regards,

-- 
Julien Grall

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Julien Grall]

Hi Stefano,

On 23/06/14 17:37, Stefano Stabellini wrote:
> @@ -786,9 +837,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>   
>       spin_lock_irqsave(&v->arch.vgic.lock, flags);
>   
> +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> +    /* update QUEUED before MIGRATING */
> +    smp_wmb();
> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
> +        goto out;

Why do you kick the current VCPU here? It looks like useless because the migration will take care of it.

> +
>       if ( !list_empty(&n->inflight) )
>       {
> -        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>           gic_raise_inflight_irq(v, irq);
>           goto out;
>       }
> @@ -796,6 +852,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>       /* vcpu offline */
>       if ( test_bit(_VPF_down, &v->pause_flags) )
>       {
> +        clear_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>           spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>           return;

Rather than setting & clearing the GUEST_QUEUED bit. Wouldn't it be better to move the if (test_bit(_VPF_down,...)) before the spin_lock?

If the VCPU is down here, it will likely be down before. Hence, the lock doesn't protect the pause_flags. This would also make the code clearer.

Regards,

-- 
Julien Grall

Re: [PATCH v6 5/5] xen: introduce sched_move_irqs

[Jan Beulich]

>>> On 23.06.14 at 18:37, <stefano.stabellini@eu.citrix.com> wrote:
> 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>

albeit from a formal pov it's George who needs to ack this since the
pattern says xen/common/sched*, not xen/common/sched_*.

Jan

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

On Mon, 23 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/23/2014 05:37 PM, Stefano Stabellini wrote:
> > +/* 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 = 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);
> 
> I'm tempted to ask you adding an ASSERT here. Just for catching coding
> error.
> 

OK


> 
> > @@ -536,8 +575,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);
> > +        vgic_unlock_rank(v, rank);
> >          return 1;
> >  
> >      case GICD_ICENABLER ... GICD_ICENABLERN:
> > @@ -547,8 +586,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);
> > +        vgic_unlock_rank(v, rank);
> >          return 1;
> 
> Sorry for not having catch this earlier. I don't really like the idea to
> extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
> can be long to execute as it may touch the GIC distributor.
> 
> In another way, the rank lock is only taken in the distributor emulation.
>
> Assuming we consider that distributor access may be slow:

We could end up enabling or disabling the wrong set of interrupts
without this change. I think it is necessary.


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

Thanks


> Aside that, that made me think about two possible issue (not related to
> this patch series) in vgic_inject_irq:
> 	- We don't take the rank lock to read the priority, even tho it's only
> a byte access.

That's a good point


> 	- If the an interrupt is injected while it's already active, we don't
> update the priority of this interrupt.

Yeah, I noticed this before. I'll write a comment to make sure we keep
in mind that we have this limitation.


> Regards,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Mon, 23 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/06/14 17:37, Stefano Stabellini wrote:
> > @@ -786,9 +837,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
> >   
> >       spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >   
> > +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> > +    /* update QUEUED before MIGRATING */
> > +    smp_wmb();
> > +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
> > +        goto out;
> 
> Why do you kick the current VCPU here? It looks like useless because the migration will take care of it.

With the physical follow virtual patch, the vcpu_unblock below is going
to be mostly useless but harmless as vgic_vcpu_inject_irq is going to be called on
the pcpu running the destination vcpu. smp_send_event_check_mask won't be called as
v == current.

On the other hand without that patch, the pcpu receiving the physical
interrupt could be different from any of the pcpus running the vcpus
involved in vcpu migration, therefore we would need the kick to wake up
the destination vcpu.


> > +
> >       if ( !list_empty(&n->inflight) )
> >       {
> > -        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> >           gic_raise_inflight_irq(v, irq);
> >           goto out;
> >       }
> > @@ -796,6 +852,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
> >       /* vcpu offline */
> >       if ( test_bit(_VPF_down, &v->pause_flags) )
> >       {
> > +        clear_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> >           spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >           return;
> 
> Rather than setting & clearing the GUEST_QUEUED bit. Wouldn't it be better to move the if (test_bit(_VPF_down,...)) before the spin_lock?
> 
> If the VCPU is down here, it will likely be down before. Hence, the lock doesn't protect the pause_flags. This would also make the code clearer.

Good suggestion

Re: [PATCH v6 5/5] xen: introduce sched_move_irqs

[George Dunlap]

On Tue, Jun 24, 2014 at 7:38 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 23.06.14 at 18:37, <stefano.stabellini@eu.citrix.com> wrote:
>> 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>

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Julien Grall]

On 24/06/14 12:38, Stefano Stabellini wrote:
>> Sorry for not having catch this earlier. I don't really like the idea to
>> extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
>> can be long to execute as it may touch the GIC distributor.
>>
>> In another way, the rank lock is only taken in the distributor emulation.
>>
>> Assuming we consider that distributor access may be slow:
>
> We could end up enabling or disabling the wrong set of interrupts
> without this change. I think it is necessary.

AFAIU, this lock only protects the rank when we retrieve the target 
VCPU, the other part of the function will still work without it.

What I meant is to call vgic_get_target_vcpu, so the lock will protect 
only what is necessary and not more.

-- 
Julien Grall

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Julien Grall]

On 24/06/14 12:57, Stefano Stabellini wrote:
> On Mon, 23 Jun 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 23/06/14 17:37, Stefano Stabellini wrote:
>>> @@ -786,9 +837,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>>>
>>>        spin_lock_irqsave(&v->arch.vgic.lock, flags);
>>>
>>> +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>>> +    /* update QUEUED before MIGRATING */
>>> +    smp_wmb();
>>> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
>>> +        goto out;
>>
>> Why do you kick the current VCPU here? It looks like useless because the migration will take care of it.
>
> With the physical follow virtual patch, the vcpu_unblock below is going
> to be mostly useless but harmless as vgic_vcpu_inject_irq is going to be called on
> the pcpu running the destination vcpu. smp_send_event_check_mask won't be called as
> v == current.
>
> On the other hand without that patch, the pcpu receiving the physical
> interrupt could be different from any of the pcpus running the vcpus
> involved in vcpu migration, therefore we would need the kick to wake up
> the destination vcpu.

If the MIGRATING bit is set it means that an IRQ is already inflight, 
and therefore gic_update_one_lr will take care of injecting this IRQ to 
the new VCPU by calling vgic_vcpu_inject_irq.
So kicking the new VCPU here is pointless and may unschedule another 
VCPU for nothing. The latter may be able to run a bit more.

With your patch #4 (physical IRQ follow virtual IRQ), there is a tiny 
range the VCPU may not run on the same pCPU where the physical IRQ as 
been routed. This is the case when the VCPU is unscheduled.

Regards,

-- 
Julien Grall

Re: [PATCH v6 3/5] xen/arm: support irq delivery to vcpu > 0

[Julien Grall]

Hi Stefano,

On 06/23/2014 05:37 PM, Stefano Stabellini wrote:
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index c7dda9b..54610ce 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -292,7 +292,7 @@ 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 */
> +    /* Route to vcpu0 by default */

This comment is wrong here... we only set the desc for a virtual SPIs.

The routing is done in domain_vgic_init.

> +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 <= 1019);

I would use gic_number_lines() rather than 1019 here.

If the IRQ is greater than the return value of the function then there
is already an issue.

IHMO, your ASSERT is implementing your comment in do_IRQ. So it would
make sense to have this ASSERT just before calling vgic_vcpu_inject_spi.

If the caller is calling this function with a PPIs or SGIs then he is
stupid.

Regards,

-- 
Julien Grall

Re: [PATCH v6 4/5] xen/arm: physical irq follow virtual irq

[Julien Grall]

Hi Stefano,

On 06/23/2014 05:37 PM, Stefano Stabellini wrote:
> -static void gic_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> +static void gic_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
>  {
> -    BUG();
> +    volatile unsigned char *bytereg;
> +    unsigned int mask;
> +
> +    if ( desc == NULL || cpumask_empty(cpu_mask) )
> +        return;

I think this check is pointless. Every irq_callback relies on the desc
is not NULL. Hence, you already check it in irq_set_affinity.

For the cpu_mask, we are in trouble is someone is calling with an empty
mask. I would replace it by an ASSERT.

The check here is pointless here. Every irq_callback rely on the desc is
not NULL. Why this

>  /* XXX different for level vs edge */
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index b2f922c..5a504ad 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -438,6 +438,32 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
>      spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
>  }
>  
> +static inline void irq_set_affinity(struct irq_desc *desc,
> +                                    const cpumask_t *cpu_mask)
> +{
> +    if ( desc != NULL )
> +        desc->handler->set_affinity(desc, cpu_mask);
> +}

This function belongs to the IRQ module. It might be useful for other
part in the future.

I would move this function in irq.c or in asm-arm/irq.h if you want to
keep it as inline function.

Regards,

-- 
Julien Grall

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

On Tue, 24 Jun 2014, Julien Grall wrote:
> On 24/06/14 12:38, Stefano Stabellini wrote:
> > > Sorry for not having catch this earlier. I don't really like the idea to
> > > extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
> > > can be long to execute as it may touch the GIC distributor.
> > > 
> > > In another way, the rank lock is only taken in the distributor emulation.
> > > 
> > > Assuming we consider that distributor access may be slow:
> > 
> > We could end up enabling or disabling the wrong set of interrupts
> > without this change. I think it is necessary.
> 
> AFAIU, this lock only protects the rank when we retrieve the target VCPU, the
> other part of the function will still work without it.
> 
> What I meant is to call vgic_get_target_vcpu, so the lock will protect only
> what is necessary and not more.

I don't think so (unless I misunderstood your suggestion): setting
ienable and enabling the interrupts need to be atomic.  Otherwise this
can happen:

    VCPU0                                       VCPU1
- rank_lock
- write icenabler
- get target vcpus
- rank_unlock
                                         - rank_lock
                                         - write icenable
                                         - get target vcpus (some are the same)
                                         - rank_unlock

                                         - vgic_enable_irqs

- vgic_enable_irqs


we now have an inconsistent state: we enabled the irqs written from
vcpu0 but icenable reflects what was written from vcpu1.

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Julien Grall]

On 06/24/2014 07:04 PM, Stefano Stabellini wrote:
> On Tue, 24 Jun 2014, Julien Grall wrote:
>> On 24/06/14 12:38, Stefano Stabellini wrote:
>>>> Sorry for not having catch this earlier. I don't really like the idea to
>>>> extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
>>>> can be long to execute as it may touch the GIC distributor.
>>>>
>>>> In another way, the rank lock is only taken in the distributor emulation.
>>>>
>>>> Assuming we consider that distributor access may be slow:
>>>
>>> We could end up enabling or disabling the wrong set of interrupts
>>> without this change. I think it is necessary.
>>
>> AFAIU, this lock only protects the rank when we retrieve the target VCPU, the
>> other part of the function will still work without it.
>>
>> What I meant is to call vgic_get_target_vcpu, so the lock will protect only
>> what is necessary and not more.
> 
> I don't think so (unless I misunderstood your suggestion): setting
> ienable and enabling the interrupts need to be atomic.  Otherwise this
> can happen:
> 
>     VCPU0                                       VCPU1
> - rank_lock
> - write icenabler
> - get target vcpus
> - rank_unlock
>                                          - rank_lock
>                                          - write icenable
>                                          - get target vcpus (some are the same)
>                                          - rank_unlock
> 
>                                          - vgic_enable_irqs
> 
> - vgic_enable_irqs
> 
> 
> we now have an inconsistent state: we enabled the irqs written from
> vcpu0 but icenable reflects what was written from vcpu1.

In your example it's not possible because we save the value of ienable
in a temporary variable. So calling to vgic_enable_irqs on 2 different
VCPU with the same range of IRQ won't be a problem.

But... there is a possible race condition between enable and disable. We
need to serialize the access otherwise we may enable/disable by mistake
an IRQ and it's not synced anymore with the register state.

This is issue is also on Xen 4.4. Can you send a single patch which move
the unlock for this branch?

Thanks,

-- 
Julien Grall

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

On Tue, 24 Jun 2014, Julien Grall wrote:
> On 06/24/2014 07:04 PM, Stefano Stabellini wrote:
> > On Tue, 24 Jun 2014, Julien Grall wrote:
> >> On 24/06/14 12:38, Stefano Stabellini wrote:
> >>>> Sorry for not having catch this earlier. I don't really like the idea to
> >>>> extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
> >>>> can be long to execute as it may touch the GIC distributor.
> >>>>
> >>>> In another way, the rank lock is only taken in the distributor emulation.
> >>>>
> >>>> Assuming we consider that distributor access may be slow:
> >>>
> >>> We could end up enabling or disabling the wrong set of interrupts
> >>> without this change. I think it is necessary.
> >>
> >> AFAIU, this lock only protects the rank when we retrieve the target VCPU, the
> >> other part of the function will still work without it.
> >>
> >> What I meant is to call vgic_get_target_vcpu, so the lock will protect only
> >> what is necessary and not more.
> > 
> > I don't think so (unless I misunderstood your suggestion): setting
> > ienable and enabling the interrupts need to be atomic.  Otherwise this
> > can happen:
> > 
> >     VCPU0                                       VCPU1
> > - rank_lock
> > - write icenabler
> > - get target vcpus
> > - rank_unlock
> >                                          - rank_lock
> >                                          - write icenable
> >                                          - get target vcpus (some are the same)
> >                                          - rank_unlock
> > 
> >                                          - vgic_enable_irqs
> > 
> > - vgic_enable_irqs
> > 
> > 
> > we now have an inconsistent state: we enabled the irqs written from
> > vcpu0 but icenable reflects what was written from vcpu1.
> 
> In your example it's not possible because we save the value of ienable
> in a temporary variable. So calling to vgic_enable_irqs on 2 different
> VCPU with the same range of IRQ won't be a problem.
> 
> But... there is a possible race condition between enable and disable. We
> need to serialize the access otherwise we may enable/disable by mistake
> an IRQ and it's not synced anymore with the register state.
> 
> This is issue is also on Xen 4.4. Can you send a single patch which move
> the unlock for this branch?

Sure



> Thanks,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Ian Campbell]

On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> @@ -589,12 +628,27 @@ 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;

Please can you add a comment here like:

                /* For word reads ignore writes where any single byte is zero */

With that: Acked-by: Ian Campbell <ian.campbell@citrix.com>

Although, might it be more reasonable to use the existing value for such
bytes? (i.e. only ignore the zero bytes, not the whole thing)

Ian.

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Ian Campbell]

On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> - 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.

You access/change those with test_bit et al which already include
appropriate barriers/ordering guarantees, I think.

The __test_foo are the variants without ordering.

IOW I think you can probably do without some/all of those barriers.

> @@ -546,6 +575,8 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>      int offset = (int)(info->gpa - v->domain->arch.vgic.dbase);
>      int gicd_reg = REG(offset);
>      uint32_t tr;
> +    unsigned long trl;

Can you add /* Need unsigned long for XXX */?

Actually, even better would be to call this variable "target" or
something. (tgt?)

And even better than that would be:

 case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
 {
      unsigned long target;
      ... stuff ...
      return 1;
 }

so the scope is limited to the uses.

> +    int i;
>  
>      switch ( gicd_reg )
>      {
[...]
> @@ -786,9 +837,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>  
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>  
> +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> +    /* update QUEUED before MIGRATING */
                               ^testing

(otherwise I wonder why you aren't setting it)

> +    smp_wmb();
> +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
> +        goto out;
> +
>      if ( !list_empty(&n->inflight) )
>      {
> -        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
>          gic_raise_inflight_irq(v, irq);
>          goto out;
>      }

Ian.

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Ian Campbell]

On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:

> +        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 */

Is it safe that MIGRATING serves dual purpose -- it indicates an irq
which is actively being moved but it also indicates an irq where we are
awaiting an EOI before migrating it. (Or have I misunderstood what is
going on here?)

I suspect what I'm worried about is something completing the migration
without realising it needs to wait for an EOI?

Ian.

Re: [PATCH v6 3/5] xen/arm: support irq delivery to vcpu > 0

[Ian Campbell]

On Tue, 2014-06-24 at 14:33 +0100, Julien Grall wrote:

> > +    /* the IRQ needs to be an SPI */
> > +    ASSERT(irq >= 32 && irq <= 1019);
> 
> I would use gic_number_lines() rather than 1019 here.

Or at least #define GIC_MAX_SPI 1019.

Ian.

Re: [PATCH v6 5/5] xen: introduce sched_move_irqs

[Ian Campbell]

On Tue, 2014-06-24 at 13:02 +0100, George Dunlap wrote:
> On Tue, Jun 24, 2014 at 7:38 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>> On 23.06.14 at 18:37, <stefano.stabellini@eu.citrix.com> wrote:
> >> 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>

(was feeling left out ;-))

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

On Fri, 27 Jun 2014, Ian Campbell wrote:
> On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> > @@ -589,12 +628,27 @@ 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;
> 
> Please can you add a comment here like:
> 
>                 /* For word reads ignore writes where any single byte is zero */
> 
> With that: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Although, might it be more reasonable to use the existing value for such
> bytes? (i.e. only ignore the zero bytes, not the whole thing)

I don't know.. I would consider the entire write as invalid as it
containts some invalid configurations.

Re: [PATCH v6 3/5] xen/arm: support irq delivery to vcpu > 0

[Stefano Stabellini]

On Tue, 24 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/23/2014 05:37 PM, Stefano Stabellini wrote:
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index c7dda9b..54610ce 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -292,7 +292,7 @@ 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 */
> > +    /* Route to vcpu0 by default */
> 
> This comment is wrong here... we only set the desc for a virtual SPIs.
> 
> The routing is done in domain_vgic_init.

OK

> > +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 <= 1019);
> 
> I would use gic_number_lines() rather than 1019 here.

OK


> If the IRQ is greater than the return value of the function then there
> is already an issue.
> 
> IHMO, your ASSERT is implementing your comment in do_IRQ. So it would
> make sense to have this ASSERT just before calling vgic_vcpu_inject_spi.
> If the caller is calling this function with a PPIs or SGIs then he is
> stupid.

I don't like the idea of moving ASSERTs to the call site: what if we end
up having 3 or 4 places that independently call vgic_vcpu_inject_spi,
Should we add the same ASSERT to all of them?
This is why I prefer to keep the ASSERT in one place,
vgic_vcpu_inject_spi.


> Regards,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Ian Campbell]

On Wed, 2014-07-02 at 16:39 +0100, Stefano Stabellini wrote:
> On Fri, 27 Jun 2014, Ian Campbell wrote:
> > On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> > > @@ -589,12 +628,27 @@ 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;
> > 
> > Please can you add a comment here like:
> > 
> >                 /* For word reads ignore writes where any single byte is zero */
> > 
> > With that: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Although, might it be more reasonable to use the existing value for such
> > bytes? (i.e. only ignore the zero bytes, not the whole thing)
> 
> I don't know.. I would consider the entire write as invalid as it
> containts some invalid configurations.

The question is what does the spec say?

Ian.

Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

On Wed, 2 Jul 2014, Ian Campbell wrote:
> On Wed, 2014-07-02 at 16:39 +0100, Stefano Stabellini wrote:
> > On Fri, 27 Jun 2014, Ian Campbell wrote:
> > > On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> > > > @@ -589,12 +628,27 @@ 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;
> > > 
> > > Please can you add a comment here like:
> > > 
> > >                 /* For word reads ignore writes where any single byte is zero */
> > > 
> > > With that: Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > > 
> > > Although, might it be more reasonable to use the existing value for such
> > > bytes? (i.e. only ignore the zero bytes, not the whole thing)
> > 
> > I don't know.. I would consider the entire write as invalid as it
> > containts some invalid configurations.
> 
> The question is what does the spec say?

Unfortunately the spec is not clear about it.

Re: [PATCH v6 4/5] xen/arm: physical irq follow virtual irq

[Stefano Stabellini]

On Tue, 24 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/23/2014 05:37 PM, Stefano Stabellini wrote:
> > -static void gic_irq_set_affinity(struct irq_desc *desc, const cpumask_t *mask)
> > +static void gic_irq_set_affinity(struct irq_desc *desc, const cpumask_t *cpu_mask)
> >  {
> > -    BUG();
> > +    volatile unsigned char *bytereg;
> > +    unsigned int mask;
> > +
> > +    if ( desc == NULL || cpumask_empty(cpu_mask) )
> > +        return;
> 
> I think this check is pointless. Every irq_callback relies on the desc
> is not NULL. Hence, you already check it in irq_set_affinity.
> 
> For the cpu_mask, we are in trouble is someone is calling with an empty
> mask. I would replace it by an ASSERT.

OK


> The check here is pointless here. Every irq_callback rely on the desc is
> not NULL. Why this
> 
> >  /* XXX different for level vs edge */
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index b2f922c..5a504ad 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -438,6 +438,32 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
> >      spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> >  }
> >  
> > +static inline void irq_set_affinity(struct irq_desc *desc,
> > +                                    const cpumask_t *cpu_mask)
> > +{
> > +    if ( desc != NULL )
> > +        desc->handler->set_affinity(desc, cpu_mask);
> > +}
> 
> This function belongs to the IRQ module. It might be useful for other
> part in the future.
> 
> I would move this function in irq.c or in asm-arm/irq.h if you want to
> keep it as inline function.

OK

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Fri, 27 Jun 2014, Ian Campbell wrote:
> On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> > - 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.
> 
> You access/change those with test_bit et al which already include
> appropriate barriers/ordering guarantees, I think.
> 
> The __test_foo are the variants without ordering.
> 
> IOW I think you can probably do without some/all of those barriers.
> 
> > @@ -546,6 +575,8 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >      int offset = (int)(info->gpa - v->domain->arch.vgic.dbase);
> >      int gicd_reg = REG(offset);
> >      uint32_t tr;
> > +    unsigned long trl;
> 
> Can you add /* Need unsigned long for XXX */?
> 
> Actually, even better would be to call this variable "target" or
> something. (tgt?)
> 
> And even better than that would be:
> 
>  case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
>  {
>       unsigned long target;
>       ... stuff ...
>       return 1;
>  }

OK

> so the scope is limited to the uses.
> 
> > +    int i;
> >  
> >      switch ( gicd_reg )
> >      {
> [...]
> > @@ -786,9 +837,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
> >  
> >      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >  
> > +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> > +    /* update QUEUED before MIGRATING */
>                                ^testing
> 
> (otherwise I wonder why you aren't setting it)

right

> > +    smp_wmb();
> > +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
> > +        goto out;
> > +
> >      if ( !list_empty(&n->inflight) )
> >      {
> > -        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> >          gic_raise_inflight_irq(v, irq);
> >          goto out;
> >      }
> 
> Ian.
> 
> 

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Fri, 27 Jun 2014, Ian Campbell wrote:
> On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> 
> > +        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 */
> 
> Is it safe that MIGRATING serves dual purpose -- it indicates an irq
> which is actively being moved but it also indicates an irq where we are
> awaiting an EOI before migrating it. (Or have I misunderstood what is
> going on here?)

As described in the commit message, MIGRATING is just to flag an irq
that has been migrated but for which we are also awaiting an EOI.
Maybe I can rename GIC_IRQ_GUEST_MIGRATING to
GIC_IRQ_GUEST_MIGRATING_EOI to make it more obvious that is targeting a
specific corner case.


> I suspect what I'm worried about is something completing the migration
> without realising it needs to wait for an EOI?

We carefully check the current status of the irq in vgic_migrate_irq
before setting GIC_IRQ_GUEST_MIGRATING. Only if the irq is already in
an LR register we set GIC_IRQ_GUEST_MIGRATING.

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Tue, 24 Jun 2014, Julien Grall wrote:
> On 24/06/14 12:57, Stefano Stabellini wrote:
> > On Mon, 23 Jun 2014, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 23/06/14 17:37, Stefano Stabellini wrote:
> > > > @@ -786,9 +837,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned
> > > > int irq)
> > > > 
> > > >        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > > 
> > > > +    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
> > > > +    /* update QUEUED before MIGRATING */
> > > > +    smp_wmb();
> > > > +    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
> > > > +        goto out;
> > > 
> > > Why do you kick the current VCPU here? It looks like useless because the
> > > migration will take care of it.
> > 
> > With the physical follow virtual patch, the vcpu_unblock below is going
> > to be mostly useless but harmless as vgic_vcpu_inject_irq is going to be
> > called on
> > the pcpu running the destination vcpu. smp_send_event_check_mask won't be
> > called as
> > v == current.
> > 
> > On the other hand without that patch, the pcpu receiving the physical
> > interrupt could be different from any of the pcpus running the vcpus
> > involved in vcpu migration, therefore we would need the kick to wake up
> > the destination vcpu.
> 
> If the MIGRATING bit is set it means that an IRQ is already inflight, and
> therefore gic_update_one_lr will take care of injecting this IRQ to the new
> VCPU by calling vgic_vcpu_inject_irq.
> So kicking the new VCPU here is pointless and may unschedule another VCPU for
> nothing. The latter may be able to run a bit more.
> 
> With your patch #4 (physical IRQ follow virtual IRQ), there is a tiny range
> the VCPU may not run on the same pCPU where the physical IRQ as been routed.
> This is the case when the VCPU is unscheduled.

I think that you are right. I'll remove the vcpu kick.
Also I realize now that I might be able to simplify the code by updating
the affinity of the physical irq only after the MIGRATING virq has been
EOIed.

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Ian Campbell]

On Wed, 2014-07-02 at 19:33 +0100, Stefano Stabellini wrote:
> On Fri, 27 Jun 2014, Ian Campbell wrote:
> > On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> > 
> > > +        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 */
> > 
> > Is it safe that MIGRATING serves dual purpose -- it indicates an irq
> > which is actively being moved but it also indicates an irq where we are
> > awaiting an EOI before migrating it. (Or have I misunderstood what is
> > going on here?)
> 
> As described in the commit message, MIGRATING is just to flag an irq
> that has been migrated but for which we are also awaiting an EOI.

Ah, for some reason I thought it was also signalling an IRQ which was
currently being migrated.

How do you handle the race between writing the real itargets register
and inflight (real) interrupts? After the write you might still see some
interrupt on the old processor I think. I thought that was what the bit
was for.

e.g. on x86 I think they don't consider the interrupt to have migrated
until they observe the first one on the target processor. Maybe x86 int
controllers and ARM ones differ wrt this sort of thing though.

> Maybe I can rename GIC_IRQ_GUEST_MIGRATING to
> GIC_IRQ_GUEST_MIGRATING_EOI to make it more obvious that is targeting a
> specific corner case.
> 
> 
> > I suspect what I'm worried about is something completing the migration
> > without realising it needs to wait for an EOI?
> 
> We carefully check the current status of the irq in vgic_migrate_irq
> before setting GIC_IRQ_GUEST_MIGRATING. Only if the irq is already in
> an LR register we set GIC_IRQ_GUEST_MIGRATING.

Assuming the above doesn't cause a rethink of the implementation then I
think just clarifying the comment:

+     * GIC_IRQ_GUEST_MIGRATING: the irq is being migrated to a different
+     * vcpu.

would be sufficient.

Ian.

Re: [PATCH v6 2/5] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Thu, 3 Jul 2014, Ian Campbell wrote:
> On Wed, 2014-07-02 at 19:33 +0100, Stefano Stabellini wrote:
> > On Fri, 27 Jun 2014, Ian Campbell wrote:
> > > On Mon, 2014-06-23 at 17:37 +0100, Stefano Stabellini wrote:
> > > 
> > > > +        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 */
> > > 
> > > Is it safe that MIGRATING serves dual purpose -- it indicates an irq
> > > which is actively being moved but it also indicates an irq where we are
> > > awaiting an EOI before migrating it. (Or have I misunderstood what is
> > > going on here?)
> > 
> > As described in the commit message, MIGRATING is just to flag an irq
> > that has been migrated but for which we are also awaiting an EOI.
> 
> Ah, for some reason I thought it was also signalling an IRQ which was
> currently being migrated.
> 
> How do you handle the race between writing the real itargets register
> and inflight (real) interrupts? After the write you might still see some
> interrupt on the old processor I think. I thought that was what the bit
> was for.

Not a problem: a new interrupt can still come to the old processor and
the gic driver would know how to inject it properly. It could even
arrive on a processor that is nethier the old or the new one and
everything would still work.
The only case that needs care is when an irq is still on an LR register
on the old processor: the MIGRATING bit is for that.


> e.g. on x86 I think they don't consider the interrupt to have migrated
> until they observe the first one on the target processor. Maybe x86 int
> controllers and ARM ones differ wrt this sort of thing though.

I don't think that is an issue for us either way, as long as we take the
right vcpu lock and we have code for that now.


> > Maybe I can rename GIC_IRQ_GUEST_MIGRATING to
> > GIC_IRQ_GUEST_MIGRATING_EOI to make it more obvious that is targeting a
> > specific corner case.
> > 
> > 
> > > I suspect what I'm worried about is something completing the migration
> > > without realising it needs to wait for an EOI?
> > 
> > We carefully check the current status of the irq in vgic_migrate_irq
> > before setting GIC_IRQ_GUEST_MIGRATING. Only if the irq is already in
> > an LR register we set GIC_IRQ_GUEST_MIGRATING.
> 
> Assuming the above doesn't cause a rethink of the implementation then I
> think just clarifying the comment:
> 
> +     * GIC_IRQ_GUEST_MIGRATING: the irq is being migrated to a different
> +     * vcpu.
> 
> would be sufficient.

OK