diff mbox

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

Message ID 1374550289-25305-2-git-send-email-nicolas.pitre@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Nicolas Pitre July 23, 2013, 3:31 a.m. UTC
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.

The register allocation in __cpu_suspend is reworked in order to better
accommodate the additional argument.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/kernel/sleep.S   | 25 +++++++++++--------------
 arch/arm/kernel/suspend.c |  8 +++++---
 2 files changed, 16 insertions(+), 17 deletions(-)

Comments

Dave Martin July 24, 2013, 4:47 p.m. UTC | #1
On Mon, Jul 22, 2013 at 11:31:17PM -0400, 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.

This only works because we happen to swap MPIDRs in cpu_logical_map
before we get here, so that cpu_logical_map(smp_processor_id())
described the post-resume situation for this logical CPU, not the
current situation.

We have to swizzle cpu_logical_map() somewhere, and the suspend path is
just as good as the resume path for that.  But this patch commits us to
requiring that on the suspend path specifically -- I think that ought to
be mentioned somewhere.  A comment in the preamble for __cpu_suspend
would be enough, I think.

Looks like the patch should work though, and it does remove the need to
read the MPIDR, which is slightly nicer for the non-switcher case.

Cheers
---Dave

> 
> The register allocation in __cpu_suspend is reworked in order to better
> accommodate the additional argument.
> 
> Signed-off-by: Nicolas Pitre <nico@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
>   */
>  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)
> +	/* 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
>  	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
> -- 
> 1.8.1.2
>
Nicolas Pitre July 24, 2013, 6:55 p.m. UTC | #2
On Wed, 24 Jul 2013, Dave Martin wrote:

> On Mon, Jul 22, 2013 at 11:31:17PM -0400, 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.
> 
> This only works because we happen to swap MPIDRs in cpu_logical_map
> before we get here, so that cpu_logical_map(smp_processor_id())
> described the post-resume situation for this logical CPU, not the
> current situation.

Yes, that's more or less what I said above.  Maybe that should be 
clarified further?

> We have to swizzle cpu_logical_map() somewhere, and the suspend path is
> just as good as the resume path for that.

Not quite.  On the resume path, we have to get to the context save array 
while having no mmu active and no stack.  There is no way we could take 
over another CPU's context in those conditions without making it 
extremely painful on ourselves.  It is therefore way easier to swizzle 
cpu_logical_map() on the suspend path and keep the suspend/resume logic 
unmodified.

> But this patch commits us to requiring that on the suspend path 
> specifically -- I think that ought to be mentioned somewhere. A 
> comment in the preamble for __cpu_suspend would be enough, I think.

What comment would you suggest?  I want to make sure the possible 
confusion you see is properly addressed.

> Looks like the patch should work though, and it does remove the need to
> read the MPIDR, which is slightly nicer for the non-switcher case.

This patch works fine here, and I'd expect rather catastrophic results 
if it didn't.  ;-)


Nicolas
Dave Martin July 25, 2013, 10:55 a.m. UTC | #3
On Wed, Jul 24, 2013 at 02:55:00PM -0400, Nicolas Pitre wrote:
> On Wed, 24 Jul 2013, Dave Martin wrote:
> 
> > On Mon, Jul 22, 2013 at 11:31:17PM -0400, 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.
> > 
> > This only works because we happen to swap MPIDRs in cpu_logical_map
> > before we get here, so that cpu_logical_map(smp_processor_id())
> > described the post-resume situation for this logical CPU, not the
> > current situation.
> 
> Yes, that's more or less what I said above.  Maybe that should be 
> clarified further?
> 
> > We have to swizzle cpu_logical_map() somewhere, and the suspend path is
> > just as good as the resume path for that.
> 
> Not quite.  On the resume path, we have to get to the context save array 
> while having no mmu active and no stack.  There is no way we could take 
> over another CPU's context in those conditions without making it 
> extremely painful on ourselves.  It is therefore way easier to swizzle 
> cpu_logical_map() on the suspend path and keep the suspend/resume logic 
> unmodified.

For this implementation, that's correct, and it's a perfectly good way
of doing things.

My point is that the user of cpu_resume() does not necessarily realise
the nature of this dependency.

> > But this patch commits us to requiring that on the suspend path 
> > specifically -- I think that ought to be mentioned somewhere. A 
> > comment in the preamble for __cpu_suspend would be enough, I think.
> 
> What comment would you suggest?  I want to make sure the possible 
> confusion you see is properly addressed.

I think we just need to state that the value of
cpu_logical_map(smp_processor_id()) must be the MPIDR of the physical
CPU the suspending logical CPU will resume on.  Consequently, if doing a
migration, cpu_logical_map() must be updated appropriately somewhere
between cpu_pm_enter() and cpu_suspend().


This also means that the choice of destination CPU can't be made
dynamically at resume time.  But I'm not sure that's useful or even
makes sense, so I'm not worrying about it.

> > Looks like the patch should work though, and it does remove the need to
> > read the MPIDR, which is slightly nicer for the non-switcher case.
> 
> This patch works fine here, and I'd expect rather catastrophic results 
> if it didn't.  ;-)

Sure, I'm not doubting that the code works.

Cheers
---Dave
Lorenzo Pieralisi July 29, 2013, 11:50 a.m. UTC | #4
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

> The register allocation in __cpu_suspend is reworked in order to better
> accommodate the additional argument.
> 
> Signed-off-by: Nicolas Pitre <nico@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.

>   */
>  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.

> +	/* 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.

>  	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@arm.com>
Nicolas Pitre July 30, 2013, 2:08 a.m. UTC | #5
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@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.

> >   */
> >  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.

> >  	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@arm.com>

Thanks.


Nicolas
Dave Martin July 30, 2013, 9:15 a.m. UTC | #6
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@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@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@arm.com>
> 
> Thanks.
> 
> 
> Nicolas
diff mbox

Patch

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
  */
 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)
+	/* 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
 	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