[PATCH v5 0/10] remove maintenance interrupts

[PATCH v5 0/10] 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 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 (10):
      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: 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/arch/arm/domain.c        |    2 +-
 xen/arch/arm/gic.c           |  282 +++++++++++++++++++++++++-----------------
 xen/arch/arm/irq.c           |    2 +-
 xen/arch/arm/time.c          |    2 +-
 xen/arch/arm/traps.c         |   10 ++
 xen/arch/arm/vgic.c          |   47 +++----
 xen/arch/arm/vtimer.c        |    4 +-
 xen/include/asm-arm/domain.h |   14 ++-
 xen/include/asm-arm/gic.h    |   10 +-
 9 files changed, 225 insertions(+), 148 deletions(-)

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

[PATCH v5 01/10] 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 v5 02/10] 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 v5 03/10] 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>

---

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 v5 04/10] 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 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        |  142 ++++++++++++++++++++++-----------------------
 xen/arch/arm/traps.c      |   10 ++++
 xen/arch/arm/vgic.c       |    3 +-
 xen/include/asm-arm/gic.h |    1 +
 4 files changed, 81 insertions(+), 75 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index a7b29d8..3262db3 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_clear_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,61 @@ out:
     return;
 }
 
+static void gic_clear_one_lr(struct vcpu *v, int i)
+{
+    struct pending_irq *p;
+    uint32_t lr;
+    int irq;
+    bool_t inflight;
+
+    ASSERT(!local_irq_is_enabled());
+    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 )
+        {
+            spin_lock(&v->arch.vgic.lock);
+            list_del_init(&p->inflight);
+            spin_unlock(&v->arch.vgic.lock);
+        }
+    }
+}
+
+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_clear_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 +952,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++;
-    }
+    /* 
+     * The maintenance interrupt handler doesn'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.
+     */
 }
 
 void gic_dump_info(struct vcpu *v)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 21c7b26..dd936be 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -68,6 +68,7 @@ static int debug_stack_lines = 40;
 
 integer_param("debug_stack_lines", debug_stack_lines);
 
+static void enter_hypervisor_head(void);
 
 void __cpuinit init_traps(void)
 {
@@ -1543,6 +1544,8 @@ 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 +1623,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);
 }
 
@@ -1642,6 +1647,11 @@ asmlinkage void leave_hypervisor_tail(void)
     }
 }
 
+static void enter_hypervisor_head(void)
+{
+    gic_clear_lrs(current);
+}
+
 /*
  * Local variables:
  * mode: C
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 v5 05/10] xen/arm: nr_lrs should be uint8_t

[Stefano Stabellini]

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3262db3..5cdcd15 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 v5 06/10] 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>

---

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 5cdcd15..13990db 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)
@@ -722,6 +723,7 @@ static void gic_clear_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))
         {
@@ -979,7 +981,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 v5 07/10] 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>
---
 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 13990db..0ebfbf0 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;
         }
     }
@@ -728,7 +728,7 @@ static void gic_clear_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 v5 08/10] 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_PENDING 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_PENDING
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 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  |   41 ++++++++++++++++++++++++-----------------
 xen/arch/arm/vgic.c |   33 +++++++++++++++++----------------
 2 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 0ebfbf0..77bdfe7 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_clear_one_lr(v, n->lr);
+        return;
+    }
 
     spin_lock_irqsave(&gic.lock, flags);
 
@@ -705,21 +713,28 @@ static void gic_clear_one_lr(struct vcpu *v, int i)
     struct pending_irq *p;
     uint32_t lr;
     int irq;
-    bool_t inflight;
 
     ASSERT(!local_irq_is_enabled());
     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_PENDING, &p->status) )
+            GICH[GICH_LR + i] = lr | GICH_LR_PENDING;
+    } else if ( lr & GICH_LR_PENDING ) {
+        clear_bit(GIC_IRQ_GUEST_PENDING, &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);
@@ -727,16 +742,11 @@ static void gic_clear_one_lr(struct vcpu *v, int i)
         if ( test_bit(GIC_IRQ_GUEST_PENDING, &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 )
-        {
-            spin_lock(&v->arch.vgic.lock);
+        } else
             list_del_init(&p->inflight);
-            spin_unlock(&v->arch.vgic.lock);
-        }
+
+        spin_unlock(&gic.lock);
     }
 }
 
@@ -796,9 +806,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 3913cf5..dc3a75f 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_PENDING, &n->status);
-        goto out;
-    }
-
     /* vcpu offline */
     if ( test_bit(_VPF_down, &v->pause_flags) )
     {
@@ -715,21 +711,26 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     n->irq = irq;
     set_bit(GIC_IRQ_GUEST_PENDING, &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);
+    } else if ( n->priority != priority )
+        gdprintk(XENLOG_WARNING, "Changing priority of an inflight interrupt is not supported");
+
 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 v5 09/10] 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>

---

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 77bdfe7..5450c8a 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_clear_one_lr(struct vcpu *v, int i)
@@ -717,6 +716,8 @@ static void gic_clear_one_lr(struct vcpu *v, int i)
     ASSERT(!local_irq_is_enabled());
     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;
     p = irq_to_pending(v, irq);
@@ -730,8 +731,6 @@ static void gic_clear_one_lr(struct vcpu *v, int i)
     } else if ( lr & GICH_LR_PENDING ) {
         clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
     } else {
-        spin_lock(&gic.lock);
-
         GICH[GICH_LR + i] = 0;
         clear_bit(i, &this_cpu(lr_mask));
 
@@ -745,8 +744,6 @@ static void gic_clear_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);
     }
 }
 
@@ -777,11 +774,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);
     }
 
 }
@@ -789,13 +786,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)
@@ -806,6 +802,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 dc3a75f..bd15be7 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 2d94d59..dcbeba1 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -67,7 +67,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 v5 10/10] 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.
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 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           |   77 ++++++++++++++++++++++++++++++++++++++----
 xen/include/asm-arm/domain.h |    5 +--
 xen/include/asm-arm/gic.h    |    3 ++
 3 files changed, 76 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5450c8a..6442d44 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -723,6 +723,7 @@ static void gic_clear_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) &&
@@ -737,6 +738,7 @@ static void gic_clear_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_PENDING, &p->status) &&
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
@@ -765,22 +767,51 @@ 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;
     unsigned long flags;
 
+    if ( list_empty(&v->arch.vgic.lr_pending) )
+        return;
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
     list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
     {
         i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
-        if ( i >= nr_lrs ) return;
+        if ( i >= nr_lrs )
+        {
+            list_for_each_entry_reverse( p_r,
+                                         &v->arch.vgic.inflight_irqs,
+                                         inflight )
+            {
+                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
+                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
+                    goto found;
+                if ( &p_r->inflight == p->inflight.next )
+                    goto out;
+            }
+            goto out;
+
+found:
+            i = p_r->lr;
+            p_r->lr = GIC_INVALID_LR;
+            set_bit(GIC_IRQ_GUEST_PENDING, &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)
@@ -796,8 +827,40 @@ void gic_clear_pending_irqs(struct vcpu *v)
 
 int gic_events_need_delivery(void)
 {
-    return (!list_empty(&current->arch.vgic.lr_pending) ||
-            this_cpu(lr_mask));
+    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;
+        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
+            if ( p->priority < max_priority )
+                max_priority = p->priority;
+        }
+        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 dcbeba1..696f36c 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -55,8 +55,9 @@ struct pending_irq
      *
      */
 #define GIC_IRQ_GUEST_PENDING  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

Re: [PATCH v5 05/10] xen/arm: nr_lrs should be uint8_t

[Julien Grall]

On 03/24/2014 06:49 PM, Stefano Stabellini wrote:
> 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 3262db3..5cdcd15 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
> 


-- 
Julien Grall

Re: [PATCH v5 03/10] xen/arm: set GICH_HCR_UIE if all the LRs are in use

[Ian Campbell]

On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> 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>
> 
> ---
> 
> Changes in v5:
> - introduce lr_all_full() helper.

I see you've also added the comment to the interrupt handler which was
requested last time but in the next patch where it also logically
belongs. Make sense.

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

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

[Ian Campbell]

On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
>  
> +static void gic_clear_one_lr(struct vcpu *v, int i)
> +{
> +    struct pending_irq *p;
> +    uint32_t lr;
> +    int irq;
> +    bool_t inflight;
> +
> +    ASSERT(!local_irq_is_enabled());
> +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
[...]
> +        if ( !inflight )
> +        {
> +            spin_lock(&v->arch.vgic.lock);

Aren't you recursively taking this lock here?

In principal could you remove the requirement for the caller to hold
this lock as long as you took it just before p = irq_to_pending(v, irq);
and released it on each iteration? (I'm not at all sure of that, just
wondering)

> +            list_del_init(&p->inflight);
> +            spin_unlock(&v->arch.vgic.lock);
> +        }
> +    }
> +}
> +
> +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_clear_one_lr(v, i);
> +        i++;
> +    }
> +
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +}
> +
>  static void gic_restore_pending_irqs(struct vcpu *v)
>  {
>      int i;
[...]

> +    /* 
> +     * The maintenance interrupt handler doesn't do anything anymore, but

"This is a dummy interrupt handler." (We don't care about "anymore" once
this is applied

> +     * receiving the interrupt is going to cause gic_inject to be called on

          Receiving... (because of the full stop I added above)

> @@ -1642,6 +1647,11 @@ asmlinkage void leave_hypervisor_tail(void)
>      }
>  }
>  
> +static void enter_hypervisor_head(void)
> +{
> +    gic_clear_lrs(current);
> +}

Just put it before the first use and avoid the need for the forward
declaration.

Ian.

Re: [PATCH v5 05/10] xen/arm: nr_lrs should be uint8_t

[Ian Campbell]

On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Does this change anything in practice?

Hopefully the compiler doesn't start doing stupid word access + mask on
every access...

Might there be more LRs with e.g. gic v3?

> ---
>  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 3262db3..5cdcd15 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

Re: [PATCH v5 06/10] xen/arm: keep track of the GICH_LR used for the irq in struct pending_irq

[Ian Campbell]

On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

Re: [PATCH v5 07/10] xen/arm: s/gic_set_guest_irq/gic_raise_guest_irq

[Ian Campbell]

On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> 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>

Re: [PATCH v5 05/10] xen/arm: nr_lrs should be uint8_t

[Julien Grall]

On 04/01/2014 12:59 PM, Ian Campbell wrote:
> On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Does this change anything in practice?

Patch #6 of Stefano's series keep track of LRs with an uint8 bits. I
asked for this patch to stupid issue later because the tracking use an
smaller number of bits.

> Hopefully the compiler doesn't start doing stupid word access + mask on
> every access...
> 
> Might there be more LRs with e.g. gic v3?

The number of LRs is encoded on 5 bits for the GICv3.

-- 
Julien Grall

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

[Ian Campbell]

Julien just told me there was a v6 of this series but I can't find it
anywhere. I swear I'm either not receiving some mail at the moment or my
MUA is nuking it somehow.

So, sorry, but could you resend?

Ian.

Re: [PATCH v5 08/10] xen/arm: second irq injection while the first irq is still inflight

[Ian Campbell]

On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> 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_PENDING because the irq has already been injected and is
> already visible by the guest.

I'm confused by this.

If the interrupt is pending but not active then the guest has yet to
read GICC_IAR, so while it might be "visible" to the guest it has not
been observed by the guest.

The comment says:

     * GIC_IRQ_GUEST_PENDING: the irq is asserted

I'm not sure how a second irq arriving would correspond to deasserting
the IRQ.

Also if you are clearing GIC_IRQ_GUEST_PENDING without clearing the
pending bit in the LR that's rather confusing -- I thought the state was
supposed to correspond to (the most recently observed) LR state.

(having been told that there should be a v6 I'm going to stop here while
I figure out where it went...)

Ian.

Re: [PATCH v5 05/10] xen/arm: nr_lrs should be uint8_t

[Ian Campbell]

On Tue, 2014-04-01 at 13:12 +0100, Julien Grall wrote:
> On 04/01/2014 12:59 PM, Ian Campbell wrote:
> > On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > Does this change anything in practice?
> 
> Patch #6 of Stefano's series keep track of LRs with an uint8 bits. I
> asked for this patch to stupid issue later because the tracking use an
> smaller number of bits.

I wish that sort of thing would be mentioned in commit messages.

> 
> > Hopefully the compiler doesn't start doing stupid word access + mask on
> > every access...
> > 
> > Might there be more LRs with e.g. gic v3?
> 
> The number of LRs is encoded on 5 bits for the GICv3.
> 

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

[Stefano Stabellini]

On Tue, 1 Apr 2014, Ian Campbell wrote:
> 
> Julien just told me there was a v6 of this series but I can't find it
> anywhere. I swear I'm either not receiving some mail at the moment or my
> MUA is nuking it somehow.
> 
> So, sorry, but could you resend?

OK. I'll resend it as is.

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

[Stefano Stabellini]

On Tue, 1 Apr 2014, Ian Campbell wrote:
> On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> >  
> > +static void gic_clear_one_lr(struct vcpu *v, int i)
> > +{
> > +    struct pending_irq *p;
> > +    uint32_t lr;
> > +    int irq;
> > +    bool_t inflight;
> > +
> > +    ASSERT(!local_irq_is_enabled());
> > +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> [...]
> > +        if ( !inflight )
> > +        {
> > +            spin_lock(&v->arch.vgic.lock);
> 
> Aren't you recursively taking this lock here?
> 
> In principal could you remove the requirement for the caller to hold
> this lock as long as you took it just before p = irq_to_pending(v, irq);
> and released it on each iteration? (I'm not at all sure of that, just
> wondering)

Yes, you are right. I had already fixed this problem in v6 (already sent
to the list). I didn't realize this problem because the spin_lock goes
away with a later patch.


> > +            list_del_init(&p->inflight);
> > +            spin_unlock(&v->arch.vgic.lock);
> > +        }
> > +    }
> > +}
> > +
> > +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_clear_one_lr(v, i);
> > +        i++;
> > +    }
> > +
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +}
> > +
> >  static void gic_restore_pending_irqs(struct vcpu *v)
> >  {
> >      int i;
> [...]
> 
> > +    /* 
> > +     * The maintenance interrupt handler doesn't do anything anymore, but
> 
> "This is a dummy interrupt handler." (We don't care about "anymore" once
> this is applied

OK

> > +     * receiving the interrupt is going to cause gic_inject to be called on
> 
>           Receiving... (because of the full stop I added above)
> 
> > @@ -1642,6 +1647,11 @@ asmlinkage void leave_hypervisor_tail(void)
> >      }
> >  }
> >  
> > +static void enter_hypervisor_head(void)
> > +{
> > +    gic_clear_lrs(current);
> > +}
> 
> Just put it before the first use and avoid the need for the forward
> declaration.
 
OK

Re: [PATCH v5 08/10] xen/arm: second irq injection while the first irq is still inflight

[Stefano Stabellini]

On Tue, 1 Apr 2014, Ian Campbell wrote:
> On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> > 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_PENDING because the irq has already been injected and is
> > already visible by the guest.
> 
> I'm confused by this.
> 
> If the interrupt is pending but not active then the guest has yet to
> read GICC_IAR, so while it might be "visible" to the guest it has not
> been observed by the guest.
 
Yes, it is visible to the guest VM but not yet been observed by the
guest operating system.


> The comment says:
> 
>      * GIC_IRQ_GUEST_PENDING: the irq is asserted
> 
> I'm not sure how a second irq arriving would correspond to deasserting
> the IRQ.

I see where the confusion is coming from.
This comment is not quite correct unfortunately.

GIC_IRQ_GUEST_PENDING is set when the irq needs to be asserted (by
adding it to the LRs). Once the irq has become guest visible,
GIC_IRQ_GUEST_PENDING is cleared.

Going back to the top of the commit message:

"If the first irq is already pending (not active), just clear
GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
already visible by the guest"

means that if the first irq has already been added to the LRs but it is
still in pending state, we cannot add it a second time, so just go ahead
and clear GIC_IRQ_GUEST_PENDING as if we did add it to the LRs, because
the guest is still going to receive a notification.


> Also if you are clearing GIC_IRQ_GUEST_PENDING without clearing the
> pending bit in the LR that's rather confusing -- I thought the state was
> supposed to correspond to (the most recently observed) LR state.

Unfortunately not quite: GIC_IRQ_GUEST_PENDING is set by
vgic_vcpu_inject_irq and cleared by gic_set_lr and gic_clear_one_lr.
Later in this series it can also be set by gic_restore_pending_irqs in
case we want to evict an irq from an LR to leave room for an higher
priority irq.


> (having been told that there should be a v6 I'm going to stop here while
> I figure out where it went...)

This patch is unchanged in v6.

Re: [PATCH v5 08/10] xen/arm: second irq injection while the first irq is still inflight

[Ian Campbell]

On Wed, 2014-04-02 at 16:31 +0100, Stefano Stabellini wrote:
> On Tue, 1 Apr 2014, Ian Campbell wrote:
> > On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> > > 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_PENDING because the irq has already been injected and is
> > > already visible by the guest.
> > 
> > I'm confused by this.
> > 
> > If the interrupt is pending but not active then the guest has yet to
> > read GICC_IAR, so while it might be "visible" to the guest it has not
> > been observed by the guest.
>  
> Yes, it is visible to the guest VM but not yet been observed by the
> guest operating system.
> 
> 
> > The comment says:
> > 
> >      * GIC_IRQ_GUEST_PENDING: the irq is asserted
> > 
> > I'm not sure how a second irq arriving would correspond to deasserting
> > the IRQ.
> 
> I see where the confusion is coming from.
> This comment is not quite correct unfortunately.
> 
> GIC_IRQ_GUEST_PENDING is set when the irq needs to be asserted (by
> adding it to the LRs). Once the irq has become guest visible,
> GIC_IRQ_GUEST_PENDING is cleared.
> 
> Going back to the top of the commit message:
> 
> "If the first irq is already pending (not active), just clear
> GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> already visible by the guest"
> 
> means that if the first irq has already been added to the LRs but it is
> still in pending state, we cannot add it a second time, so just go ahead
> and clear GIC_IRQ_GUEST_PENDING as if we did add it to the LRs, because
> the guest is still going to receive a notification.

OK, so when you use lower case pending/active you are talking about the
LR state. But when you use upper case GIC_IRQ_GUEST_PENDING this is a
completely separate state related to the queueing of interrupts within
Xen itself?

Can we s/GIC_IRQ_GUEST_PENDING/GIC_IRQ_GUEST_QUEUED/ perhaps? The
comment would be something like:
     * GIC_IRQ_GUEST_QUEUED: the irq is asserted and queued for
       injection into the guests LRs.

> 
> 
> > Also if you are clearing GIC_IRQ_GUEST_PENDING without clearing the
> > pending bit in the LR that's rather confusing -- I thought the state was
> > supposed to correspond to (the most recently observed) LR state.
> 
> Unfortunately not quite: GIC_IRQ_GUEST_PENDING is set by
> vgic_vcpu_inject_irq and cleared by gic_set_lr and gic_clear_one_lr.
> Later in this series it can also be set by gic_restore_pending_irqs in
> case we want to evict an irq from an LR to leave room for an higher
> priority irq.
> 
> 
> > (having been told that there should be a v6 I'm going to stop here while
> > I figure out where it went...)
> 
> This patch is unchanged in v6.

Thanks. I'll look into v6 ASAP, probably tomorrow AM.

Ian.

Re: [PATCH v5 08/10] xen/arm: second irq injection while the first irq is still inflight

[Stefano Stabellini]

On Wed, 2 Apr 2014, Ian Campbell wrote:
> On Wed, 2014-04-02 at 16:31 +0100, Stefano Stabellini wrote:
> > On Tue, 1 Apr 2014, Ian Campbell wrote:
> > > On Mon, 2014-03-24 at 18:49 +0000, Stefano Stabellini wrote:
> > > > 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_PENDING because the irq has already been injected and is
> > > > already visible by the guest.
> > > 
> > > I'm confused by this.
> > > 
> > > If the interrupt is pending but not active then the guest has yet to
> > > read GICC_IAR, so while it might be "visible" to the guest it has not
> > > been observed by the guest.
> >  
> > Yes, it is visible to the guest VM but not yet been observed by the
> > guest operating system.
> > 
> > 
> > > The comment says:
> > > 
> > >      * GIC_IRQ_GUEST_PENDING: the irq is asserted
> > > 
> > > I'm not sure how a second irq arriving would correspond to deasserting
> > > the IRQ.
> > 
> > I see where the confusion is coming from.
> > This comment is not quite correct unfortunately.
> > 
> > GIC_IRQ_GUEST_PENDING is set when the irq needs to be asserted (by
> > adding it to the LRs). Once the irq has become guest visible,
> > GIC_IRQ_GUEST_PENDING is cleared.
> > 
> > Going back to the top of the commit message:
> > 
> > "If the first irq is already pending (not active), just clear
> > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> > already visible by the guest"
> > 
> > means that if the first irq has already been added to the LRs but it is
> > still in pending state, we cannot add it a second time, so just go ahead
> > and clear GIC_IRQ_GUEST_PENDING as if we did add it to the LRs, because
> > the guest is still going to receive a notification.
> 
> OK, so when you use lower case pending/active you are talking about the
> LR state. But when you use upper case GIC_IRQ_GUEST_PENDING this is a
> completely separate state related to the queueing of interrupts within
> Xen itself?
> 
> Can we s/GIC_IRQ_GUEST_PENDING/GIC_IRQ_GUEST_QUEUED/ perhaps? The
> comment would be something like:
>      * GIC_IRQ_GUEST_QUEUED: the irq is asserted and queued for
>        injection into the guests LRs.

I realize now that the current naming scheme could be confusing.  When I
talk about pending, active and GICH_LR_PENDING in this commit message, I
am talking about the irq status in the LR register. When I talk about
GIC_IRQ_GUEST_PENDING, I am referring to the GIC internal state tracker.

Because we don't actually know when the guest irq goes from pending to
active, we are clearing GIC_IRQ_GUEST_PENDING as soon as we add an irq
to an LR, as stated in the comment in xen/include/asm-arm/domain.h.
However as a result GIC_IRQ_GUEST_PENDING is more like
"GIC_IRQ_GUEST_QUEUED" rather than "GIC_IRQ_GUEST_PENDING", like you
wrote.

I'll add a patch to do the renaming as suggested.

Re: [PATCH v5 08/10] xen/arm: second irq injection while the first irq is still inflight

[Ian Campbell]

On Sun, 2014-04-06 at 17:58 +0100, Stefano Stabellini wrote:

> I realize now that the current naming scheme could be confusing.  When I
> talk about pending, active and GICH_LR_PENDING in this commit message, I
> am talking about the irq status in the LR register. When I talk about
> GIC_IRQ_GUEST_PENDING, I am referring to the GIC internal state tracker.
> 
> Because we don't actually know when the guest irq goes from pending to
> active, we are clearing GIC_IRQ_GUEST_PENDING as soon as we add an irq
> to an LR, as stated in the comment in xen/include/asm-arm/domain.h.
> However as a result GIC_IRQ_GUEST_PENDING is more like
> "GIC_IRQ_GUEST_QUEUED" rather than "GIC_IRQ_GUEST_PENDING", like you
> wrote.
> 
> I'll add a patch to do the renaming as suggested.

Thanks, in that case I guess I won't need the cold towel on my forehead
when I think about this stuff ;-)

Ian.