Re: [PATCH] ARM: Wire up HAVE_SYSCALL_TRACEPOINTS

[Takuo Koguchi <takuo.koguchi.sw@hitachi.com>]

Hello,
I am glad to start getting responses.
(2012/02/01 10:47), Indan Zupancic wrote:
> Hello,
>
> CC'ing Roland.
>
> On Thu, December 1, 2011 12:01, takuo.koguchi.sw@hitachi.com wrote:
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 44789ef..84181b3 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -13,6 +13,7 @@ config ARM
>>   	select HAVE_KPROBES if !XIP_KERNEL
>>   	select HAVE_KRETPROBES if (HAVE_KPROBES)
>>   	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
>> +	select HAVE_SYSCALL_TRACEPOINTS
> Your patch totally ignores OABI, it should at least depend on CONFIG_AEABI
> and on !CONFIG_OABI_COMPAT.
Right.  As Russel King suggested, this patch depends on those configs 
until very large NR_syscalls is properly handled by ftrace.

>>   	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
>>   	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL)
>>   	select HAVE_FUNCTION_GRAPH_TRACER if (!THUMB2_KERNEL)
>> diff --git a/arch/arm/include/asm/syscall.h b/arch/arm/include/asm/syscall.h
>> new file mode 100644
>> index 0000000..cabeb67
>> --- /dev/null
>> +++ b/arch/arm/include/asm/syscall.h
>> @@ -0,0 +1,45 @@
>> +#ifndef _ASM_ARM_SYSCALL_H
>> +#define _ASM_ARM_SYSCALL_H
>> +
>> +extern const unsigned long sys_call_table[];
>> +
>> +#include<linux/sched.h>
>> +
>> +static inline long syscall_get_nr(struct task_struct *task,
>> +				  struct pt_regs *regs)
>> +{
>> +	return regs->ARM_r7;
>> +}
>> +
>> +static inline long syscall_get_return_value(struct task_struct *task,
>> +					    struct pt_regs *regs)
>> +{
>> +	return regs->ARM_r0;
>> +}
>> +
>> +static inline void syscall_get_arguments(struct task_struct *task,
>> +					 struct pt_regs *regs,
>> +					 unsigned int i, unsigned int n,
>> +					 unsigned long *args)
>> +{
>> +	BUG_ON(i);
> This is unacceptable, that would make this function useless.
>
> See Rolands patch:
>
> https://lkml.org/lkml/2009/4/24/399 for a better implementation.
Thanks.  My apology for the lack of investigation.

>> +#endif	/* _ASM_ARM_SYSCALL_H */
>> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
>> index 7b5cc8d..2509028 100644
>> --- a/arch/arm/include/asm/thread_info.h
>> +++ b/arch/arm/include/asm/thread_info.h
>> @@ -139,6 +139,7 @@ extern void vfp_flush_hwstate(struct thread_info *);
>>   #define TIF_NEED_RESCHED	1
>>   #define TIF_NOTIFY_RESUME	2	/* callback before returning to user */
>>   #define TIF_SYSCALL_TRACE	8
>> +#define TIF_SYSCALL_TRACEPOINT	15
>>   #define TIF_POLLING_NRFLAG	16
>>   #define TIF_USING_IWMMXT	17
>>   #define TIF_MEMDIE		18	/* is terminating due to OOM killer */
>> @@ -150,11 +151,13 @@ extern void vfp_flush_hwstate(struct thread_info *);
>>   #define _TIF_NEED_RESCHED	(1<<  TIF_NEED_RESCHED)
>>   #define _TIF_NOTIFY_RESUME	(1<<  TIF_NOTIFY_RESUME)
>>   #define _TIF_SYSCALL_TRACE	(1<<  TIF_SYSCALL_TRACE)
>> +#define _TIF_SYSCALL_TRACEPOINT	(1<<  TIF_SYSCALL_TRACEPOINT)
>>   #define _TIF_POLLING_NRFLAG	(1<<  TIF_POLLING_NRFLAG)
>>   #define _TIF_USING_IWMMXT	(1<<  TIF_USING_IWMMXT)
>>   #define _TIF_FREEZE		(1<<  TIF_FREEZE)
>>   #define _TIF_RESTORE_SIGMASK	(1<<  TIF_RESTORE_SIGMASK)
>>   #define _TIF_SECCOMP		(1<<  TIF_SECCOMP)
>> +#define _TIF_SYSCALL_T_OR_A	(_TIF_SYSCALL_TRACE | _TIF_SYSCALL_TRACEPOINT)
> What does 'A' stand for?
'A' originally stands for AUDIT.  I should have used a better name as 
there is no _TIF_SYSCALL_AUDIT for ARM.
Probably it is better to define _TIF_WORK_SYSCALL_ENTRY and 
_TIF_WORK_SYSCALL_EXIT properly.

>>   /*
>>    * Change these and you break ASM code in entry-common.S
>> diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
>> index 4a11237..f4eac2d 100644
>> --- a/arch/arm/include/asm/unistd.h
>> +++ b/arch/arm/include/asm/unistd.h
>> @@ -405,6 +405,9 @@
>>   #define __NR_process_vm_readv		(__NR_SYSCALL_BASE+376)
>>   #define __NR_process_vm_writev		(__NR_SYSCALL_BASE+377)
>>
>> +#ifndef __ASSEMBLY__
>> +#define NR_syscalls		378
>> +#endif
>>   /*
>>    * The following SWIs are ARM private.
>>    */
>> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
>> index b2a27b6..a1577c2 100644
>> --- a/arch/arm/kernel/entry-common.S
>> +++ b/arch/arm/kernel/entry-common.S
>> @@ -87,7 +87,7 @@ ENTRY(ret_from_fork)
>>   	get_thread_info tsk
>>   	ldr	r1, [tsk, #TI_FLAGS]		@ check for syscall tracing
>>   	mov	why, #1
>> -	tst	r1, #_TIF_SYSCALL_TRACE		@ are we tracing syscalls?
>> +	tst	r1, #_TIF_SYSCALL_T_OR_A	@ are we tracing syscalls?
>>   	beq	ret_slow_syscall
>>   	mov	r1, sp
>>   	mov	r0, #1				@ trace exit [IP = 1]
>> @@ -443,7 +443,7 @@ ENTRY(vector_swi)
>>   1:
>>   #endif
>>
>> -	tst	r10, #_TIF_SYSCALL_TRACE		@ are we tracing syscalls?
>> +	tst	r10, #_TIF_SYSCALL_T_OR_A	@ are we tracing syscalls?
>>   	bne	__sys_trace
>>
>>   	cmp	scno, #NR_syscalls		@ check upper syscall limit
>> diff --git a/arch/arm/kernel/ptrace.c b/arch/arm/kernel/ptrace.c
>> index 483727a..a690c9f 100644
>> --- a/arch/arm/kernel/ptrace.c
>> +++ b/arch/arm/kernel/ptrace.c
>> @@ -28,6 +28,9 @@
>>   #include<asm/system.h>
>>   #include<asm/traps.h>
>>
>> +#define CREATE_TRACE_POINTS
>> +#include<trace/events/syscalls.h>
>> +
>>   #define REG_PC	15
>>   #define REG_PSR	16
>>   /*
>> @@ -906,6 +909,13 @@ asmlinkage int syscall_trace(int why, struct pt_regs *regs, int
>> scno)
>>   {
>>   	unsigned long ip;
> Not used, you get the same info from 'why'.
Sorry. But I do not understand what you suggest here.  This is the 
existing code.

>
>> +	if (unlikely(test_thread_flag(TIF_SYSCALL_TRACEPOINT))) {
>> +		if (why)
>> +			trace_sys_exit(regs, regs->ARM_r0);
>> +		else
>> +			trace_sys_enter(regs, scno);
>> +	}
>> +
>>   	if (!test_thread_flag(TIF_SYSCALL_TRACE))
>>   		return scno;
>>   	if (!(current->ptrace&  PT_PTRACED))
>

Thanks,

Takuo