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 <[email protected]>
---
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
Christophe Leroy <[email protected]> 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 <[email protected]>
> ---
> 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
Le 24/01/2020 à 06:46, Michael Ellerman a écrit :
> Christophe Leroy <[email protected]> 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 <[email protected]>
>> ---
>> 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
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
Hi!
On Fri, Jan 24, 2020 at 04:46:24PM +1100, Michael Ellerman wrote:
> Christophe Leroy <[email protected]> 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
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