| Message ID | 20230913140711.266975-1-cleger@rivosinc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: sud_test: return correct emulated syscall value on RISC-V | expand |
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?
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
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
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)
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(+)