[PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V

[PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V

[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

Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V

[Palmer Dabbelt]

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?

Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V

[Clément Léger]

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

Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V

[Palmer Dabbelt]

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