[PATCH v4 0/4] vgic emulation and GICD_ITARGETSR

[PATCH v4 0/4] 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
always vcpu0 for irq >= 32.

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.


Stefano Stabellini (4):
      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           |   42 ++++++++++++--
 xen/arch/arm/irq.c           |    3 +-
 xen/arch/arm/vgic.c          |  127 +++++++++++++++++++++++++++++++++++++++---
 xen/common/event_channel.c   |    4 ++
 xen/include/asm-arm/domain.h |    4 ++
 xen/include/asm-arm/gic.h    |    3 +
 6 files changed, 168 insertions(+), 15 deletions(-)

[PATCH v4 1/4] 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.

Validate writes to GICD_ITARGETSR.

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

---

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

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index cb8df3a..e527892 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -106,7 +106,15 @@ 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++)
+    {
+        int j;
+
         spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
+        /* Only delivery to CPU0 */
+        for ( j = 0 ; j < 8 ; j++ )
+            d->arch.vgic.shared_irqs[i].itargets[j] =
+                (1<<0) | (1<<8) | (1<<16) | (1<<24);
+    }
     return 0;
 }
 
@@ -369,6 +377,22 @@ read_as_zero:
     return 1;
 }
 
+struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
+{
+    int target;
+    struct vgic_irq_rank *rank;
+    struct vcpu *v_target;
+
+    rank = vgic_irq_rank(v, 1, irq/32);
+    vgic_lock_rank(v, rank);
+    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
+    /* just return the first vcpu in the mask */
+    target = find_next_bit((const unsigned long *) &target, 8, 0);
+    v_target = v->domain->vcpu[target];
+    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;
@@ -376,12 +400,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);
@@ -399,24 +425,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 )
         {
@@ -502,6 +530,7 @@ 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;
+    int i;
 
     switch ( gicd_reg )
     {
@@ -585,6 +614,21 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
+        tr = *r & ~(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)]);
+        i = 0;
+        /* validate writes */
+        while ( (i = find_next_bit((const unsigned long *) &tr, 32, i)) < 32 )
+        {
+            unsigned int target = i % 8;
+            if ( target > v->domain->max_vcpus )
+            {
+                gdprintk(XENLOG_WARNING, "vGICD: GICD_ITARGETSR write invalid target vcpu %u\n",
+                         target);
+                vgic_unlock_rank(v, rank);
+                return 1;
+            }
+            i++;
+        }
         if ( dabt.size == 2 )
             rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;
         else
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 v4 2/4] 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. In fact 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 other 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 and take the new vcpu lock.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic.c           |   23 +++++++++++++++++++++--
 xen/arch/arm/vgic.c          |   36 ++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/domain.h |    4 ++++
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 08ae23b..92391b4 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -677,10 +677,29 @@ 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);
+
+            if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) &&
+                 test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
+            {
+                struct vcpu *v_target;
+
+                spin_unlock(&v->arch.vgic.lock);
+                v_target = vgic_get_target_vcpu(v, irq);
+                spin_lock(&v_target->arch.vgic.lock);
+
+                gic_add_to_lr_pending(v_target, p);
+                if ( v_target->is_running )
+                    smp_send_event_check_mask(cpumask_of(v_target->processor));
+
+                spin_unlock(&v_target->arch.vgic.lock);
+                spin_lock(&v->arch.vgic.lock);
+            }
+        }
     }
 }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index e527892..54d3676 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -377,6 +377,21 @@ read_as_zero:
     return 1;
 }
 
+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;
+    
+    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)
 {
     int target;
@@ -629,6 +644,21 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
             }
             i++;
         }
+        i = 0;
+        while ( (i = find_next_bit((const unsigned long *) &tr, 32, i)) < 32 )
+        {
+            unsigned int irq, target, old_target;
+            struct vcpu *v_target, *v_old;
+
+            target = i % 8;
+
+            irq = offset + (i / 8);
+            v_target = v->domain->vcpu[target];
+            old_target = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
+            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)] = *r;
         else
@@ -771,6 +801,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 v4 3/4] 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 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 92391b4..6f24b14 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..0fad647 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_irq(vgic_get_target_vcpu(d->vcpu[0], irq), irq);
         goto out_no_end;
     }
 
-- 
1.7.10.4

[PATCH v4 4/4] 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 hook in common code to call the vgic irq migration code
as evtchn_move_pirqs only deals with event channels at the moment.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: JBeulich@suse.com
---
 xen/arch/arm/gic.c         |   18 ++++++++++++++++--
 xen/arch/arm/vgic.c        |   31 +++++++++++++++++++++++++++++++
 xen/common/event_channel.c |    4 ++++
 xen/include/asm-arm/gic.h  |    1 +
 4 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6f24b14..43bef21 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 54d3676..a90d9cb 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -408,6 +408,32 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
     return v_target;
 }
 
+void vgic_move_irqs(struct vcpu *v)
+{
+    const cpumask_t *cpu_mask = cpumask_of(v->processor);
+    struct domain *d = v->domain;
+    struct pending_irq *p;
+    int i, j, k;
+
+    for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
+    {
+        for ( j = 0 ; j < 8 ; j++ )
+        {
+            for ( k = 0; k < 4; k++ )
+            {
+                uint8_t target = byte_read(d->arch.vgic.shared_irqs[i].itargets[j], 0, k);
+                target = find_next_bit((const unsigned long *) &target, 8, 0);
+                if ( target == v->vcpu_id )
+                {
+                    p = irq_to_pending(v, 32 * (i + 1) + (j * 4) + k);
+                    if ( p->desc != NULL )
+                        p->desc->handler->set_affinity(p->desc, cpu_mask);
+                }
+            }
+        }
+    }
+}
+
 static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     const unsigned long mask = r;
@@ -463,6 +489,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         }
         if ( p->desc != NULL )
         {
+            p->desc->handler->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);
@@ -649,6 +676,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         {
             unsigned int irq, target, old_target;
             struct vcpu *v_target, *v_old;
+            struct pending_irq *p;
 
             target = i % 8;
 
@@ -657,6 +685,9 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
             old_target = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
             v_old = v->domain->vcpu[old_target];
             vgic_migrate_irq(v_old, v_target, irq);
+            p = irq_to_pending(v_target, irq);
+            if ( p->desc != NULL )
+                p->desc->handler->set_affinity(p->desc, cpumask_of(v_target->processor));
             i += 8 - target;
         }
         if ( dabt.size == 2 )
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index 6853842..226321d 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1319,6 +1319,10 @@ void evtchn_move_pirqs(struct vcpu *v)
     unsigned int port;
     struct evtchn *chn;
 
+#ifdef CONFIG_ARM
+	vgic_move_irqs(v);
+#endif
+
     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 bd40628..8f457dd 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 vgic_move_irqs(struct vcpu *v);
 
 #endif /* __ASSEMBLY__ */
 #endif
-- 
1.7.10.4

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

[Julien Grall]

Hi Stefano,

On 06/06/14 18:48, Stefano Stabellini wrote:
>       return 0;
>   }
>
> @@ -369,6 +377,22 @@ read_as_zero:
>       return 1;
>   }
>
> +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> +{
> +    int target;
> +    struct vgic_irq_rank *rank;
> +    struct vcpu *v_target;
> +
> +    rank = vgic_irq_rank(v, 1, irq/32);
> +    vgic_lock_rank(v, rank);
> +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> +    /* just return the first vcpu in the mask */
> +    target = find_next_bit((const unsigned long *) &target, 8, 0);

int* and unsigned long* doesn't have the same alignment on aarch64. This 
may end up to a data abort for Xen side.

IIRC, Ian has fixed a similar issue in commit 5224a733.

[..]

>           }
>           if ( p->desc != NULL )
>           {
> @@ -502,6 +530,7 @@ 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;
> +    int i;
>
>       switch ( gicd_reg )
>       {
> @@ -585,6 +614,21 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>           rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
>           if ( rank == NULL) goto write_ignore;
>           vgic_lock_rank(v, rank);
> +        tr = *r & ~(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)]);

Write in GICD_ITARGETSR can be either half-word or word. If I'm not 
mistaken you sanity check only handle word access.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Sat, 7 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/06/14 18:48, Stefano Stabellini wrote:
> >       return 0;
> >   }
> > 
> > @@ -369,6 +377,22 @@ read_as_zero:
> >       return 1;
> >   }
> > 
> > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> > +{
> > +    int target;
> > +    struct vgic_irq_rank *rank;
> > +    struct vcpu *v_target;
> > +
> > +    rank = vgic_irq_rank(v, 1, irq/32);
> > +    vgic_lock_rank(v, rank);
> > +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> > +    /* just return the first vcpu in the mask */
> > +    target = find_next_bit((const unsigned long *) &target, 8, 0);
> 
> int* and unsigned long* doesn't have the same alignment on aarch64. This may
> end up to a data abort for Xen side.
> 
> IIRC, Ian has fixed a similar issue in commit 5224a733.
> 

Well spotted! Thanks!


> 
> >           }
> >           if ( p->desc != NULL )
> >           {
> > @@ -502,6 +530,7 @@ 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;
> > +    int i;
> > 
> >       switch ( gicd_reg )
> >       {
> > @@ -585,6 +614,21 @@ static int vgic_distr_mmio_write(struct vcpu *v,
> > mmio_info_t *info)
> >           rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
> >           if ( rank == NULL) goto write_ignore;
> >           vgic_lock_rank(v, rank);
> > +        tr = *r & ~(rank->itargets[REG_RANK_INDEX(8, gicd_reg -
> > GICD_ITARGETSR)]);
> 
> Write in GICD_ITARGETSR can be either half-word or word. If I'm not mistaken
> you sanity check only handle word access.

I realize that it is a bit tricky to read, but this works for both word
and half-word accesses.

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

[Julien Grall]

On 06/09/2014 11:47 AM, Stefano Stabellini wrote:
>> Write in GICD_ITARGETSR can be either half-word or word. If I'm not mistaken
>> you sanity check only handle word access.
> 
> I realize that it is a bit tricky to read, but this works for both word
> and half-word accesses.

I think you have to mask the unused bits in the register. We can't
assume that they will be all-zeroed.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Mon, 9 Jun 2014, Julien Grall wrote:
> On 06/09/2014 11:47 AM, Stefano Stabellini wrote:
> >> Write in GICD_ITARGETSR can be either half-word or word. If I'm not mistaken
> >> you sanity check only handle word access.
> > 
> > I realize that it is a bit tricky to read, but this works for both word
> > and half-word accesses.
> 
> I think you have to mask the unused bits in the register. We can't
> assume that they will be all-zeroed.

That's a good hint but I already pass 32 to find_next_bit as max bit to
search for. The rest is ignored.

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

[Julien Grall]

On 06/09/2014 01:04 PM, Stefano Stabellini wrote:
> On Mon, 9 Jun 2014, Julien Grall wrote:
>> On 06/09/2014 11:47 AM, Stefano Stabellini wrote:
>>>> Write in GICD_ITARGETSR can be either half-word or word. If I'm not mistaken
>>>> you sanity check only handle word access.
>>>
>>> I realize that it is a bit tricky to read, but this works for both word
>>> and half-word accesses.

Hmmm... I meant byte access not half-word. Sorry.

>> I think you have to mask the unused bits in the register. We can't
>> assume that they will be all-zeroed.
> 
> That's a good hint but I already pass 32 to find_next_bit as max bit to
> search for. The rest is ignored.
> 

So if the byte is equal 0, then you can reach a one bit in the unused
part...

-- 
Julien Grall

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

[Stefano Stabellini]

On Mon, 9 Jun 2014, Julien Grall wrote:
> On 06/09/2014 01:04 PM, Stefano Stabellini wrote:
> > On Mon, 9 Jun 2014, Julien Grall wrote:
> >> On 06/09/2014 11:47 AM, Stefano Stabellini wrote:
> >>>> Write in GICD_ITARGETSR can be either half-word or word. If I'm not mistaken
> >>>> you sanity check only handle word access.
> >>>
> >>> I realize that it is a bit tricky to read, but this works for both word
> >>> and half-word accesses.
> 
> Hmmm... I meant byte access not half-word. Sorry.
> 
> >> I think you have to mask the unused bits in the register. We can't
> >> assume that they will be all-zeroed.
> > 
> > That's a good hint but I already pass 32 to find_next_bit as max bit to
> > search for. The rest is ignored.
> > 
> 
> So if the byte is equal 0, then you can reach a one bit in the unused
> part...
 
I'll test for that even though is going to make the code a bit uglier.

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

[Ian Campbell]

On Fri, 2014-06-06 at 18:48 +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.
> 
> Validate writes to GICD_ITARGETSR.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> 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       |   60 +++++++++++++++++++++++++++++++++++++++------
>  xen/include/asm-arm/gic.h |    2 ++
>  2 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index cb8df3a..e527892 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -106,7 +106,15 @@ 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++)
> +    {
> +        int j;
> +
>          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> +        /* Only delivery to CPU0 */

s/delivery/deliver/.

And I think you should prefix it with "By default..."

> +        for ( j = 0 ; j < 8 ; j++ )
> +            d->arch.vgic.shared_irqs[i].itargets[j] =
> +                (1<<0) | (1<<8) | (1<<16) | (1<<24);

Since these are bytes I think you could do:
        memset(d->arch.vgic.shared_irqs[i].itargets, 0x1, sizeof(...))

> +    }
>      return 0;
>  }
>  
> @@ -369,6 +377,22 @@ read_as_zero:
>      return 1;
>  }
>  
> +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> +{
> +    int target;
> +    struct vgic_irq_rank *rank;
> +    struct vcpu *v_target;
> +
> +    rank = vgic_irq_rank(v, 1, irq/32);

Please can you do what vgic_vcpu_inject_irq does to look up the rank. Or
even better add a helper function which goes from an actual irq number
to the rank instead from a register offset to a bank (which is what
vgic_irq_rank actually is, i.e. vigc_irq_rank_from_gicd_offset or
something)

> +    vgic_lock_rank(v, rank);
> +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> +    /* just return the first vcpu in the mask */

Is this a valid model for delivering an interrupt? I couldn't find
anything explicit in the GIC spec saying that it is valid for the
implementation to arbitrarily deliver to a single CPU.

The stuff in there about the 1-N case only deals with what happens when
one processor of the many does the IAR and what the others then see
(spurious IRQ).

To be clear: I don't object to this implementation but I think it should
either be backed up by reference to the spec or it should be explicitly
mentioned in a comment how/where/why we deviate from it.

> +    target = find_next_bit((const unsigned long *) &target, 8, 0);
> +    v_target = v->domain->vcpu[target];
> +    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;
> @@ -376,12 +400,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);

What locks are required to poke at another vcpu's state in this way? You
don't seem to take any locks relating to v_target, and I don't think any
of the callers take any global lock e.g. you have already dropped the
rank's lock in the caller (although I confess I'm looking at the current
tree not one with all your patches applied).

WRT dropping the rank lock -- shouldn't this be inside that anyway to
handle races between different vcpus enabling/disabling a single IRQ?

Which also needs care when v==v_target if you already hold any locks
relating to v.

>          if ( p->desc != NULL )
>          {
>              spin_lock_irqsave(&p->desc->lock, flags);
> @@ -399,24 +425,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);

Locking for some of this too (although I see in one case you do take the
v_target's gic lock).

> @@ -502,6 +530,7 @@ 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;
> +    int i;
>  
>      switch ( gicd_reg )
>      {
> @@ -585,6 +614,21 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
>          if ( rank == NULL) goto write_ignore;
>          vgic_lock_rank(v, rank);
> +        tr = *r & ~(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)]);
> +        i = 0;
> +        /* validate writes */
> +        while ( (i = find_next_bit((const unsigned long *) &tr, 32, i)) < 32 )
> +        {
> +            unsigned int target = i % 8;
> +            if ( target > v->domain->max_vcpus )
> +            {
> +                gdprintk(XENLOG_WARNING, "vGICD: GICD_ITARGETSR write invalid target vcpu %u\n",
> +                         target);

The spec says:
        A CPU targets field bit that corresponds to an unimplemented CPU
        interface is RAZ/WI.

So I think you can just implement the write with the existing code and
then mask the result in rank->itargets[] to clear any invalid CPUs,
which will be a lot simpler than this I think.

Ian.

Re: [PATCH v4 2/4] xen/arm: inflight irqs during migration

[Ian Campbell]

On Fri, 2014-06-06 at 18:48 +0100, Stefano Stabellini wrote:
> We need to take special care when migrating irqs that are already
> inflight from one vcpu to another. In fact the lr_pending and inflight
> lists are per-vcpu. The lock we take to protect them is also per-vcpu.

Please can we get some references to the GIC spec to clarify/justify the
expected behaviour when writing ITARGETSn while an affected interrupt is
pending.

> 
> 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 other vcpu.

other here being the vcpu which is currently handling or the new one
which will handle?

>  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 and take the new vcpu lock.

That might be true but I'm afraid it needs an argument to be made
(rather than an assertion) about why it is safe to drop one lock before
taking the other and why it is correct to do so etc.

In particular I'm not sure what happens if v_target is messing with
ITARGETS at the same time as all this is going on.

I think it also warrants a comment in the code too.

Given that these migrations ought to be rare(ish) it might be simpler
(at least in terms of reasoning about it) for the clearing vcpu to
enqueue the irq onto a dedicated migration list which has its own lock.
Then the new target vcpu could walk that list itself and move things
onto it's own lr_pending list.

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/gic.c           |   23 +++++++++++++++++++++--
>  xen/arch/arm/vgic.c          |   36 ++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/domain.h |    4 ++++
>  3 files changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 08ae23b..92391b4 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -677,10 +677,29 @@ 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);
> +
> +            if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) &&
> +                 test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
> +            {
> +                struct vcpu *v_target;
> +
> +                spin_unlock(&v->arch.vgic.lock);
> +                v_target = vgic_get_target_vcpu(v, irq);

Handle v == v_target?

> +                spin_lock(&v_target->arch.vgic.lock);
> +
> +                gic_add_to_lr_pending(v_target, p);
> +                if ( v_target->is_running )
> +                    smp_send_event_check_mask(cpumask_of(v_target->processor));

Don't you also need to vcpu_unblock if it is sleeping?

> +                spin_unlock(&v_target->arch.vgic.lock);
> +                spin_lock(&v->arch.vgic.lock);
> +            }
> +        }
>      }
>  }
>  
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index e527892..54d3676 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -377,6 +377,21 @@ read_as_zero:
>      return 1;
>  }
>  
> +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;
> +    
> +    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)
>  {
>      int target;
> @@ -629,6 +644,21 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>              }
>              i++;
>          }
> +        i = 0;
> +        while ( (i = find_next_bit((const unsigned long *) &tr, 32, i)) < 32 )

bit ops alignment issues?

I think you should use old_tr^new_tr to only process bits which have
changed.

> +        {
> +            unsigned int irq, target, old_target;
> +            struct vcpu *v_target, *v_old;
> +
> +            target = i % 8;
> +
> +            irq = offset + (i / 8);
> +            v_target = v->domain->vcpu[target];
> +            old_target = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
> +            v_old = v->domain->vcpu[old_target];

v_target and v_old might be the same.

> +            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)] = *r;
>          else

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

[Ian Campbell]

On Fri, 2014-06-06 at 18:48 +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.

You meant SPI I think?

What about PPIs?

> 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 +--
>  2 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 92391b4..6f24b14 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..0fad647 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_irq(vgic_get_target_vcpu(d->vcpu[0], irq), irq);

Would it make sense to push vgic_get_target_vcpu down into
vgic_vcpu_inject_irq rather than have all callers need to do it?

I'm also wondering if vgic_get_target_vcpu shouldn't take d and not v.

Does this do the right thing for PPIs? vgic_get_target_vcpu will just
lookup vcpu0's target, not the actual expected target, won't it?
(something else must deal with this, or it'd be broken already I
suppose)

Ian.

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

[Ian Campbell]

On Fri, 2014-06-06 at 18:48 +0100, Stefano Stabellini wrote:
> 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 hook in common code to call the vgic irq migration code
> as evtchn_move_pirqs only deals with event channels at the moment.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: JBeulich@suse.com
> ---
>  xen/arch/arm/gic.c         |   18 ++++++++++++++++--
>  xen/arch/arm/vgic.c        |   31 +++++++++++++++++++++++++++++++
>  xen/common/event_channel.c |    4 ++++
>  xen/include/asm-arm/gic.h  |    1 +
>  4 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 6f24b14..43bef21 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);

What does this lock actually protect against here? I think the only
thing which is externally visible is the write to bytereg and all the
inputs are deterministic I think. Perhaps a suitable barrier would
suffice? Or perhaps the caller ought to be holding the lock for some
other reason already.

> +
> +    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 54d3676..a90d9cb 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -408,6 +408,32 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
>      return v_target;
>  }
>  
> +void vgic_move_irqs(struct vcpu *v)
> +{
> +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
> +    struct domain *d = v->domain;
> +    struct pending_irq *p;
> +    int i, j, k;
> +
> +    for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> +    {
> +        for ( j = 0 ; j < 8 ; j++ )
> +        {
> +            for ( k = 0; k < 4; k++ )
> +            {
> +                uint8_t target = byte_read(d->arch.vgic.shared_irqs[i].itargets[j], 0, k);

i,j,k are pretty opaque here (and I wrote that before I saw the
machinations going on in the irq_to_pending call!).

Please just iterate over irqs and use the rank accessor functions to get
to the correct itargets to read. Which might be clearest with the
irq_to_rank helper I proposed on an earlier patch.

> +                target = find_next_bit((const unsigned long *) &target, 8, 0);

This is find_first_bit, I think.

It's just occurred to me that many of the 
	i = 0
	while (i = find_enxt_bit() )
loops I've been seeing in this series might be better following a
	for (i=find_first_bit; ; i=find_next_bit(...))
pattern.

> +                if ( target == v->vcpu_id )
> +                {
> +                    p = irq_to_pending(v, 32 * (i + 1) + (j * 4) + k);
> +                    if ( p->desc != NULL )
> +                        p->desc->handler->set_affinity(p->desc, cpu_mask);

A helper for this chain of indirections might be nice.
irq_set_affinity(desc, cpu_mask) of something.

> +                }
> +            }
> +        }
> +    }
> +}
> +
>  static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
>  {
>      const unsigned long mask = r;
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 6853842..226321d 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -1319,6 +1319,10 @@ void evtchn_move_pirqs(struct vcpu *v)
>      unsigned int port;
>      struct evtchn *chn;
>  
> +#ifdef CONFIG_ARM
> +	vgic_move_irqs(v);

Indent and/or hard vs soft tab.

And this should be arch_move_irqs I think, perhaps with a stub on x86
instead of an ifdef.

> +#endif
> +
>      spin_lock(&d->event_lock);
>      for ( port = v->pirq_evtchn_head; port; port = chn->u.pirq.next_port )
>      {

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

[Julien Grall]

On 06/10/2014 01:16 PM, Ian Campbell wrote:
> Does this do the right thing for PPIs? vgic_get_target_vcpu will just
> lookup vcpu0's target, not the actual expected target, won't it?
> (something else must deal with this, or it'd be broken already I
> suppose)

physical PPIs can't be routed to the guest. We don't have any support
for this such things and adding it will be a nightmare (a guest with
more VCPUs than the pCPUs...).

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Tue, 10 Jun 2014, Ian Campbell wrote:
> On Fri, 2014-06-06 at 18:48 +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.
> > 
> > Validate writes to GICD_ITARGETSR.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > 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       |   60 +++++++++++++++++++++++++++++++++++++++------
> >  xen/include/asm-arm/gic.h |    2 ++
> >  2 files changed, 54 insertions(+), 8 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index cb8df3a..e527892 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -106,7 +106,15 @@ 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++)
> > +    {
> > +        int j;
> > +
> >          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> > +        /* Only delivery to CPU0 */
> 
> s/delivery/deliver/.
> 
> And I think you should prefix it with "By default..."

OK


> > +        for ( j = 0 ; j < 8 ; j++ )
> > +            d->arch.vgic.shared_irqs[i].itargets[j] =
> > +                (1<<0) | (1<<8) | (1<<16) | (1<<24);
> 
> Since these are bytes I think you could do:
>         memset(d->arch.vgic.shared_irqs[i].itargets, 0x1, sizeof(...))

OK


> > +    }
> >      return 0;
> >  }
> >  
> > @@ -369,6 +377,22 @@ read_as_zero:
> >      return 1;
> >  }
> >  
> > +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> > +{
> > +    int target;
> > +    struct vgic_irq_rank *rank;
> > +    struct vcpu *v_target;
> > +
> > +    rank = vgic_irq_rank(v, 1, irq/32);
> 
> Please can you do what vgic_vcpu_inject_irq does to look up the rank. Or
> even better add a helper function which goes from an actual irq number
> to the rank instead from a register offset to a bank (which is what
> vgic_irq_rank actually is, i.e. vigc_irq_rank_from_gicd_offset or
> something)

I'll add a couple of patch to rename vgic_irq_rank and add a new helper
function.


> > +    vgic_lock_rank(v, rank);
> > +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> > +    /* just return the first vcpu in the mask */
> 
> Is this a valid model for delivering an interrupt? I couldn't find
> anything explicit in the GIC spec saying that it is valid for the
> implementation to arbitrarily deliver to a single CPU.
> 
> The stuff in there about the 1-N case only deals with what happens when
> one processor of the many does the IAR and what the others then see
> (spurious IRQ).
> 
> To be clear: I don't object to this implementation but I think it should
> either be backed up by reference to the spec or it should be explicitly
> mentioned in a comment how/where/why we deviate from it.

I evaluated the possibility of following the spec to the letter but it
would be far too slow in a virtual environment. I'll improve the
comment.


> > +    target = find_next_bit((const unsigned long *) &target, 8, 0);
> > +    v_target = v->domain->vcpu[target];
> > +    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;
> > @@ -376,12 +400,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);
> 
> What locks are required to poke at another vcpu's state in this way? You
> don't seem to take any locks relating to v_target, and I don't think any
> of the callers take any global lock e.g. you have already dropped the
> rank's lock in the caller (although I confess I'm looking at the current
> tree not one with all your patches applied).
> 
> WRT dropping the rank lock -- shouldn't this be inside that anyway to
> handle races between different vcpus enabling/disabling a single IRQ?
> 
> Which also needs care when v==v_target if you already hold any locks
> relating to v.

gic_remove_from_queues takes the vgic lock and clearing
GIC_IRQ_GUEST_ENABLED is an atomic operation.
But you are right: they need to be executed atomically.
I'll keep the rank lock for the duration of the operation.


> >          if ( p->desc != NULL )
> >          {
> >              spin_lock_irqsave(&p->desc->lock, flags);
> > @@ -399,24 +425,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);
> 
> Locking for some of this too (although I see in one case you do take the
> v_target's gic lock).
> 
> > @@ -502,6 +530,7 @@ 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;
> > +    int i;
> >  
> >      switch ( gicd_reg )
> >      {
> > @@ -585,6 +614,21 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >          rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
> >          if ( rank == NULL) goto write_ignore;
> >          vgic_lock_rank(v, rank);
> > +        tr = *r & ~(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)]);
> > +        i = 0;
> > +        /* validate writes */
> > +        while ( (i = find_next_bit((const unsigned long *) &tr, 32, i)) < 32 )
> > +        {
> > +            unsigned int target = i % 8;
> > +            if ( target > v->domain->max_vcpus )
> > +            {
> > +                gdprintk(XENLOG_WARNING, "vGICD: GICD_ITARGETSR write invalid target vcpu %u\n",
> > +                         target);
> 
> The spec says:
>         A CPU targets field bit that corresponds to an unimplemented CPU
>         interface is RAZ/WI.
> 
> So I think you can just implement the write with the existing code and
> then mask the result in rank->itargets[] to clear any invalid CPUs,
> which will be a lot simpler than this I think.

OK

Re: [PATCH v4 2/4] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Tue, 10 Jun 2014, Ian Campbell wrote:
> On Fri, 2014-06-06 at 18:48 +0100, Stefano Stabellini wrote:
> > We need to take special care when migrating irqs that are already
> > inflight from one vcpu to another. In fact the lr_pending and inflight
> > lists are per-vcpu. The lock we take to protect them is also per-vcpu.
> 
> Please can we get some references to the GIC spec to clarify/justify the
> expected behaviour when writing ITARGETSn while an affected interrupt is
> pending.

OK


> > 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 other vcpu.
> 
> other here being the vcpu which is currently handling or the new one
> which will handle?

Other is the old vcpu where we still need to clear the LR.
I made it clear in the commit message.


> >  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 and take the new vcpu lock.
> 
> That might be true but I'm afraid it needs an argument to be made
> (rather than an assertion) about why it is safe to drop one lock before
> taking the other and why it is correct to do so etc.
 
It is OK to drop the vgic lock of the current vcpu because we have
already dealt with the current LR. We don't need to keep the vgic lock
between LRs but we do for simplicity.

I'll improve the comments.


> In particular I'm not sure what happens if v_target is messing with
> ITARGETS at the same time as all this is going on.

vgic_get_target_vcpu takes the rank lock so the view of the target vcpu
is consistent.


> I think it also warrants a comment in the code too.
> 
> Given that these migrations ought to be rare(ish) it might be simpler
> (at least in terms of reasoning about it) for the clearing vcpu to
> enqueue the irq onto a dedicated migration list which has its own lock.
> Then the new target vcpu could walk that list itself and move things
> onto it's own lr_pending list.
>
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/gic.c           |   23 +++++++++++++++++++++--
> >  xen/arch/arm/vgic.c          |   36 ++++++++++++++++++++++++++++++++++++
> >  xen/include/asm-arm/domain.h |    4 ++++
> >  3 files changed, 61 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 08ae23b..92391b4 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -677,10 +677,29 @@ 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);
> > +
> > +            if ( test_and_clear_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) &&
> > +                 test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
> > +            {
> > +                struct vcpu *v_target;
> > +
> > +                spin_unlock(&v->arch.vgic.lock);
> > +                v_target = vgic_get_target_vcpu(v, irq);
> 
> Handle v == v_target?

Should work


> > +                spin_lock(&v_target->arch.vgic.lock);
> > +
> > +                gic_add_to_lr_pending(v_target, p);
> > +                if ( v_target->is_running )
> > +                    smp_send_event_check_mask(cpumask_of(v_target->processor));
> 
> Don't you also need to vcpu_unblock if it is sleeping?

Good point


> > +                spin_unlock(&v_target->arch.vgic.lock);
> > +                spin_lock(&v->arch.vgic.lock);
> > +            }
> > +        }
> >      }
> >  }
> >  
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index e527892..54d3676 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -377,6 +377,21 @@ read_as_zero:
> >      return 1;
> >  }
> >  
> > +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;
> > +    
> > +    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)
> >  {
> >      int target;
> > @@ -629,6 +644,21 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
> >              }
> >              i++;
> >          }
> > +        i = 0;
> > +        while ( (i = find_next_bit((const unsigned long *) &tr, 32, i)) < 32 )
> 
> bit ops alignment issues?
> 
> I think you should use old_tr^new_tr to only process bits which have
> changed.
> 
> > +        {
> > +            unsigned int irq, target, old_target;
> > +            struct vcpu *v_target, *v_old;
> > +
> > +            target = i % 8;
> > +
> > +            irq = offset + (i / 8);
> > +            v_target = v->domain->vcpu[target];
> > +            old_target = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
> > +            v_old = v->domain->vcpu[old_target];
> 
> v_target and v_old might be the same.

No, they could not: if they were find_next_bit wouldn't find the bit set.


> > +            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)] = *r;
> >          else
> 
> 

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

[Stefano Stabellini]

On Tue, 10 Jun 2014, Ian Campbell wrote:
> > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
> > index a33c797..0fad647 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_irq(vgic_get_target_vcpu(d->vcpu[0], irq), irq);
> 
> Would it make sense to push vgic_get_target_vcpu down into
> vgic_vcpu_inject_irq rather than have all callers need to do it?
> 
> I'm also wondering if vgic_get_target_vcpu shouldn't take d and not v.

That could be a good idea.

> Does this do the right thing for PPIs? vgic_get_target_vcpu will just
> lookup vcpu0's target, not the actual expected target, won't it?
> (something else must deal with this, or it'd be broken already I
> suppose)

As Julien wrote, we don't support routing PPIs to guests.

Re: [PATCH v4 2/4] xen/arm: inflight irqs during migration

[Julien Grall]

On 06/11/2014 03:15 PM, Stefano Stabellini wrote:
>>> +        {
>>> +            unsigned int irq, target, old_target;
>>> +            struct vcpu *v_target, *v_old;
>>> +
>>> +            target = i % 8;
>>> +
>>> +            irq = offset + (i / 8);
>>> +            v_target = v->domain->vcpu[target];
>>> +            old_target = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
>>> +            v_old = v->domain->vcpu[old_target];
>>
>> v_target and v_old might be the same.
> 
> No, they could not: if they were find_next_bit wouldn't find the bit set.

Even though v_target is always != v_old (because of the tr = r & ~val
stuff), why do you migrate if the old_target will be in the new mask?

BTW, this code suffers the same issue as #1, i.e this register can be
accessed by byte.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Tue, 10 Jun 2014, Ian Campbell wrote:
> On Fri, 2014-06-06 at 18:48 +0100, Stefano Stabellini wrote:
> > 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 hook in common code to call the vgic irq migration code
> > as evtchn_move_pirqs only deals with event channels at the moment.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > CC: JBeulich@suse.com
> > ---
> >  xen/arch/arm/gic.c         |   18 ++++++++++++++++--
> >  xen/arch/arm/vgic.c        |   31 +++++++++++++++++++++++++++++++
> >  xen/common/event_channel.c |    4 ++++
> >  xen/include/asm-arm/gic.h  |    1 +
> >  4 files changed, 52 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 6f24b14..43bef21 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);
> 
> What does this lock actually protect against here? I think the only
> thing which is externally visible is the write to bytereg and all the
> inputs are deterministic I think. Perhaps a suitable barrier would
> suffice? Or perhaps the caller ought to be holding the lock for some
> other reason already.

At the moment all the accesses to GICD are protected by the gic.lock.
I don't think we should change the policy at the same time of this
change.


> > +
> > +    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 54d3676..a90d9cb 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -408,6 +408,32 @@ struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> >      return v_target;
> >  }
> >  
> > +void vgic_move_irqs(struct vcpu *v)
> > +{
> > +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
> > +    struct domain *d = v->domain;
> > +    struct pending_irq *p;
> > +    int i, j, k;
> > +
> > +    for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> > +    {
> > +        for ( j = 0 ; j < 8 ; j++ )
> > +        {
> > +            for ( k = 0; k < 4; k++ )
> > +            {
> > +                uint8_t target = byte_read(d->arch.vgic.shared_irqs[i].itargets[j], 0, k);
> 
> i,j,k are pretty opaque here (and I wrote that before I saw the
> machinations going on in the irq_to_pending call!).
> 
> Please just iterate over irqs and use the rank accessor functions to get
> to the correct itargets to read. Which might be clearest with the
> irq_to_rank helper I proposed on an earlier patch.

I hacked the alternative solution and I prefer this version.

 
> > +                target = find_next_bit((const unsigned long *) &target, 8, 0);
> 
> This is find_first_bit, I think.
> 
> It's just occurred to me that many of the 
> 	i = 0
> 	while (i = find_enxt_bit() )
> loops I've been seeing in this series might be better following a
> 	for (i=find_first_bit; ; i=find_next_bit(...))
> pattern.
> 
> > +                if ( target == v->vcpu_id )
> > +                {
> > +                    p = irq_to_pending(v, 32 * (i + 1) + (j * 4) + k);
> > +                    if ( p->desc != NULL )
> > +                        p->desc->handler->set_affinity(p->desc, cpu_mask);
> 
> A helper for this chain of indirections might be nice.
> irq_set_affinity(desc, cpu_mask) of something.

OK


> > +                }
> > +            }
> > +        }
> > +    }
> > +}
> > +
> >  static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> >  {
> >      const unsigned long mask = r;
> > diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> > index 6853842..226321d 100644
> > --- a/xen/common/event_channel.c
> > +++ b/xen/common/event_channel.c
> > @@ -1319,6 +1319,10 @@ void evtchn_move_pirqs(struct vcpu *v)
> >      unsigned int port;
> >      struct evtchn *chn;
> >  
> > +#ifdef CONFIG_ARM
> > +	vgic_move_irqs(v);
> 
> Indent and/or hard vs soft tab.
> 
> And this should be arch_move_irqs I think, perhaps with a stub on x86
> instead of an ifdef.

good idea


> > +#endif
> > +
> >      spin_lock(&d->event_lock);
> >      for ( port = v->pirq_evtchn_head; port; port = chn->u.pirq.next_port )
> >      {
> 
> 

Re: [PATCH v4 2/4] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Wed, 11 Jun 2014, Julien Grall wrote:
> On 06/11/2014 03:15 PM, Stefano Stabellini wrote:
> >>> +        {
> >>> +            unsigned int irq, target, old_target;
> >>> +            struct vcpu *v_target, *v_old;
> >>> +
> >>> +            target = i % 8;
> >>> +
> >>> +            irq = offset + (i / 8);
> >>> +            v_target = v->domain->vcpu[target];
> >>> +            old_target = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
> >>> +            v_old = v->domain->vcpu[old_target];
> >>
> >> v_target and v_old might be the same.
> > 
> > No, they could not: if they were find_next_bit wouldn't find the bit set.
> 
> Even though v_target is always != v_old (because of the tr = r & ~val
> stuff), why do you migrate if the old_target will be in the new mask?

We need to be consistent: always the lowest vcpu in the mask.


> BTW, this code suffers the same issue as #1, i.e this register can be
> accessed by byte.

I followed Ian's suggestion and I masked the register according to
v->domain->max_vcpus.

> Regards,
> 
> -- 
> Julien Grall
> 

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

[Ian Campbell]

On Wed, 2014-06-11 at 15:47 +0100, Stefano Stabellini wrote:
> > > +void vgic_move_irqs(struct vcpu *v)
> > > +{
> > > +    const cpumask_t *cpu_mask = cpumask_of(v->processor);
> > > +    struct domain *d = v->domain;
> > > +    struct pending_irq *p;
> > > +    int i, j, k;
> > > +
> > > +    for ( i = 0; i < DOMAIN_NR_RANKS(d); i++ )
> > > +    {
> > > +        for ( j = 0 ; j < 8 ; j++ )
> > > +        {
> > > +            for ( k = 0; k < 4; k++ )
> > > +            {
> > > +                uint8_t target = byte_read(d->arch.vgic.shared_irqs[i].itargets[j], 0, k);
> > 
> > i,j,k are pretty opaque here (and I wrote that before I saw the
> > machinations going on in the irq_to_pending call!).
> > 
> > Please just iterate over irqs and use the rank accessor functions to get
> > to the correct itargets to read. Which might be clearest with the
> > irq_to_rank helper I proposed on an earlier patch.
> 
> I hacked the alternative solution and I prefer this version.

But, but, this way is horrible! How much worse could the alternative
have been!

Re: [PATCH v4 2/4] xen/arm: inflight irqs during migration

[Julien Grall]

On 06/11/2014 03:49 PM, Stefano Stabellini wrote:
> On Wed, 11 Jun 2014, Julien Grall wrote:
>> On 06/11/2014 03:15 PM, Stefano Stabellini wrote:
>>>>> +        {
>>>>> +            unsigned int irq, target, old_target;
>>>>> +            struct vcpu *v_target, *v_old;
>>>>> +
>>>>> +            target = i % 8;
>>>>> +
>>>>> +            irq = offset + (i / 8);
>>>>> +            v_target = v->domain->vcpu[target];
>>>>> +            old_target = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
>>>>> +            v_old = v->domain->vcpu[old_target];
>>>>
>>>> v_target and v_old might be the same.
>>>
>>> No, they could not: if they were find_next_bit wouldn't find the bit set.
>>
>> Even though v_target is always != v_old (because of the tr = r & ~val
>> stuff), why do you migrate if the old_target will be in the new mask?
> 
> We need to be consistent: always the lowest vcpu in the mask.

It's not the case here. AFAIU tr only contains the list of VPCU that are
not yet in this register.

The old vcpu can have an cpuid lower than the new vcpu. In this case,
you won't respect your consistency.

Regards,

-- 
Julien Grall

Re: [PATCH v4 2/4] xen/arm: inflight irqs during migration

[Stefano Stabellini]

On Wed, 11 Jun 2014, Julien Grall wrote:
> On 06/11/2014 03:49 PM, Stefano Stabellini wrote:
> > On Wed, 11 Jun 2014, Julien Grall wrote:
> >> On 06/11/2014 03:15 PM, Stefano Stabellini wrote:
> >>>>> +        {
> >>>>> +            unsigned int irq, target, old_target;
> >>>>> +            struct vcpu *v_target, *v_old;
> >>>>> +
> >>>>> +            target = i % 8;
> >>>>> +
> >>>>> +            irq = offset + (i / 8);
> >>>>> +            v_target = v->domain->vcpu[target];
> >>>>> +            old_target = byte_read(rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)], 0, i/8);
> >>>>> +            v_old = v->domain->vcpu[old_target];
> >>>>
> >>>> v_target and v_old might be the same.
> >>>
> >>> No, they could not: if they were find_next_bit wouldn't find the bit set.
> >>
> >> Even though v_target is always != v_old (because of the tr = r & ~val
> >> stuff), why do you migrate if the old_target will be in the new mask?
> > 
> > We need to be consistent: always the lowest vcpu in the mask.
> 
> It's not the case here. AFAIU tr only contains the list of VPCU that are
> not yet in this register.
> 
> The old vcpu can have an cpuid lower than the new vcpu. In this case,
> you won't respect your consistency.

You are right. I'll rewrite this code.