[PATCH v3 1/5] powerpc: Rename current_stack_pointer() to current_stack_frame()

[PATCH v3 1/5] powerpc: Rename current_stack_pointer() to current_stack_frame()

[Michael Ellerman]

current_stack_pointer(), which was called __get_SP(), used to just
return the value in r1.

But that caused problems in some cases, so it was turned into a
function in commit bfe9a2cfe91a ("powerpc: Reimplement __get_SP() as a
function not a define").

Because it's a function in a separate compilation unit to all its
callers, it has the effect of causing a stack frame to be created, and
then returns the address of that frame. This is good in some cases
like those described in the above commit, but in other cases it's
overkill, we just need to know what stack page we're on.

On some other arches current_stack_pointer is just a register global
giving the stack pointer, and we'd like to do that too. So rename our
current_stack_pointer() to current_stack_frame() to make that
possible.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/perf_event.h | 2 +-
 arch/powerpc/include/asm/reg.h        | 2 +-
 arch/powerpc/kernel/irq.c             | 4 ++--
 arch/powerpc/kernel/misc.S            | 4 ++--
 arch/powerpc/kernel/process.c         | 2 +-
 arch/powerpc/kernel/stacktrace.c      | 6 +++---
 6 files changed, 10 insertions(+), 10 deletions(-)

v3: New.

diff --git a/arch/powerpc/include/asm/perf_event.h b/arch/powerpc/include/asm/perf_event.h
index 7426d7a90e1e..eed3954082fa 100644
--- a/arch/powerpc/include/asm/perf_event.h
+++ b/arch/powerpc/include/asm/perf_event.h
@@ -32,7 +32,7 @@
 	do {							\
 		(regs)->result = 0;				\
 		(regs)->nip = __ip;				\
-		(regs)->gpr[1] = current_stack_pointer();	\
+		(regs)->gpr[1] = current_stack_frame();		\
 		asm volatile("mfmsr %0" : "=r" ((regs)->msr));	\
 	} while (0)
 
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1aa46dff0957..1b1ffdba6097 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1448,7 +1448,7 @@ static inline void mtsrin(u32 val, u32 idx)
 
 #define proc_trap()	asm volatile("trap")
 
-extern unsigned long current_stack_pointer(void);
+extern unsigned long current_stack_frame(void);
 
 extern unsigned long scom970_read(unsigned int address);
 extern void scom970_write(unsigned int address, unsigned long value);
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 5c9b11878555..02118c18434d 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -602,7 +602,7 @@ static inline void check_stack_overflow(void)
 #ifdef CONFIG_DEBUG_STACKOVERFLOW
 	long sp;
 
-	sp = current_stack_pointer() & (THREAD_SIZE-1);
+	sp = current_stack_frame() & (THREAD_SIZE-1);
 
 	/* check for stack overflow: is there less than 2KB free? */
 	if (unlikely(sp < 2048)) {
@@ -647,7 +647,7 @@ void do_IRQ(struct pt_regs *regs)
 	void *cursp, *irqsp, *sirqsp;
 
 	/* Switch to the irq stack to handle this */
-	cursp = (void *)(current_stack_pointer() & ~(THREAD_SIZE - 1));
+	cursp = (void *)(current_stack_frame() & ~(THREAD_SIZE - 1));
 	irqsp = hardirq_ctx[raw_smp_processor_id()];
 	sirqsp = softirq_ctx[raw_smp_processor_id()];
 
diff --git a/arch/powerpc/kernel/misc.S b/arch/powerpc/kernel/misc.S
index 974f65f79a8e..65f9f731c229 100644
--- a/arch/powerpc/kernel/misc.S
+++ b/arch/powerpc/kernel/misc.S
@@ -110,7 +110,7 @@ _GLOBAL(longjmp)
 	li	r3, 1
 	blr
 
-_GLOBAL(current_stack_pointer)
+_GLOBAL(current_stack_frame)
 	PPC_LL	r3,0(r1)
 	blr
-EXPORT_SYMBOL(current_stack_pointer)
+EXPORT_SYMBOL(current_stack_frame)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index e730b8e522b0..110db94cdf3c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -2051,7 +2051,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
 	sp = (unsigned long) stack;
 	if (sp == 0) {
 		if (tsk == current)
-			sp = current_stack_pointer();
+			sp = current_stack_frame();
 		else
 			sp = tsk->thread.ksp;
 	}
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index e2a46cfed5fd..c477b8585a29 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -57,7 +57,7 @@ void save_stack_trace(struct stack_trace *trace)
 {
 	unsigned long sp;
 
-	sp = current_stack_pointer();
+	sp = current_stack_frame();
 
 	save_context_stack(trace, sp, current, 1);
 }
@@ -71,7 +71,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		return;
 
 	if (tsk == current)
-		sp = current_stack_pointer();
+		sp = current_stack_frame();
 	else
 		sp = tsk->thread.ksp;
 
@@ -131,7 +131,7 @@ static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
 	}
 
 	if (tsk == current)
-		sp = current_stack_pointer();
+		sp = current_stack_frame();
 	else
 		sp = tsk->thread.ksp;
 
-- 
2.21.1

[PATCH v3 2/5] powerpc: Add current_stack_pointer as a register global

[Michael Ellerman]

From: Christophe Leroy <christophe.leroy@c-s.fr>

current_stack_frame() doesn't return the stack pointer, but the
caller's stack frame. See commit bfe9a2cfe91a ("powerpc: Reimplement
__get_SP() as a function not a define") and commit
acf620ecf56c ("powerpc: Rename __get_SP() to current_stack_pointer()")
for details.

In some cases this is overkill or incorrect, as it doesn't return the
current value of r1.

So add a current_stack_pointer register global to get the value of r1
directly.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
[mpe: Split out of other patch, tweak change log]
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/435e0030e942507766cbef5bc95f906262d2ccf2.1579849665.git.christophe.leroy@c-s.fr
---
 arch/powerpc/include/asm/reg.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 1b1ffdba6097..da5cab038e25 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1450,6 +1450,8 @@ static inline void mtsrin(u32 val, u32 idx)
 
 extern unsigned long current_stack_frame(void);
 
+register unsigned long current_stack_pointer asm("r1");
+
 extern unsigned long scom970_read(unsigned int address);
 extern void scom970_write(unsigned int address, unsigned long value);
 
-- 
2.21.1

v3: Split out, and use current_stack_pointer not get_sp()

[PATCH v3 3/5] powerpc/irq: Use current_stack_pointer in check_stack_overflow()

[Michael Ellerman]

From: Christophe Leroy <christophe.leroy@c-s.fr>

The purpose of check_stack_overflow() is to verify that the stack has
not overflowed.

To really know whether the stack pointer is still within boundaries,
the check must be done directly on the value of r1.

So use current_stack_pointer, which returns the current value of r1,
rather than current_stack_frame() which causes a frame to be created
and then returns that value.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/435e0030e942507766cbef5bc95f906262d2ccf2.1579849665.git.christophe.leroy@c-s.fr
---
 arch/powerpc/kernel/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 02118c18434d..c7d6f5cdffdb 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -602,7 +602,7 @@ static inline void check_stack_overflow(void)
 #ifdef CONFIG_DEBUG_STACKOVERFLOW
 	long sp;
 
-	sp = current_stack_frame() & (THREAD_SIZE-1);
+	sp = current_stack_pointer & (THREAD_SIZE - 1);
 
 	/* check for stack overflow: is there less than 2KB free? */
 	if (unlikely(sp < 2048)) {
-- 
2.21.1

v3: s/get_sp()/current_stack_pointer/

[PATCH v3 4/5] powerpc/irq: use IS_ENABLED() in check_stack_overflow()

[Michael Ellerman]

From: Christophe Leroy <christophe.leroy@c-s.fr>

Instead of #ifdef, use IS_ENABLED(CONFIG_DEBUG_STACKOVERFLOW).
This enable GCC to check for code validity even when the option
is not selected.

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/98855694e9e8993673af08cc2e97e16e0cf50f4a.1579849665.git.christophe.leroy@c-s.fr
---
 arch/powerpc/kernel/irq.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index c7d6f5cdffdb..46d5852fb00a 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -599,9 +599,11 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
 
 static inline void check_stack_overflow(void)
 {
-#ifdef CONFIG_DEBUG_STACKOVERFLOW
 	long sp;
 
+	if (!IS_ENABLED(CONFIG_DEBUG_STACKOVERFLOW))
+		return;
+
 	sp = current_stack_pointer & (THREAD_SIZE - 1);
 
 	/* check for stack overflow: is there less than 2KB free? */
@@ -609,7 +611,6 @@ static inline void check_stack_overflow(void)
 		pr_err("do_IRQ: stack overflow: %ld\n", sp);
 		dump_stack();
 	}
-#endif
 }
 
 void __do_irq(struct pt_regs *regs)
-- 
2.21.1

[PATCH v3 5/5] powerpc/irq: Use current_stack_pointer in do_IRQ()

[Michael Ellerman]

From: Christophe Leroy <christophe.leroy@c-s.fr>

Until commit 7306e83ccf5c ("powerpc: Don't use CURRENT_THREAD_INFO to
find the stack"), the current stack base address was obtained by
calling current_thread_info(). That inline function was simply masking
out the value of r1.

In that commit, it was changed to using current_stack_pointer() (since
renamed current_stack_frame()), which is a heavier function as it is
an outline assembly function which cannot be inlined and which reads
the content of the stack at 0(r1).

Convert to using current_stack_pointer for geting r1 and masking out
its value to obtain the base address of the stack pointer as before.

Fixes: 7306e83ccf5c ("powerpc: Don't use CURRENT_THREAD_INFO to find the stack")
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Link: https://lore.kernel.org/r/a37e699e7ab897742c07b6838a08af33bc9217e3.1579849665.git.christophe.leroy@c-s.fr
---
 arch/powerpc/kernel/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index 46d5852fb00a..1bed18b7229e 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -648,7 +648,7 @@ void do_IRQ(struct pt_regs *regs)
 	void *cursp, *irqsp, *sirqsp;
 
 	/* Switch to the irq stack to handle this */
-	cursp = (void *)(current_stack_frame() & ~(THREAD_SIZE - 1));
+	cursp = (void *)(current_stack_pointer & ~(THREAD_SIZE - 1));
 	irqsp = hardirq_ctx[raw_smp_processor_id()];
 	sirqsp = softirq_ctx[raw_smp_processor_id()];
 
-- 
2.21.1

v3: s/get_sp()/current_stack_pointer/

Re: [PATCH v3 1/5] powerpc: Rename current_stack_pointer() to current_stack_frame()

[Christophe Leroy]

Le 20/02/2020 à 12:51, Michael Ellerman a écrit :
> current_stack_pointer(), which was called __get_SP(), used to just
> return the value in r1.
> 
> But that caused problems in some cases, so it was turned into a
> function in commit bfe9a2cfe91a ("powerpc: Reimplement __get_SP() as a
> function not a define").
> 
> Because it's a function in a separate compilation unit to all its
> callers, it has the effect of causing a stack frame to be created, and
> then returns the address of that frame. This is good in some cases
> like those described in the above commit, but in other cases it's
> overkill, we just need to know what stack page we're on.
> 
> On some other arches current_stack_pointer is just a register global
> giving the stack pointer, and we'd like to do that too. So rename our
> current_stack_pointer() to current_stack_frame() to make that
> possible.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

LGTM

I was afraid to do that and risk invisible conflicts hence bugs by 
reusing the same name for different purpose, but that's the best 
solution for sure.

Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>

> ---
>   arch/powerpc/include/asm/perf_event.h | 2 +-
>   arch/powerpc/include/asm/reg.h        | 2 +-
>   arch/powerpc/kernel/irq.c             | 4 ++--
>   arch/powerpc/kernel/misc.S            | 4 ++--
>   arch/powerpc/kernel/process.c         | 2 +-
>   arch/powerpc/kernel/stacktrace.c      | 6 +++---
>   6 files changed, 10 insertions(+), 10 deletions(-)
> 
> v3: New.
> 
> diff --git a/arch/powerpc/include/asm/perf_event.h b/arch/powerpc/include/asm/perf_event.h
> index 7426d7a90e1e..eed3954082fa 100644
> --- a/arch/powerpc/include/asm/perf_event.h
> +++ b/arch/powerpc/include/asm/perf_event.h
> @@ -32,7 +32,7 @@
>   	do {							\
>   		(regs)->result = 0;				\
>   		(regs)->nip = __ip;				\
> -		(regs)->gpr[1] = current_stack_pointer();	\
> +		(regs)->gpr[1] = current_stack_frame();		\
>   		asm volatile("mfmsr %0" : "=r" ((regs)->msr));	\
>   	} while (0)
>   
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 1aa46dff0957..1b1ffdba6097 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -1448,7 +1448,7 @@ static inline void mtsrin(u32 val, u32 idx)
>   
>   #define proc_trap()	asm volatile("trap")
>   
> -extern unsigned long current_stack_pointer(void);
> +extern unsigned long current_stack_frame(void);
>   
>   extern unsigned long scom970_read(unsigned int address);
>   extern void scom970_write(unsigned int address, unsigned long value);
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index 5c9b11878555..02118c18434d 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -602,7 +602,7 @@ static inline void check_stack_overflow(void)
>   #ifdef CONFIG_DEBUG_STACKOVERFLOW
>   	long sp;
>   
> -	sp = current_stack_pointer() & (THREAD_SIZE-1);
> +	sp = current_stack_frame() & (THREAD_SIZE-1);
>   
>   	/* check for stack overflow: is there less than 2KB free? */
>   	if (unlikely(sp < 2048)) {
> @@ -647,7 +647,7 @@ void do_IRQ(struct pt_regs *regs)
>   	void *cursp, *irqsp, *sirqsp;
>   
>   	/* Switch to the irq stack to handle this */
> -	cursp = (void *)(current_stack_pointer() & ~(THREAD_SIZE - 1));
> +	cursp = (void *)(current_stack_frame() & ~(THREAD_SIZE - 1));
>   	irqsp = hardirq_ctx[raw_smp_processor_id()];
>   	sirqsp = softirq_ctx[raw_smp_processor_id()];
>   
> diff --git a/arch/powerpc/kernel/misc.S b/arch/powerpc/kernel/misc.S
> index 974f65f79a8e..65f9f731c229 100644
> --- a/arch/powerpc/kernel/misc.S
> +++ b/arch/powerpc/kernel/misc.S
> @@ -110,7 +110,7 @@ _GLOBAL(longjmp)
>   	li	r3, 1
>   	blr
>   
> -_GLOBAL(current_stack_pointer)
> +_GLOBAL(current_stack_frame)
>   	PPC_LL	r3,0(r1)
>   	blr
> -EXPORT_SYMBOL(current_stack_pointer)
> +EXPORT_SYMBOL(current_stack_frame)
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index e730b8e522b0..110db94cdf3c 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -2051,7 +2051,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
>   	sp = (unsigned long) stack;
>   	if (sp == 0) {
>   		if (tsk == current)
> -			sp = current_stack_pointer();
> +			sp = current_stack_frame();
>   		else
>   			sp = tsk->thread.ksp;
>   	}
> diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
> index e2a46cfed5fd..c477b8585a29 100644
> --- a/arch/powerpc/kernel/stacktrace.c
> +++ b/arch/powerpc/kernel/stacktrace.c
> @@ -57,7 +57,7 @@ void save_stack_trace(struct stack_trace *trace)
>   {
>   	unsigned long sp;
>   
> -	sp = current_stack_pointer();
> +	sp = current_stack_frame();
>   
>   	save_context_stack(trace, sp, current, 1);
>   }
> @@ -71,7 +71,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
>   		return;
>   
>   	if (tsk == current)
> -		sp = current_stack_pointer();
> +		sp = current_stack_frame();
>   	else
>   		sp = tsk->thread.ksp;
>   
> @@ -131,7 +131,7 @@ static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
>   	}
>   
>   	if (tsk == current)
> -		sp = current_stack_pointer();
> +		sp = current_stack_frame();
>   	else
>   		sp = tsk->thread.ksp;
>   
> 

Re: [PATCH v3 1/5] powerpc: Rename current_stack_pointer() to current_stack_frame()

[Michael Ellerman]

On Thu, 2020-02-20 at 11:51:37 UTC, Michael Ellerman wrote:
> current_stack_pointer(), which was called __get_SP(), used to just
> return the value in r1.
> 
> But that caused problems in some cases, so it was turned into a
> function in commit bfe9a2cfe91a ("powerpc: Reimplement __get_SP() as a
> function not a define").
> 
> Because it's a function in a separate compilation unit to all its
> callers, it has the effect of causing a stack frame to be created, and
> then returns the address of that frame. This is good in some cases
> like those described in the above commit, but in other cases it's
> overkill, we just need to know what stack page we're on.
> 
> On some other arches current_stack_pointer is just a register global
> giving the stack pointer, and we'd like to do that too. So rename our
> current_stack_pointer() to current_stack_frame() to make that
> possible.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Series applied to powerpc next.

https://git.kernel.org/powerpc/c/3d13e839e801e081bdece0127c2affa33d0f77cf

cheers