2020-02-27 11:49:51

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v4 13/13] powerpc/ptrace: move ptrace_triggered() into hw_breakpoint.c

ptrace_triggered() is declared in asm/hw_breakpoint.h and
only needed when CONFIG_HW_BREAKPOINT is set, so move it
into hw_breakpoint.c

Signed-off-by: Christophe Leroy <[email protected]>
---
v4: removing inclusing of hw_breakpoint.h now. Previously it was done too early.
---
arch/powerpc/kernel/hw_breakpoint.c | 16 ++++++++++++++++
arch/powerpc/kernel/ptrace/ptrace.c | 19 -------------------
2 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 2462cd7c565c..2c0be9d941cf 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -427,3 +427,19 @@ void hw_breakpoint_pmu_read(struct perf_event *bp)
{
/* TODO */
}
+
+void ptrace_triggered(struct perf_event *bp,
+ struct perf_sample_data *data, struct pt_regs *regs)
+{
+ struct perf_event_attr attr;
+
+ /*
+ * Disable the breakpoint request here since ptrace has defined a
+ * one-shot behaviour for breakpoint exceptions in PPC64.
+ * The SIGTRAP signal is generated automatically for us in do_dabr().
+ * We don't have to do anything about that here
+ */
+ attr = bp->attr;
+ attr.disabled = true;
+ modify_user_hw_breakpoint(bp, &attr);
+}
diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
index a44f6e5e05ff..f6e51be47c6e 100644
--- a/arch/powerpc/kernel/ptrace/ptrace.c
+++ b/arch/powerpc/kernel/ptrace/ptrace.c
@@ -18,7 +18,6 @@
#include <linux/regset.h>
#include <linux/tracehook.h>
#include <linux/audit.h>
-#include <linux/hw_breakpoint.h>
#include <linux/context_tracking.h>
#include <linux/syscalls.h>

@@ -31,24 +30,6 @@

#include "ptrace-decl.h"

-#ifdef CONFIG_HAVE_HW_BREAKPOINT
-void ptrace_triggered(struct perf_event *bp,
- struct perf_sample_data *data, struct pt_regs *regs)
-{
- struct perf_event_attr attr;
-
- /*
- * Disable the breakpoint request here since ptrace has defined a
- * one-shot behaviour for breakpoint exceptions in PPC64.
- * The SIGTRAP signal is generated automatically for us in do_dabr().
- * We don't have to do anything about that here
- */
- attr = bp->attr;
- attr.disabled = true;
- modify_user_hw_breakpoint(bp, &attr);
-}
-#endif /* CONFIG_HAVE_HW_BREAKPOINT */
-
/*
* Called by kernel/ptrace.c when detaching..
*
--
2.25.0


2020-02-27 17:10:04

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v4 13/13] powerpc/ptrace: move ptrace_triggered() into hw_breakpoint.c

Russel,

Le 27/02/2020 à 12:49, Christophe Leroy a écrit :
> ptrace_triggered() is declared in asm/hw_breakpoint.h and
> only needed when CONFIG_HW_BREAKPOINT is set, so move it
> into hw_breakpoint.c

My series v4 is definitely buggy (I included ptrace_decl.h instead
instead of ptrace-decl.h), how can Snowpatch say build succeeded
(https://patchwork.ozlabs.org/patch/1245807/) ?

It fails at least on pmac32_defconfig and ppc64_defconfig, see:

http://kisskb.ellerman.id.au/kisskb/head/d45c91cf5f83424b8f3989b7ead28c50d8d765a9/

Christophe

>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> v4: removing inclusing of hw_breakpoint.h now. Previously it was done too early.
> ---
> arch/powerpc/kernel/hw_breakpoint.c | 16 ++++++++++++++++
> arch/powerpc/kernel/ptrace/ptrace.c | 19 -------------------
> 2 files changed, 16 insertions(+), 19 deletions(-)
>
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 2462cd7c565c..2c0be9d941cf 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -427,3 +427,19 @@ void hw_breakpoint_pmu_read(struct perf_event *bp)
> {
> /* TODO */
> }
> +
> +void ptrace_triggered(struct perf_event *bp,
> + struct perf_sample_data *data, struct pt_regs *regs)
> +{
> + struct perf_event_attr attr;
> +
> + /*
> + * Disable the breakpoint request here since ptrace has defined a
> + * one-shot behaviour for breakpoint exceptions in PPC64.
> + * The SIGTRAP signal is generated automatically for us in do_dabr().
> + * We don't have to do anything about that here
> + */
> + attr = bp->attr;
> + attr.disabled = true;
> + modify_user_hw_breakpoint(bp, &attr);
> +}
> diff --git a/arch/powerpc/kernel/ptrace/ptrace.c b/arch/powerpc/kernel/ptrace/ptrace.c
> index a44f6e5e05ff..f6e51be47c6e 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace.c
> @@ -18,7 +18,6 @@
> #include <linux/regset.h>
> #include <linux/tracehook.h>
> #include <linux/audit.h>
> -#include <linux/hw_breakpoint.h>
> #include <linux/context_tracking.h>
> #include <linux/syscalls.h>
>
> @@ -31,24 +30,6 @@
>
> #include "ptrace-decl.h"
>
> -#ifdef CONFIG_HAVE_HW_BREAKPOINT
> -void ptrace_triggered(struct perf_event *bp,
> - struct perf_sample_data *data, struct pt_regs *regs)
> -{
> - struct perf_event_attr attr;
> -
> - /*
> - * Disable the breakpoint request here since ptrace has defined a
> - * one-shot behaviour for breakpoint exceptions in PPC64.
> - * The SIGTRAP signal is generated automatically for us in do_dabr().
> - * We don't have to do anything about that here
> - */
> - attr = bp->attr;
> - attr.disabled = true;
> - modify_user_hw_breakpoint(bp, &attr);
> -}
> -#endif /* CONFIG_HAVE_HW_BREAKPOINT */
> -
> /*
> * Called by kernel/ptrace.c when detaching..
> *
>

2020-02-27 22:17:06

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v4 13/13] powerpc/ptrace: move ptrace_triggered() into hw_breakpoint.c

Christophe Leroy <[email protected]> writes:
> Russel,
>
> Le 27/02/2020 à 12:49, Christophe Leroy a écrit :
>> ptrace_triggered() is declared in asm/hw_breakpoint.h and
>> only needed when CONFIG_HW_BREAKPOINT is set, so move it
>> into hw_breakpoint.c
>
> My series v4 is definitely buggy (I included ptrace_decl.h instead
> instead of ptrace-decl.h), how can Snowpatch say build succeeded
> (https://patchwork.ozlabs.org/patch/1245807/) ?

Which links to:
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895//artifact/linux/report.txt

The actual build log of which is:
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895/artifact/linux/build_new.log

Which contains:
scripts/Makefile.build:267: recipe for target 'arch/powerpc/kernel/ptrace/ptrace-altivec.o' failed
make[3]: *** [arch/powerpc/kernel/ptrace/ptrace-altivec.o] Error 1
make[3]: *** Waiting for unfinished jobs....
scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel/ptrace' failed
make[2]: *** [arch/powerpc/kernel/ptrace] Error 2
make[2]: *** Waiting for unfinished jobs....
scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel' failed
make[1]: *** [arch/powerpc/kernel] Error 2
make[1]: *** Waiting for unfinished jobs....
Makefile:1681: recipe for target 'arch/powerpc' failed
make: *** [arch/powerpc] Error 2
make: *** Waiting for unfinished jobs....

Same for ppc64le:
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15896/artifact/linux/build_new.log


So it seems like snowpatch always reports the build as succeeded even
when it fails.

cheers

2020-02-28 04:00:53

by Andrew Donnellan

[permalink] [raw]
Subject: Re: [PATCH v4 13/13] powerpc/ptrace: move ptrace_triggered() into hw_breakpoint.c

On 28/2/20 9:16 am, Michael Ellerman wrote:
> Christophe Leroy <[email protected]> writes:
>> Russel,
>>
>> Le 27/02/2020 à 12:49, Christophe Leroy a écrit :
>>> ptrace_triggered() is declared in asm/hw_breakpoint.h and
>>> only needed when CONFIG_HW_BREAKPOINT is set, so move it
>>> into hw_breakpoint.c
>>
>> My series v4 is definitely buggy (I included ptrace_decl.h instead
>> instead of ptrace-decl.h), how can Snowpatch say build succeeded
>> (https://patchwork.ozlabs.org/patch/1245807/) ?
>
> Which links to:
> https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895//artifact/linux/report.txt
>
> The actual build log of which is:
> https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895/artifact/linux/build_new.log
>
> Which contains:
> scripts/Makefile.build:267: recipe for target 'arch/powerpc/kernel/ptrace/ptrace-altivec.o' failed
> make[3]: *** [arch/powerpc/kernel/ptrace/ptrace-altivec.o] Error 1
> make[3]: *** Waiting for unfinished jobs....
> scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel/ptrace' failed
> make[2]: *** [arch/powerpc/kernel/ptrace] Error 2
> make[2]: *** Waiting for unfinished jobs....
> scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel' failed
> make[1]: *** [arch/powerpc/kernel] Error 2
> make[1]: *** Waiting for unfinished jobs....
> Makefile:1681: recipe for target 'arch/powerpc' failed
> make: *** [arch/powerpc] Error 2
> make: *** Waiting for unfinished jobs....
>
> Same for ppc64le:
> https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15896/artifact/linux/build_new.log
>
>
> So it seems like snowpatch always reports the build as succeeded even
> when it fails.

Turns out there was an issue in a recent change in our build script
which caused build failures to return the wrong exit code and put the
wrong text in the reports, because of some confusion with bash
subshells. I've fixed it (I think).

--
Andrew Donnellan OzLabs, ADL Canberra
[email protected] IBM Australia Limited

2020-02-28 04:03:26

by Russell Currey

[permalink] [raw]
Subject: Re: [PATCH v4 13/13] powerpc/ptrace: move ptrace_triggered() into hw_breakpoint.c

This was my fault, I should really test changes like these before they go live. Apologies for the confusion caused

--
Russell Currey
[email protected]

On Fri, Feb 28, 2020, at 2:59 PM, Andrew Donnellan wrote:
> On 28/2/20 9:16 am, Michael Ellerman wrote:
> > Christophe Leroy <[email protected]> writes:
> >> Russel,
> >>
> >> Le 27/02/2020 à 12:49, Christophe Leroy a écrit :
> >>> ptrace_triggered() is declared in asm/hw_breakpoint.h and
> >>> only needed when CONFIG_HW_BREAKPOINT is set, so move it
> >>> into hw_breakpoint.c
> >>
> >> My series v4 is definitely buggy (I included ptrace_decl.h instead
> >> instead of ptrace-decl.h), how can Snowpatch say build succeeded
> >> (https://patchwork.ozlabs.org/patch/1245807/) ?
> >
> > Which links to:
> > https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895//artifact/linux/report.txt
> >
> > The actual build log of which is:
> > https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15895/artifact/linux/build_new.log
> >
> > Which contains:
> > scripts/Makefile.build:267: recipe for target 'arch/powerpc/kernel/ptrace/ptrace-altivec.o' failed
> > make[3]: *** [arch/powerpc/kernel/ptrace/ptrace-altivec.o] Error 1
> > make[3]: *** Waiting for unfinished jobs....
> > scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel/ptrace' failed
> > make[2]: *** [arch/powerpc/kernel/ptrace] Error 2
> > make[2]: *** Waiting for unfinished jobs....
> > scripts/Makefile.build:505: recipe for target 'arch/powerpc/kernel' failed
> > make[1]: *** [arch/powerpc/kernel] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > Makefile:1681: recipe for target 'arch/powerpc' failed
> > make: *** [arch/powerpc] Error 2
> > make: *** Waiting for unfinished jobs....
> >
> > Same for ppc64le:
> > https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/15896/artifact/linux/build_new.log
> >
> >
> > So it seems like snowpatch always reports the build as succeeded even
> > when it fails.
>
> Turns out there was an issue in a recent change in our build script
> which caused build failures to return the wrong exit code and put the
> wrong text in the reports, because of some confusion with bash
> subshells. I've fixed it (I think).
>
> --
> Andrew Donnellan OzLabs, ADL Canberra
> [email protected] IBM Australia Limited
>
>