[PATCH 0/5] xen/arm: multiple guests support in the GIC and VGIC

[PATCH 0/5] xen/arm: multiple guests support in the GIC and VGIC

[Stefano Stabellini]

Hi all,
this patch series fixes the GIC, VGIC and vtimer issues that caused dom1
to hang, as described by Ian in
http://marc.info/?l=xen-devel&m=133856539418794.

With this patch series applied on top of Ian's, dom1 boots up to:

VFS: Cannot open root device "(null)" or unknown-block(2,0)


Stefano Stabellini (5):
      arm/vtimer: do not let the guest interact with the physical timer
      arm/gic: fix gic context switch
      xen/gic: support injecting IRQs even to VCPUs not currently running
      xen/vgic: vgic: support irq enable/disable
      xen/vgic: initialize pending_irqs.lr_queue

 xen/arch/arm/gic.c           |   76 +++++++++++++++++++++++++++++++-----------
 xen/arch/arm/gic.h           |    2 +-
 xen/arch/arm/time.c          |    4 +-
 xen/arch/arm/vgic.c          |   30 ++++++++++++++++-
 xen/include/asm-arm/domain.h |    9 +++++
 5 files changed, 97 insertions(+), 24 deletions(-)

Cheers,

Stefano

[PATCH 1/5] arm/vtimer: do not let the guest interact with the physical timer

[Stefano Stabellini]

The guest can read the physical counter but it shouldn't be able to
cause interrupts of the physical timer to go to the hypervisor.
Trap physical timer reads/writes in vtimer.c instead.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/time.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 1587fa2..d5b71d7 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -160,8 +160,8 @@ void __cpuinit init_timer_interrupt(void)
     WRITE_CP64(0, CNTVOFF);     /* No VM-specific offset */
     WRITE_CP32(0, CNTKCTL);     /* No user-mode access */
 #if USE_HYP_TIMER
-    /* Let the VMs read the physical counter and timer so they can tell time */
-    WRITE_CP32(CNTHCTL_PA|CNTHCTL_TA, CNTHCTL);
+    /* Do not the VMs program the physical timer, only read the physical counter */
+    WRITE_CP32(CNTHCTL_PA, CNTHCTL);
 #else
     /* Cannot let VMs access physical counter if we are using it */
     WRITE_CP32(0, CNTHCTL);
-- 
1.7.2.5

[PATCH 2/5] arm/gic: fix gic context switch

[Stefano Stabellini]

gic_save/restore_state should also save and restore lr_mask and
event_mask too.

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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 3f9a061..c73f274 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -67,6 +67,8 @@ void gic_save_state(struct vcpu *v)
 
     for ( i=0; i<nr_lrs; i++)
         v->arch.gic_lr[i] = GICH[GICH_LR + i];
+    v->arch.lr_mask = gic.lr_mask;
+    v->arch.event_mask = gic.event_mask;
     /* Disable until next VCPU scheduled */
     GICH[GICH_HCR] = 0;
     isb();
@@ -79,6 +81,8 @@ void gic_restore_state(struct vcpu *v)
     if ( is_idle_vcpu(v) )
         return;
 
+    gic.lr_mask = v->arch.lr_mask;
+    gic.event_mask = v->arch.event_mask;
     for ( i=0; i<nr_lrs; i++)
         GICH[GICH_LR + i] = v->arch.gic_lr[i];
     GICH[GICH_HCR] = GICH_HCR_EN;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 230ea8c..3576d50 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -121,6 +121,8 @@ struct arch_vcpu
 
     uint32_t gic_hcr, gic_vmcr, gic_apr;
     uint32_t gic_lr[64];
+    uint64_t event_mask;
+    uint64_t lr_mask;
 
     struct {
         /*
-- 
1.7.2.5

Re: [PATCH 2/5] arm/gic: fix gic context switch

[Ian Campbell]

On Wed, 2012-06-06 at 12:22 +0100, Stefano Stabellini wrote:
> gic_save/restore_state should also save and restore lr_mask and
> event_mask too.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

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

> ---
>  xen/arch/arm/gic.c           |    4 ++++
>  xen/include/asm-arm/domain.h |    2 ++
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 3f9a061..c73f274 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -67,6 +67,8 @@ void gic_save_state(struct vcpu *v)
>  
>      for ( i=0; i<nr_lrs; i++)
>          v->arch.gic_lr[i] = GICH[GICH_LR + i];
> +    v->arch.lr_mask = gic.lr_mask;
> +    v->arch.event_mask = gic.event_mask;
>      /* Disable until next VCPU scheduled */
>      GICH[GICH_HCR] = 0;
>      isb();
> @@ -79,6 +81,8 @@ void gic_restore_state(struct vcpu *v)
>      if ( is_idle_vcpu(v) )
>          return;
>  
> +    gic.lr_mask = v->arch.lr_mask;
> +    gic.event_mask = v->arch.event_mask;
>      for ( i=0; i<nr_lrs; i++)
>          GICH[GICH_LR + i] = v->arch.gic_lr[i];
>      GICH[GICH_HCR] = GICH_HCR_EN;
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 230ea8c..3576d50 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -121,6 +121,8 @@ struct arch_vcpu
>  
>      uint32_t gic_hcr, gic_vmcr, gic_apr;
>      uint32_t gic_lr[64];
> +    uint64_t event_mask;
> +    uint64_t lr_mask;
>  
>      struct {
>          /*

[PATCH 3/5] xen/gic: support injecting IRQs even to VCPUs not currently running

[Stefano Stabellini]

The lr_pending list belongs to the vgic rather than the gic, so move it
there.

gic_set_guest_irq should take into account whether the vcpu is currently
running and if it is not it should add the irq to the right lr_pending
list.

When restoring the gic state we need to go through the lr_pending list
because it is possible that some irqs have been "injected" while the
vcpu wasn't running.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic.c           |   66 +++++++++++++++++++++++++++++------------
 xen/arch/arm/gic.h           |    2 +-
 xen/arch/arm/vgic.c          |    3 +-
 xen/include/asm-arm/domain.h |    7 ++++
 4 files changed, 56 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index c73f274..2e41d75 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -38,6 +38,7 @@
 #define GICH ((volatile uint32_t *) (FIXMAP_ADDR(FIXMAP_GICH)  \
                                      + (GIC_HR_OFFSET & 0xfff)))
 static void events_maintenance(struct vcpu *v);
+static void gic_restore_pending_irqs(struct vcpu *v);
 
 /* Global state */
 static struct {
@@ -49,13 +50,6 @@ static struct {
     spinlock_t lock;
     uint64_t event_mask;
     uint64_t lr_mask;
-    /* lr_pending is used to queue IRQs (struct pending_irq) that the
-     * vgic tried to inject in the guest (calling gic_set_guest_irq) but
-     * no LRs were available at the time.
-     * As soon as an LR is freed we remove the first IRQ from this
-     * list and write it to the LR register.
-     * lr_pending is a subset of vgic.inflight_irqs. */
-    struct list_head lr_pending;
 } gic;
 
 irq_desc_t irq_desc[NR_IRQS];
@@ -87,6 +81,8 @@ void gic_restore_state(struct vcpu *v)
         GICH[GICH_LR + i] = v->arch.gic_lr[i];
     GICH[GICH_HCR] = GICH_HCR_EN;
     isb();
+
+    gic_restore_pending_irqs(v);
 }
 
 static unsigned int gic_irq_startup(struct irq_desc *desc)
@@ -323,7 +319,6 @@ int __init gic_init(void)
 
     gic.lr_mask = 0ULL;
     gic.event_mask = 0ULL;
-    INIT_LIST_HEAD(&gic.lr_pending);
 
     spin_unlock(&gic.lock);
 
@@ -435,17 +430,20 @@ static inline void gic_set_lr(int lr, unsigned int virtual_irq,
         ((virtual_irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT);
 }
 
-void gic_set_guest_irq(unsigned int virtual_irq,
+void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
         unsigned int state, unsigned int priority)
 {
     int i;
     struct pending_irq *iter, *n;
 
-    events_maintenance(current);
+    if ( v->is_running )
+    {
+        events_maintenance(v);
+    }
 
     spin_lock_irq(&gic.lock);
 
-    if ( list_empty(&gic.lr_pending) )
+    if ( v->is_running && list_empty(&v->arch.vgic.lr_pending) )
     {
         i = find_first_zero_bit(&gic.lr_mask, nr_lrs);
         if (i < nr_lrs) {
@@ -457,8 +455,8 @@ void gic_set_guest_irq(unsigned int virtual_irq,
         }
     }
 
-    n = irq_to_pending(current, virtual_irq);
-    list_for_each_entry ( iter, &gic.lr_pending, lr_queue )
+    n = irq_to_pending(v, virtual_irq);
+    list_for_each_entry ( iter, &v->arch.vgic.lr_pending, lr_queue )
     {
         if ( iter->priority > priority )
         {
@@ -466,13 +464,40 @@ void gic_set_guest_irq(unsigned int virtual_irq,
             goto out;
         }
     }
-    list_add_tail(&n->lr_queue, &gic.lr_pending);
+    list_add_tail(&n->lr_queue, &v->arch.vgic.lr_pending);
 
 out:
     spin_unlock_irq(&gic.lock);
     return;
 }
 
+static void gic_restore_pending_irqs(struct vcpu *v)
+{
+    int i;
+    struct pending_irq *p;
+
+    /* check for new pending irqs */
+    if ( list_empty(&v->arch.vgic.lr_pending) )
+        return;
+
+    list_for_each_entry ( p, &v->arch.vgic.lr_pending, lr_queue )
+    {
+        i = find_first_zero_bit(&gic.lr_mask, nr_lrs);
+        if ( i < nr_lrs )
+        {
+            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
+            list_del_init(&p->lr_queue);
+            set_bit(i, &gic.lr_mask);
+            if ( p->irq == VGIC_IRQ_EVTCHN_CALLBACK )
+                set_bit(i, &gic.event_mask);
+        } else
+        {
+            return;
+        }
+    }
+
+}
+
 static void gic_inject_irq_start(void)
 {
     uint32_t hcr;
@@ -582,9 +607,10 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
 {
     int i = 0, virq;
     uint32_t lr;
+    struct vcpu *v = current;
     uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 32);
 
-    events_maintenance(current);
+    events_maintenance(v);
 
     while ((i = find_next_bit((const long unsigned int *) &eisr,
                               sizeof(eisr), i)) < sizeof(eisr)) {
@@ -596,8 +622,8 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
         GICH[GICH_LR + i] = 0;
         clear_bit(i, &gic.lr_mask);
 
-        if ( !list_empty(&gic.lr_pending) ) {
-            p = list_entry(gic.lr_pending.next, typeof(*p), lr_queue);
+        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, &gic.lr_mask);
@@ -608,14 +634,14 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r
         }
         spin_unlock_irq(&gic.lock);
 
-        spin_lock(&current->arch.vgic.lock);
-        p = irq_to_pending(current, virq);
+        spin_lock(&v->arch.vgic.lock);
+        p = irq_to_pending(v, virq);
         if ( p->desc != NULL ) {
             p->desc->status &= ~IRQ_INPROGRESS;
             GICC[GICC_DIR] = virq;
         }
         list_del_init(&p->inflight);
-        spin_unlock(&current->arch.vgic.lock);
+        spin_unlock(&v->arch.vgic.lock);
 
         i++;
     }
diff --git a/xen/arch/arm/gic.h b/xen/arch/arm/gic.h
index ac9cf3a..a55e146 100644
--- a/xen/arch/arm/gic.h
+++ b/xen/arch/arm/gic.h
@@ -134,7 +134,7 @@ extern void gic_route_irqs(void);
 extern void gic_inject(void);
 
 extern void __cpuinit init_maintenance_interrupt(void);
-extern void gic_set_guest_irq(unsigned int irq,
+extern void gic_set_guest_irq(struct vcpu *v, unsigned int irq,
         unsigned int state, unsigned int priority);
 extern int gic_route_irq_to_guest(struct domain *d, unsigned int irq,
                                   const char * devname);
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 6eb6ec7..5a624bd 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -112,6 +112,7 @@ int vcpu_vgic_init(struct vcpu *v)
             | (1<<(v->vcpu_id+16))
             | (1<<(v->vcpu_id+24));
     INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
+    INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
     spin_lock_init(&v->arch.vgic.lock);
 
     return 0;
@@ -568,7 +569,7 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
     else
         n->desc = NULL;
 
-    gic_set_guest_irq(irq, GICH_LR_PENDING, priority);
+    gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
     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 3576d50..2b14545 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -140,6 +140,13 @@ struct arch_vcpu
          * As soon as an IRQ is EOI'd by the guest and removed from the
          * corresponding LR it is also removed from this list. */
         struct list_head inflight_irqs;
+        /* lr_pending is used to queue IRQs (struct pending_irq) that the
+         * vgic tried to inject in the guest (calling gic_set_guest_irq) but
+         * no LRs were available at the time.
+         * As soon as an LR is freed we remove the first IRQ from this
+         * list and write it to the LR register.
+         * lr_pending is a subset of vgic.inflight_irqs. */
+        struct list_head lr_pending;
         spinlock_t lock;
     } vgic;
 
-- 
1.7.2.5

Re: [PATCH 3/5] xen/gic: support injecting IRQs even to VCPUs not currently running

[Tim Deegan]

At 12:22 +0100 on 06 Jun (1338985328), Stefano Stabellini wrote:
> +static void gic_restore_pending_irqs(struct vcpu *v)
> +{
> +    int i;
> +    struct pending_irq *p;
> +
> +    /* check for new pending irqs */
> +    if ( list_empty(&v->arch.vgic.lr_pending) )
> +        return;
> +
> +    list_for_each_entry ( p, &v->arch.vgic.lr_pending, lr_queue )
> +    {
> +        i = find_first_zero_bit(&gic.lr_mask, nr_lrs);
> +        if ( i < nr_lrs )
> +        {
> +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> +            list_del_init(&p->lr_queue);
> +            set_bit(i, &gic.lr_mask);
> +            if ( p->irq == VGIC_IRQ_EVTCHN_CALLBACK )
> +                set_bit(i, &gic.event_mask);
> +        } else
> +        {
> +            return;
> +        }

This is a bit ugly - maybe "if ( i >= nr_lrs ) return" above and don't
indent the block?

Re: [PATCH 3/5] xen/gic: support injecting IRQs even to VCPUs not currently running

[Stefano Stabellini]

On Thu, 7 Jun 2012, Tim Deegan wrote:
> At 12:22 +0100 on 06 Jun (1338985328), Stefano Stabellini wrote:
> > +static void gic_restore_pending_irqs(struct vcpu *v)
> > +{
> > +    int i;
> > +    struct pending_irq *p;
> > +
> > +    /* check for new pending irqs */
> > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > +        return;
> > +
> > +    list_for_each_entry ( p, &v->arch.vgic.lr_pending, lr_queue )
> > +    {
> > +        i = find_first_zero_bit(&gic.lr_mask, nr_lrs);
> > +        if ( i < nr_lrs )
> > +        {
> > +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> > +            list_del_init(&p->lr_queue);
> > +            set_bit(i, &gic.lr_mask);
> > +            if ( p->irq == VGIC_IRQ_EVTCHN_CALLBACK )
> > +                set_bit(i, &gic.event_mask);
> > +        } else
> > +        {
> > +            return;
> > +        }
> 
> This is a bit ugly - maybe "if ( i >= nr_lrs ) return" above and don't
> indent the block?
> 

good idea

Re: [PATCH 3/5] xen/gic: support injecting IRQs even to VCPUs not currently running

[Ian Campbell]

On Thu, 2012-06-07 at 13:46 +0100, Tim Deegan wrote:
> At 12:22 +0100 on 06 Jun (1338985328), Stefano Stabellini wrote:
> > +static void gic_restore_pending_irqs(struct vcpu *v)
> > +{
> > +    int i;
> > +    struct pending_irq *p;
> > +
> > +    /* check for new pending irqs */
> > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > +        return;
> > +
> > +    list_for_each_entry ( p, &v->arch.vgic.lr_pending, lr_queue )

Is list_for_each_entry on an empty list somehow wrong/buggy/slow?

> > +    {
> > +        i = find_first_zero_bit(&gic.lr_mask, nr_lrs);
> > +        if ( i < nr_lrs )
> > +        {
> > +            gic_set_lr(i, p->irq, GICH_LR_PENDING, p->priority);
> > +            list_del_init(&p->lr_queue);
> > +            set_bit(i, &gic.lr_mask);
> > +            if ( p->irq == VGIC_IRQ_EVTCHN_CALLBACK )
> > +                set_bit(i, &gic.event_mask);
> > +        } else
> > +        {
> > +            return;
> > +        }
> 
> This is a bit ugly - maybe "if ( i >= nr_lrs ) return" above and don't
> indent the block?
> 

Re: [PATCH 3/5] xen/gic: support injecting IRQs even to VCPUs not currently running

[Stefano Stabellini]

On Tue, 26 Jun 2012, Ian Campbell wrote:
> On Thu, 2012-06-07 at 13:46 +0100, Tim Deegan wrote:
> > At 12:22 +0100 on 06 Jun (1338985328), Stefano Stabellini wrote:
> > > +static void gic_restore_pending_irqs(struct vcpu *v)
> > > +{
> > > +    int i;
> > > +    struct pending_irq *p;
> > > +
> > > +    /* check for new pending irqs */
> > > +    if ( list_empty(&v->arch.vgic.lr_pending) )
> > > +        return;
> > > +
> > > +    list_for_each_entry ( p, &v->arch.vgic.lr_pending, lr_queue )
> 
> Is list_for_each_entry on an empty list somehow wrong/buggy/slow?

Not at all. I'll change it.

[PATCH 4/5] xen/vgic: vgic: support irq enable/disable

[Stefano Stabellini]

If vgic_vcpu_inject_irq is called (for example by a device emulator like
vtimer.c) but the corresponding irq is not enabled in the virtual gicd
just queue it in the inflight_irqs list.

When the irq is enabled make sure to call gic_set_guest_irq.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/vgic.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 5a624bd..4cdfec5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -343,6 +343,22 @@ read_as_zero:
     return 1;
 }
 
+static void vgic_enable_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,
+                        sizeof(uint32_t), i)) < sizeof(uint32_t) ) {
+        irq = i + (32 * n);
+        p = irq_to_pending(v, irq);
+        if ( !list_empty(&p->inflight) )
+            gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
+        i++;
+    }
+}
+
 static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
 {
     struct hsr_dabt dabt = info->dabt;
@@ -351,6 +367,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
     struct vgic_irq_rank *rank;
     int offset = (int)(info->gpa - VGIC_DISTR_BASE_ADDRESS);
     int gicd_reg = REG(offset);
+    uint32_t tr;
 
     switch ( gicd_reg )
     {
@@ -378,8 +395,10 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
         rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ISENABLER);
         if ( rank == NULL) goto write_ignore;
         vgic_lock_rank(v, rank);
+        tr = rank->ienable;
         rank->ienable |= *r;
         vgic_unlock_rank(v, rank);
+        vgic_enable_irqs(v, (*r) & (~tr), gicd_reg - GICD_ISENABLER);
         return 1;
 
     case GICD_ICENABLER ... GICD_ICENABLERN:
@@ -569,7 +588,9 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
     else
         n->desc = NULL;
 
-    gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
+    /* the irq is enabled */
+    if ( rank->ienable & (1 << (irq % 32)) )
+        gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
 
     spin_lock_irqsave(&v->arch.vgic.lock, flags);
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
-- 
1.7.2.5

Re: [PATCH 4/5] xen/vgic: vgic: support irq enable/disable

[Ian Campbell]

On Wed, 2012-06-06 at 12:22 +0100, Stefano Stabellini wrote:
> If vgic_vcpu_inject_irq is called (for example by a device emulator like
> vtimer.c) but the corresponding irq is not enabled in the virtual gicd
> just queue it in the inflight_irqs list.


> When the irq is enabled make sure to call gic_set_guest_irq.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

I had to go reread the comments on inflight_irqs and lr_pending again
(I'm perpetually confused by that stuff) but I think this makes sense.

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

> ---
>  xen/arch/arm/vgic.c |   23 ++++++++++++++++++++++-
>  1 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 5a624bd..4cdfec5 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -343,6 +343,22 @@ read_as_zero:
>      return 1;
>  }
>  
> +static void vgic_enable_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,
> +                        sizeof(uint32_t), i)) < sizeof(uint32_t) ) {
> +        irq = i + (32 * n);
> +        p = irq_to_pending(v, irq);
> +        if ( !list_empty(&p->inflight) )
> +            gic_set_guest_irq(v, irq, GICH_LR_PENDING, p->priority);
> +        i++;

what you have here is a for loop ;-)

> +    }
> +}
> +
>  static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>  {
>      struct hsr_dabt dabt = info->dabt;
> @@ -351,6 +367,7 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>      struct vgic_irq_rank *rank;
>      int offset = (int)(info->gpa - VGIC_DISTR_BASE_ADDRESS);
>      int gicd_reg = REG(offset);
> +    uint32_t tr;
>  
>      switch ( gicd_reg )
>      {
> @@ -378,8 +395,10 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ISENABLER);
>          if ( rank == NULL) goto write_ignore;
>          vgic_lock_rank(v, rank);
> +        tr = rank->ienable;
>          rank->ienable |= *r;
>          vgic_unlock_rank(v, rank);
> +        vgic_enable_irqs(v, (*r) & (~tr), gicd_reg - GICD_ISENABLER);
>          return 1;
>  
>      case GICD_ICENABLER ... GICD_ICENABLERN:
> @@ -569,7 +588,9 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq, int virtual)
>      else
>          n->desc = NULL;
>  
> -    gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
> +    /* the irq is enabled */
> +    if ( rank->ienable & (1 << (irq % 32)) )
> +        gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
>  
>      spin_lock_irqsave(&v->arch.vgic.lock, flags);
>      list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )

[PATCH 5/5] xen/vgic: initialize pending_irqs.lr_queue

[Stefano Stabellini]

Properly initialize all the pending_irqs.lr_queue like we do for
inflight.
Check whether we already have the irq in lr_queue before adding it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/gic.c  |    6 ++++++
 xen/arch/arm/vgic.c |    6 ++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 2e41d75..998033a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -456,6 +456,12 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
     }
 
     n = irq_to_pending(v, virtual_irq);
+    if ( !list_empty(&n->lr_queue) )
+    {
+        printk(KERN_WARNING "%s: irq %d already in lr_queue\n", __func__,
+                virtual_irq);
+        goto out;
+    }
     list_for_each_entry ( iter, &v->arch.vgic.lr_pending, lr_queue )
     {
         if ( iter->priority > priority )
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 4cdfec5..653e8e5 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -84,7 +84,10 @@ int domain_vgic_init(struct domain *d)
     d->arch.vgic.pending_irqs =
         xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
     for (i=0; i<d->arch.vgic.nr_lines; i++)
+    {
         INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
+        INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
+    }
     for (i=0; i<DOMAIN_NR_RANKS(d); i++)
         spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
     return 0;
@@ -99,7 +102,10 @@ int vcpu_vgic_init(struct vcpu *v)
 
     memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
     for (i = 0; i < 32; i++)
+    {
         INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].inflight);
+        INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue);
+    }
     printk("vcpu_vgic_init irq[0] at %p desc is %p\n",
            &v->arch.vgic.pending_irqs[0],
            v->arch.vgic.pending_irqs[0].desc);
-- 
1.7.2.5

Re: [PATCH 5/5] xen/vgic: initialize pending_irqs.lr_queue

[Ian Campbell]

On Wed, 2012-06-06 at 12:22 +0100, Stefano Stabellini wrote:
> Properly initialize all the pending_irqs.lr_queue like we do for
> inflight.
> Check whether we already have the irq in lr_queue before adding it.

Should this be a fatal error? Can a guest make this happen?

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/gic.c  |    6 ++++++
>  xen/arch/arm/vgic.c |    6 ++++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 2e41d75..998033a 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -456,6 +456,12 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
>      }
>  
>      n = irq_to_pending(v, virtual_irq);
> +    if ( !list_empty(&n->lr_queue) )
> +    {
> +        printk(KERN_WARNING "%s: irq %d already in lr_queue\n", __func__,
> +                virtual_irq);
> +        goto out;
> +    }
>      list_for_each_entry ( iter, &v->arch.vgic.lr_pending, lr_queue )
>      {
>          if ( iter->priority > priority )
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 4cdfec5..653e8e5 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -84,7 +84,10 @@ int domain_vgic_init(struct domain *d)
>      d->arch.vgic.pending_irqs =
>          xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
>      for (i=0; i<d->arch.vgic.nr_lines; i++)
> +    {
>          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
> +        INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
> +    }
>      for (i=0; i<DOMAIN_NR_RANKS(d); i++)
>          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
>      return 0;
> @@ -99,7 +102,10 @@ int vcpu_vgic_init(struct vcpu *v)
>  
>      memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
>      for (i = 0; i < 32; i++)
> +    {
>          INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].inflight);
> +        INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue);
> +    }
>      printk("vcpu_vgic_init irq[0] at %p desc is %p\n",
>             &v->arch.vgic.pending_irqs[0],
>             v->arch.vgic.pending_irqs[0].desc);

Re: [PATCH 5/5] xen/vgic: initialize pending_irqs.lr_queue

[Stefano Stabellini]

On Tue, 26 Jun 2012, Ian Campbell wrote:
> On Wed, 2012-06-06 at 12:22 +0100, Stefano Stabellini wrote:
> > Properly initialize all the pending_irqs.lr_queue like we do for
> > inflight.
> > Check whether we already have the irq in lr_queue before adding it.
> 
> Should this be a fatal error? Can a guest make this happen?

it could happen if the guest keeps enabling and disabling the irqs using
the GICD_ICENABLERn and the GICD_ISENABLERn registers.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/gic.c  |    6 ++++++
> >  xen/arch/arm/vgic.c |    6 ++++++
> >  2 files changed, 12 insertions(+), 0 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 2e41d75..998033a 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -456,6 +456,12 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> >      }
> >  
> >      n = irq_to_pending(v, virtual_irq);
> > +    if ( !list_empty(&n->lr_queue) )
> > +    {
> > +        printk(KERN_WARNING "%s: irq %d already in lr_queue\n", __func__,
> > +                virtual_irq);
> > +        goto out;
> > +    }
> >      list_for_each_entry ( iter, &v->arch.vgic.lr_pending, lr_queue )
> >      {
> >          if ( iter->priority > priority )
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 4cdfec5..653e8e5 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -84,7 +84,10 @@ int domain_vgic_init(struct domain *d)
> >      d->arch.vgic.pending_irqs =
> >          xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
> >      for (i=0; i<d->arch.vgic.nr_lines; i++)
> > +    {
> >          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
> > +        INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
> > +    }
> >      for (i=0; i<DOMAIN_NR_RANKS(d); i++)
> >          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> >      return 0;
> > @@ -99,7 +102,10 @@ int vcpu_vgic_init(struct vcpu *v)
> >  
> >      memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
> >      for (i = 0; i < 32; i++)
> > +    {
> >          INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].inflight);
> > +        INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue);
> > +    }
> >      printk("vcpu_vgic_init irq[0] at %p desc is %p\n",
> >             &v->arch.vgic.pending_irqs[0],
> >             v->arch.vgic.pending_irqs[0].desc);
> 
> 
> 

Re: [PATCH 5/5] xen/vgic: initialize pending_irqs.lr_queue

[Ian Campbell]

On Tue, 2012-06-26 at 18:53 +0100, Stefano Stabellini wrote:
> On Tue, 26 Jun 2012, Ian Campbell wrote:
> > On Wed, 2012-06-06 at 12:22 +0100, Stefano Stabellini wrote:
> > > Properly initialize all the pending_irqs.lr_queue like we do for
> > > inflight.
> > > Check whether we already have the irq in lr_queue before adding it.
> > 
> > Should this be a fatal error? Can a guest make this happen?
> 
> it could happen if the guest keeps enabling and disabling the irqs using
> the GICD_ICENABLERn and the GICD_ISENABLERn registers.

Then the printk is probably wrong. At the least it should be a gdprintk
but really I think it is unnecessary.

> 
> 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > ---
> > >  xen/arch/arm/gic.c  |    6 ++++++
> > >  xen/arch/arm/vgic.c |    6 ++++++
> > >  2 files changed, 12 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > > index 2e41d75..998033a 100644
> > > --- a/xen/arch/arm/gic.c
> > > +++ b/xen/arch/arm/gic.c
> > > @@ -456,6 +456,12 @@ void gic_set_guest_irq(struct vcpu *v, unsigned int virtual_irq,
> > >      }
> > >  
> > >      n = irq_to_pending(v, virtual_irq);
> > > +    if ( !list_empty(&n->lr_queue) )
> > > +    {
> > > +        printk(KERN_WARNING "%s: irq %d already in lr_queue\n", __func__,
> > > +                virtual_irq);
> > > +        goto out;
> > > +    }
> > >      list_for_each_entry ( iter, &v->arch.vgic.lr_pending, lr_queue )
> > >      {
> > >          if ( iter->priority > priority )
> > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > > index 4cdfec5..653e8e5 100644
> > > --- a/xen/arch/arm/vgic.c
> > > +++ b/xen/arch/arm/vgic.c
> > > @@ -84,7 +84,10 @@ int domain_vgic_init(struct domain *d)
> > >      d->arch.vgic.pending_irqs =
> > >          xzalloc_array(struct pending_irq, d->arch.vgic.nr_lines);
> > >      for (i=0; i<d->arch.vgic.nr_lines; i++)
> > > +    {
> > >          INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight);
> > > +        INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
> > > +    }
> > >      for (i=0; i<DOMAIN_NR_RANKS(d); i++)
> > >          spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> > >      return 0;
> > > @@ -99,7 +102,10 @@ int vcpu_vgic_init(struct vcpu *v)
> > >  
> > >      memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
> > >      for (i = 0; i < 32; i++)
> > > +    {
> > >          INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].inflight);
> > > +        INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue);
> > > +    }
> > >      printk("vcpu_vgic_init irq[0] at %p desc is %p\n",
> > >             &v->arch.vgic.pending_irqs[0],
> > >             v->arch.vgic.pending_irqs[0].desc);
> > 
> > 
> > 

Re: [PATCH 5/5] xen/vgic: initialize pending_irqs.lr_queue

[Stefano Stabellini]

On Wed, 27 Jun 2012, Ian Campbell wrote:
> On Tue, 2012-06-26 at 18:53 +0100, Stefano Stabellini wrote:
> > On Tue, 26 Jun 2012, Ian Campbell wrote:
> > > On Wed, 2012-06-06 at 12:22 +0100, Stefano Stabellini wrote:
> > > > Properly initialize all the pending_irqs.lr_queue like we do for
> > > > inflight.
> > > > Check whether we already have the irq in lr_queue before adding it.
> > > 
> > > Should this be a fatal error? Can a guest make this happen?
> > 
> > it could happen if the guest keeps enabling and disabling the irqs using
> > the GICD_ICENABLERn and the GICD_ISENABLERn registers.
> 
> Then the printk is probably wrong. At the least it should be a gdprintk
> but really I think it is unnecessary.

OK, I'll remove it.

Re: [PATCH 1/5] arm/vtimer: do not let the guest interact with the physical timer

[Ian Campbell]

On Wed, 2012-06-06 at 12:22 +0100, Stefano Stabellini wrote:
> The guest can read the physical counter but it shouldn't be able to
> cause interrupts of the physical timer to go to the hypervisor.
> Trap physical timer reads/writes in vtimer.c instead.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
>  xen/arch/arm/time.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 1587fa2..d5b71d7 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -160,8 +160,8 @@ void __cpuinit init_timer_interrupt(void)
>      WRITE_CP64(0, CNTVOFF);     /* No VM-specific offset */
>      WRITE_CP32(0, CNTKCTL);     /* No user-mode access */
>  #if USE_HYP_TIMER
> -    /* Let the VMs read the physical counter and timer so they can tell time */
> -    WRITE_CP32(CNTHCTL_PA|CNTHCTL_TA, CNTHCTL);
> +    /* Do not the VMs program the physical timer, only read the physical counter */

"Do not *let* the VMs..." ?

> +    WRITE_CP32(CNTHCTL_PA, CNTHCTL);
>  #else
>      /* Cannot let VMs access physical counter if we are using it */
>      WRITE_CP32(0, CNTHCTL);

Re: [PATCH 1/5] arm/vtimer: do not let the guest interact with the physical timer

[Stefano Stabellini]

On Tue, 26 Jun 2012, Ian Campbell wrote:
> On Wed, 2012-06-06 at 12:22 +0100, Stefano Stabellini wrote:
> > The guest can read the physical counter but it shouldn't be able to
> > cause interrupts of the physical timer to go to the hypervisor.
> > Trap physical timer reads/writes in vtimer.c instead.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> >  xen/arch/arm/time.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> > index 1587fa2..d5b71d7 100644
> > --- a/xen/arch/arm/time.c
> > +++ b/xen/arch/arm/time.c
> > @@ -160,8 +160,8 @@ void __cpuinit init_timer_interrupt(void)
> >      WRITE_CP64(0, CNTVOFF);     /* No VM-specific offset */
> >      WRITE_CP32(0, CNTKCTL);     /* No user-mode access */
> >  #if USE_HYP_TIMER
> > -    /* Let the VMs read the physical counter and timer so they can tell time */
> > -    WRITE_CP32(CNTHCTL_PA|CNTHCTL_TA, CNTHCTL);
> > +    /* Do not the VMs program the physical timer, only read the physical counter */
> 
> "Do not *let* the VMs..." ?

right..

Re: [PATCH 0/5] xen/arm: multiple guests support in the GIC and VGIC

[Tim Deegan]

At 12:20 +0100 on 06 Jun (1338985240), Stefano Stabellini wrote:
> Hi all,
> this patch series fixes the GIC, VGIC and vtimer issues that caused dom1
> to hang, as described by Ian in
> http://marc.info/?l=xen-devel&m=133856539418794.
> 
> With this patch series applied on top of Ian's, dom1 boots up to:
> 
> VFS: Cannot open root device "(null)" or unknown-block(2,0)
> 

I had one style nit on patch 3; you can add my Ack to all the others
(though IanC has a better grasp of the vgic stuff than I do and may
contradict me).

Cheers,

Tim.

Re: [PATCH 0/5] xen/arm: multiple guests support in the GIC and VGIC

[Stefano Stabellini]

On Thu, 7 Jun 2012, Tim Deegan wrote:
> At 12:20 +0100 on 06 Jun (1338985240), Stefano Stabellini wrote:
> > Hi all,
> > this patch series fixes the GIC, VGIC and vtimer issues that caused dom1
> > to hang, as described by Ian in
> > http://marc.info/?l=xen-devel&m=133856539418794.
> > 
> > With this patch series applied on top of Ian's, dom1 boots up to:
> > 
> > VFS: Cannot open root device "(null)" or unknown-block(2,0)
> > 
> 
> I had one style nit on patch 3; you can add my Ack to all the others
> (though IanC has a better grasp of the vgic stuff than I do and may
> contradict me).

great, thanks!