From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] xen: arm: wait 1000ms for a CPU to come up, instead of forever Date: Wed, 14 Jan 2015 14:56:34 +0000 Message-ID: <1421247394.19103.273.camel@citrix.com> References: <1421244060-1745-1-git-send-email-ian.campbell@citrix.com> <54B67CCA.9060205@linaro.org> <1421246359.19103.269.camel@citrix.com> <54B68265.20001@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54B68265.20001@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: stefano.stabellini@eu.citrix.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Wed, 2015-01-14 at 14:51 +0000, Julien Grall wrote: > On 14/01/15 14:39, Ian Campbell wrote: > > On Wed, 2015-01-14 at 14:27 +0000, Julien Grall wrote: > >> Hi Ian, > >> > >> On 14/01/15 14:01, Ian Campbell wrote: > >>> Otherwise continue without it, which is preferable to the current > >>> infinite hang. > >> > >> Nice! > >> > >>> Slightly tweak the grammar of a comment in the same function. > >>> > >>> Signed-off-by: Ian Campbell > >>> --- > >>> xen/arch/arm/smpboot.c | 26 ++++++++++++++++++++++++-- > >>> 1 file changed, 24 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c > >>> index 14054ae..12538d4 100644 > >>> --- a/xen/arch/arm/smpboot.c > >>> +++ b/xen/arch/arm/smpboot.c > >>> @@ -357,6 +357,7 @@ int __init cpu_up_send_sgi(int cpu) > >>> int __cpu_up(unsigned int cpu) > >>> { > >>> int rc; > >>> + s_time_t deadline; > >>> > >>> printk("Bringing up CPU%d\n", cpu); > >>> > >>> @@ -369,7 +370,7 @@ int __cpu_up(unsigned int cpu) > >>> /* Tell the remote CPU which stack to boot on. */ > >>> init_data.stack = idle_vcpu[cpu]->arch.stack; > >>> > >>> - /* Tell the remote CPU what is it's logical CPU ID */ > >>> + /* Tell the remote CPU what its logical CPU ID is. */ > >>> init_data.cpuid = cpu; > >>> > >>> /* Open the gate for this CPU */ > >>> @@ -386,12 +387,33 @@ int __cpu_up(unsigned int cpu) > >>> return rc; > >>> } > >>> > >>> - while ( !cpu_online(cpu) ) > >>> + deadline = NOW() + MILLISECS(1000); > >> > >> Most of cpu_up callbacks are waiting for the CPU to come back. > > > > Where do you mean exactly? > > See for instance exynos5_cpu_power_up. Appears to be waiting the h/w to acknowledge that the CPU power is on, which is no guarantee that it is going to actually boot, or even make it to Xen code. > > >> > >>> + > >>> + while ( !cpu_online(cpu) && NOW() < deadline ) > >>> { > >>> cpu_relax(); > >>> process_pending_softirqs(); > >>> } > >>> > >>> + /* > >>> + * Nuke start of day info before checking one last time if the CPU > >>> + * actually came online. > >>> + * > >>> + * Doesn't completely avoid the posibility of it trying to > >>> + * progress with another CPUs stack etc, but better than nothing, > >>> + * hopefully. > >>> + */ > >>> + init_data.stack = NULL; > >>> + init_data.cpuid = ~0; > >>> + smp_up_cpu = MPIDR_INVALID; > >>> + clean_dcache(smp_up_cpu); > >> > >> I don't understand why you need to do this. Is it for pure clean up? If > >> so, please explain it in the commit message. > > > > Is the comment right above it not sufficient explanation? I can insert > > at the end of the first paragraph "If it is not online it may still be > > trying and may show up later" is that would help. > > It's more clear for me with "If it is not online ...". > > For the second paragraph, I would say "It doesn't completely avoid ...". OK, I'll make both changes. > Also I was wondering if there is any possibility to turn off the cpu if > it doesn't come online? PSCI has a cpu_off, and there will be arch specific mechanisms. Whether they will work under the circumstances is hard to say. In any case that is out of scope for this patch. Ian.