[PATCH v3 0/6] interrupt handling fixes

[PATCH v3 0/6] interrupt handling fixes

[Stefano Stabellini]

Hi all,
this series is a reworked version of "Fix multiple issues with the
interrupts on ARM":

http://marc.info/?l=xen-devel&m=137211515720144

It fixes a few different issues that affect interrupt handling in Xen on
ARM today:

- the guest looses a vtimer interrupt notification when it sets a
deadline in the past from the guest vtimer interrupt handler, before
EOIing the interrupt;

- Xen adds a guest irq to the LR registers twice if the guest disables
and renables an interrupt before EOIing it;

- Xen enables interrupts corresponding to devices assigned to dom0
before booting dom0, resulting in the possibility of receiving an
interrupt and not knowing what to do with it.


Changes in v3:
- do not set the GUEST_PENDING bit for evtchn_irq if the irq is already
guest visible.

Changes in v2:
- remove eoi variable and check on p->desc != NULL instead;
- use atomic operations to modify the pending_irq status bits, remove
the now unnecessary locks;
- make status unsigned long;
- in maintenance_interrupt only stops injecting interrupts if no new
interrupts have been added to the LRs;
- add a state to keep track whether the guest irq is enabled at the
vgicd level;
- no need to read the current GICD_ISENABLER before writing it;
- protect startup and shutdown with gic and desc locks;
- disable IRQs that were previously disabled.


Julien Grall (2):
      xen/arm: Physical IRQ is not always equal to virtual IRQ
      xen/arm: Only enable physical IRQs when the guest asks

Stefano Stabellini (4):
      xen/arm: track the state of guest IRQs
      xen/arm: do not add a second irq to the LRs if one is already present
      xen/arm: implement gic_irq_enable and gic_irq_disable
      xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

 xen/arch/arm/gic.c           |  130 +++++++++++++++++++++++++-----------------
 xen/arch/arm/vgic.c          |   43 +++++++++++---
 xen/include/asm-arm/domain.h |   40 +++++++++++++
 3 files changed, 154 insertions(+), 59 deletions(-)

[PATCH v3 1/6] xen/arm: Physical IRQ is not always equal to virtual IRQ

[Stefano Stabellini]

From: Julien Grall <julien.grall@linaro.org>

When Xen needs to EOI a physical IRQ, we should use the IRQ number
in irq_desc instead of the virtual IRQ.

Remove the eoi flag in maintenance_interrupt and replace the check with
a check on p->desc != NULL.

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

Changes in v2:
- remove eoi variable and check on p->desc != NULL instead.
---
 xen/arch/arm/gic.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 43c11cb..5f7a548 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -880,7 +880,7 @@ static void gic_irq_eoi(void *info)
 
 static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 {
-    int i = 0, virq;
+    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);
@@ -888,10 +888,9 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
     while ((i = find_next_bit((const long unsigned int *) &eisr,
                               64, i)) < 64) {
         struct pending_irq *p;
-        int cpu, eoi;
+        int cpu;
 
         cpu = -1;
-        eoi = 0;
 
         spin_lock_irq(&gic.lock);
         lr = GICH[GICH_LR + i];
@@ -915,19 +914,19 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
             p->desc->status &= ~IRQ_INPROGRESS;
             /* Assume only one pcpu needs to EOI the irq */
             cpu = p->desc->arch.eoi_cpu;
-            eoi = 1;
+            pirq = p->desc->irq;
         }
         list_del_init(&p->inflight);
         spin_unlock_irq(&v->arch.vgic.lock);
 
-        if ( eoi ) {
+        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)virq);
+                gic_irq_eoi((void*)(uintptr_t)pirq);
             else
                 on_selected_cpus(cpumask_of(cpu),
-                                 gic_irq_eoi, (void*)(uintptr_t)virq, 0);
+                                 gic_irq_eoi, (void*)(uintptr_t)pirq, 0);
         }
 
         i++;
-- 
1.7.10.4

[PATCH v3 2/6] xen/arm: track the state of guest IRQs

[Stefano Stabellini]

Introduce a status field in struct pending_irq. Valid states are
GUEST_PENDING, GUEST_VISIBLE and GUEST_ENABLED and they are not mutually
exclusive.  See the in-code comment for an explanation of the states and
how they are used.
Use atomic operations to set and clear the status bits. Note that
setting GIC_IRQ_GUEST_VISIBLE and clearing GIC_IRQ_GUEST_PENDING can be
done in two separate operations as the underlying pending status is
actually only cleared on the LR after the guest ACKs the interrupts.
Until that happens it's not possible to receive another interrupt.

The main effect of this patch is that an IRQ can be set to GUEST_PENDING
while it is being serviced by the guest. In maintenance_interrupt we
check whether GUEST_PENDING is set and if it is we reinject the IRQ one
more time, if the interrupt is still enabled at the vgicd level.
If it is not, it is going to be injected as soon as the guest renables
the interrupt.

One exception is evtchn_irq: in that case we don't want to
set the GIC_IRQ_GUEST_PENDING bit if it is already GUEST_VISIBLE,
because as part of the event handling loop, the guest would realize that
new events are present even without a new notification.
Also we already have a way to figure out exactly when we do need to
inject a second notification if vgic_vcpu_inject_irq is called after the
end of the guest event handling loop and before the guest EOIs the
interrupt (see db453468d92369e7182663fb13e14d83ec4ce456 "arm: vgic: fix
race between evtchn upcall and evtchnop_send").

In maintenance_interrupt only stops injecting interrupts if no new
interrupts have been added to the LRs.

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

Changes in v2:
- use atomic operations to modify the pending_irq status bits, remove
the now unnecessary locks;
- make status unsigned long;
- in maintenance_interrupt only stops injecting interrupts if no new
interrupts have been added to the LRs;
- add a state to keep track whether the guest irq is enabled at the
vgicd level;

Changes in v3:
- do not set the GUEST_PENDING bit for evtchn_irq if the irq is already
guest visible.
---
 xen/arch/arm/gic.c           |   52 +++++++++++++++++++++++++++++++-----------
 xen/arch/arm/vgic.c          |   17 +++++++++++---
 xen/include/asm-arm/domain.h |   40 ++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5f7a548..d736a30 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -635,17 +635,20 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
 
     spin_lock_irqsave(&gic.lock, flags);
 
+    n = irq_to_pending(v, virtual_irq);
+
     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, virtual_irq, state, priority);
+            set_bit(_GIC_IRQ_GUEST_VISIBLE, &n->status);
+            clear_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
             goto out;
         }
     }
 
-    n = irq_to_pending(v, virtual_irq);
     if ( !list_empty(&n->lr_queue) )
         goto out;
 
@@ -680,6 +683,8 @@ static void gic_restore_pending_irqs(struct vcpu *v)
         list_del_init(&p->lr_queue);
         set_bit(i, &this_cpu(lr_mask));
         spin_unlock_irqrestore(&gic.lock, flags);
+        set_bit(_GIC_IRQ_GUEST_VISIBLE, &p->status);
+        clear_bit(_GIC_IRQ_GUEST_PENDING, &p->status);
     }
 
 }
@@ -884,10 +889,11 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
     uint32_t lr;
     struct vcpu *v = current;
     uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
+    unsigned int set_int = 0;
 
     while ((i = find_next_bit((const long unsigned int *) &eisr,
                               64, i)) < 64) {
-        struct pending_irq *p;
+        struct pending_irq *p, *p2;
         int cpu;
 
         cpu = -1;
@@ -898,17 +904,6 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
         GICH[GICH_LR + i] = 0;
         clear_bit(i, &this_cpu(lr_mask));
 
-        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
-            p = list_entry(v->arch.vgic.lr_pending.next, typeof(*p), lr_queue);
-            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
-            list_del_init(&p->lr_queue);
-            set_bit(i, &this_cpu(lr_mask));
-        } else {
-            gic_inject_irq_stop();
-        }
-        spin_unlock_irq(&gic.lock);
-
-        spin_lock_irq(&v->arch.vgic.lock);
         p = irq_to_pending(v, virq);
         if ( p->desc != NULL ) {
             p->desc->status &= ~IRQ_INPROGRESS;
@@ -916,6 +911,31 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
             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))
+        {
+            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
+            clear_bit(_GIC_IRQ_GUEST_PENDING, &p->status);
+            i++;
+            spin_unlock_irq(&gic.lock);
+            set_int++;
+            continue;
+        }
+
+        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->irq, GICH_LR_PENDING, p2->priority);
+            set_bit(_GIC_IRQ_GUEST_VISIBLE, &p2->status);
+            clear_bit(_GIC_IRQ_GUEST_PENDING, &p2->status);
+            list_del_init(&p2->lr_queue);
+            set_bit(i, &this_cpu(lr_mask));
+            set_int++;
+        }
+        spin_unlock_irq(&gic.lock);
+
+        spin_lock_irq(&v->arch.vgic.lock);
         list_del_init(&p->inflight);
         spin_unlock_irq(&v->arch.vgic.lock);
 
@@ -931,6 +951,12 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
 
         i++;
     }
+    if ( !set_int )
+    {
+        spin_lock_irq(&gic.lock);
+        gic_inject_irq_stop();
+        spin_unlock_irq(&gic.lock);
+    }
 }
 
 void gic_dump_info(struct vcpu *v)
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2e4b11f..a16bdf1 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -369,6 +369,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
         irq = i + (32 * n);
         p = irq_to_pending(v, irq);
+        set_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
         if ( !list_empty(&p->inflight) )
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
         i++;
@@ -672,8 +673,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
 
-    /* vcpu offline or irq already pending */
-    if (test_bit(_VPF_down, &v->pause_flags) || !list_empty(&n->inflight))
+    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);
+        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+        return;
+    }
+
+    /* vcpu offline */
+    if ( test_bit(_VPF_down, &v->pause_flags) )
     {
         spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
         return;
@@ -682,6 +692,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
     priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
 
     n->irq = irq;
+    set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
     n->priority = priority;
     if (!virtual)
         n->desc = irq_to_desc(irq);
@@ -689,7 +700,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
         n->desc = NULL;
 
     /* the irq is enabled */
-    if ( rank->ienable & (1 << (irq % 32)) )
+    if ( test_bit(_GIC_IRQ_GUEST_ENABLED, &n->status) )
         gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
 
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 8ebee3e..da34337 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -22,6 +22,46 @@ struct vgic_irq_rank {
 struct pending_irq
 {
     int irq;
+    /*
+     * The following two states track the lifecycle of the guest irq.
+     * However because we are not sure and we don't want to track
+     * whether an irq added to an LR register is PENDING or ACTIVE, the
+     * following states are just an approximation.
+     *
+     * GIC_IRQ_GUEST_PENDING: the irq is asserted
+     *
+     * GIC_IRQ_GUEST_VISIBLE: the irq has been added to an LR register,
+     * therefore the guest is aware of it. From the guest point of view
+     * the irq can be pending (if the guest has not acked the irq yet)
+     * or active (after acking the irq).
+     *
+     * In order for the state machine to be fully accurate, for level
+     * interrupts, we should keep the GIC_IRQ_GUEST_PENDING state until
+     * the guest deactivates the irq. However because we are not sure
+     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
+     * state when we add the irq to an LR register. We add it back when
+     * we receive another interrupt notification.
+     * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the
+     * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of
+     * the guest irq in the LR register from active to active and
+     * pending, but for simplicity we simply inject a second irq after
+     * the guest EOIs the first one.
+     *
+     *
+     * An additional state is used to keep track of whether the guest
+     * irq is enabled at the vgicd level:
+     *
+     * GIC_IRQ_GUEST_ENABLED: the guest IRQ is enabled at the VGICD
+     * level (GICD_ICENABLER/GICD_ISENABLER).
+     *
+     */
+#define _GIC_IRQ_GUEST_PENDING  0
+#define GIC_IRQ_GUEST_PENDING   (1UL<<0)
+#define _GIC_IRQ_GUEST_VISIBLE  1
+#define GIC_IRQ_GUEST_VISIBLE   (1UL<<1)
+#define _GIC_IRQ_GUEST_ENABLED  2
+#define GIC_IRQ_GUEST_ENABLED   (1UL<<2)
+    unsigned long status;
     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 v3 2/6] xen/arm: track the state of guest IRQs

[Ian Campbell]

On Tue, 2013-12-10 at 18:50 +0000, Stefano Stabellini wrote:
> Introduce a status field in struct pending_irq. Valid states are
> GUEST_PENDING, GUEST_VISIBLE and GUEST_ENABLED and they are not mutually
> exclusive.  See the in-code comment for an explanation of the states and
> how they are used.
> Use atomic operations to set and clear the status bits. Note that
> setting GIC_IRQ_GUEST_VISIBLE and clearing GIC_IRQ_GUEST_PENDING can be
> done in two separate operations as the underlying pending status is
> actually only cleared on the LR after the guest ACKs the interrupts.
> Until that happens it's not possible to receive another interrupt.

Is the ordering of the set/clear important? It seems like it ought to
be, one of the orderings risks losing information and the other risks
spurious interrupts (the latter being obviously preferred).

> One exception is evtchn_irq: in that case we don't want to
> set the GIC_IRQ_GUEST_PENDING bit if it is already GUEST_VISIBLE,
> because as part of the event handling loop, the guest would realize that
> new events are present even without a new notification.

Is the race between checking the event bit mask and unmasking and/or
acking the interrupt closed somehow?

(We had an issue like this with PVHVM on x86 didn't we?)

> Also we already have a way to figure out exactly when we do need to
> inject a second notification if vgic_vcpu_inject_irq is called after the
> end of the guest event handling loop and before the guest EOIs the
> interrupt (see db453468d92369e7182663fb13e14d83ec4ce456 "arm: vgic: fix
> race between evtchn upcall and evtchnop_send").
> 
> In maintenance_interrupt only stops injecting interrupts if no new

stops => stop.

> interrupts have been added to the LRs.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> Changes in v2:
> - use atomic operations to modify the pending_irq status bits, remove
> the now unnecessary locks;
> - make status unsigned long;
> - in maintenance_interrupt only stops injecting interrupts if no new
> interrupts have been added to the LRs;
> - add a state to keep track whether the guest irq is enabled at the
> vgicd level;
> 
> Changes in v3:
> - do not set the GUEST_PENDING bit for evtchn_irq if the irq is already
> guest visible.
> ---
>  xen/arch/arm/gic.c           |   52 +++++++++++++++++++++++++++++++-----------
>  xen/arch/arm/vgic.c          |   17 +++++++++++---
>  xen/include/asm-arm/domain.h |   40 ++++++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 5f7a548..d736a30 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -635,17 +635,20 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>  
>      spin_lock_irqsave(&gic.lock, flags);
>  
> +    n = irq_to_pending(v, virtual_irq);
> +
>      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, virtual_irq, state, priority);
> +            set_bit(_GIC_IRQ_GUEST_VISIBLE, &n->status);
> +            clear_bit(_GIC_IRQ_GUEST_PENDING, &n->status);

This gic_set_lr+set lr_mask+set VISIBLE+clear PENDING happens a lot and
always together (apart from the maint interrupt which skips setting
visible, but that would be harmless there) -- why not push the bit
manipulations down into gic_set_lr?

> @@ -884,10 +889,11 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>      uint32_t lr;
>      struct vcpu *v = current;
>      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
> +    unsigned int set_int = 0;

This doesn't need to be unsigned and doesn't need to actually count the
number, it's just a boolean, treating it as such avoids
read/modify/write cycles.

>  
>      while ((i = find_next_bit((const long unsigned int *) &eisr,
>                                64, i)) < 64) {
> -        struct pending_irq *p;
> +        struct pending_irq *p, *p2;
>          int cpu;
>  
>          cpu = -1;
> @@ -898,17 +904,6 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>          GICH[GICH_LR + i] = 0;
>          clear_bit(i, &this_cpu(lr_mask));
>  
> -        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
> -            p = list_entry(v->arch.vgic.lr_pending.next, typeof(*p), lr_queue);
> -            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> -            list_del_init(&p->lr_queue);
> -            set_bit(i, &this_cpu(lr_mask));
> -        } else {
> -            gic_inject_irq_stop();
> -        }
> -        spin_unlock_irq(&gic.lock);
> -
> -        spin_lock_irq(&v->arch.vgic.lock);
>          p = irq_to_pending(v, virq);
>          if ( p->desc != NULL ) {
>              p->desc->status &= ~IRQ_INPROGRESS;
> @@ -916,6 +911,31 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
>              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))
> +        {
> +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> +            clear_bit(_GIC_IRQ_GUEST_PENDING, &p->status);
> +            i++;
> +            spin_unlock_irq(&gic.lock);
> +            set_int++;
> +            continue;
> +        }
> +
> +        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->irq, GICH_LR_PENDING, p2->priority);
> +            set_bit(_GIC_IRQ_GUEST_VISIBLE, &p2->status);
> +            clear_bit(_GIC_IRQ_GUEST_PENDING, &p2->status);
> +            list_del_init(&p2->lr_queue);
> +            set_bit(i, &this_cpu(lr_mask));
> +            set_int++;
> +        }
> +        spin_unlock_irq(&gic.lock);

This ordering means that a low priority but frequently occurring
interrupt can starve out a high priority one, because it will keep
jumping the queue.

Our priority handling is pretty rudimentary right now, but we should at
least consciously decide we are happy with this.

The solution would be to put the interrupt back on the lr_pending queue
in its proper order and rely on it getting reinjected that way, I think?

> +
> +        spin_lock_irq(&v->arch.vgic.lock);
>          list_del_init(&p->inflight);
>          spin_unlock_irq(&v->arch.vgic.lock);
>  
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2e4b11f..a16bdf1 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -369,6 +369,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>      while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
>          irq = i + (32 * n);
>          p = irq_to_pending(v, irq);
> +        set_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
>          if ( !list_empty(&p->inflight) )
>              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
>          i++;
> @@ -672,8 +673,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
>  
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>  
> -    /* vcpu offline or irq already pending */
> -    if (test_bit(_VPF_down, &v->pause_flags) || !list_empty(&n->inflight))
> +    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);

I worry about a race here. I think the consequence is a spurious
interrupt i.e. the safe option. Right? If yes then a comment to that
affect would be useful.

> +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +        return;
> +    }
> +
> +    /* vcpu offline */
> +    if ( test_bit(_VPF_down, &v->pause_flags) )
>      {
>          spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>          return;
> @@ -682,6 +692,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
>      priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
>  
>      n->irq = irq;
> +    set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
>      n->priority = priority;
>      if (!virtual)
>          n->desc = irq_to_desc(irq);
> @@ -689,7 +700,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
>          n->desc = NULL;
>  
>      /* the irq is enabled */
> -    if ( rank->ienable & (1 << (irq % 32)) )
> +    if ( test_bit(_GIC_IRQ_GUEST_ENABLED, &n->status) )
>          gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
>  
>      list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 8ebee3e..da34337 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -22,6 +22,46 @@ struct vgic_irq_rank {
>  struct pending_irq
>  {
>      int irq;
> +    /*
> +     * The following two states track the lifecycle of the guest irq.
> +     * However because we are not sure and we don't want to track
> +     * whether an irq added to an LR register is PENDING or ACTIVE, the
> +     * following states are just an approximation.
> +     *
> +     * GIC_IRQ_GUEST_PENDING: the irq is asserted
> +     *
> +     * GIC_IRQ_GUEST_VISIBLE: the irq has been added to an LR register,
> +     * therefore the guest is aware of it. From the guest point of view
> +     * the irq can be pending (if the guest has not acked the irq yet)
> +     * or active (after acking the irq).
> +     *
> +     * In order for the state machine to be fully accurate, for level
> +     * interrupts, we should keep the GIC_IRQ_GUEST_PENDING state until
> +     * the guest deactivates the irq. However because we are not sure
> +     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
> +     * state when we add the irq to an LR register. We add it back when
> +     * we receive another interrupt notification.
> +     * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the
> +     * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of
> +     * the guest irq in the LR register from active to active and
> +     * pending, but for simplicity we simply inject a second irq after
> +     * the guest EOIs the first one.
> +     *
> +     *
> +     * An additional state is used to keep track of whether the guest
> +     * irq is enabled at the vgicd level:
> +     *
> +     * GIC_IRQ_GUEST_ENABLED: the guest IRQ is enabled at the VGICD
> +     * level (GICD_ICENABLER/GICD_ISENABLER).
> +     *
> +     */
> +#define _GIC_IRQ_GUEST_PENDING  0
> +#define GIC_IRQ_GUEST_PENDING   (1UL<<0)
> +#define _GIC_IRQ_GUEST_VISIBLE  1
> +#define GIC_IRQ_GUEST_VISIBLE   (1UL<<1)
> +#define _GIC_IRQ_GUEST_ENABLED  2
> +#define GIC_IRQ_GUEST_ENABLED   (1UL<<2)

You don't need the raw values now, only the bit offsets, I think you can
drop GIC_IRQ* and rename _GIC_IRQ* without the underscore. Doing this
will help prevent people thinking they can manipulate the status field
without bitops.

> +    unsigned long status;
>      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 v3 2/6] xen/arm: track the state of guest IRQs

[Stefano Stabellini]

On Wed, 11 Dec 2013, Ian Campbell wrote:
> On Tue, 2013-12-10 at 18:50 +0000, Stefano Stabellini wrote:
> > Introduce a status field in struct pending_irq. Valid states are
> > GUEST_PENDING, GUEST_VISIBLE and GUEST_ENABLED and they are not mutually
> > exclusive.  See the in-code comment for an explanation of the states and
> > how they are used.
> > Use atomic operations to set and clear the status bits. Note that
> > setting GIC_IRQ_GUEST_VISIBLE and clearing GIC_IRQ_GUEST_PENDING can be
> > done in two separate operations as the underlying pending status is
> > actually only cleared on the LR after the guest ACKs the interrupts.
> > Until that happens it's not possible to receive another interrupt.
> 
> Is the ordering of the set/clear important? It seems like it ought to
> be, one of the orderings risks losing information and the other risks
> spurious interrupts (the latter being obviously preferred).

The only case for which it might matter is physical edge-triggered IRQs
(I have none on the platform I am testing this series on).
If we clear GUEST_PENDING before setting GUEST_VISIBLE we slightly
reduce the chance of loosing an interrupt. But keep in mind that the
chance still exists: after we read GICC_IAR and before we clear
GUEST_PENDING, if we receive another hardware interrupt it would be
lost.
So I don't think that the relative order of GUEST_PENDING and
GUEST_VISIBLE makes any meaningful difference.


> > One exception is evtchn_irq: in that case we don't want to
> > set the GIC_IRQ_GUEST_PENDING bit if it is already GUEST_VISIBLE,
> > because as part of the event handling loop, the guest would realize that
> > new events are present even without a new notification.
> 
> Is the race between checking the event bit mask and unmasking and/or
> acking the interrupt closed somehow?
 
The window when the guest can loose evtchn notifications happens
after existing the event handling loop and before EOIing the interrupt.
We fixed it by noticing that evtchn_upcall_pending is set and trying an
evtchn_irq re-injection right before returning to guest.

Of course we don't want to re-inject the evtchn_irq twice if it is
already there so that's why I added the GIC_IRQ_GUEST_VISIBLE check in
vgic_vcpu_inject_irq. The behavior in this regard should be exactly the
same as before.

We don't want to rely on GIC_IRQ_GUEST_PENDING to inject a second
evtchn_irq because if the guest is in the event handling loop it doesn't
actually need a new notification.


> (We had an issue like this with PVHVM on x86 didn't we?)

It is different because in that case the source of notifications (the
vector callback) doesn't need an EOI.


> > Also we already have a way to figure out exactly when we do need to
> > inject a second notification if vgic_vcpu_inject_irq is called after the
> > end of the guest event handling loop and before the guest EOIs the
> > interrupt (see db453468d92369e7182663fb13e14d83ec4ce456 "arm: vgic: fix
> > race between evtchn upcall and evtchnop_send").
> > 
> > In maintenance_interrupt only stops injecting interrupts if no new
> 
> stops => stop.
> 
> > interrupts have been added to the LRs.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > Changes in v2:
> > - use atomic operations to modify the pending_irq status bits, remove
> > the now unnecessary locks;
> > - make status unsigned long;
> > - in maintenance_interrupt only stops injecting interrupts if no new
> > interrupts have been added to the LRs;
> > - add a state to keep track whether the guest irq is enabled at the
> > vgicd level;
> > 
> > Changes in v3:
> > - do not set the GUEST_PENDING bit for evtchn_irq if the irq is already
> > guest visible.
> > ---
> >  xen/arch/arm/gic.c           |   52 +++++++++++++++++++++++++++++++-----------
> >  xen/arch/arm/vgic.c          |   17 +++++++++++---
> >  xen/include/asm-arm/domain.h |   40 ++++++++++++++++++++++++++++++++
> >  3 files changed, 93 insertions(+), 16 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 5f7a548..d736a30 100644
> > --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> > @@ -635,17 +635,20 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> >  
> >      spin_lock_irqsave(&gic.lock, flags);
> >  
> > +    n = irq_to_pending(v, virtual_irq);
> > +
> >      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, virtual_irq, state, priority);
> > +            set_bit(_GIC_IRQ_GUEST_VISIBLE, &n->status);
> > +            clear_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
> 
> This gic_set_lr+set lr_mask+set VISIBLE+clear PENDING happens a lot and
> always together (apart from the maint interrupt which skips setting
> visible, but that would be harmless there) -- why not push the bit
> manipulations down into gic_set_lr?

OK


> > @@ -884,10 +889,11 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
> >      uint32_t lr;
> >      struct vcpu *v = current;
> >      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
> > +    unsigned int set_int = 0;
> 
> This doesn't need to be unsigned and doesn't need to actually count the
> number, it's just a boolean, treating it as such avoids
> read/modify/write cycles.

OK


> >  
> >      while ((i = find_next_bit((const long unsigned int *) &eisr,
> >                                64, i)) < 64) {
> > -        struct pending_irq *p;
> > +        struct pending_irq *p, *p2;
> >          int cpu;
> >  
> >          cpu = -1;
> > @@ -898,17 +904,6 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
> >          GICH[GICH_LR + i] = 0;
> >          clear_bit(i, &this_cpu(lr_mask));
> >  
> > -        if ( !list_empty(&v->arch.vgic.lr_pending) ) {
> > -            p = list_entry(v->arch.vgic.lr_pending.next, typeof(*p), lr_queue);
> > -            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> > -            list_del_init(&p->lr_queue);
> > -            set_bit(i, &this_cpu(lr_mask));
> > -        } else {
> > -            gic_inject_irq_stop();
> > -        }
> > -        spin_unlock_irq(&gic.lock);
> > -
> > -        spin_lock_irq(&v->arch.vgic.lock);
> >          p = irq_to_pending(v, virq);
> >          if ( p->desc != NULL ) {
> >              p->desc->status &= ~IRQ_INPROGRESS;
> > @@ -916,6 +911,31 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
> >              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))
> > +        {
> > +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> > +            clear_bit(_GIC_IRQ_GUEST_PENDING, &p->status);
> > +            i++;
> > +            spin_unlock_irq(&gic.lock);
> > +            set_int++;
> > +            continue;
> > +        }
> > +
> > +        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->irq, GICH_LR_PENDING, p2->priority);
> > +            set_bit(_GIC_IRQ_GUEST_VISIBLE, &p2->status);
> > +            clear_bit(_GIC_IRQ_GUEST_PENDING, &p2->status);
> > +            list_del_init(&p2->lr_queue);
> > +            set_bit(i, &this_cpu(lr_mask));
> > +            set_int++;
> > +        }
> > +        spin_unlock_irq(&gic.lock);
> 
> This ordering means that a low priority but frequently occurring
> interrupt can starve out a high priority one, because it will keep
> jumping the queue.
> 
> Our priority handling is pretty rudimentary right now, but we should at
> least consciously decide we are happy with this.
> 
> The solution would be to put the interrupt back on the lr_pending queue
> in its proper order and rely on it getting reinjected that way, I think?

You are right, I'll do that.


> > +
> > +        spin_lock_irq(&v->arch.vgic.lock);
> >          list_del_init(&p->inflight);
> >          spin_unlock_irq(&v->arch.vgic.lock);
> >  
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 2e4b11f..a16bdf1 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -369,6 +369,7 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> >      while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
> >          irq = i + (32 * n);
> >          p = irq_to_pending(v, irq);
> > +        set_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
> >          if ( !list_empty(&p->inflight) )
> >              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> >          i++;
> > @@ -672,8 +673,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
> >  
> >      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> >  
> > -    /* vcpu offline or irq already pending */
> > -    if (test_bit(_VPF_down, &v->pause_flags) || !list_empty(&n->inflight))
> > +    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);
> 
> I worry about a race here. I think the consequence is a spurious
> interrupt i.e. the safe option. Right? If yes then a comment to that
> affect would be useful.

There is no race: it would only be possible for evtchn injections but
those are completely taken care of by the in-guest evtchn loop and
db453468d92369e7182663fb13e14d83ec4ce456.

Or maybe you mean something else that I am not seeing?


> > +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +        return;
> > +    }
> > +
> > +    /* vcpu offline */
> > +    if ( test_bit(_VPF_down, &v->pause_flags) )
> >      {
> >          spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >          return;
> > @@ -682,6 +692,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
> >      priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
> >  
> >      n->irq = irq;
> > +    set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
> >      n->priority = priority;
> >      if (!virtual)
> >          n->desc = irq_to_desc(irq);
> > @@ -689,7 +700,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
> >          n->desc = NULL;
> >  
> >      /* the irq is enabled */
> > -    if ( rank->ienable & (1 << (irq % 32)) )
> > +    if ( test_bit(_GIC_IRQ_GUEST_ENABLED, &n->status) )
> >          gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
> >  
> >      list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
> > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> > index 8ebee3e..da34337 100644
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -22,6 +22,46 @@ struct vgic_irq_rank {
> >  struct pending_irq
> >  {
> >      int irq;
> > +    /*
> > +     * The following two states track the lifecycle of the guest irq.
> > +     * However because we are not sure and we don't want to track
> > +     * whether an irq added to an LR register is PENDING or ACTIVE, the
> > +     * following states are just an approximation.
> > +     *
> > +     * GIC_IRQ_GUEST_PENDING: the irq is asserted
> > +     *
> > +     * GIC_IRQ_GUEST_VISIBLE: the irq has been added to an LR register,
> > +     * therefore the guest is aware of it. From the guest point of view
> > +     * the irq can be pending (if the guest has not acked the irq yet)
> > +     * or active (after acking the irq).
> > +     *
> > +     * In order for the state machine to be fully accurate, for level
> > +     * interrupts, we should keep the GIC_IRQ_GUEST_PENDING state until
> > +     * the guest deactivates the irq. However because we are not sure
> > +     * when that happens, we simply remove the GIC_IRQ_GUEST_PENDING
> > +     * state when we add the irq to an LR register. We add it back when
> > +     * we receive another interrupt notification.
> > +     * Therefore it is possible to set GIC_IRQ_GUEST_PENDING while the
> > +     * irq is GIC_IRQ_GUEST_VISIBLE. We could also change the state of
> > +     * the guest irq in the LR register from active to active and
> > +     * pending, but for simplicity we simply inject a second irq after
> > +     * the guest EOIs the first one.
> > +     *
> > +     *
> > +     * An additional state is used to keep track of whether the guest
> > +     * irq is enabled at the vgicd level:
> > +     *
> > +     * GIC_IRQ_GUEST_ENABLED: the guest IRQ is enabled at the VGICD
> > +     * level (GICD_ICENABLER/GICD_ISENABLER).
> > +     *
> > +     */
> > +#define _GIC_IRQ_GUEST_PENDING  0
> > +#define GIC_IRQ_GUEST_PENDING   (1UL<<0)
> > +#define _GIC_IRQ_GUEST_VISIBLE  1
> > +#define GIC_IRQ_GUEST_VISIBLE   (1UL<<1)
> > +#define _GIC_IRQ_GUEST_ENABLED  2
> > +#define GIC_IRQ_GUEST_ENABLED   (1UL<<2)
> 
> You don't need the raw values now, only the bit offsets, I think you can
> drop GIC_IRQ* and rename _GIC_IRQ* without the underscore. Doing this
> will help prevent people thinking they can manipulate the status field
> without bitops.

OK

Re: [PATCH v3 2/6] xen/arm: track the state of guest IRQs

[Ian Campbell]

On Wed, 2013-12-11 at 19:00 +0000, Stefano Stabellini wrote:
> On Wed, 11 Dec 2013, Ian Campbell wrote:
> > On Tue, 2013-12-10 at 18:50 +0000, Stefano Stabellini wrote:
> > > Introduce a status field in struct pending_irq. Valid states are
> > > GUEST_PENDING, GUEST_VISIBLE and GUEST_ENABLED and they are not mutually
> > > exclusive.  See the in-code comment for an explanation of the states and
> > > how they are used.
> > > Use atomic operations to set and clear the status bits. Note that
> > > setting GIC_IRQ_GUEST_VISIBLE and clearing GIC_IRQ_GUEST_PENDING can be
> > > done in two separate operations as the underlying pending status is
> > > actually only cleared on the LR after the guest ACKs the interrupts.
> > > Until that happens it's not possible to receive another interrupt.
> > 
> > Is the ordering of the set/clear important? It seems like it ought to
> > be, one of the orderings risks losing information and the other risks
> > spurious interrupts (the latter being obviously preferred).
> 
> The only case for which it might matter is physical edge-triggered IRQs
> (I have none on the platform I am testing this series on).
> If we clear GUEST_PENDING before setting GUEST_VISIBLE we slightly
> reduce the chance of loosing an interrupt. But keep in mind that the
> chance still exists: after we read GICC_IAR and before we clear
> GUEST_PENDING, if we receive another hardware interrupt it would be
> lost.

... and that is OK because? We consider them to have been coalesced I
guess?

> So I don't think that the relative order of GUEST_PENDING and
> GUEST_VISIBLE makes any meaningful difference.
> 
> 
> > > One exception is evtchn_irq: in that case we don't want to
> > > set the GIC_IRQ_GUEST_PENDING bit if it is already GUEST_VISIBLE,
> > > because as part of the event handling loop, the guest would realize that
> > > new events are present even without a new notification.
> > 
> > Is the race between checking the event bit mask and unmasking and/or
> > acking the interrupt closed somehow?
>  
> The window when the guest can loose evtchn notifications happens
> after existing the event handling loop and before EOIing the interrupt.
> We fixed it by noticing that evtchn_upcall_pending is set and trying an
> evtchn_irq re-injection right before returning to guest.
> 
> Of course we don't want to re-inject the evtchn_irq twice if it is
> already there so that's why I added the GIC_IRQ_GUEST_VISIBLE check in
> vgic_vcpu_inject_irq. The behavior in this regard should be exactly the
> same as before.
> 
> We don't want to rely on GIC_IRQ_GUEST_PENDING to inject a second
> evtchn_irq because if the guest is in the event handling loop it doesn't
> actually need a new notification.

OK, I think...

> > (We had an issue like this with PVHVM on x86 didn't we?)
> 
> It is different because in that case the source of notifications (the
> vector callback) doesn't need an EOI.

and should we ever figure out how to do that on ARM it will need a guest
visible flag to enable anyway. OK then.

> > > @@ -672,8 +673,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
> > >  
> > >      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > >  
> > > -    /* vcpu offline or irq already pending */
> > > -    if (test_bit(_VPF_down, &v->pause_flags) || !list_empty(&n->inflight))
> > > +    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);
> > 
> > I worry about a race here. I think the consequence is a spurious
> > interrupt i.e. the safe option. Right? If yes then a comment to that
> > affect would be useful.
> 
> There is no race: it would only be possible for evtchn injections but
> those are completely taken care of by the in-guest evtchn loop and
> db453468d92369e7182663fb13e14d83ec4ce456.
> 
> Or maybe you mean something else that I am not seeing?

If something clears VISIBLE between the test and the set, do things work
correctly?

If VISIBLE is set right after the test, what happens?

Ian.

Re: [PATCH v3 2/6] xen/arm: track the state of guest IRQs

[Stefano Stabellini]

On Thu, 12 Dec 2013, Ian Campbell wrote:
> On Wed, 2013-12-11 at 19:00 +0000, Stefano Stabellini wrote:
> > On Wed, 11 Dec 2013, Ian Campbell wrote:
> > > On Tue, 2013-12-10 at 18:50 +0000, Stefano Stabellini wrote:
> > > > Introduce a status field in struct pending_irq. Valid states are
> > > > GUEST_PENDING, GUEST_VISIBLE and GUEST_ENABLED and they are not mutually
> > > > exclusive.  See the in-code comment for an explanation of the states and
> > > > how they are used.
> > > > Use atomic operations to set and clear the status bits. Note that
> > > > setting GIC_IRQ_GUEST_VISIBLE and clearing GIC_IRQ_GUEST_PENDING can be
> > > > done in two separate operations as the underlying pending status is
> > > > actually only cleared on the LR after the guest ACKs the interrupts.
> > > > Until that happens it's not possible to receive another interrupt.
> > > 
> > > Is the ordering of the set/clear important? It seems like it ought to
> > > be, one of the orderings risks losing information and the other risks
> > > spurious interrupts (the latter being obviously preferred).
> > 
> > The only case for which it might matter is physical edge-triggered IRQs
> > (I have none on the platform I am testing this series on).
> > If we clear GUEST_PENDING before setting GUEST_VISIBLE we slightly
> > reduce the chance of loosing an interrupt. But keep in mind that the
> > chance still exists: after we read GICC_IAR and before we clear
> > GUEST_PENDING, if we receive another hardware interrupt it would be
> > lost.
> 
> ... and that is OK because? We consider them to have been coalesced I
> guess?

Yes. Also I can't see another way around this.


> > So I don't think that the relative order of GUEST_PENDING and
> > GUEST_VISIBLE makes any meaningful difference.
> > 
> > 
> > > > One exception is evtchn_irq: in that case we don't want to
> > > > set the GIC_IRQ_GUEST_PENDING bit if it is already GUEST_VISIBLE,
> > > > because as part of the event handling loop, the guest would realize that
> > > > new events are present even without a new notification.
> > > 
> > > Is the race between checking the event bit mask and unmasking and/or
> > > acking the interrupt closed somehow?
> >  
> > The window when the guest can loose evtchn notifications happens
> > after existing the event handling loop and before EOIing the interrupt.
> > We fixed it by noticing that evtchn_upcall_pending is set and trying an
> > evtchn_irq re-injection right before returning to guest.
> > 
> > Of course we don't want to re-inject the evtchn_irq twice if it is
> > already there so that's why I added the GIC_IRQ_GUEST_VISIBLE check in
> > vgic_vcpu_inject_irq. The behavior in this regard should be exactly the
> > same as before.
> > 
> > We don't want to rely on GIC_IRQ_GUEST_PENDING to inject a second
> > evtchn_irq because if the guest is in the event handling loop it doesn't
> > actually need a new notification.
> 
> OK, I think...
> 
> > > (We had an issue like this with PVHVM on x86 didn't we?)
> > 
> > It is different because in that case the source of notifications (the
> > vector callback) doesn't need an EOI.
> 
> and should we ever figure out how to do that on ARM it will need a guest
> visible flag to enable anyway. OK then.
> 
> > > > @@ -672,8 +673,17 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
> > > >  
> > > >      spin_lock_irqsave(&v->arch.vgic.lock, flags);
> > > >  
> > > > -    /* vcpu offline or irq already pending */
> > > > -    if (test_bit(_VPF_down, &v->pause_flags) || !list_empty(&n->inflight))
> > > > +    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);
> > > 
> > > I worry about a race here. I think the consequence is a spurious
> > > interrupt i.e. the safe option. Right? If yes then a comment to that
> > > affect would be useful.
> > 
> > There is no race: it would only be possible for evtchn injections but
> > those are completely taken care of by the in-guest evtchn loop and
> > db453468d92369e7182663fb13e14d83ec4ce456.
> > 
> > Or maybe you mean something else that I am not seeing?
> 
> If something clears VISIBLE between the test and the set, do things work
> correctly?

Yes, because the test at the beginning of gic_inject will cover that
case.


> If VISIBLE is set right after the test, what happens?
 
It is OK because it means that the guest is getting another interrupt
notification anyway. The guest OS would lose the second interrupt even
on native: until the guest ACKs the interrupt is not supposed to be able
to receive another interrupt.
We are actually allowing the guest to receive more interrupt
notifications than it would be capable if it was running bare metal
by clearing PENDING before the guest does it.

[PATCH v3 3/6] xen/arm: do not add a second irq to the LRs if one is already present

[Stefano Stabellini]

When the guest re-enable IRQs, do not add guest IRQs to LRs twice.

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

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index a16bdf1..ab788da 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -370,7 +370,7 @@ 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) )
+        if ( !list_empty(&p->inflight) && !test_bit(_GIC_IRQ_GUEST_VISIBLE, &p->status) )
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
         i++;
     }
-- 
1.7.10.4

Re: [PATCH v3 3/6] xen/arm: do not add a second irq to the LRs if one is already present

[Julien Grall]

On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> When the guest re-enable IRQs, do not add guest IRQs to LRs twice.
>
> Suggested-by: Julien Grall <julien.grall@linaro.org>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

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

> ---
>   xen/arch/arm/vgic.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index a16bdf1..ab788da 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -370,7 +370,7 @@ 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) )
> +        if ( !list_empty(&p->inflight) && !test_bit(_GIC_IRQ_GUEST_VISIBLE, &p->status) )
>               gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
>           i++;
>       }
>

-- 
Julien Grall

[PATCH v3 4/6] xen/arm: implement gic_irq_enable and gic_irq_disable

[Stefano Stabellini]

Rename gic_irq_startup to gic_irq_enable.
Rename gic_irq_shutdown to gic_irq_disable.

Implement gic_irq_startup and gic_irq_shutdown calling gic_irq_enable
and gic_irq_disable.

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

Changes in v2:
- no need to read the current GICD_ISENABLER before writing it.
---
 xen/arch/arm/gic.c |   19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index d736a30..5e60c5a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -129,19 +129,15 @@ void gic_restore_state(struct vcpu *v)
     gic_restore_pending_irqs(v);
 }
 
-static unsigned int gic_irq_startup(struct irq_desc *desc)
+static void gic_irq_enable(struct irq_desc *desc)
 {
-    uint32_t enabler;
     int irq = desc->irq;
 
     /* Enable routing */
-    enabler = GICD[GICD_ISENABLER + irq / 32];
-    GICD[GICD_ISENABLER + irq / 32] = enabler | (1u << (irq % 32));
-
-    return 0;
+    GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
 }
 
-static void gic_irq_shutdown(struct irq_desc *desc)
+static void gic_irq_disable(struct irq_desc *desc)
 {
     int irq = desc->irq;
 
@@ -149,14 +145,15 @@ static void gic_irq_shutdown(struct irq_desc *desc)
     GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
 }
 
-static void gic_irq_enable(struct irq_desc *desc)
+static unsigned int gic_irq_startup(struct irq_desc *desc)
 {
-
+    gic_irq_enable(desc);
+    return 0;
 }
 
-static void gic_irq_disable(struct irq_desc *desc)
+static void gic_irq_shutdown(struct irq_desc *desc)
 {
-
+    gic_irq_disable(desc);
 }
 
 static void gic_irq_ack(struct irq_desc *desc)
-- 
1.7.10.4

Re: [PATCH v3 4/6] xen/arm: implement gic_irq_enable and gic_irq_disable

[Julien Grall]

On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> Rename gic_irq_startup to gic_irq_enable.
> Rename gic_irq_shutdown to gic_irq_disable.
>
> Implement gic_irq_startup and gic_irq_shutdown calling gic_irq_enable
> and gic_irq_disable.
>
> 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>

As specified in patch #5, I would move the different spin_{,un}lock in 
this patch.

-- 
Julien Grall

[PATCH v3 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Stefano Stabellini]

From: Julien Grall <julien.grall@linaro.org>

Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
Enable IRQs when the guest requests it, not unconditionally at boot time.

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

Changes in v2:
- protect startup and shutdown with gic and desc locks.
---
 xen/arch/arm/gic.c  |   46 ++++++++++++++++++++++++++--------------------
 xen/arch/arm/vgic.c |    6 ++----
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5e60c5a..62330dd 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -133,16 +133,26 @@ static void gic_irq_enable(struct irq_desc *desc)
 {
     int irq = desc->irq;
 
+    spin_lock(&desc->lock);
+    spin_lock(&gic.lock);
     /* Enable routing */
     GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
+    desc->status &= ~IRQ_DISABLED;
+    spin_unlock(&gic.lock);
+    spin_unlock(&desc->lock);
 }
 
 static void gic_irq_disable(struct irq_desc *desc)
 {
     int irq = desc->irq;
 
+    spin_lock(&desc->lock);
+    spin_lock(&gic.lock);
     /* Disable routing */
     GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
+    desc->status |= IRQ_DISABLED;
+    spin_unlock(&gic.lock);
+    spin_unlock(&desc->lock);
 }
 
 static unsigned int gic_irq_startup(struct irq_desc *desc)
@@ -247,24 +257,20 @@ static int gic_route_irq(unsigned int irq, bool_t level,
     ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
     ASSERT(irq < gic.lines);      /* Can't route interrupts that don't exist */
 
-    spin_lock_irqsave(&desc->lock, flags);
-    spin_lock(&gic.lock);
-
     if ( desc->action != NULL )
-    {
-        spin_unlock(&gic.lock);
-        spin_unlock(&desc->lock);
         return -EBUSY;
-    }
-
-    desc->handler = &gic_host_irq_type;
 
     /* Disable interrupt */
     desc->handler->shutdown(desc);
 
-    gic_set_irq_properties(irq, level, cpu_mask, priority);
+    spin_lock_irqsave(&desc->lock, flags);
 
+    desc->handler = &gic_host_irq_type;
+
+    spin_lock(&gic.lock);
+    gic_set_irq_properties(irq, level, cpu_mask, priority);
     spin_unlock(&gic.lock);
+
     spin_unlock_irqrestore(&desc->lock, flags);
     return 0;
 }
@@ -557,16 +563,13 @@ void __init release_irq(unsigned int irq)
 
     desc = irq_to_desc(irq);
 
+    desc->handler->shutdown(desc);
+
     spin_lock_irqsave(&desc->lock,flags);
     action = desc->action;
     desc->action  = NULL;
-    desc->status |= IRQ_DISABLED;
     desc->status &= ~IRQ_GUEST;
 
-    spin_lock(&gic.lock);
-    desc->handler->shutdown(desc);
-    spin_unlock(&gic.lock);
-
     spin_unlock_irqrestore(&desc->lock,flags);
 
     /* Wait to make sure it's not being used on another CPU */
@@ -583,11 +586,8 @@ static int __setup_irq(struct irq_desc *desc, unsigned int irq,
         return -EBUSY;
 
     desc->action  = new;
-    desc->status &= ~IRQ_DISABLED;
     dsb();
 
-    desc->handler->startup(desc);
-
     return 0;
 }
 
@@ -600,11 +600,12 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct irqaction *new)
     desc = irq_to_desc(irq->irq);
 
     spin_lock_irqsave(&desc->lock, flags);
-
     rc = __setup_irq(desc, irq->irq, new);
-
     spin_unlock_irqrestore(&desc->lock, flags);
 
+    desc->handler->startup(desc);
+
+
     return rc;
 }
 
@@ -740,6 +741,7 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
     unsigned long flags;
     int retval;
     bool_t level;
+    struct pending_irq *p;
 
     action = xmalloc(struct irqaction);
     if (!action)
@@ -766,6 +768,10 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
         goto out;
     }
 
+    /* TODO: do not assume delivery to vcpu0 */
+    p = irq_to_pending(d->vcpu[0], irq->irq);
+    p->desc = desc;
+
 out:
     spin_unlock(&gic.lock);
     spin_unlock_irqrestore(&desc->lock, flags);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index ab788da..c35661e 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -372,6 +372,8 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
         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);
+        if ( p->desc != NULL )
+            p->desc->handler->enable(p->desc);
         i++;
     }
 }
@@ -694,10 +696,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
     n->irq = irq;
     set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
     n->priority = priority;
-    if (!virtual)
-        n->desc = irq_to_desc(irq);
-    else
-        n->desc = NULL;
 
     /* the irq is enabled */
     if ( test_bit(_GIC_IRQ_GUEST_ENABLED, &n->status) )
-- 
1.7.10.4

Re: [PATCH v3 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Julien Grall]

On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
> Enable IRQs when the guest requests it, not unconditionally at boot time.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Changes in v2:
> - protect startup and shutdown with gic and desc locks.

I think this change can move in patch #4 where you have implemented 
gic_irq_enable/gic_irq_disable.

-- 
Julien Grall

Re: [PATCH v3 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Stefano Stabellini]

On Tue, 10 Dec 2013, Julien Grall wrote:
> On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> > From: Julien Grall <julien.grall@linaro.org>
> > 
> > Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
> > Enable IRQs when the guest requests it, not unconditionally at boot time.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Changes in v2:
> > - protect startup and shutdown with gic and desc locks.
> 
> I think this change can move in patch #4 where you have implemented
> gic_irq_enable/gic_irq_disable.

I could move the gic spin_lock there, but the desc spin_lock needs to
stay in patch #5. Not sure it's worth it.

Re: [PATCH v3 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Julien Grall]

On 12/11/2013 12:50 PM, Stefano Stabellini wrote:
> On Tue, 10 Dec 2013, Julien Grall wrote:
>> On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
>>> From: Julien Grall <julien.grall@linaro.org>
>>>
>>> Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
>>> Enable IRQs when the guest requests it, not unconditionally at boot time.
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> Changes in v2:
>>> - protect startup and shutdown with gic and desc locks.
>>
>> I think this change can move in patch #4 where you have implemented
>> gic_irq_enable/gic_irq_disable.
>
> I could move the gic spin_lock there, but the desc spin_lock needs to
> stay in patch #5. Not sure it's worth it.

Oh right, I didn't notice that __setup_irq is called with all the locks 
taken.

-- 
Julien Grall

Re: [PATCH v3 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Julien Grall]

On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> From: Julien Grall <julien.grall@linaro.org>
>
> Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
> Enable IRQs when the guest requests it, not unconditionally at boot time.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Changes in v2:
> - protect startup and shutdown with gic and desc locks.
> ---
>   xen/arch/arm/gic.c  |   46 ++++++++++++++++++++++++++--------------------
>   xen/arch/arm/vgic.c |    6 ++----
>   2 files changed, 28 insertions(+), 24 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 5e60c5a..62330dd 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -133,16 +133,26 @@ static void gic_irq_enable(struct irq_desc *desc)
>   {
>       int irq = desc->irq;
>
> +    spin_lock(&desc->lock);
> +    spin_lock(&gic.lock);
>       /* Enable routing */
>       GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
> +    desc->status &= ~IRQ_DISABLED;
> +    spin_unlock(&gic.lock);
> +    spin_unlock(&desc->lock);
>   }

I think we should have something like that:

desc->status &= ...
dsb
GICD[...] = ...

As soon as the interrupt is enabled in the distributor, it can be fired. 
With your solution, another CPU (and even this one because the interrupt 
is not disabled), can receive the interrupt. But it won't work because 
the IRQ is marked as disabled.

So you also should disable interrupt here.

-- 
Julien Grall

Re: [PATCH v3 5/6] xen/arm: Only enable physical IRQs when the guest asks

[Stefano Stabellini]

On Wed, 11 Dec 2013, Julien Grall wrote:
> On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> > From: Julien Grall <julien.grall@linaro.org>
> > 
> > Set/Unset IRQ_DISABLED from gic_irq_enable and gic_irq_disable.
> > Enable IRQs when the guest requests it, not unconditionally at boot time.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Changes in v2:
> > - protect startup and shutdown with gic and desc locks.
> > ---
> >   xen/arch/arm/gic.c  |   46 ++++++++++++++++++++++++++--------------------
> >   xen/arch/arm/vgic.c |    6 ++----
> >   2 files changed, 28 insertions(+), 24 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 5e60c5a..62330dd 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -133,16 +133,26 @@ static void gic_irq_enable(struct irq_desc *desc)
> >   {
> >       int irq = desc->irq;
> > 
> > +    spin_lock(&desc->lock);
> > +    spin_lock(&gic.lock);
> >       /* Enable routing */
> >       GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
> > +    desc->status &= ~IRQ_DISABLED;
> > +    spin_unlock(&gic.lock);
> > +    spin_unlock(&desc->lock);
> >   }
> 
> I think we should have something like that:
> 
> desc->status &= ...
> dsb
> GICD[...] = ...
> 
> As soon as the interrupt is enabled in the distributor, it can be fired. With
> your solution, another CPU (and even this one because the interrupt is not
> disabled), can receive the interrupt. But it won't work because the IRQ is
> marked as disabled.
> 
> So you also should disable interrupt here.

I think that you are right on both points. I'll make the changes.

[PATCH v3 6/6] xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

[Stefano Stabellini]

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

Changes in v2:
- disable IRQs that were previously disabled.
---
 xen/arch/arm/vgic.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index c35661e..30a413f 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -360,6 +360,22 @@ read_as_zero:
     return 1;
 }
 
+static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
+{
+    struct pending_irq *p;
+    unsigned int irq;
+    int i = 0;
+
+    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
+        irq = i + (32 * n);
+        p = irq_to_pending(v, irq);
+        clear_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
+        if ( p->desc != NULL )
+            p->desc->handler->disable(p->desc);
+        i++;
+    }
+}
+
 static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     struct pending_irq *p;
@@ -490,8 +506,10 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICENABLER);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
+        tr = rank->ienable;
         rank->ienable &= ~*r;
         vgic_unlock_rank(v, rank);
+        vgic_disable_irqs(v, (*r) & tr, gicd_reg - GICD_ICENABLER);
         return 1;
 
     case GICD_ISPENDR ... GICD_ISPENDRN:
-- 
1.7.10.4

Re: [PATCH v3 6/6] xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ

[Julien Grall]

On 12/10/2013 06:50 PM, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>
>
> Changes in v2:
> - disable IRQs that were previously disabled.
> ---
>   xen/arch/arm/vgic.c |   18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index c35661e..30a413f 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -360,6 +360,22 @@ read_as_zero:
>       return 1;
>   }
>
> +static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> +{
> +    struct pending_irq *p;
> +    unsigned int irq;
> +    int i = 0;
> +
> +    while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) < 32 ) {
> +        irq = i + (32 * n);
> +        p = irq_to_pending(v, irq);
> +        clear_bit(_GIC_IRQ_GUEST_ENABLED, &p->status);
> +        if ( p->desc != NULL )
> +            p->desc->handler->disable(p->desc);
> +        i++;
> +    }
> +}
> +
>   static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>   {
>       struct pending_irq *p;
> @@ -490,8 +506,10 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>           rank = vgic_irq_rank(v, 1, gicd_reg - GICD_ICENABLER);
>           if ( rank == NULL) goto write_ignore;
>           vgic_lock_rank(v, rank);
> +        tr = rank->ienable;
>           rank->ienable &= ~*r;
>           vgic_unlock_rank(v, rank);
> +        vgic_disable_irqs(v, (*r) & tr, gicd_reg - GICD_ICENABLER);
>           return 1;
>
>       case GICD_ISPENDR ... GICD_ISPENDRN:
>

-- 
Julien Grall

Re: [PATCH v3 0/6] interrupt handling fixes

[Julien Grall]

On 12/10/2013 06:49 PM, Stefano Stabellini wrote:
> Hi all,
> this series is a reworked version of "Fix multiple issues with the
> interrupts on ARM":
>
> http://marc.info/?l=xen-devel&m=137211515720144
>
> It fixes a few different issues that affect interrupt handling in Xen on
> ARM today:
>
> - the guest looses a vtimer interrupt notification when it sets a
> deadline in the past from the guest vtimer interrupt handler, before
> EOIing the interrupt;
>
> - Xen adds a guest irq to the LR registers twice if the guest disables
> and renables an interrupt before EOIing it;
>
> - Xen enables interrupts corresponding to devices assigned to dom0
> before booting dom0, resulting in the possibility of receiving an
> interrupt and not knowing what to do with it.

The 2 last issues was found on the versatile express. I gave a try with 
this patch series and it works fine.

For the versatile express:

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

I will give a try to the first bug on the Arndale tomorrow.

> Changes in v3:
> - do not set the GUEST_PENDING bit for evtchn_irq if the irq is already
> guest visible.
>
> Changes in v2:
> - remove eoi variable and check on p->desc != NULL instead;
> - use atomic operations to modify the pending_irq status bits, remove
> the now unnecessary locks;
> - make status unsigned long;
> - in maintenance_interrupt only stops injecting interrupts if no new
> interrupts have been added to the LRs;
> - add a state to keep track whether the guest irq is enabled at the
> vgicd level;
> - no need to read the current GICD_ISENABLER before writing it;
> - protect startup and shutdown with gic and desc locks;
> - disable IRQs that were previously disabled.
>
>
> Julien Grall (2):
>        xen/arm: Physical IRQ is not always equal to virtual IRQ
>        xen/arm: Only enable physical IRQs when the guest asks
>
> Stefano Stabellini (4):
>        xen/arm: track the state of guest IRQs
>        xen/arm: do not add a second irq to the LRs if one is already present
>        xen/arm: implement gic_irq_enable and gic_irq_disable
>        xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ
>
>   xen/arch/arm/gic.c           |  130 +++++++++++++++++++++++++-----------------
>   xen/arch/arm/vgic.c          |   43 +++++++++++---
>   xen/include/asm-arm/domain.h |   40 +++++++++++++
>   3 files changed, 154 insertions(+), 59 deletions(-)
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>

-- 
Julien Grall

Re: [PATCH v3 0/6] interrupt handling fixes

[Ian Campbell]

On Tue, 2013-12-10 at 22:09 +0000, Julien Grall wrote:
> 
> On 12/10/2013 06:49 PM, Stefano Stabellini wrote:
> > Hi all,
> > this series is a reworked version of "Fix multiple issues with the
> > interrupts on ARM":
> >
> > http://marc.info/?l=xen-devel&m=137211515720144
> >
> > It fixes a few different issues that affect interrupt handling in Xen on
> > ARM today:
> >
> > - the guest looses a vtimer interrupt notification when it sets a
> > deadline in the past from the guest vtimer interrupt handler, before
> > EOIing the interrupt;
> >
> > - Xen adds a guest irq to the LR registers twice if the guest disables
> > and renables an interrupt before EOIing it;
> >
> > - Xen enables interrupts corresponding to devices assigned to dom0
> > before booting dom0, resulting in the possibility of receiving an
> > interrupt and not knowing what to do with it.
> 
> The 2 last issues was found on the versatile express. I gave a try with 
> this patch series and it works fine.
> 
> For the versatile express:
> 
> Tested-by: Julien Grall <julien.grall@linaro.org>

I tried it on the X-gene too.

> 
> I will give a try to the first bug on the Arndale tomorrow.
> 
> > Changes in v3:
> > - do not set the GUEST_PENDING bit for evtchn_irq if the irq is already
> > guest visible.
> >
> > Changes in v2:
> > - remove eoi variable and check on p->desc != NULL instead;
> > - use atomic operations to modify the pending_irq status bits, remove
> > the now unnecessary locks;
> > - make status unsigned long;
> > - in maintenance_interrupt only stops injecting interrupts if no new
> > interrupts have been added to the LRs;
> > - add a state to keep track whether the guest irq is enabled at the
> > vgicd level;
> > - no need to read the current GICD_ISENABLER before writing it;
> > - protect startup and shutdown with gic and desc locks;
> > - disable IRQs that were previously disabled.
> >
> >
> > Julien Grall (2):
> >        xen/arm: Physical IRQ is not always equal to virtual IRQ
> >        xen/arm: Only enable physical IRQs when the guest asks
> >
> > Stefano Stabellini (4):
> >        xen/arm: track the state of guest IRQs
> >        xen/arm: do not add a second irq to the LRs if one is already present
> >        xen/arm: implement gic_irq_enable and gic_irq_disable
> >        xen/arm: disable a physical IRQ when the guest disables the corresponding IRQ
> >
> >   xen/arch/arm/gic.c           |  130 +++++++++++++++++++++++++-----------------
> >   xen/arch/arm/vgic.c          |   43 +++++++++++---
> >   xen/include/asm-arm/domain.h |   40 +++++++++++++
> >   3 files changed, 154 insertions(+), 59 deletions(-)
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> >
>