[PATCH v8 0/13] remove maintenance interrupts

[PATCH v8 0/13] 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.

This version contains a few critical bug fixes. I have tested the series
for several hours, running several workloads, without interruptions.


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

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

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

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

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

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

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


Stefano Stabellini (13):
      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
      gic_remove_from_queues: take a lock on the right vcpu

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

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

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

[Stefano Stabellini]

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

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

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

---

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

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

[PATCH v8 02/13] xen/arm: remove unused virtual parameter from vgic_vcpu_inject_irq

[Stefano Stabellini]

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

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

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

[Stefano Stabellini]

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

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

---

Changes in v5:
- introduce lr_all_full() helper.

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

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

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

[Stefano Stabellini]

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

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

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

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

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

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

---

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

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

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

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

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

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

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

[PATCH v8 05/13] xen/arm: nr_lrs should be uint8_t

[Stefano Stabellini]

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

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

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

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

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

[PATCH v8 08/13] 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>

---

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

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

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

[Stefano Stabellini]

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

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

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

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

---

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

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

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

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

[PATCH v8 10/13] 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           |   32 +++++++++++++++++---------------
 xen/arch/arm/vgic.c          |    9 +++++++--
 xen/include/asm-arm/domain.h |    5 ++++-
 3 files changed, 28 insertions(+), 18 deletions(-)

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

[PATCH v8 11/13] 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 ones are currently
active in the guest.
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 v8:
- in gic_restore_pending_irqs rename i to lr;
- add in code comments in gic_restore_pending_irqs and
gic_events_need_delivery;
- add << 3 to mask_priority.

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

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

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index eb59f5a..de8dd1c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -650,6 +650,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
     p = irq_to_pending(v, irq);
     if ( lr & GICH_LR_ACTIVE )
     {
+        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
              test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
         {
@@ -673,6 +674,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
         if ( p->desc != NULL )
             p->desc->status &= ~IRQ_INPROGRESS;
         clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+        clear_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         p->lr = GIC_INVALID_LR;
         if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
              test_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
@@ -703,20 +705,55 @@ void gic_clear_lrs(struct vcpu *v)
 
 static void gic_restore_pending_irqs(struct vcpu *v)
 {
-    int i;
-    struct pending_irq *p, *t;
+    int lr = 0, lrs = nr_lrs;
+    struct pending_irq *p, *t, *p_r;
+    struct list_head *inflight_r;
     unsigned long flags;
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    if ( list_empty(&v->arch.vgic.lr_pending) )
+        goto out;
+
+    inflight_r = &v->arch.vgic.inflight_irqs;
     list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
     {
-        i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
-        if ( i >= nr_lrs ) return;
+        lr = find_next_zero_bit(&this_cpu(lr_mask), nr_lrs, lr);
+        if ( lr >= nr_lrs )
+        {
+            /* No more free LRs: find a lower priority irq to evict */
+            list_for_each_entry_reverse( p_r, inflight_r, inflight )
+            {
+                if ( p_r->priority == p->priority )
+                    goto out;
+                if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) &&
+                     !test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
+                    goto found;
+            }
+            /* We didn't find a victim this time, and we won't next
+             * time, so quit */
+            goto out;
+
+found:
+            lr = p_r->lr;
+            p_r->lr = GIC_INVALID_LR;
+            set_bit(GIC_IRQ_GUEST_QUEUED, &p_r->status);
+            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
+            gic_add_to_lr_pending(v, p_r);
+            inflight_r = &p_r->inflight; 
+        }
 
-        gic_set_lr(i, p, GICH_LR_PENDING);
+        gic_set_lr(lr, p, GICH_LR_PENDING);
         list_del_init(&p->lr_queue);
-        set_bit(i, &this_cpu(lr_mask));
+        set_bit(lr, &this_cpu(lr_mask));
+
+        /* We can only evict nr_lrs entries */
+        lrs--;
+        if ( lrs == 0 )
+            break;
     }
+
+out:
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
@@ -733,8 +770,44 @@ 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 = mask_priority << 3;
+
+    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 >= mask_priority )
+            break;
+        lrs--;
+        if ( lrs == 0 )
+            break;
+    }
+
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+
+    if ( max_priority < active_priority &&
+         max_priority < 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 59ce196..d689675 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -56,8 +56,9 @@ struct pending_irq
      *
      */
 #define GIC_IRQ_GUEST_QUEUED   0
-#define GIC_IRQ_GUEST_VISIBLE  1
-#define GIC_IRQ_GUEST_ENABLED  2
+#define GIC_IRQ_GUEST_ACTIVE   1
+#define GIC_IRQ_GUEST_VISIBLE  2
+#define GIC_IRQ_GUEST_ENABLED  3
     unsigned long status;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
     int irq;
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 0c4c583..30e92cf 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 v8 12/13] xen/arm: introduce GIC_PRI_TO_GUEST macro

[Stefano Stabellini]

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

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

---

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index de8dd1c..2bfaba9 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -555,7 +555,7 @@ static inline void gic_set_lr(int lr, struct pending_irq *p,
     BUG_ON(lr < 0);
     BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
 
-    lr_val = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+    lr_val = state | (GIC_PRI_TO_GUEST(p->priority) << GICH_LR_PRIORITY_SHIFT) |
         ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
     if ( p->desc != NULL )
         lr_val |= GICH_LR_HW | (p->desc->irq << GICH_LR_PHYSICAL_SHIFT);
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 30e92cf..bf6fb1e 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 priority */
 
 
 #ifndef __ASSEMBLY__
-- 
1.7.10.4

[PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Stefano Stabellini]

At the moment gic_remove_from_queues doesn't handle the case where the
guest kernel disables an irq on a different vcpu compared to the one
currently receiving the interrupt.
Make sure to take the right vcpu lock before removing the irq from
lr_queue.

Document that the function should remove irqs from LR registers too.

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 2bfaba9..bb598eb 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -589,9 +589,16 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
 
 void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
 {
-    struct pending_irq *p = irq_to_pending(v, virtual_irq);
+    struct pending_irq *p;
     unsigned long flags;
 
+    /* TODO: do not assume SPI delivery on vcpu0 */
+    if ( virtual_irq >= 32 && v->vcpu_id != 0 )
+        v = v->domain->vcpu[0];
+
+    p = irq_to_pending(v, virtual_irq);
+
+    /* TODO: evict the irq from LRs */
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
     if ( !list_empty(&p->lr_queue) )
         list_del_init(&p->lr_queue);
-- 
1.7.10.4

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

[Julien Grall]

Hi Stefano,

On 22/05/14 13:32, Stefano Stabellini wrote:
> +void gic_clear_lrs(struct vcpu *v)
> +{
> +    int i = 0;
> +    unsigned long flags;
> +
> +    if ( is_idle_vcpu(v) )
> +        return;

It think it would be nice to explain why it's necessary to add these 2 
new line (i.e the LRs are not restored for idle VCPU...).

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

Small coding style error, the { should be on a newline.

With this 2 changes:

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

Regards,

-- 
Julien Grall

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

[Julien Grall]

Hi Stefano,

Sorry, I'm jumping into the discussion a bit late.

On 22/05/14 13:32, Stefano Stabellini wrote:
> 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;

As the above line will be dropped when this patch will be committed 
(because it's after the ---), it won't be clear why you move the irq field.

Could you add a line in the commit message?

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

Regards,

-- 
Julien Grall

Re: [PATCH v8 08/13] xen/arm: rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED

[Julien Grall]

Hi Stefano,

On 22/05/14 13:32, Stefano Stabellini wrote:
> Rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED and clarify its
> meaning in xen/include/asm-arm/domain.h.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

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

[Julien Grall]

On 22/05/14 13:32, 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), clear
> GIC_IRQ_GUEST_QUEUED because the guest doesn't need a second
> notification.If the irq has already been EOI'ed then just clear the
> GICH_LR right away and move the interrupt to lr_pending so that it is
> going to be reinjected by gic_restore_pending_irqs on return to guest.
>
> If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_QUEUED
> and send an SGI. The target cpu is going to be interrupted and call
> gic_clear_lrs, that is going to take the same actions.
>
> Do not call vgic_vcpu_inject_irq from gic_inject if
> evtchn_upcall_pending is set. If we remove that call, we don't need to
> special case evtchn_irq in vgic_vcpu_inject_irq anymore.
> We need to force the first injection of evtchn_irq (call
> gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> is already set by common code on vcpu creation.

If you only need it for the first time. Why can't you call 
vgic_inject_irq with the IRQ evtchn when the VCPU is turn on?

This would remove every hack with this IRQ in the GIC code.

> ---
>   xen/arch/arm/gic.c        |   48 +++++++++++++++++++++++++++++++++++++--------
>   xen/arch/arm/vgic.c       |   11 +++++++----
>   xen/include/asm-arm/gic.h |    1 +
>   3 files changed, 48 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 89d7025..a6fe566 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -66,6 +66,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>   /* Maximum cpu interface per GIC */
>   #define NR_GIC_CPU_IF 8
>
> +#undef GIC_DEBUG
>+

Did you intend to keep the debug in the final patch?

Regards,

-- 
Julien Grall

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

[Julien Grall]

Hi Stefano,

On 22/05/14 13:32, Stefano Stabellini wrote:
> GICH is banked, protect accesses by disabling interrupts.
> Protect lr_queue accesses with the vgic.lock only.
> gic.lock only protects accesses to GICD now.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Julien Grall]

Hi Stefano,

On 22/05/14 13:32, Stefano Stabellini wrote:
> At the moment gic_remove_from_queues doesn't handle the case where the
> guest kernel disables an irq on a different vcpu compared to the one
> currently receiving the interrupt.
> Make sure to take the right vcpu lock before removing the irq from
> lr_queue.

I see the same issue with vgic_enable_irqs. We may inject to the wrong 
VCPU (i.e other than 0).

I think we should have the same case in vgic_enable_irqs.

Cheers,

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 22 May 2014, Julien Grall wrote:
> > while the first one is still active.
> > If the first irq is already pending (not active), clear
> > GIC_IRQ_GUEST_QUEUED because the guest doesn't need a second
> > notification.If the irq has already been EOI'ed then just clear the
> > GICH_LR right away and move the interrupt to lr_pending so that it is
> > going to be reinjected by gic_restore_pending_irqs on return to guest.
> > 
> > If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_QUEUED
> > and send an SGI. The target cpu is going to be interrupted and call
> > gic_clear_lrs, that is going to take the same actions.
> > 
> > Do not call vgic_vcpu_inject_irq from gic_inject if
> > evtchn_upcall_pending is set. If we remove that call, we don't need to
> > special case evtchn_irq in vgic_vcpu_inject_irq anymore.
> > We need to force the first injection of evtchn_irq (call
> > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > is already set by common code on vcpu creation.
> 
> If you only need it for the first time. Why can't you call vgic_inject_irq
> with the IRQ evtchn when the VCPU is turn on?
> 
> This would remove every hack with this IRQ in the GIC code.

In principle sounds nice, but in practice it is difficult and risks
being racy. In vgic_vcpu_inject_irq we have:

    /* vcpu offline */
    if ( test_bit(_VPF_down, &v->pause_flags) )
    {
        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
        return;
    }

So we can only inject the irq once the vcpu is properly up, that is
certainly later than vcpu_initialise.


> > ---
> >   xen/arch/arm/gic.c        |   48
> > +++++++++++++++++++++++++++++++++++++--------
> >   xen/arch/arm/vgic.c       |   11 +++++++----
> >   xen/include/asm-arm/gic.h |    1 +
> >   3 files changed, 48 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 89d7025..a6fe566 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -66,6 +66,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
> >   /* Maximum cpu interface per GIC */
> >   #define NR_GIC_CPU_IF 8
> > 
> > +#undef GIC_DEBUG
> > +
> 
> Did you intend to keep the debug in the final patch?

Yes, I think it is useful.

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Stefano Stabellini]

On Thu, 22 May 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 22/05/14 13:32, Stefano Stabellini wrote:
> > At the moment gic_remove_from_queues doesn't handle the case where the
> > guest kernel disables an irq on a different vcpu compared to the one
> > currently receiving the interrupt.
> > Make sure to take the right vcpu lock before removing the irq from
> > lr_queue.
> 
> I see the same issue with vgic_enable_irqs. We may inject to the wrong VCPU
> (i.e other than 0).
> 
> I think we should have the same case in vgic_enable_irqs.

I think it would make more sense to print a warning in
vgic_distr_mmio_write GICD_ITARGETSR rather than vgic_enable_irqs.

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

[Julien Grall]

On 22/05/14 18:39, Stefano Stabellini wrote:
> On Thu, 22 May 2014, Julien Grall wrote:
>>> while the first one is still active.
>>> If the first irq is already pending (not active), clear
>>> GIC_IRQ_GUEST_QUEUED because the guest doesn't need a second
>>> notification.If the irq has already been EOI'ed then just clear the
>>> GICH_LR right away and move the interrupt to lr_pending so that it is
>>> going to be reinjected by gic_restore_pending_irqs on return to guest.
>>>
>>> If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_QUEUED
>>> and send an SGI. The target cpu is going to be interrupted and call
>>> gic_clear_lrs, that is going to take the same actions.
>>>
>>> Do not call vgic_vcpu_inject_irq from gic_inject if
>>> evtchn_upcall_pending is set. If we remove that call, we don't need to
>>> special case evtchn_irq in vgic_vcpu_inject_irq anymore.
>>> We need to force the first injection of evtchn_irq (call
>>> gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
>>> is already set by common code on vcpu creation.
>>
>> If you only need it for the first time. Why can't you call vgic_inject_irq
>> with the IRQ evtchn when the VCPU is turn on?
>>
>> This would remove every hack with this IRQ in the GIC code.
>
> In principle sounds nice, but in practice it is difficult and risks
> being racy. In vgic_vcpu_inject_irq we have:
>
>      /* vcpu offline */
>      if ( test_bit(_VPF_down, &v->pause_flags) )
>      {
>          spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>          return;
>      }
>
> So we can only inject the irq once the vcpu is properly up, that is
> certainly later than vcpu_initialise.

If we call vcpu_vgic_inject right before vcpu_wake (the _VPF_down flags 
has been cleared) we won't have any race condition.

This can be done in both arch/arm/vpsci.c and common/domain.c (VCPUOP_up).

It may require an arch specific function. Smth like arch_vcpu_prepare_up.

Regards,

-- 
Julien Grall

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Julien Grall]

On 22/05/14 18:45, Stefano Stabellini wrote:
> On Thu, 22 May 2014, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 22/05/14 13:32, Stefano Stabellini wrote:
>>> At the moment gic_remove_from_queues doesn't handle the case where the
>>> guest kernel disables an irq on a different vcpu compared to the one
>>> currently receiving the interrupt.
>>> Make sure to take the right vcpu lock before removing the irq from
>>> lr_queue.
>>
>> I see the same issue with vgic_enable_irqs. We may inject to the wrong VCPU
>> (i.e other than 0).
>>
>> I think we should have the same case in vgic_enable_irqs.
>
> I think it would make more sense to print a warning in
> vgic_distr_mmio_write GICD_ITARGETSR rather than vgic_enable_irqs.

IHMO the warning is not enougth. We may screw your state machine.


BTW, for your todo:

 > +    /* TODO: evict the irq from LRs */

We should not evict the IRQ from LRs. The guest may disable the IRQ 
while he is in the IRQ context (and before the IRQ has been EOI). If you 
drop the IRQs from the LRs, this can result to a maintenance interrupt:

"If the specified Interrupt does not exist in the
List registers, the GICH_HCR.EOIcount field is incremented, potentially 
generating a maintenance interrupt."

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Thu, 22 May 2014, Julien Grall wrote:
> On 22/05/14 18:39, Stefano Stabellini wrote:
> > On Thu, 22 May 2014, Julien Grall wrote:
> > > > while the first one is still active.
> > > > If the first irq is already pending (not active), clear
> > > > GIC_IRQ_GUEST_QUEUED because the guest doesn't need a second
> > > > notification.If the irq has already been EOI'ed then just clear the
> > > > GICH_LR right away and move the interrupt to lr_pending so that it is
> > > > going to be reinjected by gic_restore_pending_irqs on return to guest.
> > > > 
> > > > If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_QUEUED
> > > > and send an SGI. The target cpu is going to be interrupted and call
> > > > gic_clear_lrs, that is going to take the same actions.
> > > > 
> > > > Do not call vgic_vcpu_inject_irq from gic_inject if
> > > > evtchn_upcall_pending is set. If we remove that call, we don't need to
> > > > special case evtchn_irq in vgic_vcpu_inject_irq anymore.
> > > > We need to force the first injection of evtchn_irq (call
> > > > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > > > is already set by common code on vcpu creation.
> > > 
> > > If you only need it for the first time. Why can't you call vgic_inject_irq
> > > with the IRQ evtchn when the VCPU is turn on?
> > > 
> > > This would remove every hack with this IRQ in the GIC code.
> > 
> > In principle sounds nice, but in practice it is difficult and risks
> > being racy. In vgic_vcpu_inject_irq we have:
> > 
> >      /* vcpu offline */
> >      if ( test_bit(_VPF_down, &v->pause_flags) )
> >      {
> >          spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >          return;
> >      }
> > 
> > So we can only inject the irq once the vcpu is properly up, that is
> > certainly later than vcpu_initialise.
> 
> If we call vcpu_vgic_inject right before vcpu_wake (the _VPF_down flags has
> been cleared) we won't have any race condition.
> 
> This can be done in both arch/arm/vpsci.c and common/domain.c (VCPUOP_up).
>
> It may require an arch specific function. Smth like arch_vcpu_prepare_up.

The following change works:

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 33141e3..2a8456f 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -644,6 +644,8 @@ int arch_set_info_guest(
     else
         set_bit(_VPF_down, &v->pause_flags);
 
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
+
     return 0;
 }
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index af5cd6c..d597f63 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1087,6 +1087,8 @@ int construct_dom0(struct domain *d)
     }
 #endif
 
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
+
     for ( i = 1, cpu = 0; i < d->max_vcpus; i++ )
     {
         cpu = cpumask_cycle(cpu, &cpu_online_map);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 4869b87..2f86de1 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -404,17 +404,10 @@ 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 ( irq == v->domain->arch.evtchn_irq &&
-             vcpu_info(current, evtchn_upcall_pending) &&
-             list_empty(&p->inflight) )
-            vgic_vcpu_inject_irq(v, irq);
-        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);
-        }
+        spin_lock_irqsave(&v->arch.vgic.lock, flags);
+        if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+            gic_raise_guest_irq(v, irq, p->priority);
+        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
         if ( p->desc != NULL )
         {
             spin_lock_irqsave(&p->desc->lock, flags);

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

[Julien Grall]

On 05/23/2014 03:50 PM, Stefano Stabellini wrote:
> The following change works:
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 33141e3..2a8456f 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -644,6 +644,8 @@ int arch_set_info_guest(
>      else
>          set_bit(_VPF_down, &v->pause_flags);
>  
> +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> +

This is racy, we may not clear the _VPF_down bit in this function
(depending if VGCF_online is set or not).

Hopefully for ARM, libxc is setting this flags by default but it's not
always true.

>      return 0;
>  }
>  
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index af5cd6c..d597f63 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -1087,6 +1087,8 @@ int construct_dom0(struct domain *d)
>      }
>  #endif
>  
> +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> +

I think it needs a comment in code.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Fri, 23 May 2014, Julien Grall wrote:
> On 05/23/2014 03:50 PM, Stefano Stabellini wrote:
> > The following change works:
> > 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index 33141e3..2a8456f 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -644,6 +644,8 @@ int arch_set_info_guest(
> >      else
> >          set_bit(_VPF_down, &v->pause_flags);
> >  
> > +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> > +
> 
> This is racy, we may not clear the _VPF_down bit in this function
> (depending if VGCF_online is set or not).
> 
> Hopefully for ARM, libxc is setting this flags by default but it's not
> always true.

I could change the code to call vgic_vcpu_inject_irq only if VGCF_online
is set, but on second thought, would the code actually be more readable?
Or less error prone?

I think that the original patch is better. At least the hack is present
in a single very obvious place (vgic_enable_irqs).


> >      return 0;
> >  }
> >  
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index af5cd6c..d597f63 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -1087,6 +1087,8 @@ int construct_dom0(struct domain *d)
> >      }
> >  #endif
> >  
> > +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> > +
> 
> I think it needs a comment in code.
> 
> Regards,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Stefano Stabellini]

On Thu, 22 May 2014, Julien Grall wrote:
> On 22/05/14 18:45, Stefano Stabellini wrote:
> > On Thu, 22 May 2014, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 22/05/14 13:32, Stefano Stabellini wrote:
> > > > At the moment gic_remove_from_queues doesn't handle the case where the
> > > > guest kernel disables an irq on a different vcpu compared to the one
> > > > currently receiving the interrupt.
> > > > Make sure to take the right vcpu lock before removing the irq from
> > > > lr_queue.
> > > 
> > > I see the same issue with vgic_enable_irqs. We may inject to the wrong
> > > VCPU
> > > (i.e other than 0).
> > > 
> > > I think we should have the same case in vgic_enable_irqs.
> > 
> > I think it would make more sense to print a warning in
> > vgic_distr_mmio_write GICD_ITARGETSR rather than vgic_enable_irqs.
> 
> IHMO the warning is not enougth. We may screw your state machine.

That cannot happen: rank->itargets is actually unused at the moment.


> BTW, for your todo:
> 
> > +    /* TODO: evict the irq from LRs */
> 
> We should not evict the IRQ from LRs. The guest may disable the IRQ while he
> is in the IRQ context (and before the IRQ has been EOI). If you drop the IRQs
> from the LRs, this can result to a maintenance interrupt:
> 
> "If the specified Interrupt does not exist in the
> List registers, the GICH_HCR.EOIcount field is incremented, potentially
> generating a maintenance interrupt."

It is still better than the alternative: having an LR busy for no reason.
A maintenance interrupt would be harmless.

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Julien Grall]

On 05/23/2014 06:33 PM, Stefano Stabellini wrote:
> On Thu, 22 May 2014, Julien Grall wrote:
>> On 22/05/14 18:45, Stefano Stabellini wrote:
>>> On Thu, 22 May 2014, Julien Grall wrote:
>>>> Hi Stefano,
>>>>
>>>> On 22/05/14 13:32, Stefano Stabellini wrote:
>>>>> At the moment gic_remove_from_queues doesn't handle the case where the
>>>>> guest kernel disables an irq on a different vcpu compared to the one
>>>>> currently receiving the interrupt.
>>>>> Make sure to take the right vcpu lock before removing the irq from
>>>>> lr_queue.
>>>>
>>>> I see the same issue with vgic_enable_irqs. We may inject to the wrong
>>>> VCPU
>>>> (i.e other than 0).
>>>>
>>>> I think we should have the same case in vgic_enable_irqs.
>>>
>>> I think it would make more sense to print a warning in
>>> vgic_distr_mmio_write GICD_ITARGETSR rather than vgic_enable_irqs.
>>
>> IHMO the warning is not enougth. We may screw your state machine.
> 
> That cannot happen: rank->itargets is actually unused at the moment.

itargets is not used, but nothing prevent a guest to enabled an IRQ on
VCPU1. This can inject the IRQ in VCPU1 while it's already injected in
VCPU0 (assuming the IRQ what disable for a little time).

> 
>> BTW, for your todo:
>>
>>> +    /* TODO: evict the irq from LRs */
>>
>> We should not evict the IRQ from LRs. The guest may disable the IRQ while he
>> is in the IRQ context (and before the IRQ has been EOI). If you drop the IRQs
>> from the LRs, this can result to a maintenance interrupt:
>>
>> "If the specified Interrupt does not exist in the
>> List registers, the GICH_HCR.EOIcount field is incremented, potentially
>> generating a maintenance interrupt."
> 
> It is still better than the alternative: having an LR busy for no reason.
> A maintenance interrupt would be harmless.

Our internal representation (in the status field, still inflight) won't
be update-to-date for IRQ. We either inject a spurious IRQ (if it's a
virtual IRQ), other set active & pending is physical IRQ (which is
invalid from the GIC specification).

Regards,

-- 
Julien Grall

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Stefano Stabellini]

On Fri, 23 May 2014, Julien Grall wrote:
> On 05/23/2014 06:33 PM, Stefano Stabellini wrote:
> > On Thu, 22 May 2014, Julien Grall wrote:
> >> On 22/05/14 18:45, Stefano Stabellini wrote:
> >>> On Thu, 22 May 2014, Julien Grall wrote:
> >>>> Hi Stefano,
> >>>>
> >>>> On 22/05/14 13:32, Stefano Stabellini wrote:
> >>>>> At the moment gic_remove_from_queues doesn't handle the case where the
> >>>>> guest kernel disables an irq on a different vcpu compared to the one
> >>>>> currently receiving the interrupt.
> >>>>> Make sure to take the right vcpu lock before removing the irq from
> >>>>> lr_queue.
> >>>>
> >>>> I see the same issue with vgic_enable_irqs. We may inject to the wrong
> >>>> VCPU
> >>>> (i.e other than 0).
> >>>>
> >>>> I think we should have the same case in vgic_enable_irqs.
> >>>
> >>> I think it would make more sense to print a warning in
> >>> vgic_distr_mmio_write GICD_ITARGETSR rather than vgic_enable_irqs.
> >>
> >> IHMO the warning is not enougth. We may screw your state machine.
> > 
> > That cannot happen: rank->itargets is actually unused at the moment.
> 
> itargets is not used, but nothing prevent a guest to enabled an IRQ on
> VCPU1.

That is actually the problem: if vcpu1 is the one enabling an SPI, the
target vcpu should still be the one specified by itarget.


> This can inject the IRQ in VCPU1 while it's already injected in
> VCPU0 (assuming the IRQ what disable for a little time).
> 
> > 
> >> BTW, for your todo:
> >>
> >>> +    /* TODO: evict the irq from LRs */
> >>
> >> We should not evict the IRQ from LRs. The guest may disable the IRQ while he
> >> is in the IRQ context (and before the IRQ has been EOI). If you drop the IRQs
> >> from the LRs, this can result to a maintenance interrupt:
> >>
> >> "If the specified Interrupt does not exist in the
> >> List registers, the GICH_HCR.EOIcount field is incremented, potentially
> >> generating a maintenance interrupt."
> > 
> > It is still better than the alternative: having an LR busy for no reason.
> > A maintenance interrupt would be harmless.
> 
> Our internal representation (in the status field, still inflight) won't
> be update-to-date for IRQ. We either inject a spurious IRQ (if it's a
> virtual IRQ), other set active & pending is physical IRQ (which is
> invalid from the GIC specification).

I think that the best behaviour would be to evict the irq from LRs if
the irq is not active.

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Julien Grall]

On 25/05/14 16:39, Stefano Stabellini wrote:
  > That is actually the problem: if vcpu1 is the one enabling an SPI, the
> target vcpu should still be the one specified by itarget.

Yes, but by enabling I mean writing in ISENABLER* on VCPU1. In this 
case, we may inject this IRQ on this CPUs (see vgic_enable_irqs).

>
>> This can inject the IRQ in VCPU1 while it's already injected in
>> VCPU0 (assuming the IRQ what disable for a little time).
>>
>>>
>>>> BTW, for your todo:
>>>>
>>>>> +    /* TODO: evict the irq from LRs */
>>>>
>>>> We should not evict the IRQ from LRs. The guest may disable the IRQ while he
>>>> is in the IRQ context (and before the IRQ has been EOI). If you drop the IRQs
>>>> from the LRs, this can result to a maintenance interrupt:
>>>>
>>>> "If the specified Interrupt does not exist in the
>>>> List registers, the GICH_HCR.EOIcount field is incremented, potentially
>>>> generating a maintenance interrupt."
>>>
>>> It is still better than the alternative: having an LR busy for no reason.
>>> A maintenance interrupt would be harmless.
>>
>> Our internal representation (in the status field, still inflight) won't
>> be update-to-date for IRQ. We either inject a spurious IRQ (if it's a
>> virtual IRQ), other set active & pending is physical IRQ (which is
>> invalid from the GIC specification).
>
> I think that the best behaviour would be to evict the irq from LRs if
> the irq is not active.

Right.

Regards,


-- 
Julien Grall

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Stefano Stabellini]

On Sun, 25 May 2014, Julien Grall wrote:
> On 25/05/14 16:39, Stefano Stabellini wrote:
>  > That is actually the problem: if vcpu1 is the one enabling an SPI, the
> > target vcpu should still be the one specified by itarget.
> 
> Yes, but by enabling I mean writing in ISENABLER* on VCPU1. In this case, we
> may inject this IRQ on this CPUs (see vgic_enable_irqs).

Yes, that is completely wrong, I have a patch for that. I'll send it out
separately from this series.


> > > This can inject the IRQ in VCPU1 while it's already injected in
> > > VCPU0 (assuming the IRQ what disable for a little time).
> > > 
> > > > 
> > > > > BTW, for your todo:
> > > > > 
> > > > > > +    /* TODO: evict the irq from LRs */
> > > > > 
> > > > > We should not evict the IRQ from LRs. The guest may disable the IRQ
> > > > > while he
> > > > > is in the IRQ context (and before the IRQ has been EOI). If you drop
> > > > > the IRQs
> > > > > from the LRs, this can result to a maintenance interrupt:
> > > > > 
> > > > > "If the specified Interrupt does not exist in the
> > > > > List registers, the GICH_HCR.EOIcount field is incremented,
> > > > > potentially
> > > > > generating a maintenance interrupt."
> > > > 
> > > > It is still better than the alternative: having an LR busy for no
> > > > reason.
> > > > A maintenance interrupt would be harmless.
> > > 
> > > Our internal representation (in the status field, still inflight) won't
> > > be update-to-date for IRQ. We either inject a spurious IRQ (if it's a
> > > virtual IRQ), other set active & pending is physical IRQ (which is
> > > invalid from the GIC specification).
> > 
> > I think that the best behaviour would be to evict the irq from LRs if
> > the irq is not active.
> 
> Right.

Re: [PATCH v8 13/13] gic_remove_from_queues: take a lock on the right vcpu

[Stefano Stabellini]

On Sun, 25 May 2014, Stefano Stabellini wrote:
> On Sun, 25 May 2014, Julien Grall wrote:
> > On 25/05/14 16:39, Stefano Stabellini wrote:
> >  > That is actually the problem: if vcpu1 is the one enabling an SPI, the
> > > target vcpu should still be the one specified by itarget.
> > 
> > Yes, but by enabling I mean writing in ISENABLER* on VCPU1. In this case, we
> > may inject this IRQ on this CPUs (see vgic_enable_irqs).
> 
> Yes, that is completely wrong, I have a patch for that. I'll send it out
> separately from this series.

To be clear: I'll drop this patch from this series and send out two
separate patches.


> > > > This can inject the IRQ in VCPU1 while it's already injected in
> > > > VCPU0 (assuming the IRQ what disable for a little time).
> > > > 
> > > > > 
> > > > > > BTW, for your todo:
> > > > > > 
> > > > > > > +    /* TODO: evict the irq from LRs */
> > > > > > 
> > > > > > We should not evict the IRQ from LRs. The guest may disable the IRQ
> > > > > > while he
> > > > > > is in the IRQ context (and before the IRQ has been EOI). If you drop
> > > > > > the IRQs
> > > > > > from the LRs, this can result to a maintenance interrupt:
> > > > > > 
> > > > > > "If the specified Interrupt does not exist in the
> > > > > > List registers, the GICH_HCR.EOIcount field is incremented,
> > > > > > potentially
> > > > > > generating a maintenance interrupt."
> > > > > 
> > > > > It is still better than the alternative: having an LR busy for no
> > > > > reason.
> > > > > A maintenance interrupt would be harmless.
> > > > 
> > > > Our internal representation (in the status field, still inflight) won't
> > > > be update-to-date for IRQ. We either inject a spurious IRQ (if it's a
> > > > virtual IRQ), other set active & pending is physical IRQ (which is
> > > > invalid from the GIC specification).
> > > 
> > > I think that the best behaviour would be to evict the irq from LRs if
> > > the irq is not active.
> > 
> > Right.
> 

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

[Julien Grall]

On 23/05/14 18:24, Stefano Stabellini wrote:
> On Fri, 23 May 2014, Julien Grall wrote:
>> On 05/23/2014 03:50 PM, Stefano Stabellini wrote:
>>> The following change works:
>>>
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index 33141e3..2a8456f 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -644,6 +644,8 @@ int arch_set_info_guest(
>>>       else
>>>           set_bit(_VPF_down, &v->pause_flags);
>>>
>>> +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
>>> +
>>
>> This is racy, we may not clear the _VPF_down bit in this function
>> (depending if VGCF_online is set or not).
>>
>> Hopefully for ARM, libxc is setting this flags by default but it's not
>> always true.
>
> I could change the code to call vgic_vcpu_inject_irq only if VGCF_online
> is set, but on second thought, would the code actually be more readable?
> Or less error prone?
>
> I think that the original patch is better. At least the hack is present
> in a single very obvious place (vgic_enable_irqs).

Hmmm ... right. I know that this code will likely change (with GICv3 
support). Can you add a comment in the code explain this issue?

With this change:

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

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Sun, 25 May 2014, Julien Grall wrote:
> On 23/05/14 18:24, Stefano Stabellini wrote:
> > On Fri, 23 May 2014, Julien Grall wrote:
> > > On 05/23/2014 03:50 PM, Stefano Stabellini wrote:
> > > > The following change works:
> > > > 
> > > > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > > > index 33141e3..2a8456f 100644
> > > > --- a/xen/arch/arm/domain.c
> > > > +++ b/xen/arch/arm/domain.c
> > > > @@ -644,6 +644,8 @@ int arch_set_info_guest(
> > > >       else
> > > >           set_bit(_VPF_down, &v->pause_flags);
> > > > 
> > > > +    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
> > > > +
> > > 
> > > This is racy, we may not clear the _VPF_down bit in this function
> > > (depending if VGCF_online is set or not).
> > > 
> > > Hopefully for ARM, libxc is setting this flags by default but it's not
> > > always true.
> > 
> > I could change the code to call vgic_vcpu_inject_irq only if VGCF_online
> > is set, but on second thought, would the code actually be more readable?
> > Or less error prone?
> > 
> > I think that the original patch is better. At least the hack is present
> > in a single very obvious place (vgic_enable_irqs).
> 
> Hmmm ... right. I know that this code will likely change (with GICv3 support).
> Can you add a comment in the code explain this issue?

done


> With this change:
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>
> 
> Regards,
> 
> -- 
> Julien Grall
> 

Re: [PATCH v8 08/13] xen/arm: rename GIC_IRQ_GUEST_PENDING to GIC_IRQ_GUEST_QUEUED

[Ian Campbell]

On Thu, 2014-05-22 at 13:32 +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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

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

[Ian Campbell]

On Thu, 2014-05-22 at 13:32 +0100, Stefano Stabellini wrote:
> @@ -626,19 +644,36 @@ static void gic_update_one_lr(struct vcpu *v, int i)
>      ASSERT(spin_is_locked(&v->arch.vgic.lock));
>  
>      lr = GICH[GICH_LR + i];
> -    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> +    irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> +    p = irq_to_pending(v, irq);
> +    if ( lr & GICH_LR_ACTIVE )
>      {
> +        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> +             test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
> +        {
> +            if ( p->desc == NULL )
> +                GICH[GICH_LR + i] = lr | GICH_LR_PENDING;
> +            else
> +                gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
> +                         irq, v->domain->domain_id, v->vcpu_id, i);

How common is this? Or should it never actually happen in reality?

> +        }
> +    } else if ( lr & GICH_LR_PENDING ) {
> +        int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> +#ifdef GIC_DEBUG
> +        if ( q )

	   if ( test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) ) 
#ifdef GIC_DEBUG
> +            gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is already pending in LR%d\n",
> +                    irq, v->domain->domain_id, v->vcpu_id, i);
#else
               ; /* Nothing to do */
#endif

would be nicer than attribute unused IMHO.

As it's a XENLOG_DEBUG this wouldn't be all that bad by default BTW,
given that the already active case isn't even conditional.

Ian.

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

[Stefano Stabellini]

On Fri, 6 Jun 2014, Ian Campbell wrote:
> On Thu, 2014-05-22 at 13:32 +0100, Stefano Stabellini wrote:
> > @@ -626,19 +644,36 @@ static void gic_update_one_lr(struct vcpu *v, int i)
> >      ASSERT(spin_is_locked(&v->arch.vgic.lock));
> >  
> >      lr = GICH[GICH_LR + i];
> > -    if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> > +    irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > +    p = irq_to_pending(v, irq);
> > +    if ( lr & GICH_LR_ACTIVE )
> >      {
> > +        if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > +             test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) )
> > +        {
> > +            if ( p->desc == NULL )
> > +                GICH[GICH_LR + i] = lr | GICH_LR_PENDING;
> > +            else
> > +                gdprintk(XENLOG_WARNING, "unable to inject hw irq=%d into d%dv%d: already active in LR%d\n",
> > +                         irq, v->domain->domain_id, v->vcpu_id, i);
> 
> How common is this? Or should it never actually happen in reality?

It should never happen


> > +        }
> > +    } else if ( lr & GICH_LR_PENDING ) {
> > +        int q __attribute__ ((unused)) = test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> > +#ifdef GIC_DEBUG
> > +        if ( q )
> 
> 	   if ( test_and_clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status) ) 
> #ifdef GIC_DEBUG
> > +            gdprintk(XENLOG_DEBUG, "trying to inject irq=%d into d%dv%d, when it is already pending in LR%d\n",
> > +                    irq, v->domain->domain_id, v->vcpu_id, i);
> #else
>                ; /* Nothing to do */
> #endif
> 
> would be nicer than attribute unused IMHO.
> 
> As it's a XENLOG_DEBUG this wouldn't be all that bad by default BTW,
> given that the already active case isn't even conditional.

This case can actually happen though