Julien Grall
2013-Sep-18 13:15 UTC
[PATCH v3 0/6] Dissociate logical and gic/hardware CPU ID
Hi, This is the third version for this patch series. All changes can be found in each patch. With the Versatile Express TC2, it''s possible to boot only with A7 or A15. If the user choose to boot with only A7, the CPU ID will start at 0x100. As Xen relies on it to set the logical ID and the GIC, it won''t be possible to use Xen with this use case. This patch series is divided in 3 parts: - Patch 1: prepare Xen - Patch 2-4: dissociate logical and gic CPU ID - Patch 5-6: dissociate logical and hardware CPU ID For the moment this patch series only modifies Xen and not the boot process (ie head.S). So if the boot CPU ID is not equal to 0 you won''t be able to start Xen. The future Ian Campbel''s patch series should resolve this issue. The serie also depends on my patch series "Allow Xen to boot with a raw Device tree". Cheers, Julien Grall (6): xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq xen/arm: Initialize correctly IRQ routing xen/arm: gic: Use the correct CPU ID xen/arm: Fix assert in send_SGI_one xen/arm: Dissociate logical and hardware CPU ID xen/arm: Use the hardware ID to boot correctly secondary cpus xen/arch/arm/gic.c | 56 +++++++++++++++----- xen/arch/arm/setup.c | 112 ++++++++++++++++++++++++++++++++++++++- xen/arch/arm/smpboot.c | 24 +++++++-- xen/arch/arm/time.c | 6 +-- xen/common/device_tree.c | 48 ----------------- xen/include/asm-arm/gic.h | 3 +- xen/include/asm-arm/processor.h | 4 ++ 7 files changed, 181 insertions(+), 72 deletions(-) -- 1.7.10.4
Julien Grall
2013-Sep-18 13:15 UTC
[PATCH v3 1/6] xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq
Replace by cpumask_t to take advantage of cpumask_* helpers. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Rework commit message --- xen/arch/arm/gic.c | 20 ++++++++++++-------- xen/arch/arm/time.c | 6 +++--- xen/include/asm-arm/gic.h | 3 ++- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index aff57b9..091eb36 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -184,10 +184,14 @@ static hw_irq_controller gic_guest_irq_type = { /* needs to be called with gic.lock held */ static void gic_set_irq_properties(unsigned int irq, bool_t level, - unsigned int cpu_mask, unsigned int priority) + const cpumask_t *cpu_mask, + unsigned int priority) { volatile unsigned char *bytereg; uint32_t cfg, edgebit; + unsigned int mask = cpumask_bits(cpu_mask)[0]; + + ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */ /* Set edge / level */ cfg = GICD[GICD_ICFGR + irq / 16]; @@ -200,7 +204,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, /* Set target CPU mask (RAZ/WI on uniprocessor) */ bytereg = (unsigned char *) (GICD + GICD_ITARGETSR); - bytereg[irq] = cpu_mask; + bytereg[irq] = mask; /* Set priority */ bytereg = (unsigned char *) (GICD + GICD_IPRIORITYR); @@ -210,12 +214,11 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, /* Program the GIC to route an interrupt */ static int gic_route_irq(unsigned int irq, bool_t level, - unsigned int cpu_mask, unsigned int priority) + const cpumask_t *cpu_mask, unsigned int priority) { struct irq_desc *desc = irq_to_desc(irq); unsigned long flags; - ASSERT(!(cpu_mask & ~0xff)); /* Targets bitmap only supports 8 CPUs */ ASSERT(priority <= 0xff); /* Only 8 bits of priority */ ASSERT(irq < gic.lines); /* Can''t route interrupts that don''t exist */ @@ -242,7 +245,7 @@ static int gic_route_irq(unsigned int irq, bool_t level, } /* Program the GIC to route an interrupt with a dt_irq */ -void gic_route_dt_irq(const struct dt_irq *irq, unsigned int cpu_mask, +void gic_route_dt_irq(const struct dt_irq *irq, const cpumask_t *cpu_mask, unsigned int priority) { bool_t level; @@ -496,7 +499,7 @@ void gic_disable_cpu(void) void gic_route_ppis(void) { /* GIC maintenance */ - gic_route_dt_irq(&gic.maintenance, 1u << smp_processor_id(), 0xa0); + gic_route_dt_irq(&gic.maintenance, cpumask_of(smp_processor_id()), 0xa0); /* Route timer interrupt */ route_timer_interrupt(); } @@ -511,7 +514,7 @@ void gic_route_spis(void) if ( (irq = serial_dt_irq(seridx)) == NULL ) continue; - gic_route_dt_irq(irq, 1u << smp_processor_id(), 0xa0); + gic_route_dt_irq(irq, cpumask_of(smp_processor_id()), 0xa0); } } @@ -718,7 +721,8 @@ int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, level = dt_irq_is_level_triggered(irq); - gic_set_irq_properties(irq->irq, level, 1u << smp_processor_id(), 0xa0); + gic_set_irq_properties(irq->irq, level, cpumask_of(smp_processor_id()), + 0xa0); retval = __setup_irq(desc, irq->irq, action); if (retval) { diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index eb3ad5c..a30d422 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -222,11 +222,11 @@ static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) void __cpuinit route_timer_interrupt(void) { gic_route_dt_irq(&timer_irq[TIMER_PHYS_NONSECURE_PPI], - 1u << smp_processor_id(), 0xa0); + cpumask_of(smp_processor_id()), 0xa0); gic_route_dt_irq(&timer_irq[TIMER_HYP_PPI], - 1u << smp_processor_id(), 0xa0); + cpumask_of(smp_processor_id()), 0xa0); gic_route_dt_irq(&timer_irq[TIMER_VIRT_PPI], - 1u << smp_processor_id(), 0xa0); + cpumask_of(smp_processor_id()), 0xa0); } /* Set up the timer interrupt on this CPU */ diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 92a3349..3a5ef6f 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -147,7 +147,8 @@ extern void vgic_clear_pending_irqs(struct vcpu *v); extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq); /* Program the GIC to route an interrupt with a dt_irq */ -extern void gic_route_dt_irq(const struct dt_irq *irq, unsigned int cpu_mask, +extern void gic_route_dt_irq(const struct dt_irq *irq, + const cpumask_t *cpu_mask, unsigned int priority); extern void gic_route_ppis(void); extern void gic_route_spis(void); -- 1.7.10.4
Julien Grall
2013-Sep-18 13:15 UTC
[PATCH v3 2/6] xen/arm: Initialize correctly IRQ routing
When Xen initialize the GIC distributor, we need to route all the IRQs to the boot CPU. The CPU ID can differ between Xen and the GIC. When ITARGETSR0 is read, each field will return a value that corresponds only to the processor reading the register. So Xen can use the PPI 0 to initialize correctly the routing. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/gic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 091eb36..b969d23 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -258,9 +258,10 @@ void gic_route_dt_irq(const struct dt_irq *irq, const cpumask_t *cpu_mask, static void __init gic_dist_init(void) { uint32_t type; - uint32_t cpumask = 1 << smp_processor_id(); + uint32_t cpumask; int i; + cpumask = GICD[GICD_ITARGETSR] & 0xff; cpumask |= cpumask << 8; cpumask |= cpumask << 16; -- 1.7.10.4
The GIC mapping of CPU interfaces does not necessarily match the logical CPU numbering. When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID. It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC will return a value that corresponds only to the processor reading the register. So Xen can use the PPI 0 to initialize the mapping. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v3: - Correctly create the mask in gic_cpu_mask Changes in v2: - Use per-cpu variable instead of an array - Add comment for NR_GIC_CPU_IF --- xen/arch/arm/gic.c | 37 ++++++++++++++++++++++++++++++------- 1 file changed, 30 insertions(+), 7 deletions(-) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index b969d23..4061691 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); static unsigned nr_lrs; +/* The GIC mapping of CPU interfaces does not necessarily match the + * logical CPU numbering. Let''s use mapping as returned by the GIC + * itself + */ +static DEFINE_PER_CPU(u8, gic_cpu_id); + +/* Maximum cpu interface per GIC */ +#define NR_GIC_CPU_IF 8 + +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) +{ + unsigned int cpu; + unsigned int mask = 0; + + for_each_cpu(cpu, cpumask) + { + ASSERT(cpu < NR_GIC_CPU_IF); + mask |= per_cpu(gic_cpu_id, cpu); + } + + return mask; +} + unsigned int gic_number_lines(void) { return gic.lines; @@ -189,9 +212,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, { volatile unsigned char *bytereg; uint32_t cfg, edgebit; - unsigned int mask = cpumask_bits(cpu_mask)[0]; - - ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */ + unsigned int mask = gic_cpu_mask(cpu_mask); /* Set edge / level */ cfg = GICD[GICD_ICFGR + irq / 16]; @@ -300,6 +321,8 @@ static void __cpuinit gic_cpu_init(void) { int i; + this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff; + /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so * even though they are controlled with GICD registers, they must * be set up here with the other per-cpu state. */ @@ -431,13 +454,13 @@ void __init gic_init(void) void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) { - unsigned long mask = cpumask_bits(cpumask)[0]; + cpumask_t online_mask; + unsigned int mask = 0; ASSERT(sgi < 16); /* There are only 16 SGIs */ - mask &= cpumask_bits(&cpu_online_map)[0]; - - ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */ + cpumask_and(&online_mask, cpumask, &cpu_online_map); + mask = gic_cpu_mask(&online_mask); dsb(); -- 1.7.10.4
The GIC can handle maximum 8 cpus (0...7). The CPU id 7 is still valid. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/gic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index 4061691..9b7457a 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -471,7 +471,7 @@ void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) void send_SGI_one(unsigned int cpu, enum gic_sgi sgi) { - ASSERT(cpu < 7); /* Targets bitmap only supports 8 CPUs */ + ASSERT(cpu < NR_GIC_CPU_IF); /* Targets bitmap only supports 8 CPUs */ send_SGI_mask(cpumask_of(cpu), sgi); } -- 1.7.10.4
Julien Grall
2013-Sep-18 13:15 UTC
[PATCH v3 5/6] xen/arm: Dissociate logical and hardware CPU ID
Introduce cpu_logical_map to associate a logical CPU ID to an hardware CPU ID. This map will be filled during Xen boot via the device tree. Each CPU node contains a "reg" property which contains the hardware ID (ie MPIDR[0:23]). Also move /cpus parsing later so we can use the dt_* API. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v3: - re-add the mask (lost v2) for the mpidr. Changes in v2: - for_each_child was renamed to dt_for_each_child - move init_cpus_maps later to take advantage of boot_cpu_data - move to initdata tmp_map --- xen/arch/arm/setup.c | 112 ++++++++++++++++++++++++++++++++++++++- xen/arch/arm/smpboot.c | 4 ++ xen/common/device_tree.c | 48 ----------------- xen/include/asm-arm/processor.h | 4 ++ 4 files changed, 119 insertions(+), 49 deletions(-) diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index aa87fb1..7dd6c9f 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -498,6 +498,114 @@ void __init setup_cache(void) cacheline_bytes = 1U << (4 + (ccsid & 0x7)); } +/* Parse the device tree and build the logical map array containing + * MPIDR values related to logical cpus + * Code base on Linux arch/arm/kernel/devtree.c + */ +static void __init init_cpus_maps(void) +{ + register_t mpidr; + struct dt_device_node *cpus = dt_find_node_by_path("/cpus"); + struct dt_device_node *cpu; + unsigned int i, j; + unsigned int cpuidx = 1; + static u32 tmp_map[NR_CPUS] __initdata + { + [0 ... NR_CPUS - 1] = MPIDR_INVALID + }; + bool_t bootcpu_valid = 0; + + mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK; + + if ( !cpus ) + { + printk(XENLOG_WARNING "WARNING: Can''t find /cpus in the device tree.\n" + "Using only 1 CPU\n"); + return; + } + + dt_for_each_child_node( cpus, cpu ) + { + u32 hwid; + + if ( !dt_device_type_is_equal(cpu, "cpu") ) + continue; + + if ( !dt_property_read_u32(cpu, "reg", &hwid) ) + { + printk(XENLOG_WARNING "cpu node `%s`: missing reg property\n", + dt_node_full_name(cpu)); + continue; + } + + /* + * 8 MSBs must be set to 0 in the DT since the reg property + * defines the MPIDR[23:0] + */ + if ( hwid & ~MPIDR_HWID_MASK ) + { + printk(XENLOG_WARNING "cpu node `%s`: invalid hwid value (0x%x)\n", + dt_node_full_name(cpu), hwid); + continue; + } + + /* + * Duplicate MPIDRs are a recipe for disaster. Scan all initialized + * entries and check for duplicates. If any found just skip the node. + * temp values values are initialized to MPIDR_INVALID to avoid + * matching valid MPIDR[23:0] values. + */ + for ( j = 0; j < cpuidx; j++ ) + { + if ( tmp_map[j] == hwid ) + { + printk(XENLOG_WARNING "cpu node `%s`: duplicate /cpu reg properties in the DT\n", + dt_node_full_name(cpu)); + continue; + } + } + + /* + * Build a stashed array of MPIDR values. Numbering scheme requires + * that if detected the boot CPU must be assigned logical id 0. Other + * CPUs get sequential indexes starting from 1. If a CPU node + * with a reg property matching the boot CPU MPIDR is detected, + * this is recorded and so that the logical map build from DT is + * validated and can be used to set the map. + */ + if ( hwid == mpidr ) + { + i = 0; + bootcpu_valid = 1; + } + else + i = cpuidx++; + + if ( cpuidx > NR_CPUS ) + { + printk(XENLOG_WARNING "DT /cpu %u node greater than max cores %u, capping them\n", + cpuidx, NR_CPUS); + cpuidx = NR_CPUS; + break; + } + + tmp_map[i] = hwid; + } + + if ( !bootcpu_valid ) + { + printk(XENLOG_WARNING "DT missing boot CPU MPIDR[23:0]\n" + "Using only 1 CPU\n"); + return; + } + + for ( i = 0; i < cpuidx; i++ ) + { + cpumask_set_cpu(i, &cpu_possible_map); + cpu_logical_map(i) = tmp_map[i]; + } +} + /* C entry point for boot CPU */ void __init start_xen(unsigned long boot_phys_offset, unsigned long fdt_paddr, @@ -517,7 +625,6 @@ void __init start_xen(unsigned long boot_phys_offset, + (fdt_paddr & ((1 << SECOND_SHIFT) - 1)); fdt_size = device_tree_early_init(device_tree_flattened); - cpus = smp_get_max_cpus(); cmdline_parse(device_tree_bootargs(device_tree_flattened)); setup_pagetables(boot_phys_offset, get_xen_paddr()); @@ -534,6 +641,9 @@ void __init start_xen(unsigned long boot_phys_offset, processor_id(); + init_cpus_maps(); + cpus = smp_get_max_cpus(); + platform_init(); init_xen_time(); diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index b6aea63..c0d25de 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -39,6 +39,9 @@ EXPORT_SYMBOL(cpu_possible_map); struct cpuinfo_arm cpu_data[NR_CPUS]; +/* CPU logical map: map xen cpuid to an MPIDR */ +u32 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID }; + /* Fake one node for now. See also include/asm-arm/numa.h */ nodemask_t __read_mostly node_online_map = { { [0] = 1UL } }; @@ -82,6 +85,7 @@ smp_clear_cpu_maps (void) cpumask_clear(&cpu_online_map); cpumask_set_cpu(0, &cpu_online_map); cpumask_set_cpu(0, &cpu_possible_map); + cpu_logical_map(0) = READ_SYSREG(MPIDR_EL1) & MPIDR_HWID_MASK; } int __init diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c index 0ece249..9a16650 100644 --- a/xen/common/device_tree.c +++ b/xen/common/device_tree.c @@ -118,18 +118,6 @@ static bool_t __init device_tree_node_matches(const void *fdt, int node, && (name[match_len] == ''@'' || name[match_len] == ''\0''); } -static bool_t __init device_tree_type_matches(const void *fdt, int node, - const char *match) -{ - const void *prop; - - prop = fdt_getprop(fdt, node, "device_type", NULL); - if ( prop == NULL ) - return 0; - - return !dt_node_cmp(prop, match); -} - static bool_t __init device_tree_node_compatible(const void *fdt, int node, const char *match) { @@ -348,40 +336,6 @@ static void __init process_memory_node(const void *fdt, int node, } } -static void __init process_cpu_node(const void *fdt, int node, - const char *name, - u32 address_cells, u32 size_cells) -{ - const struct fdt_property *prop; - u32 cpuid; - int len; - - prop = fdt_get_property(fdt, node, "reg", &len); - if ( !prop ) - { - early_printk("fdt: node `%s'': missing `reg'' property\n", name); - return; - } - - if ( len < sizeof (cpuid) ) - { - dt_printk("fdt: node `%s'': `reg` property length is too short\n", - name); - return; - } - - cpuid = dt_read_number((const __be32 *)prop->data, 1); - - /* TODO: handle non-contiguous CPU ID */ - if ( cpuid >= NR_CPUS ) - { - dt_printk("fdt: node `%s'': reg(0x%x) >= NR_CPUS(%d)\n", - name, cpuid, NR_CPUS); - return; - } - cpumask_set_cpu(cpuid, &cpu_possible_map); -} - static void __init process_multiboot_node(const void *fdt, int node, const char *name, u32 address_cells, u32 size_cells) @@ -435,8 +389,6 @@ static int __init early_scan_node(const void *fdt, { if ( device_tree_node_matches(fdt, node, "memory") ) process_memory_node(fdt, node, name, address_cells, size_cells); - else if ( device_tree_type_matches(fdt, node, "cpu") ) - process_cpu_node(fdt, node, name, address_cells, size_cells); else if ( device_tree_node_compatible(fdt, node, "xen,multiboot-module" ) ) process_multiboot_node(fdt, node, name, address_cells, size_cells); diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index 808567e..0646422 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -13,6 +13,7 @@ #define MPIDR_AFF0_SHIFT (0) #define MPIDR_AFF0_MASK (0xff << MPIDR_AFF0_SHIFT) #define MPIDR_HWID_MASK 0xffffff +#define MPIDR_INVALID (~MPIDR_HWID_MASK) /* TTBCR Translation Table Base Control Register */ #define TTBCR_EAE 0x80000000 @@ -230,6 +231,9 @@ extern void identify_cpu(struct cpuinfo_arm *); extern struct cpuinfo_arm cpu_data[]; #define current_cpu_data cpu_data[smp_processor_id()] +extern u32 __cpu_logical_map[]; +#define cpu_logical_map(cpu) __cpu_logical_map[cpu] + union hsr { uint32_t bits; struct { -- 1.7.10.4
Julien Grall
2013-Sep-18 13:15 UTC
[PATCH v3 6/6] xen/arm: Use the hardware ID to boot correctly secondary cpus
Secondary CPUs will spin in head.S until their MPIDR[23:0] correspond to the smp_up_cpu. Actually Xen will set the value with the logical CPU ID which is wrong. Use the cpu_logical_map to get the correct CPU ID. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Replace Acked-by by a s-o-b - s/TMP/to in commit title --- xen/arch/arm/smpboot.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c index c0d25de..1952287 100644 --- a/xen/arch/arm/smpboot.c +++ b/xen/arch/arm/smpboot.c @@ -124,8 +124,7 @@ make_cpus_ready(unsigned int max_cpus, unsigned long boot_phys_offset) for ( i = 1; i < max_cpus; i++ ) { /* Tell the next CPU to get ready */ - /* TODO: handle boards where CPUIDs are not contiguous */ - *gate = i; + *gate = cpu_logical_map(i); flush_xen_dcache(*gate); isb(); sev(); @@ -139,11 +138,22 @@ make_cpus_ready(unsigned int max_cpus, unsigned long boot_phys_offset) /* Boot the current CPU */ void __cpuinit start_secondary(unsigned long boot_phys_offset, unsigned long fdt_paddr, - unsigned long cpuid) + unsigned long hwid) { + unsigned int cpuid; + memset(get_cpu_info(), 0, sizeof (struct cpu_info)); - /* TODO: handle boards where CPUIDs are not contiguous */ + /* Browse the logical map and find the associate logical cpu ID */ + for ( cpuid = 1; cpuid < nr_cpu_ids; cpuid++ ) + { + if ( cpu_logical_map(cpuid) == hwid ) + break; + } + + if ( cpuid == nr_cpu_ids ) + panic("Can''t find logical CPU id for cpu MPIDR[23:0] = 0x%lx\n", hwid); + set_processor_id(cpuid); current_cpu_data = boot_cpu_data; @@ -232,7 +242,7 @@ int __cpu_up(unsigned int cpu) /* Unblock the CPU. It should be waiting in the loop in head.S * for an event to arrive when smp_up_cpu matches its cpuid. */ - smp_up_cpu = cpu; + smp_up_cpu = cpu_logical_map(cpu); /* we need to make sure that the change to smp_up_cpu is visible to * secondary cpus with D-cache off */ flush_xen_dcache(smp_up_cpu); -- 1.7.10.4
Julien Grall
2013-Sep-20 12:44 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On 09/18/2013 02:15 PM, Julien Grall wrote:> The GIC mapping of CPU interfaces does not necessarily match the logical > CPU numbering. > > When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID. > It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC > will return a value that corresponds only to the processor reading the register. > So Xen can use the PPI 0 to initialize the mapping. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v3: > - Correctly create the mask in gic_cpu_mask > > Changes in v2: > - Use per-cpu variable instead of an array > - Add comment for NR_GIC_CPU_IF > --- > xen/arch/arm/gic.c | 37 ++++++++++++++++++++++++++++++------- > 1 file changed, 30 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > index b969d23..4061691 100644 > --- a/xen/arch/arm/gic.c > +++ b/xen/arch/arm/gic.c > @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); > > static unsigned nr_lrs; > > +/* The GIC mapping of CPU interfaces does not necessarily match the > + * logical CPU numbering. Let''s use mapping as returned by the GIC > + * itself > + */ > +static DEFINE_PER_CPU(u8, gic_cpu_id); > +Following the discussion on another thread (http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we need to initialize each CPU ID to 0xff by default, to be able to bring up secondary CPUs with an SGI (send_SGI_mask). But as I understand, per_cpu area for a give area is initialized in cpu_up and there is no way to give a default value. So user could call send_SGI_mask to early function and use an invalid CPU area. IMHO, this solution is too fragile and I would like to use the same solution as my first version (ie with an array of 8 cells). Ian, is ok for you?> +/* Maximum cpu interface per GIC */ > +#define NR_GIC_CPU_IF 8 > + > +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > +{ > + unsigned int cpu; > + unsigned int mask = 0; > + > + for_each_cpu(cpu, cpumask) > + { > + ASSERT(cpu < NR_GIC_CPU_IF); > + mask |= per_cpu(gic_cpu_id, cpu); > + } > + > + return mask; > +} > + > unsigned int gic_number_lines(void) > { > return gic.lines; > @@ -189,9 +212,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level, > { > volatile unsigned char *bytereg; > uint32_t cfg, edgebit; > - unsigned int mask = cpumask_bits(cpu_mask)[0]; > - > - ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */ > + unsigned int mask = gic_cpu_mask(cpu_mask); > > /* Set edge / level */ > cfg = GICD[GICD_ICFGR + irq / 16]; > @@ -300,6 +321,8 @@ static void __cpuinit gic_cpu_init(void) > { > int i; > > + this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff; > + > /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so > * even though they are controlled with GICD registers, they must > * be set up here with the other per-cpu state. */ > @@ -431,13 +454,13 @@ void __init gic_init(void) > > void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) > { > - unsigned long mask = cpumask_bits(cpumask)[0]; > + cpumask_t online_mask; > + unsigned int mask = 0; > > ASSERT(sgi < 16); /* There are only 16 SGIs */ > > - mask &= cpumask_bits(&cpu_online_map)[0]; > - > - ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */ > + cpumask_and(&online_mask, cpumask, &cpu_online_map); > + mask = gic_cpu_mask(&online_mask); > > dsb(); > >-- Julien Grall
Ian Campbell
2013-Sep-20 13:36 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On Fri, 2013-09-20 at 13:44 +0100, Julien Grall wrote:> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c > > index b969d23..4061691 100644 > > --- a/xen/arch/arm/gic.c > > +++ b/xen/arch/arm/gic.c > > @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); > > > > static unsigned nr_lrs; > > > > +/* The GIC mapping of CPU interfaces does not necessarily match the > > + * logical CPU numbering. Let''s use mapping as returned by the GIC > > + * itself > > + */ > > +static DEFINE_PER_CPU(u8, gic_cpu_id); > > + > > Following the discussion on another thread > (http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we > need to initialize each CPU ID to 0xff by default, to be able to bring > up secondary CPUs with an SGI (send_SGI_mask).We may as well use the send_SGI_allbutself mechanism, which doesn''t rely on knowing and target ids.> But as I understand, per_cpu area for a give area is initialized in > cpu_up and there is no way to give a default value. So user could call > send_SGI_mask to early function and use an invalid CPU area.If we need to go down this path then I would write something like cpu_online(cpu) ? this_cpu(thing, cpu) : 0xff but I don''t think we need to because we can easily avoid the problem.> IMHO, this solution is too fragile and I would like to use the same > solution as my first version (ie with an array of 8 cells).I think cpu bring up should use allbutself and the other callers can reasonable be restricted to only operate on cpus which are up and therefore have a valid mask already allocated and initialised. Ian.
Julien Grall
2013-Sep-20 13:49 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On 09/20/2013 02:36 PM, Ian Campbell wrote:> On Fri, 2013-09-20 at 13:44 +0100, Julien Grall wrote: >>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c >>> index b969d23..4061691 100644 >>> --- a/xen/arch/arm/gic.c >>> +++ b/xen/arch/arm/gic.c >>> @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); >>> >>> static unsigned nr_lrs; >>> >>> +/* The GIC mapping of CPU interfaces does not necessarily match the >>> + * logical CPU numbering. Let''s use mapping as returned by the GIC >>> + * itself >>> + */ >>> +static DEFINE_PER_CPU(u8, gic_cpu_id); >>> + >> >> Following the discussion on another thread >> (http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we >> need to initialize each CPU ID to 0xff by default, to be able to bring >> up secondary CPUs with an SGI (send_SGI_mask). > > We may as well use the send_SGI_allbutself mechanism, which doesn''t rely > on knowing and target ids. >> But as I understand, per_cpu area for a give area is initialized in >> cpu_up and there is no way to give a default value. So user could call >> send_SGI_mask to early function and use an invalid CPU area. > > If we need to go down this path then I would write something like > cpu_online(cpu) ? this_cpu(thing, cpu) : 0xff > but I don''t think we need to because we can easily avoid the problem. > >> IMHO, this solution is too fragile and I would like to use the same >> solution as my first version (ie with an array of 8 cells). > > I think cpu bring up should use allbutself and the other callers can > reasonable be restricted to only operate on cpus which are up and > therefore have a valid mask already allocated and initialised.Ok, so I will, at least, add a comment in the gic code to explain this restriction. -- Julien Grall
Julien Grall
2013-Sep-20 15:03 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
I only resend this patch. =================================================================================== commit 89aafa2d3d8a4f9ebce700673f89c8b4822a02e8 Author: Julien Grall <julien.grall@linaro.org> Date: Thu Aug 29 18:38:06 2013 +0100 xen/arm: gic: Use the correct CPU ID The GIC mapping of CPU interfaces does not necessarily match the logical CPU numbering. When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID. It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC will return a value that corresponds only to the processor reading the register. So Xen can use the PPI 0 to initialize the mapping. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v4: - Make logical and between the cpumask given in arguments and cpu_possible_map. - Make sure the per_cpu is initialized - Add comment restriction for gic_set_irq_properties Changes in v3: - Correctly create the mask in gic_cpu_mask Changes in v2: - Use per-cpu variable instead of an array - Add comment for NR_GIC_CPU_IF diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index b969d23..9f703a0 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -57,6 +57,32 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); static unsigned nr_lrs; +/* The GIC mapping of CPU interfaces does not necessarily match the + * logical CPU numbering. Let''s use mapping as returned by the GIC + * itself + */ +static DEFINE_PER_CPU(u8, gic_cpu_id); + +/* Maximum cpu interface per GIC */ +#define NR_GIC_CPU_IF 8 + +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) +{ + unsigned int cpu; + unsigned int mask = 0; + cpumask_t possible_mask; + + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); + for_each_cpu(cpu, &possible_mask) + { + ASSERT(cpu < NR_GIC_CPU_IF); + ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start); + mask |= per_cpu(gic_cpu_id, cpu); + } + + return mask; +} + unsigned int gic_number_lines(void) { return gic.lines; @@ -182,16 +208,18 @@ static hw_irq_controller gic_guest_irq_type = { .set_affinity = gic_irq_set_affinity, }; -/* needs to be called with gic.lock held */ +/* + * - needs to be called with gic.lock held + * - needs to be called with a valid cpu_mask, ie each cpu in the mask has + * already called gic_cpu_init + */ static void gic_set_irq_properties(unsigned int irq, bool_t level, const cpumask_t *cpu_mask, unsigned int priority) { volatile unsigned char *bytereg; uint32_t cfg, edgebit; - unsigned int mask = cpumask_bits(cpu_mask)[0]; - - ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */ + unsigned int mask = gic_cpu_mask(cpu_mask); /* Set edge / level */ cfg = GICD[GICD_ICFGR + irq / 16]; @@ -300,6 +328,8 @@ static void __cpuinit gic_cpu_init(void) { int i; + this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff; + /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so * even though they are controlled with GICD registers, they must * be set up here with the other per-cpu state. */ @@ -431,13 +461,13 @@ void __init gic_init(void) void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) { - unsigned long mask = cpumask_bits(cpumask)[0]; + unsigned int mask = 0; + cpumask_t online_mask; ASSERT(sgi < 16); /* There are only 16 SGIs */ - mask &= cpumask_bits(&cpu_online_map)[0]; - - ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */ + cpumask_and(&online_mask, cpumask, &cpu_online_map); + mask = gic_cpu_mask(&online_mask); dsb(); -- Julien Grall
Ian Campbell
2013-Sep-20 15:44 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > +{ > + unsigned int cpu; > + unsigned int mask = 0; > + cpumask_t possible_mask; > + > + cpumask_and(&possible_mask, cpumask, &cpu_possible_map);Based on the other subconversation doesn''t this need to be online_mask? Or is the cpu area setup for cpus which are possible but not online? Should the check (whichever it is) be an assertion?> + for_each_cpu(cpu, &possible_mask) > + { > + ASSERT(cpu < NR_GIC_CPU_IF); > + ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start); > + mask |= per_cpu(gic_cpu_id, cpu); > + } > + > + return mask; > +}
Julien Grall
2013-Sep-20 15:58 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On 09/20/2013 04:44 PM, Ian Campbell wrote:> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote: > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) >> +{ >> + unsigned int cpu; >> + unsigned int mask = 0; >> + cpumask_t possible_mask; >> + >> + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); > > Based on the other subconversation doesn''t this need to be online_mask? > Or is the cpu area setup for cpus which are possible but not online?cpu area is initialized a little bit before the cpu is bring up: * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE) * signal the cpu to boot (__cpu_up) ... * call start_secondary * initialize the gic cpu interface (gic_cpu_init) * route ppis (which used gic_cpu_mask) * set the cpu online If we use the cpu online mask, Xen won''t be able to route the different ppis to the processor.> Should the check (whichever it is) be an assertion?If we stay with the cpu_possible_map, I think we are fine without an assertion.>> + for_each_cpu(cpu, &possible_mask) >> + { >> + ASSERT(cpu < NR_GIC_CPU_IF); >> + ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start); >> + mask |= per_cpu(gic_cpu_id, cpu); >> + } >> + >> + return mask; >> +} > >-- Julien Grall
Ian Campbell
2013-Sep-20 16:06 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On Fri, 2013-09-20 at 16:58 +0100, Julien Grall wrote:> On 09/20/2013 04:44 PM, Ian Campbell wrote: > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote: > > > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > >> +{ > >> + unsigned int cpu; > >> + unsigned int mask = 0; > >> + cpumask_t possible_mask; > >> + > >> + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); > > > > Based on the other subconversation doesn''t this need to be online_mask? > > Or is the cpu area setup for cpus which are possible but not online? > > cpu area is initialized a little bit before the cpu is bring up: > * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE) > * signal the cpu to boot (__cpu_up) > ... > * call start_secondary > * initialize the gic cpu interface (gic_cpu_init) > * route ppis (which used gic_cpu_mask) > * set the cpu online > > If we use the cpu online mask, Xen won''t be able to route the different > ppis to the processor. > > > Should the check (whichever it is) be an assertion? > > If we stay with the cpu_possible_map, I think we are fine without an > assertion.OK. OOI where in the above is the cpu_possible_map setup? Quite a bit before the CPU_UP_PREPARE hook I think? It shouldn''t be a problem, just interested... Ian
Julien Grall
2013-Sep-20 18:48 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On 09/20/2013 05:06 PM, Ian Campbell wrote:> On Fri, 2013-09-20 at 16:58 +0100, Julien Grall wrote: >> On 09/20/2013 04:44 PM, Ian Campbell wrote: >>> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote: >>> >>>> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) >>>> +{ >>>> + unsigned int cpu; >>>> + unsigned int mask = 0; >>>> + cpumask_t possible_mask; >>>> + >>>> + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); >>> >>> Based on the other subconversation doesn''t this need to be online_mask? >>> Or is the cpu area setup for cpus which are possible but not online? >> >> cpu area is initialized a little bit before the cpu is bring up: >> * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE) >> * signal the cpu to boot (__cpu_up) >> ... >> * call start_secondary >> * initialize the gic cpu interface (gic_cpu_init) >> * route ppis (which used gic_cpu_mask) >> * set the cpu online >> >> If we use the cpu online mask, Xen won''t be able to route the different >> ppis to the processor. >> >>> Should the check (whichever it is) be an assertion? >> >> If we stay with the cpu_possible_map, I think we are fine without an >> assertion. > > OK. > > OOI where in the above is the cpu_possible_map setup? Quite a bit before > the CPU_UP_PREPARE hook I think? It shouldn''t be a problem, just > interested...With this patch series, it''s initialized by init_cpus_maps. -- Julien Grall
Ian Campbell
2013-Sep-25 15:35 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > +{ > + unsigned int cpu; > + unsigned int mask = 0; > + cpumask_t possible_mask; > + > + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); > + for_each_cpu(cpu, &possible_mask) > + { > + ASSERT(cpu < NR_GIC_CPU_IF); > + ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I think we can live without this check. After all the CPU is in possible map.
Ian Campbell
2013-Sep-25 15:36 UTC
Re: [PATCH v3 1/6] xen/arm: use cpumask_t to describe cpu mask in gic_route_dt_irq
On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:> Replace by cpumask_t to take advantage of cpumask_* helpers. > > Signed-off-by: Julien Grall <julien.grall@linaro.org>Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian Campbell
2013-Sep-25 15:37 UTC
Re: [PATCH v3 2/6] xen/arm: Initialize correctly IRQ routing
On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:> When Xen initialize the GIC distributor, we need to route all the IRQs to > the boot CPU. The CPU ID can differ between Xen and the GIC. > > When ITARGETSR0 is read, each field will return a value that corresponds > only to the processor reading the register. So Xen can use the PPI 0 to > initialize correctly the routing. > > Signed-off-by: Julien Grall <julien.grall@linaro.org>Acked-by: Ian Campbell <ian.campbell@citrix.com>
On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:> The GIC can handle maximum 8 cpus (0...7). The CPU id 7 is still valid. > > Signed-off-by: Julien Grall <julien.grall@linaro.org>Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian Campbell
2013-Sep-25 15:38 UTC
Re: [PATCH v3 5/6] xen/arm: Dissociate logical and hardware CPU ID
On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:> Introduce cpu_logical_map to associate a logical CPU ID to an hardware CPU ID. > This map will be filled during Xen boot via the device tree. Each CPU node > contains a "reg" property which contains the hardware ID (ie MPIDR[0:23]). > > Also move /cpus parsing later so we can use the dt_* API. > > Signed-off-by: Julien Grall <julien.grall@linaro.org>Acked-by: Ian Campbell <ian.campbell@citrix.com>
Ian Campbell
2013-Sep-25 15:41 UTC
Re: [PATCH v3 6/6] xen/arm: Use the hardware ID to boot correctly secondary cpus
On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote:> + /* Browse the logical map and find the associate logical cpu ID */We already have init_stack (the stack for the cpu which is coming up to use). In my start of day rewrite I also add/extend init_ttbr. You could add init_cpu_id or something. TBH it''s getting to the point where we might want struct init_info { ... }> + for ( cpuid = 1; cpuid < nr_cpu_ids; cpuid++ ) > + { > + if ( cpu_logical_map(cpuid) == hwid ) > + break; > + } > +
Julien Grall
2013-Sep-25 15:42 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On 09/25/2013 04:35 PM, Ian Campbell wrote:> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote: >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) >> +{ >> + unsigned int cpu; >> + unsigned int mask = 0; >> + cpumask_t possible_mask; >> + >> + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); >> + for_each_cpu(cpu, &possible_mask) >> + { >> + ASSERT(cpu < NR_GIC_CPU_IF); >> + ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start); > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I > think we can live without this check. After all the CPU is in possible > map.Being in cpu possible map doesn''t mean that the per cpu region is initialized for the given cpu. I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM platform. Can we move this define in percpu.h? -- Julien Grall
Ian Campbell
2013-Sep-25 15:48 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote:> On 09/25/2013 04:35 PM, Ian Campbell wrote: > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote: > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > >> +{ > >> + unsigned int cpu; > >> + unsigned int mask = 0; > >> + cpumask_t possible_mask; > >> + > >> + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); > >> + for_each_cpu(cpu, &possible_mask) > >> + { > >> + ASSERT(cpu < NR_GIC_CPU_IF); > >> + ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start); > > > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I > > think we can live without this check. After all the CPU is in possible > > map. > > Being in cpu possible map doesn''t mean that the per cpu region is > initialized for the given cpu.So you expect this ASSERT to trigger in practice? That''s not good...> I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM > platform. Can we move this define in percpu.h?I''m wondering if it should be the same, I think it was chosen on x86 to result in a non-canonical address (i.e. a guaranteed fault) and ARM copied it. __per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB from the top of the 64 bit address space, which is also invalid. I think, Tim, did you consider this or just copy the x86 value? Anyway, my point is that they are the same only through coincidence ;-) Ian.
Ian Campbell
2013-Sep-25 15:53 UTC
Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID
On Wed, 2013-09-25 at 16:48 +0100, Ian Campbell wrote:> On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote: > > On 09/25/2013 04:35 PM, Ian Campbell wrote: > > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote: > > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > > >> +{ > > >> + unsigned int cpu; > > >> + unsigned int mask = 0; > > >> + cpumask_t possible_mask; > > >> + > > >> + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); > > >> + for_each_cpu(cpu, &possible_mask) > > >> + { > > >> + ASSERT(cpu < NR_GIC_CPU_IF); > > >> + ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start); > > > > > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I > > > think we can live without this check. After all the CPU is in possible > > > map. > > > > Being in cpu possible map doesn''t mean that the per cpu region is > > initialized for the given cpu. > > So you expect this ASSERT to trigger in practice? That''s not good... > > > I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM > > platform. Can we move this define in percpu.h? > > I''m wondering if it should be the same, I think it was chosen on x86 to > result in a non-canonical address (i.e. a guaranteed fault) and ARM > copied it. > > __per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB > from the top of the 64 bit address space, which is also invalid. I > think, Tim, did you consider this or just copy the x86 value?Actually, since the offset of the per_cpu var gets added the result is not invalid at all, it''ll be the offset within the percpu of the area (i.e. a small number). Luckily we deliberately don''t map anything at 0..2MB and Xen is smaller than 2MB and per_cpu stuff fits inside Xen. So it works out OK (where OK means deliberately traps). Phew.
At 16:53 +0100 on 25 Sep (1380128037), Ian Campbell wrote:> On Wed, 2013-09-25 at 16:48 +0100, Ian Campbell wrote: > > On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote: > > > On 09/25/2013 04:35 PM, Ian Campbell wrote: > > > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote: > > > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask) > > > >> +{ > > > >> + unsigned int cpu; > > > >> + unsigned int mask = 0; > > > >> + cpumask_t possible_mask; > > > >> + > > > >> + cpumask_and(&possible_mask, cpumask, &cpu_possible_map); > > > >> + for_each_cpu(cpu, &possible_mask) > > > >> + { > > > >> + ASSERT(cpu < NR_GIC_CPU_IF); > > > >> + ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start); > > > > > > > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I > > > > think we can live without this check. After all the CPU is in possible > > > > map. > > > > > > Being in cpu possible map doesn''t mean that the per cpu region is > > > initialized for the given cpu. > > > > So you expect this ASSERT to trigger in practice? That''s not good... > > > > > I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM > > > platform. Can we move this define in percpu.h? > > > > I''m wondering if it should be the same, I think it was chosen on x86 to > > result in a non-canonical address (i.e. a guaranteed fault) and ARM > > copied it. > > > > __per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB > > from the top of the 64 bit address space, which is also invalid. I > > think, Tim, did you consider this or just copy the x86 value? > > Actually, since the offset of the per_cpu var gets added the result is > not invalid at all, it''ll be the offset within the percpu of the area > (i.e. a small number). Luckily we deliberately don''t map anything at > 0..2MB and Xen is smaller than 2MB and per_cpu stuff fits inside Xen. So > it works out OK (where OK means deliberately traps). Phew.Yes, it maps to NULL+offset, where offset is comfortably < 1 page in the current build; AFAICS this was deliberate on x86 and certainly deliberate when I copied it. The stronger property that offset must always < 2MB didn''t occur to me, but is reassuring. :) Tim.
Julien Grall
2013-Sep-26 10:18 UTC
Re: [PATCH v3 6/6] xen/arm: Use the hardware ID to boot correctly secondary cpus
On 09/25/2013 04:41 PM, Ian Campbell wrote:> On Wed, 2013-09-18 at 14:15 +0100, Julien Grall wrote: >> + /* Browse the logical map and find the associate logical cpu ID */ > > We already have init_stack (the stack for the cpu which is coming up to > use). In my start of day rewrite I also add/extend init_ttbr. You could > add init_cpu_id or something. > TBH it''s getting to the point where we might want struct init_info > { ... }Good idea. I will add a patch for this solution in my patch series.>> + for ( cpuid = 1; cpuid < nr_cpu_ids; cpuid++ ) >> + { >> + if ( cpu_logical_map(cpuid) == hwid ) >> + break; >> + } >> + >-- Julien Grall