[PATCH-4.5 0/4] remove maintenance interrupts

[PATCH-4.5 0/4] 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.

Please test!!


Stefano Stabellini (4):
      xen/arm: remove unused virtual parameter from vgic_vcpu_inject_irq
      xen/arm: support HW interrupts in gic_set_lr
      xen/arm: do not request maintenance_interrupts
      xen/arm: set GICH_HCR_NPIE if all the LRs are in use

 xen/arch/arm/domain.c     |    2 +-
 xen/arch/arm/gic.c        |  158 +++++++++++++++++++++++++++++++++++++++++++++++++----------------------------------------------------
 xen/arch/arm/irq.c        |    2 +-
 xen/arch/arm/time.c       |    2 +-
 xen/arch/arm/vgic.c       |    7 ++---
 xen/arch/arm/vtimer.c     |    4 +--
 xen/include/asm-arm/gic.h |    2 +-
 7 files changed, 85 insertions(+), 92 deletions(-)

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

[PATCH-4.5 1/4] xen/arm: remove unused virtual parameter from vgic_vcpu_inject_irq

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.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 635a9a4..244738d 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -791,7 +791,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 50b3a38..acf7195 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -748,7 +748,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);
     if (!gic_events_need_delivery())
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 68b939d..0548201 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 90e9707..7d10227 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -455,7 +455,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;
 }
@@ -683,7 +683,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-4.5 2/4] xen/arm: support HW interrupts in gic_set_lr

[Stefano Stabellini]

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

Also add a struct vcpu* parameter to gic_set_lr.

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index acf7195..215b679 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -618,20 +618,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     return rc;
 }
 
-static inline void gic_set_lr(int lr, unsigned int virtual_irq,
+static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq,
         unsigned int state, unsigned int priority)
 {
-    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
-    struct pending_irq *p = irq_to_pending(current, virtual_irq);
+    struct pending_irq *p = irq_to_pending(v, irq);
 
     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 |
-        ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
-        ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
+    if ( p->desc != NULL )
+        GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ |
+            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+            ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |
+            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
+    else
+        GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ |
+            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
+            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
 
     set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
     clear_bit(GIC_IRQ_GUEST_PENDING, &p->status);
@@ -666,7 +670,7 @@ void gic_remove_from_queues(struct vcpu *v, unsigned int virtual_irq)
     spin_unlock(&gic.lock);
 }
 
-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;
@@ -679,12 +683,12 @@ 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, virtual_irq, state, priority);
+            gic_set_lr(v, i, irq, state, priority);
             goto out;
         }
     }
 
-    gic_add_to_lr_pending(v, virtual_irq, priority);
+    gic_add_to_lr_pending(v, irq, priority);
 
 out:
     spin_unlock_irqrestore(&gic.lock, flags);
@@ -703,7 +707,7 @@ static void gic_restore_pending_irqs(struct vcpu *v)
         if ( i >= nr_lrs ) return;
 
         spin_lock_irqsave(&gic.lock, flags);
-        gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
+        gic_set_lr(v, i, p->irq, GICH_LR_PENDING, p->priority);
         list_del_init(&p->lr_queue);
         set_bit(i, &this_cpu(lr_mask));
         spin_unlock_irqrestore(&gic.lock, flags);
@@ -950,7 +954,7 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
 
         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->irq, GICH_LR_PENDING, p2->priority);
+            gic_set_lr(v, i, p2->irq, GICH_LR_PENDING, p2->priority);
             list_del_init(&p2->lr_queue);
             set_bit(i, &this_cpu(lr_mask));
         }
-- 
1.7.10.4

[PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Stefano Stabellini]

Do not set GICH_LR_MAINTENANCE_IRQ for every interrupt with set in the
GICH_LR registers.

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).

Remove the now unused code in maintenance_interrupts and gic_irq_eoi.

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>
---
 xen/arch/arm/gic.c  |  126 ++++++++++++++++++++++-----------------------------
 xen/arch/arm/vgic.c |    3 +-
 2 files changed, 56 insertions(+), 73 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 215b679..87bd5d3 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;
@@ -126,6 +128,7 @@ void gic_restore_state(struct vcpu *v)
     GICH[GICH_HCR] = GICH_HCR_EN;
     isb();
 
+    gic_clear_lrs(v);
     gic_restore_pending_irqs(v);
 }
 
@@ -628,12 +631,12 @@ static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq,
     BUG_ON(state & ~(GICH_LR_STATE_MASK<<GICH_LR_STATE_SHIFT));
 
     if ( p->desc != NULL )
-        GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ |
+        GICH[GICH_LR + lr] = GICH_LR_HW | state |
             ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
             ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |
             ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
     else
-        GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ |
+        GICH[GICH_LR + lr] = state |
             ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
             ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
 
@@ -695,6 +698,54 @@ out:
     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)) )
+        {
+            if ( lr & GICH_LR_HW )
+                irq = (lr >> GICH_LR_PHYSICAL_SHIFT) & GICH_LR_PHYSICAL_MASK;
+            else
+                irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
+
+            inflight = 0;
+            GICH[GICH_LR + i] = 0;
+            clear_bit(i, &this_cpu(lr_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_add_to_lr_pending(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);
+            }
+
+        }
+
+        i++;
+    }
+}
+
 static void gic_restore_pending_irqs(struct vcpu *v)
 {
     int i;
@@ -751,6 +802,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);
 
@@ -908,77 +961,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, virq, p->priority);
-        }
-
-        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(v, i, p2->irq, GICH_LR_PENDING, p2->priority);
-            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 7d10227..da15f4d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -699,8 +699,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

[PATCH-4.5 4/4] xen/arm: set GICH_HCR_NPIE 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_NPIE 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>
---
 xen/arch/arm/gic.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 87bd5d3..bee2618 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -810,8 +810,14 @@ void gic_inject(void)
     gic_restore_pending_irqs(current);
     if (!gic_events_need_delivery())
         gic_inject_irq_stop();
-    else
+    else {
         gic_inject_irq_start();
+    }
+
+    if ( !list_empty(&current->arch.vgic.lr_pending) )
+        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
+    else
+        GICH[GICH_HCR] = GICH_HCR_EN;
 }
 
 int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
-- 
1.7.10.4

Re: [PATCH-4.5 1/4] xen/arm: remove unused virtual parameter from vgic_vcpu_inject_irq

[Julien Grall]

On 07/02/14 18:56, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
>   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 635a9a4..244738d 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -791,7 +791,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 50b3a38..acf7195 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -748,7 +748,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);
>       if (!gic_events_need_delivery())
> 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 68b939d..0548201 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 90e9707..7d10227 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -455,7 +455,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;
>   }
> @@ -683,7 +683,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);
>
>

-- 
Julien Grall

Re: [PATCH-4.5 2/4] xen/arm: support HW interrupts in gic_set_lr

[Julien Grall]

Hi Stefano,

On 07/02/14 18:56, Stefano Stabellini wrote:
> If the irq to be injected is an hardware irq (p->desc != NULL), set
> GICH_LR_HW.

If you set the GICH_LR_HW, I think you should remove the EOI of physical 
interrupt in maintenance IRQ in this patch. Otherwise we will EOI twice 
and from the documentation the behavior is unpredicatable.

> Also add a struct vcpu* parameter to gic_set_lr.
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/gic.c |   28 ++++++++++++++++------------
>   1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index acf7195..215b679 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -618,20 +618,24 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
>       return rc;
>   }
>
> -static inline void gic_set_lr(int lr, unsigned int virtual_irq,
> +static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq,
>           unsigned int state, unsigned int priority)
>   {
> -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> -    struct pending_irq *p = irq_to_pending(current, virtual_irq);
> +    struct pending_irq *p = irq_to_pending(v, irq);
>
>       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 |
> -        ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> -        ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    if ( p->desc != NULL )
> +        GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ |
> +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +            ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |

We should not assume that the physical IRQ == virtual IRQ. You should 
use p->desc->irq

> +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> +    else
> +        GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ |
> +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);

The final result of virtual IRQ is a subset of the physical IRQ. Can you 
factor the code?

-- 
Julien Grall

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Julien Grall]

Hi Stefano,

On 07/02/14 18:56, Stefano Stabellini wrote:
> Do not set GICH_LR_MAINTENANCE_IRQ for every interrupt with set in the
> GICH_LR registers.
>
> 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).
>
> Remove the now unused code in maintenance_interrupts and gic_irq_eoi.
>
> 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>
> ---
>   xen/arch/arm/gic.c  |  126 ++++++++++++++++++++++-----------------------------
>   xen/arch/arm/vgic.c |    3 +-
>   2 files changed, 56 insertions(+), 73 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 215b679..87bd5d3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> +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)) )
> +        {
> +            if ( lr & GICH_LR_HW )
> +                irq = (lr >> GICH_LR_PHYSICAL_SHIFT) & GICH_LR_PHYSICAL_MASK;
> +            else
> +                irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> +

The if sentence can be simply by:

irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;


> +            inflight = 0;
> +            GICH[GICH_LR + i] = 0;
> +            clear_bit(i, &this_cpu(lr_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))
> +            {

I would add a WARN_ON(p->desc != NULL) here. AFAIK, this code path 
shouldn't be used for physical IRQ.

-- 
Julien Grall

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Julien Grall]

On 07/02/14 18:56, Stefano Stabellini wrote:
    > +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) {

Did you look at to ELRSR{0,1} registers which list the usable LRs? I 
think you can use it with the this_cpu(lr_mask) to avoid browsing every LRs.

-- 
Julien Grall

Re: [PATCH-4.5 0/4] remove maintenance interrupts

[Julien Grall]

On 07/02/14 18:56, Stefano Stabellini wrote:
> Hi all,

Hi Stefano,

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

After reading your patch series I see a possible race condition with the 
timer interrupt.

As you know, Xen can re-inject the timer interrupt before the previous 
one is EOIed. As it's the timer, the IRQ is injected on the current 
running VCPU.

vgic_vcpu_inject_irq(timer)
   -> IRQ already visible to the guest -> set PENDING
return to guest context
<--------------------- Guest EOI the IRQ
.... few milleseconds
going to hyp mode
   -> doing stuff
   -> reinject the timer IRQ

If I'm not mistaken, with your solution, the next IRQ can be delayed for 
few milliseconds. That could be fixed by updating the Lrs.

-- 
Julien Grall

Re: [PATCH-4.5 4/4] xen/arm: set GICH_HCR_NPIE if all the LRs are in use

[Julien Grall]

On 07/02/14 18:56, Stefano Stabellini wrote:
> On return to guest, if there are no free LRs and we still have more
> interrupt to inject, set GICH_HCR_NPIE 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>
> ---
>   xen/arch/arm/gic.c |    8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 87bd5d3..bee2618 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -810,8 +810,14 @@ void gic_inject(void)
>       gic_restore_pending_irqs(current);
>       if (!gic_events_need_delivery())
>           gic_inject_irq_stop();
> -    else
> +    else {
>           gic_inject_irq_start();
> +    }
> +
> +    if ( !list_empty(&current->arch.vgic.lr_pending) )
> +        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
> +    else
> +        GICH[GICH_HCR] = GICH_HCR_EN;

Any reason to not move this in the else?

BTW, I think we can safely avoid to reinject the IRQ for the event 
channels if there is pending events. It will also avoid spurious IRQ on 
some specific case :).

-- 
Julien Grall

Re: [PATCH-4.5 2/4] xen/arm: support HW interrupts in gic_set_lr

[Stefano Stabellini]

On Fri, 7 Feb 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/02/14 18:56, Stefano Stabellini wrote:
> > If the irq to be injected is an hardware irq (p->desc != NULL), set
> > GICH_LR_HW.
> 
> If you set the GICH_LR_HW, I think you should remove the EOI of physical
> interrupt in maintenance IRQ in this patch. Otherwise we will EOI twice and
> from the documentation the behavior is unpredicatable.

Yes, you are right.


> > Also add a struct vcpu* parameter to gic_set_lr.
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >   xen/arch/arm/gic.c |   28 ++++++++++++++++------------
> >   1 file changed, 16 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index acf7195..215b679 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -618,20 +618,24 @@ int __init setup_dt_irq(const struct dt_irq *irq,
> > struct irqaction *new)
> >       return rc;
> >   }
> > 
> > -static inline void gic_set_lr(int lr, unsigned int virtual_irq,
> > +static inline void gic_set_lr(struct vcpu *v, int lr, unsigned int irq,
> >           unsigned int state, unsigned int priority)
> >   {
> > -    int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> > -    struct pending_irq *p = irq_to_pending(current, virtual_irq);
> > +    struct pending_irq *p = irq_to_pending(v, irq);
> > 
> >       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 |
> > -        ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > -        ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> > +    if ( p->desc != NULL )
> > +        GICH[GICH_LR + lr] = GICH_LR_HW | state | GICH_LR_MAINTENANCE_IRQ |
> > +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > +            ((irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT) |
> 
> We should not assume that the physical IRQ == virtual IRQ. You should use
> p->desc->irq

Right, I'll make the change.


> > +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> > +    else
> > +        GICH[GICH_LR + lr] = state | GICH_LR_MAINTENANCE_IRQ |
> > +            ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> > +            ((irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
> 
> The final result of virtual IRQ is a subset of the physical IRQ. Can you
> factor the code?

Yeah

Re: [PATCH-4.5 4/4] xen/arm: set GICH_HCR_NPIE if all the LRs are in use

[Stefano Stabellini]

On Fri, 7 Feb 2014, Julien Grall wrote:
> On 07/02/14 18:56, Stefano Stabellini wrote:
> > On return to guest, if there are no free LRs and we still have more
> > interrupt to inject, set GICH_HCR_NPIE 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>
> > ---
> >   xen/arch/arm/gic.c |    8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 87bd5d3..bee2618 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -810,8 +810,14 @@ void gic_inject(void)
> >       gic_restore_pending_irqs(current);
> >       if (!gic_events_need_delivery())
> >           gic_inject_irq_stop();
> > -    else
> > +    else {
> >           gic_inject_irq_start();
> > +    }
> > +
> > +    if ( !list_empty(&current->arch.vgic.lr_pending) )
> > +        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
> > +    else
> > +        GICH[GICH_HCR] = GICH_HCR_EN;
> 
> Any reason to not move this in the else?

Yes: we need to be able to disable GICH_HCR_NPIE even if there are no
irqs to inject.


> BTW, I think we can safely avoid to reinject the IRQ for the event channels if
> there is pending events. It will also avoid spurious IRQ on some specific case
> :).

Currently we don't set GIC_IRQ_GUEST_PENDING for evtchn_irq if the irq
is already inflight. I'll see if I can come up with a patch to
streamline that behaviour.

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Stefano Stabellini]

On Fri, 7 Feb 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/02/14 18:56, Stefano Stabellini wrote:
> > Do not set GICH_LR_MAINTENANCE_IRQ for every interrupt with set in the
> > GICH_LR registers.
> > 
> > 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).
> > 
> > Remove the now unused code in maintenance_interrupts and gic_irq_eoi.
> > 
> > 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>
> > ---
> >   xen/arch/arm/gic.c  |  126
> > ++++++++++++++++++++++-----------------------------
> >   xen/arch/arm/vgic.c |    3 +-
> >   2 files changed, 56 insertions(+), 73 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 215b679..87bd5d3 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > +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)) )
> > +        {
> > +            if ( lr & GICH_LR_HW )
> > +                irq = (lr >> GICH_LR_PHYSICAL_SHIFT) &
> > GICH_LR_PHYSICAL_MASK;
> > +            else
> > +                irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > +
> 
> The if sentence can be simply by:
> 
> irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;

right

 
> > +            inflight = 0;
> > +            GICH[GICH_LR + i] = 0;
> > +            clear_bit(i, &this_cpu(lr_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))
> > +            {
> 
> I would add a WARN_ON(p->desc != NULL) here. AFAIK, this code path shouldn't
> be used for physical IRQ.

That's not true: an edge physical irq can come through while another one
of the same type is being handled. In fact pending and active bits exist
even on the physical GIC interface.

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Stefano Stabellini]

On Fri, 7 Feb 2014, Julien Grall wrote:
> On 07/02/14 18:56, Stefano Stabellini wrote:
>    > +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) {
> 
> Did you look at to ELRSR{0,1} registers which list the usable LRs? I think you
> can use it with the this_cpu(lr_mask) to avoid browsing every LRs.

Given that we only have 4 LR registers, I think that unconditionally
reading 2 ELRSR registers would cost more than simply checking lr_mask
on average.

Re: [PATCH-4.5 0/4] remove maintenance interrupts

[Stefano Stabellini]

On Fri, 7 Feb 2014, Julien Grall wrote:
> On 07/02/14 18:56, Stefano Stabellini wrote:
> > Hi all,
> 
> Hi Stefano,
> 
> > 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.
> 
> After reading your patch series I see a possible race condition with the timer
> interrupt.
> 
> As you know, Xen can re-inject the timer interrupt before the previous one is
> EOIed. As it's the timer, the IRQ is injected on the current running VCPU.
> 
> vgic_vcpu_inject_irq(timer)
>   -> IRQ already visible to the guest -> set PENDING
> return to guest context
> <--------------------- Guest EOI the IRQ
> .... few milleseconds
> going to hyp mode
>   -> doing stuff
>   -> reinject the timer IRQ
> 
> If I'm not mistaken, with your solution, the next IRQ can be delayed for few
> milliseconds. That could be fixed by updating the Lrs.

You are right, this race exists. I'll work on a fix for the next
iteration of the series.

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Ian Campbell]

On Mon, 2014-02-10 at 17:06 +0000, Stefano Stabellini wrote:
> On Fri, 7 Feb 2014, Julien Grall wrote:
> > On 07/02/14 18:56, Stefano Stabellini wrote:
> >    > +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) {
> > 
> > Did you look at to ELRSR{0,1} registers which list the usable LRs? I think you
> > can use it with the this_cpu(lr_mask) to avoid browsing every LRs.
> 
> Given that we only have 4 LR registers, I think that unconditionally
> reading 2 ELRSR registers would cost more than simply checking lr_mask
> on average.

You also need to actually read the LR and do some bit masking etc don't
you?

What about implementations with >4 LRs?

Ian.

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Julien Grall]

On 02/10/2014 05:06 PM, Stefano Stabellini wrote:
> On Fri, 7 Feb 2014, Julien Grall wrote:
>> On 07/02/14 18:56, Stefano Stabellini wrote:
>>    > +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) {
>>
>> Did you look at to ELRSR{0,1} registers which list the usable LRs? I think you
>> can use it with the this_cpu(lr_mask) to avoid browsing every LRs.
> 
> Given that we only have 4 LR registers, I think that unconditionally
> reading 2 ELRSR registers would cost more than simply checking lr_mask
> on average.

The maximum number of LR registers is 64. I agree that the current
hardwares only handle 4 ... but we should think about future hardware.

-- 
Julien Grall

Re: [PATCH-4.5 4/4] xen/arm: set GICH_HCR_NPIE if all the LRs are in use

[Julien Grall]

On 02/10/2014 04:59 PM, Stefano Stabellini wrote:
> On Fri, 7 Feb 2014, Julien Grall wrote:
>> On 07/02/14 18:56, Stefano Stabellini wrote:
>>> On return to guest, if there are no free LRs and we still have more
>>> interrupt to inject, set GICH_HCR_NPIE 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>
>>> ---
>>>   xen/arch/arm/gic.c |    8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index 87bd5d3..bee2618 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -810,8 +810,14 @@ void gic_inject(void)
>>>       gic_restore_pending_irqs(current);
>>>       if (!gic_events_need_delivery())
>>>           gic_inject_irq_stop();
>>> -    else
>>> +    else {
>>>           gic_inject_irq_start();
>>> +    }
>>> +
>>> +    if ( !list_empty(&current->arch.vgic.lr_pending) )
>>> +        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
>>> +    else
>>> +        GICH[GICH_HCR] = GICH_HCR_EN;
>>
>> Any reason to not move this in the else?
> 
> Yes: we need to be able to disable GICH_HCR_NPIE even if there are no
> irqs to inject.

In this case, can we simply enable/disable GICH_HCR_NPIE instead of
resetting the register every time? GICH_HCR contains other bits that
could be set in the future.

>> BTW, I think we can safely avoid to reinject the IRQ for the event channels if
>> there is pending events. It will also avoid spurious IRQ on some specific case
>> :).
> 
> Currently we don't set GIC_IRQ_GUEST_PENDING for evtchn_irq if the irq
> is already inflight. I'll see if I can come up with a patch to
> streamline that behaviour.

Ok.


-- 
Julien Grall

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Stefano Stabellini]

On Mon, 10 Feb 2014, Ian Campbell wrote:
> On Mon, 2014-02-10 at 17:06 +0000, Stefano Stabellini wrote:
> > On Fri, 7 Feb 2014, Julien Grall wrote:
> > > On 07/02/14 18:56, Stefano Stabellini wrote:
> > >    > +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) {
> > > 
> > > Did you look at to ELRSR{0,1} registers which list the usable LRs? I think you
> > > can use it with the this_cpu(lr_mask) to avoid browsing every LRs.
> > 
> > Given that we only have 4 LR registers, I think that unconditionally
> > reading 2 ELRSR registers would cost more than simply checking lr_mask
> > on average.
> 
> You also need to actually read the LR and do some bit masking etc don't
> you?

No bit masking but I need to read the LRs, that are just 4.

> What about implementations with >4 LRs?

I could read ELRSR only if nr_lrs > 8 or something like that.

Re: [PATCH-4.5 4/4] xen/arm: set GICH_HCR_NPIE if all the LRs are in use

[Stefano Stabellini]

On Mon, 10 Feb 2014, Julien Grall wrote:
> On 02/10/2014 04:59 PM, Stefano Stabellini wrote:
> > On Fri, 7 Feb 2014, Julien Grall wrote:
> >> On 07/02/14 18:56, Stefano Stabellini wrote:
> >>> On return to guest, if there are no free LRs and we still have more
> >>> interrupt to inject, set GICH_HCR_NPIE 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>
> >>> ---
> >>>   xen/arch/arm/gic.c |    8 +++++++-
> >>>   1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >>> index 87bd5d3..bee2618 100644
> >>> --- a/xen/arch/arm/gic.c
> >>> +++ b/xen/arch/arm/gic.c
> >>> @@ -810,8 +810,14 @@ void gic_inject(void)
> >>>       gic_restore_pending_irqs(current);
> >>>       if (!gic_events_need_delivery())
> >>>           gic_inject_irq_stop();
> >>> -    else
> >>> +    else {
> >>>           gic_inject_irq_start();
> >>> +    }
> >>> +
> >>> +    if ( !list_empty(&current->arch.vgic.lr_pending) )
> >>> +        GICH[GICH_HCR] = GICH_HCR_EN | GICH_HCR_NPIE;
> >>> +    else
> >>> +        GICH[GICH_HCR] = GICH_HCR_EN;
> >>
> >> Any reason to not move this in the else?
> > 
> > Yes: we need to be able to disable GICH_HCR_NPIE even if there are no
> > irqs to inject.
> 
> In this case, can we simply enable/disable GICH_HCR_NPIE instead of
> resetting the register every time? GICH_HCR contains other bits that
> could be set in the future.

Sure

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Ian Campbell]

On Mon, 2014-02-10 at 17:16 +0000, Stefano Stabellini wrote:
> On Mon, 10 Feb 2014, Ian Campbell wrote:
> > On Mon, 2014-02-10 at 17:06 +0000, Stefano Stabellini wrote:
> > > On Fri, 7 Feb 2014, Julien Grall wrote:
> > > > On 07/02/14 18:56, Stefano Stabellini wrote:
> > > >    > +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) {
> > > > 
> > > > Did you look at to ELRSR{0,1} registers which list the usable LRs? I think you
> > > > can use it with the this_cpu(lr_mask) to avoid browsing every LRs.
> > > 
> > > Given that we only have 4 LR registers, I think that unconditionally
> > > reading 2 ELRSR registers would cost more than simply checking lr_mask
> > > on average.
> > 
> > You also need to actually read the LR and do some bit masking etc don't
> > you?
> 
> No bit masking but I need to read the LRs, that are just 4.

Having read them then what do you do with the result? Surely you need to
examine some or all of the bits to determine if it is free or not?

> > What about implementations with >4 LRs?
> 
> I could read ELRSR only if nr_lrs > 8 or something like that.

That would be the worst of both worlds IMHO.

Ian.

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Julien Grall]

On 02/10/2014 05:03 PM, Stefano Stabellini wrote:
>  
>>> +            inflight = 0;
>>> +            GICH[GICH_LR + i] = 0;
>>> +            clear_bit(i, &this_cpu(lr_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))
>>> +            {
>>
>> I would add a WARN_ON(p->desc != NULL) here. AFAIK, this code path shouldn't
>> be used for physical IRQ.
> 
> That's not true: an edge physical irq can come through while another one
> of the same type is being handled. In fact pending and active bits exist
> even on the physical GIC interface.
> 

It won't be fired until the previous one is EOIed. The physical GIC
interface will keep it internally.

But ... after thinking the WARN is stupid here because we can have this
following case:

IRQ A fired
    -> inject IRQ A
IRQ A eoied

IRQ A fired
   -> set pending bits

clear lrs
   -> re-inject IRQ A

-- 
Julien Grall

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Stefano Stabellini]

On Mon, 10 Feb 2014, Ian Campbell wrote:
> On Mon, 2014-02-10 at 17:16 +0000, Stefano Stabellini wrote:
> > On Mon, 10 Feb 2014, Ian Campbell wrote:
> > > On Mon, 2014-02-10 at 17:06 +0000, Stefano Stabellini wrote:
> > > > On Fri, 7 Feb 2014, Julien Grall wrote:
> > > > > On 07/02/14 18:56, Stefano Stabellini wrote:
> > > > >    > +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) {
> > > > > 
> > > > > Did you look at to ELRSR{0,1} registers which list the usable LRs? I think you
> > > > > can use it with the this_cpu(lr_mask) to avoid browsing every LRs.
> > > > 
> > > > Given that we only have 4 LR registers, I think that unconditionally
> > > > reading 2 ELRSR registers would cost more than simply checking lr_mask
> > > > on average.
> > > 
> > > You also need to actually read the LR and do some bit masking etc don't
> > > you?
> > 
> > No bit masking but I need to read the LRs, that are just 4.
> 
> Having read them then what do you do with the result? Surely you need to
> examine some or all of the bits to determine if it is free or not?

Right:

if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )

I thought you meant we need to write back a masked value.


> > > What about implementations with >4 LRs?
> > 
> > I could read ELRSR only if nr_lrs > 8 or something like that.
> 
> That would be the worst of both worlds IMHO.

On second thought you are right: if we always clear the LRs, the logic
is simpler and may be slower with an high number of LRs and concurrent
irqs.

If we don't clear all the LRs on return to guest and use ELRSR to filter
them, we are slower with a small number of LRs / concurrent irqs, faster
with an high number of LRs and concurrent irqs but the code would be
much more complex.

Overall checking for nr_lrs > 8 would add too much complexity for little
benefit.

Re: [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Ian Campbell]

On Mon, 2014-02-10 at 17:24 +0000, Stefano Stabellini wrote:
> On Mon, 10 Feb 2014, Ian Campbell wrote:
> > On Mon, 2014-02-10 at 17:16 +0000, Stefano Stabellini wrote:
> > > On Mon, 10 Feb 2014, Ian Campbell wrote:
> > > > On Mon, 2014-02-10 at 17:06 +0000, Stefano Stabellini wrote:
> > > > > On Fri, 7 Feb 2014, Julien Grall wrote:
> > > > > > On 07/02/14 18:56, Stefano Stabellini wrote:
> > > > > >    > +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) {
> > > > > > 
> > > > > > Did you look at to ELRSR{0,1} registers which list the usable LRs? I think you
> > > > > > can use it with the this_cpu(lr_mask) to avoid browsing every LRs.
> > > > > 
> > > > > Given that we only have 4 LR registers, I think that unconditionally
> > > > > reading 2 ELRSR registers would cost more than simply checking lr_mask
> > > > > on average.
> > > > 
> > > > You also need to actually read the LR and do some bit masking etc don't
> > > > you?
> > > 
> > > No bit masking but I need to read the LRs, that are just 4.
> > 
> > Having read them then what do you do with the result? Surely you need to
> > examine some or all of the bits to determine if it is free or not?
> 
> Right:
> 
> if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )

This is basically exactly what the ELRSR registers do for you.

Why not just use the h/w feature which gives you exactly what you need
instead of reinventing it yourself because you think it might be faster?
I think there is a certain amount of premature optimisation going on
here.

Ian.