* [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
@ 2019-12-09 6:07 ` Christophe Leroy
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Leroy @ 2019-12-09 6:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev
current_stack_pointer() 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.
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.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/irq.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index bb34005ff9d2..4d468d835558 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
static inline void check_stack_overflow(void)
{
#ifdef CONFIG_DEBUG_STACKOVERFLOW
- long sp;
-
- sp = current_stack_pointer() & (THREAD_SIZE-1);
+ register unsigned long r1 asm("r1");
+ long sp = r1 & (THREAD_SIZE - 1);
/* check for stack overflow: is there less than 2KB free? */
if (unlikely(sp < 2048)) {
--
2.13.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
@ 2019-12-09 6:07 ` Christophe Leroy
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Leroy @ 2019-12-09 6:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
current_stack_pointer() 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.
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.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/irq.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
index bb34005ff9d2..4d468d835558 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
static inline void check_stack_overflow(void)
{
#ifdef CONFIG_DEBUG_STACKOVERFLOW
- long sp;
-
- sp = current_stack_pointer() & (THREAD_SIZE-1);
+ register unsigned long r1 asm("r1");
+ long sp = r1 & (THREAD_SIZE - 1);
/* check for stack overflow: is there less than 2KB free? */
if (unlikely(sp < 2048)) {
--
2.13.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] powerpc/irq: use IS_ENABLED() in check_stack_overflow()
2019-12-09 6:07 ` Christophe Leroy
@ 2019-12-09 6:07 ` Christophe Leroy
-1 siblings, 0 replies; 13+ messages in thread
From: Christophe Leroy @ 2019-12-09 6:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linux-kernel, linuxppc-dev
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>
---
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 4d468d835558..0aebd7843c73 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -598,16 +598,17 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
static inline void check_stack_overflow(void)
{
-#ifdef CONFIG_DEBUG_STACKOVERFLOW
register unsigned long r1 asm("r1");
long sp = r1 & (THREAD_SIZE - 1);
+ if (!IS_ENABLED(CONFIG_DEBUG_STACKOVERFLOW))
+ return;
+
/* check for stack overflow: is there less than 2KB free? */
if (unlikely(sp < 2048)) {
pr_err("do_IRQ: stack overflow: %ld\n", sp);
dump_stack();
}
-#endif
}
#ifdef CONFIG_PPC32
--
2.13.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] powerpc/irq: use IS_ENABLED() in check_stack_overflow()
@ 2019-12-09 6:07 ` Christophe Leroy
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Leroy @ 2019-12-09 6:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
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>
---
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 4d468d835558..0aebd7843c73 100644
--- a/arch/powerpc/kernel/irq.c
+++ b/arch/powerpc/kernel/irq.c
@@ -598,16 +598,17 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
static inline void check_stack_overflow(void)
{
-#ifdef CONFIG_DEBUG_STACKOVERFLOW
register unsigned long r1 asm("r1");
long sp = r1 & (THREAD_SIZE - 1);
+ if (!IS_ENABLED(CONFIG_DEBUG_STACKOVERFLOW))
+ return;
+
/* check for stack overflow: is there less than 2KB free? */
if (unlikely(sp < 2048)) {
pr_err("do_IRQ: stack overflow: %ld\n", sp);
dump_stack();
}
-#endif
}
#ifdef CONFIG_PPC32
--
2.13.3
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
2019-12-09 6:07 ` Christophe Leroy
@ 2020-01-24 5:46 ` Michael Ellerman
-1 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2020-01-24 5:46 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-kernel, linuxppc-dev, Segher Boessenkool
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> current_stack_pointer() 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.
>
> 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.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/irq.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index bb34005ff9d2..4d468d835558 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
> static inline void check_stack_overflow(void)
> {
> #ifdef CONFIG_DEBUG_STACKOVERFLOW
> - long sp;
> -
> - sp = current_stack_pointer() & (THREAD_SIZE-1);
> + register unsigned long r1 asm("r1");
> + long sp = r1 & (THREAD_SIZE - 1);
This appears to work but seems to be "unsupported" by GCC, and clang
actually complains about it:
/linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
long sp = r1 & (THREAD_SIZE - 1);
^~
The GCC docs say:
The only supported use for this feature is to specify registers for
input and output operands when calling Extended asm (see Extended
Asm).
https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
If I do this it seems to work, but feels a little dicey:
asm ("" : "=r" (r1));
sp = r1 & (THREAD_SIZE - 1);
Generated code looks OK ish:
clang:
sp = r1 & (THREAD_SIZE - 1);
22e0: a0 04 24 78 clrldi r4,r1,50
if (unlikely(sp < 2048)) {
22e4: ff 07 24 28 cmpldi r4,2047
22e8: 58 00 81 40 ble 2340 <do_IRQ+0xe0>
gcc:
if (unlikely(sp < 2048)) {
2eb4: 00 38 28 70 andi. r8,r1,14336
...
2ecc: 24 00 82 40 bne c000000000002ef0 <do_IRQ+0xa0>
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
@ 2020-01-24 5:46 ` Michael Ellerman
0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2020-01-24 5:46 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> current_stack_pointer() 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.
>
> 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.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/kernel/irq.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index bb34005ff9d2..4d468d835558 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
> static inline void check_stack_overflow(void)
> {
> #ifdef CONFIG_DEBUG_STACKOVERFLOW
> - long sp;
> -
> - sp = current_stack_pointer() & (THREAD_SIZE-1);
> + register unsigned long r1 asm("r1");
> + long sp = r1 & (THREAD_SIZE - 1);
This appears to work but seems to be "unsupported" by GCC, and clang
actually complains about it:
/linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
long sp = r1 & (THREAD_SIZE - 1);
^~
The GCC docs say:
The only supported use for this feature is to specify registers for
input and output operands when calling Extended asm (see Extended
Asm).
https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
If I do this it seems to work, but feels a little dicey:
asm ("" : "=r" (r1));
sp = r1 & (THREAD_SIZE - 1);
Generated code looks OK ish:
clang:
sp = r1 & (THREAD_SIZE - 1);
22e0: a0 04 24 78 clrldi r4,r1,50
if (unlikely(sp < 2048)) {
22e4: ff 07 24 28 cmpldi r4,2047
22e8: 58 00 81 40 ble 2340 <do_IRQ+0xe0>
gcc:
if (unlikely(sp < 2048)) {
2eb4: 00 38 28 70 andi. r8,r1,14336
...
2ecc: 24 00 82 40 bne c000000000002ef0 <do_IRQ+0xa0>
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
2020-01-24 5:46 ` Michael Ellerman
@ 2020-01-24 6:19 ` Christophe Leroy
-1 siblings, 0 replies; 13+ messages in thread
From: Christophe Leroy @ 2020-01-24 6:19 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linux-kernel, linuxppc-dev, Segher Boessenkool
Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>
>> current_stack_pointer() 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.
>>
>> 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.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/kernel/irq.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index bb34005ff9d2..4d468d835558 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
>> static inline void check_stack_overflow(void)
>> {
>> #ifdef CONFIG_DEBUG_STACKOVERFLOW
>> - long sp;
>> -
>> - sp = current_stack_pointer() & (THREAD_SIZE-1);
>> + register unsigned long r1 asm("r1");
>> + long sp = r1 & (THREAD_SIZE - 1);
>
> This appears to work but seems to be "unsupported" by GCC, and clang
> actually complains about it:
>
> /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
> long sp = r1 & (THREAD_SIZE - 1);
> ^~
>
> The GCC docs say:
>
> The only supported use for this feature is to specify registers for
> input and output operands when calling Extended asm (see Extended
> Asm).
>
> https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
>
>
> If I do this it seems to work, but feels a little dicey:
>
> asm ("" : "=r" (r1));
> sp = r1 & (THREAD_SIZE - 1);
Or we could do add in asm/reg.h what we have in boot/reg.h:
register void *__stack_pointer asm("r1");
#define get_sp() (__stack_pointer)
And use get_sp()
I'll try it.
Christophe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
@ 2020-01-24 6:19 ` Christophe Leroy
0 siblings, 0 replies; 13+ messages in thread
From: Christophe Leroy @ 2020-01-24 6:19 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
>
>> current_stack_pointer() 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.
>>
>> 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.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> arch/powerpc/kernel/irq.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
>> index bb34005ff9d2..4d468d835558 100644
>> --- a/arch/powerpc/kernel/irq.c
>> +++ b/arch/powerpc/kernel/irq.c
>> @@ -599,9 +599,8 @@ u64 arch_irq_stat_cpu(unsigned int cpu)
>> static inline void check_stack_overflow(void)
>> {
>> #ifdef CONFIG_DEBUG_STACKOVERFLOW
>> - long sp;
>> -
>> - sp = current_stack_pointer() & (THREAD_SIZE-1);
>> + register unsigned long r1 asm("r1");
>> + long sp = r1 & (THREAD_SIZE - 1);
>
> This appears to work but seems to be "unsupported" by GCC, and clang
> actually complains about it:
>
> /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
> long sp = r1 & (THREAD_SIZE - 1);
> ^~
>
> The GCC docs say:
>
> The only supported use for this feature is to specify registers for
> input and output operands when calling Extended asm (see Extended
> Asm).
>
> https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
>
>
> If I do this it seems to work, but feels a little dicey:
>
> asm ("" : "=r" (r1));
> sp = r1 & (THREAD_SIZE - 1);
Or we could do add in asm/reg.h what we have in boot/reg.h:
register void *__stack_pointer asm("r1");
#define get_sp() (__stack_pointer)
And use get_sp()
I'll try it.
Christophe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
2020-01-24 6:19 ` Christophe Leroy
(?)
@ 2020-01-24 7:03 ` Christophe Leroy
2020-01-24 8:58 ` Segher Boessenkool
-1 siblings, 1 reply; 13+ messages in thread
From: Christophe Leroy @ 2020-01-24 7:03 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
On 01/24/2020 06:19 AM, Christophe Leroy wrote:
>
>
> Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
>>
>> If I do this it seems to work, but feels a little dicey:
>>
>> asm ("" : "=r" (r1));
>> sp = r1 & (THREAD_SIZE - 1);
>
>
> Or we could do add in asm/reg.h what we have in boot/reg.h:
>
> register void *__stack_pointer asm("r1");
> #define get_sp() (__stack_pointer)
>
> And use get_sp()
>
It works, and I guess doing it this way is acceptable as it's exactly
what's done for current in asm/current.h with register r2.
Now I (still) get:
sp = get_sp() & (THREAD_SIZE - 1);
b9c: 54 24 04 fe clrlwi r4,r1,19
if (unlikely(sp < 2048)) {
ba4: 2f 84 07 ff cmpwi cr7,r4,2047
Allthough GCC 8.1 what doing exactly the same with the form CLANG don't
like:
register unsigned long r1 asm("r1");
long sp = r1 & (THREAD_SIZE - 1);
b84: 54 24 04 fe clrlwi r4,r1,19
if (unlikely(sp < 2048)) {
b8c: 2f 84 07 ff cmpwi cr7,r4,2047
Christophe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
2020-01-24 5:46 ` Michael Ellerman
@ 2020-01-24 8:44 ` Segher Boessenkool
-1 siblings, 0 replies; 13+ messages in thread
From: Segher Boessenkool @ 2020-01-24 8:44 UTC (permalink / raw)
To: Michael Ellerman
Cc: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
linux-kernel, linuxppc-dev
Hi!
On Fri, Jan 24, 2020 at 04:46:24PM +1100, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
> > static inline void check_stack_overflow(void)
> > {
> > #ifdef CONFIG_DEBUG_STACKOVERFLOW
> > - long sp;
> > -
> > - sp = current_stack_pointer() & (THREAD_SIZE-1);
> > + register unsigned long r1 asm("r1");
> > + long sp = r1 & (THREAD_SIZE - 1);
>
> This appears to work but seems to be "unsupported" by GCC, and clang
> actually complains about it:
>
> /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
> long sp = r1 & (THREAD_SIZE - 1);
> ^~
>
> The GCC docs say:
>
> The only supported use for this feature is to specify registers for
> input and output operands when calling Extended asm (see Extended
> Asm).
>
> https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
Yes. Don't use local register variables any other way. It *will* break.
> If I do this it seems to work, but feels a little dicey:
>
> asm ("" : "=r" (r1));
> sp = r1 & (THREAD_SIZE - 1);
The only thing dicey about that is that you are writing to r1. Heh.
Well that certainly is bad enough, the compiler does not know how to
handle that at all... Of course you aren't *actually* changing
anything, so it might just work.
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
@ 2020-01-24 8:44 ` Segher Boessenkool
0 siblings, 0 replies; 13+ messages in thread
From: Segher Boessenkool @ 2020-01-24 8:44 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Paul Mackerras, linux-kernel
Hi!
On Fri, Jan 24, 2020 at 04:46:24PM +1100, Michael Ellerman wrote:
> Christophe Leroy <christophe.leroy@c-s.fr> writes:
> > static inline void check_stack_overflow(void)
> > {
> > #ifdef CONFIG_DEBUG_STACKOVERFLOW
> > - long sp;
> > -
> > - sp = current_stack_pointer() & (THREAD_SIZE-1);
> > + register unsigned long r1 asm("r1");
> > + long sp = r1 & (THREAD_SIZE - 1);
>
> This appears to work but seems to be "unsupported" by GCC, and clang
> actually complains about it:
>
> /linux/arch/powerpc/kernel/irq.c:603:12: error: variable 'r1' is uninitialized when used here [-Werror,-Wuninitialized]
> long sp = r1 & (THREAD_SIZE - 1);
> ^~
>
> The GCC docs say:
>
> The only supported use for this feature is to specify registers for
> input and output operands when calling Extended asm (see Extended
> Asm).
>
> https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Local-Register-Variables.html#Local-Register-Variables
Yes. Don't use local register variables any other way. It *will* break.
> If I do this it seems to work, but feels a little dicey:
>
> asm ("" : "=r" (r1));
> sp = r1 & (THREAD_SIZE - 1);
The only thing dicey about that is that you are writing to r1. Heh.
Well that certainly is bad enough, the compiler does not know how to
handle that at all... Of course you aren't *actually* changing
anything, so it might just work.
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
2020-01-24 7:03 ` Christophe Leroy
@ 2020-01-24 8:58 ` Segher Boessenkool
0 siblings, 0 replies; 13+ messages in thread
From: Segher Boessenkool @ 2020-01-24 8:58 UTC (permalink / raw)
To: Christophe Leroy
Cc: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras,
linuxppc-dev, linux-kernel
On Fri, Jan 24, 2020 at 07:03:36AM +0000, Christophe Leroy wrote:
> >Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> >>
> >>If I do this it seems to work, but feels a little dicey:
> >>
> >> asm ("" : "=r" (r1));
> >> sp = r1 & (THREAD_SIZE - 1);
> >
> >
> >Or we could do add in asm/reg.h what we have in boot/reg.h:
> >
> >register void *__stack_pointer asm("r1");
> >#define get_sp() (__stack_pointer)
> >
> >And use get_sp()
> >
>
> It works, and I guess doing it this way is acceptable as it's exactly
> what's done for current in asm/current.h with register r2.
That is a *global* register variable. That works. We still need to
document a bit better what it does exactly, but this is the expected
use case, so that will work.
> Now I (still) get:
>
> sp = get_sp() & (THREAD_SIZE - 1);
> b9c: 54 24 04 fe clrlwi r4,r1,19
> if (unlikely(sp < 2048)) {
> ba4: 2f 84 07 ff cmpwi cr7,r4,2047
>
> Allthough GCC 8.1 what doing exactly the same with the form CLANG don't
> like:
>
> register unsigned long r1 asm("r1");
> long sp = r1 & (THREAD_SIZE - 1);
> b84: 54 24 04 fe clrlwi r4,r1,19
> if (unlikely(sp < 2048)) {
> b8c: 2f 84 07 ff cmpwi cr7,r4,2047
Sure, if it did what you expected, things will usually work out fine ;-)
(Pity that the compiler didn't come up with
rlwinm. r4,r1,0,19,20
bne bad
Or are the low bits of r4 used later again?)
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow()
@ 2020-01-24 8:58 ` Segher Boessenkool
0 siblings, 0 replies; 13+ messages in thread
From: Segher Boessenkool @ 2020-01-24 8:58 UTC (permalink / raw)
To: Christophe Leroy; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
On Fri, Jan 24, 2020 at 07:03:36AM +0000, Christophe Leroy wrote:
> >Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> >>
> >>If I do this it seems to work, but feels a little dicey:
> >>
> >> asm ("" : "=r" (r1));
> >> sp = r1 & (THREAD_SIZE - 1);
> >
> >
> >Or we could do add in asm/reg.h what we have in boot/reg.h:
> >
> >register void *__stack_pointer asm("r1");
> >#define get_sp() (__stack_pointer)
> >
> >And use get_sp()
> >
>
> It works, and I guess doing it this way is acceptable as it's exactly
> what's done for current in asm/current.h with register r2.
That is a *global* register variable. That works. We still need to
document a bit better what it does exactly, but this is the expected
use case, so that will work.
> Now I (still) get:
>
> sp = get_sp() & (THREAD_SIZE - 1);
> b9c: 54 24 04 fe clrlwi r4,r1,19
> if (unlikely(sp < 2048)) {
> ba4: 2f 84 07 ff cmpwi cr7,r4,2047
>
> Allthough GCC 8.1 what doing exactly the same with the form CLANG don't
> like:
>
> register unsigned long r1 asm("r1");
> long sp = r1 & (THREAD_SIZE - 1);
> b84: 54 24 04 fe clrlwi r4,r1,19
> if (unlikely(sp < 2048)) {
> b8c: 2f 84 07 ff cmpwi cr7,r4,2047
Sure, if it did what you expected, things will usually work out fine ;-)
(Pity that the compiler didn't come up with
rlwinm. r4,r1,0,19,20
bne bad
Or are the low bits of r4 used later again?)
Segher
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-01-24 9:00 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-09 6:07 [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() in check_stack_overflow() Christophe Leroy
2019-12-09 6:07 ` Christophe Leroy
2019-12-09 6:07 ` [PATCH 2/2] powerpc/irq: use IS_ENABLED() " Christophe Leroy
2019-12-09 6:07 ` Christophe Leroy
2020-01-24 5:46 ` [PATCH 1/2] powerpc/irq: don't use current_stack_pointer() " Michael Ellerman
2020-01-24 5:46 ` Michael Ellerman
2020-01-24 6:19 ` Christophe Leroy
2020-01-24 6:19 ` Christophe Leroy
2020-01-24 7:03 ` Christophe Leroy
2020-01-24 8:58 ` Segher Boessenkool
2020-01-24 8:58 ` Segher Boessenkool
2020-01-24 8:44 ` Segher Boessenkool
2020-01-24 8:44 ` Segher Boessenkool
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.