[RFC,PATCH] arch/arm: compute and export NR_syscalls

[RFC,PATCH] arch/arm: compute and export NR_syscalls

[Will Drewry]

At present, there is no way to determine, at compile-time, the number of
system calls for arm.  This change provides NR_syscalls via unistd.h
much like other arches.

Historically, there hasn't been a need to pull NR_syscalls into unistd.h
for ARM.  Prior lkml threads indicate as much.  However, NR_syscalls is
a prerequisite for enabling CONFIG_FTRACE_SYSCALLS on the platform
(along with asm/syscall.h population and arch_syscall_addr).

This patch is similar to x86-64 in that the system call
count is established using the Kbuild assembly generation trick
to extract constant values. It adds three files:
- arch/arm/kernel/asm-exports.c
- arch/arm/kernel/Makefile.exports

The C file provides the location to export assembly resident values,
like the number of syscalls.  The makefile mimics Kbuild for extracting
the values from the partially compiled file.

Also included are simple fixups to make calls.S safe to #include in
asm-exports.c and exporting of asm-exports.h in unistd.h.

In particular, this change yields an NR_syscalls value that matches the
number of allocated calls via calls.S and not the aligned, padded syscall
table size needed for optimal assembly.

asm-exports.c is added instead of reusing asm-offsets.c to avoid a
variety of collisions (VM_EXEC, DMA_*, etc).  It is possible to use the
same calls.S mechanism but add NR_syscalls to asm-offsets.c.  However,
at inclusion time for generated/asm-offsets.h, conflicting defines will
need to be #undef'd if !__ASSEMBLY__ since it appears that the purpose
of asm-offsets.h is to safely bind C language definitions to assembly
and not the reverse.

- Is this approach palatable?
- Should I resend only when paired with the other ftrace-needed patches?

Thanks!

Signed-off-by: Will Drewry <wad@chromium.org>
---
 arch/arm/Makefile                |    3 +-
 arch/arm/include/asm/unistd.h    |    5 ++++
 arch/arm/kernel/Makefile.exports |   42 ++++++++++++++++++++++++++++++++++++++
 arch/arm/kernel/asm-exports.c    |   36 ++++++++++++++++++++++++++++++++
 arch/arm/kernel/calls.S          |    2 +
 5 files changed, 87 insertions(+), 1 deletions(-)
 create mode 100644 arch/arm/kernel/Makefile.exports
 create mode 100644 arch/arm/kernel/asm-exports.c

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 70c424e..62abc9a 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -270,8 +270,9 @@ all:	$(KBUILD_IMAGE)
 
 boot := arch/arm/boot
 
-archprepare:
+archprepare: Kbuild
 	$(Q)$(MAKE) $(build)=arch/arm/tools include/generated/mach-types.h
+	$(Q)$(MAKE) $(build)=arch/arm/kernel -f $(src)/arch/arm/kernel/Makefile.exports include/generated/asm-exports.h
 
 # Convert bzImage to zImage
 bzImage: zImage
diff --git a/arch/arm/include/asm/unistd.h b/arch/arm/include/asm/unistd.h
index 2c04ed5..77d4928 100644
--- a/arch/arm/include/asm/unistd.h
+++ b/arch/arm/include/asm/unistd.h
@@ -481,5 +481,10 @@
 #define __IGNORE_fadvise64_64		1
 #define __IGNORE_migrate_pages		1
 
+#if !defined(COMPILER_OFFSETS) && !defined(__ASSEMBLY__)
+#include <generated/asm-exports.h>
+#define NR_syscalls (__NR_syscall_max + 1)
+#endif
+
 #endif /* __KERNEL__ */
 #endif /* __ASM_ARM_UNISTD_H */
diff --git a/arch/arm/kernel/Makefile.exports b/arch/arm/kernel/Makefile.exports
new file mode 100644
index 0000000..5ec6553
--- /dev/null
+++ b/arch/arm/kernel/Makefile.exports
@@ -0,0 +1,42 @@
+#
+# linux/arch/arm/kernel/Makefile.exports
+#
+# Copyright (C) 2011 Chromium OS Authors <chromium-os-dev@chromium.org>
+#
+# Generate asm-exports.h based on Kbuild's asm-offsets.h
+#
+
+exports-file := include/generated/asm-exports.h
+
+# Default sed regexp - multiline due to syntax constraints
+define sed-y
+	"/^->/{s:->#\(.*\):/* \1 */:; \
+	s:^->\([^ ]*\) [\$$#]*\([-0-9]*\) \(.*\):#define \1 \2 /* \3 */:; \
+	s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; \
+	s:->::; p;}"
+endef
+
+quiet_cmd_exports = GEN     $@
+define cmd_exports
+	(set -e; \
+	 echo "#ifndef __ASM_EXPORTS_H__"; \
+	 echo "#define __ASM_EXPORTS_H__"; \
+	 echo "/*"; \
+	 echo " * DO NOT MODIFY."; \
+	 echo " *"; \
+	 echo " * This file was generated by arch/arm/tools/Makefile"; \
+	 echo " *"; \
+	 echo " */"; \
+	 echo ""; \
+	 sed -ne $(sed-y) $<; \
+	 echo ""; \
+	 echo "#endif" ) > $@
+endef
+
+# We use internal kbuild rules to avoid the "is up to date" message from make
+$(src)/../asm-exports.s: $(src)/../kernel/asm-exports.c FORCE
+	$(Q)mkdir -p $(dir $@)
+	$(call if_changed_dep,cc_s_c)
+
+$(exports-file): $(src)/../kernel/asm-exports.s Kbuild
+	$(call cmd,exports)
diff --git a/arch/arm/kernel/asm-exports.c b/arch/arm/kernel/asm-exports.c
new file mode 100644
index 0000000..a2aec12
--- /dev/null
+++ b/arch/arm/kernel/asm-exports.c
@@ -0,0 +1,36 @@
+/*
+ * Copyright (C) 2011 Chromium OS Authors <chromium-os-dev@chromium.org>
+ *
+ * Generate definitions needed by C language modules from data resident
+ * in assembly language modules. Nothing exported here should be supplied
+ * by any C language headers in common use.
+ *
+ * This code generates raw asm output which is post-processed to extract
+ * and format the required data.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#define COMPILE_OFFSETS
+#include <linux/kbuild.h>
+
+/*
+ * Make sure that the compiler and target are compatible.
+ */
+#if defined(__APCS_26__)
+#error Sorry, your compiler targets APCS-26 but this kernel requires APCS-32
+#endif
+
+#define CALL(x) 1 +
+#define syscalls_counted
+#define NO_PAD_SYSCALLS 1
+static const long syscall_count =
+	#include "calls.S"
+	0;
+
+int main(void)
+{
+	DEFINE(__NR_syscall_max, syscall_count - 1);
+	return 0;
+}
diff --git a/arch/arm/kernel/calls.S b/arch/arm/kernel/calls.S
index 80f7896..02cdece 100644
--- a/arch/arm/kernel/calls.S
+++ b/arch/arm/kernel/calls.S
@@ -385,6 +385,7 @@
 		CALL(sys_syncfs)
 		CALL(sys_sendmmsg)
 /* 375 */	CALL(sys_setns)
+#ifndef NO_PAD_SYSCALLS
 #ifndef syscalls_counted
 .equ syscalls_padding, ((NR_syscalls + 3) & ~3) - NR_syscalls
 #define syscalls_counted
@@ -392,3 +393,4 @@
 .rept syscalls_padding
 		CALL(sys_ni_syscall)
 .endr
+#endif
-- 
1.7.0.4

[RFC,PATCH] arch/arm: compute and export NR_syscalls

[Arnd Bergmann]

On Tuesday 16 August 2011 16:14:26 Will Drewry wrote:
> 
> asm-exports.c is added instead of reusing asm-offsets.c to avoid a
> variety of collisions (VM_EXEC, DMA_*, etc).  It is possible to use the
> same calls.S mechanism but add NR_syscalls to asm-offsets.c.  However,
> at inclusion time for generated/asm-offsets.h, conflicting defines will
> need to be #undef'd if !__ASSEMBLY__ since it appears that the purpose
> of asm-offsets.h is to safely bind C language definitions to assembly
> and not the reverse.
> 
> - Is this approach palatable?
> - Should I resend only when paired with the other ftrace-needed patches?

This seems overly complex, compared to a one-line change adding the symbol
to asm/unistd.h. The only other architecture that uses an approach
like the one you have posted is x86-64, and it's simpler there
because it can easily be done in asm-offsets.c there without the need
to create another helper.

	Arnd

[RFC,PATCH] arch/arm: compute and export NR_syscalls

[Will Drewry]

On Tue, Aug 16, 2011 at 4:21 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 16 August 2011 16:14:26 Will Drewry wrote:
>>
>> asm-exports.c is added instead of reusing asm-offsets.c to avoid a
>> variety of collisions (VM_EXEC, DMA_*, etc). ?It is possible to use the
>> same calls.S mechanism but add NR_syscalls to asm-offsets.c. ?However,
>> at inclusion time for generated/asm-offsets.h, conflicting defines will
>> need to be #undef'd if !__ASSEMBLY__ since it appears that the purpose
>> of asm-offsets.h is to safely bind C language definitions to assembly
>> and not the reverse.
>>
>> - Is this approach palatable?
>> - Should I resend only when paired with the other ftrace-needed patches?
>
> This seems overly complex, compared to a one-line change adding the symbol
> to asm/unistd.h. The only other architecture that uses an approach
> like the one you have posted is x86-64, and it's simpler there
> because it can easily be done in asm-offsets.c there without the need
> to create another helper.

Agreed!

I proposed this approach based solely on prior threads I've seen. E.g.,
- https://lkml.org/lkml/2007/6/1/427
  (don't just #define)
- https://lkml.org/lkml/2009/8/27/280
  (todo: x86-32 to move to x86-64)

If a single line #define is good enough, then it certainly works for me.

thanks!
will

[RFC,PATCH] arch/arm: compute and export NR_syscalls

[Mikael Pettersson]

Will Drewry writes:
 > On Tue, Aug 16, 2011 at 4:21 PM, Arnd Bergmann <arnd@arndb.de> wrote:
 > > On Tuesday 16 August 2011 16:14:26 Will Drewry wrote:
 > >>
 > >> asm-exports.c is added instead of reusing asm-offsets.c to avoid a
 > >> variety of collisions (VM_EXEC, DMA_*, etc). ?It is possible to use the
 > >> same calls.S mechanism but add NR_syscalls to asm-offsets.c. ?However,
 > >> at inclusion time for generated/asm-offsets.h, conflicting defines will
 > >> need to be #undef'd if !__ASSEMBLY__ since it appears that the purpose
 > >> of asm-offsets.h is to safely bind C language definitions to assembly
 > >> and not the reverse.
 > >>
 > >> - Is this approach palatable?
 > >> - Should I resend only when paired with the other ftrace-needed patches?
 > >
 > > This seems overly complex, compared to a one-line change adding the symbol
 > > to asm/unistd.h. The only other architecture that uses an approach
 > > like the one you have posted is x86-64, and it's simpler there
 > > because it can easily be done in asm-offsets.c there without the need
 > > to create another helper.
 > 
 > Agreed!
 > 
 > I proposed this approach based solely on prior threads I've seen. E.g.,
 > - https://lkml.org/lkml/2007/6/1/427
 >   (don't just #define)
 > - https://lkml.org/lkml/2009/8/27/280
 >   (todo: x86-32 to move to x86-64)
 > 
 > If a single line #define is good enough, then it certainly works for me.

Yes, the one-line #define NR_syscalls in unistd.h is a perfectly adequate,
if not entirely elegant, solution.  Adding asm-export.c just for this is
waaay overkill.

/Mikael

[RFC,PATCH] arch/arm: compute and export NR_syscalls

[Arnd Bergmann]

On Wednesday 17 August 2011, Mikael Pettersson wrote:
>  > I proposed this approach based solely on prior threads I've seen. E.g.,
>  > - https://lkml.org/lkml/2007/6/1/427
>  >   (don't just #define)
>  > - https://lkml.org/lkml/2009/8/27/280
>  >   (todo: x86-32 to move to x86-64)
>  > 
>  > If a single line #define is good enough, then it certainly works for me.
> 
> Yes, the one-line #define NR_syscalls in unistd.h is a perfectly adequate,
> if not entirely elegant, solution.  Adding asm-export.c just for this is
> waaay overkill.

Right. While the main problem with having the constant in asm/unistd.h
(needs to be kept in sync when adding new syscalls) is an annoyance,
the suggested approach is adding more complexity than necessary.

If you want to have the value automatically computed, I'd suggest
moving the format of unistd.h over to a method like the one used
by x86-64 and asm-generic, which is to combine the syscall number
definitions with the list of syscall pointers that currently reside
in arch/arm/kernel/calls.S, for the added benefit that it's easier to
keep the two in sync as well.

The main question is what Russell's preference is on the alternatives.

	Arnd

[RFC,PATCH] arch/arm: compute and export NR_syscalls

[Russell King - ARM Linux]

On Wed, Aug 17, 2011 at 11:22:48AM +0200, Arnd Bergmann wrote:
> On Wednesday 17 August 2011, Mikael Pettersson wrote:
> >  > I proposed this approach based solely on prior threads I've seen. E.g.,
> >  > - https://lkml.org/lkml/2007/6/1/427
> >  >   (don't just #define)
> >  > - https://lkml.org/lkml/2009/8/27/280
> >  >   (todo: x86-32 to move to x86-64)
> >  > 
> >  > If a single line #define is good enough, then it certainly works for me.
> > 
> > Yes, the one-line #define NR_syscalls in unistd.h is a perfectly adequate,
> > if not entirely elegant, solution.  Adding asm-export.c just for this is
> > waaay overkill.
> 
> Right. While the main problem with having the constant in asm/unistd.h
> (needs to be kept in sync when adding new syscalls) is an annoyance,
> the suggested approach is adding more complexity than necessary.
> 
> If you want to have the value automatically computed, I'd suggest
> moving the format of unistd.h over to a method like the one used
> by x86-64 and asm-generic, which is to combine the syscall number
> definitions with the list of syscall pointers that currently reside
> in arch/arm/kernel/calls.S, for the added benefit that it's easier to
> keep the two in sync as well.

You obviously haven't looked at calls.S - the table has multiple
options depending on whether its being used for EABI or OABI.  It's
not purely a 1:1 mapping between syscall number name and function
name.

Adding an additional parameter to the CALL() macro to get around that
for the syscall number name starts making the file unweidly, especially
if we obey the 80 column limit.

Finally, the assembly 'number of syscalls' is different from the real
number of syscalls to ensure that we don't overflow the 8-bit constant
limit for the compare instruction.  Whether that needs to be included
in the C __NR_syscalls or not depends on how its used.

I personally would prefer C code not to rely on the (unprovided)
NR_syscalls due to these kinds of issues.

Finally, if its just for ftrace, well I don't have a high regard for that
code.  It's something I hardly ever use and when I have tried to use it
it has been soo dire in terms of overheads that it's just not worth
bothering with.  When I want timings out of the kernel, I have always
(and continue to do so) implement my own gathering mechanisms rather
than using the ftrace crap.

[RFC,PATCH] arch/arm: compute and export NR_syscalls

[Will Drewry]

On Sun, Aug 21, 2011 at 4:07 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Aug 17, 2011 at 11:22:48AM +0200, Arnd Bergmann wrote:
>> On Wednesday 17 August 2011, Mikael Pettersson wrote:
>> > ?> I proposed this approach based solely on prior threads I've seen. E.g.,
>> > ?> - https://lkml.org/lkml/2007/6/1/427
>> > ?> ? (don't just #define)
>> > ?> - https://lkml.org/lkml/2009/8/27/280
>> > ?> ? (todo: x86-32 to move to x86-64)
>> > ?>
>> > ?> If a single line #define is good enough, then it certainly works for me.
>> >
>> > Yes, the one-line #define NR_syscalls in unistd.h is a perfectly adequate,
>> > if not entirely elegant, solution. ?Adding asm-export.c just for this is
>> > waaay overkill.
>>
>> Right. While the main problem with having the constant in asm/unistd.h
>> (needs to be kept in sync when adding new syscalls) is an annoyance,
>> the suggested approach is adding more complexity than necessary.
>>
>> If you want to have the value automatically computed, I'd suggest
>> moving the format of unistd.h over to a method like the one used
>> by x86-64 and asm-generic, which is to combine the syscall number
>> definitions with the list of syscall pointers that currently reside
>> in arch/arm/kernel/calls.S, for the added benefit that it's easier to
>> keep the two in sync as well.
>
> You obviously haven't looked at calls.S - the table has multiple
> options depending on whether its being used for EABI or OABI. ?It's
> not purely a 1:1 mapping between syscall number name and function
> name.
>
> Adding an additional parameter to the CALL() macro to get around that
> for the syscall number name starts making the file unweidly, especially
> if we obey the 80 column limit.
>
> Finally, the assembly 'number of syscalls' is different from the real
> number of syscalls to ensure that we don't overflow the 8-bit constant
> limit for the compare instruction. ?Whether that needs to be included
> in the C __NR_syscalls or not depends on how its used.
>
> I personally would prefer C code not to rely on the (unprovided)
> NR_syscalls due to these kinds of issues.

Upon continued inspection, I largely agree.I have some out-of-tree
code (hopefully not forever) which uses ftrace.  It followed its
coding approach which statically allocates arrays based on
NR_syscalls.  While it makes lookups simple, it creates more
infrastructure headaches for arches that have a non-zero syscall base
and/or differenting numberings based on the active ABI.  I've since
changed my approach to one that works for all architectures and
doesn't require knowing NR_syscalls at compiled-time.

> Finally, if its just for ftrace, well I don't have a high regard for that
> code. ?It's something I hardly ever use and when I have tried to use it
> it has been soo dire in terms of overheads that it's just not worth
> bothering with. ?When I want timings out of the kernel, I have always
> (and continue to do so) implement my own gathering mechanisms rather
> than using the ftrace crap.

My personal interest is in reusing the shared filter engine along with
its awareness of system call metadata.  That aside, I have a sneaking
suspicion ftrace_syscalls will move away from the current
NR_syscalls-based approach in order to support more architecture
flavors, but I have no idea for sure.  (I'll be sending patches at
some point.)

thanks for your thoughts and consideration!
will