[PATCH 01/13] ARM: suspend: use hash of cpu_logical_map value to index into save array

[Dave Martin]

On Mon, Jul 29, 2013 at 10:08:06PM -0400, Nicolas Pitre wrote:
> On Mon, 29 Jul 2013, Lorenzo Pieralisi wrote:
> 
> > On Tue, Jul 23, 2013 at 04:31:17AM +0100, Nicolas Pitre wrote:
> > > Currently we hash the MPIDR of the CPU being suspended to determine which
> > > entry in the sleep_save_sp array to use. In some situations, such as when
> > > we want to resume on another physical CPU, the MPIDR of another CPU should
> > > be used instead.
> > > 
> > > So let's use the value of cpu_logical_map(smp_processor_id()) in place
> > > of the MPIDR in the suspend path.  This will result in the same index
> > > being used as with the previous code unless the caller has modified
> > > cpu_logical_map() beforehand.
> > 
> > I will rely on your wisdom to rewrite the commit log in a way that
> > does not hint at dangerous creativity, if you catch my drift :D
> 
> I've augmented it with some of the earlier comments from Dave Martin.  
> I think it is fine to be clear in the commit log about what we intend 
> this change to be used for.
> 
> > > The register allocation in __cpu_suspend is reworked in order to better
> > > accommodate the additional argument.
> > > 
> > > Signed-off-by: Nicolas Pitre <nico at linaro.org>
> > > ---
> > >  arch/arm/kernel/sleep.S   | 25 +++++++++++--------------
> > >  arch/arm/kernel/suspend.c |  8 +++++---
> > >  2 files changed, 16 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/arch/arm/kernel/sleep.S b/arch/arm/kernel/sleep.S
> > > index db1536b8b3..836d10e698 100644
> > > --- a/arch/arm/kernel/sleep.S
> > > +++ b/arch/arm/kernel/sleep.S
> > > @@ -55,6 +55,7 @@
> > >   * specific registers and some other data for resume.
> > >   *  r0 = suspend function arg0
> > >   *  r1 = suspend function
> > > + *  r2 = MPIDR value used to index into sleep_save_sp
> > 
> > CPU's MPIDR or something like that, let's not hint at possible creative usage.
> 
> What about "MPIDR of the CPU to be resumed" which should normally just 
> be the current CPU's?
> 
> After all, creativity is not always a bad thing given it is reviewed 
> properly.

OK, sounds reasonable.

> 
> > >   */
> > >  ENTRY(__cpu_suspend)
> > >  	stmfd	sp!, {r4 - r11, lr}
> > > @@ -67,23 +68,19 @@ ENTRY(__cpu_suspend)
> > >  	mov	r5, sp			@ current virtual SP
> > >  	add	r4, r4, #12		@ Space for pgd, virt sp, phys resume fn
> > >  	sub	sp, sp, r4		@ allocate CPU state on stack
> > > -	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
> > > -	add	r0, sp, #8		@ save pointer to save block
> > > -	mov	r1, r4			@ size of save block
> > > -	mov	r2, r5			@ virtual SP
> > >  	ldr	r3, =sleep_save_sp
> > > +	stmfd	sp!, {r0, r1}		@ save suspend func arg and pointer
> > >  	ldr	r3, [r3, #SLEEP_SAVE_SP_VIRT]
> > > -	ALT_SMP(mrc p15, 0, r9, c0, c0, 5)
> > > -        ALT_UP_B(1f)
> > > -	ldr	r8, =mpidr_hash
> > > -	/*
> > > -	 * This ldmia relies on the memory layout of the mpidr_hash
> > > -	 * struct mpidr_hash.
> > > -	 */
> > > -	ldmia	r8, {r4-r7}	@ r4 = mpidr mask (r5,r6,r7) = l[0,1,2] shifts
> > > -	compute_mpidr_hash	lr, r5, r6, r7, r9, r4
> > > -	add	r3, r3, lr, lsl #2
> > > +	ALT_SMP(ldr r0, =mpidr_hash)
> > > +       ALT_UP_B(1f)
> > 
> > Should be a tab, do not know if that's an email server issue or not.
> 
> Amistake on my part.
> 
> > > +	/* This ldmia relies on the memory layout of the mpidr_hash struct */
> > > +	ldmia	r0, {r1, r6-r8}	@ r1 = mpidr mask (r6,r7,r8) = l[0,1,2] shifts
> > > +	compute_mpidr_hash	r0, r6, r7, r8, r2, r1
> > > +	add	r3, r3, r0, lsl #2
> > >  1:
> > > +	mov	r2, r5			@ virtual SP
> > > +	mov	r1, r4			@ size of save block
> > > +	add	r0, sp, #8		@ pointer to save block
> > 
> > It is already a bit complex to follow, code is ok, but line above was
> > better placed closer to sp last change, not after hashing the MPIDR.
> 
> Well, I tend to disagree.  I think it is much clearer to set function 
> arguments closer to the call site so that r0 doesn't have to be live 
> with the stack location throughout this code.

Fair point, but I'm happy either way.

With or without, feel free to add

Reviewed-by: Dave Martin <Dave.Martin at arm.com>

Cheers
---Dave

> 
> > >  	bl	__cpu_suspend_save
> > >  	adr	lr, BSYM(cpu_suspend_abort)
> > >  	ldmfd	sp!, {r0, pc}		@ call suspend fn
> > > diff --git a/arch/arm/kernel/suspend.c b/arch/arm/kernel/suspend.c
> > > index 41cf3cbf75..2835d35234 100644
> > > --- a/arch/arm/kernel/suspend.c
> > > +++ b/arch/arm/kernel/suspend.c
> > > @@ -10,7 +10,7 @@
> > >  #include <asm/suspend.h>
> > >  #include <asm/tlbflush.h>
> > >  
> > > -extern int __cpu_suspend(unsigned long, int (*)(unsigned long));
> > > +extern int __cpu_suspend(unsigned long, int (*)(unsigned long), u32 cpuid);
> > >  extern void cpu_resume_mmu(void);
> > >  
> > >  #ifdef CONFIG_MMU
> > > @@ -21,6 +21,7 @@ extern void cpu_resume_mmu(void);
> > >  int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
> > >  {
> > >  	struct mm_struct *mm = current->active_mm;
> > > +	u32 __mpidr = cpu_logical_map(smp_processor_id());
> > >  	int ret;
> > >  
> > >  	if (!idmap_pgd)
> > > @@ -32,7 +33,7 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
> > >  	 * resume (indicated by a zero return code), we need to switch
> > >  	 * back to the correct page tables.
> > >  	 */
> > > -	ret = __cpu_suspend(arg, fn);
> > > +	ret = __cpu_suspend(arg, fn, __mpidr);
> > >  	if (ret == 0) {
> > >  		cpu_switch_mm(mm->pgd, mm);
> > >  		local_flush_bp_all();
> > > @@ -44,7 +45,8 @@ int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
> > >  #else
> > >  int cpu_suspend(unsigned long arg, int (*fn)(unsigned long))
> > >  {
> > > -	return __cpu_suspend(arg, fn);
> > > +	u32 __mpidr = cpu_logical_map(smp_processor_id());
> > > +	return __cpu_suspend(arg, fn, __mpidr);
> > >  }
> > >  #define	idmap_pgd	NULL
> > >  #endif
> > 
> > Apart from these nits, let's hope it is the last cpu_suspend bit we need to
> > change, please land it on -next asap, of course if Russell is ok with that.
> > 
> > I tested it on TC2.
> > 
> > FWIW:
> > 
> > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi at arm.com>
> 
> Thanks.
> 
> 
> Nicolas