diff mbox

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

Message ID 1378900784-16949-6-git-send-email-julien.grall@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall Sept. 11, 2013, 11:59 a.m. UTC
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(-)

Comments

Ian Campbell Sept. 17, 2013, 2:39 p.m. UTC | #1
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...
Julien Grall Sept. 17, 2013, 3:02 p.m. UTC | #2
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.
Ian Campbell Sept. 17, 2013, 3:08 p.m. UTC | #3
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.
Julien Grall Sept. 17, 2013, 3:18 p.m. UTC | #4
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.
Ian Campbell Sept. 17, 2013, 3:25 p.m. UTC | #5
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.
> 
>
diff mbox

Patch

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 {