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

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

[Julien Grall]

Hi,

This is the second 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-2: prepare Xen
    - Patch 3-5: dissociate logical and gic CPU ID
    - Patch 6-7: 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 v2 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 c5ef8b6..7f11df6 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -201,10 +201,14 @@ void gic_irq_disable(struct irq_desc *desc)
 
 /* 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];
@@ -217,7 +221,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);
@@ -227,12 +231,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 */
 
@@ -259,7 +262,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;
@@ -513,7 +516,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();
 }
@@ -528,7 +531,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);
     }
 }
 
@@ -770,7 +773,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 6b94355..cd936e7 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -148,7 +148,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

[PATCH v2 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 7f11df6..a10416d 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -275,9 +275,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

[PATCH v2 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 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 a10416d..7ea9ed6 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;
@@ -206,9 +229,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];
@@ -317,6 +338,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. */
@@ -448,13 +471,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

[PATCH v2 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 7ea9ed6..88431c7 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -488,7 +488,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

[PATCH v2 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 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..f9aa5c2 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;
+
+    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 b884354..5bc7259 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
@@ -234,6 +235,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

[PATCH v2 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 v2 3/6] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Wed, 2013-09-11 at 12:59 +0100, Julien Grall wrote:
> +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);

Did you mean for this to be |= or something else?

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

[Ian Campbell]

On Wed, 2013-09-11 at 12:59 +0100, Julien Grall wrote:

> +    dt_for_each_child_node( cpus, cpu )
> +    {
> +        u32 hwid;
> +
> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> +            continue;

This could eventually use dt_find_node_by_type which I added in my start
of day rework. I would assume your patch will go in first so I'll try
and remember to do that when I rebase...

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

[Julien Grall]

On 09/17/2013 03:39 PM, Ian Campbell wrote:
> On Wed, 2013-09-11 at 12:59 +0100, Julien Grall wrote:
> 
>> +    dt_for_each_child_node( cpus, cpu )
>> +    {
>> +        u32 hwid;
>> +
>> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
>> +            continue;
> 
> This could eventually use dt_find_node_by_type which I added in my start
> of day rework. I would assume your patch will go in first so I'll try
> and remember to do that when I rebase...

cpu node must be under /cpus. dt_find_node_by_type will look at all the
nodes (not only the child) so we can't replace by this call.

-- 
Julien Grall

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

[Julien Grall]

On 09/17/2013 03:36 PM, Ian Campbell wrote:
> On Wed, 2013-09-11 at 12:59 +0100, Julien Grall wrote:
>> +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);
> 
> Did you mean for this to be |= or something else?

Oh, right. I will update the patch and send it again.

-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2013-09-17 at 16:02 +0100, Julien Grall wrote:
> On 09/17/2013 03:39 PM, Ian Campbell wrote:
> > On Wed, 2013-09-11 at 12:59 +0100, Julien Grall wrote:
> > 
> >> +    dt_for_each_child_node( cpus, cpu )
> >> +    {
> >> +        u32 hwid;
> >> +
> >> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> >> +            continue;
> > 
> > This could eventually use dt_find_node_by_type which I added in my start
> > of day rework. I would assume your patch will go in first so I'll try
> > and remember to do that when I rebase...
> 
> cpu node must be under /cpus.

Must it? Documentation/devicetree/bindings/arm/cpus.txt doesn't mention
that.

But if it were required then wouldn't it be invalid to have a node with
type cpu outside that subtree? IOW looking up by type would still be OK.

FYI this is what arm64 Linux does.

>  dt_find_node_by_type will look at all the
> nodes (not only the child) so we can't replace by this call.

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

[Julien Grall]

On 09/17/2013 04:08 PM, Ian Campbell wrote:
> On Tue, 2013-09-17 at 16:02 +0100, Julien Grall wrote:
>> On 09/17/2013 03:39 PM, Ian Campbell wrote:
>>> On Wed, 2013-09-11 at 12:59 +0100, Julien Grall wrote:
>>>
>>>> +    dt_for_each_child_node( cpus, cpu )
>>>> +    {
>>>> +        u32 hwid;
>>>> +
>>>> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
>>>> +            continue;
>>>
>>> This could eventually use dt_find_node_by_type which I added in my start
>>> of day rework. I would assume your patch will go in first so I'll try
>>> and remember to do that when I rebase...
>>
>> cpu node must be under /cpus.
> 
> Must it? Documentation/devicetree/bindings/arm/cpus.txt doesn't mention
> that.

In Documentation/devicetree/booting-without-of.txt:
 b) The /cpus node

  This node is the parent of all individual CPU nodes. It doesn't
  have any specific requirements, though it's generally good practice
  to have at least:

               #address-cells = <00000001>
               #size-cells    = <00000000>

  This defines that the "address" for a CPU is a single cell, and has
  no meaningful size. This is not necessary but the kernel will assume
  that format when reading the "reg" properties of a CPU node, see
  below

> But if it were required then wouldn't it be invalid to have a node with
> type cpu outside that subtree? IOW looking up by type would still be OK.
> FYI this is what arm64 Linux does.

On arm32 Linux it's only looks in /cpus :).
I'm fine to replace this loop with dt_find_node_by_type.
Will you take care of this change, or do I need to add your patch on my
series and modify the code?

>>  dt_find_node_by_type will look at all the
>> nodes (not only the child) so we can't replace by this call.


-- 
Julien Grall

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

[Ian Campbell]

On Tue, 2013-09-17 at 16:18 +0100, Julien Grall wrote:
> On 09/17/2013 04:08 PM, Ian Campbell wrote:
> > On Tue, 2013-09-17 at 16:02 +0100, Julien Grall wrote:
> >> On 09/17/2013 03:39 PM, Ian Campbell wrote:
> >>> On Wed, 2013-09-11 at 12:59 +0100, Julien Grall wrote:
> >>>
> >>>> +    dt_for_each_child_node( cpus, cpu )
> >>>> +    {
> >>>> +        u32 hwid;
> >>>> +
> >>>> +        if ( !dt_device_type_is_equal(cpu, "cpu") )
> >>>> +            continue;
> >>>
> >>> This could eventually use dt_find_node_by_type which I added in my start
> >>> of day rework. I would assume your patch will go in first so I'll try
> >>> and remember to do that when I rebase...
> >>
> >> cpu node must be under /cpus.
> > 
> > Must it? Documentation/devicetree/bindings/arm/cpus.txt doesn't mention
> > that.
> 
> In Documentation/devicetree/booting-without-of.txt:
>  b) The /cpus node
> 
>   This node is the parent of all individual CPU nodes.

Ah, OK. I think this effectively also requires that there aren't any CPU
nodes anywhere else...
[..]
> O arm32 Linux it's only looks in /cpus :).

;-)

> I'm fine to replace this loop with dt_find_node_by_type.
> Will you take care of this change, or do I need to add your patch on my
> series and modify the code?

I think this series will go in first, so I'll do it...

> 
> >>  dt_find_node_by_type will look at all the
> >> nodes (not only the child) so we can't replace by this call.
> 
>