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

["Clément Léger" <cleger@rivosinc.com>]

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