[PATCH v7 0/12] remove maintenance interrupts

[PATCH v7 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 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: gic_events_need_delivery and irq priorities
      xen/arm: introduce GIC_PRI_TO_GUEST macro

 xen/arch/arm/domain.c        |    2 +-
 xen/arch/arm/gic.c           |  289 +++++++++++++++++++++++++-----------------
 xen/arch/arm/irq.c           |    2 +-
 xen/arch/arm/time.c          |    2 +-
 xen/arch/arm/traps.c         |    9 ++
 xen/arch/arm/vgic.c          |   46 ++++---
 xen/arch/arm/vtimer.c        |    4 +-
 xen/include/asm-arm/domain.h |   25 ++--
 xen/include/asm-arm/gic.h    |   12 +-
 9 files changed, 236 insertions(+), 155 deletions(-)

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

[PATCH v7 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 91a2982..b388ef3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -726,22 +726,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) ||
@@ -754,10 +738,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();
 }
 
 int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
-- 
1.7.10.4

[PATCH v7 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 82a1e79..e6ddedf 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -782,7 +782,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 b388ef3..dbba5d3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -735,7 +735,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 3e326b0..5daa269 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -159,7 +159,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 ba281e9..ae09c6b 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);
 }
 
 /* Route timer's IRQ on this CPU */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 553411d..aab490c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -457,7 +457,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;
 }
@@ -685,7 +685,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 e325f78..87be11e 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 vcpu_domain_init(struct domain *d)
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 071280b..6fce5c2 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -162,7 +162,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 v7 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 |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index dbba5d3..a7b29d8 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -57,6 +57,7 @@ static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
 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
@@ -738,6 +739,11 @@ 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;
 }
 
 int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
-- 
1.7.10.4

[PATCH v7 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 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>

---

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        |  137 +++++++++++++++++++++------------------------
 xen/arch/arm/traps.c      |    9 +++
 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 a7b29d8..b8b1452 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -68,6 +68,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;
@@ -626,16 +628,18 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
 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_reg;
 
     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_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
         ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
+    if ( p->desc != NULL )
+        lr_reg |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT);
+
+    GICH[GICH_LR + lr] = lr_reg;
 
     set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
     clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
@@ -695,6 +699,56 @@ out:
     return;
 }
 
+static void gic_update_one_lr(struct vcpu *v, int i)
+{
+    struct pending_irq *p;
+    uint32_t lr;
+    int irq;
+    bool_t inflight;
+
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+    lr = GICH[GICH_LR + i];
+    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
+    {
+        inflight = 0;
+        GICH[GICH_LR + i] = 0;
+        clear_bit(i, &this_cpu(lr_mask));
+
+        irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
+        spin_lock(&gic.lock);
+        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))
+        {
+            inflight = 1;
+            gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+        }
+        spin_unlock(&gic.lock);
+        if ( !inflight )
+            list_del_init(&p->inflight);
+    }
+}
+
+void gic_clear_lrs(struct vcpu *v)
+{
+    int i = 0;
+    unsigned long flags;
+
+    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;
@@ -893,77 +947,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 21c7b26..38b38a2 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1539,10 +1539,17 @@ bad_data_abort:
     inject_dabt_exception(regs, info.gva, hsr.len);
 }
 
+static void enter_hypervisor_head(void)
+{
+    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();
+
     switch (hsr.ec) {
     case HSR_EC_WFI_WFE:
         if ( !check_conditional_instr(regs, hsr) )
@@ -1620,11 +1627,13 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 
 asmlinkage void do_trap_irq(struct cpu_user_regs *regs)
 {
+    enter_hypervisor_head();
     gic_interrupt(regs, 0);
 }
 
 asmlinkage void do_trap_fiq(struct cpu_user_regs *regs)
 {
+    enter_hypervisor_head();
     gic_interrupt(regs, 1);
 }
 
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index aab490c..566f0ff 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -701,8 +701,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 6fce5c2..ebb90c6 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -220,6 +220,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 v7 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>
---
 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 b8b1452..f1ce9b7 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -56,7 +56,7 @@ static irq_desc_t irq_desc[NR_IRQS];
 static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
 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 v7 06/12] xen/arm: keep track of the GICH_LR used for the irq in struct pending_irq

[Stefano Stabellini]

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

---

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 f1ce9b7..dd63f70 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -643,6 +643,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)
@@ -721,6 +722,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))
         {
@@ -974,7 +976,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 bc20a15..2d94d59 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 v7 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 dd63f70..869c077 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -675,8 +675,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;
@@ -688,7 +688,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;
         }
     }
@@ -727,7 +727,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
         {
             inflight = 1;
-            gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+            gic_raise_guest_irq(v, irq, p->priority);
         }
         spin_unlock(&gic.lock);
         if ( !inflight )
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 566f0ff..3913cf5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -390,7 +390,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 )
             p->desc->handler->enable(p->desc);
         i++;
@@ -719,7 +719,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 ebb90c6..5a9dc77 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -178,8 +178,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);
 extern int gic_route_irq_to_guest(struct domain *d,
                                   const struct dt_irq *irq,
-- 
1.7.10.4

[PATCH v7 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>
---
 xen/arch/arm/gic.c           |    4 ++--
 xen/arch/arm/vgic.c          |    4 ++--
 xen/include/asm-arm/domain.h |   11 ++++++-----
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 869c077..bed6e9c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -642,7 +642,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
     GICH[GICH_LR + lr] = lr_reg;
 
     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;
 }
 
@@ -723,7 +723,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))
         {
             inflight = 1;
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 3913cf5..6a89a1e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -700,7 +700,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;
     }
 
@@ -714,7 +714,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 2d94d59..f96dc12 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 guests 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,12 +36,12 @@ 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 GIC_IRQ_GUEST_QUEUED state until
      * the guest deactivates the irq. However because we are not sure
-     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
+     * when that happens, we simply remove the GIC_IRQ_GUEST_QUEUED
      * 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
+     * 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
@@ -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 v7 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), just clear
GIC_IRQ_GUEST_QUEUED because the irq has already been injected and is
already visible by the guest.
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.

Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq.

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 also 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>

---

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  |   37 ++++++++++++++++++++++++-------------
 xen/arch/arm/vgic.c |   30 +++++++++++++++---------------
 2 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index bed6e9c..13ce703 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -680,6 +680,14 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
 {
     int i;
     unsigned long flags;
+    struct pending_irq *n = irq_to_pending(v, virtual_irq);
+
+    if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status))
+    {
+        if ( v == current )
+            gic_update_one_lr(v, n->lr);
+        return;
+    }
 
     spin_lock_irqsave(&gic.lock, flags);
 
@@ -705,20 +713,27 @@ static void gic_update_one_lr(struct vcpu *v, int i)
     struct pending_irq *p;
     uint32_t lr;
     int irq;
-    bool_t inflight;
 
     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 )
     {
-        inflight = 0;
+        /* HW interrupts cannot be ACTIVE and PENDING */
+        if ( p->desc == NULL &&
+             test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
+            GICH[GICH_LR + i] = lr | GICH_LR_PENDING;
+    } else if ( lr & GICH_LR_PENDING ) {
+        clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
+    } else {
+        spin_lock(&gic.lock);
+
         GICH[GICH_LR + i] = 0;
         clear_bit(i, &this_cpu(lr_mask));
 
-        irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
-        spin_lock(&gic.lock);
-        p = irq_to_pending(v, irq);
         if ( p->desc != NULL )
             p->desc->status &= ~IRQ_INPROGRESS;
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
@@ -726,12 +741,11 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         if ( test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) &&
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
         {
-            inflight = 1;
             gic_raise_guest_irq(v, irq, p->priority);
-        }
-        spin_unlock(&gic.lock);
-        if ( !inflight )
+        } else
             list_del_init(&p->inflight);
+
+        spin_unlock(&gic.lock);
     }
 }
 
@@ -791,9 +805,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);
 
     if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 6a89a1e..435a8d7 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -389,7 +389,11 @@ 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) )
+        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 )
             p->desc->handler->enable(p->desc);
@@ -696,14 +700,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
-    if ( !list_empty(&n->inflight) )
-    {
-        if ( (irq != current->domain->arch.evtchn_irq) ||
-             (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
-            set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
-        goto out;
-    }
-
     /* vcpu offline */
     if ( test_bit(_VPF_down, &v->pause_flags) )
     {
@@ -715,21 +711,25 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     n->irq = irq;
     set_bit(GIC_IRQ_GUEST_QUEUED, &n->status);
-    n->priority = priority;
 
     /* the irq is enabled */
     if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
         gic_raise_guest_irq(v, irq, priority);
 
-    list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
+    if ( list_empty(&n->inflight) )
     {
-        if ( iter->priority > priority )
+        n->priority = priority;
+        list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
         {
-            list_add_tail(&n->inflight, &iter->inflight);
-            goto out;
+            if ( iter->priority > priority )
+            {
+                list_add_tail(&n->inflight, &iter->inflight);
+                goto out;
+            }
         }
+        list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
     }
-    list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
+
 out:
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
     /* we have a new higher priority irq, inject it into the guest */
-- 
1.7.10.4

[PATCH v7 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>

---

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           |   35 ++++++++++++++++-------------------
 xen/arch/arm/vgic.c          |    9 +++++++--
 xen/include/asm-arm/domain.h |    5 ++++-
 3 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 13ce703..e6e6f1a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -119,6 +119,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;
@@ -630,6 +631,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
 {
     uint32_t lr_reg;
 
+    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));
@@ -650,6 +652,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;
 
@@ -669,19 +673,20 @@ 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_guest_irq(struct vcpu *v, unsigned int virtual_irq,
         unsigned int priority)
 {
     int i;
-    unsigned long flags;
     struct pending_irq *n = irq_to_pending(v, virtual_irq);
 
+    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
     if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status))
     {
         if ( v == current )
@@ -689,23 +694,17 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
         return;
     }
 
-    spin_lock_irqsave(&gic.lock, flags);
-
     if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
     {
         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), 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)
@@ -715,6 +714,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;
@@ -729,8 +729,6 @@ static void gic_update_one_lr(struct vcpu *v, int i)
     } else if ( lr & GICH_LR_PENDING ) {
         clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
     } else {
-        spin_lock(&gic.lock);
-
         GICH[GICH_LR + i] = 0;
         clear_bit(i, &this_cpu(lr_mask));
 
@@ -744,8 +742,6 @@ static void gic_update_one_lr(struct vcpu *v, int i)
             gic_raise_guest_irq(v, irq, p->priority);
         } else
             list_del_init(&p->inflight);
-
-        spin_unlock(&gic.lock);
     }
 }
 
@@ -776,11 +772,11 @@ static void gic_restore_pending_irqs(struct vcpu *v)
         i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
         if ( i >= nr_lrs ) return;
 
-        spin_lock_irqsave(&gic.lock, flags);
+        spin_lock_irqsave(&v->arch.vgic.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);
     }
 
 }
@@ -788,13 +784,12 @@ static void gic_restore_pending_irqs(struct vcpu *v)
 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)
@@ -805,6 +800,8 @@ int gic_events_need_delivery(void)
 
 void gic_inject(void)
 {
+    ASSERT(!local_irq_is_enabled());
+
     gic_restore_pending_irqs(current);
 
     if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 435a8d7..c5e17e0 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -393,8 +393,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 )
             p->desc->handler->enable(p->desc);
         i++;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index f96dc12..71f563f 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 v7 11/12] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini]

gic_events_need_delivery should only return positive if an outstanding
pending irq has an higher priority than the currently active irq and the
priority mask.
Introduce GIC_IRQ_GUEST_ACTIVE to track which one is the currently
active guest irq.
Rewrite the function by going through the priority ordered inflight and
lr_queue lists.

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 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           |   84 ++++++++++++++++++++++++++++++++++++++----
 xen/include/asm-arm/domain.h |    5 ++-
 xen/include/asm-arm/gic.h    |    3 ++
 3 files changed, 82 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index e6e6f1a..9295ccf 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -721,6 +721,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);
         /* HW interrupts cannot be ACTIVE and PENDING */
         if ( p->desc == NULL &&
              test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
@@ -735,6 +736,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_QUEUED, &p->status) &&
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
@@ -763,22 +765,53 @@ void gic_clear_lrs(struct vcpu *v)
 
 static void gic_restore_pending_irqs(struct vcpu *v)
 {
-    int i;
-    struct pending_irq *p, *t;
+    int i = 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;
+        i = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, i);
+        if ( i >= nr_lrs )
+        {
+            /* No more free LRs: find a lower priority irq to evict */
+            list_for_each_entry_reverse( p_r, inflight_r, inflight )
+            {
+                inflight_r = &p_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;
+            }
+            goto out;
+
+found:
+            i = 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);
+        }
 
-        spin_lock_irqsave(&v->arch.vgic.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(&v->arch.vgic.lock, flags);
+
+        lrs--;
+        if ( lrs == 0 )
+            break;
     }
 
+out:
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
 void gic_clear_pending_irqs(struct vcpu *v)
@@ -794,8 +827,43 @@ 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));
+    int mask_priority, lrs = nr_lrs;
+    int max_priority = 0xff, active_priority = 0xff;
+    struct vcpu *v = current;
+    struct pending_irq *p;
+    unsigned long flags;
+
+    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
+
+    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. */
+    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
+    {
+        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
+        {
+            if ( p->priority < active_priority )
+                active_priority = p->priority;
+            break;
+        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
+            if ( p->priority < max_priority )
+                max_priority = p->priority;
+        }
+        if ( (p->priority >> 3) >= mask_priority )
+            break;
+        lrs--;
+        if ( lrs == 0 )
+            break;
+    }
+
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+
+    if ( max_priority < active_priority &&
+         (max_priority >> 3) < mask_priority )
+        return 1;
+    else
+        return 0;
 }
 
 void gic_inject(void)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 71f563f..75cc2f3 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 5a9dc77..5d8f7f1 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

[PATCH v7 12/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>
---
 xen/arch/arm/gic.c        |    6 +++---
 xen/include/asm-arm/gic.h |    2 ++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 9295ccf..a4a1ddc 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -636,7 +636,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_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+    lr_reg = 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_reg |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT);
@@ -850,7 +850,7 @@ int gic_events_need_delivery(void)
             if ( p->priority < max_priority )
                 max_priority = p->priority;
         }
-        if ( (p->priority >> 3) >= mask_priority )
+        if ( GIC_PRI_TO_GUEST(p->priority) >= mask_priority )
             break;
         lrs--;
         if ( lrs == 0 )
@@ -860,7 +860,7 @@ int gic_events_need_delivery(void)
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 
     if ( max_priority < active_priority &&
-         (max_priority >> 3) < mask_priority )
+         GIC_PRI_TO_GUEST(max_priority) < mask_priority )
         return 1;
     else
         return 0;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 5d8f7f1..e3dae52 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -152,6 +152,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 prority */
 
 
 #ifndef __ASSEMBLY__
-- 
1.7.10.4

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

[Ian Campbell]

On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> 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 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>

Some minor nits in the event you end up respinning for some other
reason:
> @@ -626,16 +628,18 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>  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_reg;

Calling this lr_val would be clearer (lr_reg sounds like the index of
the register or something to me)
> +static void gic_update_one_lr(struct vcpu *v, int i)
> +{
> +    struct pending_irq *p;
> +    uint32_t lr;
> +    int irq;
> +    bool_t inflight;
> +
> +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> +
> +    lr = GICH[GICH_LR + i];
> +    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )

if ( lr & (GICH_...) )
    return

Then you can pull everything else in a level.

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

[Ian Campbell]

On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> 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.

Did you confirm that this doesn't lead to inefficient loading and
masking stuff on access?

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

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

Although TBH I'm not sure why unsigned int was so harmful here.

Ian.

Re: [PATCH v7 0/12] remove maintenance interrupts

[Julien Grall]

Hi,

On 04/08/2014 04:11 PM, Stefano Stabellini wrote:
> 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.

Just to avoid someone push this serie in its current state by mistake.

This version is not stable on midway, DOM0 get easily stuck when I
compile xen tools.

I bisect quickly across the various version, and the V4 seems the last
one which is stable (i.e not crash after a couple of hours of usage).

Cheers,

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> Rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED and clarify its
> meaning in xen/include/asm-arm/domain.h.

Thanks.

> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 2d94d59..f96dc12 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 guests LRs.

                            guest's

>       *
>       * 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,12 +36,12 @@ 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 GIC_IRQ_GUEST_QUEUED state until
>       * the guest deactivates the irq. However because we are not sure
> -     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
> +     * when that happens, we simply remove the GIC_IRQ_GUEST_QUEUED
>       * state when we add the irq to an LR register. We add it back when
>       * we receive another interrupt notification.

This paragraph was essentially clarifying the mismatch between the name
PENDING and the actual behaviour. It doesn't seem to make much sense for
a bit named QUEUED. In particular "we should keep the QUEUE state until
the guest deactivates the IRQ" doesn't seem logical, so the rest doesn't
follow as a workaround for it.

How about:
      * In order for the state machine to be fully accurate, for level
      * 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 instead track whether there is an interrupt
      * queued using GIC_IRQ_GUEST_QUEUED
      * state when we add the irq to an LR register. We add it back when
      * we receive another interrupt notification.

(needs rewrapping). Still doesn't sound quite right to me, but you
understand what is going on better than me so I hope you can fix it ;-)

> -     * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the
> +     * 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

Ian.

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

[Julien Grall]

On 04/23/2014 01:47 PM, Ian Campbell wrote:
> On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
>> 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.
> 
> Did you confirm that this doesn't lead to inefficient loading and
> masking stuff on access?
> 
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Acked-by: Julien Grall <julien.grall@linaro.org>
> 
> If yes then:
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Although TBH I'm not sure why unsigned int was so harmful here.

In struct irq_pending (see next patch: #6) the lr is stored using an uint8.

IHMO, the both variable should have the same type to avoid mistake.

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2014-04-23 at 13:45 +0100, Ian Campbell wrote:
> > +static void gic_update_one_lr(struct vcpu *v, int i)
> > +{
> > +    struct pending_irq *p;
> > +    uint32_t lr;
> > +    int irq;
> > +    bool_t inflight;
> > +
> > +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> > +
> > +    lr = GICH[GICH_LR + i];
> > +    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> 
> if ( lr & (GICH_...) )
>     return
> 
> Then you can pull everything else in a level.

Having read patch #9 I guess this doesn't actually make sense, since
things change there...

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

[Ian Campbell]

On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> +    if ( lr & GICH_LR_ACTIVE )
>      {
> -        inflight = 0;
> +        /* HW interrupts cannot be ACTIVE and PENDING */
> +        if ( p->desc == NULL &&
> +             test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> +             test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
> +            GICH[GICH_LR + i] = lr | GICH_LR_PENDING;

Do we really need to take no action for a HW interrupt here?

We leave it QUEUED and then something else later will inject another
one? What does that? Can you expand the comment to explain, "HW
interrupts cannot be ... and therefore we leave it active and later
on ..." etc.

> +    } else {
> +        spin_lock(&gic.lock);

It probably doesn't matter much, but do you need to expand the scope of
the lock or could you leave it where it was?

Ian.

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

[Ian Campbell]

On Wed, 2014-04-23 at 13:53 +0100, Julien Grall wrote:
> On 04/23/2014 01:47 PM, Ian Campbell wrote:
> > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> >> 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.
> > 
> > Did you confirm that this doesn't lead to inefficient loading and
> > masking stuff on access?
> > 
> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >> Acked-by: Julien Grall <julien.grall@linaro.org>
> > 
> > If yes then:
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Although TBH I'm not sure why unsigned int was so harmful here.
> 
> In struct irq_pending (see next patch: #6) the lr is stored using an uint8.
> 
> IHMO, the both variable should have the same type to avoid mistake.

That could easily be avoided by a range check before setting nr_lrs,
which we must have to do anyway.

Ian.

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

[Julien Grall]

On 04/23/2014 02:07 PM, Ian Campbell wrote:
> On Wed, 2014-04-23 at 13:53 +0100, Julien Grall wrote:
>> On 04/23/2014 01:47 PM, Ian Campbell wrote:
>>> On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
>>>> 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.
>>>
>>> Did you confirm that this doesn't lead to inefficient loading and
>>> masking stuff on access?
>>>
>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>> Acked-by: Julien Grall <julien.grall@linaro.org>
>>>
>>> If yes then:
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> Although TBH I'm not sure why unsigned int was so harmful here.
>>
>> In struct irq_pending (see next patch: #6) the lr is stored using an uint8.
>>
>> IHMO, the both variable should have the same type to avoid mistake.
> 
> That could easily be avoided by a range check before setting nr_lrs,
> which we must have to do anyway.

We might want to check against the hardcoded value used to create gic_lr
(i.e 64).

For your sentence "Did you confirm that this doesn't lead to inefficient
loading and masking stuff on access?", there is an instruction to only
load/store 8 bits (strb, ldrb).
I don't see how the compiler will do inefficient loading.

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> gic_events_need_delivery should only return positive if an outstanding
> pending irq has an higher priority than the currently active irq and the
> priority mask.
> Introduce GIC_IRQ_GUEST_ACTIVE to track which one is the currently
> active guest irq.

There can be multiple such interrupt, can't there? In which case "which
ones are the currently active guest irqs" or "which IRQs are currently
active" or something.

> Rewrite the function by going through the priority ordered inflight and
> lr_queue lists.
> 
> 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 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           |   84 ++++++++++++++++++++++++++++++++++++++----
>  xen/include/asm-arm/domain.h |    5 ++-
>  xen/include/asm-arm/gic.h    |    3 ++
>  3 files changed, 82 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index e6e6f1a..9295ccf 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -721,6 +721,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);
>          /* HW interrupts cannot be ACTIVE and PENDING */
>          if ( p->desc == NULL &&
>               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> @@ -735,6 +736,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_QUEUED, &p->status) &&
>                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> @@ -763,22 +765,53 @@ void gic_clear_lrs(struct vcpu *v)
>  
>  static void gic_restore_pending_irqs(struct vcpu *v)
>  {
> -    int i;
> -    struct pending_irq *p, *t;
> +    int i = 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;
> +        i = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, i);

While you are rewriting this then renaming the varialbe i to "lr" would
be a lot clearer.

> +        if ( i >= nr_lrs )
> +        {
> +            /* No more free LRs: find a lower priority irq to evict */
> +            list_for_each_entry_reverse( p_r, inflight_r, inflight )
> +            {
> +                inflight_r = &p_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;
> +            }

Please can you add a comment here:
		/* We didn't find a victim this time, and we won't next time, so quit */

> +            goto out;
> +
> +found:
> +            i = 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);
> +        }
>  
> -        spin_lock_irqsave(&v->arch.vgic.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(&v->arch.vgic.lock, flags);
> +
> +        lrs--;
> +        if ( lrs == 0 )
> +            break;
>      }
>  
> +out:
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>  }
>  
>  void gic_clear_pending_irqs(struct vcpu *v)
> @@ -794,8 +827,43 @@ 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));
> +    int mask_priority, lrs = nr_lrs;
> +    int max_priority = 0xff, active_priority = 0xff;
> +    struct vcpu *v = current;
> +    struct pending_irq *p;
> +    unsigned long flags;
> +
> +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;

mask_priority is a bit meaningless (I know the docs call it
priority_mask), but its the vcpus current priority, right?

Also, by adding a << 3 here then I think the rest of the logic reads
more easily, because you are then using the priority directly, and
ignoring the fact that VMCR has a limited precision. Whereas with the
shift >> 3 at the comparison sights you kind of have to think about it
in each case.

> +
> +    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. */
> +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> +    {
> +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> +        {
> +            if ( p->priority < active_priority )
> +                active_priority = p->priority;
> +            break;
> +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> +            if ( p->priority < max_priority )
> +                max_priority = p->priority;
> +        }
> +        if ( (p->priority >> 3) >= mask_priority )
> +            break;

This lrs-- stuff needs a comment. I think along the lines of only the
first nr_lrs interrupt need to be considered because XXX.

Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
entries on it (say) of which the first 5 happen to be masked, don't we
want to keep going until we've looked at more than the first 4 entries
or something? Or should this decrement be conditional on ENABLE and/or
ACTIVE perhaps?

You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
haven't yet seen an active one then don't you know that max_priority <
active_priority? (this might be best discussed around a whiteboard...)


> +        lrs--;
> +        if ( lrs == 0 )
> +            break;
> +    }
> +
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +
> +    if ( max_priority < active_priority &&
> +         (max_priority >> 3) < mask_priority )
> +        return 1;
> +    else
> +        return 0;
>  }
>  
>  void gic_inject(void)

Ian

Re: [PATCH v7 12/12] xen/arm: introduce GIC_PRI_TO_GUEST macro

[Ian Campbell]

On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> 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>

But my comments on #11 might change some of this...
> +										    5 bits for guest irq prority */

typo: priority

also hard tabs.

>  
> 
>  #ifndef __ASSEMBLY__

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

[Stefano Stabellini]

On Wed, 23 Apr 2014, Ian Campbell wrote:
> On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > Rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED and clarify its
> > meaning in xen/include/asm-arm/domain.h.
> 
> Thanks.
> 
> > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > index 2d94d59..f96dc12 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 guests LRs.
> 
>                             guest's
> 
> >       *
> >       * 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,12 +36,12 @@ 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 GIC_IRQ_GUEST_QUEUED state until
> >       * the guest deactivates the irq. However because we are not sure
> > -     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
> > +     * when that happens, we simply remove the GIC_IRQ_GUEST_QUEUED
> >       * state when we add the irq to an LR register. We add it back when
> >       * we receive another interrupt notification.
> 
> This paragraph was essentially clarifying the mismatch between the name
> PENDING and the actual behaviour. It doesn't seem to make much sense for
> a bit named QUEUED. In particular "we should keep the QUEUE state until
> the guest deactivates the IRQ" doesn't seem logical, so the rest doesn't
> follow as a workaround for it.
> 
> How about:
>       * In order for the state machine to be fully accurate, for level
>       * 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 instead track whether there is an interrupt
>       * queued using GIC_IRQ_GUEST_QUEUED
>       * state when we add the irq to an LR register. We add it back when
>       * we receive another interrupt notification.
> 
> (needs rewrapping). Still doesn't sound quite right to me, but you
> understand what is going on better than me so I hope you can fix it ;-)

If I make this change then this patch won't be a pure renaming anymore.
Should I add another patch for this?


> > -     * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the
> > +     * 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
> 
> Ian.
> 

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

[Stefano Stabellini]

On Wed, 23 Apr 2014, Ian Campbell wrote:
> On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > gic_events_need_delivery should only return positive if an outstanding
> > pending irq has an higher priority than the currently active irq and the
> > priority mask.
> > Introduce GIC_IRQ_GUEST_ACTIVE to track which one is the currently
> > active guest irq.
> 
> There can be multiple such interrupt, can't there? In which case "which
> ones are the currently active guest irqs" or "which IRQs are currently
> active" or something.
> 
> > Rewrite the function by going through the priority ordered inflight and
> > lr_queue lists.
> > 
> > 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 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           |   84 ++++++++++++++++++++++++++++++++++++++----
> >  xen/include/asm-arm/domain.h |    5 ++-
> >  xen/include/asm-arm/gic.h    |    3 ++
> >  3 files changed, 82 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index e6e6f1a..9295ccf 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -721,6 +721,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);
> >          /* HW interrupts cannot be ACTIVE and PENDING */
> >          if ( p->desc == NULL &&
> >               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > @@ -735,6 +736,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_QUEUED, &p->status) &&
> >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > @@ -763,22 +765,53 @@ void gic_clear_lrs(struct vcpu *v)
> >  
> >  static void gic_restore_pending_irqs(struct vcpu *v)
> >  {
> > -    int i;
> > -    struct pending_irq *p, *t;
> > +    int i = 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;
> > +        i = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, i);
> 
> While you are rewriting this then renaming the varialbe i to "lr" would
> be a lot clearer.
> 
> > +        if ( i >= nr_lrs )
> > +        {
> > +            /* No more free LRs: find a lower priority irq to evict */
> > +            list_for_each_entry_reverse( p_r, inflight_r, inflight )
> > +            {
> > +                inflight_r = &p_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;
> > +            }
> 
> Please can you add a comment here:
> 		/* We didn't find a victim this time, and we won't next time, so quit */
> 
> > +            goto out;
> > +
> > +found:
> > +            i = 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);
> > +        }
> >  
> > -        spin_lock_irqsave(&v->arch.vgic.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(&v->arch.vgic.lock, flags);
> > +
> > +        lrs--;
> > +        if ( lrs == 0 )
> > +            break;
> >      }
> >  
> > +out:
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >  }
> >  
> >  void gic_clear_pending_irqs(struct vcpu *v)
> > @@ -794,8 +827,43 @@ 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));
> > +    int mask_priority, lrs = nr_lrs;
> > +    int max_priority = 0xff, active_priority = 0xff;
> > +    struct vcpu *v = current;
> > +    struct pending_irq *p;
> > +    unsigned long flags;
> > +
> > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> 
> mask_priority is a bit meaningless (I know the docs call it
> priority_mask), but its the vcpus current priority, right?

It is the minimum priority that an interrupt needs to have for the cpu
to be interrupted by the gic.


> Also, by adding a << 3 here then I think the rest of the logic reads
> more easily, because you are then using the priority directly, and
> ignoring the fact that VMCR has a limited precision. Whereas with the
> shift >> 3 at the comparison sights you kind of have to think about it
> in each case.
>
> > +
> > +    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. */
> > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > +    {
> > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > +        {
> > +            if ( p->priority < active_priority )
> > +                active_priority = p->priority;
> > +            break;
> > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > +            if ( p->priority < max_priority )
> > +                max_priority = p->priority;
> > +        }
> > +        if ( (p->priority >> 3) >= mask_priority )
> > +            break;
> 
> This lrs-- stuff needs a comment. I think along the lines of only the
> first nr_lrs interrupt need to be considered because XXX.
> 
> Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
> entries on it (say) of which the first 5 happen to be masked, don't we
> want to keep going until we've looked at more than the first 4 entries
> or something? Or should this decrement be conditional on ENABLE and/or
> ACTIVE perhaps?

If we have 4 lrs and inflight_irqs has 10 entries on it (say) of which
the first 5 happen to be masked, we want to keep going until we've
looked at more than the first 4 entries but we certainly cannot swap
more than 4 entries.


> You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> haven't yet seen an active one then don't you know that max_priority <
> active_priority? (this might be best discussed around a whiteboard...)
>
> > +        lrs--;
> > +        if ( lrs == 0 )
> > +            break;
> > +    }
> > +
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +
> > +    if ( max_priority < active_priority &&
> > +         (max_priority >> 3) < mask_priority )
> > +        return 1;
> > +    else
> > +        return 0;
> >  }
> >  
> >  void gic_inject(void)
> 
> Ian
> 

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

[Ian Campbell]

On Thu, 2014-05-08 at 18:57 +0100, Stefano Stabellini wrote:
> On Wed, 23 Apr 2014, Ian Campbell wrote:
> > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > > Rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED and clarify its
> > > meaning in xen/include/asm-arm/domain.h.
> > 
> > Thanks.
> > 
> > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > > index 2d94d59..f96dc12 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 guests LRs.
> > 
> >                             guest's
> > 
> > >       *
> > >       * 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,12 +36,12 @@ 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 GIC_IRQ_GUEST_QUEUED state until
> > >       * the guest deactivates the irq. However because we are not sure
> > > -     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
> > > +     * when that happens, we simply remove the GIC_IRQ_GUEST_QUEUED
> > >       * state when we add the irq to an LR register. We add it back when
> > >       * we receive another interrupt notification.
> > 
> > This paragraph was essentially clarifying the mismatch between the name
> > PENDING and the actual behaviour. It doesn't seem to make much sense for
> > a bit named QUEUED. In particular "we should keep the QUEUE state until
> > the guest deactivates the IRQ" doesn't seem logical, so the rest doesn't
> > follow as a workaround for it.
> > 
> > How about:
> >       * In order for the state machine to be fully accurate, for level
> >       * 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 instead track whether there is an interrupt
> >       * queued using GIC_IRQ_GUEST_QUEUED
> >       * state when we add the irq to an LR register. We add it back when
> >       * we receive another interrupt notification.
> > 
> > (needs rewrapping). Still doesn't sound quite right to me, but you
> > understand what is going on better than me so I hope you can fix it ;-)
> 
> If I make this change then this patch won't be a pure renaming anymore.
> Should I add another patch for this?

I think renaming and updating the docs/comments to reflect the renaming
is OK in a single patch (whereas renaming and actual functional change
wouldn't, but I don't think that is what either of us was suggesting).

Ian.

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

[Ian Campbell]

On Thu, 2014-05-08 at 19:37 +0100, Stefano Stabellini wrote:
> On Wed, 23 Apr 2014, Ian Campbell wrote:
> > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> > 
> > mask_priority is a bit meaningless (I know the docs call it
> > priority_mask), but its the vcpus current priority, right?
> 
> It is the minimum priority that an interrupt needs to have for the cpu
> to be interrupted by the gic.

OK, I think you are stating the same thing as me but from a different
angle.
> 
> 
> > Also, by adding a << 3 here then I think the rest of the logic reads
> > more easily, because you are then using the priority directly, and
> > ignoring the fact that VMCR has a limited precision. Whereas with the
> > shift >> 3 at the comparison sights you kind of have to think about it
> > in each case.
> >
> > > +
> > > +    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. */
> > > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > > +    {
> > > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > +        {
> > > +            if ( p->priority < active_priority )
> > > +                active_priority = p->priority;
> > > +            break;
> > > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > > +            if ( p->priority < max_priority )
> > > +                max_priority = p->priority;
> > > +        }
> > > +        if ( (p->priority >> 3) >= mask_priority )
> > > +            break;
> > 
> > This lrs-- stuff needs a comment. I think along the lines of only the
> > first nr_lrs interrupt need to be considered because XXX.
> > 
> > Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
> > entries on it (say) of which the first 5 happen to be masked, don't we
> > want to keep going until we've looked at more than the first 4 entries
> > or something? Or should this decrement be conditional on ENABLE and/or
> > ACTIVE perhaps?
> 
> If we have 4 lrs and inflight_irqs has 10 entries on it (say) of which
> the first 5 happen to be masked, we want to keep going until we've
> looked at more than the first 4 entries but we certainly cannot swap
> more than 4 entries.

I think what is confusing me is that I don't see where the lrs-- is
skipped for a masked interrupt. So aren't you counting down the lrs
variable even for the first 5 which happen to be masked?

> > You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> > exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> > haven't yet seen an active one then don't you know that max_priority <
> > active_priority? (this might be best discussed around a whiteboard...)
> >
> > > +        lrs--;
> > > +        if ( lrs == 0 )
> > > +            break;
> > > +    }
> > > +
> > > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > > +
> > > +    if ( max_priority < active_priority &&
> > > +         (max_priority >> 3) < mask_priority )
> > > +        return 1;
> > > +    else
> > > +        return 0;
> > >  }
> > >  
> > >  void gic_inject(void)
> > 
> > Ian
> > 

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

[Stefano Stabellini]

> > On Wed, 23 Apr 2014, Ian Campbell wrote:
> > > On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > > > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> > > 
> > > mask_priority is a bit meaningless (I know the docs call it
> > > priority_mask), but its the vcpus current priority, right?
> > 
> > It is the minimum priority that an interrupt needs to have for the cpu
> > to be interrupted by the gic.
> 
> OK, I think you are stating the same thing as me but from a different
> angle.
> > 
> > 
> > > Also, by adding a << 3 here then I think the rest of the logic reads
> > > more easily, because you are then using the priority directly, and
> > > ignoring the fact that VMCR has a limited precision. Whereas with the
> > > shift >> 3 at the comparison sights you kind of have to think about it
> > > in each case.
> > >
> > > > +
> > > > +    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. */
> > > > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > > > +    {
> > > > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > > > +        {
> > > > +            if ( p->priority < active_priority )
> > > > +                active_priority = p->priority;
> > > > +            break;
> > > > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > > > +            if ( p->priority < max_priority )
> > > > +                max_priority = p->priority;
> > > > +        }
> > > > +        if ( (p->priority >> 3) >= mask_priority )
> > > > +            break;
> > > 
> > > This lrs-- stuff needs a comment. I think along the lines of only the
> > > first nr_lrs interrupt need to be considered because XXX.
> > > 
> > > Not sure what XXX is -- if we have 4 lrs and inflight_irqs has 10
> > > entries on it (say) of which the first 5 happen to be masked, don't we
> > > want to keep going until we've looked at more than the first 4 entries
> > > or something? Or should this decrement be conditional on ENABLE and/or
> > > ACTIVE perhaps?
> > 
> > If we have 4 lrs and inflight_irqs has 10 entries on it (say) of which
> > the first 5 happen to be masked, we want to keep going until we've
> > looked at more than the first 4 entries but we certainly cannot swap
> > more than 4 entries.
> 
> I think what is confusing me is that I don't see where the lrs-- is
> skipped for a masked interrupt. So aren't you counting down the lrs
> variable even for the first 5 which happen to be masked?

At the moment when the guest disables an irq, we remove it from the
lr_pending queue but we don't remove it from the inflight queue. So if
the irq has already been added to an LR register, the guest is going to
receive a notification still.

This patch doesn't change this behaviour: the eviction code in
gic_restore_pending_irqs doesn't distinguish between masked and unmasked
irqs, it treats them the same way, simply going by priority.
Consistently in gic_events_need_delivery, we only analyze the first
nr_lrs irqs by priority, regardless if they are masked or unmasked.

To answer your original question: no, we don't need to keep going past
the first 4 irqs in inflight_irqs, even if they are all masked.

Admittedly this behaviour could be improved, but it might be best to fix
it in a consequent patch series.


> > > You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> > > exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> > > haven't yet seen an active one then don't you know that max_priority <
> > > active_priority? (this might be best discussed around a whiteboard...)
> > >
> > > > +        lrs--;
> > > > +        if ( lrs == 0 )
> > > > +            break;
> > > > +    }
> > > > +
> > > > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > > > +
> > > > +    if ( max_priority < active_priority &&
> > > > +         (max_priority >> 3) < mask_priority )
> > > > +        return 1;
> > > > +    else
> > > > +        return 0;
> > > >  }
> > > >  
> > > >  void gic_inject(void)
> > > 
> > > Ian
> > > 
> 
> 

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

[Stefano Stabellini]

On Wed, 23 Apr 2014, Ian Campbell wrote:
> On Tue, 2014-04-08 at 16:12 +0100, Stefano Stabellini wrote:
> > gic_events_need_delivery should only return positive if an outstanding
> > pending irq has an higher priority than the currently active irq and the
> > priority mask.
> > Introduce GIC_IRQ_GUEST_ACTIVE to track which one is the currently
> > active guest irq.
> 
> There can be multiple such interrupt, can't there? In which case "which
> ones are the currently active guest irqs" or "which IRQs are currently
> active" or something.
> 
> > Rewrite the function by going through the priority ordered inflight and
> > lr_queue lists.
> > 
> > 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 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           |   84 ++++++++++++++++++++++++++++++++++++++----
> >  xen/include/asm-arm/domain.h |    5 ++-
> >  xen/include/asm-arm/gic.h    |    3 ++
> >  3 files changed, 82 insertions(+), 10 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index e6e6f1a..9295ccf 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -721,6 +721,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);
> >          /* HW interrupts cannot be ACTIVE and PENDING */
> >          if ( p->desc == NULL &&
> >               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > @@ -735,6 +736,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_QUEUED, &p->status) &&
> >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > @@ -763,22 +765,53 @@ void gic_clear_lrs(struct vcpu *v)
> >  
> >  static void gic_restore_pending_irqs(struct vcpu *v)
> >  {
> > -    int i;
> > -    struct pending_irq *p, *t;
> > +    int i = 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;
> > +        i = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, i);
> 
> While you are rewriting this then renaming the varialbe i to "lr" would
> be a lot clearer.
> 
> > +        if ( i >= nr_lrs )
> > +        {
> > +            /* No more free LRs: find a lower priority irq to evict */
> > +            list_for_each_entry_reverse( p_r, inflight_r, inflight )
> > +            {
> > +                inflight_r = &p_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;
> > +            }
> 
> Please can you add a comment here:
> 		/* We didn't find a victim this time, and we won't next time, so quit */
> 
> > +            goto out;
> > +
> > +found:
> > +            i = 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);
> > +        }
> >  
> > -        spin_lock_irqsave(&v->arch.vgic.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(&v->arch.vgic.lock, flags);
> > +
> > +        lrs--;
> > +        if ( lrs == 0 )
> > +            break;
> >      }
> >  
> > +out:
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >  }
> >  
> >  void gic_clear_pending_irqs(struct vcpu *v)
> > @@ -794,8 +827,43 @@ 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));
> > +    int mask_priority, lrs = nr_lrs;
> > +    int max_priority = 0xff, active_priority = 0xff;
> > +    struct vcpu *v = current;
> > +    struct pending_irq *p;
> > +    unsigned long flags;
> > +
> > +    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
> 
> mask_priority is a bit meaningless (I know the docs call it
> priority_mask), but its the vcpus current priority, right?
> 
> Also, by adding a << 3 here then I think the rest of the logic reads
> more easily, because you are then using the priority directly, and
> ignoring the fact that VMCR has a limited precision. Whereas with the
> shift >> 3 at the comparison sights you kind of have to think about it
> in each case.
> 
> > +
> > +    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. */
> > +    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
> > +    {
> > +        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
> > +        {
> > +            if ( p->priority < active_priority )
> > +                active_priority = p->priority;
> > +            break;
> > +        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
> > +            if ( p->priority < max_priority )
> > +                max_priority = p->priority;
> > +        }
> > +        if ( (p->priority >> 3) >= mask_priority )
> > +            break;
> 
> You exit the loop as soon as you see an ACTIVE IRQ, but can't you also
> exit if you see an ENABLED IRQ? If you see an enabled IRQ but you
> haven't yet seen an active one then don't you know that max_priority <
> active_priority? (this might be best discussed around a whiteboard...)

That would not be correct: if the current irq (p) has the same priority
as the active irq but it has been evaluated first, then if we break
immediately we would be led to think that there is an irq that needs to
be injected but actually there isn't one.