Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331373 > unrolled thread

[PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2)

Back to article view | Back to linux.kernel

Contents

  [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2) Torsten Duwe <duwe@lst.de> - 2016-02-10 18:50 +0100
    [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Torsten Duwe <duwe@lst.de> - 2016-02-10 18:50 +0100
      Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Balbir Singh <bsingharora@gmail.com> - 2016-02-11 08:50 +0100
        Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Kamalesh Babulal <kamalesh@linux.vnet.ibm.com> - 2016-02-11 09:50 +0100
          Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Balbir Singh <bsingharora@gmail.com> - 2016-02-11 10:40 +0100
            Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Murali Sampath <MSampath@alphion.com> - 2016-02-11 14:10 +0100
            Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Murali Sampath <MSampath@alphion.com> - 2016-02-11 14:10 +0100
        Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Torsten Duwe <duwe@lst.de> - 2016-02-11 09:50 +0100
          Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Balbir Singh <bsingharora@gmail.com> - 2016-02-11 10:40 +0100
          Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Michael Ellerman <mpe@ellerman.id.au> - 2016-02-15 11:30 +0100
            Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Jiri Kosina <jikos@kernel.org> - 2016-02-15 14:00 +0100
            Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Torsten Duwe <duwe@lst.de> - 2016-02-15 15:10 +0100
              Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Torsten Duwe <duwe@lst.de> - 2016-02-15 23:30 +0100
                Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Balbir Singh <bsingharora@gmail.com> - 2016-02-16 06:00 +0100
              Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Michael Ellerman <mpe@ellerman.id.au> - 2016-02-16 11:20 +0100
                Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables Torsten Duwe <duwe@lst.de> - 2016-02-16 11:40 +0100
    [PATCH v8 3/8] ppc use ftrace_modify_all_code default Torsten Duwe <duwe@lst.de> - 2016-02-10 18:50 +0100
    [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel Torsten Duwe <duwe@lst.de> - 2016-02-10 18:50 +0100
      Re: [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel Michael Ellerman <mpe@ellerman.id.au> - 2016-02-17 12:00 +0100
        Re: [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel Michael Ellerman <mpe@ellerman.id.au> - 2016-02-17 12:40 +0100
        Re: [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel Torsten Duwe <duwe@lst.de> - 2016-02-17 12:40 +0100
    [PATCH v8 6/8] Implement kernel live patching for ppc64le (ABIv2) Torsten Duwe <duwe@lst.de> - 2016-02-10 18:50 +0100
      Re: [PATCH v8 6/8] Implement kernel live patching for ppc64le  (ABIv2) Miroslav Benes <mbenes@suse.cz> - 2016-02-11 10:10 +0100
    Re: [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE  (ABI v2) Balbir Singh <bsingharora@gmail.com> - 2016-02-11 07:20 +0100
      Re: [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE  (ABI v2) Torsten Duwe <duwe@lst.de> - 2016-02-11 09:40 +0100

Page 1 of 2  [1] 2  Next page →

#1331373 — [PATCH v8 0/8] ftrace with regs + live patching for ppc64 LE (ABI v2)

Changes since V7:
  * drop "notrace" attribute for MMU-aiding functions
    and their callees.
  * merge "-mprofile-kernel"-stripping patches into one.

Changes since v6:
  * include Petr's patch, on popular demand ;)
  * move #ifdefs out of functions for readability;
    introduce static helper functions instead.
  * No more literal binary instructions in hex,
    at least not added by this patch set.
  * add compile time checker to detect the presence
    of known-good -mprofile-kernel support.
  * limit Kconfig / compile to the configurations really supported:
    + (static) FTRACE with -pg
    +  DYNAMIC_FTRACE with -pg with or without -mprofile-kernel
       (depending on the compiler)
    +  DYNAMIC_FTRACE_WITH_REGS only with -mprofile-kernel
       (will error out if the compiler is broken)

Changes since v5:
  * extra "std r0,LRSAVE(r1)" for gcc-6
    This makes the code compiler-agnostic.
  * Follow Petr Mladek's suggestion to avoid
    redefinition of HAVE_LIVEPATCH

Changes since v4:
  * change comment style in entry_64.S to C89
    (nobody is using assembler syntax comments there).
  * the bool function restore_r2 shouldn't return 2,
    that's a little confusing.
  * Test whether the compiler supports -mprofile-kernel
    and only then define CC_USING_MPROFILE_KERNEL
  * also make the return value of klp_check_compiler_support
    depend on that.

Major changes since v3:
  * the graph tracer works now.
    It turned out the stack frame it tried to manipulate does not
    exist at that point.
  * changes only needed in order to support -mprofile-kernel are now
    in a separate patch, prepended.
  * Kconfig cleanup so this is only selectable on ppc64le.

Petr Mladek (1):
  livepatch: Detect offset for the ftrace location during build

Torsten Duwe (7):
  ppc64 (le): prepare for -mprofile-kernel
  ppc64le FTRACE_WITH_REGS implementation
  ppc use ftrace_modify_all_code default
  ppc64 ftrace_with_regs configuration variables
  ppc64 ftrace_with_regs: disable profiling for some files
  Implement kernel live patching for ppc64le (ABIv2)
  Enable LIVEPATCH to be configured on ppc64le and add livepatch.o if it
    is selected

 arch/powerpc/Kconfig                        |   6 +
 arch/powerpc/Makefile                       |  17 +++
 arch/powerpc/gcc-mprofile-kernel-notrace.sh |  33 ++++++
 arch/powerpc/include/asm/code-patching.h    |  24 ++++
 arch/powerpc/include/asm/ftrace.h           |   5 +
 arch/powerpc/include/asm/livepatch.h        |  45 ++++++++
 arch/powerpc/kernel/Makefile                |  13 ++-
 arch/powerpc/kernel/entry_64.S              | 169 +++++++++++++++++++++++++++-
 arch/powerpc/kernel/ftrace.c                | 129 ++++++++++++++++-----
 arch/powerpc/kernel/livepatch.c             |  38 +++++++
 arch/powerpc/kernel/module_64.c             |  56 ++++++++-
 arch/powerpc/lib/Makefile                   |   4 +-
 arch/s390/Kconfig                           |   1 +
 kernel/livepatch/Makefile                   |  13 +++
 kernel/livepatch/core.c                     |  12 +-
 kernel/livepatch/ftrace-test.c              |   6 +
 kernel/trace/Kconfig                        |   5 +
 scripts/recordmcount.c                      |   6 +-
 scripts/recordmcount.h                      |  17 ++-
 19 files changed, 552 insertions(+), 47 deletions(-)
 create mode 100755 arch/powerpc/gcc-mprofile-kernel-notrace.sh
 create mode 100644 arch/powerpc/include/asm/livepatch.h
 create mode 100644 arch/powerpc/kernel/livepatch.c
 create mode 100644 kernel/livepatch/ftrace-test.c

-- 
1.8.5.6

[toc] | [next] | [standalone]

#1331374 — [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

  * arch/powerpc/Makefile:
    - globally use -mprofile-kernel in case it's configured,
      available and bug-free.
  * arch/powerpc/gcc-mprofile-kernel-notrace.sh:
    - make sure -mprofile-kernel works and has none of the
      known bugs.
  * arch/powerpc/kernel/ftrace.c:
    - error out on compile with HAVE_DYNAMIC_FTRACE_WITH_REGS
      and a buggy compiler.
  * arch/powerpc/Kconfig / kernel/trace/Kconfig:
    - declare that ppc64le HAVE_MPROFILE_KERNEL and
      HAVE_DYNAMIC_FTRACE_WITH_REGS, and use it.

Signed-off-by: Torsten Duwe <duwe@suse.de>
---
 arch/powerpc/Kconfig                        |  2 ++
 arch/powerpc/Makefile                       | 17 +++++++++++++++
 arch/powerpc/gcc-mprofile-kernel-notrace.sh | 33 +++++++++++++++++++++++++++++
 arch/powerpc/kernel/ftrace.c                |  5 +++++
 kernel/trace/Kconfig                        |  5 +++++
 5 files changed, 62 insertions(+)
 create mode 100755 arch/powerpc/gcc-mprofile-kernel-notrace.sh

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e4824fd..e5f288c 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -94,8 +94,10 @@ config PPC
 	select OF_RESERVED_MEM
 	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS if PPC64 && CPU_LITTLE_ENDIAN
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_GRAPH_TRACER
+	select HAVE_MPROFILE_KERNEL if PPC64 && CPU_LITTLE_ENDIAN
 	select SYSCTL_EXCEPTION_TRACE
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select VIRT_TO_BUS if !PPC64
diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 96efd82..08a3952 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile
@@ -133,6 +133,23 @@ else
 CFLAGS-$(CONFIG_GENERIC_CPU) += -mcpu=powerpc64
 endif
 
+ifeq ($(CONFIG_PPC64),y)
+ifdef CONFIG_HAVE_MPROFILE_KERNEL
+
+ifdef CONFIG_DYNAMIC_FTRACE
+ifeq ($(shell $(CONFIG_SHELL) $(srctree)/arch/powerpc/gcc-mprofile-kernel-notrace.sh $(CC) -I$(srctree)/include -D__KERNEL__), y)
+CC_USING_MPROFILE_KERNEL := -mprofile-kernel
+endif
+endif
+
+ifdef CC_USING_MPROFILE_KERNEL
+CC_FLAGS_FTRACE	:= -pg $(CC_USING_MPROFILE_KERNEL)
+KBUILD_CPPFLAGS	+= -DCC_USING_MPROFILE_KERNEL
+endif
+
+endif
+endif
+
 CFLAGS-$(CONFIG_CELL_CPU) += $(call cc-option,-mcpu=cell)
 CFLAGS-$(CONFIG_POWER4_CPU) += $(call cc-option,-mcpu=power4)
 CFLAGS-$(CONFIG_POWER5_CPU) += $(call cc-option,-mcpu=power5)
diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
new file mode 100755
index 0000000..68d6482
--- /dev/null
+++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+# Test whether the compile option -mprofile-kernel
+# generates profiling code ( = a call to mcount), and
+# whether a function without any global references sets
+# the TOC pointer properly at the beginning, and
+# whether the "notrace" function attribute successfully
+# suppresses the _mcount call.
+
+echo "int func() { return 0; }" | \
+    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+    grep -q "mcount"
+
+trace_result=$?
+
+echo "int func() { return 0; }" | \
+    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
+
+leaf_toc_result=$?
+
+/bin/echo -e "#include <linux/compiler.h>\nnotrace int func() { return 0; }" | \
+    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
+    grep -q "mcount"
+
+notrace_result=$?
+
+if [ "$trace_result" -eq "0" -a \
+	"$leaf_toc_result" -eq "0" -a \
+	"$notrace_result" -eq "1" ]; then
+	echo y
+else
+	echo n
+fi
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index ef8b916..29b7014 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -28,6 +28,11 @@
 
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+#if defined(CONFIG_DYNAMIC_FTRACE_WITH_REGS) && defined(CONFIG_PPC64) && \
+  !defined(CC_USING_MPROFILE_KERNEL)
+#error "DYNAMIC_FTRACE_WITH_REGS requires working -mprofile-kernel"
+#endif
+
 static unsigned int
 ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
 {
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index e45db6b..a138f6d 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -52,6 +52,11 @@ config HAVE_FENTRY
 	help
 	  Arch supports the gcc options -pg with -mfentry
 
+config HAVE_MPROFILE_KERNEL
+	bool
+	help
+	  Arch supports the gcc options -pg with -mprofile-kernel
+
 config HAVE_C_RECORDMCOUNT
 	bool
 	help
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1331716 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:

snip

> diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> new file mode 100755
> index 0000000..68d6482
> --- /dev/null
> +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> @@ -0,0 +1,33 @@
> +#!/bin/sh
> +# Test whether the compile option -mprofile-kernel
> +# generates profiling code ( = a call to mcount), and
> +# whether a function without any global references sets
> +# the TOC pointer properly at the beginning, and
> +# whether the "notrace" function attribute successfully
> +# suppresses the _mcount call.
> +
> +echo "int func() { return 0; }" | \
> +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> +    grep -q "mcount"
> +
> +trace_result=$?
> +
> +echo "int func() { return 0; }" | \
> +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> +
> +leaf_toc_result=$?
> +

leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
and give the patches a spin

> +/bin/echo -e "#include \nnotrace int func() { return 0; }" | \
> +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> +    grep -q "mcount"
> +
> +notrace_result=$?
> +
> +if [ "$trace_result" -eq "0" -a \
> +	"$leaf_toc_result" -eq "0" -a \
> +	"$notrace_result" -eq "1" ]; then
> +	echo y
> +else
> +	echo n
> +fi

[toc] | [prev] | [next] | [standalone]

#1331734 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

* Balbir Singh <bsingharora@gmail.com> [2016-02-11 18:48:17]:

> On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> 
> snip
> 
> > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > new file mode 100755
> > index 0000000..68d6482
> > --- /dev/null
> > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > @@ -0,0 +1,33 @@
> > +#!/bin/sh
> > +# Test whether the compile option -mprofile-kernel
> > +# generates profiling code ( = a call to mcount), and
> > +# whether a function without any global references sets
> > +# the TOC pointer properly at the beginning, and
> > +# whether the "notrace" function attribute successfully
> > +# suppresses the _mcount call.
> > +
> > +echo "int func() { return 0; }" | \
> > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > +    grep -q "mcount"
> > +
> > +trace_result=$?
> > +
> > +echo "int func() { return 0; }" | \
> > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > +
> > +leaf_toc_result=$?
> > +
> 
> leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> and give the patches a spin
> 

It fails for me to on ppc64le but pass over ppc64

# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/ppc64-redhat-linux/5.1.1/lto-wrapper
Target: ppc64-redhat-linux
[...]
gcc version 5.1.1 20150422 (Red Hat 5.1.1-1) (GCC)

# echo "int func() { return 0;  }" |     gcc -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null |     sed -n -e '/func:/,/bl _mcount/p'
func:
        .quad   .L.func,.TOC.@tocbase
        .previous
        .type   func, @function
.L.func:
        mflr 0
        std 0,16(1)
        bl _mcount

# gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/powerpc64le-linux-gnu/5/lto-wrapper
Target: powerpc64le-linux-gnu
[...]
gcc version 5.3.1 20160205 (Ubuntu/IBM 5.3.1-8ubuntu2)

# echo "int func() { return 0;  }" |     gcc -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null |     sed -n -e '/func:/,/bl _mcount/p'
func:
        mflr 0
        std 0,16(1)
        bl _mcount


I will try it over gcc-6.

Thanks,
Kamalesh

[toc] | [prev] | [next] | [standalone]

#1331781 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Thu, 2016-02-11 at 14:09 +0530, Kamalesh Babulal wrote:
> * Balbir Singh <bsingharora@gmail.com> [2016-02-11 18:48:17]:
> 
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> > 
> > snip
> > 
> > > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > new file mode 100755
> > > index 0000000..68d6482
> > > --- /dev/null
> > > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > @@ -0,0 +1,33 @@
> > > +#!/bin/sh
> > > +# Test whether the compile option -mprofile-kernel
> > > +# generates profiling code ( = a call to mcount), and
> > > +# whether a function without any global references sets
> > > +# the TOC pointer properly at the beginning, and
> > > +# whether the "notrace" function attribute successfully
> > > +# suppresses the _mcount call.
> > > +
> > > +echo "int func() { return 0; }" | \
> > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > +    grep -q "mcount"
> > > +
> > > +trace_result=$?
> > > +
> > > +echo "int func() { return 0; }" | \
> > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=$?
> > > +
> > 
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
> > 
> 
> It fails for me to on ppc64le but pass over ppc64
> 

This series is for ppc64le only, so we can safely ignore ppc64 for now

Regards,
Balbir Singh.

[toc] | [prev] | [next] | [standalone]

#1331928 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

  Original Message
From: Balbir Singh
Sent: Thursday, February 11, 2016 4:37 AM
To: Kamalesh Babulal
Cc: Petr Mladek; Jessica Yu; linux-kernel@vger.kernel.org; Steven Rostedt; Torsten Duwe; Jiri Kosina; live-patching@vger.kernel.org; Miroslav Benes; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables


On Thu, 2016-02-11 at 14:09 +0530, Kamalesh Babulal wrote:
> * Balbir Singh <bsingharora@gmail.com> [2016-02-11 18:48:17]:
>
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> >
> > snip
> >
> > > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > new file mode 100755
> > > index 0000000..68d6482
> > > --- /dev/null
> > > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > @@ -0,0 +1,33 @@
> > > +#!/bin/sh
> > > +# Test whether the compile option -mprofile-kernel
> > > +# generates profiling code ( = a call to mcount), and
> > > +# whether a function without any global references sets
> > > +# the TOC pointer properly at the beginning, and
> > > +# whether the "notrace" function attribute successfully
> > > +# suppresses the _mcount call.
> > > +
> > > +echo "int func() { return 0; }" | \
> > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > +    grep -q "mcount"
> > > +
> > > +trace_result=$?
> > > +
> > > +echo "int func() { return 0; }" | \
> > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=$?
> > > +
> >
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
> >
>
> It fails for me to on ppc64le but pass over ppc64
>

This series is for ppc64le only, so we can safely ignore ppc64 for now

Regards,
Balbir Singh.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[toc] | [prev] | [next] | [standalone]

#1331929 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

‎ t

  Original Message
From: Balbir Singh
Sent: Thursday, February 11, 2016 4:37 AM
To: Kamalesh Babulal
Cc: Petr Mladek; Jessica Yu; linux-kernel@vger.kernel.org; Steven Rostedt; Torsten Duwe; Jiri Kosina; live-patching@vger.kernel.org; Miroslav Benes; linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables


On Thu, 2016-02-11 at 14:09 +0530, Kamalesh Babulal wrote:
> * Balbir Singh <bsingharora@gmail.com> [2016-02-11 18:48:17]:
>
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> >
> > snip
> >
> > > diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > new file mode 100755
> > > index 0000000..68d6482
> > > --- /dev/null
> > > +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh
> > > @@ -0,0 +1,33 @@
> > > +#!/bin/sh
> > > +# Test whether the compile option -mprofile-kernel
> > > +# generates profiling code ( = a call to mcount), and
> > > +# whether a function without any global references sets
> > > +# the TOC pointer properly at the beginning, and
> > > +# whether the "notrace" function attribute successfully
> > > +# suppresses the _mcount call.
> > > +
> > > +echo "int func() { return 0; }" | \
> > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > +    grep -q "mcount"
> > > +
> > > +trace_result=$?
> > > +
> > > +echo "int func() { return 0; }" | \
> > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=$?
> > > +
> >
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
> >
>
> It fails for me to on ppc64le but pass over ppc64
>

This series is for ppc64le only, so we can safely ignore ppc64 for now

Regards,
Balbir Singh.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[toc] | [prev] | [next] | [standalone]

#1331737 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Thu, Feb 11, 2016 at 06:48:17PM +1100, Balbir Singh wrote:
> On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> > +
> > +echo "int func() { return 0; }" | \
> > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > +
> > +leaf_toc_result=$?
> > +
> 
> leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> and give the patches a spin

Don't bother. _All_ gccs are broken in that respect currently.
AFAIK Anton is working on this. You have to fake the test, like
static int a; return a++;

Gcc fails to set the TOC for profiled "leaf" functions, where it
thinks no global/static symbols are referenced.

	Torsten

[toc] | [prev] | [next] | [standalone]

#1331782 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Thu, 2016-02-11 at 09:42 +0100, Torsten Duwe wrote:
> On Thu, Feb 11, 2016 at 06:48:17PM +1100, Balbir Singh wrote:
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> > > +
> > > +echo "int func() { return 0; }" | \
> > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=$?
> > > +
> > 
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
> 
> Don't bother. _All_ gccs are broken in that respect currently.
> AFAIK Anton is working on this. You have to fake the test, like
> static int a; return a++;
> 
> Gcc fails to set the TOC for profiled "leaf" functions, where it
> thinks no global/static symbols are referenced.
> 
> 

Thanks for the quick answer

BTW, do we expect static/non global functions to be called from
patched contexts? I understand that not supporting it will
complicate a patch.

Balbir Singh

[toc] | [prev] | [next] | [standalone]

#1334350 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

Hi guys,

Sorry I haven't been keeping up to date with this thread I've been away.

On Thu, 2016-02-11 at 09:42 +0100, Torsten Duwe wrote:
> On Thu, Feb 11, 2016 at 06:48:17PM +1100, Balbir Singh wrote:
> > On Wed, 2016-02-10 at 17:25 +0100, Torsten Duwe wrote:
> > > +
> > > +echo "int func() { return 0; }" | \
> > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > +
> > > +leaf_toc_result=$?
> > 
> > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > and give the patches a spin
> 
> Don't bother. _All_ gccs are broken in that respect currently.

I'm not sure where we got our wires crossed on this one, but this is not a gcc
bug. In fact it's a feature :)

There is explicit code in gcc to check whether the TOC setup is needed and only
emit it when it's required. One case where it's *not* required is when the
function does not TOC accesses. (See rs6000_global_entry_point_needed_p()).

So I think this is just one of the "fun" details we have to deal with to
implement kernel tracing.

The first implication of this is that there is not a single offset value for
the _mcount call, it needs to be calculated per-function. In practice there
will only be two values, so we could try both, or we could just have the code
work it out dynamically by looking for the expected code sequence?

Secondly it means the ftrace trampoline needs to cope with being called with r2
containing something other than the kernel TOC (ie. a module's TOC pointer).
But I think that's solvable also?

cheers

[toc] | [prev] | [next] | [standalone]

#1334441 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Mon, 15 Feb 2016, Michael Ellerman wrote:

> > > > +echo "int func() { return 0; }" | \
> > > > +    $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \
> > > > +    sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC
> > > > +
> > > > +leaf_toc_result=$?
> > > 
> > > leaf_toc_result failed for me with gcc 5. I'll try and grab gcc-6
> > > and give the patches a spin
> > 
> > Don't bother. _All_ gccs are broken in that respect currently.
> 
> I'm not sure where we got our wires crossed on this one, but this is not a gcc
> bug. In fact it's a feature :)
> 
> There is explicit code in gcc to check whether the TOC setup is needed and only
> emit it when it's required. One case where it's *not* required is when the
> function does not TOC accesses. (See rs6000_global_entry_point_needed_p()).

As gcc actually implements a '-mprofile-kernel' option, it's pretty much 
aware of the fact that the inserted space will be used by the kernel for 
inserting a call (as that's the sole point of the whole thing).

Therefore it must not consider any traceable function to be leaf (even 
though it might "look" leaf from the source code); if it does, then the 
mprofile-kernel option is useless.

So I actually would dare to call it a bug.

Thanks,

-- 
Jiri Kosina
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1334473 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Mon, Feb 15, 2016 at 09:27:15PM +1100, Michael Ellerman wrote:
> 
> There is explicit code in gcc to check whether the TOC setup is needed and only

That's undestood. The claim here is: that check is incomplete, at least.

> emit it when it's required. One case where it's *not* required is when the
> function does not TOC accesses. (See rs6000_global_entry_point_needed_p()).

n.b. I cannot find this symbol in the 4.9.3 tree, but I know what you mean.

The point here is: If you profile using "-pg", gcc perfectly recognises that
it is generating a call to "_mcount", which may be non-local, and loads the TOC.
If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
load, for a similar assembler calling sequence.

Looking at the code I can _understand_ why this is so, but my GCC knowledge
is not that deep that I could easily _fix_ this reliably.

[...]

> Secondly it means the ftrace trampoline needs to cope with being called with r2
> containing something other than the kernel TOC (ie. a module's TOC pointer).
> But I think that's solvable also?

That was the alternative I asked about; but given that the _mcount / ftrace_caller
trampoline hardly differs from a normal trampoline (so far), loading R2 would be the
general case, or an excessive special case handling would result.

I'd rather see gcc getting fixed than to have workarounds in all projects that use
it, and others agreed.

	Torsten

[toc] | [prev] | [next] | [standalone]

#1334854 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Mon, Feb 15, 2016 at 03:04:08PM +0100, Torsten Duwe wrote:
> If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
> load, for a similar assembler calling sequence.
> 
> Looking at the code I can _understand_ why this is so, but my GCC knowledge
> is not that deep that I could easily _fix_ this reliably.

Nonetheless, here's a proposal.

--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -25154,6 +25154,10 @@ rs6000_emit_prologue (void)
     {
       cfun->machine->r2_setup_needed = df_regs_ever_live_p (TOC_REGNUM);
 
+      /* Profiling _will_ generate a call to a global _mcount. */
+      if (crtl->profile)
+	cfun->machine->r2_setup_needed = true;
+
       /* With -mminimal-toc we may generate an extra use of r2 below.  */
       if (TARGET_TOC && TARGET_MINIMAL_TOC && get_pool_size () != 0)
 	cfun->machine->r2_setup_needed = true;

[toc] | [prev] | [next] | [standalone]

#1334995 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Mon, 2016-02-15 at 23:21 +0100, Torsten Duwe wrote:
> On Mon, Feb 15, 2016 at 03:04:08PM +0100, Torsten Duwe wrote:
> > If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
> > load, for a similar assembler calling sequence.
> > 
> > Looking at the code I can _understand_ why this is so, but my GCC knowledge
> > is not that deep that I could easily _fix_ this reliably.
> 
> Nonetheless, here's a proposal.
> 
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -25154,6 +25154,10 @@ rs6000_emit_prologue (void)
>      {
>        cfun->machine->r2_setup_needed = df_regs_ever_live_p (TOC_REGNUM);
>  
> +      /* Profiling _will_ generate a call to a global _mcount. */
> +      if (crtl->profile)
> +	cfun->machine->r2_setup_needed = true;
> +
>        /* With -mminimal-toc we may generate an extra use of r2 below.  */
>        if (TARGET_TOC && TARGET_MINIMAL_TOC && get_pool_size () != 0)
>  	cfun->machine->r2_setup_needed = true;

I was wondering if ftrace() works correctly, specifically dynamic ftrace.
We should be able to get livepatching to work as well. I can see r2 being
saved in the stub in ftrace_caller in patch 2/8

+       mr      r0,r2   /* save callee's TOC */
+       addis   r2,r12,(.TOC.-ftrace_caller-12)@ha
+       addi    r2,r2,(.TOC.-ftrace_caller-12)@l

The only limitation today is figuring out the correct offset to patch
(8 or 16) depending on whether the TOC stub is generated or not by the
compiler

If the sequence is well known, we could potentially scan instructions
or go to the hash that ftrace maintains and search in there with an offset
of 8 to 16.

Balbir Singh

[toc] | [prev] | [next] | [standalone]

#1335251 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Mon, 2016-02-15 at 15:04 +0100, Torsten Duwe wrote:
> On Mon, Feb 15, 2016 at 09:27:15PM +1100, Michael Ellerman wrote:
> > 
> > There is explicit code in gcc to check whether the TOC setup is needed and only
> 
> That's undestood. The claim here is: that check is incomplete, at least.

OK at least we agree on what's happening.

> > emit it when it's required. One case where it's *not* required is when the
> > function does not TOC accesses. (See rs6000_global_entry_point_needed_p()).
> 
> n.b. I cannot find this symbol in the 4.9.3 tree, but I know what you mean.
> 
> The point here is: If you profile using "-pg", gcc perfectly recognises that
> it is generating a call to "_mcount", which may be non-local, and loads the TOC.
> If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
> load, for a similar assembler calling sequence.

That's by design.

mprofile-kernel is supposed to create as little overhead as possible in the
non-traced case. All of the burden is shifted to the trace function (_mcount).

The reason to do that is because modern distros always build with tracing, but
most of the time tracing will not actually be active. So we want the cost of
tracing-built-in-but-disabled to be ~zero.

> > Secondly it means the ftrace trampoline needs to cope with being called with r2
> > containing something other than the kernel TOC (ie. a module's TOC pointer).
> > But I think that's solvable also?
> 
> That was the alternative I asked about; but given that the _mcount / ftrace_caller
> trampoline hardly differs from a normal trampoline (so far), loading R2 would be the
> general case, or an excessive special case handling would result.

I'm not sure I follow what you mean there at the end.

Requiring ftrace_caller() to load the kernel TOC is not a problem IMHO.

I think I have an easier way to do it, I'll reply to the patch with that (if it
works).

cheers

[toc] | [prev] | [next] | [standalone]

#1335265 — Re: [PATCH v8 4/8] ppc64 ftrace_with_regs configuration variables

On Tue, Feb 16, 2016 at 09:09:16PM +1100, Michael Ellerman wrote:
> On Mon, 2016-02-15 at 15:04 +0100, Torsten Duwe wrote:
> > If you use "-pg -mprofile-kernel", gcc seems to forget that, and omits the TOC
> > load, for a similar assembler calling sequence.
> 
> That's by design.

Ah, ok.

> mprofile-kernel is supposed to create as little overhead as possible in the
> non-traced case. All of the burden is shifted to the trace function (_mcount).

... or its helpers, see below.

> The reason to do that is because modern distros always build with tracing, but
> most of the time tracing will not actually be active. So we want the cost of
> tracing-built-in-but-disabled to be ~zero.

Ok, that's a design goal.

> > That was the alternative I asked about; but given that the _mcount / ftrace_caller
> > trampoline hardly differs from a normal trampoline (so far), loading R2 would be the
> > general case, or an excessive special case handling would result.
> 
> I'm not sure I follow what you mean there at the end.

This suggests you have not yet actively debugged this problem ;-)

> Requiring ftrace_caller() to load the kernel TOC is not a problem IMHO.

The problem is, you don't get to ftrace_caller in the first place :)

> I think I have an easier way to do it, I'll reply to the patch with that (if it
> works).

I doubt so. Either it works, _or_ it is easier ;)

To save you some work: by the design of minimal overhead you try to follow,
SQUASH_TOC_SAVE_INSN from my patches isn't sufficient. You'll need to load the
_current_ TOC _on_ the trampoline, and in turn it will be different from the regular
trampolines; and that needs to be recognised, or the normal module linker logic
won't work.

OTOH my proposed GCC change only affects a very limited number of functions...

Looking forward to your patch!

	Torsten

[toc] | [prev] | [next] | [standalone]

#1331378 — [PATCH v8 3/8] ppc use ftrace_modify_all_code default

Convert ppc's arch_ftrace_update_code from its own function copy
to use the generic default functionality (without stop_machine --
our instructions are properly aligned and the replacements atomic ;)

With this we gain error checking and the much-needed function_trace_op
handling.

Signed-off-by: Torsten Duwe <duwe@suse.de>
---
 arch/powerpc/kernel/ftrace.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 1fad1b3..ef8b916 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -531,20 +531,12 @@ void ftrace_replace_code(int enable)
 	}
 }
 
+/* Use the default ftrace_modify_all_code, but without
+ * stop_machine().
+ */
 void arch_ftrace_update_code(int command)
 {
-	if (command & FTRACE_UPDATE_CALLS)
-		ftrace_replace_code(1);
-	else if (command & FTRACE_DISABLE_CALLS)
-		ftrace_replace_code(0);
-
-	if (command & FTRACE_UPDATE_TRACE_FUNC)
-		ftrace_update_ftrace_func(ftrace_trace_function);
-
-	if (command & FTRACE_START_FUNC_RET)
-		ftrace_enable_ftrace_graph_caller();
-	else if (command & FTRACE_STOP_FUNC_RET)
-		ftrace_disable_ftrace_graph_caller();
+	ftrace_modify_all_code(command);
 }
 
 int __init ftrace_dyn_arch_init(void)
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1331379 — [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel

The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
allows to call _mcount very early in the function, which low-level
ASM code and code patching functions need to consider.
Especially the link register and the parameter registers are still
alive and not yet saved into a new stack frame.

  * arch/powerpc/kernel/entry_64.S:
    - modify the default _mcount to be prepared for such call sites
    - have the ftrace_graph_caller save function arguments before
      calling its C helper prepare_ftrace_return
  * arch/powerpc/include/asm/code-patching.h:
    - define some common macros to make things readable.
    - pull the R2 stack location definition from
      arch/powerpc/kernel/module_64.c
  * arch/powerpc/kernel/module_64.c:
    - enhance binary code examination to handle the new patterns.

Signed-off-by: Torsten Duwe <duwe@suse.de>
---
 arch/powerpc/include/asm/code-patching.h | 24 ++++++++++++++++
 arch/powerpc/kernel/entry_64.S           | 48 +++++++++++++++++++++++++++++++-
 arch/powerpc/kernel/ftrace.c             | 44 ++++++++++++++++++++++-------
 arch/powerpc/kernel/module_64.c          | 31 +++++++++++++++++++--
 4 files changed, 133 insertions(+), 14 deletions(-)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 840a550..7820b32 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -99,4 +99,28 @@ static inline unsigned long ppc_global_function_entry(void *func)
 #endif
 }
 
+#ifdef CONFIG_PPC64
+/* Some instruction encodings commonly used in dynamic ftracing
+ * and function live patching:
+ */
+
+/* This must match the definition of STK_GOT in <asm/ppc_asm.h> */
+#if defined(_CALL_ELF) && _CALL_ELF == 2
+#define R2_STACK_OFFSET         24
+#else
+#define R2_STACK_OFFSET         40
+#endif
+
+/* load / store the TOC from / into the stack frame */
+#define PPC_INST_LD_TOC		(PPC_INST_LD  | ___PPC_RT(__REG_R2) | \
+				 ___PPC_RA(__REG_R1) | R2_STACK_OFFSET)
+#define PPC_INST_STD_TOC	(PPC_INST_STD | ___PPC_RS(__REG_R2) | \
+				 ___PPC_RA(__REG_R1) | R2_STACK_OFFSET)
+
+/* usually preceded by a mflr r0 */
+#define PPC_INST_STD_LR		(PPC_INST_STD | ___PPC_RS(__REG_R0) | \
+				 ___PPC_RA(__REG_R1) | PPC_LR_STKOFF)
+
+#endif /* CONFIG_PPC64 */
+
 #endif /* _ASM_POWERPC_CODE_PATCHING_H */
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 0d525ce..2a7313c 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -1143,7 +1143,10 @@ _GLOBAL(enter_prom)
 #ifdef CONFIG_DYNAMIC_FTRACE
 _GLOBAL(mcount)
 _GLOBAL(_mcount)
-	blr
+	mflr	r12
+	mtctr	r12
+	mtlr	r0
+	bctr
 
 _GLOBAL_TOC(ftrace_caller)
 	/* Taken from output of objdump from lib64/glibc */
@@ -1198,6 +1201,7 @@ _GLOBAL(ftrace_stub)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
+#ifndef CC_USING_MPROFILE_KERNEL
 _GLOBAL(ftrace_graph_caller)
 	/* load r4 with local address */
 	ld	r4, 128(r1)
@@ -1222,6 +1226,48 @@ _GLOBAL(ftrace_graph_caller)
 	addi	r1, r1, 112
 	blr
 
+#else /* CC_USING_MPROFILE_KERNEL */
+_GLOBAL(ftrace_graph_caller)
+	/* with -mprofile-kernel, parameter regs are still alive at _mcount */
+	std	r10, 104(r1)
+	std	r9, 96(r1)
+	std	r8, 88(r1)
+	std	r7, 80(r1)
+	std	r6, 72(r1)
+	std	r5, 64(r1)
+	std	r4, 56(r1)
+	std	r3, 48(r1)
+	mfctr	r4		/* ftrace_caller has moved local addr here */
+	std	r4, 40(r1)
+	mflr	r3		/* ftrace_caller has restored LR from stack */
+	subi	r4, r4, MCOUNT_INSN_SIZE
+
+	bl	prepare_ftrace_return
+	nop
+
+	/*
+	 * prepare_ftrace_return gives us the address we divert to.
+	 * Change the LR to this.
+	 */
+	mtlr	r3
+
+	ld	r0, 40(r1)
+	mtctr	r0
+	ld	r10, 104(r1)
+	ld	r9, 96(r1)
+	ld	r8, 88(r1)
+	ld	r7, 80(r1)
+	ld	r6, 72(r1)
+	ld	r5, 64(r1)
+	ld	r4, 56(r1)
+	ld	r3, 48(r1)
+
+	addi	r1, r1, 112
+	mflr	r0
+	std	r0, LRSAVE(r1)
+	bctr
+#endif /* CC_USING_MPROFILE_KERNEL */
+
 _GLOBAL(return_to_handler)
 	/* need to save return values */
 	std	r4,  -32(r1)
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 44d4d8e..861af90 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c
@@ -287,16 +287,14 @@ int ftrace_make_nop(struct module *mod,
 
 #ifdef CONFIG_MODULES
 #ifdef CONFIG_PPC64
+/* Examine the existing instructions for __ftrace_make_call.
+ * They should effectively be a NOP, and follow formal constraints,
+ * depending on the ABI. Return false if they don't.
+ */
+#ifndef CC_USING_MPROFILE_KERNEL
 static int
-__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
 {
-	unsigned int op[2];
-	void *ip = (void *)rec->ip;
-
-	/* read where this goes */
-	if (probe_kernel_read(op, ip, sizeof(op)))
-		return -EFAULT;
-
 	/*
 	 * We expect to see:
 	 *
@@ -306,8 +304,34 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	 * The load offset is different depending on the ABI. For simplicity
 	 * just mask it out when doing the compare.
 	 */
-	if ((op[0] != 0x48000008) || ((op[1] & 0xffff0000) != 0xe8410000)) {
-		pr_err("Unexpected call sequence: %x %x\n", op[0], op[1]);
+	if ((op0 != 0x48000008) || ((op1 & 0xffff0000) != 0xe8410000))
+		return 0;
+	return 1;
+}
+#else
+static int
+expected_nop_sequence(void *ip, unsigned int op0, unsigned int op1)
+{
+	/* look for patched "NOP" on ppc64 with -mprofile-kernel */
+	if (op0 != PPC_INST_NOP)
+		return 0;
+	return 1;
+}
+#endif
+
+static int
+__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned int op[2];
+	void *ip = (void *)rec->ip;
+
+	/* read where this goes */
+	if (probe_kernel_read(op, ip, sizeof(op)))
+		return -EFAULT;
+
+	if (!expected_nop_sequence(ip, op[0], op[1])) {
+		pr_err("Unexpected call sequence at %p: %x %x\n",
+		ip, op[0], op[1]);
 		return -EINVAL;
 	}
 
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ac64ffd..72a1a52 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -41,7 +41,6 @@
    --RR.  */
 
 #if defined(_CALL_ELF) && _CALL_ELF == 2
-#define R2_STACK_OFFSET 24
 
 /* An address is simply the address of the function. */
 typedef unsigned long func_desc_t;
@@ -73,7 +72,6 @@ static unsigned int local_entry_offset(const Elf64_Sym *sym)
 	return PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
 }
 #else
-#define R2_STACK_OFFSET 40
 
 /* An address is address of the OPD entry, which contains address of fn. */
 typedef struct ppc64_opd_entry func_desc_t;
@@ -476,17 +474,44 @@ static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
 	return (unsigned long)&stubs[i];
 }
 
+#ifdef CC_USING_MPROFILE_KERNEL
+static int is_early_mcount_callsite(u32 *instruction)
+{
+	/* -mprofile-kernel sequence starting with
+	 * mflr r0 and maybe std r0, LRSAVE(r1).
+	 */
+	if ((instruction[-3] == PPC_INST_MFLR &&
+	     instruction[-2] == PPC_INST_STD_LR) ||
+	    instruction[-2] == PPC_INST_MFLR) {
+		/* Nothing to be done here, it's an _mcount
+		 * call location and r2 will have to be
+		 * restored in the _mcount function.
+		 */
+		return 1;
+	}
+	return 0;
+}
+#else
+/* without -mprofile-kernel, mcount calls are never early */
+static int is_early_mcount_callsite(u32 *instruction)
+{
+	return 0;
+}
+#endif
+
 /* We expect a noop next: if it is, replace it with instruction to
    restore r2. */
 static int restore_r2(u32 *instruction, struct module *me)
 {
 	if (*instruction != PPC_INST_NOP) {
+		if (is_early_mcount_callsite(instruction))
+			return 1;
 		pr_err("%s: Expect noop after relocate, got %08x\n",
 		       me->name, *instruction);
 		return 0;
 	}
 	/* ld r2,R2_STACK_OFFSET(r1) */
-	*instruction = 0xe8410000 | R2_STACK_OFFSET;
+	*instruction = PPC_INST_LD_TOC;
 	return 1;
 }
 
-- 
1.8.5.6

[toc] | [prev] | [next] | [standalone]

#1336248 — Re: [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel

On Wed, 2016-02-10 at 17:21 +0100, Torsten Duwe wrote:

> The gcc switch -mprofile-kernel, available for ppc64 on gcc > 4.8.5,
> allows to call _mcount very early in the function, which low-level
> ASM code and code patching functions need to consider.
> Especially the link register and the parameter registers are still
> alive and not yet saved into a new stack frame.

...

> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index ac64ffd..72a1a52 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -476,17 +474,44 @@ static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
>  	return (unsigned long)&stubs[i];
>  }
>  
> +#ifdef CC_USING_MPROFILE_KERNEL
> +static int is_early_mcount_callsite(u32 *instruction)
> +{
> +	/* -mprofile-kernel sequence starting with
> +	 * mflr r0 and maybe std r0, LRSAVE(r1).
> +	 */
> +	if ((instruction[-3] == PPC_INST_MFLR &&
> +	     instruction[-2] == PPC_INST_STD_LR) ||
> +	    instruction[-2] == PPC_INST_MFLR) {
> +		/* Nothing to be done here, it's an _mcount
> +		 * call location and r2 will have to be
> +		 * restored in the _mcount function.
> +		 */
> +		return 1;
> +	}
> +	return 0;
> +}

So this logic to deal with the 2 vs 3 instruction version of the mcount
sequence is problematic.

On a kernel built with the 2 instruction version this will fault when the
function we're looking at is located at the beginning of a page. Because
instruction[-3] goes off the front of the mapping.

We can probably fix that. But it's still a bit dicey.

I'm wondering if we want to just say we only support the 2 instruction version.
Currently that means GCC 6 only, or a distro compiler with the backport of
e95d0248dace. But we could also ask GCC to backport it to 4.9 and 5.

Thoughts?

cheers

[toc] | [prev] | [next] | [standalone]

#1336274 — Re: [PATCH v8 1/8] ppc64 (le): prepare for -mprofile-kernel

On Wed, 2016-02-17 at 12:30 +0100, Torsten Duwe wrote:
> On Wed, Feb 17, 2016 at 09:55:40PM +1100, Michael Ellerman wrote:
> > 
> > On a kernel built with the 2 instruction version this will fault when the
> > function we're looking at is located at the beginning of a page. Because
> > instruction[-3] goes off the front of the mapping.
> > 
> > We can probably fix that. But it's still a bit dicey.
> 
> Not necessarily. Now that it's a separate function, it can be nested a bit deeper,
> so we don't take chances on compiler optimisation:
> 
> if (instruction[-2] == PPC_INST_STD_LR) /* where should R0 come from? there must be... */
>   {
>     if (instruction[-3] == PPC_INST_MFLR)
>       return 1;
>   }
> else if (instruction[-2] == PPC_INST_MFLR)
>     return 1;
> return 0;

Yeah true that should work in practice.

It's still trivial to construct a module that will oops the loader, but I guess
that's always been true.

> > I'm wondering if we want to just say we only support the 2 instruction version.
> > Currently that means GCC 6 only, or a distro compiler with the backport of
> > e95d0248dace. But we could also ask GCC to backport it to 4.9 and 5.
> 
> IMHO that's a too weak reason for a too strong limitation. OTOH getting everyone
> to use the 2 insn version sounds appealing...

Fair enough. I'm just trying to manage the complexity explosion.

I'd certainly advocate that you backport it to your toolchain.

> Is e95d0248dace self-sufficient or does it depend on other improvements?

AFAIK it's self sufficient, it just deletes a single line. I'll ask the GCC
guys tomorrow if they can backport it if you don't beat me to it :)

cheers

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web