From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v3 08/11] xen: arm: rewrite start of day page table and cpu bring up Date: Fri, 27 Sep 2013 15:21:04 +0100 Message-ID: <52459450.9010600@linaro.org> References: <1380276965.29483.178.camel@kazak.uk.xensource.com> <1380277240-27900-8-git-send-email-ian.campbell@citrix.com> <52458885.9080705@linaro.org> <1380291044.8994.100.camel@hastur.hellion.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1380291044.8994.100.camel@hastur.hellion.org.uk> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.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