[PATCH 0/7] Dissociate logical and gic/hardware CPUD ID

[PATCH 0/7] Dissociate logical and gic/hardware CPUD ID

[Julien Grall]

Hi,

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 (7):
  xen/arm: Introduce MPIDR_HWID_MASK
  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 TMP boot correctly secondary cpus

 xen/arch/arm/arm32/head.S       |    2 +-
 xen/arch/arm/gic.c              |   54 ++++++++++++++-----
 xen/arch/arm/setup.c            |  109 ++++++++++++++++++++++++++++++++++++++-
 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 |    5 ++
 8 files changed, 178 insertions(+), 73 deletions(-)

-- 
1.7.10.4

[PATCH 1/7] xen/arm: Introduce MPIDR_HWID_MASK

[Julien Grall]

This define will be use later to retrieve the correct hardware CPU ID.
Also replace hardcoded mask in arm32/head.S by this define.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/arm32/head.S       |    2 +-
 xen/include/asm-arm/processor.h |    1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index b8334e2..79e95b6 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -98,7 +98,7 @@ past_zImage:
         beq   boot_cpu
         tst   r0, #(1<<30)           /* Uniprocessor system? */
         bne   boot_cpu
-        bics  r12, r0, #(0xff << 24) /* Mask out flags to get CPU ID */
+        bics  r12, r0, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
         beq   boot_cpu               /* If we're CPU 0, boot now */
 
         /* Non-boot CPUs wait here to be woken up one at a time. */
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 12795f3..b884354 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -12,6 +12,7 @@
 #define MPIDR_SMP           (1 << 31)
 #define MPIDR_AFF0_SHIFT    (0)
 #define MPIDR_AFF0_MASK     (0xff << MPIDR_AFF0_SHIFT)
+#define MPIDR_HWID_MASK     0xffffff
 
 /* TTBCR Translation Table Base Control Register */
 #define TTBCR_EAE    0x80000000
-- 
1.7.10.4

[PATCH 2/7] xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq

[Julien Grall]

The mapping between logical ID and GIC cpu ID can differs. Currently the code
uses unsigned int for the cpu mask. Replace by cpumask_t to take advante of
cpumask_* helpers.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 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 a43ec23..37a73fb 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;
@@ -508,7 +511,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();
 }
@@ -523,7 +526,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);
     }
 }
 
@@ -765,7 +768,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 8125b92..04ede1e 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -223,11 +223,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 90e887e..26b33bc 100644
--- a/xen/include/asm-arm/gic.h
+++ b/xen/include/asm-arm/gic.h
@@ -146,7 +146,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 3/7] 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 37a73fb..cadc258 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 4/7] 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>
---
 xen/arch/arm/gic.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index cadc258..4f3a8a5 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -57,6 +57,27 @@ 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
+ */
+#define NR_GIC_CPU_IF 8
+static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff};
+
+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 |= gic_cpu_map[cpu];
+    }
+
+    return mask;
+}
+
 unsigned int gic_number_lines(void)
 {
     return gic.lines;
@@ -206,9 +227,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 +336,8 @@ static void __cpuinit gic_cpu_init(void)
 {
     int i;
 
+    gic_cpu_map[smp_processor_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. */
@@ -443,13 +464,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 5/7] 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 4f3a8a5..6dcefd7 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -481,7 +481,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 6/7] 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>
---
 xen/arch/arm/setup.c            |  109 ++++++++++++++++++++++++++++++++++++++-
 xen/arch/arm/smpboot.c          |    4 ++
 xen/common/device_tree.c        |   48 -----------------
 xen/include/asm-arm/processor.h |    4 ++
 4 files changed, 116 insertions(+), 49 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 137a65e..fabad91 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -496,6 +496,111 @@ 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;
+    u32 tmp_map[NR_CPUS] = { [0 ... NR_CPUS - 1] = MPIDR_INVALID };
+    bool_t bootcpu_valid = 0;
+
+    mpidr = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK;
+
+    if ( !cpus )
+    {
+        printk(XENLOG_WARNING "WARNING: Can't find /cpus in the device tree.\n"
+               "Using only 1 CPU\n");
+        return;
+    }
+
+    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,
@@ -515,7 +620,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());
@@ -528,6 +632,9 @@ void __init start_xen(unsigned long boot_phys_offset,
     dt_uart_init();
     console_init_preirq();
 
+    init_cpus_maps();
+    cpus = smp_get_max_cpus();
+
     system_state = SYS_STATE_boot;
 
     processor_id();
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 5620b23..cc519af 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 7/7] xen/arm: Use the hardware ID TMP 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.

Acked-by: Julien Grall <julien.grall@linaro.org>
---
 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 1/7] xen/arm: Introduce MPIDR_HWID_MASK

[Ian Campbell]

On Fri, 2013-08-30 at 14:30 +0100, Julien Grall wrote:
> This define will be use later to retrieve the correct hardware CPU ID.
> Also replace hardcoded mask in arm32/head.S by this define.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

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

> ---
>  xen/arch/arm/arm32/head.S       |    2 +-
>  xen/include/asm-arm/processor.h |    1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index b8334e2..79e95b6 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -98,7 +98,7 @@ past_zImage:
>          beq   boot_cpu
>          tst   r0, #(1<<30)           /* Uniprocessor system? */
>          bne   boot_cpu
> -        bics  r12, r0, #(0xff << 24) /* Mask out flags to get CPU ID */
> +        bics  r12, r0, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
>          beq   boot_cpu               /* If we're CPU 0, boot now */
>  
>          /* Non-boot CPUs wait here to be woken up one at a time. */
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 12795f3..b884354 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -12,6 +12,7 @@
>  #define MPIDR_SMP           (1 << 31)
>  #define MPIDR_AFF0_SHIFT    (0)
>  #define MPIDR_AFF0_MASK     (0xff << MPIDR_AFF0_SHIFT)
> +#define MPIDR_HWID_MASK     0xffffff
>  
>  /* TTBCR Translation Table Base Control Register */
>  #define TTBCR_EAE    0x80000000

Re: [PATCH 2/7] xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq

[Ian Campbell]

On Fri, 2013-08-30 at 14:30 +0100, Julien Grall wrote:
> The mapping between logical ID and GIC cpu ID can differs. Currently the code
> uses unsigned int for the cpu mask. Replace by cpumask_t to take advante of
> cpumask_* helpers.

"advantage"

In fact this replacement is all this patch does, there is no
introduction of a logical map here, that comes in patch #4?

The commit message made me thing otherwise. I think you could drop all
but the last sentence. The rest belongs in patch #4.

> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  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 a43ec23..37a73fb 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;
> @@ -508,7 +511,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();
>  }
> @@ -523,7 +526,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);
>      }
>  }
>  
> @@ -765,7 +768,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 8125b92..04ede1e 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -223,11 +223,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 90e887e..26b33bc 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -146,7 +146,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);

Re: [PATCH 3/7] xen/arm: Initialize correctly IRQ routing

[Ian Campbell]

On Fri, 2013-08-30 at 14:30 +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.

This trick is used a few times in this series, is it really the best way
to figure this out?

Not in some ID register or in DT?

>  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 37a73fb..cadc258 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;
>  

Re: [PATCH 4/7] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Fri, 2013-08-30 at 14:30 +0100, 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>
> ---
>  xen/arch/arm/gic.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index cadc258..4f3a8a5 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -57,6 +57,27 @@ 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
> + */
> +#define NR_GIC_CPU_IF 8
> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff};

Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size
of this array is the wrong way around?

Is what you want here is a per-cpu variable?

> +
> +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 |= gic_cpu_map[cpu];

Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE
system?

> +    }
> +
> +    return mask;
> +}
> +
>  unsigned int gic_number_lines(void)
>  {
>      return gic.lines;
> @@ -206,9 +227,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 +336,8 @@ static void __cpuinit gic_cpu_init(void)
>  {
>      int i;
>  
> +    gic_cpu_map[smp_processor_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. */
> @@ -443,13 +464,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();
>  

Re: [PATCH 5/7] xen/arm: Fix assert in send_SGI_one

[Ian Campbell]

On Fri, 2013-08-30 at 14:30 +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>
> ---
>  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 4f3a8a5..6dcefd7 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -481,7 +481,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 */

cpu here is a logical cpu id, not a gic id, I think?

In which case although 8 happens to be correct: NR_GIC_CPU_IF is the
wrong thing to be using and in any case this seems against the general
principals of this series.

>      send_SGI_mask(cpumask_of(cpu), sgi);
>  }
>  

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

[Ian Campbell]

t gOn Fri, 2013-08-30 at 14:30 +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>
> ---
>  xen/arch/arm/setup.c            |  109 ++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/smpboot.c          |    4 ++
>  xen/common/device_tree.c        |   48 -----------------
>  xen/include/asm-arm/processor.h |    4 ++
>  4 files changed, 116 insertions(+), 49 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 137a65e..fabad91 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -496,6 +496,111 @@ 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;
> +    u32 tmp_map[NR_CPUS] = { [0 ... NR_CPUS - 1] = MPIDR_INVALID };

This is potentially a fair bit of data on the stack? If yes then it
could be static init data?

> +    bool_t bootcpu_valid = 0;
> +
> +    mpidr = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK;

boot_cpu_mpidr would have saved me wondering why the current CPU was so
special.

> +
> +    if ( !cpus )
> +    {
> +        printk(XENLOG_WARNING "WARNING: Can't find /cpus in the device tree.\n"
> +               "Using only 1 CPU\n");
> +        return;
> +    }
> +
> +    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];

For i == 0 this happens in smp_clear_cpu_maps too. You may as well start
from 1.

> +    }
> +}
> +
>  /* C entry point for boot CPU */
>  void __init start_xen(unsigned long boot_phys_offset,
>                        unsigned long fdt_paddr,
> @@ -515,7 +620,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());
> @@ -528,6 +632,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>      dt_uart_init();
>      console_init_preirq();
>  
> +    init_cpus_maps();
> +    cpus = smp_get_max_cpus();

It seems like anything which was previously using this should by now be
using some sort of for_each_foo() helper over the apropriate bitmasks?

> +
>      system_state = SYS_STATE_boot;
>  
>      processor_id();
> 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 5620b23..cc519af 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 {

Re: [PATCH 7/7] xen/arm: Use the hardware ID TMP boot correctly secondary cpus

[Ian Campbell]

On Fri, 2013-08-30 at 14:30 +0100, Julien Grall wrote:

TMP in subject == "to"?

> 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.
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

S-o-b?

> ---
>  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);

We could stumble on without this cpu.

> +
>      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);

Re: [PATCH 1/7] xen/arm: Introduce MPIDR_HWID_MASK

[Ian Campbell]

On Mon, 2013-09-09 at 14:09 +0100, Ian Campbell wrote:
> On Fri, 2013-08-30 at 14:30 +0100, Julien Grall wrote:
> > This define will be use later to retrieve the correct hardware CPU ID.
> > Also replace hardcoded mask in arm32/head.S by this define.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

and applied.

Re: [PATCH 2/7] xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq

[Julien Grall]

On 09/09/2013 02:14 PM, Ian Campbell wrote:
> On Fri, 2013-08-30 at 14:30 +0100, Julien Grall wrote:
>> The mapping between logical ID and GIC cpu ID can differs. Currently the code
>> uses unsigned int for the cpu mask. Replace by cpumask_t to take advante of
>> cpumask_* helpers.
> 
> "advantage"
> 
> In fact this replacement is all this patch does, there is no
> introduction of a logical map here, that comes in patch #4?

Right

> The commit message made me thing otherwise. I think you could drop all
> but the last sentence. The rest belongs in patch #4.

I will rework the commit message.

>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>> ---
>>  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 a43ec23..37a73fb 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;
>> @@ -508,7 +511,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();
>>  }
>> @@ -523,7 +526,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);
>>      }
>>  }
>>  
>> @@ -765,7 +768,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 8125b92..04ede1e 100644
>> --- a/xen/arch/arm/time.c
>> +++ b/xen/arch/arm/time.c
>> @@ -223,11 +223,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 90e887e..26b33bc 100644
>> --- a/xen/include/asm-arm/gic.h
>> +++ b/xen/include/asm-arm/gic.h
>> @@ -146,7 +146,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);
> 
> 


-- 
Julien Grall

Re: [PATCH 3/7] xen/arm: Initialize correctly IRQ routing

[Julien Grall]

On 09/09/2013 02:17 PM, Ian Campbell wrote:
> On Fri, 2013-08-30 at 14:30 +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.
> 
> This trick is used a few times in this series, is it really the best way
> to figure this out?
> Not in some ID register or in DT?

Unfortunately not. On the patch #4, I introduce an array to store the
mapping between logical ID and gic ID.
I didn't use this trick here, because the distributor is initialized
before each cpu interface.

>>  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 37a73fb..cadc258 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;
>>  
> 
> 


-- 
Julien Grall

Re: [PATCH 3/7] xen/arm: Initialize correctly IRQ routing

[Julien Grall]

On 09/09/2013 02:17 PM, Ian Campbell wrote:
> On Fri, 2013-08-30 at 14:30 +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.
> 
> This trick is used a few times in this series, is it really the best way
> to figure this out?

I forgot to answer to this question. When I wrote this code, I wasn't
sure if it's the best way. Linux does the same and the gic documentation
doesn't offer a better solution.

I saw some device tree with GIC cpu interface node but it's not upstream
and disappear from the latest Linaro kernel.

-- 
Julien Grall

Re: [PATCH 3/7] xen/arm: Initialize correctly IRQ routing

[Ian Campbell]

On Tue, 2013-09-10 at 16:29 +0100, Julien Grall wrote:
> On 09/09/2013 02:17 PM, Ian Campbell wrote:
> > On Fri, 2013-08-30 at 14:30 +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.
> > 
> > This trick is used a few times in this series, is it really the best way
> > to figure this out?
> 
> I forgot to answer to this question. When I wrote this code, I wasn't
> sure if it's the best way. Linux does the same and the gic documentation
> doesn't offer a better solution.

OK. Lets chalk it up to hardware skankiness!

> I saw some device tree with GIC cpu interface node but it's not upstream
> and disappear from the latest Linaro kernel.

I suppose it was deemed unnecessary given that the above works.

Re: [PATCH 4/7] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

On 09/09/2013 02:28 PM, Ian Campbell wrote:
> On Fri, 2013-08-30 at 14:30 +0100, 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>
>> ---
>>  xen/arch/arm/gic.c |   35 ++++++++++++++++++++++++++++-------
>>  1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index cadc258..4f3a8a5 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -57,6 +57,27 @@ 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
>> + */
>> +#define NR_GIC_CPU_IF 8
>> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff};
> 
> Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size
> of this array is the wrong way around?

This array maps a logical cpu id (the index) to a gic cpus id. The size
if correct, because Xen will allocate logical cpu id from 0 to 7.

> Is what you want here is a per-cpu variable?

Does per-cpu variable allow any cpu to retrieve data of another cpu? I
didn't find a such function.

>> +
>> +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 |= gic_cpu_map[cpu];
> 
> Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE
> system?

0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0.

>> +    }
>> +
>> +    return mask;
>> +}
>> +
>>  unsigned int gic_number_lines(void)
>>  {
>>      return gic.lines;
>> @@ -206,9 +227,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 +336,8 @@ static void __cpuinit gic_cpu_init(void)
>>  {
>>      int i;
>>  
>> +    gic_cpu_map[smp_processor_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. */
>> @@ -443,13 +464,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 5/7] xen/arm: Fix assert in send_SGI_one

[Julien Grall]

On 09/09/2013 02:30 PM, Ian Campbell wrote:
> On Fri, 2013-08-30 at 14:30 +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>
>> ---
>>  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 4f3a8a5..6dcefd7 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -481,7 +481,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 */
> 
> cpu here is a logical cpu id, not a gic id, I think?

Right.

> In which case although 8 happens to be correct: NR_GIC_CPU_IF is the
> wrong thing to be using and in any case this seems against the general
> principals of this series.

NR_GIC_CPU_IF indicates the number of cpu interface handled by the GIC.
Xen won't be able to register more than 8 CPUs, ie logical cpu id 0..7.

>>      send_SGI_mask(cpumask_of(cpu), sgi);
>>  }
>>  
> 
> 

-- 
Julien Grall

Re: [PATCH 4/7] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Tue, 2013-09-10 at 16:42 +0100, Julien Grall wrote:
> On 09/09/2013 02:28 PM, Ian Campbell wrote:
> > On Fri, 2013-08-30 at 14:30 +0100, 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>
> >> ---
> >>  xen/arch/arm/gic.c |   35 ++++++++++++++++++++++++++++-------
> >>  1 file changed, 28 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index cadc258..4f3a8a5 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -57,6 +57,27 @@ 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
> >> + */
> >> +#define NR_GIC_CPU_IF 8
> >> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff};
> > 
> > Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size
> > of this array is the wrong way around?
> 
> This array maps a logical cpu id (the index) to a gic cpus id. The size
> if correct, because Xen will allocate logical cpu id from 0 to 7.

Oh right, yes.

I think NR_CPUS (which should be 8 given the other limitations) is the
right size for this array.

> 
> > Is what you want here is a per-cpu variable?
> 
> Does per-cpu variable allow any cpu to retrieve data of another cpu? I
> didn't find a such function.

per_cpu(variable, cpu) will do it.

> 
> >> +
> >> +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 |= gic_cpu_map[cpu];
> > 
> > Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE
> > system?
> 
> 0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0.

Right.

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

[Julien Grall]

On 09/09/2013 02:38 PM, Ian Campbell wrote:
> t gOn Fri, 2013-08-30 at 14:30 +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>
>> ---
>>  xen/arch/arm/setup.c            |  109 ++++++++++++++++++++++++++++++++++++++-
>>  xen/arch/arm/smpboot.c          |    4 ++
>>  xen/common/device_tree.c        |   48 -----------------
>>  xen/include/asm-arm/processor.h |    4 ++
>>  4 files changed, 116 insertions(+), 49 deletions(-)
>>
>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
>> index 137a65e..fabad91 100644
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -496,6 +496,111 @@ 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;
>> +    u32 tmp_map[NR_CPUS] = { [0 ... NR_CPUS - 1] = MPIDR_INVALID };
> 
> This is potentially a fair bit of data on the stack? If yes then it
> could be static init data?

Rigth. I will move to init data.

> 
>> +    bool_t bootcpu_valid = 0;
>> +
>> +    mpidr = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK;
> 
> boot_cpu_mpidr would have saved me wondering why the current CPU was so
> special.

boot_cpu_mpidr is filled a bit after. I will move init_cpus_map later.

>> +
>> +    if ( !cpus )
>> +    {
>> +        printk(XENLOG_WARNING "WARNING: Can't find /cpus in the device tree.\n"
>> +               "Using only 1 CPU\n");
>> +        return;
>> +    }
>> +
>> +    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];
> 
> For i == 0 this happens in smp_clear_cpu_maps too. You may as well start
> from 1.

Ok.

>> +    }
>> +}
>> +
>>  /* C entry point for boot CPU */
>>  void __init start_xen(unsigned long boot_phys_offset,
>>                        unsigned long fdt_paddr,
>> @@ -515,7 +620,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());
>> @@ -528,6 +632,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>>      dt_uart_init();
>>      console_init_preirq();
>>  
>> +    init_cpus_maps();
>> +    cpus = smp_get_max_cpus();
> 
> It seems like anything which was previously using this should by now be
> using some sort of for_each_foo() helper over the apropriate bitmasks?

Yes. I will send a patch for that.

>> +
>>      system_state = SYS_STATE_boot;
>>  
>>      processor_id();
>> 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 5620b23..cc519af 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 {
> 
> 


-- 
Julien Grall

Re: [PATCH 7/7] xen/arm: Use the hardware ID TMP boot correctly secondary cpus

[Julien Grall]

On 09/09/2013 02:40 PM, Ian Campbell wrote:
> On Fri, 2013-08-30 at 14:30 +0100, Julien Grall wrote:
> 
> TMP in subject == "to"?

Yes. I didn't pay attention when I sent this patch. I will fix it.

> 
>> 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.
>>
>> Acked-by: Julien Grall <julien.grall@linaro.org>
> 
> S-o-b?

Same here :/

> 
>> ---
>>  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);
> 
> We could stumble on without this cpu.

No. The boot cpu will wait forever the secondary cpu because we don't
have timeout in __cpu_up.

> 
>> +
>>      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);
> 
> 


-- 
Julien Grall

Re: [PATCH 4/7] xen/arm: gic: Use the correct CPU ID

[Julien Grall]

On 09/10/2013 04:52 PM, Ian Campbell wrote:
> On Tue, 2013-09-10 at 16:42 +0100, Julien Grall wrote:
>> On 09/09/2013 02:28 PM, Ian Campbell wrote:
>>> On Fri, 2013-08-30 at 14:30 +0100, 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>
>>>> ---
>>>>  xen/arch/arm/gic.c |   35 ++++++++++++++++++++++++++++-------
>>>>  1 file changed, 28 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index cadc258..4f3a8a5 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -57,6 +57,27 @@ 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
>>>> + */
>>>> +#define NR_GIC_CPU_IF 8
>>>> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff};
>>>
>>> Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size
>>> of this array is the wrong way around?
>>
>> This array maps a logical cpu id (the index) to a gic cpus id. The size
>> if correct, because Xen will allocate logical cpu id from 0 to 7.
> 
> Oh right, yes.
> 
> I think NR_CPUS (which should be 8 given the other limitations) is the
> right size for this array.
> 
>>
>>> Is what you want here is a per-cpu variable?
>>
>> Does per-cpu variable allow any cpu to retrieve data of another cpu? I
>> didn't find a such function.
> 
> per_cpu(variable, cpu) will do it.

Is it fast ? I think this solution is slower, because the mapping is
used often used, mainly when an SGI is sent to another cpu.

>>
>>>> +
>>>> +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 |= gic_cpu_map[cpu];
>>>
>>> Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE
>>> system?
>>
>> 0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0.
> 
> Right.
> 
> 


-- 
Julien Grall

Re: [PATCH 4/7] xen/arm: gic: Use the correct CPU ID

[Ian Campbell]

On Tue, 2013-09-10 at 17:45 +0100, Julien Grall wrote:
> On 09/10/2013 04:52 PM, Ian Campbell wrote:
> > On Tue, 2013-09-10 at 16:42 +0100, Julien Grall wrote:
> >> On 09/09/2013 02:28 PM, Ian Campbell wrote:
> >>> On Fri, 2013-08-30 at 14:30 +0100, 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>
> >>>> ---
> >>>>  xen/arch/arm/gic.c |   35 ++++++++++++++++++++++++++++-------
> >>>>  1 file changed, 28 insertions(+), 7 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >>>> index cadc258..4f3a8a5 100644
> >>>> --- a/xen/arch/arm/gic.c
> >>>> +++ b/xen/arch/arm/gic.c
> >>>> @@ -57,6 +57,27 @@ 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
> >>>> + */
> >>>> +#define NR_GIC_CPU_IF 8
> >>>> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff};
> >>>
> >>> Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size
> >>> of this array is the wrong way around?
> >>
> >> This array maps a logical cpu id (the index) to a gic cpus id. The size
> >> if correct, because Xen will allocate logical cpu id from 0 to 7.
> > 
> > Oh right, yes.
> > 
> > I think NR_CPUS (which should be 8 given the other limitations) is the
> > right size for this array.
> > 
> >>
> >>> Is what you want here is a per-cpu variable?
> >>
> >> Does per-cpu variable allow any cpu to retrieve data of another cpu? I
> >> didn't find a such function.
> > 
> > per_cpu(variable, cpu) will do it.
> 
> Is it fast ?

It looks up the cpus offset in an array and then adds it to the address
of variable before loading the result.

>  I think this solution is slower, because the mapping is
> used often used, mainly when an SGI is sent to another cpu.

I doubt it is measurable. 

Ian

> 
> >>
> >>>> +
> >>>> +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 |= gic_cpu_map[cpu];
> >>>
> >>> Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE
> >>> system?
> >>
> >> 0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0.
> > 
> > Right.
> > 
> > 
> 
>