Re: [PATCH v3 08/11] xen: arm: rewrite start of day page table and cpu bring up

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

On 09/27/2013 03:10 PM, Ian Campbell wrote:
> On Fri, 2013-09-27 at 14:30 +0100, Julien Grall wrote:
> 
>>> @@ -581,6 +589,12 @@ static void __init init_cpus_maps(void)
>>>              }
>>>          }
>>>  
>>> +        if ( (rc = arch_cpu_init(hwid, cpu)) < 0 )
>>
>> As I understand your patch #6, arch_cpu_init take a logical cpu id (on
>> ARM64 it's used as an index in an array).
> 
> Yes, I thought I wanted to pass the hwid here, but it looks like I've
> got my wires crossed.
> 
>> So you should used j here.
> 
> You mean cpuidx I think, after having moved the call after the cpuidx++

In fact I mean i. Because i = 0 (if it's the boot CPU) or cpuidx++ for
non-boot CPU.

> 
> I wanted to handle the case where the function failed by having
> possible_map not contain failed cpus. I think I'll handle this by making
> tmp_map[i] == INVALID_MIDR in that case and checking that in the loop
> which sets bits in cpu_possible_map.

Sounds a good place.

>> Also, do we really need to call arch_cpu_init on the boot CPU?
> 
> Good question. I suppose not. On the other hand it gives the platform
> the option of doing per-cpu init not related to bring up. I think I'll
> keep this hook in place.

Ok.

>>>  };
>>>  
>>>  /* Shared state for coordinating CPU bringup */
>>> -unsigned long smp_up_cpu = 0;
>>> +unsigned long smp_up_cpu = ~0UL;
>>
>> MPIDR_INVALID?
> 
> yes, for all of those.
> 
>>> @@ -176,6 +147,7 @@ void __cpuinit start_secondary(unsigned long boot_phys_offset,
>>>      wmb();
>>>  
>>>      /* Now report this CPU is up */
>>> +    smp_up_cpu = ~0UL;
>>
>> smp_up_cpu = MPIDR_INVALID?
>>
>> Also, perhaps a dsb is needed here to ensure to update smp_up_cpu before
>> cpumask_set_cpu is updated.
> 
> Does anything rely on the order of these two writes? There is a wmb
> right after the writes so they will become visible together. I think
> it's probably OK to see one slightly before the other in either order.

I though smp_up_cpu was updated just after the loop
while( !cpu_online(cpu) ) but not. So it's fine for me.

>>
>>>      cpumask_set_cpu(cpuid, &cpu_online_map);
>>>      wmb();
>>
> 
> 

-- 
Julien Grall