* [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V @ 2023-09-13 14:07 Clément Léger 2023-11-09 3:26 ` Palmer Dabbelt 0 siblings, 1 reply; 4+ messages in thread From: Clément Léger @ 2023-09-13 14:07 UTC (permalink / raw) To: Shuah Khan, Gabriel Krisman Bertazi, linux-kselftest, linux-kernel, linux-riscv Cc: Clément Léger Currently, the sud_test expects the emulated syscall to return the emulated syscall number. This assumption only works on architectures were the syscall calling convention use the same register for syscall number/syscall return value. This is not the case for RISC-V and thus the return value must be also emulated using the provided ucontext. Signed-off-by: Clément Léger <cleger@rivosinc.com> --- tools/testing/selftests/syscall_user_dispatch/sud_test.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c b/tools/testing/selftests/syscall_user_dispatch/sud_test.c index b5d592d4099e..1b5553c19700 100644 --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c @@ -158,6 +158,14 @@ static void handle_sigsys(int sig, siginfo_t *info, void *ucontext) /* In preparation for sigreturn. */ SYSCALL_DISPATCH_OFF(glob_sel); + + /* + * Modify interrupted context returned value according to syscall + * calling convention + */ +#if defined(__riscv) + ((ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] = MAGIC_SYSCALL_1; +#endif } TEST(dispatch_and_return) -- 2.40.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V 2023-09-13 14:07 [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V Clément Léger @ 2023-11-09 3:26 ` Palmer Dabbelt 2023-11-09 8:22 ` Clément Léger 0 siblings, 1 reply; 4+ messages in thread From: Palmer Dabbelt @ 2023-11-09 3:26 UTC (permalink / raw) To: cleger; +Cc: shuah, krisman, linux-kselftest, linux-kernel, linux-riscv, cleger On Wed, 13 Sep 2023 07:07:11 PDT (-0700), cleger@rivosinc.com wrote: > Currently, the sud_test expects the emulated syscall to return the > emulated syscall number. This assumption only works on architectures > were the syscall calling convention use the same register for syscall > number/syscall return value. This is not the case for RISC-V and thus > the return value must be also emulated using the provided ucontext. > > Signed-off-by: Clément Léger <cleger@rivosinc.com> > --- > tools/testing/selftests/syscall_user_dispatch/sud_test.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c b/tools/testing/selftests/syscall_user_dispatch/sud_test.c > index b5d592d4099e..1b5553c19700 100644 > --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c > +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c > @@ -158,6 +158,14 @@ static void handle_sigsys(int sig, siginfo_t *info, void *ucontext) > > /* In preparation for sigreturn. */ > SYSCALL_DISPATCH_OFF(glob_sel); > + > + /* > + * Modify interrupted context returned value according to syscall > + * calling convention > + */ > +#if defined(__riscv) > + ((ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] = MAGIC_SYSCALL_1; > +#endif > } > > TEST(dispatch_and_return) I'm not sure if I'm just tired, but it took me a while to figure out why this was necessary. I think this is a better explanation: diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c b/tools/testing/selftests/syscall_user_dispatch/sud_test.c index b5d592d4099e..a913fd90cfa3 100644 --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c @@ -158,6 +158,16 @@ static void handle_sigsys(int sig, siginfo_t *info, void *ucontext) /* In preparation for sigreturn. */ SYSCALL_DISPATCH_OFF(glob_sel); + /* + * The tests for argument handling assume that `syscall(x) == x`. This + * is a NOP on x86 because the syscall number is passed in %rax, which + * happens to also be the function ABI return register. Other + * architectures may need to swizzle the arguments around. + */ +#if defined(__riscv) + (ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] = + (ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A7]; +#endif } TEST(dispatch_and_return) but also Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> as I agree this is correct. also: wouldn't arm64 also need to move x8 into x0 here, for essentially the same reason as we do? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V 2023-11-09 3:26 ` Palmer Dabbelt @ 2023-11-09 8:22 ` Clément Léger 2023-11-09 16:14 ` Palmer Dabbelt 0 siblings, 1 reply; 4+ messages in thread From: Clément Léger @ 2023-11-09 8:22 UTC (permalink / raw) To: Palmer Dabbelt; +Cc: shuah, krisman, linux-kselftest, linux-kernel, linux-riscv On 09/11/2023 04:26, Palmer Dabbelt wrote: > On Wed, 13 Sep 2023 07:07:11 PDT (-0700), cleger@rivosinc.com wrote: >> Currently, the sud_test expects the emulated syscall to return the >> emulated syscall number. This assumption only works on architectures >> were the syscall calling convention use the same register for syscall >> number/syscall return value. This is not the case for RISC-V and thus >> the return value must be also emulated using the provided ucontext. >> >> Signed-off-by: Clément Léger <cleger@rivosinc.com> >> --- >> tools/testing/selftests/syscall_user_dispatch/sud_test.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c >> b/tools/testing/selftests/syscall_user_dispatch/sud_test.c >> index b5d592d4099e..1b5553c19700 100644 >> --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c >> +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c >> @@ -158,6 +158,14 @@ static void handle_sigsys(int sig, siginfo_t >> *info, void *ucontext) >> >> /* In preparation for sigreturn. */ >> SYSCALL_DISPATCH_OFF(glob_sel); >> + >> + /* >> + * Modify interrupted context returned value according to syscall >> + * calling convention >> + */ >> +#if defined(__riscv) >> + ((ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] = >> MAGIC_SYSCALL_1; >> +#endif >> } >> >> TEST(dispatch_and_return) > > I'm not sure if I'm just tired, but it took me a while to figure out why > this was necessary. I think this is a better explanation: I think it's because this mechanism does not behave like other syscalls at all and the classic calling convention does not really apply... > > diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c > b/tools/testing/selftests/syscall_user_dispatch/sud_test.c > index b5d592d4099e..a913fd90cfa3 100644 > --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c > +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c > @@ -158,6 +158,16 @@ static void handle_sigsys(int sig, siginfo_t > *info, void *ucontext) > /* In preparation for sigreturn. */ > SYSCALL_DISPATCH_OFF(glob_sel); > + /* > + * The tests for argument handling assume that `syscall(x) == > x`. This > + * is a NOP on x86 because the syscall number is passed in %rax, > which > + * happens to also be the function ABI return register. Other > + * architectures may need to swizzle the arguments around. > + */ Indeed, that is more clear. Should I send a v2 ? > +#if defined(__riscv) > + (ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] = > + (ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A7]; > +#endif > } > TEST(dispatch_and_return) > > but also > > Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> > > as I agree this is correct. > > also: wouldn't arm64 also need to move x8 into x0 here, for essentially > the same reason as we do? Yes, as well as for a bunch of other architectures. I suspect this has only been tested on x86. AFAIK, this feature is mainly for wine usage which then makes sense for x86 and games. Thanks, Clément ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V 2023-11-09 8:22 ` Clément Léger @ 2023-11-09 16:14 ` Palmer Dabbelt 0 siblings, 0 replies; 4+ messages in thread From: Palmer Dabbelt @ 2023-11-09 16:14 UTC (permalink / raw) To: cleger, tglx; +Cc: shuah, krisman, linux-kselftest, linux-kernel, linux-riscv On Thu, 09 Nov 2023 00:22:46 PST (-0800), cleger@rivosinc.com wrote: > > > On 09/11/2023 04:26, Palmer Dabbelt wrote: >> On Wed, 13 Sep 2023 07:07:11 PDT (-0700), cleger@rivosinc.com wrote: >>> Currently, the sud_test expects the emulated syscall to return the >>> emulated syscall number. This assumption only works on architectures >>> were the syscall calling convention use the same register for syscall >>> number/syscall return value. This is not the case for RISC-V and thus >>> the return value must be also emulated using the provided ucontext. >>> >>> Signed-off-by: Clément Léger <cleger@rivosinc.com> >>> --- >>> tools/testing/selftests/syscall_user_dispatch/sud_test.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c >>> b/tools/testing/selftests/syscall_user_dispatch/sud_test.c >>> index b5d592d4099e..1b5553c19700 100644 >>> --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c >>> +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c >>> @@ -158,6 +158,14 @@ static void handle_sigsys(int sig, siginfo_t >>> *info, void *ucontext) >>> >>> /* In preparation for sigreturn. */ >>> SYSCALL_DISPATCH_OFF(glob_sel); >>> + >>> + /* >>> + * Modify interrupted context returned value according to syscall >>> + * calling convention >>> + */ >>> +#if defined(__riscv) >>> + ((ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] = >>> MAGIC_SYSCALL_1; >>> +#endif >>> } >>> >>> TEST(dispatch_and_return) >> >> I'm not sure if I'm just tired, but it took me a while to figure out why >> this was necessary. I think this is a better explanation: > > I think it's because this mechanism does not behave like other syscalls > at all and the classic calling convention does not really apply... Yep. I also got tripped up because I mis-read the docs and though SIGSYS was only for some error case (where it's actually for all the intercepted syscalls). >> diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c >> b/tools/testing/selftests/syscall_user_dispatch/sud_test.c >> index b5d592d4099e..a913fd90cfa3 100644 >> --- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c >> +++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c >> @@ -158,6 +158,16 @@ static void handle_sigsys(int sig, siginfo_t >> *info, void *ucontext) >> /* In preparation for sigreturn. */ >> SYSCALL_DISPATCH_OFF(glob_sel); >> + /* >> + * The tests for argument handling assume that `syscall(x) == >> x`. This >> + * is a NOP on x86 because the syscall number is passed in %rax, >> which >> + * happens to also be the function ABI return register. Other >> + * architectures may need to swizzle the arguments around. >> + */ > > Indeed, that is more clear. Should I send a v2 ? I would, but +Thomas as it looks like he's the one taking patches for this. > >> +#if defined(__riscv) >> + (ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A0] = >> + (ucontext_t*)ucontext)->uc_mcontext.__gregs[REG_A7]; >> +#endif >> } >> TEST(dispatch_and_return) >> >> but also >> >> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com> >> Acked-by: Palmer Dabbelt <palmer@rivosinc.com> >> >> as I agree this is correct. >> >> also: wouldn't arm64 also need to move x8 into x0 here, for essentially >> the same reason as we do? > > Yes, as well as for a bunch of other architectures. I suspect this has > only been tested on x86. AFAIK, this feature is mainly for wine usage > which then makes sense for x86 and games. Ya, makes sense -- I'd just looked at Arm to double-check my understanding here, as we usually don't find bugs in generic code before Arm does. > > Thanks, > > Clément ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-11-09 16:14 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-09-13 14:07 [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V Clément Léger 2023-11-09 3:26 ` Palmer Dabbelt 2023-11-09 8:22 ` Clément Léger 2023-11-09 16:14 ` Palmer Dabbelt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®