On Thu, 09 Nov 2023 00:22:46 PST (-0800), cleger@xxxxxxxxxxxx wrote:
On 09/11/2023 04:26, Palmer Dabbelt wrote:
On Wed, 13 Sep 2023 07:07:11 PDT (-0700), cleger@xxxxxxxxxxxx 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@xxxxxxxxxxxx>
---
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@xxxxxxxxxxxx>
Acked-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
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
