[PATCH] ftrace: unify arch_syscall_addr() implementations

[PATCH] ftrace: unify arch_syscall_addr() implementations

[Mike Frysinger]

Every arch_syscall_addr() implementation thus far is the same, so unify
them as a default weak in common code so more arches don't have to waste
time copying & pasting this simple function.  The Blackfin version is
going to be exactly the same.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
note: only thing that needs double checking is s390 and sparc where they
declared the sys_call_table as an array of ints.  considering this table
is supposed to be an array of function pointers, this seems like more of
a typo to me ...

also, does the arch_syscall_addr() even really need to be weak ?  or can
we assume that everyone is going to be sane and do it the same way ...

 Documentation/trace/ftrace-design.txt |    6 +++++-
 arch/s390/kernel/ftrace.c             |   10 ----------
 arch/sh/kernel/ftrace.c               |    9 ---------
 arch/sparc/kernel/ftrace.c            |   11 -----------
 arch/x86/kernel/ftrace.c              |   10 ----------
 include/linux/ftrace.h                |    6 ++++++
 kernel/trace/trace_syscalls.c         |    6 ++++++
 7 files changed, 17 insertions(+), 41 deletions(-)

diff --git a/Documentation/trace/ftrace-design.txt b/Documentation/trace/ftrace-design.txt
index 6a5a579..1a67b8c 100644
--- a/Documentation/trace/ftrace-design.txt
+++ b/Documentation/trace/ftrace-design.txt
@@ -240,8 +240,12 @@ You need very few things to get the syscalls tracing in an arch.
 
 - Have a NR_syscalls variable in <asm/unistd.h> that provides the number
   of syscalls supported by the arch.
+- Implement <asm/syscall.h>.
 - Implement arch_syscall_addr() that resolves a syscall address from a
-  syscall number.
+  syscall number.  For the simple arches where your syscall table is an
+  array of longs named "sys_call_table", there is a default implementation
+  in kernel/trace/trace_syscalls.c.  If your arch needs something weird,
+  then you'll have to define the function yourself.
 - Support the TIF_SYSCALL_TRACEPOINT thread flags
 - Put the trace_sys_enter() and trace_sys_exit() tracepoints calls from ptrace
   in the ptrace syscalls tracing path.
diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
index 5a82bc6..9e69449 100644
--- a/arch/s390/kernel/ftrace.c
+++ b/arch/s390/kernel/ftrace.c
@@ -200,13 +200,3 @@ out:
 	return parent;
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned int sys_call_table[];
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-#endif
diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
index a48cded..30e1319 100644
--- a/arch/sh/kernel/ftrace.c
+++ b/arch/sh/kernel/ftrace.c
@@ -399,12 +399,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-extern unsigned long *sys_call_table;
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-#endif /* CONFIG_FTRACE_SYSCALLS */
diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
index 29973da..9103a56 100644
--- a/arch/sparc/kernel/ftrace.c
+++ b/arch/sparc/kernel/ftrace.c
@@ -91,14 +91,3 @@ int __init ftrace_dyn_arch_init(void *data)
 	return 0;
 }
 #endif
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned int sys_call_table[];
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-
-#endif
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 3096892..0d93a94 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -484,13 +484,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned long *sys_call_table;
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)(&sys_call_table)[nr];
-}
-#endif
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 0b4f97d..1cbb36f 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -511,4 +511,10 @@ static inline void trace_hw_branch_oops(void) {}
 
 #endif /* CONFIG_HW_BRANCH_TRACER */
 
+#ifdef CONFIG_FTRACE_SYSCALLS
+
+unsigned long arch_syscall_addr(int nr);
+
+#endif /* CONFIG_FTRACE_SYSCALLS */
+
 #endif /* _LINUX_FTRACE_H */
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 75289f3..671c670 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -394,6 +394,12 @@ int init_syscall_trace(struct ftrace_event_call *call)
 	return 0;
 }
 
+unsigned long __init __weak arch_syscall_addr(int nr)
+{
+	extern const unsigned long sys_call_table[];
+	return sys_call_table[nr];
+}
+
 int __init init_ftrace_syscalls(void)
 {
 	struct syscall_metadata *meta;
-- 
1.6.6.1

Re: [PATCH] ftrace: unify arch_syscall_addr() implementations

[Steven Rostedt]

[ Added Heiko Carstens and Paul Mundt to Cc ]

On Fri, 2010-01-22 at 08:43 -0500, Mike Frysinger wrote:
> Every arch_syscall_addr() implementation thus far is the same, so unify
> them as a default weak in common code so more arches don't have to waste
> time copying & pasting this simple function.  The Blackfin version is
> going to be exactly the same.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> ---
> note: only thing that needs double checking is s390 and sparc where they
> declared the sys_call_table as an array of ints.  considering this table
> is supposed to be an array of function pointers, this seems like more of
> a typo to me ...

I would not be too sure. s390 is very strange, and I would definitely
want to get an Ack from the arch maintainers first.


> 
> also, does the arch_syscall_addr() even really need to be weak ?  or can
> we assume that everyone is going to be sane and do it the same way ...
> 
>  Documentation/trace/ftrace-design.txt |    6 +++++-
>  arch/s390/kernel/ftrace.c             |   10 ----------
>  arch/sh/kernel/ftrace.c               |    9 ---------
>  arch/sparc/kernel/ftrace.c            |   11 -----------
>  arch/x86/kernel/ftrace.c              |   10 ----------
>  include/linux/ftrace.h                |    6 ++++++
>  kernel/trace/trace_syscalls.c         |    6 +++++

Actually for this patch to be accepted, we need to get the acks from the
maintainers of s390, sh, and sparc since it touches their code. Doesn't
matter if it is just removing duplicate code. Still need their acks.

Thanks,

-- Steve

> +
>  7 files changed, 17 insertions(+), 41 deletions(-)
> 
> diff --git a/Documentation/trace/ftrace-design.txt b/Documentation/trace/ftrace-design.txt
> index 6a5a579..1a67b8c 100644
> --- a/Documentation/trace/ftrace-design.txt
> +++ b/Documentation/trace/ftrace-design.txt
> @@ -240,8 +240,12 @@ You need very few things to get the syscalls tracing in an arch.
>  
>  - Have a NR_syscalls variable in <asm/unistd.h> that provides the number
>    of syscalls supported by the arch.
> +- Implement <asm/syscall.h>.
>  - Implement arch_syscall_addr() that resolves a syscall address from a
> -  syscall number.
> +  syscall number.  For the simple arches where your syscall table is an
> +  array of longs named "sys_call_table", there is a default implementation
> +  in kernel/trace/trace_syscalls.c.  If your arch needs something weird,
> +  then you'll have to define the function yourself.
>  - Support the TIF_SYSCALL_TRACEPOINT thread flags
>  - Put the trace_sys_enter() and trace_sys_exit() tracepoints calls from ptrace
>    in the ptrace syscalls tracing path.
> diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
> index 5a82bc6..9e69449 100644
> --- a/arch/s390/kernel/ftrace.c
> +++ b/arch/s390/kernel/ftrace.c
> @@ -200,13 +200,3 @@ out:
>  	return parent;
>  }
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> -
> -#ifdef CONFIG_FTRACE_SYSCALLS
> -
> -extern unsigned int sys_call_table[];
> -
> -unsigned long __init arch_syscall_addr(int nr)
> -{
> -	return (unsigned long)sys_call_table[nr];
> -}
> -#endif
> diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
> index a48cded..30e1319 100644
> --- a/arch/sh/kernel/ftrace.c
> +++ b/arch/sh/kernel/ftrace.c
> @@ -399,12 +399,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
>  	}
>  }
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> -
> -#ifdef CONFIG_FTRACE_SYSCALLS
> -extern unsigned long *sys_call_table;
> -
> -unsigned long __init arch_syscall_addr(int nr)
> -{
> -	return (unsigned long)sys_call_table[nr];
> -}
> -#endif /* CONFIG_FTRACE_SYSCALLS */
> diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
> index 29973da..9103a56 100644
> --- a/arch/sparc/kernel/ftrace.c
> +++ b/arch/sparc/kernel/ftrace.c
> @@ -91,14 +91,3 @@ int __init ftrace_dyn_arch_init(void *data)
>  	return 0;
>  }
>  #endif
> -
> -#ifdef CONFIG_FTRACE_SYSCALLS
> -
> -extern unsigned int sys_call_table[];
> -
> -unsigned long __init arch_syscall_addr(int nr)
> -{
> -	return (unsigned long)sys_call_table[nr];
> -}
> -
> -#endif
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 3096892..0d93a94 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -484,13 +484,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
>  	}
>  }
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> -
> -#ifdef CONFIG_FTRACE_SYSCALLS
> -
> -extern unsigned long *sys_call_table;
> -
> -unsigned long __init arch_syscall_addr(int nr)
> -{
> -	return (unsigned long)(&sys_call_table)[nr];
> -}
> -#endif
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 0b4f97d..1cbb36f 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -511,4 +511,10 @@ static inline void trace_hw_branch_oops(void) {}
>  
>  #endif /* CONFIG_HW_BRANCH_TRACER */
>  
> +#ifdef CONFIG_FTRACE_SYSCALLS
> +
> +unsigned long arch_syscall_addr(int nr);
> +
> +#endif /* CONFIG_FTRACE_SYSCALLS */
> +
>  #endif /* _LINUX_FTRACE_H */
> diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
> index 75289f3..671c670 100644
> --- a/kernel/trace/trace_syscalls.c
> +++ b/kernel/trace/trace_syscalls.c
> @@ -394,6 +394,12 @@ int init_syscall_trace(struct ftrace_event_call *call)
>  	return 0;
>  }
>  
> +unsigned long __init __weak arch_syscall_addr(int nr)
> +{
> +	extern const unsigned long sys_call_table[];
> +	return sys_call_table[nr];
> +}
> +
>  int __init init_ftrace_syscalls(void)
>  {
>  	struct syscall_metadata *meta;

Re: [PATCH] ftrace: unify arch_syscall_addr() implementations

[Heiko Carstens]

On Fri, Jan 22, 2010 at 09:36:17AM -0500, Steven Rostedt wrote:
> [ Added Heiko Carstens and Paul Mundt to Cc ]
> 
> On Fri, 2010-01-22 at 08:43 -0500, Mike Frysinger wrote:
> > Every arch_syscall_addr() implementation thus far is the same, so unify
> > them as a default weak in common code so more arches don't have to waste
> > time copying & pasting this simple function.  The Blackfin version is
> > going to be exactly the same.
> > 
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> > ---
> > note: only thing that needs double checking is s390 and sparc where they
> > declared the sys_call_table as an array of ints.  considering this table
> > is supposed to be an array of function pointers, this seems like more of
> > a typo to me ...
> 
> I would not be too sure. s390 is very strange, and I would definitely
> want to get an Ack from the arch maintainers first.

It's not a typo. The syscall table on s390 contains always 32 bit pointers
since we know that the address of the function to be called is (way) below
4GB. So this saves us a few bytes.
In addition this makes syscall patching done by some security modules a
bit more difficult, since they would need to store a 64 bit pointer.
That's because we make sure that module addresses are always above 4GB.

Re: [PATCH] ftrace: unify arch_syscall_addr() implementations

[Mike Frysinger]

On Fri, Jan 22, 2010 at 11:37, Heiko Carstens wrote:
> On Fri, Jan 22, 2010 at 09:36:17AM -0500, Steven Rostedt wrote:
>> On Fri, 2010-01-22 at 08:43 -0500, Mike Frysinger wrote:
>> > Every arch_syscall_addr() implementation thus far is the same, so unify
>> > them as a default weak in common code so more arches don't have to waste
>> > time copying & pasting this simple function.  The Blackfin version is
>> > going to be exactly the same.
>> >
>> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>> > ---
>> > note: only thing that needs double checking is s390 and sparc where they
>> > declared the sys_call_table as an array of ints.  considering this table
>> > is supposed to be an array of function pointers, this seems like more of
>> > a typo to me ...
>>
>> I would not be too sure. s390 is very strange, and I would definitely
>> want to get an Ack from the arch maintainers first.
>
> It's not a typo. The syscall table on s390 contains always 32 bit pointers
> since we know that the address of the function to be called is (way) below
> 4GB. So this saves us a few bytes.
> In addition this makes syscall patching done by some security modules a
> bit more difficult, since they would need to store a 64 bit pointer.
> That's because we make sure that module addresses are always above 4GB.

i'll update the s390 code to add an explanatory comment then and post
a new version
-mike

Re: [PATCH] ftrace: unify arch_syscall_addr() implementations

[David Miller]

From: Steven Rostedt <rostedt@goodmis.org>
Date: Fri, 22 Jan 2010 09:36:17 -0500

> [ Added Heiko Carstens and Paul Mundt to Cc ]
> 
> On Fri, 2010-01-22 at 08:43 -0500, Mike Frysinger wrote:
>> also, does the arch_syscall_addr() even really need to be weak ?  or can
>> we assume that everyone is going to be sane and do it the same way ...
>> 
>>  Documentation/trace/ftrace-design.txt |    6 +++++-
>>  arch/s390/kernel/ftrace.c             |   10 ----------
>>  arch/sh/kernel/ftrace.c               |    9 ---------
>>  arch/sparc/kernel/ftrace.c            |   11 -----------
>>  arch/x86/kernel/ftrace.c              |   10 ----------
>>  include/linux/ftrace.h                |    6 ++++++
>>  kernel/trace/trace_syscalls.c         |    6 +++++
> 
> Actually for this patch to be accepted, we need to get the acks from the
> maintainers of s390, sh, and sparc since it touches their code. Doesn't
> matter if it is just removing duplicate code. Still need their acks.

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] ftrace: unify arch_syscall_addr() implementations

[David Miller]

From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Fri, 22 Jan 2010 17:37:00 +0100

> On Fri, Jan 22, 2010 at 09:36:17AM -0500, Steven Rostedt wrote:
>> I would not be too sure. s390 is very strange, and I would definitely
>> want to get an Ack from the arch maintainers first.
> 
> It's not a typo. The syscall table on s390 contains always 32 bit pointers
> since we know that the address of the function to be called is (way) below
> 4GB. So this saves us a few bytes.
> In addition this makes syscall patching done by some security modules a
> bit more difficult, since they would need to store a 64 bit pointer.
> That's because we make sure that module addresses are always above 4GB.

Sparc64 is the same way.

[PATCH v2] ftrace: unify arch_syscall_addr() implementations

[Mike Frysinger]

Most implementations of arch_syscall_addr() are the same, so create a
default weak version in common code.  New arch ports don't have to waste
time copying & pasting this simple function.  The Blackfin version is
going to be exactly the same for example.

The s390/sparc versions need to be different, so document why.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
v2
	- document s390/sparc funkiness

 Documentation/trace/ftrace-design.txt |    6 +++++-
 arch/s390/kernel/ftrace.c             |    7 ++++++-
 arch/sh/kernel/ftrace.c               |    9 ---------
 arch/sparc/kernel/ftrace.c            |    7 ++++++-
 arch/x86/kernel/ftrace.c              |   10 ----------
 include/linux/ftrace.h                |    6 ++++++
 kernel/trace/trace_syscalls.c         |    6 ++++++
 7 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/Documentation/trace/ftrace-design.txt b/Documentation/trace/ftrace-design.txt
index 6a5a579..1a67b8c 100644
--- a/Documentation/trace/ftrace-design.txt
+++ b/Documentation/trace/ftrace-design.txt
@@ -240,8 +240,12 @@ You need very few things to get the syscalls tracing in an arch.
 
 - Have a NR_syscalls variable in <asm/unistd.h> that provides the number
   of syscalls supported by the arch.
+- Implement <asm/syscall.h>.
 - Implement arch_syscall_addr() that resolves a syscall address from a
-  syscall number.
+  syscall number.  For the simple arches where your syscall table is an
+  array of longs named "sys_call_table", there is a default implementation
+  in kernel/trace/trace_syscalls.c.  If your arch needs something weird,
+  then you'll have to define the function yourself.
 - Support the TIF_SYSCALL_TRACEPOINT thread flags
 - Put the trace_sys_enter() and trace_sys_exit() tracepoints calls from ptrace
   in the ptrace syscalls tracing path.
diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
index 5a82bc6..d5fa352 100644
--- a/arch/s390/kernel/ftrace.c
+++ b/arch/s390/kernel/ftrace.c
@@ -203,7 +203,12 @@ out:
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 
-extern unsigned int sys_call_table[];
+/*
+ * The syscall table always contains 32 bit pointers since we know that the
+ * address of the function to be called is (way) below 4GB.  So the "int"
+ * type here is what we want [need] for both 32 bit and 64 bit systems.
+ */
+extern const unsigned int sys_call_table[];
 
 unsigned long __init arch_syscall_addr(int nr)
 {
diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
index a48cded..30e1319 100644
--- a/arch/sh/kernel/ftrace.c
+++ b/arch/sh/kernel/ftrace.c
@@ -399,12 +399,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-extern unsigned long *sys_call_table;
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-#endif /* CONFIG_FTRACE_SYSCALLS */
diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
index 29973da..deeeaa6 100644
--- a/arch/sparc/kernel/ftrace.c
+++ b/arch/sparc/kernel/ftrace.c
@@ -94,7 +94,12 @@ int __init ftrace_dyn_arch_init(void *data)
 
 #ifdef CONFIG_FTRACE_SYSCALLS
 
-extern unsigned int sys_call_table[];
+/*
+ * The syscall table always contains 32 bit pointers since we know that the
+ * address of the function to be called is (way) below 4GB.  So the "int"
+ * type here is what we want [need] for both 32 bit and 64 bit systems.
+ */
+extern const unsigned int sys_call_table[];
 
 unsigned long __init arch_syscall_addr(int nr)
 {
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 3096892..0d93a94 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -484,13 +484,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned long *sys_call_table;
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)(&sys_call_table)[nr];
-}
-#endif
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 0b4f97d..1cbb36f 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -511,4 +511,10 @@ static inline void trace_hw_branch_oops(void) {}
 
 #endif /* CONFIG_HW_BRANCH_TRACER */
 
+#ifdef CONFIG_FTRACE_SYSCALLS
+
+unsigned long arch_syscall_addr(int nr);
+
+#endif /* CONFIG_FTRACE_SYSCALLS */
+
 #endif /* _LINUX_FTRACE_H */
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 75289f3..671c670 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -394,6 +394,12 @@ int init_syscall_trace(struct ftrace_event_call *call)
 	return 0;
 }
 
+unsigned long __init __weak arch_syscall_addr(int nr)
+{
+	extern const unsigned long sys_call_table[];
+	return sys_call_table[nr];
+}
+
 int __init init_ftrace_syscalls(void)
 {
 	struct syscall_metadata *meta;
-- 
1.6.6.1

Re: [PATCH v2] ftrace: unify arch_syscall_addr() implementations

[David Miller]

From: Mike Frysinger <vapier@gentoo.org>
Date: Sat, 23 Jan 2010 02:10:43 -0500

> Most implementations of arch_syscall_addr() are the same, so create a
> default weak version in common code.  New arch ports don't have to waste
> time copying & pasting this simple function.  The Blackfin version is
> going to be exactly the same for example.
> 
> The s390/sparc versions need to be different, so document why.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

You can just have the extern declaration match whatever the
arch needs (long or int) in the generic ftrace.c code and
then unconditionally do the (unsigned long) case.

That way even s390/sparc can use the generic implementation.

Re: [PATCH v2] ftrace: unify arch_syscall_addr() implementations

[Mike Frysinger]

On Sat, Jan 23, 2010 at 03:28, David Miller <davem@davemloft.net> wrote:
> From: Mike Frysinger <vapier@gentoo.org>
> Date: Sat, 23 Jan 2010 02:10:43 -0500
>
>> Most implementations of arch_syscall_addr() are the same, so create a
>> default weak version in common code.  New arch ports don't have to waste
>> time copying & pasting this simple function.  The Blackfin version is
>> going to be exactly the same for example.
>>
>> The s390/sparc versions need to be different, so document why.
>>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>
> You can just have the extern declaration match whatever the
> arch needs (long or int) in the generic ftrace.c code and
> then unconditionally do the (unsigned long) case.
>
> That way even s390/sparc can use the generic implementation.

asm/syscall.h is the most reasonable location to move the decls to in
asm/ ... people OK with that ?
-mike

Re: [PATCH] ftrace: unify arch_syscall_addr() implementations

[Paul Mundt]

On Fri, Jan 22, 2010 at 09:36:17AM -0500, Steven Rostedt wrote:
> [ Added Heiko Carstens and Paul Mundt to Cc ]
> 
> On Fri, 2010-01-22 at 08:43 -0500, Mike Frysinger wrote:
> > also, does the arch_syscall_addr() even really need to be weak ?  or can
> > we assume that everyone is going to be sane and do it the same way ...
> > 
> >  Documentation/trace/ftrace-design.txt |    6 +++++-
> >  arch/s390/kernel/ftrace.c             |   10 ----------
> >  arch/sh/kernel/ftrace.c               |    9 ---------
> >  arch/sparc/kernel/ftrace.c            |   11 -----------
> >  arch/x86/kernel/ftrace.c              |   10 ----------
> >  include/linux/ftrace.h                |    6 ++++++
> >  kernel/trace/trace_syscalls.c         |    6 +++++
> 
> Actually for this patch to be accepted, we need to get the acks from the
> maintainers of s390, sh, and sparc since it touches their code. Doesn't
> matter if it is just removing duplicate code. Still need their acks.
> 
Acked-by: Paul Mundt <lethal@linux-sh.org>

[PATCH v3] ftrace: unify arch_syscall_addr() implementations

[Mike Frysinger]

Most implementations of arch_syscall_addr() are the same, so create a
default version in common code and move the one piece that differs (the
syscall table) to asm/syscall.h.  New arch ports don't have to waste
time copying & pasting this simple function.

The s390/sparc versions need to be different, so document why.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: David S. Miller <davem@davemloft.net>
Acked-by: Paul Mundt <lethal@linux-sh.org>
---
v3
	- move sys_call_table[] decl to asm/syscall.h
	- unify everyone's arch_syscall_addr() again
	- mention right tracehook requirements

 Documentation/trace/ftrace-design.txt |    5 ++---
 arch/s390/include/asm/syscall.h       |    7 +++++++
 arch/s390/kernel/ftrace.c             |   10 ----------
 arch/sh/include/asm/syscall.h         |    2 ++
 arch/sh/kernel/ftrace.c               |    9 ---------
 arch/sparc/include/asm/syscall.h      |    7 +++++++
 arch/sparc/kernel/ftrace.c            |   11 -----------
 arch/x86/include/asm/syscall.h        |    2 ++
 arch/x86/kernel/ftrace.c              |   10 ----------
 include/linux/ftrace.h                |    6 ++++++
 kernel/trace/trace_syscalls.c         |    5 +++++
 11 files changed, 31 insertions(+), 43 deletions(-)

diff --git a/Documentation/trace/ftrace-design.txt b/Documentation/trace/ftrace-design.txt
index 6a5a579..f1f81af 100644
--- a/Documentation/trace/ftrace-design.txt
+++ b/Documentation/trace/ftrace-design.txt
@@ -238,11 +238,10 @@ HAVE_SYSCALL_TRACEPOINTS
 
 You need very few things to get the syscalls tracing in an arch.
 
+- Support HAVE_ARCH_TRACEHOOK (see arch/Kconfig).
 - Have a NR_syscalls variable in <asm/unistd.h> that provides the number
   of syscalls supported by the arch.
-- Implement arch_syscall_addr() that resolves a syscall address from a
-  syscall number.
-- Support the TIF_SYSCALL_TRACEPOINT thread flags
+- Support the TIF_SYSCALL_TRACEPOINT thread flags.
 - Put the trace_sys_enter() and trace_sys_exit() tracepoints calls from ptrace
   in the ptrace syscalls tracing path.
 - Tag this arch as HAVE_SYSCALL_TRACEPOINTS.
diff --git a/arch/s390/include/asm/syscall.h b/arch/s390/include/asm/syscall.h
index e0a73d3..8429686 100644
--- a/arch/s390/include/asm/syscall.h
+++ b/arch/s390/include/asm/syscall.h
@@ -15,6 +15,13 @@
 #include <linux/sched.h>
 #include <asm/ptrace.h>
 
+/*
+ * The syscall table always contains 32 bit pointers since we know that the
+ * address of the function to be called is (way) below 4GB.  So the "int"
+ * type here is what we want [need] for both 32 bit and 64 bit systems.
+ */
+extern const unsigned int sys_call_table[];
+
 static inline long syscall_get_nr(struct task_struct *task,
 				  struct pt_regs *regs)
 {
diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
index 5a82bc6..9e69449 100644
--- a/arch/s390/kernel/ftrace.c
+++ b/arch/s390/kernel/ftrace.c
@@ -200,13 +200,3 @@ out:
 	return parent;
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned int sys_call_table[];
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-#endif
diff --git a/arch/sh/include/asm/syscall.h b/arch/sh/include/asm/syscall.h
index 6a38142..aa7777b 100644
--- a/arch/sh/include/asm/syscall.h
+++ b/arch/sh/include/asm/syscall.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_SH_SYSCALL_H
 #define __ASM_SH_SYSCALL_H
 
+extern const unsigned long sys_call_table[];
+
 #ifdef CONFIG_SUPERH32
 # include "syscall_32.h"
 #else
diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
index a48cded..30e1319 100644
--- a/arch/sh/kernel/ftrace.c
+++ b/arch/sh/kernel/ftrace.c
@@ -399,12 +399,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-extern unsigned long *sys_call_table;
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-#endif /* CONFIG_FTRACE_SYSCALLS */
diff --git a/arch/sparc/include/asm/syscall.h b/arch/sparc/include/asm/syscall.h
index 7486c60..025a02a 100644
--- a/arch/sparc/include/asm/syscall.h
+++ b/arch/sparc/include/asm/syscall.h
@@ -5,6 +5,13 @@
 #include <linux/sched.h>
 #include <asm/ptrace.h>
 
+/*
+ * The syscall table always contains 32 bit pointers since we know that the
+ * address of the function to be called is (way) below 4GB.  So the "int"
+ * type here is what we want [need] for both 32 bit and 64 bit systems.
+ */
+extern const unsigned int sys_call_table[];
+
 /* The system call number is given by the user in %g1 */
 static inline long syscall_get_nr(struct task_struct *task,
 				  struct pt_regs *regs)
diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
index 29973da..9103a56 100644
--- a/arch/sparc/kernel/ftrace.c
+++ b/arch/sparc/kernel/ftrace.c
@@ -91,14 +91,3 @@ int __init ftrace_dyn_arch_init(void *data)
 	return 0;
 }
 #endif
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned int sys_call_table[];
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-
-#endif
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 8d33bc5..c4a348f 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -16,6 +16,8 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 
+extern const unsigned long sys_call_table[];
+
 /*
  * Only the low 32 bits of orig_ax are meaningful, so we return int.
  * This importantly ignores the high bits on 64-bit, so comparisons
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 3096892..0d93a94 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -484,13 +484,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned long *sys_call_table;
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)(&sys_call_table)[nr];
-}
-#endif
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 0b4f97d..1cbb36f 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -511,4 +511,10 @@ static inline void trace_hw_branch_oops(void) {}
 
 #endif /* CONFIG_HW_BRANCH_TRACER */
 
+#ifdef CONFIG_FTRACE_SYSCALLS
+
+unsigned long arch_syscall_addr(int nr);
+
+#endif /* CONFIG_FTRACE_SYSCALLS */
+
 #endif /* _LINUX_FTRACE_H */
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 75289f3..a72732e 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -394,6 +394,11 @@ int init_syscall_trace(struct ftrace_event_call *call)
 	return 0;
 }
 
+unsigned long __init arch_syscall_addr(int nr)
+{
+	return (unsigned long)sys_call_table[nr];
+}
+
 int __init init_ftrace_syscalls(void)
 {
 	struct syscall_metadata *meta;
-- 
1.6.6.1

Re: [PATCH v3] ftrace: unify arch_syscall_addr() implementations

[Heiko Carstens]

On Tue, Jan 26, 2010 at 04:40:03AM -0500, Mike Frysinger wrote:
> Most implementations of arch_syscall_addr() are the same, so create a
> default version in common code and move the one piece that differs (the
> syscall table) to asm/syscall.h.  New arch ports don't have to waste
> time copying & pasting this simple function.
> 
> The s390/sparc versions need to be different, so document why.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> Acked-by: David S. Miller <davem@davemloft.net>
> Acked-by: Paul Mundt <lethal@linux-sh.org>
> ---
> v3
> 	- move sys_call_table[] decl to asm/syscall.h
> 	- unify everyone's arch_syscall_addr() again
> 	- mention right tracehook requirements
> 
>  Documentation/trace/ftrace-design.txt |    5 ++---
>  arch/s390/include/asm/syscall.h       |    7 +++++++
>  arch/s390/kernel/ftrace.c             |   10 ----------

Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Re: [PATCH] ftrace: unify arch_syscall_addr() implementations

[Mike Frysinger]

On Fri, Jan 22, 2010 at 09:36, Steven Rostedt wrote:
> On Fri, 2010-01-22 at 08:43 -0500, Mike Frysinger wrote:
>> Every arch_syscall_addr() implementation thus far is the same, so unify
>> them as a default weak in common code so more arches don't have to waste
>> time copying & pasting this simple function.  The Blackfin version is
>> going to be exactly the same.
>>
>> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>> ---
>> note: only thing that needs double checking is s390 and sparc where they
>> declared the sys_call_table as an array of ints.  considering this table
>> is supposed to be an array of function pointers, this seems like more of
>> a typo to me ...
>
> I would not be too sure. s390 is very strange, and I would definitely
> want to get an Ack from the arch maintainers first.

i think everyone has checked in with v3 ...
-mike

Re: [PATCH v3] ftrace: unify arch_syscall_addr() implementations

[Frederic Weisbecker]

On Tue, Jan 26, 2010 at 04:40:03AM -0500, Mike Frysinger wrote:
> Most implementations of arch_syscall_addr() are the same, so create a
> default version in common code and move the one piece that differs (the
> syscall table) to asm/syscall.h.  New arch ports don't have to waste
> time copying & pasting this simple function.
> 
> The s390/sparc versions need to be different, so document why.
> 
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> Acked-by: David S. Miller <davem@davemloft.net>
> Acked-by: Paul Mundt <lethal@linux-sh.org>


Thanks!

I'm queuing it, unless Steve has already beaten me at it :)

[tip:tracing/core] tracing: Unify arch_syscall_addr() implementations

[tip-bot for Mike Frysinger]

Commit-ID:  e7b8e675d9c71b868b66f62f725a948047514719
Gitweb:     http://git.kernel.org/tip/e7b8e675d9c71b868b66f62f725a948047514719
Author:     Mike Frysinger <vapier@gentoo.org>
AuthorDate: Tue, 26 Jan 2010 04:40:03 -0500
Committer:  Frederic Weisbecker <fweisbec@gmail.com>
CommitDate: Wed, 17 Feb 2010 13:07:21 +0100

tracing: Unify arch_syscall_addr() implementations

Most implementations of arch_syscall_addr() are the same, so create a
default version in common code and move the one piece that differs (the
syscall table) to asm/syscall.h.  New arch ports don't have to waste
time copying & pasting this simple function.

The s390/sparc versions need to be different, so document why.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
Acked-by: David S. Miller <davem@davemloft.net>
Acked-by: Paul Mundt <lethal@linux-sh.org>
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
LKML-Reference: <1264498803-17278-1-git-send-email-vapier@gentoo.org>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 Documentation/trace/ftrace-design.txt |    5 ++---
 arch/s390/include/asm/syscall.h       |    7 +++++++
 arch/s390/kernel/ftrace.c             |   10 ----------
 arch/sh/include/asm/syscall.h         |    2 ++
 arch/sh/kernel/ftrace.c               |    9 ---------
 arch/sparc/include/asm/syscall.h      |    7 +++++++
 arch/sparc/kernel/ftrace.c            |   11 -----------
 arch/x86/include/asm/syscall.h        |    2 ++
 arch/x86/kernel/ftrace.c              |   10 ----------
 include/linux/ftrace.h                |    6 ++++++
 kernel/trace/trace_syscalls.c         |    5 +++++
 11 files changed, 31 insertions(+), 43 deletions(-)

diff --git a/Documentation/trace/ftrace-design.txt b/Documentation/trace/ftrace-design.txt
index 239f14b..99df110 100644
--- a/Documentation/trace/ftrace-design.txt
+++ b/Documentation/trace/ftrace-design.txt
@@ -218,11 +218,10 @@ HAVE_SYSCALL_TRACEPOINTS
 
 You need very few things to get the syscalls tracing in an arch.
 
+- Support HAVE_ARCH_TRACEHOOK (see arch/Kconfig).
 - Have a NR_syscalls variable in <asm/unistd.h> that provides the number
   of syscalls supported by the arch.
-- Implement arch_syscall_addr() that resolves a syscall address from a
-  syscall number.
-- Support the TIF_SYSCALL_TRACEPOINT thread flags
+- Support the TIF_SYSCALL_TRACEPOINT thread flags.
 - Put the trace_sys_enter() and trace_sys_exit() tracepoints calls from ptrace
   in the ptrace syscalls tracing path.
 - Tag this arch as HAVE_SYSCALL_TRACEPOINTS.
diff --git a/arch/s390/include/asm/syscall.h b/arch/s390/include/asm/syscall.h
index e0a73d3..8429686 100644
--- a/arch/s390/include/asm/syscall.h
+++ b/arch/s390/include/asm/syscall.h
@@ -15,6 +15,13 @@
 #include <linux/sched.h>
 #include <asm/ptrace.h>
 
+/*
+ * The syscall table always contains 32 bit pointers since we know that the
+ * address of the function to be called is (way) below 4GB.  So the "int"
+ * type here is what we want [need] for both 32 bit and 64 bit systems.
+ */
+extern const unsigned int sys_call_table[];
+
 static inline long syscall_get_nr(struct task_struct *task,
 				  struct pt_regs *regs)
 {
diff --git a/arch/s390/kernel/ftrace.c b/arch/s390/kernel/ftrace.c
index 5a82bc6..9e69449 100644
--- a/arch/s390/kernel/ftrace.c
+++ b/arch/s390/kernel/ftrace.c
@@ -200,13 +200,3 @@ out:
 	return parent;
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned int sys_call_table[];
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-#endif
diff --git a/arch/sh/include/asm/syscall.h b/arch/sh/include/asm/syscall.h
index 6a38142..aa7777b 100644
--- a/arch/sh/include/asm/syscall.h
+++ b/arch/sh/include/asm/syscall.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_SH_SYSCALL_H
 #define __ASM_SH_SYSCALL_H
 
+extern const unsigned long sys_call_table[];
+
 #ifdef CONFIG_SUPERH32
 # include "syscall_32.h"
 #else
diff --git a/arch/sh/kernel/ftrace.c b/arch/sh/kernel/ftrace.c
index a48cded..30e1319 100644
--- a/arch/sh/kernel/ftrace.c
+++ b/arch/sh/kernel/ftrace.c
@@ -399,12 +399,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-extern unsigned long *sys_call_table;
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-#endif /* CONFIG_FTRACE_SYSCALLS */
diff --git a/arch/sparc/include/asm/syscall.h b/arch/sparc/include/asm/syscall.h
index 7486c60..025a02a 100644
--- a/arch/sparc/include/asm/syscall.h
+++ b/arch/sparc/include/asm/syscall.h
@@ -5,6 +5,13 @@
 #include <linux/sched.h>
 #include <asm/ptrace.h>
 
+/*
+ * The syscall table always contains 32 bit pointers since we know that the
+ * address of the function to be called is (way) below 4GB.  So the "int"
+ * type here is what we want [need] for both 32 bit and 64 bit systems.
+ */
+extern const unsigned int sys_call_table[];
+
 /* The system call number is given by the user in %g1 */
 static inline long syscall_get_nr(struct task_struct *task,
 				  struct pt_regs *regs)
diff --git a/arch/sparc/kernel/ftrace.c b/arch/sparc/kernel/ftrace.c
index 29973da..9103a56 100644
--- a/arch/sparc/kernel/ftrace.c
+++ b/arch/sparc/kernel/ftrace.c
@@ -91,14 +91,3 @@ int __init ftrace_dyn_arch_init(void *data)
 	return 0;
 }
 #endif
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned int sys_call_table[];
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)sys_call_table[nr];
-}
-
-#endif
diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index 8d33bc5..c4a348f 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -16,6 +16,8 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 
+extern const unsigned long sys_call_table[];
+
 /*
  * Only the low 32 bits of orig_ax are meaningful, so we return int.
  * This importantly ignores the high bits on 64-bit, so comparisons
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 3096892..0d93a94 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -484,13 +484,3 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
 	}
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-
-#ifdef CONFIG_FTRACE_SYSCALLS
-
-extern unsigned long *sys_call_table;
-
-unsigned long __init arch_syscall_addr(int nr)
-{
-	return (unsigned long)(&sys_call_table)[nr];
-}
-#endif
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 0b4f97d..1cbb36f 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -511,4 +511,10 @@ static inline void trace_hw_branch_oops(void) {}
 
 #endif /* CONFIG_HW_BRANCH_TRACER */
 
+#ifdef CONFIG_FTRACE_SYSCALLS
+
+unsigned long arch_syscall_addr(int nr);
+
+#endif /* CONFIG_FTRACE_SYSCALLS */
+
 #endif /* _LINUX_FTRACE_H */
diff --git a/kernel/trace/trace_syscalls.c b/kernel/trace/trace_syscalls.c
index 49cea70..ecf0078 100644
--- a/kernel/trace/trace_syscalls.c
+++ b/kernel/trace/trace_syscalls.c
@@ -394,6 +394,11 @@ int init_syscall_trace(struct ftrace_event_call *call)
 	return id;
 }
 
+unsigned long __init arch_syscall_addr(int nr)
+{
+	return (unsigned long)sys_call_table[nr];
+}
+
 int __init init_ftrace_syscalls(void)
 {
 	struct syscall_metadata *meta;