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

From: Palmer Dabbelt <palmer@rivosinc.com>
To: cleger@rivosinc.com, tglx@linutronix.de
Cc: shuah@kernel.org, krisman@collabora.com,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-riscv@lists.infradead.org
Subject: Re: [PATCH] selftests: sud_test: return correct emulated syscall value on RISC-V
Date: Thu, 09 Nov 2023 08:14:36 -0800 (PST)	[thread overview]
Message-ID: <mhng-9af5865a-9208-4308-9eb6-4ec07a4b4cb2@palmer-ri-x1c9a> (raw)
In-Reply-To: <ef980c24-6901-4bb3-8a1b-5902675e7851@rivosinc.com>

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