[PATCH v3 0/6] Dissociate logical and gic/hardware CPU ID

[PATCH v3 0/6] Dissociate logical and gic/hardware CPU ID

[Julien Grall]

Hi,

This is the third version for this patch series. All changes can be found in
each patch.

With the Versatile Express TC2, it's possible to boot only with A7 or A15. If
the user choose to boot with only A7, the CPU ID will start at 0x100. As Xen
relies on it to set the logical ID and the GIC, it won't be possible to use
Xen with this use case.

This patch series is divided in 3 parts:
    - Patch 1: prepare Xen
    - Patch 2-4: dissociate logical and gic CPU ID
    - Patch 5-6: dissociate logical and hardware CPU ID

For the moment this patch series only modifies Xen and not the boot process
(ie head.S). So if the boot CPU ID is not equal to 0 you won't be able to start
Xen. The future Ian Campbel's patch series should resolve this issue.

The serie also depends on my patch series "Allow Xen to boot with a raw Device
tree".

Cheers,

Julien Grall (6):
  xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq
  xen/arm: Initialize correctly IRQ routing
  xen/arm: gic: Use the correct CPU ID
  xen/arm: Fix assert in send_SGI_one
  xen/arm: Dissociate logical and hardware CPU ID
  xen/arm: Use the hardware ID to boot correctly secondary cpus

 xen/arch/arm/gic.c              |   56 +++++++++++++++-----
 xen/arch/arm/setup.c            |  112 ++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/smpboot.c          |   24 +++++++--
 xen/arch/arm/time.c             |    6 +--
 xen/common/device_tree.c        |   48 -----------------
 xen/include/asm-arm/gic.h       |    3 +-
 xen/include/asm-arm/processor.h |    4 ++
 7 files changed, 181 insertions(+), 72 deletions(-)

-- 
1.7.10.4

[PATCH v3 1/6] xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq

[Julien Grall]

Replace by cpumask_t to take advantage of cpumask_* helpers.

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

---
    Changes in v2:
        - Rework commit message
---
 xen/arch/arm/gic.c        |   20 ++++++++++++--------
 xen/arch/arm/time.c       |    6 +++---
 xen/include/asm-arm/gic.h |    3 ++-
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index aff57b9..091eb36 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -184,10 +184,14 @@ static hw_irq_controller gic_guest_irq_type = {
 
 /* needs to be called with gic.lock held */
 static void gic_set_irq_properties(unsigned int irq, bool_t level,
-        unsigned int cpu_mask, unsigned int priority)
+                                   const cpumask_t *cpu_mask,
+                                   unsigned int priority)
 {
     volatile unsigned char *bytereg;
     uint32_t cfg, edgebit;
+    unsigned int mask = cpumask_bits(cpu_mask)[0];
+
+    ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */
 
     /* Set edge / level */
     cfg = GICD[GICD_ICFGR + irq / 16];
@@ -200,7 +204,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
 
     /* Set target CPU mask (RAZ/WI on uniprocessor) */
     bytereg = (unsigned char *) (GICD + GICD_ITARGETSR);
-    bytereg[irq] = cpu_mask;
+    bytereg[irq] = mask;
 
     /* Set priority */
     bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR);
@@ -210,12 +214,11 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
 
 /* Program the GIC to route an interrupt */
 static int gic_route_irq(unsigned int irq, bool_t level,
-                         unsigned int cpu_mask, unsigned int priority)
+                         const cpumask_t *cpu_mask, unsigned int priority)
 {
     struct irq_desc *desc = irq_to_desc(irq);
     unsigned long flags;
 
-    ASSERT(!(cpu_mask & ~0xff));  /* Targets bitmap only supports 8 CPUs */
     ASSERT(priority <= 0xff);     /* Only 8 bits of priority */
     ASSERT(irq < gic.lines);      /* Can't route interrupts that don't exist */
 
@@ -242,7 +245,7 @@ static int gic_route_irq(unsigned int irq, bool_t level,
 }
 
 /* Program the GIC to route an interrupt with a dt_irq */
-void gic_route_dt_irq(const struct dt_irq *irq, unsigned int cpu_mask,
+void gic_route_dt_irq(const struct dt_irq *irq, const cpumask_t *cpu_mask,
                       unsigned int priority)
 {
     bool_t level;
@@ -496,7 +499,7 @@ void gic_disable_cpu(void)
 void gic_route_ppis(void)
 {
     /* GIC maintenance */
-    gic_route_dt_irq(&gic.maintenance, 1u << smp_processor_id(), 0xa0);
+    gic_route_dt_irq(&gic.maintenance, cpumask_of(smp_processor_id()), 0xa0);
     /* Route timer interrupt */
     route_timer_interrupt();
 }
@@ -511,7 +514,7 @@ void gic_route_spis(void)
         if ( (irq = serial_dt_irq(seridx)) == NULL )
             continue;
 
-        gic_route_dt_irq(irq, 1u << smp_processor_id(), 0xa0);
+        gic_route_dt_irq(irq, cpumask_of(smp_processor_id()), 0xa0);
     }
 }
 
@@ -718,7 +721,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq,
 
     level = dt_irq_is_level_triggered(irq);
 
-    gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0);
+    gic_set_irq_properties(irq->irq, level, cpumask_of(smp_processor_id()),
+                           0xa0);
 
     retval = __setup_irq(desc, irq->irq, action);
     if (retval) {
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index eb3ad5c..a30d422 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -222,11 +222,11 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
 void __cpuinit route_timer_interrupt(void)
 {
     gic_route_dt_irq(&timer_irq[TIMER_PHYS_NONSECURE_PPI],
-                     1u << smp_processor_id(), 0xa0);
+                     cpumask_of(smp_processor_id()), 0xa0);
     gic_route_dt_irq(&timer_irq[TIMER_HYP_PPI],
-                     1u << smp_processor_id(), 0xa0);
+                     cpumask_of(smp_processor_id()), 0xa0);
     gic_route_dt_irq(&timer_irq[TIMER_VIRT_PPI],
-                     1u << smp_processor_id(), 0xa0);
+                     cpumask_of(smp_processor_id()), 0xa0);
 }
 
 /* Set up the timer interrupt on this CPU */
diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
index 92a3349..3a5ef6f 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -147,7 +147,8 @@ extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 
 /* Program the GIC to route an interrupt with a dt_irq */
-extern void gic_route_dt_irq(const struct dt_irq *irq, unsigned int cpu_mask,
+extern void gic_route_dt_irq(const struct dt_irq *irq,
+                             const cpumask_t *cpu_mask,
                              unsigned int priority);
 extern void gic_route_ppis(void);
 extern void gic_route_spis(void);
-- 
1.7.10.4

Re: [PATCH v3 1/6] xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq

[Ian Campbell]

On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:
> Replace by cpumask_t to take advantage of cpumask_* helpers.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH v3 2/6] xen/arm: Initialize correctly IRQ routing

[Julien Grall]

When Xen initialize the GIC distributor, we need to route all the IRQs to
the boot CPU. The CPU ID can differ between Xen and the GIC.

When ITARGETSR0 is read, each field will return a value that corresponds
only to the processor reading the register. So Xen can use the PPI 0 to
initialize correctly the routing.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 091eb36..b969d23 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -258,9 +258,10 @@ void gic_route_dt_irq(const struct dt_irq *irq, const cpumask_t *cpu_mask,
 static void __init gic_dist_init(void)
 {
     uint32_t type;
-    uint32_t cpumask = 1 << smp_processor_id();
+    uint32_t cpumask;
     int i;
 
+    cpumask = GICD[GICD_ITARGETSR] & 0xff;
     cpumask |= cpumask << 8;
     cpumask |= cpumask << 16;
 
-- 
1.7.10.4

Re: [PATCH v3 2/6] xen/arm: Initialize correctly IRQ routing

[Ian Campbell]

On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:
> When Xen initialize the GIC distributor, we need to route all the IRQs to
> the boot CPU. The CPU ID can differ between Xen and the GIC.
> 
> When ITARGETSR0 is read, each field will return a value that corresponds
> only to the processor reading the register. So Xen can use the PPI 0 to
> initialize correctly the routing.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

The GIC mapping of CPU interfaces does not necessarily match the logical
CPU numbering.

When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID.
It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC
will return a value that corresponds only to the processor reading the register.
So Xen can use the PPI 0 to initialize the mapping.

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

---
    Changes in v3:
        - Correctly create the mask in gic_cpu_mask

    Changes in v2:
        - Use per-cpu variable instead of an array
        - Add comment for NR_GIC_CPU_IF
---
 xen/arch/arm/gic.c |   37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index b969d23..4061691 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask);
 
 static unsigned nr_lrs;
 
+/* The GIC mapping of CPU interfaces does not necessarily match the
+ * logical CPU numbering. Let's use mapping as returned by the GIC
+ * itself
+ */
+static DEFINE_PER_CPU(u8, gic_cpu_id);
+
+/* Maximum cpu interface per GIC */
+#define NR_GIC_CPU_IF 8
+
+static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
+{
+    unsigned int cpu;
+    unsigned int mask = 0;
+
+    for_each_cpu(cpu, cpumask)
+    {
+        ASSERT(cpu < NR_GIC_CPU_IF);
+        mask |= per_cpu(gic_cpu_id, cpu);
+    }
+
+    return mask;
+}
+
 unsigned int gic_number_lines(void)
 {
     return gic.lines;
@@ -189,9 +212,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
 {
     volatile unsigned char *bytereg;
     uint32_t cfg, edgebit;
-    unsigned int mask = cpumask_bits(cpu_mask)[0];
-
-    ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */
+    unsigned int mask = gic_cpu_mask(cpu_mask);
 
     /* Set edge / level */
     cfg = GICD[GICD_ICFGR + irq / 16];
@@ -300,6 +321,8 @@ static void __cpuinit gic_cpu_init(void)
 {
     int i;
 
+    this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff;
+
     /* 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. */
@@ -431,13 +454,13 @@ void __init gic_init(void)
 
 void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
 {
-    unsigned long mask = cpumask_bits(cpumask)[0];
+    cpumask_t online_mask;
+    unsigned int mask = 0;
 
     ASSERT(sgi < 16); /* There are only 16 SGIs */
 
-    mask &= cpumask_bits(&cpu_online_map)[0];
-
-    ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */
+    cpumask_and(&online_mask, cpumask, &cpu_online_map);
+    mask = gic_cpu_mask(&online_mask);
 
     dsb();
 
-- 
1.7.10.4

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

On 09/18/2013 02:15 PM, Julien Grall wrote:
> The GIC mapping of CPU interfaces does not necessarily match the logical
> CPU numbering.
> 
> When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID.
> It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC
> will return a value that corresponds only to the processor reading the register.
> So Xen can use the PPI 0 to initialize the mapping.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v3:
>         - Correctly create the mask in gic_cpu_mask
> 
>     Changes in v2:
>         - Use per-cpu variable instead of an array
>         - Add comment for NR_GIC_CPU_IF
> ---
>  xen/arch/arm/gic.c |   37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index b969d23..4061691 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask);
>  
>  static unsigned nr_lrs;
>  
> +/* The GIC mapping of CPU interfaces does not necessarily match the
> + * logical CPU numbering. Let's use mapping as returned by the GIC
> + * itself
> + */
> +static DEFINE_PER_CPU(u8, gic_cpu_id);
> +

Following the discussion on another thread
(http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we
need to initialize each CPU ID to 0xff by default, to be able to bring
up secondary CPUs with an SGI (send_SGI_mask).
But as I understand, per_cpu area for a give area is initialized in
cpu_up and there is no way to give a default value. So user could call
send_SGI_mask to early function and use an invalid CPU area.

IMHO, this solution is too fragile and I would like to use the same
solution as my first version (ie with an array of 8 cells).

Ian, is ok for you?

> +/* Maximum cpu interface per GIC */
> +#define NR_GIC_CPU_IF 8
> +
> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> +{
> +    unsigned int cpu;
> +    unsigned int mask = 0;
> +
> +    for_each_cpu(cpu, cpumask)
> +    {
> +        ASSERT(cpu < NR_GIC_CPU_IF);
> +        mask |= per_cpu(gic_cpu_id, cpu);
> +    }
> +
> +    return mask;
> +}
> +
>  unsigned int gic_number_lines(void)
>  {
>      return gic.lines;
> @@ -189,9 +212,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>  {
>      volatile unsigned char *bytereg;
>      uint32_t cfg, edgebit;
> -    unsigned int mask = cpumask_bits(cpu_mask)[0];
> -
> -    ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */
> +    unsigned int mask = gic_cpu_mask(cpu_mask);
>  
>      /* Set edge / level */
>      cfg = GICD[GICD_ICFGR + irq / 16];
> @@ -300,6 +321,8 @@ static void __cpuinit gic_cpu_init(void)
>  {
>      int i;
>  
> +    this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff;
> +
>      /* 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. */
> @@ -431,13 +454,13 @@ void __init gic_init(void)
>  
>  void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
>  {
> -    unsigned long mask = cpumask_bits(cpumask)[0];
> +    cpumask_t online_mask;
> +    unsigned int mask = 0;
>  
>      ASSERT(sgi < 16); /* There are only 16 SGIs */
>  
> -    mask &= cpumask_bits(&cpu_online_map)[0];
> -
> -    ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */
> +    cpumask_and(&online_mask, cpumask, &cpu_online_map);
> +    mask = gic_cpu_mask(&online_mask);
>  
>      dsb();
>  
> 


-- 
Julien Grall

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Fri, 2013-09-20 at 13:44 +0100, Julien Grall wrote:
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index b969d23..4061691 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask);
> >  
> >  static unsigned nr_lrs;
> >  
> > +/* The GIC mapping of CPU interfaces does not necessarily match the
> > + * logical CPU numbering. Let's use mapping as returned by the GIC
> > + * itself
> > + */
> > +static DEFINE_PER_CPU(u8, gic_cpu_id);
> > +
> 
> Following the discussion on another thread
> (http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we
> need to initialize each CPU ID to 0xff by default, to be able to bring
> up secondary CPUs with an SGI (send_SGI_mask).

We may as well use the send_SGI_allbutself mechanism, which doesn't rely
on knowing and target ids.

> But as I understand, per_cpu area for a give area is initialized in
> cpu_up and there is no way to give a default value. So user could call
> send_SGI_mask to early function and use an invalid CPU area.

If we need to go down this path then I would write something like
	cpu_online(cpu) ? this_cpu(thing, cpu) :  0xff
but I don't think we need to because we can easily avoid the problem.

> IMHO, this solution is too fragile and I would like to use the same
> solution as my first version (ie with an array of 8 cells).

I think cpu bring up should use allbutself and the other callers can
reasonable be restricted to only operate on cpus which are up and
therefore have a valid mask already allocated and initialised.

Ian.

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

On 09/20/2013 02:36 PM, Ian Campbell wrote:
> On Fri, 2013-09-20 at 13:44 +0100, Julien Grall wrote:
>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>> index b969d23..4061691 100644
>>> --- a/xen/arch/arm/gic.c
>>> +++ b/xen/arch/arm/gic.c
>>> @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask);
>>>  
>>>  static unsigned nr_lrs;
>>>  
>>> +/* The GIC mapping of CPU interfaces does not necessarily match the
>>> + * logical CPU numbering. Let's use mapping as returned by the GIC
>>> + * itself
>>> + */
>>> +static DEFINE_PER_CPU(u8, gic_cpu_id);
>>> +
>>
>> Following the discussion on another thread
>> (http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we
>> need to initialize each CPU ID to 0xff by default, to be able to bring
>> up secondary CPUs with an SGI (send_SGI_mask).
> 
> We may as well use the send_SGI_allbutself mechanism, which doesn't rely
> on knowing and target ids.
>> But as I understand, per_cpu area for a give area is initialized in
>> cpu_up and there is no way to give a default value. So user could call
>> send_SGI_mask to early function and use an invalid CPU area.
> 
> If we need to go down this path then I would write something like
> 	cpu_online(cpu) ? this_cpu(thing, cpu) :  0xff
> but I don't think we need to because we can easily avoid the problem.
> 
>> IMHO, this solution is too fragile and I would like to use the same
>> solution as my first version (ie with an array of 8 cells).
> 
> I think cpu bring up should use allbutself and the other callers can
> reasonable be restricted to only operate on cpus which are up and
> therefore have a valid mask already allocated and initialised.


Ok, so I will, at least, add a comment in the gic code to explain this
restriction.

-- 
Julien Grall

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

I only resend this patch.

====================================================================================

commit 89aafa2d3d8a4f9ebce700673f89c8b4822a02e8
Author: Julien Grall <julien.grall@linaro.org>
Date:   Thu Aug 29 18:38:06 2013 +0100

    xen/arm: gic: Use the correct CPU ID
    
    The GIC mapping of CPU interfaces does not necessarily match the logical
    CPU numbering.
    
    When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID.
    It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC
    will return a value that corresponds only to the processor reading the register.
    So Xen can use the PPI 0 to initialize the mapping.
    
    Signed-off-by: Julien Grall <julien.grall@linaro.org>
    
    ---
        Changes in v4:
            - Make logical and between the cpumask given in arguments and
            cpu_possible_map.
            - Make sure the per_cpu is initialized
            - Add comment restriction for gic_set_irq_properties
    
        Changes in v3:
            - Correctly create the mask in gic_cpu_mask
    
        Changes in v2:
            - Use per-cpu variable instead of an array
            - Add comment for NR_GIC_CPU_IF

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index b969d23..9f703a0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -57,6 +57,32 @@ static DEFINE_PER_CPU(uint64_t, lr_mask);
 
 static unsigned nr_lrs;
 
+/* The GIC mapping of CPU interfaces does not necessarily match the
+ * logical CPU numbering. Let's use mapping as returned by the GIC
+ * itself
+ */
+static DEFINE_PER_CPU(u8, gic_cpu_id);
+
+/* Maximum cpu interface per GIC */
+#define NR_GIC_CPU_IF 8
+
+static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
+{
+    unsigned int cpu;
+    unsigned int mask = 0;
+    cpumask_t possible_mask;
+
+    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
+    for_each_cpu(cpu, &possible_mask)
+    {
+        ASSERT(cpu < NR_GIC_CPU_IF);
+        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
+        mask |= per_cpu(gic_cpu_id, cpu);
+    }
+
+    return mask;
+}
+
 unsigned int gic_number_lines(void)
 {
     return gic.lines;
@@ -182,16 +208,18 @@ static hw_irq_controller gic_guest_irq_type = {
     .set_affinity = gic_irq_set_affinity,
 };
 
-/* needs to be called with gic.lock held */
+/*
+ * - needs to be called with gic.lock held
+ * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
+ * already called gic_cpu_init
+ */
 static void gic_set_irq_properties(unsigned int irq, bool_t level,
                                    const cpumask_t *cpu_mask,
                                    unsigned int priority)
 {
     volatile unsigned char *bytereg;
     uint32_t cfg, edgebit;
-    unsigned int mask = cpumask_bits(cpu_mask)[0];
-
-    ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */
+    unsigned int mask = gic_cpu_mask(cpu_mask);
 
     /* Set edge / level */
     cfg = GICD[GICD_ICFGR + irq / 16];
@@ -300,6 +328,8 @@ static void __cpuinit gic_cpu_init(void)
 {
     int i;
 
+    this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff;
+
     /* 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. */
@@ -431,13 +461,13 @@ void __init gic_init(void)
 
 void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
 {
-    unsigned long mask = cpumask_bits(cpumask)[0];
+    unsigned int mask = 0;
+    cpumask_t online_mask;
 
     ASSERT(sgi < 16); /* There are only 16 SGIs */
 
-    mask &= cpumask_bits(&cpu_online_map)[0];
-
-    ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */
+    cpumask_and(&online_mask, cpumask, &cpu_online_map);
+    mask = gic_cpu_mask(&online_mask);
 
     dsb();

-- 
Julien Grall

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:

> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> +{
> +    unsigned int cpu;
> +    unsigned int mask = 0;
> +    cpumask_t possible_mask;
> +
> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);

Based on the other subconversation doesn't this need to be online_mask?
Or is the cpu area setup for cpus which are possible but not online?

Should the check (whichever it is) be an assertion?

> +    for_each_cpu(cpu, &possible_mask)
> +    {
> +        ASSERT(cpu < NR_GIC_CPU_IF);
> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> +        mask |= per_cpu(gic_cpu_id, cpu);
> +    }
> +
> +    return mask;
> +}

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

On 09/20/2013 04:44 PM, Ian Campbell wrote:
> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> 
>> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
>> +{
>> +    unsigned int cpu;
>> +    unsigned int mask = 0;
>> +    cpumask_t possible_mask;
>> +
>> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> 
> Based on the other subconversation doesn't this need to be online_mask?
> Or is the cpu area setup for cpus which are possible but not online?

cpu area is initialized a little bit before the cpu is bring up:
  * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE)
  * signal the cpu to boot (__cpu_up)
  ...
  * call start_secondary
      * initialize the gic cpu interface (gic_cpu_init)
      * route ppis (which used gic_cpu_mask)
      * set the cpu online

If we use the cpu online mask, Xen won't be able to route the different
ppis to the processor.

> Should the check (whichever it is) be an assertion?

If we stay with the cpu_possible_map, I think we are fine without an
assertion.

>> +    for_each_cpu(cpu, &possible_mask)
>> +    {
>> +        ASSERT(cpu < NR_GIC_CPU_IF);
>> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
>> +        mask |= per_cpu(gic_cpu_id, cpu);
>> +    }
>> +
>> +    return mask;
>> +}
> 
> 


-- 
Julien Grall

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Fri, 2013-09-20 at 16:58 +0100, Julien Grall wrote:
> On 09/20/2013 04:44 PM, Ian Campbell wrote:
> > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> > 
> >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> >> +{
> >> +    unsigned int cpu;
> >> +    unsigned int mask = 0;
> >> +    cpumask_t possible_mask;
> >> +
> >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> > 
> > Based on the other subconversation doesn't this need to be online_mask?
> > Or is the cpu area setup for cpus which are possible but not online?
> 
> cpu area is initialized a little bit before the cpu is bring up:
>   * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE)
>   * signal the cpu to boot (__cpu_up)
>   ...
>   * call start_secondary
>       * initialize the gic cpu interface (gic_cpu_init)
>       * route ppis (which used gic_cpu_mask)
>       * set the cpu online
> 
> If we use the cpu online mask, Xen won't be able to route the different
> ppis to the processor.
> 
> > Should the check (whichever it is) be an assertion?
> 
> If we stay with the cpu_possible_map, I think we are fine without an
> assertion.

OK.

OOI where in the above is the cpu_possible_map setup? Quite a bit before
the CPU_UP_PREPARE hook I think? It shouldn't be a problem, just
interested...

Ian

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

On 09/20/2013 05:06 PM, Ian Campbell wrote:
> On Fri, 2013-09-20 at 16:58 +0100, Julien Grall wrote:
>> On 09/20/2013 04:44 PM, Ian Campbell wrote:
>>> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
>>>
>>>> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
>>>> +{
>>>> +    unsigned int cpu;
>>>> +    unsigned int mask = 0;
>>>> +    cpumask_t possible_mask;
>>>> +
>>>> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
>>>
>>> Based on the other subconversation doesn't this need to be online_mask?
>>> Or is the cpu area setup for cpus which are possible but not online?
>>
>> cpu area is initialized a little bit before the cpu is bring up:
>>    * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE)
>>    * signal the cpu to boot (__cpu_up)
>>    ...
>>    * call start_secondary
>>        * initialize the gic cpu interface (gic_cpu_init)
>>        * route ppis (which used gic_cpu_mask)
>>        * set the cpu online
>>
>> If we use the cpu online mask, Xen won't be able to route the different
>> ppis to the processor.
>>
>>> Should the check (whichever it is) be an assertion?
>>
>> If we stay with the cpu_possible_map, I think we are fine without an
>> assertion.
>
> OK.
>
> OOI where in the above is the cpu_possible_map setup? Quite a bit before
> the CPU_UP_PREPARE hook I think? It shouldn't be a problem, just
> interested...

With this patch series, it's initialized by init_cpus_maps.

-- 
Julien Grall

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> +{
> +    unsigned int cpu;
> +    unsigned int mask = 0;
> +    cpumask_t possible_mask;
> +
> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> +    for_each_cpu(cpu, &possible_mask)
> +    {
> +        ASSERT(cpu < NR_GIC_CPU_IF);
> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);

This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
think we can live without this check. After all the CPU is in possible
map.

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

On 09/25/2013 04:35 PM, Ian Campbell wrote:
> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
>> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
>> +{
>> +    unsigned int cpu;
>> +    unsigned int mask = 0;
>> +    cpumask_t possible_mask;
>> +
>> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
>> +    for_each_cpu(cpu, &possible_mask)
>> +    {
>> +        ASSERT(cpu < NR_GIC_CPU_IF);
>> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> 
> This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> think we can live without this check. After all the CPU is in possible
> map.

Being in cpu possible map doesn't mean that the per cpu region is
initialized for the given cpu.
I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
platform. Can we move this define in percpu.h?

-- 
Julien Grall

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote:
> On 09/25/2013 04:35 PM, Ian Campbell wrote:
> > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> >> +{
> >> +    unsigned int cpu;
> >> +    unsigned int mask = 0;
> >> +    cpumask_t possible_mask;
> >> +
> >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> >> +    for_each_cpu(cpu, &possible_mask)
> >> +    {
> >> +        ASSERT(cpu < NR_GIC_CPU_IF);
> >> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> > 
> > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> > think we can live without this check. After all the CPU is in possible
> > map.
> 
> Being in cpu possible map doesn't mean that the per cpu region is
> initialized for the given cpu.

So you expect this ASSERT to trigger in practice? That's not good...

> I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
> platform. Can we move this define in percpu.h?

I'm wondering if it should be the same, I think it was chosen on x86 to
result in a non-canonical address (i.e. a guaranteed fault) and ARM
copied it.

__per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB
from the top of the 64 bit address space, which is also invalid. I
think, Tim, did you consider this or just copy the x86 value?

Anyway, my point is that they are the same only through coincidence ;-)

Ian.

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Wed, 2013-09-25 at 16:48 +0100, Ian Campbell wrote:
> On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote:
> > On 09/25/2013 04:35 PM, Ian Campbell wrote:
> > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> > >> +{
> > >> +    unsigned int cpu;
> > >> +    unsigned int mask = 0;
> > >> +    cpumask_t possible_mask;
> > >> +
> > >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> > >> +    for_each_cpu(cpu, &possible_mask)
> > >> +    {
> > >> +        ASSERT(cpu < NR_GIC_CPU_IF);
> > >> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> > > 
> > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> > > think we can live without this check. After all the CPU is in possible
> > > map.
> > 
> > Being in cpu possible map doesn't mean that the per cpu region is
> > initialized for the given cpu.
> 
> So you expect this ASSERT to trigger in practice? That's not good...
> 
> > I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
> > platform. Can we move this define in percpu.h?
> 
> I'm wondering if it should be the same, I think it was chosen on x86 to
> result in a non-canonical address (i.e. a guaranteed fault) and ARM
> copied it.
> 
> __per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB
> from the top of the 64 bit address space, which is also invalid. I
> think, Tim, did you consider this or just copy the x86 value?

Actually, since the offset of the per_cpu var gets added the result is
not invalid at all, it'll be the offset within the percpu of the area
(i.e. a small number). Luckily we deliberately don't map anything at
0..2MB and Xen is smaller than 2MB and per_cpu stuff fits inside Xen. So
it works out OK (where OK means deliberately traps). Phew.

Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Tim Deegan]

At 16:53 +0100 on 25 Sep (1380128037), Ian Campbell wrote:
> On Wed, 2013-09-25 at 16:48 +0100, Ian Campbell wrote:
> > On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote:
> > > On 09/25/2013 04:35 PM, Ian Campbell wrote:
> > > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> > > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> > > >> +{
> > > >> +    unsigned int cpu;
> > > >> +    unsigned int mask = 0;
> > > >> +    cpumask_t possible_mask;
> > > >> +
> > > >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> > > >> +    for_each_cpu(cpu, &possible_mask)
> > > >> +    {
> > > >> +        ASSERT(cpu < NR_GIC_CPU_IF);
> > > >> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> > > > 
> > > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> > > > think we can live without this check. After all the CPU is in possible
> > > > map.
> > > 
> > > Being in cpu possible map doesn't mean that the per cpu region is
> > > initialized for the given cpu.
> > 
> > So you expect this ASSERT to trigger in practice? That's not good...
> > 
> > > I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
> > > platform. Can we move this define in percpu.h?
> > 
> > I'm wondering if it should be the same, I think it was chosen on x86 to
> > result in a non-canonical address (i.e. a guaranteed fault) and ARM
> > copied it.
> > 
> > __per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB
> > from the top of the 64 bit address space, which is also invalid. I
> > think, Tim, did you consider this or just copy the x86 value?
> 
> Actually, since the offset of the per_cpu var gets added the result is
> not invalid at all, it'll be the offset within the percpu of the area
> (i.e. a small number). Luckily we deliberately don't map anything at
> 0..2MB and Xen is smaller than 2MB and per_cpu stuff fits inside Xen. So
> it works out OK (where OK means deliberately traps). Phew.

Yes, it maps to NULL+offset, where offset is comfortably < 1 page in the
current build; AFAICS this was deliberate on x86 and certainly
deliberate when I copied it.  The stronger property that offset must
always < 2MB didn't occur to me, but is reassuring. :)

Tim.

[PATCH v3 4/6] xen/arm: Fix assert in send_SGI_one

[Julien Grall]

The GIC can handle maximum 8 cpus (0...7). The CPU id 7 is still valid.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/gic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 4061691..9b7457a 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -471,7 +471,7 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
 
 void send_SGI_one(unsigned int cpu, enum gic_sgi sgi)
 {
-    ASSERT(cpu < 7);  /* Targets bitmap only supports 8 CPUs */
+    ASSERT(cpu < NR_GIC_CPU_IF);  /* Targets bitmap only supports 8 CPUs */
     send_SGI_mask(cpumask_of(cpu), sgi);
 }
 
-- 
1.7.10.4

Re: [PATCH v3 4/6] xen/arm: Fix assert in send_SGI_one

[Ian Campbell]

On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:
> The GIC can handle maximum 8 cpus (0...7). The CPU id 7 is still valid.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH v3 5/6] xen/arm: Dissociate logical and hardware CPU ID

[Julien Grall]

Introduce cpu_logical_map to associate a logical CPU ID to an hardware CPU ID.
This map will be filled during Xen boot via the device tree. Each CPU node
contains a "reg" property which contains the hardware ID (ie MPIDR[0:23]).

Also move /cpus parsing later so we can use the dt_* API.

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

---
    Changes in v3:
        - re-add the mask (lost v2) for the mpidr.

    Changes in v2:
        - for_each_child was renamed to dt_for_each_child
        - move init_cpus_maps later to take advantage of boot_cpu_data
        - move to initdata tmp_map
---
 xen/arch/arm/setup.c            |  112 ++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/smpboot.c          |    4 ++
 xen/common/device_tree.c        |   48 -----------------
 xen/include/asm-arm/processor.h |    4 ++
 4 files changed, 119 insertions(+), 49 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index aa87fb1..7dd6c9f 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -498,6 +498,114 @@ void __init setup_cache(void)
     cacheline_bytes = 1U << (4 + (ccsid & 0x7));
 }
 
+/* Parse the device tree and build the logical map array containing
+ * MPIDR values related to logical cpus
+ * Code base on Linux arch/arm/kernel/devtree.c
+ */
+static void __init init_cpus_maps(void)
+{
+    register_t mpidr;
+    struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
+    struct dt_device_node *cpu;
+    unsigned int i, j;
+    unsigned int cpuidx = 1;
+    static u32 tmp_map[NR_CPUS] __initdata =
+    {
+        [0 ... NR_CPUS - 1] = MPIDR_INVALID
+    };
+    bool_t bootcpu_valid = 0;
+
+    mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK;
+
+    if ( !cpus )
+    {
+        printk(XENLOG_WARNING "WARNING: Can't find /cpus in the device tree.\n"
+               "Using only 1 CPU\n");
+        return;
+    }
+
+    dt_for_each_child_node( cpus, cpu )
+    {
+        u32 hwid;
+
+        if ( !dt_device_type_is_equal(cpu, "cpu") )
+            continue;
+
+        if ( !dt_property_read_u32(cpu, "reg", &hwid) )
+        {
+            printk(XENLOG_WARNING "cpu node `%s`: missing reg property\n",
+                   dt_node_full_name(cpu));
+            continue;
+        }
+
+        /*
+         * 8 MSBs must be set to 0 in the DT since the reg property
+         * defines the MPIDR[23:0]
+         */
+        if ( hwid & ~MPIDR_HWID_MASK )
+        {
+            printk(XENLOG_WARNING "cpu node `%s`: invalid hwid value (0x%x)\n",
+                   dt_node_full_name(cpu), hwid);
+            continue;
+        }
+
+        /*
+         * Duplicate MPIDRs are a recipe for disaster. Scan all initialized
+         * entries and check for duplicates. If any found just skip the node.
+         * temp values values are initialized to MPIDR_INVALID to avoid
+         * matching valid MPIDR[23:0] values.
+         */
+        for ( j = 0; j < cpuidx; j++ )
+        {
+            if ( tmp_map[j] == hwid )
+            {
+                printk(XENLOG_WARNING "cpu node `%s`: duplicate /cpu reg properties in the DT\n",
+                       dt_node_full_name(cpu));
+                continue;
+            }
+        }
+
+        /*
+         * Build a stashed array of MPIDR values. Numbering scheme requires
+         * that if detected the boot CPU must be assigned logical id 0. Other
+         * CPUs get sequential indexes starting from 1. If a CPU node
+         * with a reg property matching the boot CPU MPIDR is detected,
+         * this is recorded and so that the logical map build from DT is
+         * validated and can be used to set the map.
+         */
+        if ( hwid == mpidr )
+        {
+            i = 0;
+            bootcpu_valid = 1;
+        }
+        else
+            i = cpuidx++;
+
+        if ( cpuidx > NR_CPUS )
+        {
+            printk(XENLOG_WARNING "DT /cpu %u node greater than max cores %u, capping them\n",
+                   cpuidx, NR_CPUS);
+            cpuidx = NR_CPUS;
+            break;
+        }
+
+        tmp_map[i] = hwid;
+    }
+
+    if ( !bootcpu_valid )
+    {
+        printk(XENLOG_WARNING "DT missing boot CPU MPIDR[23:0]\n"
+               "Using only 1 CPU\n");
+        return;
+    }
+
+    for ( i = 0; i < cpuidx; i++ )
+    {
+        cpumask_set_cpu(i, &cpu_possible_map);
+        cpu_logical_map(i) = tmp_map[i];
+    }
+}
+
 /* C entry point for boot CPU */
 void __init start_xen(unsigned long boot_phys_offset,
                       unsigned long fdt_paddr,
@@ -517,7 +625,6 @@ void __init start_xen(unsigned long boot_phys_offset,
         + (fdt_paddr & ((1 << SECOND_SHIFT) - 1));
     fdt_size = device_tree_early_init(device_tree_flattened);
 
-    cpus = smp_get_max_cpus();
     cmdline_parse(device_tree_bootargs(device_tree_flattened));
 
     setup_pagetables(boot_phys_offset, get_xen_paddr());
@@ -534,6 +641,9 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     processor_id();
 
+    init_cpus_maps();
+    cpus = smp_get_max_cpus();
+
     platform_init();
 
     init_xen_time();
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index b6aea63..c0d25de 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -39,6 +39,9 @@ EXPORT_SYMBOL(cpu_possible_map);
 
 struct cpuinfo_arm cpu_data[NR_CPUS];
 
+/* CPU logical map: map xen cpuid to an MPIDR */
+u32 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
+
 /* Fake one node for now. See also include/asm-arm/numa.h */
 nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
 
@@ -82,6 +85,7 @@ smp_clear_cpu_maps (void)
     cpumask_clear(&cpu_online_map);
     cpumask_set_cpu(0, &cpu_online_map);
     cpumask_set_cpu(0, &cpu_possible_map);
+    cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK;
 }
 
 int __init
diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 0ece249..9a16650 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -118,18 +118,6 @@ static bool_t __init device_tree_node_matches(const void *fdt, int node,
         && (name[match_len] == '@' || name[match_len] == '\0');
 }
 
-static bool_t __init device_tree_type_matches(const void *fdt, int node,
-                                       const char *match)
-{
-    const void *prop;
-
-    prop = fdt_getprop(fdt, node, "device_type", NULL);
-    if ( prop == NULL )
-        return 0;
-
-    return !dt_node_cmp(prop, match);
-}
-
 static bool_t __init device_tree_node_compatible(const void *fdt, int node,
                                                  const char *match)
 {
@@ -348,40 +336,6 @@ static void __init process_memory_node(const void *fdt, int node,
     }
 }
 
-static void __init process_cpu_node(const void *fdt, int node,
-                                    const char *name,
-                                    u32 address_cells, u32 size_cells)
-{
-    const struct fdt_property *prop;
-    u32 cpuid;
-    int len;
-
-    prop = fdt_get_property(fdt, node, "reg", &len);
-    if ( !prop )
-    {
-        early_printk("fdt: node `%s': missing `reg' property\n", name);
-        return;
-    }
-
-    if ( len < sizeof (cpuid) )
-    {
-        dt_printk("fdt: node `%s': `reg` property length is too short\n",
-                  name);
-        return;
-    }
-
-    cpuid = dt_read_number((const __be32 *)prop->data, 1);
-
-    /* TODO: handle non-contiguous CPU ID */
-    if ( cpuid >= NR_CPUS )
-    {
-        dt_printk("fdt: node `%s': reg(0x%x) >= NR_CPUS(%d)\n",
-                  name, cpuid, NR_CPUS);
-        return;
-    }
-    cpumask_set_cpu(cpuid, &cpu_possible_map);
-}
-
 static void __init process_multiboot_node(const void *fdt, int node,
                                           const char *name,
                                           u32 address_cells, u32 size_cells)
@@ -435,8 +389,6 @@ static int __init early_scan_node(const void *fdt,
 {
     if ( device_tree_node_matches(fdt, node, "memory") )
         process_memory_node(fdt, node, name, address_cells, size_cells);
-    else if ( device_tree_type_matches(fdt, node, "cpu") )
-        process_cpu_node(fdt, node, name, address_cells, size_cells);
     else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) )
         process_multiboot_node(fdt, node, name, address_cells, size_cells);
 
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 808567e..0646422 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -13,6 +13,7 @@
 #define MPIDR_AFF0_SHIFT    (0)
 #define MPIDR_AFF0_MASK     (0xff << MPIDR_AFF0_SHIFT)
 #define MPIDR_HWID_MASK     0xffffff
+#define MPIDR_INVALID       (~MPIDR_HWID_MASK)
 
 /* TTBCR Translation Table Base Control Register */
 #define TTBCR_EAE    0x80000000
@@ -230,6 +231,9 @@ extern void identify_cpu(struct cpuinfo_arm *);
 extern struct cpuinfo_arm cpu_data[];
 #define current_cpu_data cpu_data[smp_processor_id()]
 
+extern u32 __cpu_logical_map[];
+#define cpu_logical_map(cpu) __cpu_logical_map[cpu]
+
 union hsr {
     uint32_t bits;
     struct {
-- 
1.7.10.4

Re: [PATCH v3 5/6] xen/arm: Dissociate logical and hardware CPU ID

[Ian Campbell]

On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:
> Introduce cpu_logical_map to associate a logical CPU ID to an hardware CPU ID.
> This map will be filled during Xen boot via the device tree. Each CPU node
> contains a "reg" property which contains the hardware ID (ie MPIDR[0:23]).
> 
> Also move /cpus parsing later so we can use the dt_* API.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

[PATCH v3 6/6] xen/arm: Use the hardware ID to boot correctly secondary cpus

[Julien Grall]

Secondary CPUs will spin in head.S until their MPIDR[23:0] correspond to
the smp_up_cpu. Actually Xen will set the value with the logical CPU ID
which is wrong. Use the cpu_logical_map to get the correct CPU ID.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
    Changes in v2:
        - Replace Acked-by by a s-o-b
        - s/TMP/to in commit title
---
 xen/arch/arm/smpboot.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index c0d25de..1952287 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -124,8 +124,7 @@ make_cpus_ready(unsigned int max_cpus, unsigned long boot_phys_offset)
     for ( i = 1; i < max_cpus; i++ )
     {
         /* Tell the next CPU to get ready */
-        /* TODO: handle boards where CPUIDs are not contiguous */
-        *gate = i;
+        *gate = cpu_logical_map(i);
         flush_xen_dcache(*gate);
         isb();
         sev();
@@ -139,11 +138,22 @@ make_cpus_ready(unsigned int max_cpus, unsigned long boot_phys_offset)
 /* Boot the current CPU */
 void __cpuinit start_secondary(unsigned long boot_phys_offset,
                                unsigned long fdt_paddr,
-                               unsigned long cpuid)
+                               unsigned long hwid)
 {
+    unsigned int cpuid;
+
     memset(get_cpu_info(), 0, sizeof (struct cpu_info));
 
-    /* TODO: handle boards where CPUIDs are not contiguous */
+    /* Browse the logical map and find the associate logical cpu ID */
+    for ( cpuid = 1; cpuid < nr_cpu_ids; cpuid++ )
+    {
+        if ( cpu_logical_map(cpuid) == hwid )
+            break;
+    }
+
+    if ( cpuid == nr_cpu_ids )
+        panic("Can't find logical CPU id for cpu MPIDR[23:0] = 0x%lx\n", hwid);
+
     set_processor_id(cpuid);
 
     current_cpu_data = boot_cpu_data;
@@ -232,7 +242,7 @@ int __cpu_up(unsigned int cpu)
 
     /* Unblock the CPU.  It should be waiting in the loop in head.S
      * for an event to arrive when smp_up_cpu matches its cpuid. */
-    smp_up_cpu = cpu;
+    smp_up_cpu = cpu_logical_map(cpu);
     /* we need to make sure that the change to smp_up_cpu is visible to
      * secondary cpus with D-cache off */
     flush_xen_dcache(smp_up_cpu);
-- 
1.7.10.4

Re: [PATCH v3 6/6] xen/arm: Use the hardware ID to boot correctly secondary cpus

[Ian Campbell]

On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:
> +    /* Browse the logical map and find the associate logical cpu ID */

We already have init_stack (the stack for the cpu which is coming up to
use). In my start of day rewrite I also add/extend init_ttbr. You could
add init_cpu_id or something.

TBH it's getting to the point where we might want struct init_info
{ ... }

> +    for ( cpuid = 1; cpuid < nr_cpu_ids; cpuid++ )
> +    {
> +        if ( cpu_logical_map(cpuid) == hwid )
> +            break;
> +    }
> +

Re: [PATCH v3 6/6] xen/arm: Use the hardware ID to boot correctly secondary cpus

[Julien Grall]

On 09/25/2013 04:41 PM, Ian Campbell wrote:
> On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:
>> +    /* Browse the logical map and find the associate logical cpu ID */
> 
> We already have init_stack (the stack for the cpu which is coming up to
> use). In my start of day rewrite I also add/extend init_ttbr. You could
> add init_cpu_id or something.
> TBH it's getting to the point where we might want struct init_info
> { ... }

Good idea. I will add a patch for this solution in my patch series.

>> +    for ( cpuid = 1; cpuid < nr_cpu_ids; cpuid++ )
>> +    {
>> +        if ( cpu_logical_map(cpuid) == hwid )
>> +            break;
>> +    }
>> +
> 

-- 
Julien Grall