thr3ads.net - Xen devel - [PATCH 0/4] arm: SMP interrupt handling fixes [Aug 2012]

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

Ian Campbell

2012-Aug-06 11:34 UTC

[PATCH 0/4] arm: SMP interrupt handling fixes

With the following series I now successfully boot to a dom0 prompt on a
4 way FastModel.

The most important one is not disabling the GICD on all CPUs, although I
suspect the per-CPU irq_desc thing is pretty important too. The other
two are just incidental things I happened to find while investigating.

        arm: disable distributor delivery on boot CPU only
        arm: don''t bother setting up vtimer, vgic etc on idle CPUs
        arm/vtimer: convert result to ticks when reading CNTPCT register
        arm: Use per-CPU irq_desc for PPIs and SGIs

Ian.

Ian Campbell

2012-Aug-06 11:34 UTC

head link

[PATCH 1/4] arm: disable distributor delivery on boot CPU only

The secondary processors do not call enter_hyp_mode until the boot CPU
has brought most of the system up, including enabling delivery via the
distributor. This means that bringing up secondary CPUs unexpectedly
disables the GICD again, meaning we get no further interrupts on any
CPU.

It''s not clear that the distributor actually needs to be disabled to
modify the group registers but it seems reasonable that the bringup
code should make sure the GICD is disabled even if not doing the
transition to hyp mode, so move this to the main flow of head.S and
only do it on the boot processor.

For completeness also disable the GICC (CPU interface) on all CPUs
too.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/head.S        |   14 ++++++++++++++
 xen/arch/arm/mode_switch.S |   14 +++++++++-----
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/head.S b/xen/arch/arm/head.S
index cdbe011..a69bf72 100644
--- a/xen/arch/arm/head.S
+++ b/xen/arch/arm/head.S
@@ -67,6 +67,12 @@ start:
 	add   r8, r10                /* r8 := paddr(DTB) */
 #endif
 
+	/* Disable interrupt delivery at the GIC''s CPU interface */
+	mov   r0, #GIC_BASE_ADDRESS
+	add   r0, r0, #GIC_CR_OFFSET
+	mov   r1, #0
+	str   r1, [r0]
+
 	/* Are we the boot CPU? */
 	mov   r12, #0                /* r12 := CPU ID */
 	mrc   CP32(r0, MPIDR)
@@ -85,8 +91,16 @@ start:
 	ldr   r1, [r0]               /* Which CPU is being booted? */
 	teq   r1, r12                /* Is it us? */
 	bne   1b
+	b     secondary_cpu
 
 boot_cpu:
+	/* Setup which only needs to be done once, on the boot CPU */
+	mov   r0, #GIC_BASE_ADDRESS
+	add   r0, r0, #GIC_DR_OFFSET
+	mov   r1, #0
+	str   r1, [r0]               /* Disable delivery in the distributor */
+
+secondary_cpu:
 #ifdef EARLY_UART_ADDRESS
 	ldr   r11, =EARLY_UART_ADDRESS  /* r11 := UART base address */
 	teq   r12, #0                   /* CPU 0 sets up the UART too */
diff --git a/xen/arch/arm/mode_switch.S b/xen/arch/arm/mode_switch.S
index f5549d7..9211d26 100644
--- a/xen/arch/arm/mode_switch.S
+++ b/xen/arch/arm/mode_switch.S
@@ -49,13 +49,17 @@ enter_hyp_mode:
 	/* Continuing ugliness: Set up the GIC so NS state owns interrupts */
 	mov   r0, #GIC_BASE_ADDRESS
 	add   r0, r0, #GIC_DR_OFFSET
-	mov   r1, #0
-	str   r1, [r0]               /* Disable delivery in the distributor */
 	add   r0, r0, #0x80          /* GICD_IGROUP0 */
-	mov   r2, #0xffffffff        /* All interrupts to group 1 */
+	mov   r2, #0xffffffff        /* Interrupts 0-31 (SGI&PPI) to group 1 */
+	/* The remaining interrupts are Shared Periphal Interrupts and so
+	 * need reconfiguring only once, on the boot CPU */
 	str   r2, [r0]
-	str   r2, [r0, #4]
-	str   r2, [r0, #8]
+	teq   r12, #0
+	bne   skip_spi
+	str   r2, [r0, #4]           /* Interrupts 32-63 (SPI) to group 1 */
+	str   r2, [r0, #8]           /* Interrupts 64-95 (SPI) to group 1 */
+skip_spi:
+	
 	/* Must drop priority mask below 0x80 before entering NS state */
 	mov   r0, #GIC_BASE_ADDRESS
 	add   r0, r0, #GIC_CR_OFFSET
-- 
1.7.9.1

Ian Campbell

2012-Aug-06 11:34 UTC

head link

[PATCH 2/4] arm: don''t bother setting up vtimer, vgic etc on idle CPUs

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/domain.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index ee58d68..f47db4f 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -386,6 +386,10 @@ int vcpu_initialise(struct vcpu *v)
     v->arch.saved_context.sp = (uint32_t)v->arch.cpu_info;
     v->arch.saved_context.pc = (uint32_t)continue_new_vcpu;
 
+    /* Idle VCPUs don''t need the rest of this setup */
+    if ( is_idle_vcpu(v) )
+        return rc;
+
     if ( (rc = vcpu_vgic_init(v)) != 0 )
         return rc;
 
-- 
1.7.9.1

Ian Campbell

2012-Aug-06 11:34 UTC

head link

[PATCH 3/4] arm/vtimer: convert result to ticks when reading CNTPCT register

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/vtimer.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 6b1152e..92c385c 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -103,6 +103,7 @@ static int vtimer_emulate_64(struct cpu_user_regs *regs,
union hsr hsr)
     struct hsr_cp64 cp64 = hsr.cp64;
     uint32_t *r1 = &regs->r0 + cp64.reg1;
     uint32_t *r2 = &regs->r0 + cp64.reg2;
+    uint64_t ticks
     s_time_t now;
 
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
@@ -111,8 +112,9 @@ static int vtimer_emulate_64(struct cpu_user_regs *regs,
union hsr hsr)
         if ( cp64.read )
         {
             now = NOW() - v->arch.vtimer.offset;
-            *r1 = (uint32_t)(now & 0xffffffff);
-            *r2 = (uint32_t)(now >> 32);
+            ticks = ns_to_ticks(now);
+            *r1 = (uint32_t)(ticks & 0xffffffff);
+            *r2 = (uint32_t)(ticks >> 32);
             return 1;
         }
         else
-- 
1.7.9.1

Ian Campbell

2012-Aug-06 11:35 UTC

head link

[PATCH 4/4] arm: Use per-CPU irq_desc for PPIs and SGIs

The first 32 interrupts on a GIC are the Peripheral Private Interrupts
and Software-Generated Interrupts and are local to each processor.

The irq_desc cannot be shared since we use irq_desc->status to track
whether the IRQ is in-progress etc. Therefore give each processor its
own local irq_desc for each of these interupts.

We must also route them on each CPU, so do so.

This feels like a bit of a layering violation (since the core ARM
irq.c now knows about thinkgs wich are really gic.c business)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/gic.c          |   23 ++++++++++++++++++-----
 xen/arch/arm/gic.h          |    3 ++-
 xen/arch/arm/irq.c          |   23 ++++++++++++++++++++++-
 xen/arch/arm/setup.c        |    3 ++-
 xen/arch/arm/smpboot.c      |    9 ++++++++-
 xen/include/asm-arm/irq.h   |   13 +++++++++++++
 xen/include/asm-arm/setup.h |    2 --
 xen/include/xen/irq.h       |   10 ++--------
 8 files changed, 67 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 6f5b0e1..f674111 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -50,9 +50,17 @@ static struct {
     uint64_t lr_mask;
 } gic;
 
-irq_desc_t irq_desc[NR_IRQS];
+static irq_desc_t irq_desc[NR_IRQS];
+static DEFINE_PER_CPU(irq_desc_t[NR_LOCAL_IRQS], local_irq_desc);
+
 unsigned nr_lrs;
 
+irq_desc_t *__irq_to_desc(int irq)
+{
+    if (irq < NR_LOCAL_IRQS) return &this_cpu(local_irq_desc)[irq];
+    return &irq_desc[irq-NR_LOCAL_IRQS];
+}
+
 void gic_save_state(struct vcpu *v)
 {
     int i;
@@ -256,8 +264,8 @@ static void __cpuinit gic_cpu_init(void)
 {
     int i;
 
-    /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so 
-     * even though they are controlled with GICD registers, they must 
+    /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so
+     * even though they are controlled with GICD registers, they must
      * be set up here with the other per-cpu state. */
     GICD[GICD_ICENABLER] = 0xffff0000; /* Disable all PPI */
     GICD[GICD_ISENABLER] = 0x0000ffff; /* Enable all SGI */
@@ -338,7 +346,7 @@ void gic_disable_cpu(void)
     spin_unlock_irq(&gic.lock);
 }
 
-void gic_route_irqs(void)
+void gic_route_ppis(void)
 {
     /* XXX should get these from DT */
     /* GIC maintenance */
@@ -347,6 +355,11 @@ void gic_route_irqs(void)
     gic_route_irq(26, 1, 1u << smp_processor_id(), 0xa0);
     /* Timer */
     gic_route_irq(30, 1, 1u << smp_processor_id(), 0xa0);
+}
+
+void gic_route_spis(void)
+{
+    /* XXX should get these from DT */
     /* UART */
     gic_route_irq(37, 0, 1u << smp_processor_id(), 0xa0);
 }
@@ -404,7 +417,7 @@ int __init setup_irq(unsigned int irq, struct irqaction
*new)
 
     rc = __setup_irq(desc, irq, new);
 
-    spin_unlock_irqrestore(&desc->lock,flags);
+    spin_unlock_irqrestore(&desc->lock, flags);
 
     return rc;
 }
diff --git a/xen/arch/arm/gic.h b/xen/arch/arm/gic.h
index fa2cf06..b8f9f201 100644
--- a/xen/arch/arm/gic.h
+++ b/xen/arch/arm/gic.h
@@ -132,7 +132,8 @@ extern int vcpu_vgic_init(struct vcpu *v);
 extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq,int virtual);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 
-extern void gic_route_irqs(void);
+extern void gic_route_ppis(void);
+extern void gic_route_spis(void);
 
 extern void gic_inject(void);
 
diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
index f9d663b..72e83e6 100644
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -58,20 +58,41 @@ static int __init init_irq_data(void)
 {
     int irq;
 
-    for (irq = 0; irq < NR_IRQS; irq++) {
+    for (irq = NR_LOCAL_IRQS; irq < NR_IRQS; irq++) {
         struct irq_desc *desc = irq_to_desc(irq);
         init_one_irq_desc(desc);
         desc->irq = irq;
         desc->action  = NULL;
     }
+
+    return 0;
+}
+
+static int __cpuinit init_local_irq_data(void)
+{
+    int irq;
+
+    for (irq = 0; irq < NR_LOCAL_IRQS; irq++) {
+        struct irq_desc *desc = irq_to_desc(irq);
+        init_one_irq_desc(desc);
+        desc->irq = irq;
+        desc->action  = NULL;
+    }
+
     return 0;
 }
 
 void __init init_IRQ(void)
 {
+    BUG_ON(init_local_irq_data() < 0);
     BUG_ON(init_irq_data() < 0);
 }
 
+void __cpuinit init_secondary_IRQ(void)
+{
+    BUG_ON(init_local_irq_data() < 0);
+}
+
 int __init request_irq(unsigned int irq,
         void (*handler)(int, void *, struct cpu_user_regs *),
         unsigned long irqflags, const char * devname, void *dev_id)
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index fd70553..c4ca270 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -199,7 +199,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     init_IRQ();
 
-    gic_route_irqs();
+    gic_route_ppis();
+    gic_route_spis();
 
     init_maintenance_interrupt();
     init_timer_interrupt();
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index 6463a8d..c0750c0 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -26,6 +26,8 @@
 #include <xen/sched.h>
 #include <xen/smp.h>
 #include <xen/softirq.h>
+#include <xen/timer.h>
+#include <xen/irq.h>
 #include <asm/vfp.h>
 #include "gic.h"
 
@@ -129,8 +131,13 @@ void __cpuinit start_secondary(unsigned long
boot_phys_offset,
     enable_vfp();
 
     gic_init_secondary_cpu();
+
+    init_secondary_IRQ();
+
+    gic_route_ppis();
+
+    init_maintenance_interrupt();
     init_timer_interrupt();
-    gic_route_irqs();
 
     set_current(idle_vcpu[cpuid]);
 
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 21e0b85..abde839 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -17,10 +17,23 @@ struct irq_cfg {
 #define arch_irq_desc irq_cfg
 };
 
+#define NR_LOCAL_IRQS	32
+#define NR_IRQS		1024
+#define nr_irqs NR_IRQS
+
+struct irq_desc;
+
+struct irq_desc *__irq_to_desc(int irq);
+
+#define irq_to_desc(irq)    __irq_to_desc(irq)
+
 void do_IRQ(struct cpu_user_regs *regs, unsigned int irq, int is_fiq);
 
 #define domain_pirq_to_irq(d, pirq) (pirq)
 
+void init_IRQ(void);
+void init_secondary_IRQ(void);
+
 #endif /* _ASM_HW_IRQ_H */
 /*
  * Local variables:
diff --git a/xen/include/asm-arm/setup.h b/xen/include/asm-arm/setup.h
index 6433b4e..8769f66 100644
--- a/xen/include/asm-arm/setup.h
+++ b/xen/include/asm-arm/setup.h
@@ -9,8 +9,6 @@ void arch_get_xen_caps(xen_capabilities_info_t *info);
 
 int construct_dom0(struct domain *d);
 
-void init_IRQ(void);
-
 #endif
 /*
  * Local variables:
diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
index cbe1dbc..5973cce 100644
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -88,21 +88,15 @@ typedef struct irq_desc {
     struct list_head rl_link;
 } __cacheline_aligned irq_desc_t;
 
+#ifndef irq_to_desc
 #define irq_to_desc(irq)    (&irq_desc[irq])
+#endif
 
 int init_one_irq_desc(struct irq_desc *);
 int arch_init_one_irq_desc(struct irq_desc *);
 
 #define irq_desc_initialized(desc) ((desc)->handler != NULL)
 
-#if defined(__arm__)
-
-#define NR_IRQS		1024
-#define nr_irqs NR_IRQS
-extern irq_desc_t irq_desc[NR_IRQS];
-
-#endif
-
 extern int setup_irq(unsigned int irq, struct irqaction *);
 extern void release_irq(unsigned int irq);
 extern int request_irq(unsigned int irq,
-- 
1.7.9.1

Tim Deegan

2012-Aug-06 11:55 UTC

head link

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

At 11:34 +0000 on 06 Aug (1344252897), Ian Campbell
wrote:> The secondary processors do not call enter_hyp_mode until the boot CPU
> has brought most of the system up, including enabling delivery via the
> distributor. This means that bringing up secondary CPUs unexpectedly
> disables the GICD again, meaning we get no further interrupts on any
> CPU.
> 
> It''s not clear that the distributor actually needs to be disabled
to
> modify the group registers but it seems reasonable that the bringup
> code should make sure the GICD is disabled even if not doing the
> transition to hyp mode, so move this to the main flow of head.S and
> only do it on the boot processor.
> 
> For completeness also disable the GICC (CPU interface) on all CPUs
> too.
I think that having interrupts disabled is something we can rely on the
bootloader/firmware handling for us, so this should all stay in
mode_switch.S for now, and avoid leaking GIC_* magic constants into
head.S.  (Unless you fancy writing a DT parser in assembler :))

Tim.
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/head.S        |   14 ++++++++++++++
>  xen/arch/arm/mode_switch.S |   14 +++++++++-----
>  2 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/head.S b/xen/arch/arm/head.S
> index cdbe011..a69bf72 100644
> --- a/xen/arch/arm/head.S
> +++ b/xen/arch/arm/head.S
> @@ -67,6 +67,12 @@ start:
>  	add   r8, r10                /* r8 := paddr(DTB) */
>  #endif
>  
> +	/* Disable interrupt delivery at the GIC''s CPU interface */
> +	mov   r0, #GIC_BASE_ADDRESS
> +	add   r0, r0, #GIC_CR_OFFSET
> +	mov   r1, #0
> +	str   r1, [r0]
> +
>  	/* Are we the boot CPU? */
>  	mov   r12, #0                /* r12 := CPU ID */
>  	mrc   CP32(r0, MPIDR)
> @@ -85,8 +91,16 @@ start:
>  	ldr   r1, [r0]               /* Which CPU is being booted? */
>  	teq   r1, r12                /* Is it us? */
>  	bne   1b
> +	b     secondary_cpu
>  
>  boot_cpu:
> +	/* Setup which only needs to be done once, on the boot CPU */
> +	mov   r0, #GIC_BASE_ADDRESS
> +	add   r0, r0, #GIC_DR_OFFSET
> +	mov   r1, #0
> +	str   r1, [r0]               /* Disable delivery in the distributor */
> +
> +secondary_cpu:
>  #ifdef EARLY_UART_ADDRESS
>  	ldr   r11, =EARLY_UART_ADDRESS  /* r11 := UART base address */
>  	teq   r12, #0                   /* CPU 0 sets up the UART too */
> diff --git a/xen/arch/arm/mode_switch.S b/xen/arch/arm/mode_switch.S
> index f5549d7..9211d26 100644
> --- a/xen/arch/arm/mode_switch.S
> +++ b/xen/arch/arm/mode_switch.S
> @@ -49,13 +49,17 @@ enter_hyp_mode:
>  	/* Continuing ugliness: Set up the GIC so NS state owns interrupts */
>  	mov   r0, #GIC_BASE_ADDRESS
>  	add   r0, r0, #GIC_DR_OFFSET
> -	mov   r1, #0
> -	str   r1, [r0]               /* Disable delivery in the distributor */
>  	add   r0, r0, #0x80          /* GICD_IGROUP0 */
> -	mov   r2, #0xffffffff        /* All interrupts to group 1 */
> +	mov   r2, #0xffffffff        /* Interrupts 0-31 (SGI&PPI) to group 1
*/
> +	/* The remaining interrupts are Shared Periphal Interrupts and so
> +	 * need reconfiguring only once, on the boot CPU */
>  	str   r2, [r0]
> -	str   r2, [r0, #4]
> -	str   r2, [r0, #8]
> +	teq   r12, #0
> +	bne   skip_spi
> +	str   r2, [r0, #4]           /* Interrupts 32-63 (SPI) to group 1 */
> +	str   r2, [r0, #8]           /* Interrupts 64-95 (SPI) to group 1 */
> +skip_spi:
> +	
>  	/* Must drop priority mask below 0x80 before entering NS state */
>  	mov   r0, #GIC_BASE_ADDRESS
>  	add   r0, r0, #GIC_CR_OFFSET
> -- 
> 1.7.9.1
>

Ian Campbell

2012-Aug-06 11:56 UTC

head link

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

On Mon, 2012-08-06 at 12:55 +0100, Tim Deegan wrote:> At 11:34 +0000 on 06 Aug (1344252897), Ian Campbell wrote:
> > The secondary processors do not call enter_hyp_mode until the boot CPU
> > has brought most of the system up, including enabling delivery via the
> > distributor. This means that bringing up secondary CPUs unexpectedly
> > disables the GICD again, meaning we get no further interrupts on any
> > CPU.
> > 
> > It''s not clear that the distributor actually needs to be
disabled to
> > modify the group registers but it seems reasonable that the bringup
> > code should make sure the GICD is disabled even if not doing the
> > transition to hyp mode, so move this to the main flow of head.S and
> > only do it on the boot processor.
> > 
> > For completeness also disable the GICC (CPU interface) on all CPUs
> > too.
> 
> I think that having interrupts disabled is something we can rely on the
> bootloader/firmware handling for us, so this should all stay in
> mode_switch.S for now, and avoid leaking GIC_* magic constants into
> head.S.  (Unless you fancy writing a DT parser in assembler :))
Not really ;-) I''ll move this back then.

Ian Campbell

2012-Aug-06 13:45 UTC

head link

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

On Mon, 2012-08-06 at 12:56 +0100, Ian Campbell wrote:> On Mon, 2012-08-06 at 12:55 +0100, Tim Deegan wrote:
> > At 11:34 +0000 on 06 Aug (1344252897), Ian Campbell wrote:
> > > The secondary processors do not call enter_hyp_mode until the
boot CPU
> > > has brought most of the system up, including enabling delivery
via the
> > > distributor. This means that bringing up secondary CPUs
unexpectedly
> > > disables the GICD again, meaning we get no further interrupts on
any
> > > CPU.
> > > 
> > > It''s not clear that the distributor actually needs to be
disabled to
> > > modify the group registers but it seems reasonable that the
bringup
> > > code should make sure the GICD is disabled even if not doing the
> > > transition to hyp mode, so move this to the main flow of head.S
and
> > > only do it on the boot processor.
> > > 
> > > For completeness also disable the GICC (CPU interface) on all
CPUs
> > > too.
> > 
> > I think that having interrupts disabled is something we can rely on
the
> > bootloader/firmware handling for us, so this should all stay in
> > mode_switch.S for now, and avoid leaking GIC_* magic constants into
> > head.S.  (Unless you fancy writing a DT parser in assembler :))
> 
> Not really ;-) I''ll move this back then.
8<---------------------------------------------------------------

From 6440d1868cb03573ebacf5eb3cfc69f4f6abdf15 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Mon, 6 Aug 2012 09:40:59 +0000
Subject: [PATCH] arm: disable distributor delivery on boot CPU only

The secondary processors do not call enter_hyp_mode until the boot CPU
has brought most of the system up, including enabling delivery via the
distributor. This means that bringing up secondary CPUs unexpectedly
disables the GICD again, meaning we get no further interrupts on any
CPU.

For completeness also disable the GICC (CPU interface) on all CPUs
too.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/mode_switch.S |   23 +++++++++++++++++------
 1 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/mode_switch.S b/xen/arch/arm/mode_switch.S
index f5549d7..acbd523 100644
--- a/xen/arch/arm/mode_switch.S
+++ b/xen/arch/arm/mode_switch.S
@@ -23,6 +23,8 @@
 
 /* Get up a CPU into Hyp mode.  Clobbers r0-r3.
  *
+ * Expects r12 == CPU number
+ *
  * This code is specific to the VE model, and not intended to be used
  * on production systems.  As such it''s a bit hackier than the main
  * boot code in head.S.  In future it will be replaced by better
@@ -46,19 +48,28 @@ enter_hyp_mode:
 	mcr   CP32(r0, CNTFRQ)
 	ldr   r0, =0x40c00           /* SMP, c11, c10 in non-secure mode */
 	mcr   CP32(r0, NSACR)
-	/* Continuing ugliness: Set up the GIC so NS state owns interrupts */
 	mov   r0, #GIC_BASE_ADDRESS
 	add   r0, r0, #GIC_DR_OFFSET
+	/* Disable the GIC distributor, on the boot CPU only */
 	mov   r1, #0
-	str   r1, [r0]               /* Disable delivery in the distributor */
+	teq   r12, #0                /* Is this the boot CPU? */
+	streq r1, [r0]
+	/* Continuing ugliness: Set up the GIC so NS state owns interrupts,
+	 * The first 32 interrupts (SGIs & PPIs) must be configured on all
+	 * CPUs while the remainder are SPIs and only need to be done one, on
+	 * the boot CPU. */
 	add   r0, r0, #0x80          /* GICD_IGROUP0 */
 	mov   r2, #0xffffffff        /* All interrupts to group 1 */
-	str   r2, [r0]
-	str   r2, [r0, #4]
-	str   r2, [r0, #8]
-	/* Must drop priority mask below 0x80 before entering NS state */
+	teq   r12, #0                /* Boot CPU? */
+	str   r2, [r0]               /* Interrupts  0-31 (SGI & PPI) */
+	streq r2, [r0, #4]           /* Interrupts 32-63 (SPI) */
+	streq r2, [r0, #8]           /* Interrupts 64-95 (SPI) */
+	/* Disable the GIC CPU interface on all processors */
 	mov   r0, #GIC_BASE_ADDRESS
 	add   r0, r0, #GIC_CR_OFFSET
+	mov   r1, #0
+	str   r1, [r0]		     
+	/* Must drop priority mask below 0x80 before entering NS state */
 	ldr   r1, =0xff
 	str   r1, [r0, #0x4]         /* -> GICC_PMR */
 	/* Reset a few config registers */
-- 
1.7.9.1

Ian Campbell

2012-Aug-06 13:46 UTC

head link

Re: [PATCH 3/4] arm/vtimer: convert result to ticks when reading CNTPCT register

On Mon, 2012-08-06 at 12:34 +0100, Ian Campbell wrote:> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/vtimer.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 6b1152e..92c385c 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -103,6 +103,7 @@ static int vtimer_emulate_64(struct cpu_user_regs
*regs, union hsr hsr)
>      struct hsr_cp64 cp64 = hsr.cp64;
>      uint32_t *r1 = &regs->r0 + cp64.reg1;
>      uint32_t *r2 = &regs->r0 + cp64.reg2;
> +    uint64_t ticks
Ahem, I need to remember to build the final version of my patches before
sending...

8<---------------------------------------------------------

From fd78d6059ae46ab46b0f3ade0e696dcf288a8b99 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Mon, 6 Aug 2012 11:08:12 +0000
Subject: [PATCH] arm/vtimer: convert result to ticks when reading CNTPCT
 register

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/vtimer.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 6b1152e..490b021 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -103,6 +103,7 @@ static int vtimer_emulate_64(struct cpu_user_regs *regs,
union hsr hsr)
     struct hsr_cp64 cp64 = hsr.cp64;
     uint32_t *r1 = &regs->r0 + cp64.reg1;
     uint32_t *r2 = &regs->r0 + cp64.reg2;
+    uint64_t ticks;
     s_time_t now;
 
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
@@ -111,8 +112,9 @@ static int vtimer_emulate_64(struct cpu_user_regs *regs,
union hsr hsr)
         if ( cp64.read )
         {
             now = NOW() - v->arch.vtimer.offset;
-            *r1 = (uint32_t)(now & 0xffffffff);
-            *r2 = (uint32_t)(now >> 32);
+            ticks = ns_to_ticks(now);
+            *r1 = (uint32_t)(ticks & 0xffffffff);
+            *r2 = (uint32_t)(ticks >> 32);
             return 1;
         }
         else
-- 
1.7.9.1

Tim Deegan

2012-Aug-07 09:42 UTC

head link

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

At 14:45 +0100 on 06 Aug (1344264356), Ian Campbell
wrote:> From 6440d1868cb03573ebacf5eb3cfc69f4f6abdf15 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Mon, 6 Aug 2012 09:40:59 +0000
> Subject: [PATCH] arm: disable distributor delivery on boot CPU only
> 
> The secondary processors do not call enter_hyp_mode until the boot CPU
> has brought most of the system up, including enabling delivery via the
> distributor. This means that bringing up secondary CPUs unexpectedly
> disables the GICD again, meaning we get no further interrupts on any
> CPU.
> 
> For completeness also disable the GICC (CPU interface) on all CPUs
> too.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>
> ---
>  xen/arch/arm/mode_switch.S |   23 +++++++++++++++++------
>  1 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/mode_switch.S b/xen/arch/arm/mode_switch.S
> index f5549d7..acbd523 100644
> --- a/xen/arch/arm/mode_switch.S
> +++ b/xen/arch/arm/mode_switch.S
> @@ -23,6 +23,8 @@
>  
>  /* Get up a CPU into Hyp mode.  Clobbers r0-r3.
>   *
> + * Expects r12 == CPU number
> + *
>   * This code is specific to the VE model, and not intended to be used
>   * on production systems.  As such it''s a bit hackier than the
main
>   * boot code in head.S.  In future it will be replaced by better
> @@ -46,19 +48,28 @@ enter_hyp_mode:
>  	mcr   CP32(r0, CNTFRQ)
>  	ldr   r0, =0x40c00           /* SMP, c11, c10 in non-secure mode */
>  	mcr   CP32(r0, NSACR)
> -	/* Continuing ugliness: Set up the GIC so NS state owns interrupts */
>  	mov   r0, #GIC_BASE_ADDRESS
>  	add   r0, r0, #GIC_DR_OFFSET
> +	/* Disable the GIC distributor, on the boot CPU only */
>  	mov   r1, #0
> -	str   r1, [r0]               /* Disable delivery in the distributor */
> +	teq   r12, #0                /* Is this the boot CPU? */
> +	streq r1, [r0]
> +	/* Continuing ugliness: Set up the GIC so NS state owns interrupts,
> +	 * The first 32 interrupts (SGIs & PPIs) must be configured on all
> +	 * CPUs while the remainder are SPIs and only need to be done one, on
> +	 * the boot CPU. */
>  	add   r0, r0, #0x80          /* GICD_IGROUP0 */
>  	mov   r2, #0xffffffff        /* All interrupts to group 1 */
> -	str   r2, [r0]
> -	str   r2, [r0, #4]
> -	str   r2, [r0, #8]
> -	/* Must drop priority mask below 0x80 before entering NS state */
> +	teq   r12, #0                /* Boot CPU? */
> +	str   r2, [r0]               /* Interrupts  0-31 (SGI & PPI) */
> +	streq r2, [r0, #4]           /* Interrupts 32-63 (SPI) */
> +	streq r2, [r0, #8]           /* Interrupts 64-95 (SPI) */
> +	/* Disable the GIC CPU interface on all processors */
>  	mov   r0, #GIC_BASE_ADDRESS
>  	add   r0, r0, #GIC_CR_OFFSET
> +	mov   r1, #0
> +	str   r1, [r0]		     
> +	/* Must drop priority mask below 0x80 before entering NS state */
>  	ldr   r1, =0xff
>  	str   r1, [r0, #0x4]         /* -> GICC_PMR */
>  	/* Reset a few config registers */

Ian Campbell

2012-Aug-09 09:10 UTC

head link

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

On Tue, 2012-08-07 at 10:42 +0100, Tim Deegan wrote:> Acked-by: Tim Deegan <tim@xen.org>
Thanks, pushed to my arm-for-4.3 branch.

Any comments/objections/acks for patches #2-4?

Ian.

Tim Deegan

2012-Aug-09 11:26 UTC

head link

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

At 10:10 +0100 on 09 Aug (1344507051), Ian Campbell
wrote:> On Tue, 2012-08-07 at 10:42 +0100, Tim Deegan wrote:
> > Acked-by: Tim Deegan <tim@xen.org>
> 
> Thanks, pushed to my arm-for-4.3 branch.
> 
> Any comments/objections/acks for patches #2-4?
Ack to all three.

Tim.

Ian Campbell

2012-Aug-09 11:37 UTC

head link

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

On Thu, 2012-08-09 at 12:26 +0100, Tim Deegan wrote:> At 10:10 +0100 on 09 Aug (1344507051), Ian Campbell wrote:
> > On Tue, 2012-08-07 at 10:42 +0100, Tim Deegan wrote:
> > > Acked-by: Tim Deegan <tim@xen.org>
> > 
> > Thanks, pushed to my arm-for-4.3 branch.
> > 
> > Any comments/objections/acks for patches #2-4?
> 
> Ack to all three.
Thanks, I''ll push shortly.

Ian.

Xen devel - Aug 2012 - [PATCH 0/4] arm: SMP interrupt handling fixes

[PATCH 0/4] arm: SMP interrupt handling fixes

[PATCH 1/4] arm: disable distributor delivery on boot CPU only

[PATCH 2/4] arm: don''t bother setting up vtimer, vgic etc on idle CPUs

[PATCH 3/4] arm/vtimer: convert result to ticks when reading CNTPCT register

[PATCH 4/4] arm: Use per-CPU irq_desc for PPIs and SGIs

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

Re: [PATCH 3/4] arm/vtimer: convert result to ticks when reading CNTPCT register

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only

Re: [PATCH 1/4] arm: disable distributor delivery on boot CPU only