thr3ads.net - Xen devel - [PATCH v4 0/6] interrupt handling fixes [Dec 2013]

If this information is useful, please help other people find it:
Share via:

Stefano Stabellini

2013-Dec-11 19:06 UTC

[PATCH v4 0/6] interrupt handling fixes

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 v4:
- move set_bit _GIC_IRQ_GUEST_VISIBLE and clear_bit
_GIC_IRQ_GUEST_PENDING to gic_set_lr;
- turn set_int into a bool_t;
- remove raw GIC_IRQ_GUEST values;
- in maintenance_interrupt if the irq is still PENDING, add it back into
the lr_pending queue instead of immediately reinjecting it;
- disable interrupts in gic_irq_enable;
- clear IRQ_DISABLED and dsb() before writing to the GICD register.

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           |  134 ++++++++++++++++++++++++++----------------
 xen/arch/arm/vgic.c          |   43 +++++++++++---
 xen/include/asm-arm/domain.h |   37 ++++++++++++
 3 files changed, 156 insertions(+), 58 deletions(-)

Stefano Stabellini

2013-Dec-11 19:07 UTC

head link

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

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

Stefano Stabellini

2013-Dec-11 19:07 UTC

head link

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

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 add the irq back into
the lr_pending queue so that it''s going to be reinjected 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 stop 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.

Changes in v4:
- move set_bit _GIC_IRQ_GUEST_VISIBLE and clear_bit
_GIC_IRQ_GUEST_PENDING to gic_set_lr;
- turn set_int into a bool_t;
- remove raw GIC_IRQ_GUEST values;
- in maintenance_interrupt if the irq is still PENDING, add it back into
the lr_pending queue instead of immediately reinjecting it.
---
 xen/arch/arm/gic.c           |   54 ++++++++++++++++++++++++++++++++----------
 xen/arch/arm/vgic.c          |   17 ++++++++++---
 xen/include/asm-arm/domain.h |   37 +++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5f7a548..19e0ae4 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -615,6 +615,7 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
         unsigned int state, unsigned int priority)
 {
     int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
+    struct pending_irq *p = irq_to_pending(current, virtual_irq);
 
     BUG_ON(lr >= nr_lrs);
     BUG_ON(lr < 0);
@@ -624,6 +625,9 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
         maintenance_int |
         ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
         ((virtual_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);
 }
 
 void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
@@ -884,10 +888,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);
+    bool_t 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, *iter;
         int cpu;
 
         cpu = -1;
@@ -898,17 +903,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 +910,36 @@ 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))
+        {
+            BUG_ON(!list_empty(&p->lr_queue));
+
+            list_for_each_entry ( iter, &v->arch.vgic.lr_pending,
lr_queue )
+            {
+                if ( iter->priority > p->priority )
+                {
+                    list_add_tail(&p->lr_queue, &iter->lr_queue);
+                    goto out;
+                }
+            }
+            list_add_tail(&p->lr_queue,
&v->arch.vgic.lr_pending);
+out:
+            set_int = 1;
+        }
+
+        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);
+            list_del_init(&p2->lr_queue);
+            set_bit(i, &this_cpu(lr_mask));
+            set_int = 1;
+        }
+        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 +955,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..b449f45 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..0733c5e 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -22,6 +22,43 @@ 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_VISIBLE  1
+#define GIC_IRQ_GUEST_ENABLED  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

Stefano Stabellini

2013-Dec-11 19:07 UTC

head link

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

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

Stefano Stabellini

2013-Dec-11 19:07 UTC

head link

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

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 19e0ae4..631013b 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

Stefano Stabellini

2013-Dec-11 19:07 UTC

head link

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

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.

Changes in v4:
- disable interrupts in gic_irq_enable;
- clear IRQ_DISABLED and dsb() before writing to the GICD register.
---
 xen/arch/arm/gic.c  |   48 ++++++++++++++++++++++++++++--------------------
 xen/arch/arm/vgic.c |    6 ++----
 2 files changed, 30 insertions(+), 24 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 631013b..81b7c9d 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -132,17 +132,29 @@ void gic_restore_state(struct vcpu *v)
 static void gic_irq_enable(struct irq_desc *desc)
 {
     int irq = desc->irq;
+    unsigned long flags;
 
+    spin_lock_irqsave(&desc->lock, flags);
+    spin_lock(&gic.lock);
+    desc->status &= ~IRQ_DISABLED;
+    dsb();
     /* Enable routing */
     GICD[GICD_ISENABLER + irq / 32] = (1u << (irq % 32));
+    spin_unlock(&gic.lock);
+    spin_unlock_irqrestore(&desc->lock, flags);
 }
 
 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 +259,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 +565,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 +588,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 +602,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;
 }
 
@@ -739,6 +742,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)
@@ -765,6 +769,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 f2f1702..ad17d1d 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

Stefano Stabellini

2013-Dec-11 19:07 UTC

head link

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

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

Ian Campbell

2013-Dec-12 12:03 UTC

head link

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

On Wed, 2013-12-11 at 19:07 +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.
> 
> 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 add the irq back into
> the lr_pending queue so that it''s going to be reinjected 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 stop 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.
> 
> Changes in v4:
> - move set_bit _GIC_IRQ_GUEST_VISIBLE and clear_bit
> _GIC_IRQ_GUEST_PENDING to gic_set_lr;
> - turn set_int into a bool_t;
> - remove raw GIC_IRQ_GUEST values;
> - in maintenance_interrupt if the irq is still PENDING, add it back into
> the lr_pending queue instead of immediately reinjecting it.
> ---
>  xen/arch/arm/gic.c           |   54
++++++++++++++++++++++++++++++++----------
>  xen/arch/arm/vgic.c          |   17 ++++++++++---
>  xen/include/asm-arm/domain.h |   37 +++++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+), 15 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 5f7a548..19e0ae4 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -615,6 +615,7 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
>          unsigned int state, unsigned int priority)
>  {
>      int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> +    struct pending_irq *p = irq_to_pending(current, virtual_irq);
>  
>      BUG_ON(lr >= nr_lrs);
>      BUG_ON(lr < 0);
> @@ -624,6 +625,9 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
>          maintenance_int |
>          ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
>          ((virtual_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);
So WRT races in vgic_vcpu_inject_irq we were discussing on v3. The above
vs:

+        if ( (irq != current->domain->arch.evtchn_irq) ||
+             (!test_bit(_GIC_IRQ_GUEST_VISIBLE, &n->status)) )
+            set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);

If one cpu (A) is in gic_set_lr while another (B) is in
vgic_vcpu_inject_irq, is there any ordering which fails to do the right
thing.

        A -- set VISIBLE
        B -- test VISIBLE
        B -- set PENDING
        A -- clear PENDING

Doesn''t seem right?

        B -- test VISIBLE
        A -- set VISIBLE
        A -- clear PENDING
        B -- doesn''t set PENDING
        
Seems OK.

        A -- set VISIBLE
    B -- test VISIBLE
    A -- clear PENDING
    B -- set PENDING

OK?

        B -- test VISIBLE
    A -- set VISIBLE
        B -- set PENDING
        A -- clear PENDING
   
Wrong?

If A clear''s pending first does that work out right every time?

Ian.

Ian Campbell

2013-Dec-12 12:18 UTC

head link

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

On Wed, 2013-12-11 at 19:07 +0000, Stefano Stabellini
wrote:> @@ -916,6 +910,36 @@ 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))
> +        {
> +            BUG_ON(!list_empty(&p->lr_queue));
> +
> +            list_for_each_entry ( iter, &v->arch.vgic.lr_pending,
lr_queue )
> +            {
> +                if ( iter->priority > p->priority )
> +                {
> +                    list_add_tail(&p->lr_queue,
&iter->lr_queue);
> +                    goto out;
> +                }
> +            }
> +            list_add_tail(&p->lr_queue,
&v->arch.vgic.lr_pending);
> +out:
Is there really no helper function for all that? It looks very similar
to the tail of gic_set_guest_irq.
> +            set_int = 1;
> +        }
> +
> +        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);
> +            list_del_init(&p2->lr_queue);
> +            set_bit(i, &this_cpu(lr_mask));
> +            set_int = 1;
> +        }
> +        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 +955,12 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>  
>          i++;
>      }
> +    if ( !set_int )
> +    {
> +        spin_lock_irq(&gic.lock);
Dropping the lock and then retaking it here is a bit of a suspicious
pattern -- what if something changes in the interim?

Ian.

Stefano Stabellini

2013-Dec-12 15:09 UTC

head link

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

On Thu, 12 Dec 2013, Ian Campbell wrote:> On Wed, 2013-12-11 at 19:07 +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.
> > 
> > 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 add the irq back
into
> > the lr_pending queue so that it''s going to be reinjected 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 stop 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.
> > 
> > Changes in v4:
> > - move set_bit _GIC_IRQ_GUEST_VISIBLE and clear_bit
> > _GIC_IRQ_GUEST_PENDING to gic_set_lr;
> > - turn set_int into a bool_t;
> > - remove raw GIC_IRQ_GUEST values;
> > - in maintenance_interrupt if the irq is still PENDING, add it back
into
> > the lr_pending queue instead of immediately reinjecting it.
> > ---
> >  xen/arch/arm/gic.c           |   54
++++++++++++++++++++++++++++++++----------
> >  xen/arch/arm/vgic.c          |   17 ++++++++++---
> >  xen/include/asm-arm/domain.h |   37 +++++++++++++++++++++++++++++
> >  3 files changed, 93 insertions(+), 15 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 5f7a548..19e0ae4 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -615,6 +615,7 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
> >          unsigned int state, unsigned int priority)
> >  {
> >      int maintenance_int = GICH_LR_MAINTENANCE_IRQ;
> > +    struct pending_irq *p = irq_to_pending(current, virtual_irq);
> >  
> >      BUG_ON(lr >= nr_lrs);
> >      BUG_ON(lr < 0);
> > @@ -624,6 +625,9 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
> >          maintenance_int |
> >          ((priority >> 3) << GICH_LR_PRIORITY_SHIFT) |
> >          ((virtual_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);
> 
> So WRT races in vgic_vcpu_inject_irq we were discussing on v3. The above
> vs:
> 
> +        if ( (irq != current->domain->arch.evtchn_irq) ||
> +             (!test_bit(_GIC_IRQ_GUEST_VISIBLE, &n->status)) )
> +            set_bit(_GIC_IRQ_GUEST_PENDING, &n->status);
> 
> If one cpu (A) is in gic_set_lr while another (B) is in
> vgic_vcpu_inject_irq, is there any ordering which fails to do the right
> thing.
> 
>         A -- set VISIBLE
>         B -- test VISIBLE
>         B -- set PENDING
>         A -- clear PENDING
> 
> Doesn''t seem right?
You got the test wrong.
Assuming irq != evtchn_irq, it would be:

A -- set VISIBLE
B -- set PENDING
A -- clear PENDING

So the guest would loose a potential future interrupt notification. On
the other hand the guest hasn''t acked the interrupt yet, so it is not
supposed to be able to receive a second interrupt notification at this
time. Moreover consider that a second interrupt at this time can only
happen with hardware edge-trigger interrupts. This race exists even on
bare metal between the second interrupt and the guest ACKing the first.
I think it should be OK.

>         B -- test VISIBLE
>         A -- set VISIBLE
>         A -- clear PENDING
>         B -- doesn''t set PENDING
>         
> Seems OK.
Still assuming irq != evtchn_irq, the other case would be:

A -- set VISIBLE
A -- clear PENDING
B -- set PENDING

so the guest would get one notification more from the hardware.
It can only happen with hardware edge-trigger interrupts.

Stefano Stabellini

2013-Dec-12 15:17 UTC

head link

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

On Thu, 12 Dec 2013, Ian Campbell wrote:> On Wed, 2013-12-11 at 19:07 +0000, Stefano Stabellini wrote:
> > @@ -916,6 +910,36 @@ 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))
> > +        {
> > +            BUG_ON(!list_empty(&p->lr_queue));
> > +
> > +            list_for_each_entry ( iter,
&v->arch.vgic.lr_pending, lr_queue )
> > +            {
> > +                if ( iter->priority > p->priority )
> > +                {
> > +                    list_add_tail(&p->lr_queue,
&iter->lr_queue);
> > +                    goto out;
> > +                }
> > +            }
> > +            list_add_tail(&p->lr_queue,
&v->arch.vgic.lr_pending);
> > +out:
> 
> Is there really no helper function for all that? It looks very similar
> to the tail of gic_set_guest_irq.
That''s a good point, I can add an helper.

> > +            set_int = 1;
> > +        }
> > +
> > +        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);
> > +            list_del_init(&p2->lr_queue);
> > +            set_bit(i, &this_cpu(lr_mask));
> > +            set_int = 1;
> > +        }
> > +        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 +955,12 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
> >  
> >          i++;
> >      }
> > +    if ( !set_int )
> > +    {
> > +        spin_lock_irq(&gic.lock);
> 
> Dropping the lock and then retaking it here is a bit of a suspicious
> pattern -- what if something changes in the interim?
Vcpu A has EOIed a guest IRQ, causing a maintenance_interrupt.
At the same time vcpu B is trying to inject a new interrupt into the
vcpu A.


A: maintenance_interrupt: set_int = 0
B: create a new inflight irq for A
B: send an IPI
A: receive an IPI
A: gic_inject->gic_inject_irq_start
A: maintenance_interrupt: gic_inject_irq_stop
A: gic_inject->gic_inject_irq_start


It works! :)

Ian Campbell

2013-Dec-12 15:23 UTC

head link

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

On Thu, 2013-12-12 at 15:17 +0000, Stefano Stabellini wrote:
> > > +            set_int = 1;
> > > +        }
> > > +
> > > +        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);
> > > +            list_del_init(&p2->lr_queue);
> > > +            set_bit(i, &this_cpu(lr_mask));
> > > +            set_int = 1;
> > > +        }
> > > +        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 +955,12 @@ static void maintenance_interrupt(int irq,
void *dev_id, struct cpu_user_regs *r
> > >  
> > >          i++;
> > >      }
> > > +    if ( !set_int )
> > > +    {
> > > +        spin_lock_irq(&gic.lock);
> > 
> > Dropping the lock and then retaking it here is a bit of a suspicious
> > pattern -- what if something changes in the interim?
> 
> Vcpu A has EOIed a guest IRQ, causing a maintenance_interrupt.
> At the same time vcpu B is trying to inject a new interrupt into the
> vcpu A.
> 
> 
> A: maintenance_interrupt: set_int = 0
> B: create a new inflight irq for A
> B: send an IPI
> A: receive an IPI
> A: gic_inject->gic_inject_irq_start
> A: maintenance_interrupt: gic_inject_irq_stop
> A: gic_inject->gic_inject_irq_start
> 
> 
> It works! :)
What is the last "A: gic_inject->gic_inject_irq_start" from?

Stefano Stabellini

2013-Dec-12 15:25 UTC

head link

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

On Thu, 12 Dec 2013, Ian Campbell wrote:> On Thu, 2013-12-12 at 15:17 +0000, Stefano Stabellini wrote:
> 
> > > > +            set_int = 1;
> > > > +        }
> > > > +
> > > > +        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);
> > > > +            list_del_init(&p2->lr_queue);
> > > > +            set_bit(i, &this_cpu(lr_mask));
> > > > +            set_int = 1;
> > > > +        }
> > > > +        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 +955,12 @@ static void maintenance_interrupt(int
irq, void *dev_id, struct cpu_user_regs *r
> > > >  
> > > >          i++;
> > > >      }
> > > > +    if ( !set_int )
> > > > +    {
> > > > +        spin_lock_irq(&gic.lock);
> > > 
> > > Dropping the lock and then retaking it here is a bit of a
suspicious
> > > pattern -- what if something changes in the interim?
> > 
> > Vcpu A has EOIed a guest IRQ, causing a maintenance_interrupt.
> > At the same time vcpu B is trying to inject a new interrupt into the
> > vcpu A.
> > 
> > 
> > A: maintenance_interrupt: set_int = 0
> > B: create a new inflight irq for A
> > B: send an IPI
> > A: receive an IPI
> > A: gic_inject->gic_inject_irq_start
> > A: maintenance_interrupt: gic_inject_irq_stop
> > A: gic_inject->gic_inject_irq_start
> > 
> > 
> > It works! :)
> 
> What is the last "A: gic_inject->gic_inject_irq_start" from?
leave_hypervisor_tail->gic_inject->gic_inject_irq_start

Ian Campbell

2013-Dec-12 15:45 UTC

head link

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

On Thu, 2013-12-12 at 15:25 +0000, Stefano Stabellini
wrote:> On Thu, 12 Dec 2013, Ian Campbell wrote:
> > On Thu, 2013-12-12 at 15:17 +0000, Stefano Stabellini wrote:
> > 
> > > > > +            set_int = 1;
> > > > > +        }
> > > > > +
> > > > > +        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);
> > > > > +            list_del_init(&p2->lr_queue);
> > > > > +            set_bit(i, &this_cpu(lr_mask));
> > > > > +            set_int = 1;
> > > > > +        }
> > > > > +        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 +955,12 @@ static void
maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
> > > > >  
> > > > >          i++;
> > > > >      }
> > > > > +    if ( !set_int )
> > > > > +    {
> > > > > +        spin_lock_irq(&gic.lock);
> > > > 
> > > > Dropping the lock and then retaking it here is a bit of a
suspicious
> > > > pattern -- what if something changes in the interim?
> > > 
> > > Vcpu A has EOIed a guest IRQ, causing a maintenance_interrupt.
> > > At the same time vcpu B is trying to inject a new interrupt into
the
> > > vcpu A.
> > > 
> > > 
> > > A: maintenance_interrupt: set_int = 0
> > > B: create a new inflight irq for A
> > > B: send an IPI
> > > A: receive an IPI
> > > A: gic_inject->gic_inject_irq_start
> > > A: maintenance_interrupt: gic_inject_irq_stop
> > > A: gic_inject->gic_inject_irq_start
> > > 
> > > 
> > > It works! :)
> > 
> > What is the last "A: gic_inject->gic_inject_irq_start"
from?
> 
> leave_hypervisor_tail->gic_inject->gic_inject_irq_start
Can''t the stop in the maintenance interrupt be unconditional then? If
there are additional interrupts pending then they will always be picked
up via leave_hypervisor_tail, which is a nice and simple rule to
remember.

Ian.

Ian Campbell

2013-Dec-12 15:46 UTC

head link

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

On Thu, 2013-12-12 at 15:45 +0000, Ian Campbell wrote:> On Thu, 2013-12-12 at 15:25 +0000, Stefano Stabellini wrote:
> > On Thu, 12 Dec 2013, Ian Campbell wrote:
> > > On Thu, 2013-12-12 at 15:17 +0000, Stefano Stabellini wrote:
> > > 
> > > > > > +            set_int = 1;
> > > > > > +        }
> > > > > > +
> > > > > > +        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);
> > > > > > +            list_del_init(&p2->lr_queue);
> > > > > > +            set_bit(i, &this_cpu(lr_mask));
> > > > > > +            set_int = 1;
> > > > > > +        }
> > > > > > +        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 +955,12 @@ static void
maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
> > > > > >  
> > > > > >          i++;
> > > > > >      }
> > > > > > +    if ( !set_int )
> > > > > > +    {
> > > > > > +        spin_lock_irq(&gic.lock);
> > > > > 
> > > > > Dropping the lock and then retaking it here is a bit of
a suspicious
> > > > > pattern -- what if something changes in the interim?
> > > > 
> > > > Vcpu A has EOIed a guest IRQ, causing a
maintenance_interrupt.
> > > > At the same time vcpu B is trying to inject a new interrupt
into the
> > > > vcpu A.
> > > > 
> > > > 
> > > > A: maintenance_interrupt: set_int = 0
> > > > B: create a new inflight irq for A
> > > > B: send an IPI
> > > > A: receive an IPI
> > > > A: gic_inject->gic_inject_irq_start
> > > > A: maintenance_interrupt: gic_inject_irq_stop
> > > > A: gic_inject->gic_inject_irq_start
> > > > 
> > > > 
> > > > It works! :)
> > > 
> > > What is the last "A:
gic_inject->gic_inject_irq_start" from?
> > 
> > leave_hypervisor_tail->gic_inject->gic_inject_irq_start
> 
> Can''t the stop in the maintenance interrupt be unconditional then?
If
> there are additional interrupts pending then they will always be picked
> up via leave_hypervisor_tail, which is a nice and simple rule to
> remember.
Or going one step further -- why isn''t leave_hypervisor_tail the only
place which touches HCR_VI at all? That''s a very simple rule...
> 
> Ian.

Stefano Stabellini

2013-Dec-12 15:48 UTC

head link

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

On Thu, 12 Dec 2013, Ian Campbell wrote:> On Thu, 2013-12-12 at 15:25 +0000, Stefano Stabellini wrote:
> > On Thu, 12 Dec 2013, Ian Campbell wrote:
> > > On Thu, 2013-12-12 at 15:17 +0000, Stefano Stabellini wrote:
> > > 
> > > > > > +            set_int = 1;
> > > > > > +        }
> > > > > > +
> > > > > > +        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);
> > > > > > +            list_del_init(&p2->lr_queue);
> > > > > > +            set_bit(i, &this_cpu(lr_mask));
> > > > > > +            set_int = 1;
> > > > > > +        }
> > > > > > +        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 +955,12 @@ static void
maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
> > > > > >  
> > > > > >          i++;
> > > > > >      }
> > > > > > +    if ( !set_int )
> > > > > > +    {
> > > > > > +        spin_lock_irq(&gic.lock);
> > > > > 
> > > > > Dropping the lock and then retaking it here is a bit of
a suspicious
> > > > > pattern -- what if something changes in the interim?
> > > > 
> > > > Vcpu A has EOIed a guest IRQ, causing a
maintenance_interrupt.
> > > > At the same time vcpu B is trying to inject a new interrupt
into the
> > > > vcpu A.
> > > > 
> > > > 
> > > > A: maintenance_interrupt: set_int = 0
> > > > B: create a new inflight irq for A
> > > > B: send an IPI
> > > > A: receive an IPI
> > > > A: gic_inject->gic_inject_irq_start
> > > > A: maintenance_interrupt: gic_inject_irq_stop
> > > > A: gic_inject->gic_inject_irq_start
> > > > 
> > > > 
> > > > It works! :)
> > > 
> > > What is the last "A:
gic_inject->gic_inject_irq_start" from?
> > 
> > leave_hypervisor_tail->gic_inject->gic_inject_irq_start
> 
> Can''t the stop in the maintenance interrupt be unconditional then?
If
> there are additional interrupts pending then they will always be picked
> up via leave_hypervisor_tail, which is a nice and simple rule to
> remember. 
I would rather just remove the gic_inject_irq_stop call from
maintenance_interrupt because similarly gic_inject will figure out if it
needs to call gic_inject_irq_stop.

Stefano Stabellini

2013-Dec-12 15:50 UTC

head link

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

On Thu, 12 Dec 2013, Ian Campbell wrote:> On Thu, 2013-12-12 at 15:45 +0000, Ian Campbell wrote:
> > On Thu, 2013-12-12 at 15:25 +0000, Stefano Stabellini wrote:
> > > On Thu, 12 Dec 2013, Ian Campbell wrote:
> > > > On Thu, 2013-12-12 at 15:17 +0000, Stefano Stabellini wrote:
> > > > 
> > > > > > > +            set_int = 1;
> > > > > > > +        }
> > > > > > > +
> > > > > > > +        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);
> > > > > > > +           
list_del_init(&p2->lr_queue);
> > > > > > > +            set_bit(i,
&this_cpu(lr_mask));
> > > > > > > +            set_int = 1;
> > > > > > > +        }
> > > > > > > +        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 +955,12 @@ static void
maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
> > > > > > >  
> > > > > > >          i++;
> > > > > > >      }
> > > > > > > +    if ( !set_int )
> > > > > > > +    {
> > > > > > > +        spin_lock_irq(&gic.lock);
> > > > > > 
> > > > > > Dropping the lock and then retaking it here is a
bit of a suspicious
> > > > > > pattern -- what if something changes in the
interim?
> > > > > 
> > > > > Vcpu A has EOIed a guest IRQ, causing a
maintenance_interrupt.
> > > > > At the same time vcpu B is trying to inject a new
interrupt into the
> > > > > vcpu A.
> > > > > 
> > > > > 
> > > > > A: maintenance_interrupt: set_int = 0
> > > > > B: create a new inflight irq for A
> > > > > B: send an IPI
> > > > > A: receive an IPI
> > > > > A: gic_inject->gic_inject_irq_start
> > > > > A: maintenance_interrupt: gic_inject_irq_stop
> > > > > A: gic_inject->gic_inject_irq_start
> > > > > 
> > > > > 
> > > > > It works! :)
> > > > 
> > > > What is the last "A:
gic_inject->gic_inject_irq_start" from?
> > > 
> > > leave_hypervisor_tail->gic_inject->gic_inject_irq_start
> > 
> > Can''t the stop in the maintenance interrupt be unconditional
then? If
> > there are additional interrupts pending then they will always be
picked
> > up via leave_hypervisor_tail, which is a nice and simple rule to
> > remember.
> 
> Or going one step further -- why isn''t leave_hypervisor_tail the
only
> place which touches HCR_VI at all? That''s a very simple rule...
I agree. I''ll make the change.

Ian Campbell

2013-Dec-12 15:56 UTC

head link

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

On Wed, 2013-12-11 at 19:07 +0000, Stefano Stabellini
wrote:> @@ -916,6 +910,36 @@ 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))
> +        {
> +            BUG_ON(!list_empty(&p->lr_queue));
> +
> +            list_for_each_entry ( iter, &v->arch.vgic.lr_pending,
lr_queue )
> +            {
> +                if ( iter->priority > p->priority )
> +                {
> +                    list_add_tail(&p->lr_queue,
&iter->lr_queue);
> +                    goto out;
> +                }
> +            }
> +            list_add_tail(&p->lr_queue,
&v->arch.vgic.lr_pending);
> +out:
> +            set_int = 1;
> +        }
> +
> +        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);
> +            list_del_init(&p2->lr_queue);
> +            set_bit(i, &this_cpu(lr_mask));
> +            set_int = 1;
> +        }
> +        spin_unlock_irq(&gic.lock);
> +
> +        spin_lock_irq(&v->arch.vgic.lock);
>          list_del_init(&p->inflight);
Is it correct that we remove the IRQ from the inflight list here even in
the case where we have reinjected it above?

Ian.

Stefano Stabellini

2013-Dec-12 16:06 UTC

head link

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

On Thu, 12 Dec 2013, Ian Campbell wrote:> On Wed, 2013-12-11 at 19:07 +0000, Stefano Stabellini wrote:
> > @@ -916,6 +910,36 @@ 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))
> > +        {
> > +            BUG_ON(!list_empty(&p->lr_queue));
> > +
> > +            list_for_each_entry ( iter,
&v->arch.vgic.lr_pending, lr_queue )
> > +            {
> > +                if ( iter->priority > p->priority )
> > +                {
> > +                    list_add_tail(&p->lr_queue,
&iter->lr_queue);
> > +                    goto out;
> > +                }
> > +            }
> > +            list_add_tail(&p->lr_queue,
&v->arch.vgic.lr_pending);
> > +out:
> > +            set_int = 1;
> > +        }
> > +
> > +        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);
> > +            list_del_init(&p2->lr_queue);
> > +            set_bit(i, &this_cpu(lr_mask));
> > +            set_int = 1;
> > +        }
> > +        spin_unlock_irq(&gic.lock);
> > +
> > +        spin_lock_irq(&v->arch.vgic.lock);
> >          list_del_init(&p->inflight);
> 
> Is it correct that we remove the IRQ from the inflight list here even in
> the case where we have reinjected it above?
Well spotted! This bug was introduced in v4. I''ll fix it.

Ian Campbell

2013-Dec-12 16:06 UTC

head link

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

On Wed, 2013-12-11 at 19:07 +0000, 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 ad17d1d..9022658 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);
Do we need to remove from some queues, like lr_pending? In case it is
already pending when it gets masked we don''t want to pick it off the
queue in the maint interrupt do we?

Ian.

Stefano Stabellini

2013-Dec-12 16:34 UTC

head link

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

On Thu, 12 Dec 2013, Ian Campbell wrote:> On Wed, 2013-12-11 at 19:07 +0000, 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 ad17d1d..9022658 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);
> 
> Do we need to remove from some queues, like lr_pending? In case it is
> already pending when it gets masked we don''t want to pick it off
the
> queue in the maint interrupt do we?
I think that you are right. Removing irqs from lr_pending should be all
that is needed to solve this problem.

Xen devel - Dec 2013 - [PATCH v4 0/6] interrupt handling fixes

[PATCH v4 0/6] interrupt handling fixes

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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