[PATCH v5 0/6] vgic emulation and GICD_ITARGETSR

[PATCH v5 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.



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: rename vgic_irq_rank to vgic_rank_offset
      xen/arm: introduce vgic_rank_irq
      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/arch/arm/gic.c           |   44 +++++++--
 xen/arch/arm/irq.c           |    3 +-
 xen/arch/arm/vgic.c          |  202 +++++++++++++++++++++++++++++++++++-------
 xen/common/event_channel.c   |    2 +
 xen/include/asm-arm/domain.h |    4 +
 xen/include/asm-arm/gic.h    |    4 +
 xen/include/asm-x86/irq.h    |    2 +
 7 files changed, 222 insertions(+), 39 deletions(-)

[PATCH v5 1/6] xen/arm: rename vgic_irq_rank to vgic_rank_offset

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/vgic.c |   40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index cb8df3a..f8cb3d5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -61,7 +61,7 @@ static inline int REG_RANK_NR(int b, uint32_t n)
  * Returns rank corresponding to a GICD_<FOO><n> register for
  * GICD_<FOO> with <b>-bits-per-interrupt.
  */
-static struct vgic_irq_rank *vgic_irq_rank(struct vcpu *v, int b, int n)
+static struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n)
 {
     int rank = REG_RANK_NR(b, n);
 
@@ -216,7 +216,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ISENABLER ... GICD_ISENABLERN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISENABLER);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISENABLER);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = rank->ienable;
@@ -225,7 +225,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICENABLER);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICENABLER);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = rank->ienable;
@@ -234,7 +234,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISPENDR);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISPENDR);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = byte_read(rank->ipend, dabt.sign, offset);
@@ -243,7 +243,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ICPENDR ... GICD_ICPENDRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICPENDR);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICPENDR);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = byte_read(rank->ipend, dabt.sign, offset);
@@ -252,7 +252,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ISACTIVER ... GICD_ISACTIVERN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISACTIVER);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = rank->iactive;
@@ -261,7 +261,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ICACTIVER ... GICD_ICACTIVERN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICACTIVER);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = rank->iactive;
@@ -270,7 +270,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ITARGETSR ... GICD_ITARGETSRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
+        rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
         if ( rank == NULL) goto read_as_zero;
 
         vgic_lock_rank(v, rank);
@@ -282,7 +282,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 8, gicd_reg - GICD_IPRIORITYR);
+        rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR);
         if ( rank == NULL) goto read_as_zero;
 
         vgic_lock_rank(v, rank);
@@ -294,7 +294,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ICFGR ... GICD_ICFGRN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 2, gicd_reg - GICD_ICFGR);
+        rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR)];
@@ -313,7 +313,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_CPENDSGIR ... GICD_CPENDSGIRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_CPENDSGIR);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_CPENDSGIR);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = byte_read(rank->pendsgi, dabt.sign, offset);
@@ -322,7 +322,7 @@ static int vgic_distr_mmio_read(struct vcpu *v, mmio_info_t *info)
 
     case GICD_SPENDSGIR ... GICD_SPENDSGIRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_SPENDSGIR);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_SPENDSGIR);
         if ( rank == NULL) goto read_as_zero;
         vgic_lock_rank(v, rank);
         *r = byte_read(rank->pendsgi, dabt.sign, offset);
@@ -526,7 +526,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ISENABLER ... GICD_ISENABLERN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISENABLER);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISENABLER);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
         tr = rank->ienable;
@@ -537,7 +537,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICENABLER);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICENABLER);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
         tr = rank->ienable;
@@ -560,7 +560,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ISACTIVER ... GICD_ISACTIVERN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ISACTIVER);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ISACTIVER);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
         rank->iactive &= ~*r;
@@ -569,7 +569,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ICACTIVER ... GICD_ICACTIVERN:
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICACTIVER);
+        rank = vgic_rank_offset(v, 1, gicd_reg - GICD_ICACTIVER);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
         rank->iactive &= ~*r;
@@ -582,7 +582,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_ITARGETSR + 8 ... GICD_ITARGETSRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
+        rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
         if ( dabt.size == 2 )
@@ -595,7 +595,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 
     case GICD_IPRIORITYR ... GICD_IPRIORITYRN:
         if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 8, gicd_reg - GICD_IPRIORITYR);
+        rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
         if ( dabt.size == 2 )
@@ -613,7 +613,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         goto write_ignore;
     case GICD_ICFGR + 2 ... GICD_ICFGRN: /* SPIs */
         if ( dabt.size != 2 ) goto bad_width;
-        rank = vgic_irq_rank(v, 2, gicd_reg - GICD_ICFGR);
+        rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
         rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR)] = *r;
@@ -720,7 +720,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 {
     int idx = irq >> 2, byte = irq & 0x3;
     uint8_t priority;
-    struct vgic_irq_rank *rank = vgic_irq_rank(v, 8, idx);
+    struct vgic_irq_rank *rank = vgic_rank_offset(v, 8, idx);
     struct pending_irq *iter, *n = irq_to_pending(v, irq);
     unsigned long flags;
     bool_t running;
-- 
1.7.10.4

[PATCH v5 2/6] xen/arm: introduce vgic_rank_irq

[Stefano Stabellini]

Introduce vgic_rank_irq: a new helper function that gives you the struct
vgic_irq_rank corresponding to a given irq number.
Use it in vgic_vcpu_inject_irq.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/vgic.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index f8cb3d5..757707e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -73,6 +73,11 @@ static struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n)
         return NULL;
 }
 
+static struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq)
+{
+    return vgic_rank_offset(v, 8, irq >> 2);
+}
+
 int domain_vgic_init(struct domain *d)
 {
     int i;
@@ -720,7 +725,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 {
     int idx = irq >> 2, byte = irq & 0x3;
     uint8_t priority;
-    struct vgic_irq_rank *rank = vgic_rank_offset(v, 8, idx);
+    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
     struct pending_irq *iter, *n = irq_to_pending(v, irq);
     unsigned long flags;
     bool_t running;
-- 
1.7.10.4

[PATCH v5 3/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>

---

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.

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

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 757707e..4d655af 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,
+               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));
+    }
     return 0;
 }
 
@@ -374,6 +380,32 @@ read_as_zero:
     return 1;
 }
 
+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);
+
+    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((const unsigned long *) &target, 8);
+    v_target = v->domain->vcpu[target];
+    return v_target;
+}
+
+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 +413,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 +438,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 +572,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 +583,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 +625,23 @@ 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 */
+        tr = (1 << v->domain->max_vcpus) - 1;
+        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
+        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 bf6fb1e..bd40628 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -227,6 +227,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 v5 4/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, we 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.

Using barriers and test_bit on GIC_IRQ_GUEST_MIGRATING, make sure that
vgic_migrate_irq can run at the same time as gic_update_one_lr on
different cpus with consistent results.

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

---

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           |   25 ++++++++++++++--
 xen/arch/arm/vgic.c          |   66 +++++++++++++++++++++++++++++++++++++-----
 xen/include/asm-arm/domain.h |    4 +++
 3 files changed, 85 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5e502df..8ed242e 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -677,10 +677,31 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         p->lr = GIC_INVALID_LR;
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
-             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
+             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
+             !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
             gic_raise_guest_irq(v, irq, p->priority);
-        else
+        else {
             list_del_init(&p->inflight);
+
+            /* inflight is cleared before clearing
+             * GIC_IRQ_GUEST_MIGRATING */
+            dsb(sy);
+            if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) &&
+                 test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
+            {
+                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 4d655af..e640de9 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -395,6 +395,25 @@ static 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) )
+        set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
+    spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+}
+
 struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
 {
     struct vcpu *v_target;
@@ -543,6 +562,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 )
     {
@@ -626,22 +647,45 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
         if ( rank == NULL) goto write_ignore;
         /* 8-bit vcpu mask for this domain */
-        tr = (1 << v->domain->max_vcpus) - 1;
-        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
-        tr &= *r;
+        trl = (1 << v->domain->max_vcpus) - 1;
+        if ( dabt.size == 2 )
+            trl = trl | (trl << 8) | (trl << 16) | (trl << 24);
+        else
+            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((const unsigned long *)&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((const unsigned long *) &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;
 
@@ -779,6 +823,12 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
+    if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &n->status) )
+    {
+        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
+        goto out;
+    }
+
     if ( !list_empty(&n->inflight) )
     {
         set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d689675..743c020 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -54,11 +54,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 v5 5/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 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        |    1 -
 xen/arch/arm/irq.c        |    3 +--
 xen/arch/arm/vgic.c       |    6 ++++++
 xen/include/asm-arm/gic.h |    1 +
 4 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 8ed242e..82a0be4 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -287,7 +287,6 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
     gic_set_irq_properties(desc->irq, level, cpumask_of(smp_processor_id()),
                            GIC_PRI_IRQ);
 
-    /* TODO: do not assume delivery to vcpu0 */
     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 a33c797..756250c 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -175,8 +175,7 @@ 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);
+        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 e640de9..2192a8c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -871,6 +871,12 @@ out:
         smp_send_event_check_mask(cpumask_of(v->processor));
 }
 
+void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq)
+{
+    struct vcpu *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 bd40628..8f09933 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 v5 6/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.

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 called by evtchn_move_pirqs.

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

---

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        |   42 ++++++++++++++++++++++++++++++++++++------
 xen/common/event_channel.c |    2 ++
 xen/include/asm-arm/gic.h  |    1 +
 xen/include/asm-x86/irq.h  |    2 ++
 5 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 82a0be4..a4422fd 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 2192a8c..696f9f4 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -395,6 +395,17 @@ static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
     return v_target;
 }
 
+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_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
 {
     unsigned long flags;
@@ -414,15 +425,30 @@ static void vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int ir
     spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
 }
 
-struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
+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;
-    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
+    int i;
 
-    vgic_lock_rank(v, rank);
-    v_target = _vgic_get_target_vcpu(v, irq);
-    vgic_unlock_rank(v, rank);
-    return v_target;
+    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)
@@ -480,6 +506,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);
@@ -667,6 +694,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);
@@ -678,6 +706,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/common/event_channel.c b/xen/common/event_channel.c
index 6853842..383057c 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1319,6 +1319,8 @@ void evtchn_move_pirqs(struct vcpu *v)
     unsigned int port;
     struct evtchn *chn;
 
+    arch_move_irqs(v);
+
     spin_lock(&d->event_lock);
     for ( port = v->pirq_evtchn_head; port; port = chn->u.pirq.next_port )
     {
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 8f09933..ce18cfe 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -229,6 +229,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

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

[Jan Beulich]

>>> Stefano Stabellini <stefano.stabellini@eu.citrix.com> 06/11/14 6:27 PM >>>
>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 called by evtchn_move_pirqs.
>
>Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>---
 >xen/arch/arm/gic.c         |   18 ++++++++++++++++--
 >xen/arch/arm/vgic.c        |   42 ++++++++++++++++++++++++++++++++++++------
 >xen/common/event_channel.c |    2 ++
 >xen/include/asm-arm/gic.h  |    1 +
 >xen/include/asm-x86/irq.h  |    2 ++
 >5 files changed, 57 insertions(+), 8 deletions(-)

Common and x86 part
Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v5 1/6] xen/arm: rename vgic_irq_rank to vgic_rank_offset

[Julien Grall]

Hi Stefano,

On 11/06/14 17:27, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

Regards,

-- 
Julien Grall

Re: [PATCH v5 2/6] xen/arm: introduce vgic_rank_irq

[Julien Grall]

Hi Stefano,

On 11/06/14 17:27, Stefano Stabellini wrote:
> Introduce vgic_rank_irq: a new helper function that gives you the struct
> vgic_irq_rank corresponding to a given irq number.
> Use it in vgic_vcpu_inject_irq.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

-- 
Julien Grall

Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Julien Grall]

Hi Stefano,

On 11/06/14 17:27, Stefano Stabellini wrote:
> +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)

[..]

> +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)

Can you add comment explaining what are the differences between these 2 
functions?

AFAIU, the first one is assuming the rank lock is taken. If so, I would 
add an ASSERT in it. I would avoid people misuse the 2 functions.

>       case GICD_ISPENDR ... GICD_ISPENDRN:
> @@ -589,12 +625,23 @@ 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)? Just for enforcement.

> +        tr = (1 << v->domain->max_vcpus) - 1;
> +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> +        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;

I quite difficult to understand this check. Does this check is only for 
word-access?

AFAIU, with byte-access it's possible to write 0 in itargets. For this 
case, the register may contain some 1 out of the byte offset.

Regards,

-- 
Julien Grall

Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

On Thu, 12 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/06/14 17:27, Stefano Stabellini wrote:
> > +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> 
> [..]
> 
> > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> 
> Can you add comment explaining what are the differences between these 2
> functions?
> 
> AFAIU, the first one is assuming the rank lock is taken. If so, I would add an
> ASSERT in it. I would avoid people misuse the 2 functions.

OK


> >       case GICD_ISPENDR ... GICD_ISPENDRN:
> > @@ -589,12 +625,23 @@ 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)? Just for enforcement.

OK


> > +        tr = (1 << v->domain->max_vcpus) - 1;
> > +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > +        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;
> 
> I quite difficult to understand this check. Does this check is only for
> word-access?

The previous test covers byte-access after the change of the following
patch. I should move it to this patch to make it clearer.


> AFAIU, with byte-access it's possible to write 0 in itargets. For this case,
> the register may contain some 1 out of the byte offset.

Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Julien Grall]

On 12/06/14 15:42, Stefano Stabellini wrote:
>>> +        tr = (1 << v->domain->max_vcpus) - 1;
>>> +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
>>> +        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;
>>
>> I quite difficult to understand this check. Does this check is only for
>> word-access?
>
> The previous test covers byte-access after the change of the following
> patch. I should move it to this patch to make it clearer.

Yes please.

Regards,

-- 
Julien Grall

Re: [PATCH v5 1/6] xen/arm: rename vgic_irq_rank to vgic_rank_offset

[Ian Campbell]

On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

vgic_rank_from_offset (_from_reg_offset) would have been nicer still,
but this is an improvement over the current name so:
        Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH v5 2/6] xen/arm: introduce vgic_rank_irq

[Ian Campbell]

On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> Introduce vgic_rank_irq: a new helper function that gives you the struct
> vgic_irq_rank corresponding to a given irq number.
> Use it in vgic_vcpu_inject_irq.

Thanks.

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

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

Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Ian Campbell]

On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> 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 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.
> 
> 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       |   71 +++++++++++++++++++++++++++++++++++++--------
>  xen/include/asm-arm/gic.h |    2 ++
>  2 files changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 757707e..4d655af 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,
> +               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));

8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]) ==
sizeof(d->arch.vgic.shared_irqs[i].itargets) doesn't it?

> +    }
>      return 0;
>  }
>  
> @@ -374,6 +380,32 @@ read_as_zero:
>      return 1;
>  }
>  
> +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);
> +
> +    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((const unsigned long *) &target, 8);

Is this definitely aligned ok?

Also the cast isn't necessary, the type is already unsigned long * and
if find_first_bit takes a const that'll happen automatically.

> @@ -589,12 +625,23 @@ 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 */
> +        tr = (1 << v->domain->max_vcpus) - 1;
> +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> +        tr &= *r;
> +        /* ignore zero writes */
> +        if ( !tr )
> +            goto write_ignore;

Is this per the spec? Can you provide a reference. If not then I think
writing target==0 is the guest's problem.

> +        if ( dabt.size == 2 &&
> +            !((tr & 0xff) && (tr & (0xff << 8)) &&
> +             (tr & (0xff << 16)) && (tr & (0xff << 24))))
> +            goto write_ignore;

Isn't this just !(tr & 0xffffffff) ? Even then I'm not sure what it is
actually for.

>          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 bf6fb1e..bd40628 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -227,6 +227,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
>  

Re: [PATCH v5 4/6] xen/arm: inflight irqs during migration

[Ian Campbell]

On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> 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, we 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.
> 
> Using barriers and test_bit on GIC_IRQ_GUEST_MIGRATING, make sure that
> vgic_migrate_irq can run at the same time as gic_update_one_lr on
> different cpus with consistent results.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> 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           |   25 ++++++++++++++--
>  xen/arch/arm/vgic.c          |   66 +++++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-arm/domain.h |    4 +++
>  3 files changed, 85 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 5e502df..8ed242e 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -677,10 +677,31 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>          clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>          p->lr = GIC_INVALID_LR;
>          if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> -             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
> +             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
> +             !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
>              gic_raise_guest_irq(v, irq, p->priority);
> -        else
> +        else {
>              list_del_init(&p->inflight);
> +
> +            /* inflight is cleared before clearing
> +             * GIC_IRQ_GUEST_MIGRATING */
> +            dsb(sy);

Is sy really necessary? THis is all about state stored in RAM not the
actual GIC, right? I think "ish" is probably sufficient. POssibly even a
dmb of some sort.

> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 4d655af..e640de9 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -543,6 +562,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 )
>      {
> @@ -626,22 +647,45 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
>          if ( rank == NULL) goto write_ignore;
>          /* 8-bit vcpu mask for this domain */
> -        tr = (1 << v->domain->max_vcpus) - 1;
> -        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> -        tr &= *r;
> +        trl = (1 << v->domain->max_vcpus) - 1;
> +        if ( dabt.size == 2 )
> +            trl = trl | (trl << 8) | (trl << 16) | (trl << 24);
> +        else
> +            trl = (trl << (8 * (offset & 0x3)));
> +        trl &= *r;

What does trl stand for? Why is it an unsigned long when r and tr are
uint32_t?

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

I'm still not sure what this is ;-)

>          vgic_lock_rank(v, rank);
> +        i = 0;
> +        while ( (i = find_next_bit((const unsigned long *)&trl, 32, i)) < 32 )

Unnecessary cast (if trl really should be an unsigned long).

> +        {
> +            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((const unsigned long *) &old_target_mask, 8);

Another unnecessary cast.

> +
> +            if ( target != old_target )

Given the implementation only supports a single target I'm wondering if
maybe we should just store the configured target.

If we weren't required to read back the actual value then we could even
dispense with rank->itargets completely and just fabricate it.

> +            {
> +                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;

I think this whole loop would be less subtle as:

	for ( byte = 0 ; byte < 4 ; byte++ )
		target = find_first_bit((char *)(&tr)+byte, 8)
		if ( target > 8 ) continue;
		old_target  =...
	}

The use of find_next_bit is not really semantically what is happening
here.

Ian.

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

[Ian Campbell]

On Wed, 2014-06-11 at 17:27 +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>
> 
> ---
> 
> 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        |    1 -
>  xen/arch/arm/irq.c        |    3 +--
>  xen/arch/arm/vgic.c       |    6 ++++++
>  xen/include/asm-arm/gic.h |    1 +
>  4 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 8ed242e..82a0be4 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -287,7 +287,6 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
>      gic_set_irq_properties(desc->irq, level, cpumask_of(smp_processor_id()),
>                             GIC_PRI_IRQ);
>  
> -    /* TODO: do not assume delivery to vcpu0 */

Replace with "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 a33c797..756250c 100644
> --- a/xen/arch/arm/irq.c
> +++ b/xen/arch/arm/irq.c
> @@ -175,8 +175,7 @@ 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);
> +        vgic_vcpu_inject_spi(d, irq);

This needs an assert (or at least a comment) that the irq is not a PPI
because IRQ_GUEST is set and such things cannot be PPIs right now (and
that last subtlety needs a comment even with the assert)

>          goto out_no_end;
>      }
>  
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index e640de9..2192a8c 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -871,6 +871,12 @@ out:
>          smp_send_event_check_mask(cpumask_of(v->processor));
>  }
>  
> +void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq)
> +{

Or maybe an ASSERT is needed here instead, or as well.

I'm almost inclined to suggest that vgic_get_target_vcpu should take
both d and v and that this caller should pass v==NULL and
vgic_get_target_vcpu should assert that IRQ is a SPI when v==NULL.

> +    struct vcpu *v = vgic_get_target_vcpu(d->vcpu[0], irq);
> +    vgic_vcpu_inject_irq(v, irq);
> +}
> +

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

[Ian Campbell]

On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> +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;
> +}

Looks like you just moved this? Did it also change? Please can you
either introduce it in the right place in earlier patch or leave it
where it is in this one.

> @@ -678,6 +706,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 think vgic_migrate_irq should take care of this stuff too. Any reson
not to?

> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 6853842..383057c 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1319,6 +1319,8 @@ void evtchn_move_pirqs(struct vcpu *v)
>      unsigned int port;
>      struct evtchn *chn;
>  
> +    arch_move_irqs(v);

It seems odd to do this from event_channel.c

I suggest adding sched_move_irqs to scheduler.c and having it call
evtchn_move_pirqs and arch_move_irqs and then replace all the existing
calls to evtchn_move_pirqs with it.

Re: [PATCH v5 0/6] vgic emulation and GICD_ITARGETSR

[Ian Campbell]

On Wed, 2014-06-11 at 17:25 +0100, Stefano Stabellini wrote:
> Stefano Stabellini (6):
>       xen/arm: rename vgic_irq_rank to vgic_rank_offset
>       xen/arm: introduce vgic_rank_irq

Applied these two.

Ian

Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

On Wed, 18 Jun 2014, Ian Campbell wrote:
> On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> > 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 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.
> > 
> > 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       |   71 +++++++++++++++++++++++++++++++++++++--------
> >  xen/include/asm-arm/gic.h |    2 ++
> >  2 files changed, 61 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 757707e..4d655af 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,
> > +               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));
> 
> 8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]) ==
> sizeof(d->arch.vgic.shared_irqs[i].itargets) doesn't it?

Yeah, I'll make the change.


> > +    }
> >      return 0;
> >  }
> >  
> > @@ -374,6 +380,32 @@ read_as_zero:
> >      return 1;
> >  }
> >  
> > +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);
> > +
> > +    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((const unsigned long *) &target, 8);
> 
> Is this definitely aligned ok?

I think so: target is unsigned long and find_first_bit expects an
unsigned long. The problem is when target is a uint32_t or an unsigned
int and we cast it to unsigned long on armv8.


> Also the cast isn't necessary, the type is already unsigned long * and
> if find_first_bit takes a const that'll happen automatically.

OK


> > @@ -589,12 +625,23 @@ 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 */
> > +        tr = (1 << v->domain->max_vcpus) - 1;
> > +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > +        tr &= *r;
> > +        /* ignore zero writes */
> > +        if ( !tr )
> > +            goto write_ignore;
> 
> Is this per the spec? Can you provide a reference. If not then I think
> writing target==0 is the guest's problem.

The reference is 4.3.12 of the GIC architecture specification but
unfortunately it is not clearly written how the gic should behave in
case of 0 writes to GICD_ITARGETSR.

I wrote the patch thinking that a 0 write is invalid and therefore
should be ignored. Now I am not sure anymore. You could effectively
disable an interrupt by writing 0 to GICD_ITARGETSR. Why do you even
need GICD_ICENABLER in that case?

That said, the current code cannot deal with itargets being 0, so this
check is needed for security. I could change the code to be able to deal
with invalid values or 0 values to itargets, then we could remove the
check. I am not sure what is the best course of action.


> > +        if ( dabt.size == 2 &&
> > +            !((tr & 0xff) && (tr & (0xff << 8)) &&
> > +             (tr & (0xff << 16)) && (tr & (0xff << 24))))
> > +            goto write_ignore;
> 
> Isn't this just !(tr & 0xffffffff) ? Even then I'm not sure what it is
> actually for.

tr & 0xffffffff

is non-zero if any of the bytes in tr is non-zero.

((tr & 0xff) && (tr & (0xff << 8)) && (tr & (0xff << 16)) && (tr & (0xff << 24)))

is non-zero if all the bytes in tr are non-zero.


> >          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 bf6fb1e..bd40628 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -227,6 +227,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
> >  
> 
> 

Re: [PATCH v5 4/6] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Wed, 18 Jun 2014, Ian Campbell wrote:
> On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> > 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, we 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.
> > 
> > Using barriers and test_bit on GIC_IRQ_GUEST_MIGRATING, make sure that
> > vgic_migrate_irq can run at the same time as gic_update_one_lr on
> > different cpus with consistent results.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > 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           |   25 ++++++++++++++--
> >  xen/arch/arm/vgic.c          |   66 +++++++++++++++++++++++++++++++++++++-----
> >  xen/include/asm-arm/domain.h |    4 +++
> >  3 files changed, 85 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 5e502df..8ed242e 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -677,10 +677,31 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >          clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> >          p->lr = GIC_INVALID_LR;
> >          if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > -             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
> > +             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
> > +             !test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
> >              gic_raise_guest_irq(v, irq, p->priority);
> > -        else
> > +        else {
> >              list_del_init(&p->inflight);
> > +
> > +            /* inflight is cleared before clearing
> > +             * GIC_IRQ_GUEST_MIGRATING */
> > +            dsb(sy);
> 
> Is sy really necessary? THis is all about state stored in RAM not the
> actual GIC, right? I think "ish" is probably sufficient. POssibly even a
> dmb of some sort.

Yeah, it is only about the state in RAM. It only needs to be visible
across all the cores, not the devices. So I guess the right function to
call would be smp_wmb() that translates to dmb(ishst)?
That makes me think that I should add smp_rmb() to the corresponding
critical section: vgic_migrate_irq.


> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 4d655af..e640de9 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -543,6 +562,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 )
> >      {
> > @@ -626,22 +647,45 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >          rank = vgic_rank_offset(v, 8, gicd_reg - GICD_ITARGETSR);
> >          if ( rank == NULL) goto write_ignore;
> >          /* 8-bit vcpu mask for this domain */
> > -        tr = (1 << v->domain->max_vcpus) - 1;
> > -        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > -        tr &= *r;
> > +        trl = (1 << v->domain->max_vcpus) - 1;
> > +        if ( dabt.size == 2 )
> > +            trl = trl | (trl << 8) | (trl << 16) | (trl << 24);
> > +        else
> > +            trl = (trl << (8 * (offset & 0x3)));
> > +        trl &= *r;
> 
> What does trl stand for? Why is it an unsigned long when r and tr are
> uint32_t?

It is an unfortunate name. trl is exactly like tr but unsigned long
instead of uint32_t.
It is unsigned long so that it can be used as an argument of
find_next_bit below. uint32_t cannot be used because it doesn't have the
right alignment on armv8.


> >          /* 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;
> 
> I'm still not sure what this is ;-)

All the bytes in trl needs to be non-zero for the write to be valid
(considering a 0 write to any of the byte fields as invalid).


> >          vgic_lock_rank(v, rank);
> > +        i = 0;
> > +        while ( (i = find_next_bit((const unsigned long *)&trl, 32, i)) < 32 )
> 
> Unnecessary cast (if trl really should be an unsigned long).

The argument to find_next_bit really needs to be unsigned long. I'll
remove the cast.


> > +        {
> > +            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((const unsigned long *) &old_target_mask, 8);
> 
> Another unnecessary cast.

Right.


> > +
> > +            if ( target != old_target )
> 
> Given the implementation only supports a single target I'm wondering if
> maybe we should just store the configured target.
> 
> If we weren't required to read back the actual value then we could even
> dispense with rank->itargets completely and just fabricate it.

Yeah, that's a good point, I am not sure.
The spec says that it is valid to statically configure GICD_ITARGETSR
and ignore any writes to it, but it doesn't say that we can ignore
/some/ writes to it.


> > +            {
> > +                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;
> 
> I think this whole loop would be less subtle as:
> 
> 	for ( byte = 0 ; byte < 4 ; byte++ )
> 		target = find_first_bit((char *)(&tr)+byte, 8)

That would have alignment issues, wouldn't it?


> 		if ( target > 8 ) continue;
> 		old_target  =...
> 	}
> 
> The use of find_next_bit is not really semantically what is happening
> here.

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

[Stefano Stabellini]

On Wed, 18 Jun 2014, Ian Campbell wrote:
> On Wed, 2014-06-11 at 17:27 +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>
> > 
> > ---
> > 
> > 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        |    1 -
> >  xen/arch/arm/irq.c        |    3 +--
> >  xen/arch/arm/vgic.c       |    6 ++++++
> >  xen/include/asm-arm/gic.h |    1 +
> >  4 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 8ed242e..82a0be4 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -287,7 +287,6 @@ void gic_route_irq_to_guest(struct domain *d, struct irq_desc *desc,
> >      gic_set_irq_properties(desc->irq, level, cpumask_of(smp_processor_id()),
> >                             GIC_PRI_IRQ);
> >  
> > -    /* TODO: do not assume delivery to vcpu0 */
> 
> Replace with "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 a33c797..756250c 100644
> > --- a/xen/arch/arm/irq.c
> > +++ b/xen/arch/arm/irq.c
> > @@ -175,8 +175,7 @@ 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);
> > +        vgic_vcpu_inject_spi(d, irq);
> 
> This needs an assert (or at least a comment) that the irq is not a PPI
> because IRQ_GUEST is set and such things cannot be PPIs right now (and
> that last subtlety needs a comment even with the assert)
> 
> >          goto out_no_end;
> >      }
> >  
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index e640de9..2192a8c 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -871,6 +871,12 @@ out:
> >          smp_send_event_check_mask(cpumask_of(v->processor));
> >  }
> >  
> > +void vgic_vcpu_inject_spi(struct domain *d, unsigned int irq)
> > +{
> 
> Or maybe an ASSERT is needed here instead, or as well.
> 
> I'm almost inclined to suggest that vgic_get_target_vcpu should take
> both d and v and that this caller should pass v==NULL and
> vgic_get_target_vcpu should assert that IRQ is a SPI when v==NULL.

I prefer to assert in vgic_vcpu_inject_spi


> > +    struct vcpu *v = vgic_get_target_vcpu(d->vcpu[0], irq);
> > +    vgic_vcpu_inject_irq(v, irq);
> > +}
> > +
> 
> 

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

[Stefano Stabellini]

On Wed, 18 Jun 2014, Ian Campbell wrote:
> On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> > +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;
> > +}
> 
> Looks like you just moved this? Did it also change? Please can you
> either introduce it in the right place in earlier patch or leave it
> where it is in this one.

Strange, this chuck shouldn't be here at all.


> > @@ -678,6 +706,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 think vgic_migrate_irq should take care of this stuff too. Any reson
> not to?

This covers a different case: if the guest moves an irq from vcpu0 to
vcpu1, we want to also move the corresponding physical irq from the pcpu
that is running vcpu0 to the pcpu that is running vcpu1.


> > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > index 6853842..383057c 100644
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -1319,6 +1319,8 @@ void evtchn_move_pirqs(struct vcpu *v)
> >      unsigned int port;
> >      struct evtchn *chn;
> >  
> > +    arch_move_irqs(v);
> 
> It seems odd to do this from event_channel.c
> 
> I suggest adding sched_move_irqs to scheduler.c and having it call
> evtchn_move_pirqs and arch_move_irqs and then replace all the existing
> calls to evtchn_move_pirqs with it.
 
I am OK with that.

Re: [PATCH v5 3/6] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Stefano Stabellini]

On Thu, 19 Jun 2014, Stefano Stabellini wrote:
> On Wed, 18 Jun 2014, Ian Campbell wrote:
> > On Wed, 2014-06-11 at 17:27 +0100, Stefano Stabellini wrote:
> > > 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 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.
> > > 
> > > 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       |   71 +++++++++++++++++++++++++++++++++++++--------
> > >  xen/include/asm-arm/gic.h |    2 ++
> > >  2 files changed, 61 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 757707e..4d655af 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,
> > > +               8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]));
> > 
> > 8*sizeof(d->arch.vgic.shared_irqs[i].itargets[0]) ==
> > sizeof(d->arch.vgic.shared_irqs[i].itargets) doesn't it?
> 
> Yeah, I'll make the change.
> 
> 
> > > +    }
> > >      return 0;
> > >  }
> > >  
> > > @@ -374,6 +380,32 @@ read_as_zero:
> > >      return 1;
> > >  }
> > >  
> > > +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);
> > > +
> > > +    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((const unsigned long *) &target, 8);
> > 
> > Is this definitely aligned ok?
> 
> I think so: target is unsigned long and find_first_bit expects an
> unsigned long. The problem is when target is a uint32_t or an unsigned
> int and we cast it to unsigned long on armv8.
> 
> 
> > Also the cast isn't necessary, the type is already unsigned long * and
> > if find_first_bit takes a const that'll happen automatically.
> 
> OK
> 
> 
> > > @@ -589,12 +625,23 @@ 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 */
> > > +        tr = (1 << v->domain->max_vcpus) - 1;
> > > +        tr = tr | (tr << 8) | (tr << 16) | (tr << 24);
> > > +        tr &= *r;
> > > +        /* ignore zero writes */
> > > +        if ( !tr )
> > > +            goto write_ignore;
> > 
> > Is this per the spec? Can you provide a reference. If not then I think
> > writing target==0 is the guest's problem.
> 
> The reference is 4.3.12 of the GIC architecture specification but
> unfortunately it is not clearly written how the gic should behave in
> case of 0 writes to GICD_ITARGETSR.
> 
> I wrote the patch thinking that a 0 write is invalid and therefore
> should be ignored. Now I am not sure anymore. You could effectively
> disable an interrupt by writing 0 to GICD_ITARGETSR. Why do you even
> need GICD_ICENABLER in that case?
> 
> That said, the current code cannot deal with itargets being 0, so this
> check is needed for security. I could change the code to be able to deal
> with invalid values or 0 values to itargets, then we could remove the
> check. I am not sure what is the best course of action.


I confirm that a real GICv2 (the one on Midway) ignores 0 writes. I
strongly suggest we do the same.