Re: [RFC PATCH 2/7] ARM: SMP: generic SMP spin-table method routines

[Alex Elder <elder-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

On 03/31/2014 10:21 AM, Mark Rutland wrote:
> Hi Alex,
> 
> On Fri, Mar 28, 2014 at 09:12:55PM +0000, Alex Elder wrote:
>> Implement a centralized version of the spin table (a.k.a. "holding
>> pen") method of secondary CPU initialization.  This is the first
>> step in removing a number of duplicate implementations of this code.
>>
>> The eventual goal is to allow "enable-method" properties in device
>> tree nodes for CPUs to select and use this common code.  As such,
>> some of the names are selected to match the names used in the SMP
>> spin-table code for ARM64.

Thanks for reviewing this Mark.  I'll respond below, but given
that Russell King is not interested in incorporating my changes
into the core code I think it may be moot.  Russell believes
centralizing this code encourages people who don't have a clue
what the hell they're doing to cargo-cult program.

> Given that there is a fundamental difference to the spin-table protocol
> in use on arm64 (in that here we are required to poke an arbitrary
> interrupt controller to send an SGI rather than just issuing a SEV), I
> would prefer that this had a name other than "spin-table" to
> disambiguate the two protocols.

It's possible (but I have no way of knowing) that a SEV is
sufficient to wake up the processor as well.  It is on the
platform I'm working on.  But in any case I would happily
use a different name, maybe "holding-pen" or something.

>> Note:
>> Most implementations examine only the bottom 4 bits of the MPIDR in
>> order to determine a CPU's id.  This version looks at the bottom 24
>> bits instead, based on MPIDR_HWID_BITMASK.  If using only 4 bits is
>> a requirement for most of the platforms that might use it I'll
>> switch this use 4 bits instead.
> 
> Given that we require people to describe all of the MPIDR Aff* fields in
> the DT, and can update any board files as necessary, is this a problem?

You're right, it should not be a problem.  I'm just a little
nervous about changing *any* behavior when I don't have the
hardware available to test the result.  So I wanted to be
sure to call attention to this.

> IMO Using all the Aff bits is preferable.

I agree, absolutely.

Thanks again.

					-Alex

> Cheers,
> Mark.
> 
>>
>> Signed-off-by: Alex Elder <elder-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> ---
>>  arch/arm/include/asm/smp.h |    5 +++
>>  arch/arm/kernel/head.S     |   33 +++++++++++++++++++
>>  arch/arm/kernel/smp.c      |   77 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 115 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/smp.h b/arch/arm/include/asm/smp.h
>> index 22a3b9b..83064d1 100644
>> --- a/arch/arm/include/asm/smp.h
>> +++ b/arch/arm/include/asm/smp.h
>> @@ -75,6 +75,11 @@ struct secondary_data {
>>  extern struct secondary_data secondary_data;
>>  extern volatile int pen_release;
>>  
>> +extern volatile u32 secondary_holding_pen_release;
>> +extern void secondary_holding_pen(void);
>> +extern int smp_boot_secondary(unsigned int cpu, struct task_struct *idle);
>> +extern void smp_secondary_init(unsigned int cpu);
>> +
>>  extern int __cpu_disable(void);
>>  
>>  extern void __cpu_die(unsigned int cpu);
>> diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
>> index f5f381d..3340f94 100644
>> --- a/arch/arm/kernel/head.S
>> +++ b/arch/arm/kernel/head.S
>> @@ -22,6 +22,7 @@
>>  #include <asm/memory.h>
>>  #include <asm/thread_info.h>
>>  #include <asm/pgtable.h>
>> +#include <asm/cputype.h>
>>  
>>  #if defined(CONFIG_DEBUG_LL) && !defined(CONFIG_DEBUG_SEMIHOSTING)
>>  #include CONFIG_DEBUG_LL_INCLUDE
>> @@ -402,6 +403,38 @@ __secondary_data:
>>  	.long	.
>>  	.long	secondary_data
>>  	.long	__secondary_switched
>> +
>> +
>> +	/*
>> +	 * Secondary cores spin in this "holding pen" until they are
>> +	 * signaled to proceed by jumping to secondary_startup
>> +	 * (above).  A core knows to proceed when it finds that the
>> +	 * value of the secondary_holding_pen_release global matches
>> +	 * its (hardware) CPU ID.  The secondary core acknowledges
>> +	 * it has begun executing by writing an invalid value (-1)
>> +	 * back into secondary_holding_pen_release (in
>> +	 * smp_operations->smp_secondary_init).
>> +	 */
>> +ENTRY(secondary_holding_pen)
>> + ARM_BE8(setend	be)
>> +	mrc	p15, 0, r0, c0, c0, 5	@ Get MPIDR and extract CPU id from it
>> +	and	r0, r0, #MPIDR_HWID_BITMASK
>> +	adr	r4, 1f			@ Get secondary_holding_pen_release
>> +	ldmia	r4, {r5, r6}		@ and compute its physical address
>> +	sub	r4, r4, r5
>> +	add	r6, r6, r4
>> +pen:	ldr	r7, [r6]		@ while secondary_holding_pen_release
>> +	cmp	r7, r0			@ doesn't hold our CPU id, spin
>> +	bne	pen
>> +	/*
>> +	 * At this point we have been released from the holding pen;
>> +	 * secondary_stack now contains the SVC stack for this core.
>> +	 */
>> +	b	secondary_startup
>> +ENDPROC(secondary_holding_pen)
>> +	.align
>> +1:	.long	.
>> +	.long	secondary_holding_pen_release
>>  #endif /* defined(CONFIG_SMP) */
>>  
>>  
>> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>> index b7b4c86..e18151a 100644
>> --- a/arch/arm/kernel/smp.c
>> +++ b/arch/arm/kernel/smp.c
>> @@ -59,6 +59,8 @@ struct secondary_data secondary_data;
>>   * boot "holding pen"
>>   */
>>  volatile int pen_release = -1;
>> +volatile u32 secondary_holding_pen_release = -1;
>> +static DEFINE_RAW_SPINLOCK(boot_lock);
>>  
>>  enum ipi_msg_type {
>>  	IPI_WAKEUP,
>> @@ -386,6 +388,81 @@ asmlinkage void secondary_start_kernel(void)
>>  	cpu_startup_entry(CPUHP_ONLINE);
>>  }
>>  
>> +static void write_pen_release(int val)
>> +{
>> +	secondary_holding_pen_release = val;
>> +	smp_wmb();
>> +	sync_cache_w(&secondary_holding_pen_release);
>> +}
>> +
>> +/*
>> + * This is a smp_operations->smp_boot_secondary function, called by
>> + * boot_secondary() to signal a secondary core spinning in
>> + * secondary_holding_pen() that it should proceed.  The current
>> + * (boot) core writes the secondary's (hardware) CPU ID into
>> + * secondary_holding_pen_release.  The secondary core signals it has
>> + * started running by rewriting an invalid value (-1) back
>> + * into secondary_holding_pen_release.
>> + */
>> +int smp_boot_secondary(unsigned int cpu, struct task_struct *idle)
>> +{
>> +	unsigned long timeout;
>> +
>> +	/*
>> +	 * The secondary core will wait for this lock after
>> +	 * signaling it has started.  That way we know it won't
>> +	 * proceed until we've recognized the acknowledgement.
>> +	 */
>> +	raw_spin_lock(&boot_lock);
>> +
>> +	/*
>> +	 * Release the secondary core from its holding pen by
>> +	 * writing its CPU ID into secondary_holding_pen_release.
>> +	 */
>> +	write_pen_release(cpu_logical_map(cpu));
>> +
>> +	/*
>> +	 * Send the secondary CPU a soft interrupt, thereby causing
>> +	 * it to jump to its secondary entry point.
>> +	 */
>> +	arch_send_wakeup_ipi_mask(cpumask_of(cpu));
>> +
>> +	/* Give it some time to start running. */
>> +	timeout = jiffies + (1 * HZ);
>> +	while (time_before(jiffies, timeout)) {
>> +		smp_rmb();
>> +		if (secondary_holding_pen_release == -1)
>> +			break;
>> +
>> +		udelay(10);
>> +	}
>> +
>> +	/*
>> +	 * We now know that the secondary core is running (or it
>> +	 * timed out).  Release the lock so it can proceed.
>> +	 */
>> +	raw_spin_unlock(&boot_lock);
>> +
>> +	return secondary_holding_pen_release == -1 ? 0 : -ENOSYS;
>> +}
>> +
>> +/*
>> + * This is a smp_operations->secondary_init function, called by
>> + * secondary_start_kernel() on a newly-booted secondary cpu to do
>> + * some initialization activity.  All we need to do is write
>> + * secondary_holding_pen_release with an invalid value to signal
>> + * we've started executing.  We synchronize with the boot core by
>> + * waiting to acquire the boot lock before continuing.
>> + */
>> +void smp_secondary_init(unsigned int cpu)
>> +{
>> +	/* Let the primary processor know we're out of the pen. */
>> +	write_pen_release(-1);
>> +
>> +	raw_spin_lock(&boot_lock);
>> +	raw_spin_unlock(&boot_lock);
>> +}
>> +
>>  void __init smp_cpus_done(unsigned int max_cpus)
>>  {
>>  	printk(KERN_INFO "SMP: Total of %d processors activated.\n",
>> -- 
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html