[PATCH] arm64: syscall: unmask DAIF for tracing status

[Guo Hui]

On 6/6/23 9:22 PM, Mark Rutland wrote:

> On Fri, May 26, 2023 at 10:47:15AM +0800, Guo Hui wrote:
>> The following code:
>> static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
>>                              const syscall_fn_t syscall_table[])
>> {
>>      ...
>>
>>      if (!has_syscall_work(flags) && !IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
>>          local_daif_mask();
>>          flags = read_thread_flags(); -------------------------------- A
>>          if (!has_syscall_work(flags) && !(flags & _TIF_SINGLESTEP))
>>               return;
>>          local_daif_restore(DAIF_PROCCTX);
>>       }
>>
>> trace_exit:
>>       syscall_trace_exit(regs); ------- B
>> }
>>
>> 1. The flags in the if conditional statement should be used
>> in the same way as the flags in the function syscall_trace_exit,
>> because DAIF is not shielded in the function syscall_trace_exit,
>> so when the flags are obtained in line A of the code,
>> there is no need to shield DAIF.
>> Don't care about the modification of flags after line A.
>>
>> 2. Masking DAIF caused syscall performance to deteriorate by about 10%.
>> The Unixbench single core syscall performance data is as follows:
>>
>> Machine: Kunpeng 920
>>
>> Mask DAIF:
>> System Call Overhead                    1172314.1 lps   (10.0 s, 7 samples)
>>
>> System Benchmarks Partial Index          BASELINE       RESULT    INDEX
>> System Call Overhead                      15000.0    1172314.1    781.5
>>                                                                 ========
>> System Benchmarks Index Score (Partial Only)                      781.5
>>
>> Unmask DAIF:
>> System Call Overhead                    1287944.6 lps   (10.0 s, 7 samples)
>>
>> System Benchmarks Partial Index          BASELINE       RESULT    INDEX
>> System Call Overhead                      15000.0    1287944.6    858.6
>>                                                                 ========
>> System Benchmarks Index Score (Partial Only)                      858.6
>>
>> Signed-off-by: Guo Hui <guohui at uniontech.com>
> I was about to send a patch doing the same thing, so FWIW:
>
> Reviewed-by: Mark Rutland <mark.rutland at arm.com>
>
> As for why this is safe, rationale below.
>
> This masking is an artifact of the old "ret_fast_syscall" assembly that
> was converted to C in commit:
>
>    f37099b6992a0b81 ("arm64: convert syscall trace logic to C")
>
> The assembly would mask DAIF, check the thread flags, and fall through
> to kernel_exit without unmasking if no tracing was needed. The
> conversion copied this masking into the C version, though this wasn't
> strictly necessary.
>
> As (in general) thread flags can be manipulated by other threads, it's
> not safe to manipulate the thread flags with plain reads and writes, and
> since commit:
>
>    342b3808786518ce ("arm64: Snapshot thread flags")
>
> ... we use read_thread_flags() to read the flags atomically.
>
> With this, there is no need to mask DAIF transiently around reading the
> flags, as we only decide whether to trace while DAIF is masked, and the
> actual tracing occurs with DAIF unmasked. When el0_svc_common() returns
> its caller will unconditionally mask DAIF via exit_to_user_mode(), so
> the masking is redundant.
>
> Mark.
>
Ok thanks, Mark, you are right.

>> ---
>>   arch/arm64/kernel/syscall.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c
>> index da84cf855c44..5a668d7f3c1f 100644
>> --- a/arch/arm64/kernel/syscall.c
>> +++ b/arch/arm64/kernel/syscall.c
>> @@ -147,11 +147,9 @@ static void el0_svc_common(struct pt_regs *regs, int scno, int sc_nr,
>>   	 * exit regardless, as the old entry assembly did.
>>   	 */
>>   	if (!has_syscall_work(flags) && !IS_ENABLED(CONFIG_DEBUG_RSEQ)) {
>> -		local_daif_mask();
>>   		flags = read_thread_flags();
>>   		if (!has_syscall_work(flags) && !(flags & _TIF_SINGLESTEP))
>>   			return;
>> -		local_daif_restore(DAIF_PROCCTX);
>>   	}
>>   
>>   trace_exit:
>> -- 
>> 2.20.1
>>