* [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state @ 2021-09-01 16:54 Nicholas Piggin 2021-09-01 16:54 ` [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests Nicholas Piggin 2021-09-01 17:21 ` [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state Christophe Leroy 0 siblings, 2 replies; 11+ messages in thread From: Nicholas Piggin @ 2021-09-01 16:54 UTC (permalink / raw) To: linuxppc-dev; +Cc: Eirik Fuller, Nicholas Piggin If a system call is made with a transaction active, the kernel immediately aborts it and returns. scv system calls disable irqs even earlier in their interrupt handler, and tabort_syscall does not fix this up. This can result in irq soft-mask state being messed up on the next kernel entry, and crashing at BUG_ON(arch_irq_disabled_regs(regs)) in the kernel exit handlers, or possibly worse. Fix this by having tabort_syscall setting irq soft-mask back to enabled (which requires MSR[EE] be disabled first). Reported-by: Eirik Fuller <efuller@redhat.com> Fixes: 7fa95f9adaee7 ("powerpc/64s: system call support for scv/rfscv instructions") Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- Tested the wrong kernel before sending v1 and missed a bug, sorry. arch/powerpc/kernel/interrupt_64.S | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S index d4212d2ff0b5..9c31d65b4851 100644 --- a/arch/powerpc/kernel/interrupt_64.S +++ b/arch/powerpc/kernel/interrupt_64.S @@ -428,16 +428,22 @@ RESTART_TABLE(.Lsyscall_rst_start, .Lsyscall_rst_end, syscall_restart) #ifdef CONFIG_PPC_TRANSACTIONAL_MEM tabort_syscall: _ASM_NOKPROBE_SYMBOL(tabort_syscall) - /* Firstly we need to enable TM in the kernel */ + /* We need to enable TM in the kernel, and disable EE (for scv) */ mfmsr r10 li r9, 1 rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG + LOAD_REG_IMMEDIATE(r9, MSR_EE) + andc r10, r10, r9 mtmsrd r10, 0 /* tabort, this dooms the transaction, nothing else */ li r9, (TM_CAUSE_SYSCALL|TM_CAUSE_PERSISTENT) TABORT(R9) + /* scv has disabled irqs so must re-enable. sc just remains enabled */ + li r9,IRQS_ENABLED + stb r9,PACAIRQSOFTMASK(r13) + /* * Return directly to userspace. We have corrupted user register state, * but userspace will never see that register state. Execution will -- 2.23.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests 2021-09-01 16:54 [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state Nicholas Piggin @ 2021-09-01 16:54 ` Nicholas Piggin 2021-09-01 17:15 ` Christophe Leroy 2021-09-02 5:36 ` Michael Ellerman 2021-09-01 17:21 ` [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state Christophe Leroy 1 sibling, 2 replies; 11+ messages in thread From: Nicholas Piggin @ 2021-09-01 16:54 UTC (permalink / raw) To: linuxppc-dev; +Cc: Eirik Fuller, Nicholas Piggin The basic TM vs syscall test code hard codes an sc instruction for the system call, which fails to cover scv even when the userspace libc has support for it. Duplicate the tests with hard coded scv variants so both are tested when possible. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- .../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++ .../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++--- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S index bd1ca25febe4..849316831e6a 100644 --- a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S +++ b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S @@ -2,6 +2,10 @@ #include <ppc-asm.h> #include <asm/unistd.h> +/* ppc-asm.h does not define r0 or r1 */ +#define r0 0 +#define r1 1 + .text FUNC_START(getppid_tm_active) tbegin. @@ -26,3 +30,45 @@ FUNC_START(getppid_tm_suspended) 1: li r3, -1 blr + +FUNC_START(getppid_scv_tm_active) + mflr r0 + std r0,16(r1) + stdu r1,-32(r1) + tbegin. + beq 1f + li r0, __NR_getppid + scv 0 + tend. + addi r1,r1,32 + ld r0,16(r1) + mtlr r0 + blr +1: + li r3, -1 + addi r1,r1,32 + ld r0,16(r1) + mtlr r0 + blr + +FUNC_START(getppid_scv_tm_suspended) + mflr r0 + std r0,16(r1) + stdu r1,-32(r1) + tbegin. + beq 1f + li r0, __NR_getppid + tsuspend. + scv 0 + tresume. + tend. + addi r1,r1,32 + ld r0,16(r1) + mtlr r0 + blr +1: + li r3, -1 + addi r1,r1,32 + ld r0,16(r1) + mtlr r0 + blr diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c b/tools/testing/selftests/powerpc/tm/tm-syscall.c index becb8207b432..9a822208680e 100644 --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c +++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c @@ -19,24 +19,37 @@ #include "utils.h" #include "tm.h" +#ifndef PPC_FEATURE2_SCV +#define PPC_FEATURE2_SCV 0x00100000 /* scv syscall */ +#endif + extern int getppid_tm_active(void); extern int getppid_tm_suspended(void); +extern int getppid_scv_tm_active(void); +extern int getppid_scv_tm_suspended(void); unsigned retries = 0; #define TEST_DURATION 10 /* seconds */ #define TM_RETRIES 100 -pid_t getppid_tm(bool suspend) +pid_t getppid_tm(bool scv, bool suspend) { int i; pid_t pid; for (i = 0; i < TM_RETRIES; i++) { - if (suspend) - pid = getppid_tm_suspended(); - else - pid = getppid_tm_active(); + if (suspend) { + if (scv) + pid = getppid_scv_tm_suspended(); + else + pid = getppid_tm_suspended(); + } else { + if (scv) + pid = getppid_scv_tm_active(); + else + pid = getppid_tm_active(); + } if (pid >= 0) return pid; @@ -82,15 +95,24 @@ int tm_syscall(void) * Test a syscall within a suspended transaction and verify * that it succeeds. */ - FAIL_IF(getppid_tm(true) == -1); /* Should succeed. */ + FAIL_IF(getppid_tm(false, true) == -1); /* Should succeed. */ /* * Test a syscall within an active transaction and verify that * it fails with the correct failure code. */ - FAIL_IF(getppid_tm(false) != -1); /* Should fail... */ + FAIL_IF(getppid_tm(false, false) != -1); /* Should fail... */ FAIL_IF(!failure_is_persistent()); /* ...persistently... */ FAIL_IF(!failure_is_syscall()); /* ...with code syscall. */ + + /* Now do it all again with scv if it is available. */ + if (have_hwcap2(PPC_FEATURE2_SCV)) { + FAIL_IF(getppid_tm(true, true) == -1); /* Should succeed. */ + FAIL_IF(getppid_tm(true, false) != -1); /* Should fail... */ + FAIL_IF(!failure_is_persistent()); /* ...persistently... */ + FAIL_IF(!failure_is_syscall()); /* ...with code syscall. */ + } + gettimeofday(&now, 0); } -- 2.23.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests 2021-09-01 16:54 ` [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests Nicholas Piggin @ 2021-09-01 17:15 ` Christophe Leroy 2021-09-02 3:27 ` Nicholas Piggin 2021-09-02 5:36 ` Michael Ellerman 1 sibling, 1 reply; 11+ messages in thread From: Christophe Leroy @ 2021-09-01 17:15 UTC (permalink / raw) To: Nicholas Piggin, linuxppc-dev; +Cc: Eirik Fuller Le 01/09/2021 à 18:54, Nicholas Piggin a écrit : > The basic TM vs syscall test code hard codes an sc instruction for the > system call, which fails to cover scv even when the userspace libc has > support for it. > > Duplicate the tests with hard coded scv variants so both are tested > when possible. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > .../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++ > .../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++--- > 2 files changed, 75 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S > index bd1ca25febe4..849316831e6a 100644 > --- a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S > +++ b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S > @@ -2,6 +2,10 @@ > #include <ppc-asm.h> > #include <asm/unistd.h> > > +/* ppc-asm.h does not define r0 or r1 */ > +#define r0 0 > +#define r1 1 > + See https://github.com/gcc-mirror/gcc/blob/master/gcc/config/rs6000/ppc-asm.h It doesn't not define r1 but it defines r0. And it defines 'sp' as register 1. > .text > FUNC_START(getppid_tm_active) > tbegin. > @@ -26,3 +30,45 @@ FUNC_START(getppid_tm_suspended) > 1: > li r3, -1 > blr > + > +FUNC_START(getppid_scv_tm_active) > + mflr r0 > + std r0,16(r1) > + stdu r1,-32(r1) > + tbegin. > + beq 1f > + li r0, __NR_getppid > + scv 0 > + tend. > + addi r1,r1,32 > + ld r0,16(r1) > + mtlr r0 > + blr > +1: > + li r3, -1 > + addi r1,r1,32 > + ld r0,16(r1) > + mtlr r0 > + blr > + > +FUNC_START(getppid_scv_tm_suspended) > + mflr r0 > + std r0,16(r1) > + stdu r1,-32(r1) > + tbegin. > + beq 1f > + li r0, __NR_getppid > + tsuspend. > + scv 0 > + tresume. > + tend. > + addi r1,r1,32 > + ld r0,16(r1) > + mtlr r0 > + blr > +1: > + li r3, -1 > + addi r1,r1,32 > + ld r0,16(r1) > + mtlr r0 > + blr > diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c b/tools/testing/selftests/powerpc/tm/tm-syscall.c > index becb8207b432..9a822208680e 100644 > --- a/tools/testing/selftests/powerpc/tm/tm-syscall.c > +++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c > @@ -19,24 +19,37 @@ > #include "utils.h" > #include "tm.h" > > +#ifndef PPC_FEATURE2_SCV > +#define PPC_FEATURE2_SCV 0x00100000 /* scv syscall */ > +#endif > + > extern int getppid_tm_active(void); > extern int getppid_tm_suspended(void); > +extern int getppid_scv_tm_active(void); > +extern int getppid_scv_tm_suspended(void); > > unsigned retries = 0; > > #define TEST_DURATION 10 /* seconds */ > #define TM_RETRIES 100 > > -pid_t getppid_tm(bool suspend) > +pid_t getppid_tm(bool scv, bool suspend) > { > int i; > pid_t pid; > > for (i = 0; i < TM_RETRIES; i++) { > - if (suspend) > - pid = getppid_tm_suspended(); > - else > - pid = getppid_tm_active(); > + if (suspend) { > + if (scv) > + pid = getppid_scv_tm_suspended(); > + else > + pid = getppid_tm_suspended(); > + } else { > + if (scv) > + pid = getppid_scv_tm_active(); > + else > + pid = getppid_tm_active(); > + } > > if (pid >= 0) > return pid; > @@ -82,15 +95,24 @@ int tm_syscall(void) > * Test a syscall within a suspended transaction and verify > * that it succeeds. > */ > - FAIL_IF(getppid_tm(true) == -1); /* Should succeed. */ > + FAIL_IF(getppid_tm(false, true) == -1); /* Should succeed. */ > > /* > * Test a syscall within an active transaction and verify that > * it fails with the correct failure code. > */ > - FAIL_IF(getppid_tm(false) != -1); /* Should fail... */ > + FAIL_IF(getppid_tm(false, false) != -1); /* Should fail... */ > FAIL_IF(!failure_is_persistent()); /* ...persistently... */ > FAIL_IF(!failure_is_syscall()); /* ...with code syscall. */ > + > + /* Now do it all again with scv if it is available. */ > + if (have_hwcap2(PPC_FEATURE2_SCV)) { > + FAIL_IF(getppid_tm(true, true) == -1); /* Should succeed. */ > + FAIL_IF(getppid_tm(true, false) != -1); /* Should fail... */ > + FAIL_IF(!failure_is_persistent()); /* ...persistently... */ > + FAIL_IF(!failure_is_syscall()); /* ...with code syscall. */ > + } > + > gettimeofday(&now, 0); > } > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests 2021-09-01 17:15 ` Christophe Leroy @ 2021-09-02 3:27 ` Nicholas Piggin 2021-09-02 3:44 ` Michael Ellerman 0 siblings, 1 reply; 11+ messages in thread From: Nicholas Piggin @ 2021-09-02 3:27 UTC (permalink / raw) To: Christophe Leroy, linuxppc-dev; +Cc: Eirik Fuller Excerpts from Christophe Leroy's message of September 2, 2021 3:15 am: > > > Le 01/09/2021 à 18:54, Nicholas Piggin a écrit : >> The basic TM vs syscall test code hard codes an sc instruction for the >> system call, which fails to cover scv even when the userspace libc has >> support for it. >> >> Duplicate the tests with hard coded scv variants so both are tested >> when possible. >> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> .../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++ >> .../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++--- >> 2 files changed, 75 insertions(+), 7 deletions(-) >> >> diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S >> index bd1ca25febe4..849316831e6a 100644 >> --- a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S >> +++ b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S >> @@ -2,6 +2,10 @@ >> #include <ppc-asm.h> >> #include <asm/unistd.h> >> >> +/* ppc-asm.h does not define r0 or r1 */ >> +#define r0 0 >> +#define r1 1 >> + > > See https://github.com/gcc-mirror/gcc/blob/master/gcc/config/rs6000/ppc-asm.h > > It doesn't not define r1 but it defines r0. Oops, I'll fix that. > And it defines 'sp' as register 1. Does userspace code typically use that? Kernel code AFAIKS does not. Thanks, Nick ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests 2021-09-02 3:27 ` Nicholas Piggin @ 2021-09-02 3:44 ` Michael Ellerman 0 siblings, 0 replies; 11+ messages in thread From: Michael Ellerman @ 2021-09-02 3:44 UTC (permalink / raw) To: Nicholas Piggin, Christophe Leroy, linuxppc-dev; +Cc: Eirik Fuller Nicholas Piggin <npiggin@gmail.com> writes: > Excerpts from Christophe Leroy's message of September 2, 2021 3:15 am: >> Le 01/09/2021 à 18:54, Nicholas Piggin a écrit : >>> The basic TM vs syscall test code hard codes an sc instruction for the >>> system call, which fails to cover scv even when the userspace libc has >>> support for it. >>> >>> Duplicate the tests with hard coded scv variants so both are tested >>> when possible. >>> >>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >>> --- >>> .../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++ >>> .../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++--- >>> 2 files changed, 75 insertions(+), 7 deletions(-) >>> >>> diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S >>> index bd1ca25febe4..849316831e6a 100644 >>> --- a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S >>> +++ b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S >>> @@ -2,6 +2,10 @@ >>> #include <ppc-asm.h> >>> #include <asm/unistd.h> >>> >>> +/* ppc-asm.h does not define r0 or r1 */ >>> +#define r0 0 >>> +#define r1 1 >>> + >> >> See https://github.com/gcc-mirror/gcc/blob/master/gcc/config/rs6000/ppc-asm.h >> >> It doesn't not define r1 but it defines r0. > > Oops, I'll fix that. > >> And it defines 'sp' as register 1. > > Does userspace code typically use that? Kernel code AFAIKS does not. Some does, but it's not used consistently IME. I'd prefer you just use %r1. cheers ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests 2021-09-01 16:54 ` [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests Nicholas Piggin 2021-09-01 17:15 ` Christophe Leroy @ 2021-09-02 5:36 ` Michael Ellerman 1 sibling, 0 replies; 11+ messages in thread From: Michael Ellerman @ 2021-09-02 5:36 UTC (permalink / raw) To: Nicholas Piggin, linuxppc-dev; +Cc: Eirik Fuller, Nicholas Piggin Nicholas Piggin <npiggin@gmail.com> writes: > The basic TM vs syscall test code hard codes an sc instruction for the > system call, which fails to cover scv even when the userspace libc has > support for it. > > Duplicate the tests with hard coded scv variants so both are tested > when possible. > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > .../selftests/powerpc/tm/tm-syscall-asm.S | 46 +++++++++++++++++++ > .../testing/selftests/powerpc/tm/tm-syscall.c | 36 ++++++++++++--- > 2 files changed, 75 insertions(+), 7 deletions(-) > > diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S > index bd1ca25febe4..849316831e6a 100644 > --- a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S > +++ b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S > @@ -2,6 +2,10 @@ > #include <ppc-asm.h> > #include <asm/unistd.h> > > +/* ppc-asm.h does not define r0 or r1 */ > +#define r0 0 > +#define r1 1 > + > .text > FUNC_START(getppid_tm_active) > tbegin. > @@ -26,3 +30,45 @@ FUNC_START(getppid_tm_suspended) > 1: > li r3, -1 > blr > + > +FUNC_START(getppid_scv_tm_active) > + mflr r0 > + std r0,16(r1) > + stdu r1,-32(r1) > + tbegin. > + beq 1f > + li r0, __NR_getppid > + scv 0 > + tend. > + addi r1,r1,32 > + ld r0,16(r1) > + mtlr r0 > + blr > +1: > + li r3, -1 > + addi r1,r1,32 > + ld r0,16(r1) > + mtlr r0 > + blr There's some macros in tools/testing/selftests/powerpc/include/basic_asm.h that can do some of this boiler plate stack setup/teardown. Incremental diff below to use them, only build tested. cheers diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S index 849316831e6a..a73694daca71 100644 --- a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S +++ b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S @@ -1,10 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0 */ -#include <ppc-asm.h> -#include <asm/unistd.h> - -/* ppc-asm.h does not define r0 or r1 */ -#define r0 0 -#define r1 1 +#include <basic_asm.h> .text FUNC_START(getppid_tm_active) @@ -32,29 +27,21 @@ FUNC_START(getppid_tm_suspended) blr FUNC_START(getppid_scv_tm_active) - mflr r0 - std r0,16(r1) - stdu r1,-32(r1) + PUSH_BASIC_STACK(0) tbegin. beq 1f li r0, __NR_getppid scv 0 tend. - addi r1,r1,32 - ld r0,16(r1) - mtlr r0 + POP_BASIC_STACK(0) blr 1: li r3, -1 - addi r1,r1,32 - ld r0,16(r1) - mtlr r0 + POP_BASIC_STACK(0) blr FUNC_START(getppid_scv_tm_suspended) - mflr r0 - std r0,16(r1) - stdu r1,-32(r1) + PUSH_BASIC_STACK(0) tbegin. beq 1f li r0, __NR_getppid @@ -62,13 +49,9 @@ FUNC_START(getppid_scv_tm_suspended) scv 0 tresume. tend. - addi r1,r1,32 - ld r0,16(r1) - mtlr r0 + POP_BASIC_STACK(0) blr 1: li r3, -1 - addi r1,r1,32 - ld r0,16(r1) - mtlr r0 + POP_BASIC_STACK(0) blr ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state 2021-09-01 16:54 [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state Nicholas Piggin 2021-09-01 16:54 ` [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests Nicholas Piggin @ 2021-09-01 17:21 ` Christophe Leroy 2021-09-02 3:33 ` Nicholas Piggin 1 sibling, 1 reply; 11+ messages in thread From: Christophe Leroy @ 2021-09-01 17:21 UTC (permalink / raw) To: Nicholas Piggin, linuxppc-dev; +Cc: Eirik Fuller Le 01/09/2021 à 18:54, Nicholas Piggin a écrit : > If a system call is made with a transaction active, the kernel > immediately aborts it and returns. scv system calls disable irqs even > earlier in their interrupt handler, and tabort_syscall does not fix this > up. > > This can result in irq soft-mask state being messed up on the next > kernel entry, and crashing at BUG_ON(arch_irq_disabled_regs(regs)) in > the kernel exit handlers, or possibly worse. > > Fix this by having tabort_syscall setting irq soft-mask back to enabled > (which requires MSR[EE] be disabled first). > > Reported-by: Eirik Fuller <efuller@redhat.com> > Fixes: 7fa95f9adaee7 ("powerpc/64s: system call support for scv/rfscv instructions") > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > > Tested the wrong kernel before sending v1 and missed a bug, sorry. > > arch/powerpc/kernel/interrupt_64.S | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S > index d4212d2ff0b5..9c31d65b4851 100644 > --- a/arch/powerpc/kernel/interrupt_64.S > +++ b/arch/powerpc/kernel/interrupt_64.S > @@ -428,16 +428,22 @@ RESTART_TABLE(.Lsyscall_rst_start, .Lsyscall_rst_end, syscall_restart) > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > tabort_syscall: > _ASM_NOKPROBE_SYMBOL(tabort_syscall) > - /* Firstly we need to enable TM in the kernel */ > + /* We need to enable TM in the kernel, and disable EE (for scv) */ > mfmsr r10 > li r9, 1 > rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG > + LOAD_REG_IMMEDIATE(r9, MSR_EE) > + andc r10, r10, r9 Why not use 'rlwinm' to mask out MSR_EE ? Something like rlwinm r10, r10, 0, ~MSR_EE > mtmsrd r10, 0 > > /* tabort, this dooms the transaction, nothing else */ > li r9, (TM_CAUSE_SYSCALL|TM_CAUSE_PERSISTENT) > TABORT(R9) > > + /* scv has disabled irqs so must re-enable. sc just remains enabled */ > + li r9,IRQS_ENABLED > + stb r9,PACAIRQSOFTMASK(r13) > + > /* > * Return directly to userspace. We have corrupted user register state, > * but userspace will never see that register state. Execution will > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state 2021-09-01 17:21 ` [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state Christophe Leroy @ 2021-09-02 3:33 ` Nicholas Piggin 2021-09-02 21:52 ` Segher Boessenkool 0 siblings, 1 reply; 11+ messages in thread From: Nicholas Piggin @ 2021-09-02 3:33 UTC (permalink / raw) To: Christophe Leroy, linuxppc-dev; +Cc: Eirik Fuller Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am: > > > Le 01/09/2021 à 18:54, Nicholas Piggin a écrit : >> If a system call is made with a transaction active, the kernel >> immediately aborts it and returns. scv system calls disable irqs even >> earlier in their interrupt handler, and tabort_syscall does not fix this >> up. >> >> This can result in irq soft-mask state being messed up on the next >> kernel entry, and crashing at BUG_ON(arch_irq_disabled_regs(regs)) in >> the kernel exit handlers, or possibly worse. >> >> Fix this by having tabort_syscall setting irq soft-mask back to enabled >> (which requires MSR[EE] be disabled first). >> >> Reported-by: Eirik Fuller <efuller@redhat.com> >> Fixes: 7fa95f9adaee7 ("powerpc/64s: system call support for scv/rfscv instructions") >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- >> >> Tested the wrong kernel before sending v1 and missed a bug, sorry. >> >> arch/powerpc/kernel/interrupt_64.S | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S >> index d4212d2ff0b5..9c31d65b4851 100644 >> --- a/arch/powerpc/kernel/interrupt_64.S >> +++ b/arch/powerpc/kernel/interrupt_64.S >> @@ -428,16 +428,22 @@ RESTART_TABLE(.Lsyscall_rst_start, .Lsyscall_rst_end, syscall_restart) >> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM >> tabort_syscall: >> _ASM_NOKPROBE_SYMBOL(tabort_syscall) >> - /* Firstly we need to enable TM in the kernel */ >> + /* We need to enable TM in the kernel, and disable EE (for scv) */ >> mfmsr r10 >> li r9, 1 >> rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG >> + LOAD_REG_IMMEDIATE(r9, MSR_EE) >> + andc r10, r10, r9 > > Why not use 'rlwinm' to mask out MSR_EE ? > > Something like > > rlwinm r10, r10, 0, ~MSR_EE Mainly because I'm bad at powerpc assembly. Why do you think I'm trying to change as much as possible to C? Actually there should really be no need for mfmsr either, I wanted to rewrite the thing entirely as ld r10,PACAKMSR(r13) LOAD_REG_IMMEDIATE(r9, MSR_TM) or r10,r10,r9 mtmsrd r10 But I thought that's not a minimal bug fix. Thanks, Nick > >> mtmsrd r10, 0 >> >> /* tabort, this dooms the transaction, nothing else */ >> li r9, (TM_CAUSE_SYSCALL|TM_CAUSE_PERSISTENT) >> TABORT(R9) >> >> + /* scv has disabled irqs so must re-enable. sc just remains enabled */ >> + li r9,IRQS_ENABLED >> + stb r9,PACAIRQSOFTMASK(r13) >> + >> /* >> * Return directly to userspace. We have corrupted user register state, >> * but userspace will never see that register state. Execution will >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state 2021-09-02 3:33 ` Nicholas Piggin @ 2021-09-02 21:52 ` Segher Boessenkool 2021-09-02 22:20 ` Segher Boessenkool 0 siblings, 1 reply; 11+ messages in thread From: Segher Boessenkool @ 2021-09-02 21:52 UTC (permalink / raw) To: Nicholas Piggin; +Cc: Eirik Fuller, linuxppc-dev On Thu, Sep 02, 2021 at 01:33:10PM +1000, Nicholas Piggin wrote: > Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am: > >> - /* Firstly we need to enable TM in the kernel */ > >> + /* We need to enable TM in the kernel, and disable EE (for scv) */ > >> mfmsr r10 > >> li r9, 1 > >> rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG > >> + LOAD_REG_IMMEDIATE(r9, MSR_EE) > >> + andc r10, r10, r9 > > > > Why not use 'rlwinm' to mask out MSR_EE ? > > > > Something like > > > > rlwinm r10, r10, 0, ~MSR_EE > > Mainly because I'm bad at powerpc assembly. Why do you think I'm trying > to change as much as possible to C? The actual bit (bit 31, i.e. with value 1UL << 32) cannot be cleared with rlwinm (only the low 32 bits can). There are many ways to do it using two insns of course. > Actually there should really be no need for mfmsr either, I wanted to > rewrite the thing entirely as > > ld r10,PACAKMSR(r13) > LOAD_REG_IMMEDIATE(r9, MSR_TM) > or r10,r10,r9 > mtmsrd r10 > > But I thought that's not a minimal bug fix. That (LOAD_REG_IMMEDIATE+or) can be done with just two insns, not three as written: li r0,1 rldimi r10,r0,32,31 (you can write that last insns as rldimi r10,r0,MSR_TM_LG,MSR_TM_LG-1 if you insist :-) ) It isn't like a few integer computational insns will kill you here of course, there are much more important cycles to shave off :-) Segher ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state 2021-09-02 21:52 ` Segher Boessenkool @ 2021-09-02 22:20 ` Segher Boessenkool 2021-09-03 5:04 ` Christophe Leroy 0 siblings, 1 reply; 11+ messages in thread From: Segher Boessenkool @ 2021-09-02 22:20 UTC (permalink / raw) To: Nicholas Piggin; +Cc: Eirik Fuller, linuxppc-dev On Thu, Sep 02, 2021 at 04:52:03PM -0500, Segher Boessenkool wrote: > On Thu, Sep 02, 2021 at 01:33:10PM +1000, Nicholas Piggin wrote: > > Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am: > > >> - /* Firstly we need to enable TM in the kernel */ > > >> + /* We need to enable TM in the kernel, and disable EE (for scv) */ > > >> mfmsr r10 > > >> li r9, 1 > > >> rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG > > >> + LOAD_REG_IMMEDIATE(r9, MSR_EE) > > >> + andc r10, r10, r9 > > > > > > Why not use 'rlwinm' to mask out MSR_EE ? > > > > > > Something like > > > > > > rlwinm r10, r10, 0, ~MSR_EE > > > > Mainly because I'm bad at powerpc assembly. Why do you think I'm trying > > to change as much as possible to C? > > The actual bit (bit 31, i.e. with value 1UL << 32) cannot be cleared > with rlwinm (only the low 32 bits can). There are many ways to do it > using two insns of course. Wow I misread that, you want to clear MSR[EE] really, not MSR[TM]. You cannot use rlwinm and keep the high 32 bits of the target register intact. You either clear all to 0 or set them to a copy of the rotated value in the low 32 bits. Segher ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state 2021-09-02 22:20 ` Segher Boessenkool @ 2021-09-03 5:04 ` Christophe Leroy 0 siblings, 0 replies; 11+ messages in thread From: Christophe Leroy @ 2021-09-03 5:04 UTC (permalink / raw) To: Segher Boessenkool, Nicholas Piggin; +Cc: Eirik Fuller, linuxppc-dev Le 03/09/2021 à 00:20, Segher Boessenkool a écrit : > On Thu, Sep 02, 2021 at 04:52:03PM -0500, Segher Boessenkool wrote: >> On Thu, Sep 02, 2021 at 01:33:10PM +1000, Nicholas Piggin wrote: >>> Excerpts from Christophe Leroy's message of September 2, 2021 3:21 am: >>>>> - /* Firstly we need to enable TM in the kernel */ >>>>> + /* We need to enable TM in the kernel, and disable EE (for scv) */ >>>>> mfmsr r10 >>>>> li r9, 1 >>>>> rldimi r10, r9, MSR_TM_LG, 63-MSR_TM_LG >>>>> + LOAD_REG_IMMEDIATE(r9, MSR_EE) >>>>> + andc r10, r10, r9 >>>> >>>> Why not use 'rlwinm' to mask out MSR_EE ? >>>> >>>> Something like >>>> >>>> rlwinm r10, r10, 0, ~MSR_EE >>> >>> Mainly because I'm bad at powerpc assembly. Why do you think I'm trying >>> to change as much as possible to C? >> >> The actual bit (bit 31, i.e. with value 1UL << 32) cannot be cleared >> with rlwinm (only the low 32 bits can). There are many ways to do it >> using two insns of course. > > Wow I misread that, you want to clear MSR[EE] really, not MSR[TM]. > > You cannot use rlwinm and keep the high 32 bits of the target register > intact. You either clear all to 0 or set them to a copy of the rotated > value in the low 32 bits. > Oops, my mistake. When I tested it in C to see what was generated by GCC I forgot the ~ so I got rlwinm r3,r3,0,16,16 and didn't realise it was different from rlwinm r3,r3,0,~(1<<15) By the way it would be more explicit if objdump could display the mask instead of the mask boundaries. Is there a way to do that ? Christophe ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-09-03 5:05 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-01 16:54 [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state Nicholas Piggin 2021-09-01 16:54 ` [PATCH v2 2/2] selftests/powerpc: Add scv versions of the basic TM syscall tests Nicholas Piggin 2021-09-01 17:15 ` Christophe Leroy 2021-09-02 3:27 ` Nicholas Piggin 2021-09-02 3:44 ` Michael Ellerman 2021-09-02 5:36 ` Michael Ellerman 2021-09-01 17:21 ` [PATCH v2 1/2] powerpc/64s: system call scv tabort fix for corrupt irq soft-mask state Christophe Leroy 2021-09-02 3:33 ` Nicholas Piggin 2021-09-02 21:52 ` Segher Boessenkool 2021-09-02 22:20 ` Segher Boessenkool 2021-09-03 5:04 ` Christophe Leroy
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.