[PATCH v4 0/10] remove maintenance interrupts

[PATCH v4 0/10] remove maintenance interrupts

[Stefano Stabellini]

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


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

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

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



Stefano Stabellini (10):
      xen/arm: no need to set HCR_VI when using the vgic to inject irqs
      xen/arm: remove unused virtual parameter from vgic_vcpu_inject_irq
      xen/arm: support HW interrupts, do not request maintenance_interrupts
      xen/arm: set GICH_HCR_UIE if all the LRs are in use
      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: call gic_clear_lrs on entry to the hypervisor
      xen/arm: second irq injection while the first irq is still inflight
      xen/arm: don't protect GICH and lr_queue accesses with gic.lock
      xen/arm: gic_events_need_delivery and irq priorities

 xen/arch/arm/domain.c        |    2 +-
 xen/arch/arm/gic.c           |  258 +++++++++++++++++++++++-------------------
 xen/arch/arm/irq.c           |    2 +-
 xen/arch/arm/time.c          |    2 +-
 xen/arch/arm/traps.c         |   10 ++
 xen/arch/arm/vgic.c          |   47 ++++----
 xen/arch/arm/vtimer.c        |    4 +-
 xen/include/asm-arm/domain.h |   11 +-
 xen/include/asm-arm/gic.h    |   10 +-
 9 files changed, 196 insertions(+), 150 deletions(-)

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

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

[Stefano Stabellini]

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

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

---

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 91a2982..b388ef3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -726,22 +726,6 @@ void gic_clear_pending_irqs(struct vcpu *v)
     spin_unlock_irqrestore(&gic.lock, flags);
 }
 
-static void gic_inject_irq_start(void)
-{
-    register_t hcr = READ_SYSREG(HCR_EL2);
-    WRITE_SYSREG(hcr | HCR_VI, HCR_EL2);
-    isb();
-}
-
-static void gic_inject_irq_stop(void)
-{
-    register_t hcr = READ_SYSREG(HCR_EL2);
-    if (hcr & HCR_VI) {
-        WRITE_SYSREG(hcr & ~HCR_VI, HCR_EL2);
-        isb();
-    }
-}
-
 int gic_events_need_delivery(void)
 {
     return (!list_empty(&current->arch.vgic.lr_pending) ||
@@ -754,10 +738,6 @@ void gic_inject(void)
         vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq, 1);
 
     gic_restore_pending_irqs(current);
-    if (!gic_events_need_delivery())
-        gic_inject_irq_stop();
-    else
-        gic_inject_irq_start();
 }
 
 int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
-- 
1.7.10.4

Re: [PATCH v4 01/10] xen/arm: no need to set HCR_VI when using the vgic to inject irqs

[Julien Grall]

On 03/19/2014 12:31 PM, Stefano Stabellini wrote:
> 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

Re: [PATCH v4 01/10] xen/arm: no need to set HCR_VI when using the vgic to inject irqs

[Ian Campbell]

On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

Do you happen to have a chapter number from the ARM ARM and/or the GIC
spec to confirm what you say here? If so please can you include them.

(did I fail to hit send, I guess so... )

[PATCH v4 02/10] xen/arm: remove unused virtual parameter from vgic_vcpu_inject_irq

[Stefano Stabellini]

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

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 82a1e79..e6ddedf 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -782,7 +782,7 @@ void vcpu_mark_events_pending(struct vcpu *v)
     if ( already_pending )
         return;
 
-    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq, 1);
+    vgic_vcpu_inject_irq(v, v->domain->arch.evtchn_irq);
 }
 
 /*
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index b388ef3..dbba5d3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -735,7 +735,7 @@ int gic_events_need_delivery(void)
 void gic_inject(void)
 {
     if ( vcpu_info(current, evtchn_upcall_pending) )
-        vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq, 1);
+        vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq);
 
     gic_restore_pending_irqs(current);
 }
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index 3e326b0..5daa269 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -159,7 +159,7 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq)
         desc->arch.eoi_cpu = smp_processor_id();
 
         /* XXX: inject irq into all guest vcpus */
-        vgic_vcpu_inject_irq(d->vcpu[0], irq, 0);
+        vgic_vcpu_inject_irq(d->vcpu[0], irq);
         goto out_no_end;
     }
 
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index ba281e9..ae09c6b 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -215,7 +215,7 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
     current->arch.virt_timer.ctl = READ_SYSREG32(CNTV_CTL_EL0);
     WRITE_SYSREG32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL_EL0);
-    vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq, 1);
+    vgic_vcpu_inject_irq(current, current->arch.virt_timer.irq);
 }
 
 /* Route timer's IRQ on this CPU */
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 553411d..aab490c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -457,7 +457,7 @@ static int vgic_to_sgi(struct vcpu *v, register_t sgir)
                      sgir, vcpu_mask);
             continue;
         }
-        vgic_vcpu_inject_irq(d->vcpu[vcpuid], virtual_irq, 1);
+        vgic_vcpu_inject_irq(d->vcpu[vcpuid], virtual_irq);
     }
     return 1;
 }
@@ -685,7 +685,7 @@ void vgic_clear_pending_irqs(struct vcpu *v)
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
-void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
+void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 {
     int idx = irq >> 2, byte = irq & 0x3;
     uint8_t priority;
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index e325f78..87be11e 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -34,14 +34,14 @@ static void phys_timer_expired(void *data)
     struct vtimer *t = data;
     t->ctl |= CNTx_CTL_PENDING;
     if ( !(t->ctl & CNTx_CTL_MASK) )
-        vgic_vcpu_inject_irq(t->v, t->irq, 1);
+        vgic_vcpu_inject_irq(t->v, t->irq);
 }
 
 static void virt_timer_expired(void *data)
 {
     struct vtimer *t = data;
     t->ctl |= CNTx_CTL_MASK;
-    vgic_vcpu_inject_irq(t->v, t->irq, 1);
+    vgic_vcpu_inject_irq(t->v, t->irq);
 }
 
 int vcpu_domain_init(struct domain *d)
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 071280b..6fce5c2 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -162,7 +162,7 @@ extern void domain_vgic_free(struct domain *d);
 
 extern int vcpu_vgic_init(struct vcpu *v);
 
-extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq,int virtual);
+extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 
-- 
1.7.10.4

[PATCH v4 03/10] xen/arm: support HW interrupts, do not request maintenance_interrupts

[Stefano Stabellini]

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

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

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

Call gic_clear_lrs from gic_restore_state and on return to guest
(gic_inject).

In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu,
send and SGI to it to interrupt it and force it to clear the old LRs.

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

---

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  |  135 ++++++++++++++++++++++-----------------------------
 xen/arch/arm/vgic.c |    3 +-
 2 files changed, 60 insertions(+), 78 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index dbba5d3..32d3bea 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 #define NR_GIC_CPU_IF 8
 
+static void gic_clear_lrs(struct vcpu *v);
+
 static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
 {
     unsigned int cpu;
@@ -128,6 +130,7 @@ void gic_restore_state(struct vcpu *v)
     GICH[GICH_HCR] = GICH_HCR_EN;
     isb();
 
+    gic_clear_lrs(v);
     gic_restore_pending_irqs(v);
 }
 
@@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
 static inline void gic_set_lr(int lr, struct pending_irq *p,
         unsigned int state)
 {
-    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
+    uint32_t lr_reg;
 
     BUG_ON(lr >= nr_lrs);
     BUG_ON(lr < 0);
     BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
 
-    GICH[GICH_LR + lr] = state |
-        maintenance_int |
-        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
         ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
+    if ( p->desc != NULL )
+        lr_reg |= GICH_LR_HW |
+            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT);
+
+    GICH[GICH_LR + lr] = lr_reg;
 
     set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
     clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
@@ -669,7 +675,7 @@ 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,
+void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
         unsigned int state, unsigned int priority)
 {
     int i;
@@ -682,18 +688,62 @@ 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, irq), state);
             goto out;
         }
     }
 
-    gic_add_to_lr_pending(v, irq_to_pending(v, virtual_irq));
+    gic_add_to_lr_pending(v, irq_to_pending(v, irq));
 
 out:
     spin_unlock_irqrestore(&gic.lock, flags);
     return;
 }
 
+static void gic_clear_lrs(struct vcpu *v)
+{
+    struct pending_irq *p;
+    int i = 0, irq;
+    uint32_t lr;
+    bool_t inflight;
+
+    ASSERT(!local_irq_is_enabled());
+
+    while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
+                              nr_lrs, i)) < nr_lrs) {
+        lr = GICH[GICH_LR + i];
+        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
+        {
+            inflight = 0;
+            GICH[GICH_LR + i] = 0;
+            clear_bit(i, &this_cpu(lr_mask));
+
+            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
+            spin_lock(&gic.lock);
+            p = irq_to_pending(v, irq);
+            if ( p->desc != NULL )
+                p->desc->status &= ~IRQ_INPROGRESS;
+            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+            if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
+                    test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
+            {
+                inflight = 1;
+                gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+            }
+            spin_unlock(&gic.lock);
+            if ( !inflight )
+            {
+                spin_lock(&v->arch.vgic.lock);
+                list_del_init(&p->inflight);
+                spin_unlock(&v->arch.vgic.lock);
+            }
+
+        }
+
+        i++;
+    }
+}
+
 static void gic_restore_pending_irqs(struct vcpu *v)
 {
     int i;
@@ -734,6 +784,8 @@ int gic_events_need_delivery(void)
 
 void gic_inject(void)
 {
+    gic_clear_lrs(current);
+
     if ( vcpu_info(current, evtchn_upcall_pending) )
         vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq);
 
@@ -887,77 +939,8 @@ 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++;
-    }
 }
 
 void gic_dump_info(struct vcpu *v)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index aab490c..566f0ff 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
         if ( (irq != current->domain->arch.evtchn_irq) ||
              (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
             set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
-        return;
+        goto out;
     }
 
     /* vcpu offline */
-- 
1.7.10.4

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

[Julien Grall]

Hi Stefano,

On 03/19/2014 12:31 PM, Stefano Stabellini wrote:

[..]

> @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>  static inline void gic_set_lr(int lr, struct pending_irq *p,
>          unsigned int state)
>  {
> -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> +    uint32_t lr_reg;
>  
>      BUG_ON(lr >= nr_lrs);
>      BUG_ON(lr < 0);
>      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
>  
> -    GICH[GICH_LR + lr] = state |
> -        maintenance_int |
> -        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
>          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    if ( p->desc != NULL )
> +        lr_reg |= GICH_LR_HW |
> +            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT);
> +
> +    GICH[GICH_LR + lr] = lr_reg;
>  
>      set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
>      clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
> @@ -669,7 +675,7 @@ 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,
> +void gic_set_guest_irq(struct vcpu *v, unsigned int irq,

Any reason to rename virtual_irq into irq?

[..]

> +static void gic_clear_lrs(struct vcpu *v)
> +{
> +    struct pending_irq *p;
> +    int i = 0, irq;
> +    uint32_t lr;
> +    bool_t inflight;
> +
> +    ASSERT(!local_irq_is_enabled());
> +
> +    while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
> +                              nr_lrs, i)) < nr_lrs) {
> +        lr = GICH[GICH_LR + i];
> +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> +        {
> +            inflight = 0;
> +            GICH[GICH_LR + i] = 0;
> +            clear_bit(i, &this_cpu(lr_mask));
> +
> +            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> +            spin_lock(&gic.lock);

Not completely related to this patch ... taking gic.lock seems a bit too
strong here. The critical section only update data for the current
domain. It seems a bit stupid to block the other interrupt to handle
their interrupts at the same time.

Maybe introducing a dist lock would be a better solution?

[..]

>  void gic_dump_info(struct vcpu *v)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index aab490c..566f0ff 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>          if ( (irq != current->domain->arch.evtchn_irq) ||
>               (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
>              set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
> -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> -        return;
> +        goto out;

We don't want to kick the other VCPU every time. I think it's enough
when the interrupt is updated.

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Wed, 19 Mar 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/19/2014 12:31 PM, Stefano Stabellini wrote:
> 
> [..]
> 
> > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
> >  static inline void gic_set_lr(int lr, struct pending_irq *p,
> >          unsigned int state)
> >  {
> > -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> > +    uint32_t lr_reg;
> >  
> >      BUG_ON(lr >= nr_lrs);
> >      BUG_ON(lr < 0);
> >      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
> >  
> > -    GICH[GICH_LR + lr] = state |
> > -        maintenance_int |
> > -        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > +    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> >          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> > +    if ( p->desc != NULL )
> > +        lr_reg |= GICH_LR_HW |
> > +            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT);
> > +
> > +    GICH[GICH_LR + lr] = lr_reg;
> >  
> >      set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> >      clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
> > @@ -669,7 +675,7 @@ 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,
> > +void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
> 
> Any reason to rename virtual_irq into irq?

Just that with this patch we also use this function to inject hw irqs.


> [..]
> 
> > +static void gic_clear_lrs(struct vcpu *v)
> > +{
> > +    struct pending_irq *p;
> > +    int i = 0, irq;
> > +    uint32_t lr;
> > +    bool_t inflight;
> > +
> > +    ASSERT(!local_irq_is_enabled());
> > +
> > +    while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
> > +                              nr_lrs, i)) < nr_lrs) {
> > +        lr = GICH[GICH_LR + i];
> > +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> > +        {
> > +            inflight = 0;
> > +            GICH[GICH_LR + i] = 0;
> > +            clear_bit(i, &this_cpu(lr_mask));
> > +
> > +            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > +            spin_lock(&gic.lock);
> 
> Not completely related to this patch ... taking gic.lock seems a bit too
> strong here. The critical section only update data for the current
> domain. It seems a bit stupid to block the other interrupt to handle
> their interrupts at the same time.
> 
> Maybe introducing a dist lock would be a better solution?

It gets removed by a later patch: "don't protect GICH and lr_queue
accesses with gic.lock".


> [..]
> 
> >  void gic_dump_info(struct vcpu *v)
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index aab490c..566f0ff 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
> >          if ( (irq != current->domain->arch.evtchn_irq) ||
> >               (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
> >              set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
> > -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > -        return;
> > +        goto out;
> 
> We don't want to kick the other VCPU every time. I think it's enough
> when the interrupt is updated.

Without maintenance interrupts, the other vcpu potentially might never
return. We need to send an SGI to it to make sure it gets interrupted.
Once it is interrupted, it is going to run gic_clear_lrs that clears the
old LR and inject the new interrupt.

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

[Julien Grall]

Hi Stefano,

On 03/19/2014 02:43 PM, Stefano Stabellini wrote:
> On Wed, 19 Mar 2014, Julien Grall wrote:
>> On 03/19/2014 12:31 PM, Stefano Stabellini wrote:
>>
>> [..]
>>
>>> @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>>>  static inline void gic_set_lr(int lr, struct pending_irq *p,
>>>          unsigned int state)
>>>  {
>>> -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
>>> +    uint32_t lr_reg;
>>>  
>>>      BUG_ON(lr >= nr_lrs);
>>>      BUG_ON(lr < 0);
>>>      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
>>>  
>>> -    GICH[GICH_LR + lr] = state |
>>> -        maintenance_int |
>>> -        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
>>> +    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
>>>          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
>>> +    if ( p->desc != NULL )
>>> +        lr_reg |= GICH_LR_HW |
>>> +            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT);
>>> +
>>> +    GICH[GICH_LR + lr] = lr_reg;
>>>  
>>>      set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
>>>      clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
>>> @@ -669,7 +675,7 @@ 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,
>>> +void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
>>
>> Any reason to rename virtual_irq into irq?
> 
> Just that with this patch we also use this function to inject hw irqs.

As I understand the code, this function still takes a VIRQ. If Xen
injects an hw irq, it will before translate it to a VIRQ.

By VIRQ I mean the IRQ number from guest POV.

>> [..]
>>
>>> +static void gic_clear_lrs(struct vcpu *v)
>>> +{
>>> +    struct pending_irq *p;
>>> +    int i = 0, irq;
>>> +    uint32_t lr;
>>> +    bool_t inflight;
>>> +
>>> +    ASSERT(!local_irq_is_enabled());
>>> +
>>> +    while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
>>> +                              nr_lrs, i)) < nr_lrs) {
>>> +        lr = GICH[GICH_LR + i];
>>> +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
>>> +        {
>>> +            inflight = 0;
>>> +            GICH[GICH_LR + i] = 0;
>>> +            clear_bit(i, &this_cpu(lr_mask));
>>> +
>>> +            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
>>> +            spin_lock(&gic.lock);
>>
>> Not completely related to this patch ... taking gic.lock seems a bit too
>> strong here. The critical section only update data for the current
>> domain. It seems a bit stupid to block the other interrupt to handle
>> their interrupts at the same time.
>>
>> Maybe introducing a dist lock would be a better solution?
> 
> It gets removed by a later patch: "don't protect GICH and lr_queue
> accesses with gic.lock".

Ok.

>> [..]
>>
>>>  void gic_dump_info(struct vcpu *v)
>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>> index aab490c..566f0ff 100644
>>> --- a/xen/arch/arm/vgic.c
>>> +++ b/xen/arch/arm/vgic.c
>>> @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>>>          if ( (irq != current->domain->arch.evtchn_irq) ||
>>>               (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
>>>              set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
>>> -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>>> -        return;
>>> +        goto out;
>>
>> We don't want to kick the other VCPU every time. I think it's enough
>> when the interrupt is updated.
> 
> Without maintenance interrupts, the other vcpu potentially might never
> return. We need to send an SGI to it to make sure it gets interrupted.
> Once it is interrupted, it is going to run gic_clear_lrs that clears the
> old LR and inject the new interrupt.

I'm not entirely convince, we only need to update when you set
GIC_IRQ_GUEST_PENDING in &n->status (e.g in the if sentence). If you
don't update the IRQ status, you don't need to kick the VCPUs because
either he is already unblock or it will be schedule soon.

Anyway this case only happen when we inject the evtchn IRQ. I think it
might need some comment as reading only the code is unclear what is done
here...

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Wed, 19 Mar 2014, Julien Grall wrote:
> >> [..]
> >>
> >>>  void gic_dump_info(struct vcpu *v)
> >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> >>> index aab490c..566f0ff 100644
> >>> --- a/xen/arch/arm/vgic.c
> >>> +++ b/xen/arch/arm/vgic.c
> >>> @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
> >>>          if ( (irq != current->domain->arch.evtchn_irq) ||
> >>>               (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
> >>>              set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
> >>> -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >>> -        return;
> >>> +        goto out;
> >>
> >> We don't want to kick the other VCPU every time. I think it's enough
> >> when the interrupt is updated.
> > 
> > Without maintenance interrupts, the other vcpu potentially might never
> > return. We need to send an SGI to it to make sure it gets interrupted.
> > Once it is interrupted, it is going to run gic_clear_lrs that clears the
> > old LR and inject the new interrupt.
> 
> I'm not entirely convince, we only need to update when you set
> GIC_IRQ_GUEST_PENDING in &n->status (e.g in the if sentence). If you
> don't update the IRQ status, you don't need to kick the VCPUs because
> either he is already unblock or it will be schedule soon.
> 
> Anyway this case only happen when we inject the evtchn IRQ. I think it
> might need some comment as reading only the code is unclear what is done
> here...

Reading the code again and your reply I think you might be right, it
introduces few unneeded interruptions to the other cpu.

However this is one of the bits of code that gets changed by another
patch, see "second irq injection while the first irq is still inflight".
That patch fixes this issue and more.

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

[Julien Grall]

On 03/19/2014 03:53 PM, Stefano Stabellini wrote:
> However this is one of the bits of code that gets changed by another
> patch, see "second irq injection while the first irq is still inflight".
> That patch fixes this issue and more.

Thanks, I didn't yet look at this patch.

Regards,

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> If the irq to be injected is an hardware irq (p->desc != NULL), set
> GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ.
> 
> Remove the code to EOI a physical interrupt on behalf of the guest
> because it has become unnecessary.

Stupid question: there is no possibility of a h/w interrupt which for
one reason or another cannot be injected using these GIC facilities?

> 
> 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 from gic_restore_state and on return to guest
> (gic_inject).
> 
> In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu,
> send and SGI to it to interrupt it and force it to clear the old LRs.

s/and/an/

Is the SGI just there to cause it to flush its LRs? Does it not also
serve the purpose of causing the pcpu to pick up the potential new
interrupt?

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> 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  |  135 ++++++++++++++++++++++-----------------------------
>  xen/arch/arm/vgic.c |    3 +-
>  2 files changed, 60 insertions(+), 78 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index dbba5d3..32d3bea 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -128,6 +130,7 @@ void gic_restore_state(struct vcpu *v)
>      GICH[GICH_HCR] = GICH_HCR_EN;
>      isb();
>  
> +    gic_clear_lrs(v);
>      gic_restore_pending_irqs(v);

Not related to this patch exactly, but is this function badly named? It
seems to not actually be restoring anything but is actually looking for
any newly pending irqs which it can now inject, is that right?

Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which
suggests that the clear should be done there (which also seems logical).

>  }
>  
> @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>  static inline void gic_set_lr(int lr, struct pending_irq *p,
>          unsigned int state)
>  {
> -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> +    uint32_t lr_reg;
>  
>      BUG_ON(lr >= nr_lrs);
>      BUG_ON(lr < 0);
>      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
>  
> -    GICH[GICH_LR + lr] = state |
> -        maintenance_int |
> -        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
>          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    if ( p->desc != NULL )
> +        lr_reg |= GICH_LR_HW |
> +            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT);

It seems like it would be a silicon design bug if this were ever true.
So BUG_ON(p->desc->irq & ~GICH_LR_PHYSICAL_MASK)?

I think this should be checked at the time the interrupt is routed
instead of here, which gets it out of this hotter path.

Another future cleanup: I think a lot of this register frobbing code
would be cleaner if GICH_LR_FOO_MASK was already shifted.

[...]
> +static void gic_clear_lrs(struct vcpu *v)
> +{
> +    struct pending_irq *p;
> +    int i = 0, irq;
> +    uint32_t lr;
> +    bool_t inflight;
> +
> +    ASSERT(!local_irq_is_enabled());
> +
> +    while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),

"long unsigned int" is an odd one isn't it?

It seems like all of the other places which do bitops on lr_mask manage
to avoid any case at all.

> +                              nr_lrs, i)) < nr_lrs) {
> +        lr = GICH[GICH_LR + i];
> +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )

This state means the lr is "completed" (or inactive, however you want to
think about it)? A little helper macro lr_completed(lr) would clarify
this without needing a comment.

Isn't that condition also true for an LR which was never injected? Won't
we then try and complete a non-existent interrupt? Ah, this is protected
by the lr_mask isn't it.

> +        {
> +            inflight = 0;
> +            GICH[GICH_LR + i] = 0;
> +            clear_bit(i, &this_cpu(lr_mask));
> +
> +            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> +            spin_lock(&gic.lock);
> +            p = irq_to_pending(v, irq);
> +            if ( p->desc != NULL )
> +                p->desc->status &= ~IRQ_INPROGRESS;
> +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> +            if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> +                    test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> +            {
> +                inflight = 1;
> +                gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> +            }
> +            spin_unlock(&gic.lock);

Can an interrupt arrive at this point and make this interrupt "inflight"
again, such that the following list remove is actually wrong?

> +            if ( !inflight )
> +            {
> +                spin_lock(&v->arch.vgic.lock);
> +                list_del_init(&p->inflight);
> +                spin_unlock(&v->arch.vgic.lock);
> +            }
> +
> +        }
> +
> +        i++;
> +    }
> +}
> +
>  static void gic_restore_pending_irqs(struct vcpu *v)
>  {
>      int i;
>  static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
>  {
[...]

Is now empty but not removed?

>  }
>  
>  void gic_dump_info(struct vcpu *v)
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index aab490c..566f0ff 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -701,8 +701,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
>          if ( (irq != current->domain->arch.evtchn_irq) ||
>               (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
>              set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
> -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> -        return;
> +        goto out;
>      }
>  
>      /* vcpu offline */

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> > If the irq to be injected is an hardware irq (p->desc != NULL), set
> > GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ.
> > 
> > Remove the code to EOI a physical interrupt on behalf of the guest
> > because it has become unnecessary.
> 
> Stupid question: there is no possibility of a h/w interrupt which for
> one reason or another cannot be injected using these GIC facilities?

I don't think so. Nothing is written in spec about such a case.


> > 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 from gic_restore_state and on return to guest
> > (gic_inject).
> > 
> > In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu,
> > send and SGI to it to interrupt it and force it to clear the old LRs.
> 
> s/and/an/
> 
> Is the SGI just there to cause it to flush its LRs? Does it not also
> serve the purpose of causing the pcpu to pick up the potential new
> interrupt?

Yes. Before this patch we were already sending an SGI to the other pcpu
so that it would pick up the new IRQ.
Now we also send an SGI to the other pcpu even if the IRQ is already
inflight, so that the second pcpu can clear the LR corresponding to the
previous injection as well as injecting the new interrupt.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > Changes in 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  |  135 ++++++++++++++++++++++-----------------------------
> >  xen/arch/arm/vgic.c |    3 +-
> >  2 files changed, 60 insertions(+), 78 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index dbba5d3..32d3bea 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -128,6 +130,7 @@ void gic_restore_state(struct vcpu *v)
> >      GICH[GICH_HCR] = GICH_HCR_EN;
> >      isb();
> >  
> > +    gic_clear_lrs(v);
> >      gic_restore_pending_irqs(v);
> 
> Not related to this patch exactly, but is this function badly named? It
> seems to not actually be restoring anything but is actually looking for
> any newly pending irqs which it can now inject, is that right?

Yeah, that is correct.


> Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which
> suggests that the clear should be done there (which also seems logical).

It is just temporary: patch "call gic_clear_lrs on entry to the
hypervisor" moves the call to gic_clear_lrs to traps.c.


> >  }
> >  
> > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
> >  static inline void gic_set_lr(int lr, struct pending_irq *p,
> >          unsigned int state)
> >  {
> > -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> > +    uint32_t lr_reg;
> >  
> >      BUG_ON(lr >= nr_lrs);
> >      BUG_ON(lr < 0);
> >      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
> >  
> > -    GICH[GICH_LR + lr] = state |
> > -        maintenance_int |
> > -        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > +    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> >          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> > +    if ( p->desc != NULL )
> > +        lr_reg |= GICH_LR_HW |
> > +            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT);
> 
> It seems like it would be a silicon design bug if this were ever true.
> So BUG_ON(p->desc->irq & ~GICH_LR_PHYSICAL_MASK)?
 
Right, I'll remove it.


> I think this should be checked at the time the interrupt is routed
> instead of here, which gets it out of this hotter path.

Actually it cannot happen as desc->irq is initialized by init_irq_data
in a for loop up to NR_IRQS (1024).


> Another future cleanup: I think a lot of this register frobbing code
> would be cleaner if GICH_LR_FOO_MASK was already shifted.
> 
> [...]
> > +static void gic_clear_lrs(struct vcpu *v)
> > +{
> > +    struct pending_irq *p;
> > +    int i = 0, irq;
> > +    uint32_t lr;
> > +    bool_t inflight;
> > +
> > +    ASSERT(!local_irq_is_enabled());
> > +
> > +    while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
> 
> "long unsigned int" is an odd one isn't it?
> 
> It seems like all of the other places which do bitops on lr_mask manage
> to avoid any case at all.

_find_next_bit_le requires a const unsigned long *p as first argument.
I'll make the change to const unsigned long.


> > +                              nr_lrs, i)) < nr_lrs) {
> > +        lr = GICH[GICH_LR + i];
> > +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> 
> This state means the lr is "completed" (or inactive, however you want to
> think about it)? A little helper macro lr_completed(lr) would clarify
> this without needing a comment.

This is another one of those pieces of code that are going to disappear
in the following patches. I think the final version is going to look
nicer, even without macros.


> Isn't that condition also true for an LR which was never injected? Won't
> we then try and complete a non-existent interrupt? Ah, this is protected
> by the lr_mask isn't it.

We are only iterating over lr_mask, that means that we have written to
this lr in the recent past.


> > +        {
> > +            inflight = 0;
> > +            GICH[GICH_LR + i] = 0;
> > +            clear_bit(i, &this_cpu(lr_mask));
> > +
> > +            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > +            spin_lock(&gic.lock);
> > +            p = irq_to_pending(v, irq);
> > +            if ( p->desc != NULL )
> > +                p->desc->status &= ~IRQ_INPROGRESS;
> > +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > +            if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> > +                    test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > +            {
> > +                inflight = 1;
> > +                gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> > +            }
> > +            spin_unlock(&gic.lock);
> 
> Can an interrupt arrive at this point and make this interrupt "inflight"
> again, such that the following list remove is actually wrong?

gic_clear_lrs is always called with irq disabled, see the ASSERT at the
beginning of the function


> > +            {
> > +                spin_lock(&v->arch.vgic.lock);
> > +                list_del_init(&p->inflight);
> > +                spin_unlock(&v->arch.vgic.lock);
> > +            }
> > +
> > +        }
> > +
> > +        i++;
> > +    }
> > +}
> > +
> >  static void gic_restore_pending_irqs(struct vcpu *v)
> >  {
> >      int i;
> >  static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
> >  {
> [...]
> 
> Is now empty but not removed?

Yes. We want to keep receiving maintenance interrupts, but we don't need
to do anything anymore in the handler because everything we need to do
is done on return to guest.

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

[Ian Campbell]

On Fri, 2014-03-21 at 15:55 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> > > If the irq to be injected is an hardware irq (p->desc != NULL), set
> > > GICH_LR_HW. Do not set GICH_LR_MAINTENANCE_IRQ.
> > > 
> > > Remove the code to EOI a physical interrupt on behalf of the guest
> > > because it has become unnecessary.
> > 
> > Stupid question: there is no possibility of a h/w interrupt which for
> > one reason or another cannot be injected using these GIC facilities?
> 
> I don't think so. Nothing is written in spec about such a case.
> 
> 
> > > 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 from gic_restore_state and on return to guest
> > > (gic_inject).
> > > 
> > > In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu,
> > > send and SGI to it to interrupt it and force it to clear the old LRs.
> > 
> > s/and/an/
> > 
> > Is the SGI just there to cause it to flush its LRs? Does it not also
> > serve the purpose of causing the pcpu to pick up the potential new
> > interrupt?
> 
> Yes. Before this patch we were already sending an SGI to the other pcpu
> so that it would pick up the new IRQ.
> Now we also send an SGI to the other pcpu even if the IRQ is already
> inflight, so that the second pcpu can clear the LR corresponding to the
> previous injection as well as injecting the new interrupt.

Can you clarify that in the commit message please? (pretty much
inserting what you just said will do).

I assume that there is no chance we will send two SGIs?

> > Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which
> > suggests that the clear should be done there (which also seems logical).
> 
> It is just temporary: patch "call gic_clear_lrs on entry to the
> hypervisor" moves the call to gic_clear_lrs to traps.c.

I noticed that later, any reason for taking this roundabout path to the
final destination?

> > > @@ -625,16 +628,19 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
> > >  static inline void gic_set_lr(int lr, struct pending_irq *p,
> > >          unsigned int state)
> > >  {
> > > -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> > > +    uint32_t lr_reg;
> > >  
> > >      BUG_ON(lr >= nr_lrs);
> > >      BUG_ON(lr < 0);
> > >      BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
> > >  
> > > -    GICH[GICH_LR + lr] = state |
> > > -        maintenance_int |
> > > -        ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > > +    lr_reg = state | ((p->priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > >          ((p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> > > +    if ( p->desc != NULL )
> > > +        lr_reg |= GICH_LR_HW |
> > > +            ((p->desc->irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT);
> > 
> > It seems like it would be a silicon design bug if this were ever true.
> > So BUG_ON(p->desc->irq & ~GICH_LR_PHYSICAL_MASK)?
>  
> Right, I'll remove it.
> 
> 
> > I think this should be checked at the time the interrupt is routed
> > instead of here, which gets it out of this hotter path.
> 
> Actually it cannot happen as desc->irq is initialized by init_irq_data
> in a for loop up to NR_IRQS (1024).

Excellent.

(although beware gic v3 ;-))


> > > +        {
> > > +            inflight = 0;
> > > +            GICH[GICH_LR + i] = 0;
> > > +            clear_bit(i, &this_cpu(lr_mask));
> > > +
> > > +            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > > +            spin_lock(&gic.lock);
> > > +            p = irq_to_pending(v, irq);
> > > +            if ( p->desc != NULL )
> > > +                p->desc->status &= ~IRQ_INPROGRESS;
> > > +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > > +            if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> > > +                    test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > > +            {
> > > +                inflight = 1;
> > > +                gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> > > +            }
> > > +            spin_unlock(&gic.lock);
> > 
> > Can an interrupt arrive at this point and make this interrupt "inflight"
> > again, such that the following list remove is actually wrong?
> 
> gic_clear_lrs is always called with irq disabled, see the ASSERT at the
> beginning of the function

Great.

> > 
> > Is now empty but not removed?
> 
> Yes. We want to keep receiving maintenance interrupts, but we don't need
> to do anything anymore in the handler because everything we need to do
> is done on return to guest.

I figured that out on the next patch -- a comment in any empty interrupt
handler would be very useful.

Ian.

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which
> > > suggests that the clear should be done there (which also seems logical).
> > 
> > It is just temporary: patch "call gic_clear_lrs on entry to the
> > hypervisor" moves the call to gic_clear_lrs to traps.c.
> 
> I noticed that later, any reason for taking this roundabout path to the
> final destination?

I can merge the two patches together if you prefer.
I tried to keep all the logical changes separated to allow better
debugging and easier reviews. Also it is much easier to merge patches
later upon request :-)

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

[Ian Campbell]

On Mon, 2014-03-24 at 12:11 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > > Both callers of gic_restore_pending_irqs call gic_clear_lrs. Which
> > > > suggests that the clear should be done there (which also seems logical).
> > > 
> > > It is just temporary: patch "call gic_clear_lrs on entry to the
> > > hypervisor" moves the call to gic_clear_lrs to traps.c.
> > 
> > I noticed that later, any reason for taking this roundabout path to the
> > final destination?
> 
> I can merge the two patches together if you prefer.
> I tried to keep all the logical changes separated to allow better
> debugging and easier reviews. Also it is much easier to merge patches
> later upon request :-)

The flip side is that reviewers now and up looking at some intermediate
state which isn't actually useful (and is actually something of a waste
of their time).

Ian.

[PATCH v4 04/10] xen/arm: set GICH_HCR_UIE if all the LRs are in use

[Stefano Stabellini]

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

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

---

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 32d3bea..d445e8b 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -790,6 +790,12 @@ 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) &&
+         this_cpu(lr_mask) == ((1 << nr_lrs) - 1) )
+        GICH[GICH_HCR] |= GICH_HCR_UIE;
+    else
+        GICH[GICH_HCR] &= ~GICH_HCR_UIE;
 }
 
 int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
-- 
1.7.10.4

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

[Julien Grall]

On 03/19/2014 12:31 PM, Stefano Stabellini wrote:
> On return to guest, if there are no free LRs and we still have more
> interrupt to inject, set GICH_HCR_UIE so that we are going to receive a
> maintenance interrupt when no pending interrupts are present in the LR
> registers.
> The maintenance interrupt handler won't do anything anymore, but
> receiving the interrupt is going to cause gic_inject to be called on
> return to guest that is going to clear the old LRs and inject new
> interrupts.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

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

[Ian Campbell]

On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> On return to guest, if there are no free LRs and we still have more
> interrupt to inject, set GICH_HCR_UIE so that we are going to receive a
> maintenance interrupt when no pending interrupts are present in the LR
> registers.
> The maintenance interrupt handler won't do anything anymore, but
> receiving the interrupt is going to cause gic_inject to be called on
> return to guest that is going to clear the old LRs and inject new
> interrupts.

Can you add a comment to the (now dummy) interrupt handler explaining
this please.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v2:
> - disable/enable the GICH_HCR_UIE bit in GICH_HCR;
> - only enable GICH_HCR_UIE if this_cpu(lr_mask) == ((1 << nr_lrs) - 1).
> ---
>  xen/arch/arm/gic.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 32d3bea..d445e8b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -790,6 +790,12 @@ 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) &&
> +         this_cpu(lr_mask) == ((1 << nr_lrs) - 1) )

Helper like lr_all_full?

> +        GICH[GICH_HCR] |= GICH_HCR_UIE;
> +    else
> +        GICH[GICH_HCR] &= ~GICH_HCR_UIE;
>  }
>  
>  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> > On return to guest, if there are no free LRs and we still have more
> > interrupt to inject, set GICH_HCR_UIE so that we are going to receive a
> > maintenance interrupt when no pending interrupts are present in the LR
> > registers.
> > The maintenance interrupt handler won't do anything anymore, but
> > receiving the interrupt is going to cause gic_inject to be called on
> > return to guest that is going to clear the old LRs and inject new
> > interrupts.
> 
> Can you add a comment to the (now dummy) interrupt handler explaining
> this please.

Sure


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - disable/enable the GICH_HCR_UIE bit in GICH_HCR;
> > - only enable GICH_HCR_UIE if this_cpu(lr_mask) == ((1 << nr_lrs) - 1).
> > ---
> >  xen/arch/arm/gic.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 32d3bea..d445e8b 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -790,6 +790,12 @@ 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) &&
> > +         this_cpu(lr_mask) == ((1 << nr_lrs) - 1) )
> 
> Helper like lr_all_full?

Good idea


> > +        GICH[GICH_HCR] |= GICH_HCR_UIE;
> > +    else
> > +        GICH[GICH_HCR] &= ~GICH_HCR_UIE;
> >  }
> >  
> >  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
> 
> 

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

[Ian Campbell]

On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> On return to guest, if there are no free LRs and we still have more
> interrupt to inject, set GICH_HCR_UIE so that we are going to receive a
> maintenance interrupt when no pending interrupts are present in the LR
> registers.

Should this go before the previous patch? Otherwise how to things work
between #3 and #4? This would be harmless to go in first I think, apart
from some spurious maintenance interrupts?

> 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>
> 
> ---
> 
> Changes in v2:
> - disable/enable the GICH_HCR_UIE bit in GICH_HCR;
> - only enable GICH_HCR_UIE if this_cpu(lr_mask) == ((1 << nr_lrs) - 1).
> ---
>  xen/arch/arm/gic.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 32d3bea..d445e8b 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -790,6 +790,12 @@ 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) &&
> +         this_cpu(lr_mask) == ((1 << nr_lrs) - 1) )
> +        GICH[GICH_HCR] |= GICH_HCR_UIE;
> +    else
> +        GICH[GICH_HCR] &= ~GICH_HCR_UIE;
>  }
>  
>  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:31 +0000, Stefano Stabellini wrote:
> > On return to guest, if there are no free LRs and we still have more
> > interrupt to inject, set GICH_HCR_UIE so that we are going to receive a
> > maintenance interrupt when no pending interrupts are present in the LR
> > registers.
> 
> Should this go before the previous patch? Otherwise how to things work
> between #3 and #4? This would be harmless to go in first I think, apart
> from some spurious maintenance interrupts?

In practice you always receive timer interrupts so things would work
even without this patch.  However like you say moving this patch earlier
is harmless, so I'll do it.


> > 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>
> > 
> > ---
> > 
> > Changes in v2:
> > - disable/enable the GICH_HCR_UIE bit in GICH_HCR;
> > - only enable GICH_HCR_UIE if this_cpu(lr_mask) == ((1 << nr_lrs) - 1).
> > ---
> >  xen/arch/arm/gic.c |    6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 32d3bea..d445e8b 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -790,6 +790,12 @@ 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) &&
> > +         this_cpu(lr_mask) == ((1 << nr_lrs) - 1) )
> > +        GICH[GICH_HCR] |= GICH_HCR_UIE;
> > +    else
> > +        GICH[GICH_HCR] &= ~GICH_HCR_UIE;
> >  }
> >  
> >  int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
> 
> 

[PATCH v4 05/10] xen/arm: keep track of the GICH_LR used for the irq in struct pending_irq

[Stefano Stabellini]

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index d445e8b..78e043c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -644,6 +644,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)
@@ -724,6 +725,7 @@ static void gic_clear_lrs(struct vcpu *v)
             if ( p->desc != NULL )
                 p->desc->status &= ~IRQ_INPROGRESS;
             clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+            p->lr = nr_lrs;
             if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
                     test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
             {
@@ -966,12 +968,12 @@ 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 )
     {
-        printk("Pending irq=%d\n", p->irq);
+        printk("Pending irq=%d lr=%u\n", p->irq, p->lr);
     }
 
 }
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index bc20a15..ea89057 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -59,6 +59,7 @@ struct pending_irq
 #define GIC_IRQ_GUEST_VISIBLE  1
 #define GIC_IRQ_GUEST_ENABLED  2
     unsigned long status;
+    uint8_t lr;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
     uint8_t priority;
     /* inflight is used to append instances of pending_irq to
-- 
1.7.10.4

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

[Julien Grall]

Hi Stefano,

On 03/19/2014 12:32 PM, Stefano Stabellini wrote:
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index bc20a15..ea89057 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -59,6 +59,7 @@ struct pending_irq
>  #define GIC_IRQ_GUEST_VISIBLE  1
>  #define GIC_IRQ_GUEST_ENABLED  2
>      unsigned long status;
> +    uint8_t lr;

You are using uint8_t here but nr_lrs is defined as unsigned. Can you
also change nr_lrs type to uint8_t?

Regards,

-- 
Julien Grall

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

[Stefano Stabellini]

On Wed, 19 Mar 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 03/19/2014 12:32 PM, Stefano Stabellini wrote:
> > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > index bc20a15..ea89057 100644
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -59,6 +59,7 @@ struct pending_irq
> >  #define GIC_IRQ_GUEST_VISIBLE  1
> >  #define GIC_IRQ_GUEST_ENABLED  2
> >      unsigned long status;
> > +    uint8_t lr;
> 
> You are using uint8_t here but nr_lrs is defined as unsigned. Can you
> also change nr_lrs type to uint8_t?

Yes, I'll add a patch for that.

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

[Ian Campbell]

On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:

Strictly you are tracking the last LR which this interrupt was in, since
you don't clear p->lr AFAICT. Maybe this is OK and things never get
confused by it, but it might have surprising results...

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/gic.c           |    6 ++++--
>  xen/include/asm-arm/domain.h |    1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index d445e8b..78e043c 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -644,6 +644,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)
> @@ -724,6 +725,7 @@ static void gic_clear_lrs(struct vcpu *v)
>              if ( p->desc != NULL )
>                  p->desc->status &= ~IRQ_INPROGRESS;
>              clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> +            p->lr = nr_lrs;
>              if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
>                      test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
>              {
> @@ -966,12 +968,12 @@ 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 )
>      {
> -        printk("Pending irq=%d\n", p->irq);
> +        printk("Pending irq=%d lr=%u\n", p->irq, p->lr);

Are lr_pending interrupts in an LR? I thought they were waiting for an
LR to become available.

>      }
>  
>  }
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index bc20a15..ea89057 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -59,6 +59,7 @@ struct pending_irq
>  #define GIC_IRQ_GUEST_VISIBLE  1
>  #define GIC_IRQ_GUEST_ENABLED  2
>      unsigned long status;
> +    uint8_t lr;

Put this next to priority to improve the packing, you've just added
another 7 byte hole to the struct on arm64 (and 3 on arm32).

(pulling int irq from just out of scope down into the same area might
also improve packing on arm64, since irq is just 4 bytes).

>      struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
>      uint8_t priority;
>      /* inflight is used to append instances of pending_irq to

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> 
> Strictly you are tracking the last LR which this interrupt was in, since
> you don't clear p->lr AFAICT. Maybe this is OK and things never get
> confused by it, but it might have surprising results...

Actually the patch is clearing p->lr by setting it to nr_lrs, that of
course is invalid.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/gic.c           |    6 ++++--
> >  xen/include/asm-arm/domain.h |    1 +
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index d445e8b..78e043c 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -644,6 +644,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)
> > @@ -724,6 +725,7 @@ static void gic_clear_lrs(struct vcpu *v)
> >              if ( p->desc != NULL )
> >                  p->desc->status &= ~IRQ_INPROGRESS;
> >              clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > +            p->lr = nr_lrs;
> >              if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> >                      test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> >              {
> > @@ -966,12 +968,12 @@ 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 )
> >      {
> > -        printk("Pending irq=%d\n", p->irq);
> > +        printk("Pending irq=%d lr=%u\n", p->irq, p->lr);
> 
> Are lr_pending interrupts in an LR? I thought they were waiting for an
> LR to become available.

You are right, this change is wrong (and confusing).


> >      }
> >  
> >  }
> > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > index bc20a15..ea89057 100644
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -59,6 +59,7 @@ struct pending_irq
> >  #define GIC_IRQ_GUEST_VISIBLE  1
> >  #define GIC_IRQ_GUEST_ENABLED  2
> >      unsigned long status;
> > +    uint8_t lr;
> 
> Put this next to priority to improve the packing, you've just added
> another 7 byte hole to the struct on arm64 (and 3 on arm32).
>
> (pulling int irq from just out of scope down into the same area might
> also improve packing on arm64, since irq is just 4 bytes).

Good idea

> >      struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
> >      uint8_t priority;
> >      /* inflight is used to append instances of pending_irq to
> 
> 

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

[Ian Campbell]

On Fri, 2014-03-21 at 16:19 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > 
> > Strictly you are tracking the last LR which this interrupt was in, since
> > you don't clear p->lr AFAICT. Maybe this is OK and things never get
> > confused by it, but it might have surprising results...
> 
> Actually the patch is clearing p->lr by setting it to nr_lrs, that of
> course is invalid.

Ah, that is a bit non-obvious. Better would be
        #define INVALID_LR ~(type_t)0
and use that.

> > >      }
> > >  
> > >  }
> > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > > index bc20a15..ea89057 100644
> > > --- a/xen/include/asm-arm/domain.h
> > > +++ b/xen/include/asm-arm/domain.h
> > > @@ -59,6 +59,7 @@ struct pending_irq
> > >  #define GIC_IRQ_GUEST_VISIBLE  1
> > >  #define GIC_IRQ_GUEST_ENABLED  2
> > >      unsigned long status;
> > > +    uint8_t lr;
> > 
> > Put this next to priority to improve the packing, you've just added
> > another 7 byte hole to the struct on arm64 (and 3 on arm32).
> >
> > (pulling int irq from just out of scope down into the same area might
> > also improve packing on arm64, since irq is just 4 bytes).
> 
> Good idea

There was a tool which would tell you about the holes in your structs.
Now what was it called...

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Fri, 2014-03-21 at 16:19 +0000, Stefano Stabellini wrote:
> > On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > 
> > > Strictly you are tracking the last LR which this interrupt was in, since
> > > you don't clear p->lr AFAICT. Maybe this is OK and things never get
> > > confused by it, but it might have surprising results...
> > 
> > Actually the patch is clearing p->lr by setting it to nr_lrs, that of
> > course is invalid.
> 
> Ah, that is a bit non-obvious. Better would be
>         #define INVALID_LR ~(type_t)0
> and use that.

Yeah, this is a good idea, I'll make the change.

[PATCH v4 06/10] xen/arm: s/gic_set_guest_irq/gic_raise_guest_irq

[Stefano Stabellini]

Rename gic_set_guest_irq to gic_raise_guest_irq and remove the state
parameter.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 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 78e043c..a5a4da3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -676,8 +676,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 irq,
-        unsigned int state, unsigned int priority)
+void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
+                         unsigned int priority)
 {
     int i;
     unsigned long flags;
@@ -689,7 +689,7 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int 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, irq), state);
+            gic_set_lr(i, irq_to_pending(v, irq), GICH_LR_PENDING);
             goto out;
         }
     }
@@ -730,7 +730,7 @@ static void gic_clear_lrs(struct vcpu *v)
                     test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
             {
                 inflight = 1;
-                gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+                gic_raise_guest_irq(v, irq, p->priority);
             }
             spin_unlock(&gic.lock);
             if ( !inflight )
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 566f0ff..3913cf5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -390,7 +390,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         p = irq_to_pending(v, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
         if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-            gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+            gic_raise_guest_irq(v, irq, p->priority);
         if ( p->desc != NULL )
             p->desc->handler->enable(p->desc);
         i++;
@@ -719,7 +719,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     /* the irq is enabled */
     if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
-        gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
+        gic_raise_guest_irq(v, irq, priority);
 
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
     {
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 6fce5c2..4834cd6 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -178,8 +178,8 @@ extern void gic_clear_pending_irqs(struct vcpu *v);
 extern int gic_events_need_delivery(void);
 
 extern void __cpuinit init_maintenance_interrupt(void);
-extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
-        unsigned int state, unsigned int priority);
+extern void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
+        unsigned int priority);
 extern void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq);
 extern int gic_route_irq_to_guest(struct domain *d,
                                   const struct dt_irq *irq,
-- 
1.7.10.4

Re: [PATCH v4 06/10] xen/arm: s/gic_set_guest_irq/gic_raise_guest_irq

[Julien Grall]

On 03/19/2014 12:32 PM, Stefano Stabellini wrote:
> Rename gic_set_guest_irq to gic_raise_guest_irq and remove the state
> parameter.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

Re: [PATCH v4 06/10] xen/arm: s/gic_set_guest_irq/gic_raise_guest_irq

[Ian Campbell]

On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> Rename gic_set_guest_irq to gic_raise_guest_irq and remove the state
> parameter.
> 
> @@ -689,7 +689,7 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int 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, irq), state);
> +            gic_set_lr(i, irq_to_pending(v, irq), GICH_LR_PENDING);

Is this a stray change which belopngs in another patch?

Re: [PATCH v4 06/10] xen/arm: s/gic_set_guest_irq/gic_raise_guest_irq

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > Rename gic_set_guest_irq to gic_raise_guest_irq and remove the state
> > parameter.
> > 
> > @@ -689,7 +689,7 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int 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, irq), state);
> > +            gic_set_lr(i, irq_to_pending(v, irq), GICH_LR_PENDING);
> 
> Is this a stray change which belopngs in another patch?

No, in the commit message I wrote:

"Rename gic_set_guest_irq to gic_raise_guest_irq and remove the state
parameter."

Re: [PATCH v4 06/10] xen/arm: s/gic_set_guest_irq/gic_raise_guest_irq

[Ian Campbell]

On Fri, 2014-03-21 at 16:20 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > Rename gic_set_guest_irq to gic_raise_guest_irq and remove the state
> > > parameter.
> > > 
> > > @@ -689,7 +689,7 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int 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, irq), state);
> > > +            gic_set_lr(i, irq_to_pending(v, irq), GICH_LR_PENDING);
> > 
> > Is this a stray change which belopngs in another patch?
> 
> No, in the commit message I wrote:
> 
> "Rename gic_set_guest_irq to gic_raise_guest_irq and remove the state
> parameter."

Ah, I missed that this change was inside gic_set_guest_irq itself, sorry
for the noise.

Ian.

[PATCH v4 07/10] xen/arm: call gic_clear_lrs on entry to the hypervisor

[Stefano Stabellini]

This change is needed by other patches later on. It is going 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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic.c        |   12 +++++-------
 xen/arch/arm/traps.c      |   10 ++++++++++
 xen/include/asm-arm/gic.h |    1 +
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index a5a4da3..3f061cf 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -67,8 +67,6 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 #define NR_GIC_CPU_IF 8
 
-static void gic_clear_lrs(struct vcpu *v);
-
 static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
 {
     unsigned int cpu;
@@ -130,7 +128,6 @@ void gic_restore_state(struct vcpu *v)
     GICH[GICH_HCR] = GICH_HCR_EN;
     isb();
 
-    gic_clear_lrs(v);
     gic_restore_pending_irqs(v);
 }
 
@@ -701,14 +698,15 @@ out:
     return;
 }
 
-static void gic_clear_lrs(struct vcpu *v)
+void gic_clear_lrs(struct vcpu *v)
 {
     struct pending_irq *p;
     int i = 0, irq;
     uint32_t lr;
     bool_t inflight;
+    unsigned long flags;
 
-    ASSERT(!local_irq_is_enabled());
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
     while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
                               nr_lrs, i)) < nr_lrs) {
@@ -744,6 +742,8 @@ static void gic_clear_lrs(struct vcpu *v)
 
         i++;
     }
+
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
 static void gic_restore_pending_irqs(struct vcpu *v)
@@ -786,8 +786,6 @@ int gic_events_need_delivery(void)
 
 void gic_inject(void)
 {
-    gic_clear_lrs(current);
-
     if ( vcpu_info(current, evtchn_upcall_pending) )
         vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq);
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 21c7b26..dd936be 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -68,6 +68,7 @@ static int debug_stack_lines = 40;
 
 integer_param("debug_stack_lines", debug_stack_lines);
 
+static void enter_hypervisor_head(void);
 
 void __cpuinit init_traps(void)
 {
@@ -1543,6 +1544,8 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 {
     union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) };
 
+    enter_hypervisor_head();
+
     switch (hsr.ec) {
     case HSR_EC_WFI_WFE:
         if ( !check_conditional_instr(regs, hsr) )
@@ -1620,11 +1623,13 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs)
 
 asmlinkage void do_trap_irq(struct cpu_user_regs *regs)
 {
+    enter_hypervisor_head();
     gic_interrupt(regs, 0);
 }
 
 asmlinkage void do_trap_fiq(struct cpu_user_regs *regs)
 {
+    enter_hypervisor_head();
     gic_interrupt(regs, 1);
 }
 
@@ -1642,6 +1647,11 @@ asmlinkage void leave_hypervisor_tail(void)
     }
 }
 
+static void enter_hypervisor_head(void)
+{
+    gic_clear_lrs(current);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 4834cd6..5a9dc77 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -220,6 +220,7 @@ extern unsigned int gic_number_lines(void);
 /* IRQ translation function for the device tree */
 int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
                   unsigned int *out_hwirq, unsigned int *out_type);
+void gic_clear_lrs(struct vcpu *v);
 
 #endif /* __ASSEMBLY__ */
 #endif
-- 
1.7.10.4

Re: [PATCH v4 07/10] xen/arm: call gic_clear_lrs on entry to the hypervisor

[Julien Grall]

On 03/19/2014 12:32 PM, Stefano Stabellini wrote:
> This change is needed by other patches later on. It is going 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

Re: [PATCH v4 07/10] xen/arm: call gic_clear_lrs on entry to the hypervisor

[Ian Campbell]

On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:

Can this not be folded back into the patch which added this function?

> This change is needed by other patches later on. It is going 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.

Hrm, can we not do this on demand just at the point where we are about
to make such a calculation? There are going to be lots of hypervisor
entries which don't want to do anything at all with interrupts, aren't
there?

Ian.

Re: [PATCH v4 07/10] xen/arm: call gic_clear_lrs on entry to the hypervisor

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> 
> Can this not be folded back into the patch which added this function?

Yes, it can.


> > This change is needed by other patches later on. It is going 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.
> 
> Hrm, can we not do this on demand just at the point where we are about
> to make such a calculation? There are going to be lots of hypervisor
> entries which don't want to do anything at all with interrupts, aren't
> there?

The alternative would be calling gic_clear_lrs at the beginning of
gic_inject and gic_events_need_delivery, that is called by
local_events_need_delivery*.  It could be called multiple times before
returning to guest.

However gic_clear_lrs only iterates over the LRs in lr_mask, so even
calling it multiple times shouldn't cause more work, the only slow down
would be due to the spin_lock.

Re: [PATCH v4 07/10] xen/arm: call gic_clear_lrs on entry to the hypervisor

[Ian Campbell]

On Fri, 2014-03-21 at 16:34 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > 
> > Can this not be folded back into the patch which added this function?
> 
> Yes, it can.
> 
> 
> > > This change is needed by other patches later on. It is going 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.
> > 
> > Hrm, can we not do this on demand just at the point where we are about
> > to make such a calculation? There are going to be lots of hypervisor
> > entries which don't want to do anything at all with interrupts, aren't
> > there?
> 
> The alternative would be calling gic_clear_lrs at the beginning of
> gic_inject and gic_events_need_delivery, that is called by
> local_events_need_delivery*.  It could be called multiple times before
> returning to guest.

We could probably invent some sort of "once per h/v entry" construct,
but ick.

What I was actually thinking of though was to do it even further up --
e.g. in the function which makes the interrupt pending in the first
place. I suppose that is called too infrequently though in the absence
of maintenance interrupts.

> 
> However gic_clear_lrs only iterates over the LRs in lr_mask, so even
> calling it multiple times shouldn't cause more work, the only slow down
> would be due to the spin_lock.

Re: [PATCH v4 07/10] xen/arm: call gic_clear_lrs on entry to the hypervisor

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Fri, 2014-03-21 at 16:34 +0000, Stefano Stabellini wrote:
> > On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > 
> > > Can this not be folded back into the patch which added this function?
> > 
> > Yes, it can.
> > 
> > 
> > > > This change is needed by other patches later on. It is going 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.
> > > 
> > > Hrm, can we not do this on demand just at the point where we are about
> > > to make such a calculation? There are going to be lots of hypervisor
> > > entries which don't want to do anything at all with interrupts, aren't
> > > there?
> > 
> > The alternative would be calling gic_clear_lrs at the beginning of
> > gic_inject and gic_events_need_delivery, that is called by
> > local_events_need_delivery*.  It could be called multiple times before
> > returning to guest.
> 
> We could probably invent some sort of "once per h/v entry" construct,
> but ick.
> 
> What I was actually thinking of though was to do it even further up --
> e.g. in the function which makes the interrupt pending in the first
> place. I suppose that is called too infrequently though in the absence
> of maintenance interrupts.

Yes, that function would not be called often enough: in order to
calculate correctly if events need delivery, we need to know the current
value of GICV_PMR.Priority, that is aliased by GICH_VMCR and can be
changed by the guest without traps, and the current active highest
priority interrupt in the LRs.  As a consequence I think that
gic_events_need_delivery needs to be called at least once before
returning to guest.

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

[Stefano Stabellini]

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

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

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

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

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

---
Changes in v3:
- do not use the PENDING and ACTIVE state for HW interrupts;
- unify the inflight and non-inflight code paths in
vgic_vcpu_inject_irq.
---
 xen/arch/arm/gic.c  |   89 +++++++++++++++++++++++++++++----------------------
 xen/arch/arm/vgic.c |   33 ++++++++++---------
 2 files changed, 68 insertions(+), 54 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3f061cf..128d071 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
 /* Maximum cpu interface per GIC */
 #define NR_GIC_CPU_IF 8
 
+static void _gic_clear_lr(struct vcpu *v, int i);
+
 static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
 {
     unsigned int cpu;
@@ -678,6 +680,14 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
 {
     int i;
     unsigned long flags;
+    struct pending_irq *n = irq_to_pending(v, irq);
+
+    if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status))
+    {
+        if ( v == current )
+            _gic_clear_lr(v, n->lr);
+        return;
+    }
 
     spin_lock_irqsave(&gic.lock, flags);
 
@@ -698,51 +708,57 @@ out:
     return;
 }
 
-void gic_clear_lrs(struct vcpu *v)
+static void _gic_clear_lr(struct vcpu *v, int i)
 {
-    struct pending_irq *p;
-    int i = 0, irq;
+    int irq;
     uint32_t lr;
-    bool_t inflight;
+    struct pending_irq *p;
+
+    lr = GICH[GICH_LR + i];
+    irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
+    p = irq_to_pending(v, irq);
+    if ( lr & GICH_LR_ACTIVE )
+    {
+        /* HW interrupts cannot be ACTIVE and PENDING */
+        if ( p->desc == NULL &&
+             test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
+             test_and_clear_bit(GIC_IRQ_GUEST_PENDING, &p->status) )
+            GICH[GICH_LR + i] = lr | GICH_LR_PENDING;
+    } else if ( lr & GICH_LR_PENDING ) {
+        clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
+    } else {
+        spin_lock(&gic.lock);
+
+        GICH[GICH_LR + i] = 0;
+        clear_bit(i, &this_cpu(lr_mask));
+
+        if ( p->desc != NULL )
+            p->desc->status &= ~IRQ_INPROGRESS;
+        clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
+        p->lr = nr_lrs;
+        if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
+                test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
+        {
+            gic_raise_guest_irq(v, irq, p->priority);
+        } else
+            list_del_init(&p->inflight);
+
+        spin_unlock(&gic.lock);
+    }
+}
+
+void gic_clear_lrs(struct vcpu *v)
+{
+    int i = 0;
     unsigned long flags;
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
-
     while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask),
                               nr_lrs, i)) < nr_lrs) {
-        lr = GICH[GICH_LR + i];
-        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
-        {
-            inflight = 0;
-            GICH[GICH_LR + i] = 0;
-            clear_bit(i, &this_cpu(lr_mask));
-
-            irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
-            spin_lock(&gic.lock);
-            p = irq_to_pending(v, irq);
-            if ( p->desc != NULL )
-                p->desc->status &= ~IRQ_INPROGRESS;
-            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
-            p->lr = nr_lrs;
-            if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
-                    test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
-            {
-                inflight = 1;
-                gic_raise_guest_irq(v, irq, p->priority);
-            }
-            spin_unlock(&gic.lock);
-            if ( !inflight )
-            {
-                spin_lock(&v->arch.vgic.lock);
-                list_del_init(&p->inflight);
-                spin_unlock(&v->arch.vgic.lock);
-            }
-
-        }
 
+        _gic_clear_lr(v, i);
         i++;
     }
-
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
@@ -786,9 +802,6 @@ int gic_events_need_delivery(void)
 
 void gic_inject(void)
 {
-    if ( vcpu_info(current, evtchn_upcall_pending) )
-        vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq);
-
     gic_restore_pending_irqs(current);
 
     if ( !list_empty(&current->arch.vgic.lr_pending) &&
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 3913cf5..dc3a75f 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -389,7 +389,11 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         irq = i + (32 * n);
         p = irq_to_pending(v, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
-        if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+        if ( irq == v->domain->arch.evtchn_irq &&
+             vcpu_info(current, evtchn_upcall_pending) &&
+             list_empty(&p->inflight) )
+            vgic_vcpu_inject_irq(v, irq);
+        else if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
             gic_raise_guest_irq(v, irq, p->priority);
         if ( p->desc != NULL )
             p->desc->handler->enable(p->desc);
@@ -696,14 +700,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
-    if ( !list_empty(&n->inflight) )
-    {
-        if ( (irq != current->domain->arch.evtchn_irq) ||
-             (!test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status)) )
-            set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
-        goto out;
-    }
-
     /* vcpu offline */
     if ( test_bit(_VPF_down, &v->pause_flags) )
     {
@@ -715,21 +711,26 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq)
 
     n->irq = irq;
     set_bit(GIC_IRQ_GUEST_PENDING, &n->status);
-    n->priority = priority;
 
     /* the irq is enabled */
     if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
         gic_raise_guest_irq(v, irq, priority);
 
-    list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
+    if ( list_empty(&n->inflight) )
     {
-        if ( iter->priority > priority )
+        n->priority = priority;
+        list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
         {
-            list_add_tail(&n->inflight, &iter->inflight);
-            goto out;
+            if ( iter->priority > priority )
+            {
+                list_add_tail(&n->inflight, &iter->inflight);
+                goto out;
+            }
         }
-    }
-    list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
+        list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
+    } else if ( n->priority != priority )
+        gdprintk(XENLOG_WARNING, "Changing priority of an inflight interrupt is not supported");
+
 out:
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
     /* we have a new higher priority irq, inject it into the guest */
-- 
1.7.10.4

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

[Ian Campbell]

On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq
> while the first one is still active.
> If the first irq is already pending (not active), just clear
> GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> already visible by the guest.
> If the irq has already been EOI'ed then just clear the GICH_LR right
> away and move the interrupt to lr_pending so that it is going to be
> reinjected by gic_restore_pending_irqs on return to guest.
> 
> If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_PENDING
> and send an SGI. The target cpu is going to be interrupted and call
> gic_clear_lrs, that is going to take the same actions.
> 
> Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq.
> 
> Do not call vgic_vcpu_inject_irq from gic_inject if
> evtchn_upcall_pending is set. If we remove that call, we don't need to
> special case evtchn_irq in vgic_vcpu_inject_irq anymore.

That's the consequence of removing it, but what is the rationale for why
it is OK?

> We also need to force the first injection of evtchn_irq (call
> gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> is already set by common code on vcpu creation.

This is because the common code expects that the guest is moving from
sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but
on ARM we start off that way anyway.

I suppose it's a minor wrinkle, but I wonder if we can get rid of it...

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> Changes in v3:
> - do not use the PENDING and ACTIVE state for HW interrupts;
> - unify the inflight and non-inflight code paths in
> vgic_vcpu_inject_irq.
> ---
>  xen/arch/arm/gic.c  |   89 +++++++++++++++++++++++++++++----------------------
>  xen/arch/arm/vgic.c |   33 ++++++++++---------
>  2 files changed, 68 insertions(+), 54 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 3f061cf..128d071 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
>  /* Maximum cpu interface per GIC */
>  #define NR_GIC_CPU_IF 8
>  
> +static void _gic_clear_lr(struct vcpu *v, int i);

Single underbar prefixes are reserved for the compiler.

gic_clear_one_lr would be an adequate name I think.

You could also have done this at the time you introduced gic_clear_lrs,
which would save me now asking: is the split into _gic_clear_lr pure
code motion or are there actual substantive changes in it?

Ian.

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq
> > while the first one is still active.
> > If the first irq is already pending (not active), just clear
> > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> > already visible by the guest.
> > If the irq has already been EOI'ed then just clear the GICH_LR right
> > away and move the interrupt to lr_pending so that it is going to be
> > reinjected by gic_restore_pending_irqs on return to guest.
> > 
> > If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_PENDING
> > and send an SGI. The target cpu is going to be interrupted and call
> > gic_clear_lrs, that is going to take the same actions.
> > 
> > Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq.
> > 
> > Do not call vgic_vcpu_inject_irq from gic_inject if
> > evtchn_upcall_pending is set. If we remove that call, we don't need to
> > special case evtchn_irq in vgic_vcpu_inject_irq anymore.
> 
> That's the consequence of removing it, but what is the rationale for why
> it is OK?

Because with this patch we are able to inject a second interrupt while
the first one is still in progress.


> > We also need to force the first injection of evtchn_irq (call
> > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > is already set by common code on vcpu creation.
> 
> This is because the common code expects that the guest is moving from
> sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but
> on ARM we start off that way anyway.
> 
> I suppose it's a minor wrinkle, but I wonder if we can get rid of it...

Do you mean getting rid of evtchn_upcall_pending being set at vcpu
creation? Wouldn't that cause possible undesirable side effects to
guests that came to rely on it somehow? It would be an ABI change.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > Changes in v3:
> > - do not use the PENDING and ACTIVE state for HW interrupts;
> > - unify the inflight and non-inflight code paths in
> > vgic_vcpu_inject_irq.
> > ---
> >  xen/arch/arm/gic.c  |   89 +++++++++++++++++++++++++++++----------------------
> >  xen/arch/arm/vgic.c |   33 ++++++++++---------
> >  2 files changed, 68 insertions(+), 54 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 3f061cf..128d071 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
> >  /* Maximum cpu interface per GIC */
> >  #define NR_GIC_CPU_IF 8
> >  
> > +static void _gic_clear_lr(struct vcpu *v, int i);
> 
> Single underbar prefixes are reserved for the compiler.
> 
> gic_clear_one_lr would be an adequate name I think.

OK


> You could also have done this at the time you introduced gic_clear_lrs,
> which would save me now asking: is the split into _gic_clear_lr pure
> code motion or are there actual substantive changes in it?

It is not pure code motion, the changes are listed in the first
paragraph of the commit message.

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

[Ian Campbell]

On Fri, 2014-03-21 at 16:46 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq
> > > while the first one is still active.
> > > If the first irq is already pending (not active), just clear
> > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> > > already visible by the guest.
> > > If the irq has already been EOI'ed then just clear the GICH_LR right
> > > away and move the interrupt to lr_pending so that it is going to be
> > > reinjected by gic_restore_pending_irqs on return to guest.
> > > 
> > > If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_PENDING
> > > and send an SGI. The target cpu is going to be interrupted and call
> > > gic_clear_lrs, that is going to take the same actions.
> > > 
> > > Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq.
> > > 
> > > Do not call vgic_vcpu_inject_irq from gic_inject if
> > > evtchn_upcall_pending is set. If we remove that call, we don't need to
> > > special case evtchn_irq in vgic_vcpu_inject_irq anymore.
> > 
> > That's the consequence of removing it, but what is the rationale for why
> > it is OK?
> 
> Because with this patch we are able to inject a second interrupt while
> the first one is still in progress.

IOW gic_inject of the evtchn just works even if the evtchn is already
pending?

Don't we then have a problem with a potentialy spurious evtchn upcall?

Event chn A raised
 -> IRQ inject
    -> GUest takes IRQ
        -> Guest enters evtchn handling loop
            -> handles A
Event chn B raised
 -> IRQ becomes pendign again
            -> handles B
         -> Finishes evtchn handling loop
         -> unmasks interrupt
    -> Guest takes anotheer IRQ
          -> Nothing to do (B already dealt with)

?

> > > We also need to force the first injection of evtchn_irq (call
> > > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > > is already set by common code on vcpu creation.
> > 
> > This is because the common code expects that the guest is moving from
> > sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but
> > on ARM we start off that way anyway.
> > 
> > I suppose it's a minor wrinkle, but I wonder if we can get rid of it...
> 
> Do you mean getting rid of evtchn_upcall_pending being set at vcpu
> creation? Wouldn't that cause possible undesirable side effects to
> guests that came to rely on it somehow? It would be an ABI change.

I mean precisely for the boot cpu when it is brought online, there isn't
much sense in immediately taking an interrupt when that cpu enables
them.

The reason for setting it right now is only for the case of a cpu moving
its vcpu info, to ensure it can't miss anything.

> 
> 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > 
> > > ---
> > > Changes in v3:
> > > - do not use the PENDING and ACTIVE state for HW interrupts;
> > > - unify the inflight and non-inflight code paths in
> > > vgic_vcpu_inject_irq.
> > > ---
> > >  xen/arch/arm/gic.c  |   89 +++++++++++++++++++++++++++++----------------------
> > >  xen/arch/arm/vgic.c |   33 ++++++++++---------
> > >  2 files changed, 68 insertions(+), 54 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > index 3f061cf..128d071 100644
> > > --- a/xen/arch/arm/gic.c
> > > +++ b/xen/arch/arm/gic.c
> > > @@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
> > >  /* Maximum cpu interface per GIC */
> > >  #define NR_GIC_CPU_IF 8
> > >  
> > > +static void _gic_clear_lr(struct vcpu *v, int i);
> > 
> > Single underbar prefixes are reserved for the compiler.
> > 
> > gic_clear_one_lr would be an adequate name I think.
> 
> OK
> 
> 
> > You could also have done this at the time you introduced gic_clear_lrs,
> > which would save me now asking: is the split into _gic_clear_lr pure
> > code motion or are there actual substantive changes in it?
> 
> It is not pure code motion, the changes are listed in the first
> paragraph of the commit message.

It's not at all clear which of those changes impact the code which has
moved.

Please can you do the code motion separately then or better yet just
introduce the code in the right place to start with so that the changes
here are just changes.

Ian.

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Fri, 2014-03-21 at 16:46 +0000, Stefano Stabellini wrote:
> > On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq
> > > > while the first one is still active.
> > > > If the first irq is already pending (not active), just clear
> > > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> > > > already visible by the guest.
> > > > If the irq has already been EOI'ed then just clear the GICH_LR right
> > > > away and move the interrupt to lr_pending so that it is going to be
> > > > reinjected by gic_restore_pending_irqs on return to guest.
> > > > 
> > > > If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_PENDING
> > > > and send an SGI. The target cpu is going to be interrupted and call
> > > > gic_clear_lrs, that is going to take the same actions.
> > > > 
> > > > Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq.
> > > > 
> > > > Do not call vgic_vcpu_inject_irq from gic_inject if
> > > > evtchn_upcall_pending is set. If we remove that call, we don't need to
> > > > special case evtchn_irq in vgic_vcpu_inject_irq anymore.
> > > 
> > > That's the consequence of removing it, but what is the rationale for why
> > > it is OK?
> > 
> > Because with this patch we are able to inject a second interrupt while
> > the first one is still in progress.
> 
> IOW gic_inject of the evtchn just works even if the evtchn is already
> pending?
>
> Don't we then have a problem with a potentialy spurious evtchn upcall?
> 
> Event chn A raised
>  -> IRQ inject
>     -> GUest takes IRQ
>         -> Guest enters evtchn handling loop
>             -> handles A
> Event chn B raised
>  -> IRQ becomes pendign again
>             -> handles B
>          -> Finishes evtchn handling loop
>          -> unmasks interrupt
>     -> Guest takes anotheer IRQ
>           -> Nothing to do (B already dealt with)
> 
> ?

No, because vcpu_mark_events_pending only calls vgic_vcpu_inject_irq if
evtchn_upcall_pending is not set.

We only inject a second evtchn_irq if the guest actually needs a second
notification.


> > > > We also need to force the first injection of evtchn_irq (call
> > > > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > > > is already set by common code on vcpu creation.
> > > 
> > > This is because the common code expects that the guest is moving from
> > > sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but
> > > on ARM we start off that way anyway.
> > > 
> > > I suppose it's a minor wrinkle, but I wonder if we can get rid of it...
> > 
> > Do you mean getting rid of evtchn_upcall_pending being set at vcpu
> > creation? Wouldn't that cause possible undesirable side effects to
> > guests that came to rely on it somehow? It would be an ABI change.
> 
> I mean precisely for the boot cpu when it is brought online, there isn't
> much sense in immediately taking an interrupt when that cpu enables
> them.
> 
> The reason for setting it right now is only for the case of a cpu moving
> its vcpu info, to ensure it can't miss anything.

What about secondary vcpus? Should we keep the spurious injection for
them?
In any case I agree with you that the current behaviour is not nice,
however I am worried about changing a guest visible interface like this
one, that would affect x86 guests too.


> > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > 
> > > > ---
> > > > Changes in v3:
> > > > - do not use the PENDING and ACTIVE state for HW interrupts;
> > > > - unify the inflight and non-inflight code paths in
> > > > vgic_vcpu_inject_irq.
> > > > ---
> > > >  xen/arch/arm/gic.c  |   89 +++++++++++++++++++++++++++++----------------------
> > > >  xen/arch/arm/vgic.c |   33 ++++++++++---------
> > > >  2 files changed, 68 insertions(+), 54 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > > index 3f061cf..128d071 100644
> > > > --- a/xen/arch/arm/gic.c
> > > > +++ b/xen/arch/arm/gic.c
> > > > @@ -67,6 +67,8 @@ static DEFINE_PER_CPU(u8, gic_cpu_id);
> > > >  /* Maximum cpu interface per GIC */
> > > >  #define NR_GIC_CPU_IF 8
> > > >  
> > > > +static void _gic_clear_lr(struct vcpu *v, int i);
> > > 
> > > Single underbar prefixes are reserved for the compiler.
> > > 
> > > gic_clear_one_lr would be an adequate name I think.
> > 
> > OK
> > 
> > 
> > > You could also have done this at the time you introduced gic_clear_lrs,
> > > which would save me now asking: is the split into _gic_clear_lr pure
> > > code motion or are there actual substantive changes in it?
> > 
> > It is not pure code motion, the changes are listed in the first
> > paragraph of the commit message.
> 
> It's not at all clear which of those changes impact the code which has
> moved.
> 
> Please can you do the code motion separately then or better yet just
> introduce the code in the right place to start with so that the changes
> here are just changes.

OK

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

[Ian Campbell]

On Mon, 2014-03-24 at 12:54 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > On Fri, 2014-03-21 at 16:46 +0000, Stefano Stabellini wrote:
> > > On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > > > Set GICH_LR_PENDING in the corresponding GICH_LR to inject a second irq
> > > > > while the first one is still active.
> > > > > If the first irq is already pending (not active), just clear
> > > > > GIC_IRQ_GUEST_PENDING because the irq has already been injected and is
> > > > > already visible by the guest.
> > > > > If the irq has already been EOI'ed then just clear the GICH_LR right
> > > > > away and move the interrupt to lr_pending so that it is going to be
> > > > > reinjected by gic_restore_pending_irqs on return to guest.
> > > > > 
> > > > > If the target cpu is not the current cpu, then set GIC_IRQ_GUEST_PENDING
> > > > > and send an SGI. The target cpu is going to be interrupted and call
> > > > > gic_clear_lrs, that is going to take the same actions.
> > > > > 
> > > > > Unify the inflight and non-inflight code paths in vgic_vcpu_inject_irq.
> > > > > 
> > > > > Do not call vgic_vcpu_inject_irq from gic_inject if
> > > > > evtchn_upcall_pending is set. If we remove that call, we don't need to
> > > > > special case evtchn_irq in vgic_vcpu_inject_irq anymore.
> > > > 
> > > > That's the consequence of removing it, but what is the rationale for why
> > > > it is OK?
> > > 
> > > Because with this patch we are able to inject a second interrupt while
> > > the first one is still in progress.
> > 
> > IOW gic_inject of the evtchn just works even if the evtchn is already
> > pending?
> >
> > Don't we then have a problem with a potentialy spurious evtchn upcall?
> > 
> > Event chn A raised
> >  -> IRQ inject
> >     -> GUest takes IRQ
> >         -> Guest enters evtchn handling loop
> >             -> handles A
> > Event chn B raised
> >  -> IRQ becomes pendign again
> >             -> handles B
> >          -> Finishes evtchn handling loop
> >          -> unmasks interrupt
> >     -> Guest takes anotheer IRQ
> >           -> Nothing to do (B already dealt with)
> > 
> > ?
> 
> No, because vcpu_mark_events_pending only calls vgic_vcpu_inject_irq if
> evtchn_upcall_pending is not set.
> 
> We only inject a second evtchn_irq if the guest actually needs a second
> notification.

OK, thanks for the explanation.

> > > > > We also need to force the first injection of evtchn_irq (call
> > > > > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > > > > is already set by common code on vcpu creation.
> > > > 
> > > > This is because the common code expects that the guest is moving from
> > > > sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but
> > > > on ARM we start off that way anyway.
> > > > 
> > > > I suppose it's a minor wrinkle, but I wonder if we can get rid of it...
> > > 
> > > Do you mean getting rid of evtchn_upcall_pending being set at vcpu
> > > creation? Wouldn't that cause possible undesirable side effects to
> > > guests that came to rely on it somehow? It would be an ABI change.
> > 
> > I mean precisely for the boot cpu when it is brought online, there isn't
> > much sense in immediately taking an interrupt when that cpu enables
> > them.
> > 
> > The reason for setting it right now is only for the case of a cpu moving
> > its vcpu info, to ensure it can't miss anything.
> 
> What about secondary vcpus? Should we keep the spurious injection for
> them?

Not sure. No?

> In any case I agree with you that the current behaviour is not nice,
> however I am worried about changing a guest visible interface like this
> one, that would affect x86 guests too.

Oh, I certainly wouldn't change this for x86! Or maybe I would change it
but only for cpus which are not online at the time when the init happens
(which is effectively the difference between the x86 and arm cases)

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

[Stefano Stabellini]

On Mon, 24 Mar 2014, Ian Campbell wrote:
> > > > > > We also need to force the first injection of evtchn_irq (call
> > > > > > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > > > > > is already set by common code on vcpu creation.
> > > > > 
> > > > > This is because the common code expects that the guest is moving from
> > > > > sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but
> > > > > on ARM we start off that way anyway.
> > > > > 
> > > > > I suppose it's a minor wrinkle, but I wonder if we can get rid of it...
> > > > 
> > > > Do you mean getting rid of evtchn_upcall_pending being set at vcpu
> > > > creation? Wouldn't that cause possible undesirable side effects to
> > > > guests that came to rely on it somehow? It would be an ABI change.
> > > 
> > > I mean precisely for the boot cpu when it is brought online, there isn't
> > > much sense in immediately taking an interrupt when that cpu enables
> > > them.
> > > 
> > > The reason for setting it right now is only for the case of a cpu moving
> > > its vcpu info, to ensure it can't miss anything.
> > 
> > What about secondary vcpus? Should we keep the spurious injection for
> > them?
> 
> Not sure. No?
> 
> > In any case I agree with you that the current behaviour is not nice,
> > however I am worried about changing a guest visible interface like this
> > one, that would affect x86 guests too.
> 
> Oh, I certainly wouldn't change this for x86! Or maybe I would change it
> but only for cpus which are not online at the time when the init happens
> (which is effectively the difference between the x86 and arm cases)
 
Today on ARM and x86 PV on HVM VCPUOP_register_vcpu_info is called by
each vcpu independently when is coming online, so secondary cpus would
be already online at the time of the hypercall.

On x86 PV VCPUOP_register_vcpu_info is called in a loop by vcpu0 for all
the possible vcpus before smp bringup.

Either way I don't think we can easily change this behaviour without
affecting x86 guests.

Alternatively of course this (ugly) change in Xen would work:

diff --git a/xen/common/domain.c b/xen/common/domain.c
index ad8a1b6..bd81f43 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -893,7 +893,9 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
     void *mapping;
     vcpu_info_t *new_info;
     struct page_info *page;
+#ifdef CONFIG_X86
     int i;
+#endif
 
     if ( offset > (PAGE_SIZE - sizeof(vcpu_info_t)) )
         return -EINVAL;
@@ -942,6 +944,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
     /* Set new vcpu_info pointer /before/ setting pending flags. */
     smp_wmb();
 
+#ifdef CONFIG_X86
     /*
      * Mark everything as being pending just to make sure nothing gets
      * lost.  The domain will get a spurious event, but it can cope.
@@ -949,7 +952,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
     vcpu_info(v, evtchn_upcall_pending) = 1;
     for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
         set_bit(i, &vcpu_info(v, evtchn_pending_sel));
-
+#endif
     return 0;
 }

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

[Ian Campbell]

On Mon, 2014-03-24 at 16:41 +0000, Stefano Stabellini wrote:
> On Mon, 24 Mar 2014, Ian Campbell wrote:
> > > > > > > We also need to force the first injection of evtchn_irq (call
> > > > > > > gic_vcpu_inject_irq) from vgic_enable_irqs because evtchn_upcall_pending
> > > > > > > is already set by common code on vcpu creation.
> > > > > > 
> > > > > > This is because the common code expects that the guest is moving from
> > > > > > sharedinfo based vcpu info using VCPUOP_register_vcpu_info on x86, but
> > > > > > on ARM we start off that way anyway.
> > > > > > 
> > > > > > I suppose it's a minor wrinkle, but I wonder if we can get rid of it...
> > > > > 
> > > > > Do you mean getting rid of evtchn_upcall_pending being set at vcpu
> > > > > creation? Wouldn't that cause possible undesirable side effects to
> > > > > guests that came to rely on it somehow? It would be an ABI change.
> > > > 
> > > > I mean precisely for the boot cpu when it is brought online, there isn't
> > > > much sense in immediately taking an interrupt when that cpu enables
> > > > them.
> > > > 
> > > > The reason for setting it right now is only for the case of a cpu moving
> > > > its vcpu info, to ensure it can't miss anything.
> > > 
> > > What about secondary vcpus? Should we keep the spurious injection for
> > > them?
> > 
> > Not sure. No?
> > 
> > > In any case I agree with you that the current behaviour is not nice,
> > > however I am worried about changing a guest visible interface like this
> > > one, that would affect x86 guests too.
> > 
> > Oh, I certainly wouldn't change this for x86! Or maybe I would change it
> > but only for cpus which are not online at the time when the init happens
> > (which is effectively the difference between the x86 and arm cases)
>  
> Today on ARM and x86 PV on HVM VCPUOP_register_vcpu_info is called by
> each vcpu independently when is coming online, so secondary cpus would
> be already online at the time of the hypercall.
> 
> On x86 PV VCPUOP_register_vcpu_info is called in a loop by vcpu0 for all
> the possible vcpus before smp bringup.
> 
> Either way I don't think we can easily change this behaviour without
> affecting x86 guests.

OK, it was worth considering but I think you are right that it isn't
worth it. Hopefully any sane OS is going to be prepared to handle
interrupts before it enables them!

> Alternatively of course this (ugly) change in Xen would work:

Yeah, lets not ;-)

> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index ad8a1b6..bd81f43 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -893,7 +893,9 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
>      void *mapping;
>      vcpu_info_t *new_info;
>      struct page_info *page;
> +#ifdef CONFIG_X86
>      int i;
> +#endif
>  
>      if ( offset > (PAGE_SIZE - sizeof(vcpu_info_t)) )
>          return -EINVAL;
> @@ -942,6 +944,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
>      /* Set new vcpu_info pointer /before/ setting pending flags. */
>      smp_wmb();
>  
> +#ifdef CONFIG_X86
>      /*
>       * Mark everything as being pending just to make sure nothing gets
>       * lost.  The domain will get a spurious event, but it can cope.
> @@ -949,7 +952,7 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
>      vcpu_info(v, evtchn_upcall_pending) = 1;
>      for ( i = 0; i < BITS_PER_EVTCHN_WORD(d); i++ )
>          set_bit(i, &vcpu_info(v, evtchn_pending_sel));
> -
> +#endif
>      return 0;
>  }

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

[Stefano Stabellini]

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

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

---

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 128d071..bc9d66d 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -667,19 +667,15 @@ 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);
-    unsigned long flags;
 
-    spin_lock_irqsave(&gic.lock, flags);
     if ( !list_empty(&p->lr_queue) )
         list_del_init(&p->lr_queue);
-    spin_unlock_irqrestore(&gic.lock, flags);
 }
 
 void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
                          unsigned int priority)
 {
     int i;
-    unsigned long flags;
     struct pending_irq *n = irq_to_pending(v, irq);
 
     if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status))
@@ -689,23 +685,17 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
         return;
     }
 
-    spin_lock_irqsave(&gic.lock, flags);
-
     if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
     {
         i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
         if (i < nr_lrs) {
             set_bit(i, &this_cpu(lr_mask));
             gic_set_lr(i, irq_to_pending(v, irq), GICH_LR_PENDING);
-            goto out;
+            return;
         }
     }
 
     gic_add_to_lr_pending(v, irq_to_pending(v, irq));
-
-out:
-    spin_unlock_irqrestore(&gic.lock, flags);
-    return;
 }
 
 static void _gic_clear_lr(struct vcpu *v, int i)
@@ -727,8 +717,6 @@ static void _gic_clear_lr(struct vcpu *v, int i)
     } else if ( lr & GICH_LR_PENDING ) {
         clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
     } else {
-        spin_lock(&gic.lock);
-
         GICH[GICH_LR + i] = 0;
         clear_bit(i, &this_cpu(lr_mask));
 
@@ -742,8 +730,6 @@ static void _gic_clear_lr(struct vcpu *v, int i)
             gic_raise_guest_irq(v, irq, p->priority);
         } else
             list_del_init(&p->inflight);
-
-        spin_unlock(&gic.lock);
     }
 }
 
@@ -773,11 +759,11 @@ static void gic_restore_pending_irqs(struct vcpu *v)
         i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
         if ( i >= nr_lrs ) return;
 
-        spin_lock_irqsave(&gic.lock, flags);
+        spin_lock_irqsave(&v->arch.vgic.lock, flags);
         gic_set_lr(i, p, GICH_LR_PENDING);
         list_del_init(&p->lr_queue);
         set_bit(i, &this_cpu(lr_mask));
-        spin_unlock_irqrestore(&gic.lock, flags);
+        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
     }
 
 }
@@ -785,13 +771,10 @@ static void gic_restore_pending_irqs(struct vcpu *v)
 void gic_clear_pending_irqs(struct vcpu *v)
 {
     struct pending_irq *p, *t;
-    unsigned long flags;
 
-    spin_lock_irqsave(&gic.lock, flags);
     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)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index dc3a75f..bd15be7 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -393,8 +393,13 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
              vcpu_info(current, evtchn_upcall_pending) &&
              list_empty(&p->inflight) )
             vgic_vcpu_inject_irq(v, irq);
-        else if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
-            gic_raise_guest_irq(v, irq, p->priority);
+        else {
+            unsigned long flags;
+            spin_lock_irqsave(&v->arch.vgic.lock, flags);
+            if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
+                gic_raise_guest_irq(v, irq, p->priority);
+            spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+        }
         if ( p->desc != NULL )
             p->desc->handler->enable(p->desc);
         i++;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index ea89057..517128e 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -66,7 +66,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

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

[Ian Campbell]

On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> GICH is banked, protect accesses by disabling interrupts.
> Protect lr_queue accesses with the vgic.lock only.

Does this rely on using the irq disabling spinlock_irq variants for this
lock to also protect GICH?

I don't see any actual calls to irq_disable so I suppose such things are
always nested inside holding a vgic lock.

> gic.lock only protects accesses to GICD now.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> ---
> 
> Changes in v4:
> - improved in code comments.
> ---
>  xen/arch/arm/gic.c           |   23 +++--------------------
>  xen/arch/arm/vgic.c          |    9 +++++++--
>  xen/include/asm-arm/domain.h |    5 ++++-
>  3 files changed, 14 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 128d071..bc9d66d 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -667,19 +667,15 @@ 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);
> -    unsigned long flags;
>  
> -    spin_lock_irqsave(&gic.lock, flags);
>      if ( !list_empty(&p->lr_queue) )
>          list_del_init(&p->lr_queue);

Where is vgic.lock held here? I looked back in the callchain and didn't
see it.

> -    spin_unlock_irqrestore(&gic.lock, flags);
>  }
>  
>  void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
>                           unsigned int priority)
>  {
>      int i;
> -    unsigned long flags;
>      struct pending_irq *n = irq_to_pending(v, irq);
>  
>      if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status))
> @@ -689,23 +685,17 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
>          return;
>      }
>  
> -    spin_lock_irqsave(&gic.lock, flags);

This function requires the vgic lock to be held when it is called.

This locking (and implicit interrupt flag based locking) is getting
pretty complex. I think it would be a good idea to start documenting
this sort of requirement for this code in a comment at the top of the
function, and perhaps with an assert in the entry path.

Likewise for functions which require interrupts to be disabled with a
comment and an assert.

Ian.

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

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > GICH is banked, protect accesses by disabling interrupts.
> > Protect lr_queue accesses with the vgic.lock only.
> 
> Does this rely on using the irq disabling spinlock_irq variants for this
> lock to also protect GICH?

Yes, specifically in gic_set_lr and gic_clear_one_lr.


> I don't see any actual calls to irq_disable so I suppose such things are
> always nested inside holding a vgic lock.

Yes, most of the times. However GICH changes are also made in
gic_save_state, gic_restore_state, gic_hyp_init, gic_hyp_disable and
gic_inject, where we can be sure that interrupts are disabled for other
reasons.


> > gic.lock only protects accesses to GICD now.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > ---
> > 
> > Changes in v4:
> > - improved in code comments.
> > ---
> >  xen/arch/arm/gic.c           |   23 +++--------------------
> >  xen/arch/arm/vgic.c          |    9 +++++++--
> >  xen/include/asm-arm/domain.h |    5 ++++-
> >  3 files changed, 14 insertions(+), 23 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 128d071..bc9d66d 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -667,19 +667,15 @@ 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);
> > -    unsigned long flags;
> >  
> > -    spin_lock_irqsave(&gic.lock, flags);
> >      if ( !list_empty(&p->lr_queue) )
> >          list_del_init(&p->lr_queue);
> 
> Where is vgic.lock held here? I looked back in the callchain and didn't
> see it.

Well spotted! This is a mistake! I'll fix it.


> > -    spin_unlock_irqrestore(&gic.lock, flags);
> >  }
> >  
> >  void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
> >                           unsigned int priority)
> >  {
> >      int i;
> > -    unsigned long flags;
> >      struct pending_irq *n = irq_to_pending(v, irq);
> >  
> >      if ( test_bit(GIC_IRQ_GUEST_VISIBLE, &n->status))
> > @@ -689,23 +685,17 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int irq,
> >          return;
> >      }
> >  
> > -    spin_lock_irqsave(&gic.lock, flags);
> 
> This function requires the vgic lock to be held when it is called.
> 
> This locking (and implicit interrupt flag based locking) is getting
> pretty complex. I think it would be a good idea to start documenting
> this sort of requirement for this code in a comment at the top of the
> function, and perhaps with an assert in the entry path.
> 
> Likewise for functions which require interrupts to be disabled with a
> comment and an assert.
 
Good idea, I'll make the changes.

[PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini]

gic_events_need_delivery should only return positive if an outstanding
pending irq has an higher priority than the currently active irq and the
priority mask.
Rewrite the function by going through the priority ordered inflight and
lr_queue lists.

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

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

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index bc9d66d..533964e 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -709,6 +709,7 @@ static void _gic_clear_lr(struct vcpu *v, int i)
     p = irq_to_pending(v, irq);
     if ( lr & GICH_LR_ACTIVE )
     {
+        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
         /* HW interrupts cannot be ACTIVE and PENDING */
         if ( p->desc == NULL &&
              test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
@@ -723,6 +724,7 @@ static void _gic_clear_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 = nr_lrs;
         if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
                 test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
@@ -750,22 +752,47 @@ void gic_clear_lrs(struct vcpu *v)
 
 static void gic_restore_pending_irqs(struct vcpu *v)
 {
-    int i;
-    struct pending_irq *p, *t;
+    int i = 0, lrs = nr_lrs;
+    struct pending_irq *p, *t, *p_r;
     unsigned long flags;
 
+    if ( list_empty(&v->arch.vgic.lr_pending) )
+        return;
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    p_r = list_entry(v->arch.vgic.inflight_irqs.prev,
+                         typeof(*p_r), inflight);
     list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
     {
         i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
-        if ( i >= nr_lrs ) return;
+        if ( i >= nr_lrs )
+        {
+            while ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) ||
+                    test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
+            {
+                p_r = list_entry(p_r->inflight.prev, typeof(*p_r), inflight);
+                if ( &p_r->inflight == p->inflight.next )
+                    goto out;
+            }
+            i = p_r->lr;
+            p_r->lr = nr_lrs;
+            set_bit(GIC_IRQ_GUEST_PENDING, &p_r->status);
+            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
+            gic_add_to_lr_pending(v, p_r);
+        }
 
-        spin_lock_irqsave(&v->arch.vgic.lock, flags);
         gic_set_lr(i, p, GICH_LR_PENDING);
         list_del_init(&p->lr_queue);
         set_bit(i, &this_cpu(lr_mask));
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+
+        lrs--;
+        if ( lrs == 0 )
+            break;
     }
 
+out:
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
 }
 
 void gic_clear_pending_irqs(struct vcpu *v)
@@ -779,8 +806,38 @@ void gic_clear_pending_irqs(struct vcpu *v)
 
 int gic_events_need_delivery(void)
 {
-    return (!list_empty(&current->arch.vgic.lr_pending) ||
-            this_cpu(lr_mask));
+    int mask_priority, lrs = nr_lrs;
+    int max_priority = 0xff, active_priority = 0xff;
+    struct vcpu *v = current;
+    struct pending_irq *p;
+    unsigned long flags;
+
+    mask_priority = (GICH[GICH_VMCR] >> GICH_VMCR_PRIORITY_SHIFT) & GICH_VMCR_PRIORITY_MASK;
+
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
+    {
+        if ( test_bit(GIC_IRQ_GUEST_ACTIVE, &p->status) )
+        {
+            if ( p->priority < active_priority )
+                active_priority = p->priority;
+        } else if ( test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) ) {
+            if ( p->priority < max_priority )
+                max_priority = p->priority;
+        }
+        lrs--;
+        if ( lrs == 0 )
+            break;
+    }
+
+    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+
+    if ( max_priority < active_priority &&
+         (max_priority >> 3) < mask_priority )
+        return 1;
+    else
+        return 0;
 }
 
 void gic_inject(void)
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 517128e..fe20fd2 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_PENDING  0
-#define GIC_IRQ_GUEST_VISIBLE  1
-#define GIC_IRQ_GUEST_ENABLED  2
+#define GIC_IRQ_GUEST_ACTIVE   1
+#define GIC_IRQ_GUEST_VISIBLE  2
+#define GIC_IRQ_GUEST_ENABLED  3
     unsigned long status;
     uint8_t lr;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical irq */
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 5a9dc77..5d8f7f1 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -129,6 +129,9 @@
 #define GICH_LR_CPUID_SHIFT     9
 #define GICH_VTR_NRLRGS         0x3f
 
+#define GICH_VMCR_PRIORITY_MASK   0x1f
+#define GICH_VMCR_PRIORITY_SHIFT  27
+
 /*
  * The minimum GICC_BPR is required to be in the range 0-3. We set
  * GICC_BPR to 0 but we must expect that it might be 3. This means we
-- 
1.7.10.4

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Julien Grall]

Hi Stefano,

On 03/19/2014 12:32 PM, Stefano Stabellini wrote:
> gic_events_need_delivery should only return positive if an outstanding
> pending irq has an higher priority than the currently active irq and the
> priority mask.
> 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>

I'd like a comment somewhere in the code to explain the priority is only
for the guest and doesn't reflect the priority set on the host.

So people won't think we are handling correctly priority at the host level.

Regards,

-- 
Julien Grall

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Ian Campbell]

On Wed, 2014-03-19 at 14:00 +0000, Julien Grall wrote:
> So people won't think we are handling correctly priority at the host level.

I think we mostly handle host level interrupt priorities OK, don't we?

Ian.

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Julien Grall]

On 03/20/2014 11:03 AM, Ian Campbell wrote:
> On Wed, 2014-03-19 at 14:00 +0000, Julien Grall wrote:
>> So people won't think we are handling correctly priority at the host level.
> 
> I think we mostly handle host level interrupt priorities OK, don't we?

We handle host level interrupt priorities but we don't propagate the
guest priorities.

Regards,

-- 
Julien Grall

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Ian Campbell]

On Thu, 2014-03-20 at 15:10 +0000, Julien Grall wrote:
> On 03/20/2014 11:03 AM, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 14:00 +0000, Julien Grall wrote:
> >> So people won't think we are handling correctly priority at the host level.
> > 
> > I think we mostly handle host level interrupt priorities OK, don't we?
> 
> We handle host level interrupt priorities but we don't propagate the
> guest priorities.

Right, that is indeed a missing piece of the puzzle.

Ian.

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Ian Campbell]

On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> gic_events_need_delivery should only return positive if an outstanding
> pending irq has an higher priority than the currently active irq and the
> priority mask.
> 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 v4:
> - in gic_events_need_delivery go through inflight_irqs and only consider
> enabled irqs.
> ---
>  xen/arch/arm/gic.c           |   71 +++++++++++++++++++++++++++++++++++++-----
>  xen/include/asm-arm/domain.h |    5 +--
>  xen/include/asm-arm/gic.h    |    3 ++
>  3 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index bc9d66d..533964e 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -709,6 +709,7 @@ static void _gic_clear_lr(struct vcpu *v, int i)
>      p = irq_to_pending(v, irq);
>      if ( lr & GICH_LR_ACTIVE )
>      {
> +        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
>          /* HW interrupts cannot be ACTIVE and PENDING */
>          if ( p->desc == NULL &&
>               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> @@ -723,6 +724,7 @@ static void _gic_clear_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 = nr_lrs;
>          if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
>                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> @@ -750,22 +752,47 @@ void gic_clear_lrs(struct vcpu *v)
>  
>  static void gic_restore_pending_irqs(struct vcpu *v)
>  {
> -    int i;
> -    struct pending_irq *p, *t;
> +    int i = 0, lrs = nr_lrs;
> +    struct pending_irq *p, *t, *p_r;
>      unsigned long flags;
>  
> +    if ( list_empty(&v->arch.vgic.lr_pending) )
> +        return;
> +
> +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> +
> +    p_r = list_entry(v->arch.vgic.inflight_irqs.prev,
> +                         typeof(*p_r), inflight);

Is this getting the tail of the list or something else?

>      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
>      {
>          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
> -        if ( i >= nr_lrs ) return;
> +        if ( i >= nr_lrs )
> +        {
> +            while ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) ||
> +                    test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> +            {
> +                p_r = list_entry(p_r->inflight.prev, typeof(*p_r), inflight);

Oh, maybe this (and the thing above) is an open coded list_for_each_prev
or one of its variants? e.g. list_for_each_entry_reverse?

> +                if ( &p_r->inflight == p->inflight.next )
> +                    goto out;
> +            }
> +            i = p_r->lr;
> +            p_r->lr = nr_lrs;
> +            set_bit(GIC_IRQ_GUEST_PENDING, &p_r->status);
> +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
> +            gic_add_to_lr_pending(v, p_r);
> +        }
>  
> -        spin_lock_irqsave(&v->arch.vgic.lock, flags);
>          gic_set_lr(i, p, GICH_LR_PENDING);
>          list_del_init(&p->lr_queue);
>          set_bit(i, &this_cpu(lr_mask));
> -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +
> +        lrs--;
> +        if ( lrs == 0 )
> +            break;
>      }
>  
> +out:
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>  }
>  
>  void gic_clear_pending_irqs(struct vcpu *v)

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini]

On Fri, 21 Mar 2014, Ian Campbell wrote:
> On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > gic_events_need_delivery should only return positive if an outstanding
> > pending irq has an higher priority than the currently active irq and the
> > priority mask.
> > 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 v4:
> > - in gic_events_need_delivery go through inflight_irqs and only consider
> > enabled irqs.
> > ---
> >  xen/arch/arm/gic.c           |   71 +++++++++++++++++++++++++++++++++++++-----
> >  xen/include/asm-arm/domain.h |    5 +--
> >  xen/include/asm-arm/gic.h    |    3 ++
> >  3 files changed, 70 insertions(+), 9 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index bc9d66d..533964e 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -709,6 +709,7 @@ static void _gic_clear_lr(struct vcpu *v, int i)
> >      p = irq_to_pending(v, irq);
> >      if ( lr & GICH_LR_ACTIVE )
> >      {
> > +        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> >          /* HW interrupts cannot be ACTIVE and PENDING */
> >          if ( p->desc == NULL &&
> >               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > @@ -723,6 +724,7 @@ static void _gic_clear_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 = nr_lrs;
> >          if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > @@ -750,22 +752,47 @@ void gic_clear_lrs(struct vcpu *v)
> >  
> >  static void gic_restore_pending_irqs(struct vcpu *v)
> >  {
> > -    int i;
> > -    struct pending_irq *p, *t;
> > +    int i = 0, lrs = nr_lrs;
> > +    struct pending_irq *p, *t, *p_r;
> >      unsigned long flags;
> >  
> > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > +        return;
> > +
> > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > +
> > +    p_r = list_entry(v->arch.vgic.inflight_irqs.prev,
> > +                         typeof(*p_r), inflight);
> 
> Is this getting the tail of the list or something else?
> 
> >      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> >      {
> >          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
> > -        if ( i >= nr_lrs ) return;
> > +        if ( i >= nr_lrs )
> > +        {
> > +            while ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) ||
> > +                    test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> > +            {
> > +                p_r = list_entry(p_r->inflight.prev, typeof(*p_r), inflight);
> 
> Oh, maybe this (and the thing above) is an open coded list_for_each_prev
> or one of its variants? e.g. list_for_each_entry_reverse?

Yes, it is a list_for_each_entry_reverse that starts from the current
p_r and stops when it finds the first entry that is
GIC_IRQ_GUEST_VISIBLE and GIC_IRQ_GUEST_ACTIVE.

It can also stop if the next entry that would be found going through
inflight in reverse is the same that we are evaluating going through
lr_pending in regular order.


> > +                if ( &p_r->inflight == p->inflight.next )
> > +                    goto out;
> > +            }
> > +            i = p_r->lr;
> > +            p_r->lr = nr_lrs;
> > +            set_bit(GIC_IRQ_GUEST_PENDING, &p_r->status);
> > +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status);
> > +            gic_add_to_lr_pending(v, p_r);
> > +        }
> >  
> > -        spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >          gic_set_lr(i, p, GICH_LR_PENDING);
> >          list_del_init(&p->lr_queue);
> >          set_bit(i, &this_cpu(lr_mask));
> > -        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +
> > +        lrs--;
> > +        if ( lrs == 0 )
> > +            break;
> >      }
> >  
> > +out:
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >  }
> >  
> >  void gic_clear_pending_irqs(struct vcpu *v)
> 
> 

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Ian Campbell]

On Mon, 2014-03-24 at 12:00 +0000, Stefano Stabellini wrote:
> On Fri, 21 Mar 2014, Ian Campbell wrote:
> > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > gic_events_need_delivery should only return positive if an outstanding
> > > pending irq has an higher priority than the currently active irq and the
> > > priority mask.
> > > 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 v4:
> > > - in gic_events_need_delivery go through inflight_irqs and only consider
> > > enabled irqs.
> > > ---
> > >  xen/arch/arm/gic.c           |   71 +++++++++++++++++++++++++++++++++++++-----
> > >  xen/include/asm-arm/domain.h |    5 +--
> > >  xen/include/asm-arm/gic.h    |    3 ++
> > >  3 files changed, 70 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > index bc9d66d..533964e 100644
> > > --- a/xen/arch/arm/gic.c
> > > +++ b/xen/arch/arm/gic.c
> > > @@ -709,6 +709,7 @@ static void _gic_clear_lr(struct vcpu *v, int i)
> > >      p = irq_to_pending(v, irq);
> > >      if ( lr & GICH_LR_ACTIVE )
> > >      {
> > > +        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> > >          /* HW interrupts cannot be ACTIVE and PENDING */
> > >          if ( p->desc == NULL &&
> > >               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > > @@ -723,6 +724,7 @@ static void _gic_clear_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 = nr_lrs;
> > >          if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> > >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > > @@ -750,22 +752,47 @@ void gic_clear_lrs(struct vcpu *v)
> > >  
> > >  static void gic_restore_pending_irqs(struct vcpu *v)
> > >  {
> > > -    int i;
> > > -    struct pending_irq *p, *t;
> > > +    int i = 0, lrs = nr_lrs;
> > > +    struct pending_irq *p, *t, *p_r;
> > >      unsigned long flags;
> > >  
> > > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > > +        return;
> > > +
> > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > +
> > > +    p_r = list_entry(v->arch.vgic.inflight_irqs.prev,
> > > +                         typeof(*p_r), inflight);
> > 
> > Is this getting the tail of the list or something else?
> > 
> > >      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> > >      {
> > >          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
> > > -        if ( i >= nr_lrs ) return;
> > > +        if ( i >= nr_lrs )
> > > +        {
> > > +            while ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) ||
> > > +                    test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> > > +            {
> > > +                p_r = list_entry(p_r->inflight.prev, typeof(*p_r), inflight);
> > 
> > Oh, maybe this (and the thing above) is an open coded list_for_each_prev
> > or one of its variants? e.g. list_for_each_entry_reverse?
> 
> Yes, it is a list_for_each_entry_reverse that starts from the current
> p_r and stops when it finds the first entry that is
> GIC_IRQ_GUEST_VISIBLE and GIC_IRQ_GUEST_ACTIVE.

I think this can/should be recast in terms of the regular macro then.

> It can also stop if the next entry that would be found going through
> inflight in reverse is the same that we are evaluating going through
> lr_pending in regular order.

I think it should also be possible to incorporate this behaviour.

But is it worth going backwards? I'd have though that for a long list of
inflight interrupts the normal case would be a smallish number of
VISIBLE+ACTIVE interrupts at the head and a potentially larger tail up
to nr_lrs. So you would be better off searching in normal order.

Ian.

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini]

On Mon, 24 Mar 2014, Ian Campbell wrote:
> On Mon, 2014-03-24 at 12:00 +0000, Stefano Stabellini wrote:
> > On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > > gic_events_need_delivery should only return positive if an outstanding
> > > > pending irq has an higher priority than the currently active irq and the
> > > > priority mask.
> > > > 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 v4:
> > > > - in gic_events_need_delivery go through inflight_irqs and only consider
> > > > enabled irqs.
> > > > ---
> > > >  xen/arch/arm/gic.c           |   71 +++++++++++++++++++++++++++++++++++++-----
> > > >  xen/include/asm-arm/domain.h |    5 +--
> > > >  xen/include/asm-arm/gic.h    |    3 ++
> > > >  3 files changed, 70 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > > index bc9d66d..533964e 100644
> > > > --- a/xen/arch/arm/gic.c
> > > > +++ b/xen/arch/arm/gic.c
> > > > @@ -709,6 +709,7 @@ static void _gic_clear_lr(struct vcpu *v, int i)
> > > >      p = irq_to_pending(v, irq);
> > > >      if ( lr & GICH_LR_ACTIVE )
> > > >      {
> > > > +        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> > > >          /* HW interrupts cannot be ACTIVE and PENDING */
> > > >          if ( p->desc == NULL &&
> > > >               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > > > @@ -723,6 +724,7 @@ static void _gic_clear_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 = nr_lrs;
> > > >          if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> > > >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > > > @@ -750,22 +752,47 @@ void gic_clear_lrs(struct vcpu *v)
> > > >  
> > > >  static void gic_restore_pending_irqs(struct vcpu *v)
> > > >  {
> > > > -    int i;
> > > > -    struct pending_irq *p, *t;
> > > > +    int i = 0, lrs = nr_lrs;
> > > > +    struct pending_irq *p, *t, *p_r;
> > > >      unsigned long flags;
> > > >  
> > > > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > > > +        return;
> > > > +
> > > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > > +
> > > > +    p_r = list_entry(v->arch.vgic.inflight_irqs.prev,
> > > > +                         typeof(*p_r), inflight);
> > > 
> > > Is this getting the tail of the list or something else?
> > > 
> > > >      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> > > >      {
> > > >          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
> > > > -        if ( i >= nr_lrs ) return;
> > > > +        if ( i >= nr_lrs )
> > > > +        {
> > > > +            while ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) ||
> > > > +                    test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> > > > +            {
> > > > +                p_r = list_entry(p_r->inflight.prev, typeof(*p_r), inflight);
> > > 
> > > Oh, maybe this (and the thing above) is an open coded list_for_each_prev
> > > or one of its variants? e.g. list_for_each_entry_reverse?
> > 
> > Yes, it is a list_for_each_entry_reverse that starts from the current
> > p_r and stops when it finds the first entry that is
> > GIC_IRQ_GUEST_VISIBLE and GIC_IRQ_GUEST_ACTIVE.
> 
> I think this can/should be recast in terms of the regular macro then.

OK.
We'll need to introduce list_for_each_entry_continue_reverse.

> > It can also stop if the next entry that would be found going through
> > inflight in reverse is the same that we are evaluating going through
> > lr_pending in regular order.
> 
> I think it should also be possible to incorporate this behaviour.
> 
> But is it worth going backwards? I'd have though that for a long list of
> inflight interrupts the normal case would be a smallish number of
> VISIBLE+ACTIVE interrupts at the head and a potentially larger tail up
> to nr_lrs. So you would be better off searching in normal order.

I think it is worth going backward to get the lowest priority interrupt
currently being injected. Inflight is also ordered by priority.

Re: [PATCH v4 10/10] xen/arm: gic_events_need_delivery and irq priorities

[Stefano Stabellini]

On Mon, 24 Mar 2014, Stefano Stabellini wrote:
> On Mon, 24 Mar 2014, Ian Campbell wrote:
> > On Mon, 2014-03-24 at 12:00 +0000, Stefano Stabellini wrote:
> > > On Fri, 21 Mar 2014, Ian Campbell wrote:
> > > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote:
> > > > > gic_events_need_delivery should only return positive if an outstanding
> > > > > pending irq has an higher priority than the currently active irq and the
> > > > > priority mask.
> > > > > 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 v4:
> > > > > - in gic_events_need_delivery go through inflight_irqs and only consider
> > > > > enabled irqs.
> > > > > ---
> > > > >  xen/arch/arm/gic.c           |   71 +++++++++++++++++++++++++++++++++++++-----
> > > > >  xen/include/asm-arm/domain.h |    5 +--
> > > > >  xen/include/asm-arm/gic.h    |    3 ++
> > > > >  3 files changed, 70 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > > > index bc9d66d..533964e 100644
> > > > > --- a/xen/arch/arm/gic.c
> > > > > +++ b/xen/arch/arm/gic.c
> > > > > @@ -709,6 +709,7 @@ static void _gic_clear_lr(struct vcpu *v, int i)
> > > > >      p = irq_to_pending(v, irq);
> > > > >      if ( lr & GICH_LR_ACTIVE )
> > > > >      {
> > > > > +        set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
> > > > >          /* HW interrupts cannot be ACTIVE and PENDING */
> > > > >          if ( p->desc == NULL &&
> > > > >               test_bit(GIC_IRQ_GUEST_ENABLED, &p->status) &&
> > > > > @@ -723,6 +724,7 @@ static void _gic_clear_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 = nr_lrs;
> > > > >          if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> > > > >                  test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > > > > @@ -750,22 +752,47 @@ void gic_clear_lrs(struct vcpu *v)
> > > > >  
> > > > >  static void gic_restore_pending_irqs(struct vcpu *v)
> > > > >  {
> > > > > -    int i;
> > > > > -    struct pending_irq *p, *t;
> > > > > +    int i = 0, lrs = nr_lrs;
> > > > > +    struct pending_irq *p, *t, *p_r;
> > > > >      unsigned long flags;
> > > > >  
> > > > > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > > > > +        return;
> > > > > +
> > > > > +    spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > > > +
> > > > > +    p_r = list_entry(v->arch.vgic.inflight_irqs.prev,
> > > > > +                         typeof(*p_r), inflight);
> > > > 
> > > > Is this getting the tail of the list or something else?
> > > > 
> > > > >      list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
> > > > >      {
> > > > >          i = find_first_zero_bit(&this_cpu(lr_mask), nr_lrs);
> > > > > -        if ( i >= nr_lrs ) return;
> > > > > +        if ( i >= nr_lrs )
> > > > > +        {
> > > > > +            while ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p_r->status) ||
> > > > > +                    test_bit(GIC_IRQ_GUEST_ACTIVE, &p_r->status) )
> > > > > +            {
> > > > > +                p_r = list_entry(p_r->inflight.prev, typeof(*p_r), inflight);
> > > > 
> > > > Oh, maybe this (and the thing above) is an open coded list_for_each_prev
> > > > or one of its variants? e.g. list_for_each_entry_reverse?
> > > 
> > > Yes, it is a list_for_each_entry_reverse that starts from the current
> > > p_r and stops when it finds the first entry that is
> > > GIC_IRQ_GUEST_VISIBLE and GIC_IRQ_GUEST_ACTIVE.
> > 
> > I think this can/should be recast in terms of the regular macro then.
> 
> OK.
> We'll need to introduce list_for_each_entry_continue_reverse.

Actually there is no need: like you wrote inflight is a small list and
the number of pending irqs that don't fit in any LRs is going to be even
smaller. We can just iterate in reverse from the end of the list every
time.