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

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

Stefano Stabellini

2013-Dec-06 17:26 UTC

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


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           |   82 +++++++++++++++++++++++++++---------------
 xen/arch/arm/vgic.c          |   42 ++++++++++++++++++----
 xen/include/asm-arm/domain.h |   29 +++++++++++++++
 3 files changed, 118 insertions(+), 35 deletions(-)

Stefano Stabellini

2013-Dec-06 17:26 UTC

head link

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

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

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.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 43c11cb..7e87acb 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;
     uint32_t lr;
     struct vcpu *v = current;
     uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) <<
32);
@@ -916,6 +916,7 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
             /* 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);
@@ -924,10 +925,10 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
             /* 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-06 17:26 UTC

head link

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

Introduce a status field in struct pending_irq. Valid states are
GUEST_PENDING and GUEST_VISIBLE and they are not mutually exclusive.
See the in-code comment for an explanation of the states and how they
are used. Protect pending_irq state manipulations with the vgic lock.

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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic.c           |   49 ++++++++++++++++++++++++++++++------------
 xen/arch/arm/vgic.c          |   17 +++++++++++++--
 xen/include/asm-arm/domain.h |   29 +++++++++++++++++++++++++
 3 files changed, 79 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 7e87acb..a8a5c2a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -626,6 +626,7 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
         ((virtual_irq & GICH_LR_VIRTUAL_MASK) <<
GICH_LR_VIRTUAL_SHIFT);
 }
 
+/* needs to be called with vgic lock held */
 void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
         unsigned int state, unsigned int priority)
 {
@@ -635,17 +636,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);
+            n->status |= GIC_IRQ_GUEST_VISIBLE;
+            n->status &= ~GIC_IRQ_GUEST_PENDING;
             goto out;
         }
     }
 
-    n = irq_to_pending(v, virtual_irq);
     if ( !list_empty(&n->lr_queue) )
         goto out;
 
@@ -679,6 +683,8 @@ static void gic_restore_pending_irqs(struct vcpu *v)
         gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
         list_del_init(&p->lr_queue);
         set_bit(i, &this_cpu(lr_mask));
+        p->status |= GIC_IRQ_GUEST_VISIBLE;
+        p->status &= ~GIC_IRQ_GUEST_PENDING;
         spin_unlock_irqrestore(&gic.lock, flags);
     }
 
@@ -884,6 +890,7 @@ 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 long flags;
 
     while ((i = find_next_bit((const long unsigned int *) &eisr,
                               64, i)) < 64) {
@@ -893,23 +900,13 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
         cpu = -1;
         eoi = 0;
 
-        spin_lock_irq(&gic.lock);
+        spin_lock(&v->arch.vgic.lock);
+        spin_lock_irqsave(&gic.lock, flags);
         lr = GICH[GICH_LR + i];
         virq = lr & GICH_LR_VIRTUAL_MASK;
         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;
@@ -918,8 +915,32 @@ static void maintenance_interrupt(int irq, void *dev_id,
struct cpu_user_regs *r
             eoi = 1;
             pirq = p->desc->irq;
         }
+        if ( p->status & GIC_IRQ_GUEST_PENDING )
+        {
+            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
+            p->status &= ~GIC_IRQ_GUEST_PENDING;
+            i++;
+            spin_unlock_irqrestore(&gic.lock, flags);
+            spin_unlock(&v->arch.vgic.lock);
+            continue;
+        }
+
+        p->status &= ~GIC_IRQ_GUEST_VISIBLE;
         list_del_init(&p->inflight);
-        spin_unlock_irq(&v->arch.vgic.lock);
+
+        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);
+            p->status |= GIC_IRQ_GUEST_VISIBLE;
+            p->status &= ~GIC_IRQ_GUEST_PENDING;
+            list_del_init(&p->lr_queue);
+            set_bit(i, &this_cpu(lr_mask));
+        } else {
+            gic_inject_irq_stop();
+        }
+
+        spin_unlock_irqrestore(&gic.lock, flags);
+        spin_unlock(&v->arch.vgic.lock);
 
         if ( eoi ) {
             /* this is not racy because we can''t receive another irq
of the
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 2e4b11f..c71db37 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -365,6 +365,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
n)
     struct pending_irq *p;
     unsigned int irq;
     int i = 0;
+    unsigned long f;
+
+    spin_lock_irqsave(&v->arch.vgic.lock, f);
 
     while ( (i = find_next_bit((const long unsigned int *) &r, 32, i)) <
32 ) {
         irq = i + (32 * n);
@@ -373,6 +376,8 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
n)
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
         i++;
     }
+
+    spin_unlock_irqrestore(&v->arch.vgic.lock, f);
 }
 
 static inline int is_vcpu_running(struct domain *d, int vcpuid)
@@ -672,8 +677,15 @@ 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) )
+    {
+        n->status |= GIC_IRQ_GUEST_PENDING;
+        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 +694,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;
+    n->status = GIC_IRQ_GUEST_PENDING;
     n->priority = priority;
     if (!virtual)
         n->desc = irq_to_desc(irq);
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d5cae2e..5e7bb58 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -22,6 +22,35 @@ 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.
+     *
+     */
+#define GIC_IRQ_GUEST_PENDING  (1<<1)
+#define GIC_IRQ_GUEST_VISIBLE   (1<<2)
+    int 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-06 17:26 UTC

head link

[PATCH 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>
---
 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 c71db37..969ad2d 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -372,7 +372,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);
-        if ( !list_empty(&p->inflight) )
+        if ( !list_empty(&p->inflight) && !(p->status &
GIC_IRQ_GUEST_VISIBLE) )
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
         i++;
     }
-- 
1.7.10.4

Stefano Stabellini

2013-Dec-06 17:26 UTC

head link

[PATCH 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>
---
 xen/arch/arm/gic.c |   15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index a8a5c2a..5ec3ff9 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -129,7 +129,7 @@ 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;
@@ -137,11 +137,9 @@ static unsigned int gic_irq_startup(struct irq_desc *desc)
     /* Enable routing */
     enabler = GICD[GICD_ISENABLER + irq / 32];
     GICD[GICD_ISENABLER + irq / 32] = enabler | (1u << (irq % 32));
-
-    return 0;
 }
 
-static void gic_irq_shutdown(struct irq_desc *desc)
+static void gic_irq_disable(struct irq_desc *desc)
 {
     int irq = desc->irq;
 
@@ -149,14 +147,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-06 17:26 UTC

head link

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

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

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>
---
 xen/arch/arm/gic.c  |   13 +++++++++----
 xen/arch/arm/vgic.c |    6 ++----
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 5ec3ff9..72f978d 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -137,6 +137,7 @@ static void gic_irq_enable(struct irq_desc *desc)
     /* Enable routing */
     enabler = GICD[GICD_ISENABLER + irq / 32];
     GICD[GICD_ISENABLER + irq / 32] = enabler | (1u << (irq % 32));
+    desc->status &= ~IRQ_DISABLED;
 }
 
 static void gic_irq_disable(struct irq_desc *desc)
@@ -145,6 +146,7 @@ static void gic_irq_disable(struct irq_desc *desc)
 
     /* Disable routing */
     GICD[GICD_ICENABLER + irq / 32] = (1u << (irq % 32));
+    desc->status |= IRQ_DISABLED;
 }
 
 static unsigned int gic_irq_startup(struct irq_desc *desc)
@@ -562,7 +564,6 @@ void __init release_irq(unsigned int irq)
     spin_lock_irqsave(&desc->lock,flags);
     action = desc->action;
     desc->action  = NULL;
-    desc->status |= IRQ_DISABLED;
     desc->status &= ~IRQ_GUEST;
 
     spin_lock(&gic.lock);
@@ -585,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;
 }
 
@@ -605,6 +603,8 @@ int __init setup_dt_irq(const struct dt_irq *irq, struct
irqaction *new)
 
     rc = __setup_irq(desc, irq->irq, new);
 
+    desc->handler->startup(desc);
+
     spin_unlock_irqrestore(&desc->lock, flags);
 
     return rc;
@@ -743,6 +743,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)
@@ -769,6 +770,10 @@ int gic_route_irq_to_guest(struct domain *d, const struct
dt_irq *irq,
         goto out;
     }
 
+    /* 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 969ad2d..5b533ea 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -374,6 +374,8 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int
n)
         p = irq_to_pending(v, irq);
         if ( !list_empty(&p->inflight) && !(p->status &
GIC_IRQ_GUEST_VISIBLE) )
             gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+        if ( p->desc != NULL && (p->desc->status &
IRQ_DISABLED) )
+            p->desc->handler->enable(p->desc);
         i++;
     }
 
@@ -696,10 +698,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq,
int virtual)
     n->irq = irq;
     n->status = GIC_IRQ_GUEST_PENDING;
     n->priority = priority;
-    if (!virtual)
-        n->desc = irq_to_desc(irq);
-    else
-        n->desc = NULL;
 
     /* the irq is enabled */
     if ( rank->ienable & (1 << (irq % 32)) )
-- 
1.7.10.4

Stefano Stabellini

2013-Dec-06 17:26 UTC

head link

[PATCH 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>
---
 xen/arch/arm/vgic.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5b533ea..9ac24b0 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -360,6 +360,21 @@ 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);
+        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;
@@ -494,8 +509,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

Julien Grall

2013-Dec-06 17:56 UTC

head link

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

On 12/06/2013 05:26 PM, Stefano Stabellini wrote:> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
[..]
>   static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
>   {
>       struct pending_irq *p;
> @@ -494,8 +509,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);

Why ~tr? We want to disable only enabled IRQ.

-- 
Julien Grall

Julien Grall

2013-Dec-06 18:24 UTC

head link

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

On 12/06/2013 05:26 PM, Stefano Stabellini wrote:> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>   xen/arch/arm/vgic.c |   17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5b533ea..9ac24b0 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -360,6 +360,21 @@ 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);
> +        if ( p->desc != NULL )
> +            p->desc->handler->disable(p->desc);
Because of disable function will both both the GIC and irq_desc, you 
will need to take:
	- the GIC lock
         - the IRQ desc lock

-- 
Julien Grall

Julien Grall

2013-Dec-06 18:28 UTC

head link

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

On 12/06/2013 05:26 PM, Stefano Stabellini wrote:> From: Julien Grall <julien.grall@linaro.org>
>
> 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>
> ---
[..]
> @@ -605,6 +603,8 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct irqaction *new)
>
>       rc = __setup_irq(desc, irq->irq, new);
>
> +    desc->handler->startup(desc);
> +
You forgot to take the GIC lock here.
>       spin_unlock_irqrestore(&desc->lock, flags);
>
>       return rc;
> @@ -743,6 +743,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)
> @@ -769,6 +770,10 @@ int gic_route_irq_to_guest(struct domain *d, const
struct dt_irq *irq,
>           goto out;
>       }
>
> +    /* do not assume delivery to vcpu0 */
I would add TODO: in the comment.

-- 
Julien Grall

Julien Grall

2013-Dec-06 18:29 UTC

head link

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

On 12/06/2013 05:26 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>> ---
>   xen/arch/arm/gic.c |   15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index a8a5c2a..5ec3ff9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -129,7 +129,7 @@ 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;
> @@ -137,11 +137,9 @@ static unsigned int gic_irq_startup(struct irq_desc
*desc)
>       /* Enable routing */
>       enabler = GICD[GICD_ISENABLER + irq / 32];
>       GICD[GICD_ISENABLER + irq / 32] = enabler | (1u << (irq % 32));
> -
> -    return 0;
>   }
>
> -static void gic_irq_shutdown(struct irq_desc *desc)
> +static void gic_irq_disable(struct irq_desc *desc)
>   {
>       int irq = desc->irq;
>
> @@ -149,14 +147,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)
>
-- 
Julien Grall

Stefano Stabellini

2013-Dec-09 12:26 UTC

head link

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

On Fri, 6 Dec 2013, Julien Grall wrote:> On 12/06/2013 05:26 PM, Stefano Stabellini wrote:
> > From: Julien Grall <julien.grall@linaro.org>
> > 
> > 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>
> > ---
> 
> [..]
> 
> > @@ -605,6 +603,8 @@ int __init setup_dt_irq(const struct dt_irq *irq,
struct
> > irqaction *new)
> > 
> >       rc = __setup_irq(desc, irq->irq, new);
> > 
> > +    desc->handler->startup(desc);
> > +
> 
> You forgot to take the GIC lock here.
You are right, the call to startup should be protected by the gic lock.
Interestingly the original code doesn''t do that either.

Stefano Stabellini

2013-Dec-09 12:37 UTC

head link

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

On Fri, 6 Dec 2013, Julien Grall wrote:> On 12/06/2013 05:26 PM, Stefano Stabellini wrote:
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> 
> [..]
> 
> >   static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> >   {
> >       struct pending_irq *p;
> > @@ -494,8 +509,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);
> 
> 
> Why ~tr? We want to disable only enabled IRQ.
You are right, I''ll make the change.

Ian Campbell

2013-Dec-09 12:48 UTC

head link

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

On Mon, 2013-12-09 at 12:37 +0000, Stefano Stabellini
wrote:> On Fri, 6 Dec 2013, Julien Grall wrote:
> > On 12/06/2013 05:26 PM, Stefano Stabellini wrote:
> > > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> > 
> > [..]
> > 
> > >   static void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
> > >   {
> > >       struct pending_irq *p;
> > > @@ -494,8 +509,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);
> > 
> > 
> > Why ~tr? We want to disable only enabled IRQ.
> 
> You are right, I''ll make the change.
Don''t forget that the semantics of this register are that you right a 1
to the bit position for the interrupt you want to disable.

Ian.

Ian Campbell

2013-Dec-09 12:50 UTC

head link

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

On Fri, 2013-12-06 at 17:26 +0000, Stefano Stabellini
wrote:> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/vgic.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5b533ea..9ac24b0 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -360,6 +360,21 @@ read_as_zero:
>      return 1;
>  }
>  
> +static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
Does this function need a permission check to make sure it owns the
interrupt which it is disabling?

Perhaps p->desc != NULL indicates that? If so then a comment would be
nice.
> +{
> +    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);
> +        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;
> @@ -494,8 +509,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:

Ian Campbell

2013-Dec-09 17:10 UTC

head link

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

On Fri, 2013-12-06 at 17:26 +0000, Stefano Stabellini
wrote:> From: Julien Grall <julien.grall@linaro.org>
> 
> From: Julien Grall <julien.grall@linaro.org>
Oops ;-)> 
> When Xen needs to EOI a physical IRQ, we should use the IRQ number
> in irq_desc instead of the virtual IRQ.
> 
> 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>
> ---
>  xen/arch/arm/gic.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 43c11cb..7e87acb 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;
>      uint32_t lr;
>      struct vcpu *v = current;
>      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1])
<< 32);
> @@ -916,6 +916,7 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>              /* 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);
> @@ -924,10 +925,10 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>              /* 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);
This relies on pirq being set whenever eoi is, which is currently true,
but it seems a bit fragile. eoi is interchangeable with p->desc != NULL,
isn''t it? And since everything needed to do the eoi is in there is
there
any reason not to switch to that conditional?

Ian Campbell

2013-Dec-09 17:38 UTC

head link

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

On Fri, 2013-12-06 at 17:26 +0000, Stefano Stabellini
wrote:> Introduce a status field in struct pending_irq. Valid states are
> GUEST_PENDING and GUEST_VISIBLE and they are not mutually exclusive.
Are all four combinations valid? In particular is !PENDING and VISIBLE
valid?
> See the in-code comment for an explanation of the states and how they
> are used. Protect pending_irq state manipulations with the vgic lock.
> 
> 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.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Pulling the referenced comment to the top:
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index d5cae2e..5e7bb58 100644> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -22,6 +22,35 @@ 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).
> 
Is this visibility "sticky" in the case where the the use of the LR is
preempted by a new higher priority IRQ? (I suppose this is only a
theoretical concern right now).

+     * 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.
> 
This last paragraph is just saying that we don''t necessarily reuse the
same LR slot, right?

+     *> +     */
> +#define GIC_IRQ_GUEST_PENDING  (1<<1)
> +#define GIC_IRQ_GUEST_VISIBLE   (1<<2)
> +    int status;
>
bit flags should be unsigned I think.

     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
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 7e87acb..a8a5c2a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -626,6 +626,7 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
>          ((virtual_irq & GICH_LR_VIRTUAL_MASK) <<
GICH_LR_VIRTUAL_SHIFT);
>  }
>  
> +/* needs to be called with vgic lock held */
>  void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>          unsigned int state, unsigned int priority)
>  {
> @@ -635,17 +636,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);
> +            n->status |= GIC_IRQ_GUEST_VISIBLE;
> +            n->status &= ~GIC_IRQ_GUEST_PENDING;
Would using set/clear_bit here allow the locking restrictions to be
relaxed?
>              goto out;
>          }
>      }
>  
> -    n = irq_to_pending(v, virtual_irq);
>      if ( !list_empty(&n->lr_queue) )
>          goto out;
>  
> @@ -679,6 +683,8 @@ static void gic_restore_pending_irqs(struct vcpu *v)
>          gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
>          list_del_init(&p->lr_queue);
>          set_bit(i, &this_cpu(lr_mask));
> +        p->status |= GIC_IRQ_GUEST_VISIBLE;
> +        p->status &= ~GIC_IRQ_GUEST_PENDING;
>          spin_unlock_irqrestore(&gic.lock, flags);
>      }
>  
> @@ -884,6 +890,7 @@ 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 long flags;
>  
>      while ((i = find_next_bit((const long unsigned int *) &eisr,
>                                64, i)) < 64) {
> @@ -893,23 +900,13 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>          cpu = -1;
>          eoi = 0;
>  
> -        spin_lock_irq(&gic.lock);
> +        spin_lock(&v->arch.vgic.lock);
> +        spin_lock_irqsave(&gic.lock, flags);
Is the locking hierarchy for vgic.lock, gic.lock and desc.lock written
down somewhere?

Having irqsave on the inner lock is weird, isn''t it? The outer one will
clobber the irq status anyway.

You haven''t changed the contexts where maintenance_interrupt can be
called either -- have you?
>          lr = GICH[GICH_LR + i];
>          virq = lr & GICH_LR_VIRTUAL_MASK;
>          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;
> @@ -918,8 +915,32 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
>              eoi = 1;
>              pirq = p->desc->irq;
>          }
> +        if ( p->status & GIC_IRQ_GUEST_PENDING )
> +        {
> +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
Does this conflict with what the last para of the main comment said?
i.e. we do infact reuse the slot via gic_set_lr here rather than
reinjecting?
> +            p->status &= ~GIC_IRQ_GUEST_PENDING;
Don''t need to mess with VISIBLE here?
> +            i++;
> +            spin_unlock_irqrestore(&gic.lock, flags);
> +            spin_unlock(&v->arch.vgic.lock);
> +            continue;
> +        }
> +
> +        p->status &= ~GIC_IRQ_GUEST_VISIBLE;
>          list_del_init(&p->inflight);
> -        spin_unlock_irq(&v->arch.vgic.lock);
> +
> +        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);
> +            p->status |= GIC_IRQ_GUEST_VISIBLE;
> +            p->status &= ~GIC_IRQ_GUEST_PENDING;
> +            list_del_init(&p->lr_queue);
> +            set_bit(i, &this_cpu(lr_mask));
> +        } else {
> +            gic_inject_irq_stop();
> +        }
> +
> +        spin_unlock_irqrestore(&gic.lock, flags);
> +        spin_unlock(&v->arch.vgic.lock);
>  
>          if ( eoi ) {
>              /* this is not racy because we can''t receive another
irq of the
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 2e4b11f..c71db37 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -365,6 +365,9 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t
r, int n)
>      struct pending_irq *p;
>      unsigned int irq;
>      int i = 0;
> +    unsigned long f;
"flags" is conventional.
> +
> +    spin_lock_irqsave(&v->arch.vgic.lock, f);
>  
>      while ( (i = find_next_bit((const long unsigned int *) &r, 32, i))
< 32 ) {
>          irq = i + (32 * n);
> @@ -373,6 +376,8 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t
r, int n)
>              gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
>          i++;
>      }
> +
> +    spin_unlock_irqrestore(&v->arch.vgic.lock, f);
>  }
>  
>  static inline int is_vcpu_running(struct domain *d, int vcpuid)
> @@ -672,8 +677,15 @@ 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) )
> +    {
> +        n->status |= GIC_IRQ_GUEST_PENDING;
> +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> +        return;
> +    }
> +
> +    /* vcpu offline */
You should test for this first I think? Otherwise you would end up
injecting it. 
> +    if ( test_bit(_VPF_down, &v->pause_flags) )
>      {
>          spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
>          return;
> @@ -682,6 +694,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;
> +    n->status = GIC_IRQ_GUEST_PENDING;
Should this not be |=? Otherwise we potentially lose the fact that the
IRQ is VISIBLE?
>      n->priority = priority;
>      if (!virtual)
>          n->desc = irq_to_desc(irq);

Ian Campbell

2013-Dec-09 17:42 UTC

head link

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

On Fri, 2013-12-06 at 17:26 +0000, 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>
> ---
>  xen/arch/arm/gic.c |   15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index a8a5c2a..5ec3ff9 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -129,7 +129,7 @@ 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;
> @@ -137,11 +137,9 @@ static unsigned int gic_irq_startup(struct irq_desc
*desc)
>      /* Enable routing */
>      enabler = GICD[GICD_ISENABLER + irq / 32];
>      GICD[GICD_ISENABLER + irq / 32] = enabler | (1u << (irq % 32));
"enabler | " here is not necessary for the same reasons it was wrong
in
gic_irq_disable. It''s more benign here but I think it should be fixed
anyway.

This patch itself is OK though:
Acked-by: Ian Campbell <ian.campbell@citrix.com>
> -
> -    return 0;
>  }
>  
> -static void gic_irq_shutdown(struct irq_desc *desc)
> +static void gic_irq_disable(struct irq_desc *desc)
>  {
>      int irq = desc->irq;
>  
> @@ -149,14 +147,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)

Ian Campbell

2013-Dec-09 17:44 UTC

head link

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

On Fri, 2013-12-06 at 17:26 +0000, Stefano Stabellini
wrote:> From: Julien Grall <julien.grall@linaro.org>
> 
> From: Julien Grall <julien.grall@linaro.org>
You don''t need this in your commit message -- git
format-patch/send-email (whichever one it is) will insert it
automatically.

With things like this I''m liable to make a mess of it when I apply.

Ian.

Stefano Stabellini

2013-Dec-10 13:02 UTC

head link

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

On Mon, 9 Dec 2013, Ian Campbell wrote:> On Fri, 2013-12-06 at 17:26 +0000, Stefano Stabellini wrote:
> > Introduce a status field in struct pending_irq. Valid states are
> > GUEST_PENDING and GUEST_VISIBLE and they are not mutually exclusive.
> 
> Are all four combinations valid? In particular is !PENDING and VISIBLE
> valid?
Yes, they are all valid.

> > See the in-code comment for an explanation of the states and how they
> > are used. Protect pending_irq state manipulations with the vgic lock.
> > 
> > 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.
> > 
> > Signed-off-by: Stefano Stabellini
<stefano.stabellini@eu.citrix.com>
> 
> 
> Pulling the referenced comment to the top:
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index d5cae2e..5e7bb58 100644
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -22,6 +22,35 @@ 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).
> > 
> 
> Is this visibility "sticky" in the case where the the use of the
LR is
> preempted by a new higher priority IRQ? (I suppose this is only a
> theoretical concern right now).
Yes, I think it should be "sticky".

> +     * 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.
> > 
> 
> This last paragraph is just saying that we don''t necessarily reuse
the
> same LR slot, right?
Yes, if by reuse you mean change the status on the existing LR.

> +     *
> > +     */
> > +#define GIC_IRQ_GUEST_PENDING  (1<<1)
> > +#define GIC_IRQ_GUEST_VISIBLE   (1<<2)
> > +    int status;
> >
> 
> bit flags should be unsigned I think.
OK

>      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
> > 
> 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 7e87acb..a8a5c2a 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -626,6 +626,7 @@ static inline void gic_set_lr(int lr, unsigned int
virtual_irq,
> >          ((virtual_irq & GICH_LR_VIRTUAL_MASK) <<
GICH_LR_VIRTUAL_SHIFT);
> >  }
> >  
> > +/* needs to be called with vgic lock held */
> >  void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> >          unsigned int state, unsigned int priority)
> >  {
> > @@ -635,17 +636,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);
> > +            n->status |= GIC_IRQ_GUEST_VISIBLE;
> > +            n->status &= ~GIC_IRQ_GUEST_PENDING;
> 
> Would using set/clear_bit here allow the locking restrictions to be
> relaxed?
Yes, good idea.

> >              goto out;
> >          }
> >      }
> >  
> > -    n = irq_to_pending(v, virtual_irq);
> >      if ( !list_empty(&n->lr_queue) )
> >          goto out;
> >  
> > @@ -679,6 +683,8 @@ static void gic_restore_pending_irqs(struct vcpu
*v)
> >          gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> >          list_del_init(&p->lr_queue);
> >          set_bit(i, &this_cpu(lr_mask));
> > +        p->status |= GIC_IRQ_GUEST_VISIBLE;
> > +        p->status &= ~GIC_IRQ_GUEST_PENDING;
> >          spin_unlock_irqrestore(&gic.lock, flags);
> >      }
> >  
> > @@ -884,6 +890,7 @@ 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 long flags;
> >  
> >      while ((i = find_next_bit((const long unsigned int *) &eisr,
> >                                64, i)) < 64) {
> > @@ -893,23 +900,13 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
> >          cpu = -1;
> >          eoi = 0;
> >  
> > -        spin_lock_irq(&gic.lock);
> > +        spin_lock(&v->arch.vgic.lock);
> > +        spin_lock_irqsave(&gic.lock, flags);
> 
> Is the locking hierarchy for vgic.lock, gic.lock and desc.lock written
> down somewhere?
> 
> Having irqsave on the inner lock is weird, isn''t it? The outer one
will
> clobber the irq status anyway.
> 
> You haven''t changed the contexts where maintenance_interrupt can
be
> called either -- have you?
This goes away thanks to the atomic bit operations, see next version of
the patch.

> >          lr = GICH[GICH_LR + i];
> >          virq = lr & GICH_LR_VIRTUAL_MASK;
> >          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;
> > @@ -918,8 +915,32 @@ static void maintenance_interrupt(int irq, void
*dev_id, struct cpu_user_regs *r
> >              eoi = 1;
> >              pirq = p->desc->irq;
> >          }
> > +        if ( p->status & GIC_IRQ_GUEST_PENDING )
> > +        {
> > +            gic_set_lr(i, p->irq, GICH_LR_PENDING,
p->priority);
> 
> Does this conflict with what the last para of the main comment said?
> i.e. we do infact reuse the slot via gic_set_lr here rather than
> reinjecting?
I see where the confusion is: reusing or not exactly the same LR
register doesn''t make a difference. What does is that we do not
reuse the same interrupt notification by changing the status of the live
LR before it is EOIed by the guest.

> > +            p->status &= ~GIC_IRQ_GUEST_PENDING;
> 
> Don''t need to mess with VISIBLE here?
Nope because it is still visible after gic_set_lr (the second
notification is visible).

> > +            i++;
> > +            spin_unlock_irqrestore(&gic.lock, flags);
> > +            spin_unlock(&v->arch.vgic.lock);
> > +            continue;
> > +        }
> > +
> > +        p->status &= ~GIC_IRQ_GUEST_VISIBLE;
> >          list_del_init(&p->inflight);
> > -        spin_unlock_irq(&v->arch.vgic.lock);
> > +
> > +        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);
> > +            p->status |= GIC_IRQ_GUEST_VISIBLE;
> > +            p->status &= ~GIC_IRQ_GUEST_PENDING;
> > +            list_del_init(&p->lr_queue);
> > +            set_bit(i, &this_cpu(lr_mask));
> > +        } else {
> > +            gic_inject_irq_stop();
> > +        }
> > +
> > +        spin_unlock_irqrestore(&gic.lock, flags);
> > +        spin_unlock(&v->arch.vgic.lock);
> >  
> >          if ( eoi ) {
> >              /* this is not racy because we can''t receive
another irq of the
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 2e4b11f..c71db37 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -365,6 +365,9 @@ static void vgic_enable_irqs(struct vcpu *v,
uint32_t r, int n)
> >      struct pending_irq *p;
> >      unsigned int irq;
> >      int i = 0;
> > +    unsigned long f;
> 
> "flags" is conventional.
this also goes away after switching to atomic bits operations.

> > +
> > +    spin_lock_irqsave(&v->arch.vgic.lock, f);
> >  
> >      while ( (i = find_next_bit((const long unsigned int *) &r,
32, i)) < 32 ) {
> >          irq = i + (32 * n);
> > @@ -373,6 +376,8 @@ static void vgic_enable_irqs(struct vcpu *v,
uint32_t r, int n)
> >              gic_set_guest_irq(v, irq, GICH_LR_PENDING,
p->priority);
> >          i++;
> >      }
> > +
> > +    spin_unlock_irqrestore(&v->arch.vgic.lock, f);
> >  }
> >  
> >  static inline int is_vcpu_running(struct domain *d, int vcpuid)
> > @@ -672,8 +677,15 @@ 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) )
> > +    {
> > +        n->status |= GIC_IRQ_GUEST_PENDING;
> > +        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> > +        return;
> > +    }
> > +
> > +    /* vcpu offline */
> 
> You should test for this first I think? Otherwise you would end up
> injecting it. 
I don''t think so: a cpu can receive an interrupt from the distributor
even the cpu is not responding.
Of course the vcpu cannot actually receive the interrupt until is woken
up.

> > +    if ( test_bit(_VPF_down, &v->pause_flags) )
> >      {
> >          spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >          return;
> > @@ -682,6 +694,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;
> > +    n->status = GIC_IRQ_GUEST_PENDING;
> 
> Should this not be |=? Otherwise we potentially lose the fact that the
> IRQ is VISIBLE?
Yep

> >      n->priority = priority;
> >      if (!virtual)
> >          n->desc = irq_to_desc(irq);

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

[PATCH 0/6] interrupt handling fixes

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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