| Message ID | 1374550289-25305-2-git-send-email-nicolas.pitre@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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>
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
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 --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
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(-)