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

[Julien Grall <julien.grall@linaro.org>]

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