[PATCH v9 0/12] remove maintenance interrupts

[PATCH v9 0/12] remove maintenance interrupts

[Stefano Stabellini]

Hi all,
this patch series removes any needs for maintenance interrupts for both
hardware and software interrupts in Xen.
It achieves the goal by using the GICH_LR_HW bit for hardware interrupts
and by checking the status of the GICH_LR registers on return to guest,
clearing the registers that are invalid and handling the lifecycle of
the corresponding interrupts in Xen data structures.
It also improves priority handling, keeping the highest priority
outstanding interrupts in the GICH_LR registers.


Changes in v9:
- code style fix;
- add a comment;
- drop patch #13;
- gic_events_need_delivery: read GICH_APR to find the active group
priority;
- gic_events_need_delivery: simplify the search loop.

Changes in v8:
- fix a recursive lock bug in patch #4: only call gic_clear_lrs on
hypervisor entry if we are coming from guest_mode;
- fix a data abort in patch #4: do not clear_lrs for the idle domain;
- rename lr_reg to lr_val;
- remove double spin_lock in gic_update_one_lr;
- update comment in domain.h to better reflect the renaming;
- do not unify the inflight and non-inflight code paths in
vgic_vcpu_inject_irq: it creates many buggy corner cases. Introduce
gic_raise_inflight_irq instead;
- add warnings for cases of lost interrupts;
- in gic_restore_pending_irqs rename i to lr;
- add in code comments in gic_restore_pending_irqs and
gic_events_need_delivery;
- add << 3 to mask_priority;
- fix typo and hard tabs;
- add a patch to fix the spin_lock taken by gic_remove_from_queues.

Changes in v7:
- move enter_hypervisor_head before the first use to avoid forward
declaration;
- improve and add in code comments;
- rename gic_clear_one_lr to gic_update_one_lr;
- add patch "rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED";
- remove warning printk "Changing priority of an inflight interrupt is
not supported";
- fix locking for the list_empty case in gic_restore_pending_irqs;
- gic_events_need_delivery: break out of the loop as soon as we find the
active irq as inflight_irqs is ordered by priority;
- gic_events_need_delivery: break out of the loop if p->priority is
lower than mask_priority as inflight_irqs is ordered by priority;
- use find_next_zero_bit instead of find_first_zero_bit;
- in gic_restore_pending_irqs remember the last position of the inner
loop search and continue from there;
- in gic_restore_pending_irqs use a priority check to get out of the
inner loop;
- add patch "introduce GIC_PRI_TO_GUEST macro".

Changes in v6:
- remove double spin_lock on the vgic.lock introduced in v5.

Changes in v5:
- introduce lr_all_full() helper;
- do not rename virtual_irq to irq;
- replace "const long unsigned int" with "const unsigned long";
- remove useless "& GICH_LR_PHYSICAL_MASK" in gic_set_lr;
- add a comment in maintenance_interrupts to explain its new purpose;
- introduce gic_clear_one_lr;
- don't print p->lr in case the irq is in lr_pending, as it doesn't have
an LR associate to it;
- improve packing of struct pending_irq;
- #define GIC_INVALID_LR and use it instead of nr_lrs;
- gic_remove_from_queues need to be protected with a vgic lock;
- introduce ASSERTs to check the vgic is locked and interrupts are
disabled;
- improve in code comments;
- use list_for_each_entry_reverse instead of writing my own list walker.

Changes in v4:
- rebase;
- merged patch #3 and #4 into a single patch;
- improved in code comments;
- in gic_events_need_delivery go through inflight_irqs and only consider
enabled irqs;
- remove debug patch.

Changes in v3:
- add "no need to set HCR_VI when using the vgic to inject irqs";
- add "s/gic_set_guest_irq/gic_raise_guest_irq";
- add "xen/arm: call gic_clear_lrs on entry to the hypervisor";
- do not use the PENDING and ACTIVE state for HW interrupts;
- unify the inflight and non-inflight code paths in
vgic_vcpu_inject_irq;
- remove "avoid taking unconditionally the vgic.lock in gic_clear_lrs";
- add "xen/arm: gic_events_need_delivery and irq priorities";
- use spin_lock_irqsave and spin_unlock_irqrestore in gic_dump_info.

Changes in v2:
- do not assume physical IRQ == virtual IRQ;
- refactor gic_set_lr;
- simplify gic_clear_lrs;
- disable/enable the GICH_HCR_UIE bit in GICH_HCR;
- only enable GICH_HCR_UIE if this_cpu(lr_mask) == ((1 << nr_lrs) - 1);
- add a patch to keep track of the LR number in pending_irq;
- add a patch to set GICH_LR_PENDING to inject a second irq while the
first one is still active;
- add a patch to simplify and reduce the usage of gic.lock;
- add a patch to reduce the usage of vgic.lock;
- add a patch to use GICH_ELSR[01] to avoid reading all the GICH_LRs in
gic_clear_lrs;
- add a debug patch to print more info in gic_dump_info.



Stefano Stabellini (12):
      xen/arm: no need to set HCR_VI when using the vgic to inject irqs
      xen/arm: remove unused virtual parameter from vgic_vcpu_inject_irq
      xen/arm: set GICH_HCR_UIE if all the LRs are in use
      xen/arm: support HW interrupts, do not request maintenance_interrupts
      xen/arm: nr_lrs should be uint8_t
      xen/arm: keep track of the GICH_LR used for the irq in struct pending_irq
      xen/arm: s/gic_set_guest_irq/gic_raise_guest_irq
      xen/arm: rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED
      xen/arm: second irq injection while the first irq is still inflight
      xen/arm: don't protect GICH and lr_queue accesses with gic.lock
      xen/arm: introduce GIC_PRI_TO_GUEST macro
      xen/arm: gic_events_need_delivery and irq priorities

 xen/arch/arm/domain.c        |    2 +-
 xen/arch/arm/gic.c           |  321 ++++++++++++++++++++++++++----------------
 xen/arch/arm/irq.c           |    2 +-
 xen/arch/arm/time.c          |    2 +-
 xen/arch/arm/traps.c         |   10 ++
 xen/arch/arm/vgic.c          |   32 +++--
 xen/arch/arm/vtimer.c        |    4 +-
 xen/include/asm-arm/domain.h |   37 +++--
 xen/include/asm-arm/gic.h    |   13 +-
 9 files changed, 270 insertions(+), 153 deletions(-)

git://xenbits.xen.org/people/sstabellini/xen-unstable.git no_maintenance_interrupts-v9

[PATCH v9 01/12] xen/arm: no need to set HCR_VI when using the vgic to inject irqs

[Stefano Stabellini]

HCR_VI forces the guest to resume execution in IRQ mode and can actually
cause spurious interrupt injections.
The GIC is capable of injecting interrupts into the guest and causing it
to switch to IRQ mode automatically, without any need for the hypervisor
to set HCR_VI manually.

See ARM ARM B1.8.11 and chapter 5.4 of the Generic Interrupt Controller
Architecture Specification.

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

---

Changes in v4:
- improve commit message.
---
 xen/arch/arm/gic.c |   20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 577d85b..a449ef3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -643,22 +643,6 @@ void gic_clear_pending_irqs(struct vcpu *v)
     spin_unlock_irqrestore(&gic.lock, flags);
 }
 
-static void gic_inject_irq_start(void)
-{
-    register_t hcr = READ_SYSREG(HCR_EL2);
-    WRITE_SYSREG(hcr | HCR_VI, HCR_EL2);
-    isb();
-}
-
-static void gic_inject_irq_stop(void)
-{
-    register_t hcr = READ_SYSREG(HCR_EL2);
-    if (hcr & HCR_VI) {
-        WRITE_SYSREG(hcr & ~HCR_VI, HCR_EL2);
-        isb();
-    }
-}
-
 int gic_events_need_delivery(void)
 {
     return (!list_empty(&current->arch.vgic.lr_pending) ||
@@ -671,10 +655,6 @@ void gic_inject(void)
         vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq, 1);
 
     gic_restore_pending_irqs(current);
-    if (!gic_events_need_delivery())
-        gic_inject_irq_stop();
-    else
-        gic_inject_irq_start();
 }
 
 static void do_sgi(struct cpu_user_regs *regs, int othercpu, enum gic_sgi sgi)
-- 
1.7.10.4

[PATCH v9 02/12] xen/arm: remove unused virtual parameter from vgic_vcpu_inject_irq

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain.c     |    2 +-
 xen/arch/arm/gic.c        |    2 +-
 xen/arch/arm/irq.c        |    2 +-
 xen/arch/arm/time.c       |    2 +-
 xen/arch/arm/vgic.c       |    4 ++--
 xen/arch/arm/vtimer.c     |    4 ++--
 xen/include/asm-arm/gic.h |    2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 40f1c3a..33141e3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -769,7 +769,7 @@ void vcpu_mark_events_pending(struct vcpu *v)
     if ( already_pending )
         return;
 
-    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq, 1);
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
 }
 
 /*
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index a449ef3..6d917a0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -652,7 +652,7 @@ int gic_events_need_delivery(void)
 void gic_inject(void)
 {
     if ( vcpu_info(current, evtchn_upcall_pending) )
-        vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq, 1);
+        vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq);
 
     gic_restore_pending_irqs(current);
 }
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 44696e7..a33c797 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -176,7 +176,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         desc->arch.eoi_cpu = smp_processor_id();
 
         /* XXX: inject irq into all guest vcpus */
-        vgic_vcpu_inject_irq(d->vcpu[0], irq, 0);
+        vgic_vcpu_inject_irq(d->vcpu[0], irq);
         goto out_no_end;
     }
 
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index d04c97a..6e8d1f3 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -215,7 +215,7 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
     current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
     WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
-    vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq, 1);
+    vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq);
 }
 
 /* Set up the timer interrupt on this CPU */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 4cf6470..9838ce5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -476,7 +476,7 @@ static int vgic_to_sgi(struct vcpu *v, register_t sgir)
                      sgir, vcpu_mask);
             continue;
         }
-        vgic_vcpu_inject_irq(d->vcpu[vcpuid], virtual_irq, 1);
+        vgic_vcpu_inject_irq(d->vcpu[vcpuid], virtual_irq);
     }
     return 1;
 }
@@ -704,7 +704,7 @@ void vgic_clear_pending_irqs(struct vcpu *v)
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
-void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
+void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 {
     int idx = irq >> 2, byte = irq & 0x3;
     uint8_t priority;
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index b93153e..b751692 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -34,14 +34,14 @@ static void phys_timer_expired(void *data)
     struct vtimer *t = data;
     t->ctl |= CNTx_CTL_PENDING;
     if ( !(t->ctl & CNTx_CTL_MASK) )
-        vgic_vcpu_inject_irq(t->v, t->irq, 1);
+        vgic_vcpu_inject_irq(t->v, t->irq);
 }
 
 static void virt_timer_expired(void *data)
 {
     struct vtimer *t = data;
     t->ctl |= CNTx_CTL_MASK;
-    vgic_vcpu_inject_irq(t->v, t->irq, 1);
+    vgic_vcpu_inject_irq(t->v, t->irq);
 }
 
 int domain_vtimer_init(struct domain *d)
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index b750b17..b1b4fd5 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -163,7 +163,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,int virtual);
+extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 
-- 
1.7.10.4

[PATCH v9 03/12] xen/arm: set GICH_HCR_UIE if all the LRs are in use

[Stefano Stabellini]

On return to guest, if there are no free LRs and we still have more
interrupt to inject, set GICH_HCR_UIE so that we are going to receive a
maintenance interrupt when no pending interrupts are present in the LR
registers.
The maintenance interrupt handler won't do anything anymore, but
receiving the interrupt is going to cause gic_inject to be called on
return to guest that is going to clear the old LRs and inject new
interrupts.

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

---

Changes in v5:
- introduce lr_all_full() helper.

Changes in v2:
- disable/enable the GICH_HCR_UIE bit in GICH_HCR;
- only enable GICH_HCR_UIE if this_cpu(lr_mask) == ((1 << nr_lrs) - 1).
---
 xen/arch/arm/gic.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6d917a0..6b21945 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -55,6 +55,7 @@ static struct {
 static DEFINE_PER_CPU(uint64_t, lr_mask);
 
 static unsigned nr_lrs;
+#define lr_all_full() (this_cpu(lr_mask) == ((1 << nr_lrs) - 1))
 
 /* The GIC mapping of CPU interfaces does not necessarily match the
  * logical CPU numbering. Let's use mapping as returned by the GIC
@@ -655,6 +656,13 @@ void gic_inject(void)
         vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq);
 
     gic_restore_pending_irqs(current);
+
+
+    if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
+        GICH[GICH_HCR] |= GICH_HCR_UIE;
+    else
+        GICH[GICH_HCR] &= ~GICH_HCR_UIE;
+
 }
 
 static void do_sgi(struct cpu_user_regs *regs, int othercpu, enum gic_sgi sgi)
-- 
1.7.10.4

[PATCH v9 04/12] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Stefano Stabellini]

If the irq to be injected is an hardware irq (p->desc != NULL), set
GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ.

Remove the code to EOI a physical interrupt on behalf of the guest
because it has become unnecessary.

Introduce a new function, gic_clear_lrs, that goes over the GICH_LR
registers, clear the invalid ones and free the corresponding interrupts
from the inflight queue if appropriate. Add the interrupt to lr_pending
if the GIC_IRQ_GUEST_PENDING is still set.

Call gic_clear_lrs on entry to the hypervisor if we are coming from
guest mode to make sure that the calculation in Xen of the highest
priority interrupt currently inflight is correct and accurate and not
based on stale data.

In vgic_vcpu_inject_irq, if the target is a vcpu running on another
pcpu, we are already sending an SGI to the other pcpu so that it would
pick up the new IRQ to inject.  Now also send an SGI to the other pcpu
even if the IRQ is already inflight, so that it can clear the LR
corresponding to the previous injection as well as injecting the new
interrupt.

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

---

Changes in v9:
- code style fix;
- add a comment.

Changes in v8:
- do not clear LRs for the idle domain;
- do not clear LRs on hypervisor entry if we are not coming from guest
mode;
- rename lr_reg to lr_val;
- remove double spin_lock in gic_update_one_lr.

Changes in v7:
- move enter_hypervisor_head before the first use to avoid forward
declaration;
- improve in code comments;
- rename gic_clear_one_lr to gic_update_one_lr.

Changes in v6:
- remove double spin_lock on the vgic.lock introduced in v5.

Changes in v5:
- do not rename virtual_irq to irq;
- replace "const long unsigned int" with "const unsigned long";
- remove useless "& GICH_LR_PHYSICAL_MASK" in gic_set_lr;
- add a comment in maintenance_interrupts to explain its new purpose.
- introduce gic_clear_one_lr.

Changes in v4:
- merged patch #3 and #4 into a single patch.

Changes in v2:
- remove the EOI code, now unnecessary;
- do not assume physical IRQ == virtual IRQ;
- refactor gic_set_lr.
---
 xen/arch/arm/gic.c        |  136 +++++++++++++++++++++------------------------
 xen/arch/arm/traps.c      |   10 ++++
 xen/arch/arm/vgic.c       |    3 +-
 xen/include/asm-arm/gic.h |    1 +
 4 files changed, 75 insertions(+), 75 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6b21945..4af8c1a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -66,6 +66,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 #define NR_GIC_CPU_IF 8
 
+static void gic_update_one_lr(struct vcpu *v, int i);
+
 static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
 {
     unsigned int cpu;
@@ -543,16 +545,18 @@ void gic_disable_cpu(void)
 static inline void gic_set_lr(int lr, struct pending_irq *p,
         unsigned int state)
 {
-    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
+    uint32_t lr_val;
 
     BUG_ON(lr >= nr_lrs);
     BUG_ON(lr < 0);
     BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
 
-    GICH[GICH_LR + lr] = state |
-        maintenance_int |
-        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+    lr_val = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
         ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
+    if ( p->desc != NULL )
+        lr_val |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT);
+
+    GICH[GICH_LR + lr] = lr_val;
 
     set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
     clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
@@ -612,6 +616,55 @@ out:
     return;
 }
 
+static void gic_update_one_lr(struct vcpu *v, int i)
+{
+    struct pending_irq *p;
+    uint32_t lr;
+    int irq;
+
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    lr = GICH[GICH_LR + i];
+    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
+    {
+        GICH[GICH_LR + i] = 0;
+        clear_bit(i, &this_cpu(lr_mask));
+
+        irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
+        p = irq_to_pending(v, irq);
+        if ( p->desc != NULL )
+            p->desc->status &= ~IRQ_INPROGRESS;
+        clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+        if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
+                test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
+            gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+        else
+            list_del_init(&p->inflight);
+    }
+}
+
+void gic_clear_lrs(struct vcpu *v)
+{
+    int i = 0;
+    unsigned long flags;
+
+    /* The idle domain has no LRs to be cleared. Since gic_restore_state
+     * doesn't write any LR registers for the idle domain they could be
+     * non-zero. */
+    if ( is_idle_vcpu(v) )
+        return;
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    while ((i = find_next_bit((const unsigned long *) &this_cpu(lr_mask),
+                              nr_lrs, i)) < nr_lrs ) {
+        gic_update_one_lr(v, i);
+        i++;
+    }
+
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+}
+
 static void gic_restore_pending_irqs(struct vcpu *v)
 {
     int i;
@@ -767,77 +820,14 @@ int gicv_setup(struct domain *d)
 
 }
 
-static void gic_irq_eoi(void *info)
-{
-    int virq = (uintptr_t) info;
-    GICC[GICC_DIR] = virq;
-}
-
 static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
-    int i = 0, virq, pirq = -1;
-    uint32_t lr;
-    struct vcpu *v = current;
-    uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
-
-    while ((i = find_next_bit((const long unsigned int *) &eisr,
-                              64, i)) < 64) {
-        struct pending_irq *p, *p2;
-        int cpu;
-        bool_t inflight;
-
-        cpu = -1;
-        inflight = 0;
-
-        spin_lock_irq(&gic.lock);
-        lr = GICH[GICH_LR + i];
-        virq = lr & GICH_LR_VIRTUAL_MASK;
-        GICH[GICH_LR + i] = 0;
-        clear_bit(i, &this_cpu(lr_mask));
-
-        p = irq_to_pending(v, virq);
-        if ( p->desc != NULL ) {
-            p->desc->status &= ~IRQ_INPROGRESS;
-            /* Assume only one pcpu needs to EOI the irq */
-            cpu = p->desc->arch.eoi_cpu;
-            pirq = p->desc->irq;
-        }
-        if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
-             test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
-        {
-            inflight = 1;
-            gic_add_to_lr_pending(v, p);
-        }
-
-        clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
-
-        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
-            p2 = list_entry(v->arch.vgic.lr_pending.next, typeof(*p2), lr_queue);
-            gic_set_lr(i, p2, GICH_LR_PENDING);
-            list_del_init(&p2->lr_queue);
-            set_bit(i, &this_cpu(lr_mask));
-        }
-        spin_unlock_irq(&gic.lock);
-
-        if ( !inflight )
-        {
-            spin_lock_irq(&v->arch.vgic.lock);
-            list_del_init(&p->inflight);
-            spin_unlock_irq(&v->arch.vgic.lock);
-        }
-
-        if ( p->desc != NULL ) {
-            /* this is not racy because we can't receive another irq of the
-             * same type until we EOI it.  */
-            if ( cpu == smp_processor_id() )
-                gic_irq_eoi((void*)(uintptr_t)pirq);
-            else
-                on_selected_cpus(cpumask_of(cpu),
-                                 gic_irq_eoi, (void*)(uintptr_t)pirq, 0);
-        }
-
-        i++;
-    }
+    /* 
+     * This is a dummy interrupt handler.
+     * Receiving the interrupt is going to cause gic_inject to be called
+     * on return to guest that is going to clear the old LRs and inject
+     * new interrupts.
+     */
 }
 
 void gic_dump_info(struct vcpu *v)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 03a3da6..a4bdaaa 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1658,10 +1658,18 @@ bad_data_abort:
     inject_dabt_exception(regs, info.gva, hsr.len);
 }
 
+static void enter_hypervisor_head(struct cpu_user_regs *regs)
+{
+    if ( guest_mode(regs) )
+        gic_clear_lrs(current);
+}
+
 asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 {
     union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
 
+    enter_hypervisor_head(regs);
+
     switch (hsr.ec) {
     case HSR_EC_WFI_WFE:
         if ( !check_conditional_instr(regs, hsr) )
@@ -1750,11 +1758,13 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 
 asmlinkage void do_trap_irq(struct cpu_user_regs *regs)
 {
+    enter_hypervisor_head(regs);
     gic_interrupt(regs, 0);
 }
 
 asmlinkage void do_trap_fiq(struct cpu_user_regs *regs)
 {
+    enter_hypervisor_head(regs);
     gic_interrupt(regs, 1);
 }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 9838ce5..d5b3a4b 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -720,8 +720,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
         if ( (irq != current->domain->arch.evtchn_irq) ||
              (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
             set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
-        return;
+        goto out;
     }
 
     /* vcpu offline */
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index b1b4fd5..92a8916 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -219,6 +219,7 @@ extern unsigned int gic_number_lines(void);
 /* IRQ translation function for the device tree */
 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);
 
 #endif /* __ASSEMBLY__ */
 #endif
-- 
1.7.10.4

[PATCH v9 05/12] xen/arm: nr_lrs should be uint8_t

[Stefano Stabellini]

A later patch is going to use uint8_t to keep track of LRs.
Both GICv3 and GICv2 don't need any more than an uint8_t to keep track
of the number of LRs.

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 4af8c1a..99b6bb4 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -54,7 +54,7 @@ static struct {
 
 static DEFINE_PER_CPU(uint64_t, lr_mask);
 
-static unsigned nr_lrs;
+static uint8_t nr_lrs;
 #define lr_all_full() (this_cpu(lr_mask) == ((1 << nr_lrs) - 1))
 
 /* The GIC mapping of CPU interfaces does not necessarily match the
-- 
1.7.10.4

[PATCH v9 06/12] xen/arm: keep track of the GICH_LR used for the irq in struct pending_irq

[Stefano Stabellini]

Move the irq field in pending_irq to improve packing.

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

---

Changes in v5:
- don't print p->lr in case the irq is in lr_pending, as it doesn't have
an LR associate to it;
- improve packing of struct pending_irq;
- #define GIC_INVALID_LR and use it instead of nr_lrs.
---
 xen/arch/arm/gic.c           |    4 +++-
 xen/include/asm-arm/domain.h |    4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 99b6bb4..97cd1bf 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -560,6 +560,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
 
     set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
     clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
+    p->lr = lr;
 }
 
 static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
@@ -635,6 +636,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         if ( p->desc != NULL )
             p->desc->status &= ~IRQ_INPROGRESS;
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+        p->lr = GIC_INVALID_LR;
         if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
@@ -847,7 +849,7 @@ void gic_dump_info(struct vcpu *v)
 
     list_for_each_entry ( p, &v->arch.vgic.inflight_irqs, inflight )
     {
-        printk("Inflight irq=%d\n", p->irq);
+        printk("Inflight irq=%d lr=%u\n", p->irq, p->lr);
     }
 
     list_for_each_entry( p, &v->arch.vgic.lr_pending, lr_queue )
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index aabeb51..e5db9e3 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -21,7 +21,6 @@ struct vgic_irq_rank {
 
 struct pending_irq
 {
-    int irq;
     /*
      * The following two states track the lifecycle of the guest irq.
      * However because we are not sure and we don't want to track
@@ -60,6 +59,9 @@ struct pending_irq
 #define GIC_IRQ_GUEST_ENABLED  2
     unsigned long status;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
+    int irq;
+#define GIC_INVALID_LR         ~(uint8_t)0
+    uint8_t lr;
     uint8_t priority;
     /* inflight is used to append instances of pending_irq to
      * vgic.inflight_irqs */
-- 
1.7.10.4

[PATCH v9 07/12] xen/arm: s/gic_set_guest_irq/gic_raise_guest_irq

[Stefano Stabellini]

Rename gic_set_guest_irq to gic_raise_guest_irq and remove the state
parameter.

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 97cd1bf..1aa529b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -592,8 +592,8 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
     spin_unlock_irqrestore(&gic.lock, flags);
 }
 
-void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
-        unsigned int state, unsigned int priority)
+void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
+        unsigned int priority)
 {
     int i;
     unsigned long flags;
@@ -605,7 +605,7 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
         i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
         if (i < nr_lrs) {
             set_bit(i, &this_cpu(lr_mask));
-            gic_set_lr(i, irq_to_pending(v, virtual_irq), state);
+            gic_set_lr(i, irq_to_pending(v, virtual_irq), GICH_LR_PENDING);
             goto out;
         }
     }
@@ -639,7 +639,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         p->lr = GIC_INVALID_LR;
         if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
-            gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+            gic_raise_guest_irq(v, irq, p->priority);
         else
             list_del_init(&p->inflight);
     }
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index d5b3a4b..b6c3ebe 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -405,7 +405,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         p = irq_to_pending(v, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
         if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-            gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+            gic_raise_guest_irq(v, irq, p->priority);
         if ( p->desc != NULL )
         {
             spin_lock_irqsave(&p->desc->lock, flags);
@@ -738,7 +738,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     /* the irq is enabled */
     if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
-        gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
+        gic_raise_guest_irq(v, irq, priority);
 
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
     {
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 92a8916..15f94eb 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -180,8 +180,8 @@ extern void gic_clear_pending_irqs(struct vcpu *v);
 extern int gic_events_need_delivery(void);
 
 extern void __cpuinit init_maintenance_interrupt(void);
-extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
-        unsigned int state, unsigned int priority);
+extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
+        unsigned int priority);
 extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq);
 
 /* Accept an interrupt from the GIC and dispatch its handler */
-- 
1.7.10.4

[PATCH v9 08/12] xen/arm: rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED

[Stefano Stabellini]

Rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED and clarify its
meaning in xen/include/asm-arm/domain.h.

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

---

Changes in v8:
- update comment in domain.h to better reflect the renaming.
---
 xen/arch/arm/gic.c           |    4 ++--
 xen/arch/arm/vgic.c          |    4 ++--
 xen/include/asm-arm/domain.h |   23 ++++++++++++-----------
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 1aa529b..7e5c201 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -559,7 +559,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
     GICH[GICH_LR + lr] = lr_val;
 
     set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
-    clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
+    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
     p->lr = lr;
 }
 
@@ -637,7 +637,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
             p->desc->status &= ~IRQ_INPROGRESS;
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
         p->lr = GIC_INVALID_LR;
-        if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
+        if ( test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
             gic_raise_guest_irq(v, irq, p->priority);
         else
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index b6c3ebe..b44937d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -719,7 +719,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
     {
         if ( (irq != current->domain->arch.evtchn_irq) ||
              (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
-            set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
+            set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
         goto out;
     }
 
@@ -733,7 +733,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
     priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
 
     n->irq = irq;
-    set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
+    set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
     n->priority = priority;
 
     /* the irq is enabled */
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index e5db9e3..c39756f 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -27,7 +27,8 @@ struct pending_irq
      * whether an irq added to an LR register is PENDING or ACTIVE, the
      * following states are just an approximation.
      *
-     * GIC_IRQ_GUEST_PENDING: the irq is asserted
+     * GIC_IRQ_GUEST_QUEUED: the irq is asserted and queued for
+     * injection into the guest's LRs.
      *
      * GIC_IRQ_GUEST_VISIBLE: the irq has been added to an LR register,
      * therefore the guest is aware of it. From the guest point of view
@@ -35,16 +36,16 @@ struct pending_irq
      * or active (after acking the irq).
      *
      * In order for the state machine to be fully accurate, for level
-     * interrupts, we should keep the GIC_IRQ_GUEST_PENDING state until
+     * interrupts, we should keep the interrupt's pending state until
      * the guest deactivates the irq. However because we are not sure
-     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
-     * state when we add the irq to an LR register. We add it back when
-     * we receive another interrupt notification.
-     * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the
-     * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of
-     * the guest irq in the LR register from active to active and
-     * pending, but for simplicity we simply inject a second irq after
-     * the guest EOIs the first one.
+     * when that happens, we instead track whether there is an interrupt
+     * queued using GIC_IRQ_GUEST_QUEUED. We clear it when we add it to
+     * an LR register. We set it when we receive another interrupt
+     * notification.  Therefore it is possible to set
+     * GIC_IRQ_GUEST_QUEUED while the irq is GIC_IRQ_GUEST_VISIBLE. We
+     * could also change the state of the guest irq in the LR register
+     * from active to active and pending, but for simplicity we simply
+     * inject a second irq after the guest EOIs the first one.
      *
      *
      * An additional state is used to keep track of whether the guest
@@ -54,7 +55,7 @@ struct pending_irq
      * level (GICD_ICENABLER/GICD_ISENABLER).
      *
      */
-#define GIC_IRQ_GUEST_PENDING  0
+#define GIC_IRQ_GUEST_QUEUED   0
 #define GIC_IRQ_GUEST_VISIBLE  1
 #define GIC_IRQ_GUEST_ENABLED  2
     unsigned long status;
-- 
1.7.10.4

[PATCH v9 09/12] xen/arm: second irq injection while the first irq is still inflight

[Stefano Stabellini]

Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq
while the first one is still active.
If the first irq is already pending (not active), clear
GIC_IRQ_GUEST_QUEUED because the guest doesn't need a second
notification.If the irq has already been EOI'ed then just clear the
GICH_LR right away and move the interrupt to lr_pending so that it is
going to be reinjected by gic_restore_pending_irqs on return to guest.

If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_QUEUED
and send an SGI. The target cpu is going to be interrupted and call
gic_clear_lrs, that is going to take the same actions.

Do not call vgic_vcpu_inject_irq from gic_inject if
evtchn_upcall_pending is set. If we remove that call, we don't need to
special case evtchn_irq in vgic_vcpu_inject_irq anymore.
We need to force the first injection of evtchn_irq (call
gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
is already set by common code on vcpu creation.

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

---

Changes in v8:
- do not unify the inflight and non-inflight code paths in
vgic_vcpu_inject_irq: it creates many buggy corner cases. Introduce
gic_raise_inflight_irq instead;
- add warnings for cases of lost interrupts.

Changes in v7:
- remove warning printk "Changing priority of an inflight interrupt is
not supported".

Changes in v3:
- do not use the PENDING and ACTIVE state for HW interrupts;
- unify the inflight and non-inflight code paths in
vgic_vcpu_inject_irq.
---
 xen/arch/arm/gic.c        |   48 +++++++++++++++++++++++++++++++++++++--------
 xen/arch/arm/vgic.c       |   14 +++++++++----
 xen/include/asm-arm/gic.h |    1 +
 3 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 7e5c201..4fb5c01 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -66,6 +66,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 #define NR_GIC_CPU_IF 8
 
+#undef GIC_DEBUG
+
 static void gic_update_one_lr(struct vcpu *v, int i);
 
 static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
@@ -592,6 +594,22 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
     spin_unlock_irqrestore(&gic.lock, flags);
 }
 
+void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
+{
+    struct pending_irq *n = irq_to_pending(v, virtual_irq);
+
+    if ( list_empty(&n->lr_queue) )
+    {
+        if ( v == current )
+            gic_update_one_lr(v, n->lr);
+    }
+#ifdef GIC_DEBUG
+    else
+        gdprintk(XENLOG_DEBUG, "trying to inject irq=%u into d%dv%d, when it is still lr_pending\n",
+                 virtual_irq, v->domain->domain_id, v->vcpu_id);
+#endif
+}
+
 void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
         unsigned int priority)
 {
@@ -626,19 +644,36 @@ static void gic_update_one_lr(struct vcpu *v, int i)
     ASSERT(spin_is_locked(&v->arch.vgic.lock));
 
     lr = GICH[GICH_LR + i];
-    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
+    irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
+    p = irq_to_pending(v, irq);
+    if ( lr & GICH_LR_ACTIVE )
     {
+        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
+        {
+            if ( p->desc == NULL )
+                GICH[GICH_LR + i] = lr | GICH_LR_PENDING;
+            else
+                gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
+                         irq, v->domain->domain_id, v->vcpu_id, i);
+        }
+    } else if ( lr & GICH_LR_PENDING ) {
+        int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+#ifdef GIC_DEBUG
+        if ( q )
+            gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is already pending in LR%d\n",
+                    irq, v->domain->domain_id, v->vcpu_id, i);
+#endif
+    } else {
         GICH[GICH_LR + i] = 0;
         clear_bit(i, &this_cpu(lr_mask));
 
-        irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
-        p = irq_to_pending(v, irq);
         if ( p->desc != NULL )
             p->desc->status &= ~IRQ_INPROGRESS;
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
         p->lr = GIC_INVALID_LR;
-        if ( test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
-                test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
+        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
             gic_raise_guest_irq(v, irq, p->priority);
         else
             list_del_init(&p->inflight);
@@ -707,9 +742,6 @@ int gic_events_need_delivery(void)
 
 void gic_inject(void)
 {
-    if ( vcpu_info(current, evtchn_upcall_pending) )
-        vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq);
-
     gic_restore_pending_irqs(current);
 
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index b44937d..c7abf9f 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -404,7 +404,14 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         irq = i + (32 * n);
         p = irq_to_pending(v, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &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 &&
+             vcpu_info(current, evtchn_upcall_pending) &&
+             list_empty(&p->inflight) )
+            vgic_vcpu_inject_irq(v, irq);
+        else if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
             gic_raise_guest_irq(v, irq, p->priority);
         if ( p->desc != NULL )
         {
@@ -717,9 +724,8 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     if ( !list_empty(&n->inflight) )
     {
-        if ( (irq != current->domain->arch.evtchn_irq) ||
-             (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
-            set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
+        set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
+        gic_raise_inflight_irq(v, irq);
         goto out;
     }
 
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 15f94eb..0c4c583 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -182,6 +182,7 @@ extern int gic_events_need_delivery(void);
 extern void __cpuinit init_maintenance_interrupt(void);
 extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
         unsigned int priority);
+extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
 extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq);
 
 /* Accept an interrupt from the GIC and dispatch its handler */
-- 
1.7.10.4

[PATCH v9 10/12] xen/arm: don't protect GICH and lr_queue accesses with gic.lock

[Stefano Stabellini]

GICH is banked, protect accesses by disabling interrupts.
Protect lr_queue accesses with the vgic.lock only.
gic.lock only protects accesses to GICD now.

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

---

Changes in v5:
- gic_remove_from_queues need to be protected with a vgic lock;
- introduce ASSERTs to check the vgic is locked and interrupts are
disabled.

Changes in v4:
- improved in code comments.
---
 xen/arch/arm/gic.c           |   32 +++++++++++++++++---------------
 xen/arch/arm/vgic.c          |    9 +++++++--
 xen/include/asm-arm/domain.h |    5 ++++-
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 4fb5c01..29c0502 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -113,6 +113,7 @@ void gic_save_state(struct vcpu *v)
 void gic_restore_state(struct vcpu *v)
 {
     int i;
+    ASSERT(!local_irq_is_enabled());
 
     if ( is_idle_vcpu(v) )
         return;
@@ -549,6 +550,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
 {
     uint32_t lr_val;
 
+    ASSERT(!local_irq_is_enabled());
     BUG_ON(lr >= nr_lrs);
     BUG_ON(lr < 0);
     BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
@@ -569,6 +571,8 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
 {
     struct pending_irq *iter;
 
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
     if ( !list_empty(&n->lr_queue) )
         return;
 
@@ -588,16 +592,18 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
     struct pending_irq *p = irq_to_pending(v, virtual_irq);
     unsigned long flags;
 
-    spin_lock_irqsave(&gic.lock, flags);
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
     if ( !list_empty(&p->lr_queue) )
         list_del_init(&p->lr_queue);
-    spin_unlock_irqrestore(&gic.lock, flags);
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
 void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
 {
     struct pending_irq *n = irq_to_pending(v, virtual_irq);
 
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
     if ( list_empty(&n->lr_queue) )
     {
         if ( v == current )
@@ -614,9 +620,8 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
         unsigned int priority)
 {
     int i;
-    unsigned long flags;
 
-    spin_lock_irqsave(&gic.lock, flags);
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
 
     if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
     {
@@ -624,15 +629,11 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
         if (i < nr_lrs) {
             set_bit(i, &this_cpu(lr_mask));
             gic_set_lr(i, irq_to_pending(v, virtual_irq), GICH_LR_PENDING);
-            goto out;
+            return;
         }
     }
 
     gic_add_to_lr_pending(v, irq_to_pending(v, virtual_irq));
-
-out:
-    spin_unlock_irqrestore(&gic.lock, flags);
-    return;
 }
 
 static void gic_update_one_lr(struct vcpu *v, int i)
@@ -642,6 +643,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
     int irq;
 
     ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(!local_irq_is_enabled());
 
     lr = GICH[GICH_LR + i];
     irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
@@ -708,30 +710,28 @@ static void gic_restore_pending_irqs(struct vcpu *v)
     struct pending_irq *p, *t;
     unsigned long flags;
 
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
     list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
     {
         i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
         if ( i >= nr_lrs ) return;
 
-        spin_lock_irqsave(&gic.lock, flags);
         gic_set_lr(i, p, GICH_LR_PENDING);
         list_del_init(&p->lr_queue);
         set_bit(i, &this_cpu(lr_mask));
-        spin_unlock_irqrestore(&gic.lock, flags);
     }
-
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
 void gic_clear_pending_irqs(struct vcpu *v)
 {
     struct pending_irq *p, *t;
-    unsigned long flags;
 
-    spin_lock_irqsave(&gic.lock, flags);
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
     v->arch.lr_mask = 0;
     list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
         list_del_init(&p->lr_queue);
-    spin_unlock_irqrestore(&gic.lock, flags);
 }
 
 int gic_events_need_delivery(void)
@@ -742,6 +742,8 @@ int gic_events_need_delivery(void)
 
 void gic_inject(void)
 {
+    ASSERT(!local_irq_is_enabled());
+
     gic_restore_pending_irqs(current);
 
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c7abf9f..cb8df3a 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -411,8 +411,13 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
              vcpu_info(current, evtchn_upcall_pending) &&
              list_empty(&p->inflight) )
             vgic_vcpu_inject_irq(v, irq);
-        else if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-            gic_raise_guest_irq(v, irq, p->priority);
+        else {
+            unsigned long flags;
+            spin_lock_irqsave(&v->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);
+        }
         if ( p->desc != NULL )
         {
             spin_lock_irqsave(&p->desc->lock, flags);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index c39756f..59ce196 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -68,7 +68,10 @@ struct pending_irq
      * vgic.inflight_irqs */
     struct list_head inflight;
     /* lr_queue is used to append instances of pending_irq to
-     * gic.lr_pending */
+     * lr_pending. lr_pending is a per vcpu queue, therefore lr_queue
+     * accesses are protected with the vgic lock.
+     * TODO: when implementing irq migration, taking only the current
+     * vgic lock is not going to be enough. */
     struct list_head lr_queue;
 };
 
-- 
1.7.10.4

[PATCH v9 11/12] xen/arm: introduce GIC_PRI_TO_GUEST macro

[Stefano Stabellini]

GICH_LR registers and GICH_VMCR only support 5 bits for guest irq
priorities.
Introduce a macro to reduce the 8-bit priority fields to 5 bits; use it
in gic.c.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---

Changes in v8:
- fix typo and hard tabs.
---
 xen/arch/arm/gic.c        |    2 +-
 xen/include/asm-arm/gic.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 29c0502..3728182 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -555,7 +555,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
     BUG_ON(lr < 0);
     BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
 
-    lr_val = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+    lr_val = state | (GIC_PRI_TO_GUEST(p->priority) << GICH_LR_PRIORITY_SHIFT) |
         ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
     if ( p->desc != NULL )
         lr_val |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 0c4c583..aede45c 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -149,6 +149,8 @@
 #define GIC_PRI_IRQ        0xa0
 #define GIC_PRI_IPI        0x90 /* IPIs must preempt normal interrupts */
 #define GIC_PRI_HIGHEST    0x80 /* Higher priorities belong to Secure-World */
+#define GIC_PRI_TO_GUEST(pri) (pri >> 3) /* GICH_LR and GICH_VMCR only support
+                                            5 bits for guest irq priority */
 
 
 #ifndef __ASSEMBLY__
-- 
1.7.10.4

[PATCH v9 12/12] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini]

Introduce GIC_IRQ_GUEST_ACTIVE to track which irqs are currently
active in the guest.

gic_events_need_delivery should only return positive if an outstanding
pending irq has an higher group priority than the currently active group
priotity and the priority mask.
Read GICH_APR to find the active group priority.
Read GICH_VMCR to find the priority mask.
Find the highest priority non-active enabled irq by going through the
inflight list.

In gic_restore_pending_irqs replace lower priority pending (and not
active) irqs in GICH_LRs with higher priority irqs if no more GICH_LRs
are available.

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

---

Changes in v9:
- gic_events_need_delivery: read GICH_APR to find the active group
priority;
- gic_events_need_delivery: simplify the search loop.

Changes in v8:
- in gic_restore_pending_irqs rename i to lr;
- add in code comments in gic_restore_pending_irqs and
gic_events_need_delivery;
- add << 3 to mask_priority.

Changes in v7:
- fix locking for the list_empty case in gic_restore_pending_irqs;
- add in code comment;
- gic_events_need_delivery: break out of the loop as soon as we find the
active irq as inflight_irqs is ordered by priority;
- gic_events_need_delivery: break out of the loop if p->priority is
lower than mask_priority as inflight_irqs is ordered by priority;
- use find_next_zero_bit instead of find_first_zero_bit;
- in gic_restore_pending_irqs remember the last position of the inner
loop search and continue from there;
- in gic_restore_pending_irqs use a priority check to get out of the
inner loop.

Changes in v5:
- improve in code comments;
- use list_for_each_entry_reverse instead of writing my own list walker.

Changes in v4:
- in gic_events_need_delivery go through inflight_irqs and only consider
enabled irqs.
---
 xen/arch/arm/gic.c           |   85 ++++++++++++++++++++++++++++++++++++++----
 xen/include/asm-arm/domain.h |    5 ++-
 xen/include/asm-arm/gic.h    |    3 ++
 3 files changed, 83 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3728182..5e502df 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -650,6 +650,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
     p = irq_to_pending(v, irq);
     if ( lr & GICH_LR_ACTIVE )
     {
+        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
              test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
         {
@@ -673,6 +674,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         if ( p->desc != NULL )
             p->desc->status &= ~IRQ_INPROGRESS;
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+        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) )
@@ -706,20 +708,55 @@ void gic_clear_lrs(struct vcpu *v)
 
 static void gic_restore_pending_irqs(struct vcpu *v)
 {
-    int i;
-    struct pending_irq *p, *t;
+    int lr = 0, lrs = nr_lrs;
+    struct pending_irq *p, *t, *p_r;
+    struct list_head *inflight_r;
     unsigned long flags;
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    if ( list_empty(&v->arch.vgic.lr_pending) )
+        goto out;
+
+    inflight_r = &v->arch.vgic.inflight_irqs;
     list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
     {
-        i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
-        if ( i >= nr_lrs ) return;
+        lr = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, lr);
+        if ( lr >= nr_lrs )
+        {
+            /* No more free LRs: find a lower priority irq to evict */
+            list_for_each_entry_reverse( p_r, inflight_r, inflight )
+            {
+                if ( p_r->priority == p->priority )
+                    goto out;
+                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
+                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
+                    goto found;
+            }
+            /* We didn't find a victim this time, and we won't next
+             * time, so quit */
+            goto out;
+
+found:
+            lr = p_r->lr;
+            p_r->lr = GIC_INVALID_LR;
+            set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
+            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
+            gic_add_to_lr_pending(v, p_r);
+            inflight_r = &p_r->inflight; 
+        }
 
-        gic_set_lr(i, p, GICH_LR_PENDING);
+        gic_set_lr(lr, p, GICH_LR_PENDING);
         list_del_init(&p->lr_queue);
-        set_bit(i, &this_cpu(lr_mask));
+        set_bit(lr, &this_cpu(lr_mask));
+
+        /* We can only evict nr_lrs entries */
+        lrs--;
+        if ( lrs == 0 )
+            break;
     }
+
+out:
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
@@ -736,8 +773,40 @@ void gic_clear_pending_irqs(struct vcpu *v)
 
 int gic_events_need_delivery(void)
 {
-    return (!list_empty(&current->arch.vgic.lr_pending) ||
-            this_cpu(lr_mask));
+    struct vcpu *v = current;
+    struct pending_irq *p;
+    unsigned long flags;
+    const unsigned long apr = GICH[GICH_APR];
+    int mask_priority;
+    int active_priority;
+    int rc = 0;
+
+    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
+    active_priority = find_next_bit(&apr, 32, 0);
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    /* TODO: We order the guest irqs by priority, but we don't change
+     * the priority of host irqs. */
+
+    /* find the first enabled non-active irq, the queue is already
+     * ordered by priority */
+    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
+    {
+        if ( GIC_PRI_TO_GUEST(p->priority) >= mask_priority )
+            goto out;
+        if ( GIC_PRI_TO_GUEST(p->priority) >= active_priority )
+            goto out;
+        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) )
+        {
+            rc = 1;
+            goto out;
+        }
+    }
+
+out:
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+    return rc;
 }
 
 void gic_inject(void)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 59ce196..d689675 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -56,8 +56,9 @@ struct pending_irq
      *
      */
 #define GIC_IRQ_GUEST_QUEUED   0
-#define GIC_IRQ_GUEST_VISIBLE  1
-#define GIC_IRQ_GUEST_ENABLED  2
+#define GIC_IRQ_GUEST_ACTIVE   1
+#define GIC_IRQ_GUEST_VISIBLE  2
+#define GIC_IRQ_GUEST_ENABLED  3
     unsigned long status;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
     int irq;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index aede45c..bf6fb1e 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -129,6 +129,9 @@
 #define GICH_LR_CPUID_SHIFT     9
 #define GICH_VTR_NRLRGS         0x3f
 
+#define GICH_VMCR_PRIORITY_MASK   0x1f
+#define GICH_VMCR_PRIORITY_SHIFT  27
+
 /*
  * The minimum GICC_BPR is required to be in the range 0-3. We set
  * GICC_BPR to 0 but we must expect that it might be 3. This means we
-- 
1.7.10.4

Re: [PATCH v9 04/12] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Julien Grall]

Hi Stefano,

On 06/10/2014 03:07 PM, Stefano Stabellini wrote:
> +static void gic_update_one_lr(struct vcpu *v, int i)
> +{
> +    struct pending_irq *p;
> +    uint32_t lr;
> +    int irq;
> +
> +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> +
> +    lr = GICH[GICH_LR + i];
> +    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> +    {
> +        GICH[GICH_LR + i] = 0;
> +        clear_bit(i, &this_cpu(lr_mask));
> +
> +        irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> +        p = irq_to_pending(v, irq);
> +        if ( p->desc != NULL )
> +            p->desc->status &= ~IRQ_INPROGRESS;

Reading again this patch... shouldn't we take the desc->lock here?

It's possible to receive the same interrupt while we update the LRs
(depending how the IRQ has been physically route) so we may overwrite
the IRQ_INPROGRESS bit.

Regards,

-- 
Julien Grall

Re: [PATCH v9 04/12] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Stefano Stabellini]

On Wed, 11 Jun 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/10/2014 03:07 PM, Stefano Stabellini wrote:
> > +static void gic_update_one_lr(struct vcpu *v, int i)
> > +{
> > +    struct pending_irq *p;
> > +    uint32_t lr;
> > +    int irq;
> > +
> > +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> > +
> > +    lr = GICH[GICH_LR + i];
> > +    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> > +    {
> > +        GICH[GICH_LR + i] = 0;
> > +        clear_bit(i, &this_cpu(lr_mask));
> > +
> > +        irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > +        p = irq_to_pending(v, irq);
> > +        if ( p->desc != NULL )
> > +            p->desc->status &= ~IRQ_INPROGRESS;
> 
> Reading again this patch... shouldn't we take the desc->lock here?
> 
> It's possible to receive the same interrupt while we update the LRs
> (depending how the IRQ has been physically route) so we may overwrite
> the IRQ_INPROGRESS bit.

It is not possible, because we have interrupts disabled at this point.
A later patch introduce

ASSERT(!local_irq_is_enabled());

at the beginning of gic_update_one_lr.

Re: [PATCH v9 04/12] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Julien Grall]

On 12/06/14 16:57, Stefano Stabellini wrote:
> On Wed, 11 Jun 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 06/10/2014 03:07 PM, Stefano Stabellini wrote:
>>> +static void gic_update_one_lr(struct vcpu *v, int i)
>>> +{
>>> +    struct pending_irq *p;
>>> +    uint32_t lr;
>>> +    int irq;
>>> +
>>> +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
>>> +
>>> +    lr = GICH[GICH_LR + i];
>>> +    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
>>> +    {
>>> +        GICH[GICH_LR + i] = 0;
>>> +        clear_bit(i, &this_cpu(lr_mask));
>>> +
>>> +        irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
>>> +        p = irq_to_pending(v, irq);
>>> +        if ( p->desc != NULL )
>>> +            p->desc->status &= ~IRQ_INPROGRESS;
>>
>> Reading again this patch... shouldn't we take the desc->lock here?
>>
>> It's possible to receive the same interrupt while we update the LRs
>> (depending how the IRQ has been physically route) so we may overwrite
>> the IRQ_INPROGRESS bit.
>
> It is not possible, because we have interrupts disabled at this point.
> A later patch introduce
>
> ASSERT(!local_irq_is_enabled());
>
> at the beginning of gic_update_one_lr.

You are assuming this IRQ is routed to the current pCPU. It's not always 
that case, at least without your GICD_ITARGETSR.

Regards,

-- 
Julien Grall

Re: [PATCH v9 04/12] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Stefano Stabellini]

On Thu, 12 Jun 2014, Julien Grall wrote:
> On 12/06/14 16:57, Stefano Stabellini wrote:
> > On Wed, 11 Jun 2014, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 06/10/2014 03:07 PM, Stefano Stabellini wrote:
> > > > +static void gic_update_one_lr(struct vcpu *v, int i)
> > > > +{
> > > > +    struct pending_irq *p;
> > > > +    uint32_t lr;
> > > > +    int irq;
> > > > +
> > > > +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> > > > +
> > > > +    lr = GICH[GICH_LR + i];
> > > > +    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> > > > +    {
> > > > +        GICH[GICH_LR + i] = 0;
> > > > +        clear_bit(i, &this_cpu(lr_mask));
> > > > +
> > > > +        irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > > > +        p = irq_to_pending(v, irq);
> > > > +        if ( p->desc != NULL )
> > > > +            p->desc->status &= ~IRQ_INPROGRESS;
> > > 
> > > Reading again this patch... shouldn't we take the desc->lock here?
> > > 
> > > It's possible to receive the same interrupt while we update the LRs
> > > (depending how the IRQ has been physically route) so we may overwrite
> > > the IRQ_INPROGRESS bit.
> > 
> > It is not possible, because we have interrupts disabled at this point.
> > A later patch introduce
> > 
> > ASSERT(!local_irq_is_enabled());
> > 
> > at the beginning of gic_update_one_lr.
> 
> You are assuming this IRQ is routed to the current pCPU. It's not always that
> case, at least without your GICD_ITARGETSR.

You right.
Rather than adding another lock and making the lock chain more
complicated, I'll change accesses to the p->desc->status flags to be
atomic.

I am undecided whether I should do this at the beginning of the series
and resend a v10, or fix it separately.

Re: [PATCH v9 12/12] xen/arm: gic_events_need_delivery and irq priorities

[Ian Campbell]

On Tue, 2014-06-10 at 15:07 +0100, Stefano Stabellini wrote:
> Introduce GIC_IRQ_GUEST_ACTIVE to track which irqs are currently
> active in the guest.
> 
> gic_events_need_delivery should only return positive if an outstanding
> pending irq has an higher group priority than the currently active group
> priotity and the priority mask.
> Read GICH_APR to find the active group priority.
> Read GICH_VMCR to find the priority mask.
> Find the highest priority non-active enabled irq by going through the
> inflight list.
> 
> In gic_restore_pending_irqs replace lower priority pending (and not
> active) irqs in GICH_LRs with higher priority irqs if no more GICH_LRs
> are available.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v9:
> - gic_events_need_delivery: read GICH_APR to find the active group
> priority;
> - gic_events_need_delivery: simplify the search loop.

Thanks, I can actually grok this version ;-)

Acked this + #9.

AIUI there is no relationship here with "xen/arm: make accesses to
desc->status flags atomic" which fixes an issue which already present
before this series.

There applied (this series, not the atomic flags one, yet). thanks.

Ian.

Re: [PATCH v9 12/12] xen/arm: gic_events_need_delivery and irq priorities

[Ian Campbell]

On Tue, 2014-06-10 at 15:07 +0100, Stefano Stabellini wrote:
> Introduce GIC_IRQ_GUEST_ACTIVE to track which irqs are currently
> active in the guest.
> 
> gic_events_need_delivery should only return positive if an outstanding
> pending irq has an higher group priority than the currently active group
> priotity and the priority mask.
> Read GICH_APR to find the active group priority.
> Read GICH_VMCR to find the priority mask.
> Find the highest priority non-active enabled irq by going through the
> inflight list.
> 
> In gic_restore_pending_irqs replace lower priority pending (and not
> active) irqs in GICH_LRs with higher priority irqs if no more GICH_LRs
> are available.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v9:
> - gic_events_need_delivery: read GICH_APR to find the active group
> priority;
> - gic_events_need_delivery: simplify the search loop.

Thanks, I can actually grok this version ;-)

Acked this + #9.

AIUI there is no relationship here with "xen/arm: make accesses to
desc->status flags atomic" which fixes an issue which already present
before this series.

There applied (this series, not the atomic flags one, yet). thanks.

Ian.

Re: [PATCH v9 12/12] xen/arm: gic_events_need_delivery and irq priorities

[Julien Grall]

On 06/18/2014 11:36 AM, Ian Campbell wrote:
> AIUI there is no relationship here with "xen/arm: make accesses to
> desc->status flags atomic" which fixes an issue which already present
> before this series.

The error was not really there before. We were "safe" because the flags
were cleared before EOIing the IRQ.

Here, we are clearing after EOIing (it has been made by the guest). So
this race condition can happen more often.

Regards,

-- 
Julien Grall

Re: [PATCH v9 12/12] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini]

On Wed, 18 Jun 2014, Julien Grall wrote:
> On 06/18/2014 11:36 AM, Ian Campbell wrote:
> > AIUI there is no relationship here with "xen/arm: make accesses to
> > desc->status flags atomic" which fixes an issue which already present
> > before this series.
> 
> The error was not really there before. We were "safe" because the flags
> were cleared before EOIing the IRQ.
> 
> Here, we are clearing after EOIing (it has been made by the guest). So
> this race condition can happen more often.

However considering that when IRQ_GUEST is set, IRQ_INPROGRESS is only
read by irq.c, I think the window for errors is extremely small.

That doesn't mean we should expose ourselves to risks, so I still
recommend we apply "make accesses to desc->status flags atomic".

Re: [PATCH v9 12/12] xen/arm: gic_events_need_delivery and irq priorities

[Julien Grall]

On 06/18/2014 01:51 PM, Stefano Stabellini wrote:
> On Wed, 18 Jun 2014, Julien Grall wrote:
>> On 06/18/2014 11:36 AM, Ian Campbell wrote:
>>> AIUI there is no relationship here with "xen/arm: make accesses to
>>> desc->status flags atomic" which fixes an issue which already present
>>> before this series.
>>
>> The error was not really there before. We were "safe" because the flags
>> were cleared before EOIing the IRQ.
>>
>> Here, we are clearing after EOIing (it has been made by the guest). So
>> this race condition can happen more often.
> 
> However considering that when IRQ_GUEST is set, IRQ_INPROGRESS is only
> read by irq.c, I think the window for errors is extremely small.
> 
> That doesn't mean we should expose ourselves to risks, so I still
> recommend we apply "make accesses to desc->status flags atomic".

FYI, with my upcoming device passthrough series, I rely of
IRQ_INPROGRESS in release_irq to know if we need to EOI an IRQ routed to
the guest.

So this series is definitely necessary, otherwise we loose the interrupt
for ever.

Regards,

-- 
Julien Grall

Re: [PATCH v9 12/12] xen/arm: gic_events_need_delivery and irq priorities

[Ian Campbell]

On Wed, 2014-06-18 at 13:51 +0100, Stefano Stabellini wrote:
> On Wed, 18 Jun 2014, Julien Grall wrote:
> > On 06/18/2014 11:36 AM, Ian Campbell wrote:
> > > AIUI there is no relationship here with "xen/arm: make accesses to
> > > desc->status flags atomic" which fixes an issue which already present
> > > before this series.
> > 
> > The error was not really there before. We were "safe" because the flags
> > were cleared before EOIing the IRQ.
> > 
> > Here, we are clearing after EOIing (it has been made by the guest). So
> > this race condition can happen more often.
> 
> However considering that when IRQ_GUEST is set, IRQ_INPROGRESS is only
> read by irq.c, I think the window for errors is extremely small.
> 
> That doesn't mean we should expose ourselves to risks, so I still
> recommend we apply "make accesses to desc->status flags atomic".

Agreed, I'll see how sufficiently acked it is when I get that far
through my queue.

Ian.