* Re: [PATCH v2] powerpc: fix EDEADLOCK redefinition error in uapi/asm/errno.h
From: Arnd Bergmann @ 2020-09-17 14:01 UTC (permalink / raw)
To: Michael Ellerman
Cc: linux-arch, Tony Ambardar, linux-kernel@vger.kernel.org,
Paul Mackerras, Rosen Penev, bpf, linuxppc-dev
In-Reply-To: <87363gpqhz.fsf@mpe.ellerman.id.au>
On Thu, Sep 17, 2020 at 1:55 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> [ Cc += linux-arch & Arnd ]
>
> Hi Tony,
>
> This looks OK to me, but I'm always a bit nervous about changes in uapi.
> I've Cc'ed linux-arch and Arnd who look after the asm-generic headers,
> which this is slightly related to, just in case.
>
> One minor comment below.
>
> Tony Ambardar <tony.ambardar@gmail.com> writes:
> > A few archs like powerpc have different errno.h values for macros
> > EDEADLOCK and EDEADLK. In code including both libc and linux versions of
> > errno.h, this can result in multiple definitions of EDEADLOCK in the
> > include chain. Definitions to the same value (e.g. seen with mips) do
> > not raise warnings, but on powerpc there are redefinitions changing the
> > value, which raise warnings and errors (if using "-Werror").
> >
> > Guard against these redefinitions to avoid build errors like the following,
> > first seen cross-compiling libbpf v5.8.9 for powerpc using GCC 8.4.0 with
> > musl 1.1.24:
> >
> > In file included from ../../arch/powerpc/include/uapi/asm/errno.h:5,
> > from ../../include/linux/err.h:8,
> > from libbpf.c:29:
> > ../../include/uapi/asm-generic/errno.h:40: error: "EDEADLOCK" redefined [-Werror]
> > #define EDEADLOCK EDEADLK
> >
> > In file included from toolchain-powerpc_8540_gcc-8.4.0_musl/include/errno.h:10,
> > from libbpf.c:26:
> > toolchain-powerpc_8540_gcc-8.4.0_musl/include/bits/errno.h:58: note: this is the location of the previous definition
> > #define EDEADLOCK 58
> >
> > cc1: all warnings being treated as errors
> >
> > Fixes: 95f28190aa01 ("tools include arch: Grab a copy of errno.h for arch's supported by perf")
> > Fixes: c3617f72036c ("UAPI: (Scripted) Disintegrate arch/powerpc/include/asm")
>
> I suspect that's not the right commit to tag. It just moved errno.h from
> arch/powerpc/include/asm to arch/powerpc/include/uapi/asm. It's content
> was almost identical, and entirely identical as far as EDEADLOCK was
> concerned.
>
> Prior to that the file lived in asm-powerpc/errno.h, eg:
>
> $ git cat-file -p b8b572e1015f^:include/asm-powerpc/errno.h
>
> Before that it was include/asm-ppc64/errno.h, content still the same.
>
> To go back further we'd have to look at the historical git trees, which
> is probably overkill. I'm pretty sure it's always had this problem.
>
> So we should probably drop the Fixes tags and just Cc: stable, that
> means please backport it as far back as possible.
I can see that the two numbers (35 and 58) were consistent across
multiple architectures (i386, m68k, ppc32) up to linux-2.0.1, while
other architectures had two unique numbers (alpha, mips, sparc)
at the time, and sparc had BSD and Solaris compatible numbers
in addition.
In linux-2.0.2, alpha and i386 got changed to use 35 for both,
but the other architectures remained unchanged. All later
architectures followed x86 in using the same number for both.
I foudn a message about tcl breaking at compile time when
it changed:
http://lkml.iu.edu/hypermail/linux/kernel/9607.3/0500.html
The errno man page says they are supposed to be synonyms,
and glibc defines it that way, while musl uses the numbers
from the kernel.
Arnd
^ permalink raw reply
* [PATCH v3] powerpc: fix EDEADLOCK redefinition error in uapi/asm/errno.h
From: Tony Ambardar @ 2020-09-17 13:54 UTC (permalink / raw)
To: Michael Ellerman
Cc: linux-arch, Tony Ambardar, Arnd Bergmann, linux-kernel, Stable,
Paul Mackerras, Rosen Penev, bpf, linuxppc-dev
In-Reply-To: <20200917000757.1232850-1-Tony.Ambardar@gmail.com>
A few archs like powerpc have different errno.h values for macros
EDEADLOCK and EDEADLK. In code including both libc and linux versions of
errno.h, this can result in multiple definitions of EDEADLOCK in the
include chain. Definitions to the same value (e.g. seen with mips) do
not raise warnings, but on powerpc there are redefinitions changing the
value, which raise warnings and errors (if using "-Werror").
Guard against these redefinitions to avoid build errors like the following,
first seen cross-compiling libbpf v5.8.9 for powerpc using GCC 8.4.0 with
musl 1.1.24:
In file included from ../../arch/powerpc/include/uapi/asm/errno.h:5,
from ../../include/linux/err.h:8,
from libbpf.c:29:
../../include/uapi/asm-generic/errno.h:40: error: "EDEADLOCK" redefined [-Werror]
#define EDEADLOCK EDEADLK
In file included from toolchain-powerpc_8540_gcc-8.4.0_musl/include/errno.h:10,
from libbpf.c:26:
toolchain-powerpc_8540_gcc-8.4.0_musl/include/bits/errno.h:58: note: this is the location of the previous definition
#define EDEADLOCK 58
cc1: all warnings being treated as errors
CC: Stable <stable@vger.kernel.org>
Reported-by: Rosen Penev <rosenp@gmail.com>
Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com>
---
v1 -> v2:
* clean up commit description formatting
v2 -> v3: (per Michael Ellerman)
* drop indeterminate 'Fixes' tags, request stable backports instead
---
arch/powerpc/include/uapi/asm/errno.h | 1 +
tools/arch/powerpc/include/uapi/asm/errno.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/powerpc/include/uapi/asm/errno.h b/arch/powerpc/include/uapi/asm/errno.h
index cc79856896a1..4ba87de32be0 100644
--- a/arch/powerpc/include/uapi/asm/errno.h
+++ b/arch/powerpc/include/uapi/asm/errno.h
@@ -2,6 +2,7 @@
#ifndef _ASM_POWERPC_ERRNO_H
#define _ASM_POWERPC_ERRNO_H
+#undef EDEADLOCK
#include <asm-generic/errno.h>
#undef EDEADLOCK
diff --git a/tools/arch/powerpc/include/uapi/asm/errno.h b/tools/arch/powerpc/include/uapi/asm/errno.h
index cc79856896a1..4ba87de32be0 100644
--- a/tools/arch/powerpc/include/uapi/asm/errno.h
+++ b/tools/arch/powerpc/include/uapi/asm/errno.h
@@ -2,6 +2,7 @@
#ifndef _ASM_POWERPC_ERRNO_H
#define _ASM_POWERPC_ERRNO_H
+#undef EDEADLOCK
#include <asm-generic/errno.h>
#undef EDEADLOCK
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver
From: Mark Brown @ 2020-09-17 13:53 UTC (permalink / raw)
To: Viorel Suman (OSS)
Cc: devicetree, alsa-devel, Matthias Schiffer, Viorel Suman,
Timur Tabi, Xiubo Li, Shengjiu Wang, linuxppc-dev, Takashi Iwai,
Liam Girdwood, Nicolin Chen, Rob Herring, NXP Linux Team,
Philipp Zabel, Viorel Suman, Cosmin-Gabriel Samoila,
Jaroslav Kysela, Fabio Estevam, linux-kernel
In-Reply-To: <1600247876-8013-2-git-send-email-viorel.suman@oss.nxp.com>
[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]
On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
This looks mostly good, a few smallish things below but nothing major.
> +static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr)
> +{
> + struct device *dev = &xcvr->pdev->dev;
> + const struct firmware *fw;
> + int ret = 0, rem, off, out, page = 0, size = FSL_XCVR_REG_OFFSET;
> + u32 mask, val;
> +
> + ret = request_firmware(&fw, xcvr->fw_name, dev);
> + if (ret) {
> + dev_err(dev, "failed to request firmware.\n");
> + return ret;
> + }
> +
> + rem = fw->size;
It would be good to see some explicit validation of the image size, at
least printing an error message if the image is bigger than can be
loaded. The code should be safe in that it won't overflow the device
region it's writing to but it feels like it'd be better to tell people
if we spot a problem rather than just silently truncating the file.
> + /* RAM is 20KiB => max 10 pages 2KiB each */
> + for (page = 0; page < 10; page++) {
> + ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_CTRL,
> + FSL_XCVR_EXT_CTRL_PAGE_MASK,
> + FSL_XCVR_EXT_CTRL_PAGE(page));
regmap does have paging support, though given that this is currently the
only place where paging is used this probably doesn't matter too much.
> +static irqreturn_t irq0_isr(int irq, void *devid)
> +{
> + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> + struct device *dev = &xcvr->pdev->dev;
> + struct regmap *regmap = xcvr->regmap;
> + void __iomem *reg_ctrl, *reg_buff;
> + u32 isr, val, i;
> +
> + regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr);
> + regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr);
This will unconditionally clear any interrupts, even those we don't
understand - it might be better to only clear bits that are supported so
the IRQ core can complain if there's something unexpected showing up.
> + if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR)
> + dev_dbg(dev, "RX/TX FIFO full/empty\n");
Should this be dev_err()?
> +static irqreturn_t irq1_isr(int irq, void *devid)
> +{
> + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> + struct device *dev = &xcvr->pdev->dev;
> +
> + dev_dbg(dev, "irq[1]: %d\n", irq);
> +
> + return IRQ_HANDLED;
> +}
Is there any value in even requesting this and irq2 given the lack of
meaningful handling?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v2] powerpc: fix EDEADLOCK redefinition error in uapi/asm/errno.h
From: Tony Ambardar @ 2020-09-17 13:42 UTC (permalink / raw)
To: Michael Ellerman
Cc: linux-arch, Arnd Bergmann, linux-kernel, Rosen Penev,
Paul Mackerras, bpf, linuxppc-dev
In-Reply-To: <87363gpqhz.fsf@mpe.ellerman.id.au>
On Thu, 17 Sep 2020 at 04:55, Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> [ Cc += linux-arch & Arnd ]
>
> Hi Tony,
>
> This looks OK to me, but I'm always a bit nervous about changes in uapi.
> I've Cc'ed linux-arch and Arnd who look after the asm-generic headers,
> which this is slightly related to, just in case.
>
I agree with the caution and would welcome any other insights.
> One minor comment below.
>
> Tony Ambardar <tony.ambardar@gmail.com> writes:
> > A few archs like powerpc have different errno.h values for macros
> > EDEADLOCK and EDEADLK. In code including both libc and linux versions of
> > errno.h, this can result in multiple definitions of EDEADLOCK in the
> > include chain. Definitions to the same value (e.g. seen with mips) do
> > not raise warnings, but on powerpc there are redefinitions changing the
> > value, which raise warnings and errors (if using "-Werror").
> >
> > Guard against these redefinitions to avoid build errors like the following,
> > first seen cross-compiling libbpf v5.8.9 for powerpc using GCC 8.4.0 with
> > musl 1.1.24:
> >
> > In file included from ../../arch/powerpc/include/uapi/asm/errno.h:5,
> > from ../../include/linux/err.h:8,
> > from libbpf.c:29:
> > ../../include/uapi/asm-generic/errno.h:40: error: "EDEADLOCK" redefined [-Werror]
> > #define EDEADLOCK EDEADLK
> >
> > In file included from toolchain-powerpc_8540_gcc-8.4.0_musl/include/errno.h:10,
> > from libbpf.c:26:
> > toolchain-powerpc_8540_gcc-8.4.0_musl/include/bits/errno.h:58: note: this is the location of the previous definition
> > #define EDEADLOCK 58
> >
> > cc1: all warnings being treated as errors
> >
> > Fixes: 95f28190aa01 ("tools include arch: Grab a copy of errno.h for arch's supported by perf")
> > Fixes: c3617f72036c ("UAPI: (Scripted) Disintegrate arch/powerpc/include/asm")
>
> I suspect that's not the right commit to tag. It just moved errno.h from
> arch/powerpc/include/asm to arch/powerpc/include/uapi/asm. It's content
> was almost identical, and entirely identical as far as EDEADLOCK was
> concerned.
>
> Prior to that the file lived in asm-powerpc/errno.h, eg:
>
> $ git cat-file -p b8b572e1015f^:include/asm-powerpc/errno.h
>
> Before that it was include/asm-ppc64/errno.h, content still the same.
>
> To go back further we'd have to look at the historical git trees, which
> is probably overkill. I'm pretty sure it's always had this problem.
>
> So we should probably drop the Fixes tags and just Cc: stable, that
> means please backport it as far back as possible.
>
Yes, you're right. Those two commits were simply where I stopped tracing back
the long chain. I'll drop them as you suggest and request a backport instead in
the next version of the patch.
Thanks for your feedback!
> cheers
>
>
> > Reported-by: Rosen Penev <rosenp@gmail.com>
> > Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com>
> > ---
> > v1 -> v2:
> > * clean up commit description formatting
> > ---
> > arch/powerpc/include/uapi/asm/errno.h | 1 +
> > tools/arch/powerpc/include/uapi/asm/errno.h | 1 +
> > 2 files changed, 2 insertions(+)
> >
> > diff --git a/arch/powerpc/include/uapi/asm/errno.h b/arch/powerpc/include/uapi/asm/errno.h
> > index cc79856896a1..4ba87de32be0 100644
> > --- a/arch/powerpc/include/uapi/asm/errno.h
> > +++ b/arch/powerpc/include/uapi/asm/errno.h
> > @@ -2,6 +2,7 @@
> > #ifndef _ASM_POWERPC_ERRNO_H
> > #define _ASM_POWERPC_ERRNO_H
> >
> > +#undef EDEADLOCK
> > #include <asm-generic/errno.h>
> >
> > #undef EDEADLOCK
> > diff --git a/tools/arch/powerpc/include/uapi/asm/errno.h b/tools/arch/powerpc/include/uapi/asm/errno.h
> > index cc79856896a1..4ba87de32be0 100644
> > --- a/tools/arch/powerpc/include/uapi/asm/errno.h
> > +++ b/tools/arch/powerpc/include/uapi/asm/errno.h
> > @@ -2,6 +2,7 @@
> > #ifndef _ASM_POWERPC_ERRNO_H
> > #define _ASM_POWERPC_ERRNO_H
> >
> > +#undef EDEADLOCK
> > #include <asm-generic/errno.h>
> >
> > #undef EDEADLOCK
> > --
> > 2.25.1
^ permalink raw reply
* Re: [PATCH v6 8/8] powerpc/watchpoint/selftests: Tests for kernel accessing user memory
From: Rogerio Alves @ 2020-09-17 13:26 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-9-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> Introduce tests to cover simple scenarios where user is watching
> memory which can be accessed by kernel as well. We also support
> _MODE_EXACT with _SETHWDEBUG interface. Move those testcases out-
> side of _BP_RANGE condition. This will help to test _MODE_EXACT
> scenarios when CONFIG_HAVE_HW_BREAKPOINT is not set, eg:
>
> $ ./ptrace-hwbreak
> ...
> PTRACE_SET_DEBUGREG, Kernel Access Userspace, len: 8: Ok
> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, WO, len: 1: Ok
> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, RO, len: 1: Ok
> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, RW, len: 1: Ok
> PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace, len: 1: Ok
> success: ptrace-hwbreak
>
> Suggested-by: Pedro Miraglia Franco de Carvalho <pedromfc@linux.ibm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
> .../selftests/powerpc/ptrace/ptrace-hwbreak.c | 48 ++++++++++++++++++-
> 1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c b/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
> index fc477dfe86a2..2e0d86e0687e 100644
> --- a/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
> +++ b/tools/testing/selftests/powerpc/ptrace/ptrace-hwbreak.c
> @@ -20,6 +20,8 @@
> #include <signal.h>
> #include <sys/types.h>
> #include <sys/wait.h>
> +#include <sys/syscall.h>
> +#include <linux/limits.h>
> #include "ptrace.h"
>
> #define SPRN_PVR 0x11F
> @@ -44,6 +46,7 @@ struct gstruct {
> };
> static volatile struct gstruct gstruct __attribute__((aligned(512)));
>
> +static volatile char cwd[PATH_MAX] __attribute__((aligned(8)));
>
> static void get_dbginfo(pid_t child_pid, struct ppc_debug_info *dbginfo)
> {
> @@ -138,6 +141,9 @@ static void test_workload(void)
> write_var(len);
> }
>
> + /* PTRACE_SET_DEBUGREG, Kernel Access Userspace test */
> + syscall(__NR_getcwd, &cwd, PATH_MAX);
> +
> /* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, WO test */
> write_var(1);
>
> @@ -150,6 +156,9 @@ static void test_workload(void)
> else
> read_var(1);
>
> + /* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace test */
> + syscall(__NR_getcwd, &cwd, PATH_MAX);
> +
> /* PPC_PTRACE_SETHWDEBUG, MODE_RANGE, DW ALIGNED, WO test */
> gstruct.a[rand() % A_LEN] = 'a';
>
> @@ -293,6 +302,24 @@ static int test_set_debugreg(pid_t child_pid)
> return 0;
> }
>
> +static int test_set_debugreg_kernel_userspace(pid_t child_pid)
> +{
> + unsigned long wp_addr = (unsigned long)cwd;
> + char *name = "PTRACE_SET_DEBUGREG";
> +
> + /* PTRACE_SET_DEBUGREG, Kernel Access Userspace test */
> + wp_addr &= ~0x7UL;
> + wp_addr |= (1Ul << DABR_READ_SHIFT);
> + wp_addr |= (1UL << DABR_WRITE_SHIFT);
> + wp_addr |= (1UL << DABR_TRANSLATION_SHIFT);
> + ptrace_set_debugreg(child_pid, wp_addr);
> + ptrace(PTRACE_CONT, child_pid, NULL, 0);
> + check_success(child_pid, name, "Kernel Access Userspace", wp_addr, 8);
> +
> + ptrace_set_debugreg(child_pid, 0);
> + return 0;
> +}
> +
> static void get_ppc_hw_breakpoint(struct ppc_hw_breakpoint *info, int type,
> unsigned long addr, int len)
> {
> @@ -338,6 +365,22 @@ static void test_sethwdebug_exact(pid_t child_pid)
> ptrace_delhwdebug(child_pid, wh);
> }
>
> +static void test_sethwdebug_exact_kernel_userspace(pid_t child_pid)
> +{
> + struct ppc_hw_breakpoint info;
> + unsigned long wp_addr = (unsigned long)&cwd;
> + char *name = "PPC_PTRACE_SETHWDEBUG, MODE_EXACT";
> + int len = 1; /* hardcoded in kernel */
> + int wh;
> +
> + /* PPC_PTRACE_SETHWDEBUG, MODE_EXACT, Kernel Access Userspace test */
> + get_ppc_hw_breakpoint(&info, PPC_BREAKPOINT_TRIGGER_WRITE, wp_addr, 0);
> + wh = ptrace_sethwdebug(child_pid, &info);
> + ptrace(PTRACE_CONT, child_pid, NULL, 0);
> + check_success(child_pid, name, "Kernel Access Userspace", wp_addr, len);
> + ptrace_delhwdebug(child_pid, wh);
> +}
> +
> static void test_sethwdebug_range_aligned(pid_t child_pid)
> {
> struct ppc_hw_breakpoint info;
> @@ -452,9 +495,10 @@ static void
> run_tests(pid_t child_pid, struct ppc_debug_info *dbginfo, bool dawr)
> {
> test_set_debugreg(child_pid);
> + test_set_debugreg_kernel_userspace(child_pid);
> + test_sethwdebug_exact(child_pid);
> + test_sethwdebug_exact_kernel_userspace(child_pid);
> if (dbginfo->features & PPC_DEBUG_FEATURE_DATA_BP_RANGE) {
> - test_sethwdebug_exact(child_pid);
> -
> test_sethwdebug_range_aligned(child_pid);
> if (dawr || is_8xx) {
> test_sethwdebug_range_unaligned(child_pid);
>
^ permalink raw reply
* Re: [PATCH v6 7/8] powerpc/watchpoint/ptrace: Introduce PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
From: Rogerio Alves @ 2020-09-17 13:26 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-8-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 can be used to determine whether
> we are running on an ISA 3.1 compliant machine. Which is needed to
> determine DAR behaviour, 512 byte boundary limit etc. This was
> requested by Pedro Miraglia Franco de Carvalho for extending
> watchpoint features in gdb. Note that availability of 2nd DAWR is
> independent of this flag and should be checked using
> ppc_debug_info->num_data_bps.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
> Documentation/powerpc/ptrace.rst | 1 +
> arch/powerpc/include/uapi/asm/ptrace.h | 1 +
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 ++
> 3 files changed, 4 insertions(+)
>
> diff --git a/Documentation/powerpc/ptrace.rst b/Documentation/powerpc/ptrace.rst
> index 864d4b6dddd1..77725d69eb4a 100644
> --- a/Documentation/powerpc/ptrace.rst
> +++ b/Documentation/powerpc/ptrace.rst
> @@ -46,6 +46,7 @@ features will have bits indicating whether there is support for::
> #define PPC_DEBUG_FEATURE_DATA_BP_RANGE 0x4
> #define PPC_DEBUG_FEATURE_DATA_BP_MASK 0x8
> #define PPC_DEBUG_FEATURE_DATA_BP_DAWR 0x10
> + #define PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 0x20
>
> 2. PTRACE_SETHWDEBUG
>
> diff --git a/arch/powerpc/include/uapi/asm/ptrace.h b/arch/powerpc/include/uapi/asm/ptrace.h
> index f5f1ccc740fc..7004cfea3f5f 100644
> --- a/arch/powerpc/include/uapi/asm/ptrace.h
> +++ b/arch/powerpc/include/uapi/asm/ptrace.h
> @@ -222,6 +222,7 @@ struct ppc_debug_info {
> #define PPC_DEBUG_FEATURE_DATA_BP_RANGE 0x0000000000000004
> #define PPC_DEBUG_FEATURE_DATA_BP_MASK 0x0000000000000008
> #define PPC_DEBUG_FEATURE_DATA_BP_DAWR 0x0000000000000010
> +#define PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 0x0000000000000020
>
> #ifndef __ASSEMBLY__
>
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 48c52426af80..aa36fcad36cd 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -57,6 +57,8 @@ void ppc_gethwdinfo(struct ppc_debug_info *dbginfo)
> } else {
> dbginfo->features = 0;
> }
> + if (cpu_has_feature(CPU_FTR_ARCH_31))
> + dbginfo->features |= PPC_DEBUG_FEATURE_DATA_BP_ARCH_31;
> }
>
> int ptrace_get_debugreg(struct task_struct *child, unsigned long addr,
>
^ permalink raw reply
* Re: [PATCH v6 6/8] powerpc/watchpoint: Add hw_len wherever missing
From: Rogerio Alves @ 2020-09-17 13:26 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-7-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> There are couple of places where we set len but not hw_len. For
> ptrace/perf watchpoints, when CONFIG_HAVE_HW_BREAKPOINT=Y, hw_len
> will be calculated and set internally while parsing watchpoint.
> But when CONFIG_HAVE_HW_BREAKPOINT=N, we need to manually set
> 'hw_len'. Similarly for xmon as well, hw_len needs to be set
> directly.
>
> Fixes: b57aeab811db ("powerpc/watchpoint: Fix length calculation for unaligned target")
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 1 +
> arch/powerpc/xmon/xmon.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index c9122ed91340..48c52426af80 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -219,6 +219,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
> brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
> brk.type = HW_BRK_TYPE_TRANSLATE | HW_BRK_TYPE_PRIV_ALL;
> brk.len = DABR_MAX_LEN;
> + brk.hw_len = DABR_MAX_LEN;
> if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> brk.type |= HW_BRK_TYPE_READ;
> if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index df7bca00f5ec..55c43a6c9111 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -969,6 +969,7 @@ static void insert_cpu_bpts(void)
> brk.address = dabr[i].address;
> brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
> brk.len = 8;
> + brk.hw_len = 8;
> __set_breakpoint(i, &brk);
> }
> }
>
^ permalink raw reply
* Re: [PATCH v6 5/8] powerpc/watchpoint: Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N
From: Rogerio Alves @ 2020-09-17 13:25 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-6-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> On powerpc, ptrace watchpoint works in one-shot mode. i.e. kernel
> disables event every time it fires and user has to re-enable it.
> Also, in case of ptrace watchpoint, kernel notifies ptrace user
> before executing instruction.
>
> With CONFIG_HAVE_HW_BREAKPOINT=N, kernel is missing to disable
> ptrace event and thus it's causing infinite loop of exceptions.
> This is especially harmful when user watches on a data which is
> also read/written by kernel, eg syscall parameters. In such case,
> infinite exceptions happens in kernel mode which causes soft-lockup.
>
> Fixes: 9422de3e953d ("powerpc: Hardware breakpoints rewrite to handle non DABR breakpoint registers")
> Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@linux.ibm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 3 ++
> arch/powerpc/kernel/process.c | 48 +++++++++++++++++++++++
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 4 +-
> 3 files changed, 54 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 81872c420476..abebfbee5b1c 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -18,6 +18,7 @@ struct arch_hw_breakpoint {
> u16 type;
> u16 len; /* length of the target data symbol */
> u16 hw_len; /* length programmed in hw */
> + u8 flags;
> };
>
> /* Note: Don't change the first 6 bits below as they are in the same order
> @@ -37,6 +38,8 @@ struct arch_hw_breakpoint {
> #define HW_BRK_TYPE_PRIV_ALL (HW_BRK_TYPE_USER | HW_BRK_TYPE_KERNEL | \
> HW_BRK_TYPE_HYP)
>
> +#define HW_BRK_FLAG_DISABLED 0x1
> +
> /* Minimum granularity */
> #ifdef CONFIG_PPC_8xx
> #define HW_BREAKPOINT_SIZE 0x4
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 016bd831908e..160fbbf41d40 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -636,6 +636,44 @@ void do_send_trap(struct pt_regs *regs, unsigned long address,
> (void __user *)address);
> }
> #else /* !CONFIG_PPC_ADV_DEBUG_REGS */
> +
> +static void do_break_handler(struct pt_regs *regs)
> +{
> + struct arch_hw_breakpoint null_brk = {0};
> + struct arch_hw_breakpoint *info;
> + struct ppc_inst instr = ppc_inst(0);
> + int type = 0;
> + int size = 0;
> + unsigned long ea;
> + int i;
> +
> + /*
> + * If underneath hw supports only one watchpoint, we know it
> + * caused exception. 8xx also falls into this category.
> + */
> + if (nr_wp_slots() == 1) {
> + __set_breakpoint(0, &null_brk);
> + current->thread.hw_brk[0] = null_brk;
> + current->thread.hw_brk[0].flags |= HW_BRK_FLAG_DISABLED;
> + return;
> + }
> +
> + /* Otherwise findout which DAWR caused exception and disable it. */
> + wp_get_instr_detail(regs, &instr, &type, &size, &ea);
> +
> + for (i = 0; i < nr_wp_slots(); i++) {
> + info = ¤t->thread.hw_brk[i];
> + if (!info->address)
> + continue;
> +
> + if (wp_check_constraints(regs, instr, ea, type, size, info)) {
> + __set_breakpoint(i, &null_brk);
> + current->thread.hw_brk[i] = null_brk;
> + current->thread.hw_brk[i].flags |= HW_BRK_FLAG_DISABLED;
> + }
> + }
> +}
> +
> void do_break (struct pt_regs *regs, unsigned long address,
> unsigned long error_code)
> {
> @@ -647,6 +685,16 @@ void do_break (struct pt_regs *regs, unsigned long address,
> if (debugger_break_match(regs))
> return;
>
> + /*
> + * We reach here only when watchpoint exception is generated by ptrace
> + * event (or hw is buggy!). Now if CONFIG_HAVE_HW_BREAKPOINT is set,
> + * watchpoint is already handled by hw_breakpoint_handler() so we don't
> + * have to do anything. But when CONFIG_HAVE_HW_BREAKPOINT is not set,
> + * we need to manually handle the watchpoint here.
> + */
> + if (!IS_ENABLED(CONFIG_HAVE_HW_BREAKPOINT))
> + do_break_handler(regs);
> +
> /* Deliver the signal to userspace */
> force_sig_fault(SIGTRAP, TRAP_HWBKPT, (void __user *)address);
> }
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 57a0ab822334..c9122ed91340 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -286,11 +286,13 @@ long ppc_del_hwdebug(struct task_struct *child, long data)
> }
> return ret;
> #else /* CONFIG_HAVE_HW_BREAKPOINT */
> - if (child->thread.hw_brk[data - 1].address == 0)
> + if (!(child->thread.hw_brk[data - 1].flags & HW_BRK_FLAG_DISABLED) &&
> + child->thread.hw_brk[data - 1].address == 0)
> return -ENOENT;
>
> child->thread.hw_brk[data - 1].address = 0;
> child->thread.hw_brk[data - 1].type = 0;
> + child->thread.hw_brk[data - 1].flags = 0;
> #endif /* CONFIG_HAVE_HW_BREAKPOINT */
>
> return 0;
>
^ permalink raw reply
* Re: [PATCH v6 4/8] powerpc/watchpoint: Move DAWR detection logic outside of hw_breakpoint.c
From: Rogerio Alves @ 2020-09-17 13:25 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-5-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> Power10 hw has multiple DAWRs but hw doesn't tell which DAWR caused
> the exception. So we have a sw logic to detect that in hw_breakpoint.c.
> But hw_breakpoint.c gets compiled only with CONFIG_HAVE_HW_BREAKPOINT=Y.
> Move DAWR detection logic outside of hw_breakpoint.c so that it can be
> reused when CONFIG_HAVE_HW_BREAKPOINT is not set.
>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 8 +
> arch/powerpc/kernel/Makefile | 3 +-
> arch/powerpc/kernel/hw_breakpoint.c | 159 +----------------
> .../kernel/hw_breakpoint_constraints.c | 162 ++++++++++++++++++
> 4 files changed, 174 insertions(+), 158 deletions(-)
> create mode 100644 arch/powerpc/kernel/hw_breakpoint_constraints.c
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index 9b68eafebf43..81872c420476 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -10,6 +10,7 @@
> #define _PPC_BOOK3S_64_HW_BREAKPOINT_H
>
> #include <asm/cpu_has_feature.h>
> +#include <asm/inst.h>
>
> #ifdef __KERNEL__
> struct arch_hw_breakpoint {
> @@ -52,6 +53,13 @@ static inline int nr_wp_slots(void)
> return cpu_has_feature(CPU_FTR_DAWR1) ? 2 : 1;
> }
>
> +bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr,
> + unsigned long ea, int type, int size,
> + struct arch_hw_breakpoint *info);
> +
> +void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
> + int *type, int *size, unsigned long *ea);
> +
> #ifdef CONFIG_HAVE_HW_BREAKPOINT
> #include <linux/kdebug.h>
> #include <asm/reg.h>
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index cbf41fb4ee89..a5550c2b24c4 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -45,7 +45,8 @@ obj-y := cputable.o syscalls.o \
> signal.o sysfs.o cacheinfo.o time.o \
> prom.o traps.o setup-common.o \
> udbg.o misc.o io.o misc_$(BITS).o \
> - of_platform.o prom_parse.o firmware.o
> + of_platform.o prom_parse.o firmware.o \
> + hw_breakpoint_constraints.o
> obj-y += ptrace/
> obj-$(CONFIG_PPC64) += setup_64.o \
> paca.o nvram_64.o note.o syscall_64.o
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index f6b24838ca3c..f4e8f21046f5 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -494,161 +494,6 @@ void thread_change_pc(struct task_struct *tsk, struct pt_regs *regs)
> }
> }
>
> -static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
> -{
> - return ((info->address <= dar) && (dar - info->address < info->len));
> -}
> -
> -static bool ea_user_range_overlaps(unsigned long ea, int size,
> - struct arch_hw_breakpoint *info)
> -{
> - return ((ea < info->address + info->len) &&
> - (ea + size > info->address));
> -}
> -
> -static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
> -{
> - unsigned long hw_start_addr, hw_end_addr;
> -
> - hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
> - hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
> -
> - return ((hw_start_addr <= dar) && (hw_end_addr > dar));
> -}
> -
> -static bool ea_hw_range_overlaps(unsigned long ea, int size,
> - struct arch_hw_breakpoint *info)
> -{
> - unsigned long hw_start_addr, hw_end_addr;
> - unsigned long align_size = HW_BREAKPOINT_SIZE;
> -
> - /*
> - * On p10 predecessors, quadword is handle differently then
> - * other instructions.
> - */
> - if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
> - align_size = HW_BREAKPOINT_SIZE_QUADWORD;
> -
> - hw_start_addr = ALIGN_DOWN(info->address, align_size);
> - hw_end_addr = ALIGN(info->address + info->len, align_size);
> -
> - return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
> -}
> -
> -/*
> - * If hw has multiple DAWR registers, we also need to check all
> - * dawrx constraint bits to confirm this is _really_ a valid event.
> - * If type is UNKNOWN, but privilege level matches, consider it as
> - * a positive match.
> - */
> -static bool check_dawrx_constraints(struct pt_regs *regs, int type,
> - struct arch_hw_breakpoint *info)
> -{
> - if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
> - return false;
> -
> - /*
> - * The Cache Management instructions other than dcbz never
> - * cause a match. i.e. if type is CACHEOP, the instruction
> - * is dcbz, and dcbz is treated as Store.
> - */
> - if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE))
> - return false;
> -
> - if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
> - return false;
> -
> - if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
> - return false;
> -
> - return true;
> -}
> -
> -/*
> - * Return true if the event is valid wrt dawr configuration,
> - * including extraneous exception. Otherwise return false.
> - */
> -static bool check_constraints(struct pt_regs *regs, struct ppc_inst instr,
> - unsigned long ea, int type, int size,
> - struct arch_hw_breakpoint *info)
> -{
> - bool in_user_range = dar_in_user_range(regs->dar, info);
> - bool dawrx_constraints;
> -
> - /*
> - * 8xx supports only one breakpoint and thus we can
> - * unconditionally return true.
> - */
> - if (IS_ENABLED(CONFIG_PPC_8xx)) {
> - if (!in_user_range)
> - info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> - return true;
> - }
> -
> - if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) {
> - if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> - !dar_in_hw_range(regs->dar, info))
> - return false;
> -
> - return true;
> - }
> -
> - dawrx_constraints = check_dawrx_constraints(regs, type, info);
> -
> - if (type == UNKNOWN) {
> - if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> - !dar_in_hw_range(regs->dar, info))
> - return false;
> -
> - return dawrx_constraints;
> - }
> -
> - if (ea_user_range_overlaps(ea, size, info))
> - return dawrx_constraints;
> -
> - if (ea_hw_range_overlaps(ea, size, info)) {
> - if (dawrx_constraints) {
> - info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> - return true;
> - }
> - }
> - return false;
> -}
> -
> -static int cache_op_size(void)
> -{
> -#ifdef __powerpc64__
> - return ppc64_caches.l1d.block_size;
> -#else
> - return L1_CACHE_BYTES;
> -#endif
> -}
> -
> -static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
> - int *type, int *size, unsigned long *ea)
> -{
> - struct instruction_op op;
> -
> - if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
> - return;
> -
> - analyse_instr(&op, regs, *instr);
> - *type = GETTYPE(op.type);
> - *ea = op.ea;
> -#ifdef __powerpc64__
> - if (!(regs->msr & MSR_64BIT))
> - *ea &= 0xffffffffUL;
> -#endif
> -
> - *size = GETSIZE(op.type);
> - if (*type == CACHEOP) {
> - *size = cache_op_size();
> - *ea &= ~(*size - 1);
> - } else if (*type == LOAD_VMX || *type == STORE_VMX) {
> - *ea &= ~(*size - 1);
> - }
> -}
> -
> static bool is_larx_stcx_instr(int type)
> {
> return type == LARX || type == STCX;
> @@ -732,7 +577,7 @@ int hw_breakpoint_handler(struct die_args *args)
> rcu_read_lock();
>
> if (!IS_ENABLED(CONFIG_PPC_8xx))
> - get_instr_detail(regs, &instr, &type, &size, &ea);
> + wp_get_instr_detail(regs, &instr, &type, &size, &ea);
>
> for (i = 0; i < nr_wp_slots(); i++) {
> bp[i] = __this_cpu_read(bp_per_reg[i]);
> @@ -742,7 +587,7 @@ int hw_breakpoint_handler(struct die_args *args)
> info[i] = counter_arch_bp(bp[i]);
> info[i]->type &= ~HW_BRK_TYPE_EXTRANEOUS_IRQ;
>
> - if (check_constraints(regs, instr, ea, type, size, info[i])) {
> + if (wp_check_constraints(regs, instr, ea, type, size, info[i])) {
> if (!IS_ENABLED(CONFIG_PPC_8xx) &&
> ppc_inst_equal(instr, ppc_inst(0))) {
> handler_error(bp[i], info[i]);
> diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
> new file mode 100644
> index 000000000000..867ee4aa026a
> --- /dev/null
> +++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +#include <linux/kernel.h>
> +#include <linux/uaccess.h>
> +#include <linux/sched.h>
> +#include <asm/hw_breakpoint.h>
> +#include <asm/sstep.h>
> +#include <asm/cache.h>
> +
> +static bool dar_in_user_range(unsigned long dar, struct arch_hw_breakpoint *info)
> +{
> + return ((info->address <= dar) && (dar - info->address < info->len));
> +}
> +
> +static bool ea_user_range_overlaps(unsigned long ea, int size,
> + struct arch_hw_breakpoint *info)
> +{
> + return ((ea < info->address + info->len) &&
> + (ea + size > info->address));
> +}
> +
> +static bool dar_in_hw_range(unsigned long dar, struct arch_hw_breakpoint *info)
> +{
> + unsigned long hw_start_addr, hw_end_addr;
> +
> + hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
> + hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
> +
> + return ((hw_start_addr <= dar) && (hw_end_addr > dar));
> +}
> +
> +static bool ea_hw_range_overlaps(unsigned long ea, int size,
> + struct arch_hw_breakpoint *info)
> +{
> + unsigned long hw_start_addr, hw_end_addr;
> + unsigned long align_size = HW_BREAKPOINT_SIZE;
> +
> + /*
> + * On p10 predecessors, quadword is handle differently then
> + * other instructions.
> + */
> + if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
> + align_size = HW_BREAKPOINT_SIZE_QUADWORD;
> +
> + hw_start_addr = ALIGN_DOWN(info->address, align_size);
> + hw_end_addr = ALIGN(info->address + info->len, align_size);
> +
> + return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
> +}
> +
> +/*
> + * If hw has multiple DAWR registers, we also need to check all
> + * dawrx constraint bits to confirm this is _really_ a valid event.
> + * If type is UNKNOWN, but privilege level matches, consider it as
> + * a positive match.
> + */
> +static bool check_dawrx_constraints(struct pt_regs *regs, int type,
> + struct arch_hw_breakpoint *info)
> +{
> + if (OP_IS_LOAD(type) && !(info->type & HW_BRK_TYPE_READ))
> + return false;
> +
> + /*
> + * The Cache Management instructions other than dcbz never
> + * cause a match. i.e. if type is CACHEOP, the instruction
> + * is dcbz, and dcbz is treated as Store.
> + */
> + if ((OP_IS_STORE(type) || type == CACHEOP) && !(info->type & HW_BRK_TYPE_WRITE))
> + return false;
> +
> + if (is_kernel_addr(regs->nip) && !(info->type & HW_BRK_TYPE_KERNEL))
> + return false;
> +
> + if (user_mode(regs) && !(info->type & HW_BRK_TYPE_USER))
> + return false;
> +
> + return true;
> +}
> +
> +/*
> + * Return true if the event is valid wrt dawr configuration,
> + * including extraneous exception. Otherwise return false.
> + */
> +bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr,
> + unsigned long ea, int type, int size,
> + struct arch_hw_breakpoint *info)
> +{
> + bool in_user_range = dar_in_user_range(regs->dar, info);
> + bool dawrx_constraints;
> +
> + /*
> + * 8xx supports only one breakpoint and thus we can
> + * unconditionally return true.
> + */
> + if (IS_ENABLED(CONFIG_PPC_8xx)) {
> + if (!in_user_range)
> + info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> + return true;
> + }
> +
> + if (unlikely(ppc_inst_equal(instr, ppc_inst(0)))) {
> + if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> + !dar_in_hw_range(regs->dar, info))
> + return false;
> +
> + return true;
> + }
> +
> + dawrx_constraints = check_dawrx_constraints(regs, type, info);
> +
> + if (type == UNKNOWN) {
> + if (cpu_has_feature(CPU_FTR_ARCH_31) &&
> + !dar_in_hw_range(regs->dar, info))
> + return false;
> +
> + return dawrx_constraints;
> + }
> +
> + if (ea_user_range_overlaps(ea, size, info))
> + return dawrx_constraints;
> +
> + if (ea_hw_range_overlaps(ea, size, info)) {
> + if (dawrx_constraints) {
> + info->type |= HW_BRK_TYPE_EXTRANEOUS_IRQ;
> + return true;
> + }
> + }
> + return false;
> +}
> +
> +static int cache_op_size(void)
> +{
> +#ifdef __powerpc64__
> + return ppc64_caches.l1d.block_size;
> +#else
> + return L1_CACHE_BYTES;
> +#endif
> +}
> +
> +void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
> + int *type, int *size, unsigned long *ea)
> +{
> + struct instruction_op op;
> +
> + if (__get_user_instr_inatomic(*instr, (void __user *)regs->nip))
> + return;
> +
> + analyse_instr(&op, regs, *instr);
> + *type = GETTYPE(op.type);
> + *ea = op.ea;
> +#ifdef __powerpc64__
> + if (!(regs->msr & MSR_64BIT))
> + *ea &= 0xffffffffUL;
> +#endif
> +
> + *size = GETSIZE(op.type);
> + if (*type == CACHEOP) {
> + *size = cache_op_size();
> + *ea &= ~(*size - 1);
> + } else if (*type == LOAD_VMX || *type == STORE_VMX) {
> + *ea &= ~(*size - 1);
> + }
> +}
>
^ permalink raw reply
* Re: [PATCH v6 3/8] powerpc/watchpoint/ptrace: Fix SETHWDEBUG when CONFIG_HAVE_HW_BREAKPOINT=N
From: Rogerio Alves @ 2020-09-17 13:25 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-4-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> When kernel is compiled with CONFIG_HAVE_HW_BREAKPOINT=N, user can
> still create watchpoint using PPC_PTRACE_SETHWDEBUG, with limited
> functionalities. But, such watchpoints are never firing because of
> the missing privilege settings. Fix that.
>
> It's safe to set HW_BRK_TYPE_PRIV_ALL because we don't really leak
> any kernel address in signal info. Setting HW_BRK_TYPE_PRIV_ALL will
> also help to find scenarios when kernel accesses user memory.
>
> Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@linux.ibm.com>
> Suggested-by: Pedro Miraglia Franco de Carvalho <pedromfc@linux.ibm.com>
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/ptrace/ptrace-noadv.c b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> index 697c7e4b5877..57a0ab822334 100644
> --- a/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> +++ b/arch/powerpc/kernel/ptrace/ptrace-noadv.c
> @@ -217,7 +217,7 @@ long ppc_set_hwdebug(struct task_struct *child, struct ppc_hw_breakpoint *bp_inf
> return -EIO;
>
> brk.address = ALIGN_DOWN(bp_info->addr, HW_BREAKPOINT_SIZE);
> - brk.type = HW_BRK_TYPE_TRANSLATE;
> + brk.type = HW_BRK_TYPE_TRANSLATE | HW_BRK_TYPE_PRIV_ALL;
> brk.len = DABR_MAX_LEN;
> if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
> brk.type |= HW_BRK_TYPE_READ;
>
^ permalink raw reply
* Re: [PATCH v6 2/8] powerpc/watchpoint: Fix handling of vector instructions
From: Rogerio Alves @ 2020-09-17 13:25 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-3-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> Vector load/store instructions are special because they are always
> aligned. Thus unaligned EA needs to be aligned down before comparing
> it with watch ranges. Otherwise we might consider valid event as
> invalid.
>
> Fixes: 74c6881019b7 ("powerpc/watchpoint: Prepare handler to handle more than one watchpoint")
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
> arch/powerpc/kernel/hw_breakpoint.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 9f7df1c37233..f6b24838ca3c 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -644,6 +644,8 @@ static void get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
> if (*type == CACHEOP) {
> *size = cache_op_size();
> *ea &= ~(*size - 1);
> + } else if (*type == LOAD_VMX || *type == STORE_VMX) {
> + *ea &= ~(*size - 1);
> }
> }
>
>
^ permalink raw reply
* Re: [PATCH v6 1/8] powerpc/watchpoint: Fix quarword instruction handling on p10 predecessors
From: Rogerio Alves @ 2020-09-17 13:25 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-2-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> On p10 predecessors, watchpoint with quarword access is compared at
> quardword length. If the watch range is doubleword or less than that
> in a first half of quarword aligned 16 bytes, and if there is any
> unaligned quadword access which will access only the 2nd half, the
> handler should consider it as extraneous and emulate/single-step it
> before continuing.
>
> Reported-by: Pedro Miraglia Franco de Carvalho <pedromfc@linux.ibm.com>
> Fixes: 74c6881019b7 ("powerpc/watchpoint: Prepare handler to handle more than one watchpoint")
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Tested-by: Rogerio Alves <rcardoso@linux.ibm.com>
> ---
> arch/powerpc/include/asm/hw_breakpoint.h | 1 +
> arch/powerpc/kernel/hw_breakpoint.c | 12 ++++++++++--
> 2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
> index db206a7f38e2..9b68eafebf43 100644
> --- a/arch/powerpc/include/asm/hw_breakpoint.h
> +++ b/arch/powerpc/include/asm/hw_breakpoint.h
> @@ -42,6 +42,7 @@ struct arch_hw_breakpoint {
> #else
> #define HW_BREAKPOINT_SIZE 0x8
> #endif
> +#define HW_BREAKPOINT_SIZE_QUADWORD 0x10
>
> #define DABR_MAX_LEN 8
> #define DAWR_MAX_LEN 512
> diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
> index 1f4a1efa0074..9f7df1c37233 100644
> --- a/arch/powerpc/kernel/hw_breakpoint.c
> +++ b/arch/powerpc/kernel/hw_breakpoint.c
> @@ -520,9 +520,17 @@ static bool ea_hw_range_overlaps(unsigned long ea, int size,
> struct arch_hw_breakpoint *info)
> {
> unsigned long hw_start_addr, hw_end_addr;
> + unsigned long align_size = HW_BREAKPOINT_SIZE;
>
> - hw_start_addr = ALIGN_DOWN(info->address, HW_BREAKPOINT_SIZE);
> - hw_end_addr = ALIGN(info->address + info->len, HW_BREAKPOINT_SIZE);
> + /*
> + * On p10 predecessors, quadword is handle differently then
> + * other instructions.
> + */
> + if (!cpu_has_feature(CPU_FTR_ARCH_31) && size == 16)
> + align_size = HW_BREAKPOINT_SIZE_QUADWORD;
> +
> + hw_start_addr = ALIGN_DOWN(info->address, align_size);
> + hw_end_addr = ALIGN(info->address + info->len, align_size);
>
> return ((ea < hw_end_addr) && (ea + size > hw_start_addr));
> }
>
^ permalink raw reply
* Re: [PATCH v6 0/8] powerpc/watchpoint: Bug fixes plus new feature flag
From: Rogerio Alves @ 2020-09-17 13:24 UTC (permalink / raw)
To: Ravi Bangoria, mpe, christophe.leroy
Cc: mikey, jniethe5, pedromfc, linux-kernel, paulus, rogealve,
naveen.n.rao, linuxppc-dev
In-Reply-To: <20200902042945.129369-1-ravi.bangoria@linux.ibm.com>
On 9/2/20 1:29 AM, Ravi Bangoria wrote:
> Patch #1 fixes issue for quardword instruction on p10 predecessors.
> Patch #2 fixes issue for vector instructions.
> Patch #3 fixes a bug about watchpoint not firing when created with
> ptrace PPC_PTRACE_SETHWDEBUG and CONFIG_HAVE_HW_BREAKPOINT=N.
> The fix uses HW_BRK_TYPE_PRIV_ALL for ptrace user which, I
> guess, should be fine because we don't leak any kernel
> addresses and PRIV_ALL will also help to cover scenarios when
> kernel accesses user memory.
> Patch #4,#5 fixes infinite exception bug, again the bug happens only
> with CONFIG_HAVE_HW_BREAKPOINT=N.
> Patch #6 fixes two places where we are missing to set hw_len.
> Patch #7 introduce new feature bit PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
> which will be set when running on ISA 3.1 compliant machine.
> Patch #8 finally adds selftest to test scenarios fixed by patch#2,#3
> and also moves MODE_EXACT tests outside of BP_RANGE condition.
>
> Christophe, let me know if this series breaks something for 8xx.
>
> v5: https://lore.kernel.org/r/20200825043617.1073634-1-ravi.bangoria@linux.ibm.com
>
> v5->v6:
> - Fix build faulure reported by kernel test robot
> - patch #5. Use more compact if condition, suggested by Christophe
>
>
> Ravi Bangoria (8):
> powerpc/watchpoint: Fix quarword instruction handling on p10
> predecessors
> powerpc/watchpoint: Fix handling of vector instructions
> powerpc/watchpoint/ptrace: Fix SETHWDEBUG when
> CONFIG_HAVE_HW_BREAKPOINT=N
> powerpc/watchpoint: Move DAWR detection logic outside of
> hw_breakpoint.c
> powerpc/watchpoint: Fix exception handling for
> CONFIG_HAVE_HW_BREAKPOINT=N
> powerpc/watchpoint: Add hw_len wherever missing
> powerpc/watchpoint/ptrace: Introduce PPC_DEBUG_FEATURE_DATA_BP_ARCH_31
> powerpc/watchpoint/selftests: Tests for kernel accessing user memory
>
> Documentation/powerpc/ptrace.rst | 1 +
> arch/powerpc/include/asm/hw_breakpoint.h | 12 ++
> arch/powerpc/include/uapi/asm/ptrace.h | 1 +
> arch/powerpc/kernel/Makefile | 3 +-
> arch/powerpc/kernel/hw_breakpoint.c | 149 +---------------
> .../kernel/hw_breakpoint_constraints.c | 162 ++++++++++++++++++
> arch/powerpc/kernel/process.c | 48 ++++++
> arch/powerpc/kernel/ptrace/ptrace-noadv.c | 9 +-
> arch/powerpc/xmon/xmon.c | 1 +
> .../selftests/powerpc/ptrace/ptrace-hwbreak.c | 48 +++++-
> 10 files changed, 282 insertions(+), 152 deletions(-)
> create mode 100644 arch/powerpc/kernel/hw_breakpoint_constraints.c
>
Tested this patch set for:
- SETHWDEBUG when CONFIG_HAVE_HW_BREAKPOINT=N = OK
- Fix exception handling for CONFIG_HAVE_HW_BREAKPOINT=N = OK
- Check for PPC_DEBUG_FEATURE_DATA_BP_ARCH_31 = OK
- Fix quarword instruction handling on p10 predecessors = OK
- Fix handling of vector instructions = OK
Also tested for:
- Set second watchpoint (P10 Mambo) = OK
- Infinity loop on sc instruction = OK
^ permalink raw reply
* Re: [PATCH] fsl: imx-audmix : Replace seq_printf with seq_puts
From: Mark Brown @ 2020-09-17 13:04 UTC (permalink / raw)
To: Xu Wang
Cc: alsa-devel, linuxppc-dev, linux-kernel, timur, Xiubo.Lee,
festevam, s.hauer, tiwai, lgirdwood, perex, nicoleotsuka,
linux-imx, kernel, shawnguo, shengjiu.wang, linux-arm-kernel
In-Reply-To: <20200916061420.10403-1-vulab@iscas.ac.cn>
[-- Attachment #1: Type: text/plain, Size: 318 bytes --]
On Wed, Sep 16, 2020 at 06:14:20AM +0000, Xu Wang wrote:
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "devm_kcalloc".
This looks fine but the subject says it's about seq_puts() not
kcalloc().
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] powerpc/perf: Exclude pmc5/6 from the irrelevant PMU group constraints
From: Athira Rajeev @ 2020-09-17 13:01 UTC (permalink / raw)
To: Michael Ellerman; +Cc: maddy, linuxppc-dev
In-Reply-To: <87wo0sob28.fsf@mpe.ellerman.id.au>
> On 17-Sep-2020, at 5:43 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
>> PMU counter support functions enforces event constraints for group of
>> events to check if all events in a group can be monitored. Incase of
>> event codes using PMC5 and PMC6 ( 500fa and 600f4 respectively ),
>> not all constraints are applicable, say the threshold or sample bits.
>> But current code includes pmc5 and pmc6 in some group constraints (like
>> IC_DC Qualifier bits) which is actually not applicable and hence results
>> in those events not getting counted when scheduled along with group of
>> other events. Patch fixes this by excluding PMC5/6 from constraints
>> which are not relevant for it.
>>
>> Fixes: 7ffd948 ('powerpc/perf: factor out power8 pmu functions')
>> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/perf/isa207-common.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
>> index 964437a..186fad8 100644
>> --- a/arch/powerpc/perf/isa207-common.c
>> +++ b/arch/powerpc/perf/isa207-common.c
>> @@ -288,6 +288,9 @@ int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp)
>>
>> mask |= CNST_PMC_MASK(pmc);
>> value |= CNST_PMC_VAL(pmc);
>> +
>> + if (pmc >= 5)
>> + goto ebb_bhrb;
>
> This is fairly subtle. Can you please put a block comment above the if
> explaining in a few lines what's going on.
Hi Michael,
Sure, I will include a comment explaining the change in V2.
Thanks
Athira
>
> cheers
^ permalink raw reply
* Re: [PATCH 0/3] powerpc/mce: Fix mce handler and add selftest
From: Michal Suchánek @ 2020-09-17 12:29 UTC (permalink / raw)
To: Ganesh Goudar; +Cc: linuxppc-dev, npiggin, mahesh
In-Reply-To: <20200916172228.83271-1-ganeshgr@linux.ibm.com>
Hello,
On Wed, Sep 16, 2020 at 10:52:25PM +0530, Ganesh Goudar wrote:
> This patch series fixes mce handling for pseries, provides debugfs
> interface for mce injection and adds selftest to test mce handling
> on pseries/powernv machines running in hash mmu mode.
> debugfs interface and sleftest are added only for slb multihit
> injection, We can add other tests in future if possible.
>
> Ganesh Goudar (3):
> powerpc/mce: remove nmi_enter/exit from real mode handler
> powerpc/mce: Add debugfs interface to inject MCE
> selftest/powerpc: Add slb multihit selftest
Is the below logic sound? It does not agree with what is added here:
void machine_check_exception(struct pt_regs *regs)
{
int recover = 0;
/*
* BOOK3S_64 does not call this handler as a non-maskable interrupt
* (it uses its own early real-mode handler to handle the MCE proper
* and then raises irq_work to call this handler when interrupts are
* enabled).
*
* This is silly. The BOOK3S_64 should just call a different function
* rather than expecting semantics to magically change. Something
* like 'non_nmi_machine_check_exception()', perhaps?
*/
const bool nmi = !IS_ENABLED(CONFIG_PPC_BOOK3S_64);
if (nmi) nmi_enter();
Thanks
Michal
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/mce: Add debugfs interface to inject MCE
From: Michal Suchánek @ 2020-09-17 12:23 UTC (permalink / raw)
To: Ganesh Goudar; +Cc: linuxppc-dev, npiggin, mahesh
In-Reply-To: <20200916172228.83271-3-ganeshgr@linux.ibm.com>
Hello,
On Wed, Sep 16, 2020 at 10:52:27PM +0530, Ganesh Goudar wrote:
> To test machine check handling, add debugfs interface to inject
> slb multihit errors.
>
> To inject slb multihit:
> #echo 1 > /sys/kernel/debug/powerpc/mce_error_inject/inject_slb_multihit
>
> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
> ---
> arch/powerpc/Kconfig.debug | 9 ++
> arch/powerpc/sysdev/Makefile | 2 +
> arch/powerpc/sysdev/mce_error_inject.c | 148 +++++++++++++++++++++++++
> 3 files changed, 159 insertions(+)
> create mode 100644 arch/powerpc/sysdev/mce_error_inject.c
>
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index b88900f4832f..61db133f2f0d 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -398,3 +398,12 @@ config KASAN_SHADOW_OFFSET
> hex
> depends on KASAN
> default 0xe0000000
> +
> +config MCE_ERROR_INJECT
> + bool "Enable MCE error injection through debugfs"
> + depends on DEBUG_FS
> + default y
> + help
> + This option creates an mce_error_inject directory in the
> + powerpc debugfs directory that allows limited injection of
> + Machine Check Errors (MCEs).
> diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile
> index 026b3f01a991..7fc102222b77 100644
> --- a/arch/powerpc/sysdev/Makefile
> +++ b/arch/powerpc/sysdev/Makefile
> @@ -52,3 +52,5 @@ obj-$(CONFIG_PPC_XICS) += xics/
> obj-$(CONFIG_PPC_XIVE) += xive/
>
> obj-$(CONFIG_GE_FPGA) += ge/
> +
> +obj-$(CONFIG_MCE_ERROR_INJECT) += mce_error_inject.o
> diff --git a/arch/powerpc/sysdev/mce_error_inject.c b/arch/powerpc/sysdev/mce_error_inject.c
> new file mode 100644
> index 000000000000..ca4726bfa2d9
> --- /dev/null
> +++ b/arch/powerpc/sysdev/mce_error_inject.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Machine Check Exception injection code
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +#include <linux/fs.h>
> +#include <linux/debugfs.h>
> +#include <asm/debugfs.h>
> +
> +static inline unsigned long get_slb_index(void)
> +{
> + unsigned long index;
> +
> + index = get_paca()->stab_rr;
> +
> + /*
> + * simple round-robin replacement of slb starting at SLB_NUM_BOLTED.
> + */
> + if (index < (mmu_slb_size - 1))
> + index++;
> + else
> + index = SLB_NUM_BOLTED;
> + get_paca()->stab_rr = index;
> + return index;
> +}
> +
> +#define slb_esid_mask(ssize) \
> + (((ssize) == MMU_SEGSIZE_256M) ? ESID_MASK : ESID_MASK_1T)
> +
> +static inline unsigned long mk_esid_data(unsigned long ea, int ssize,
> + unsigned long slot)
> +{
> + return (ea & slb_esid_mask(ssize)) | SLB_ESID_V | slot;
> +}
> +
> +#define slb_vsid_shift(ssize) \
> + ((ssize) == MMU_SEGSIZE_256M ? SLB_VSID_SHIFT : SLB_VSID_SHIFT_1T)
> +
> +static inline unsigned long mk_vsid_data(unsigned long ea, int ssize,
> + unsigned long flags)
> +{
> + return (get_kernel_vsid(ea, ssize) << slb_vsid_shift(ssize)) | flags |
> + ((unsigned long)ssize << SLB_VSID_SSIZE_SHIFT);
> +}
> +
> +static void insert_slb_entry(char *p, int ssize)
> +{
> + unsigned long flags, entry;
> + struct paca_struct *paca;
> +
> + flags = SLB_VSID_KERNEL | mmu_psize_defs[MMU_PAGE_64K].sllp;
> +
> + preempt_disable();
> +
> + paca = get_paca();
This seems unused?
> +
> + entry = get_slb_index();
> + asm volatile("slbmte %0,%1" :
> + : "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
> + "r" (mk_esid_data((unsigned long)p, ssize, entry))
> + : "memory");
> +
> + entry = get_slb_index();
> + asm volatile("slbmte %0,%1" :
> + : "r" (mk_vsid_data((unsigned long)p, ssize, flags)),
> + "r" (mk_esid_data((unsigned long)p, ssize, entry))
> + : "memory");
> + preempt_enable();
> + p[0] = '!';
> +}
> +
> +static void inject_vmalloc_slb_multihit(void)
> +{
> + char *p;
> +
> + p = vmalloc(2048);
> + if (!p)
> + return;
> +
> + insert_slb_entry(p, MMU_SEGSIZE_1T);
> + vfree(p);
> +}
> +
> +static void inject_kmalloc_slb_multihit(void)
> +{
> + char *p;
> +
> + p = kmalloc(2048, GFP_KERNEL);
> + if (!p)
> + return;
> +
> + insert_slb_entry(p, MMU_SEGSIZE_1T);
> + kfree(p);
> +}
> +
> +static ssize_t inject_slb_multihit(const char __user *u_buf, size_t count)
> +{
> + char buf[32];
> + size_t buf_size;
> +
> + buf_size = min(count, (sizeof(buf) - 1));
> + if (copy_from_user(buf, u_buf, buf_size))
> + return -EFAULT;
> + buf[buf_size] = '\0';
> +
> + if (buf[0] != '1')
> + return -EINVAL;
> +
> + inject_vmalloc_slb_multihit();
> + inject_kmalloc_slb_multihit();
This is missing the test of multihit in paca which is for some reason
special.
Thanks
Michal
> + return count;
> +}
> +
> +static ssize_t inject_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + static ssize_t (*func)(const char __user *, size_t);
> +
> + func = file->f_inode->i_private;
> + return func(buf, count);
> +}
> +
> +static const struct file_operations inject_fops = {
> + .write = inject_write,
> + .llseek = default_llseek,
> +};
> +
> +static int mce_error_inject_setup(void)
> +{
> + struct dentry *mce_error_inject_dir;
> +
> + mce_error_inject_dir = debugfs_create_dir("mce_error_inject",
> + powerpc_debugfs_root);
> +
> + if (mmu_has_feature(MMU_FTR_HPTE_TABLE)) {
> + (void)debugfs_create_file("inject_slb_multihit", 0200,
> + mce_error_inject_dir,
> + &inject_slb_multihit,
> + &inject_fops);
> + }
> +
> + return 0;
> +}
> +
> +device_initcall(mce_error_inject_setup);
> --
> 2.26.2
>
^ permalink raw reply
* Re: [PATCH v2 6/7] powerpc/perf: Remove unused variable 'target' in trace_imc_event_init()
From: Athira Rajeev @ 2020-09-17 12:20 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Christophe Leroy, anju@linux.vnet.ibm.com, linuxppc-dev
In-Reply-To: <20200914211007.2285999-7-clg@kaod.org>
> On 15-Sep-2020, at 2:40 AM, Cédric Le Goater <clg@kaod.org> wrote:
>
> This fixes a compile error with W=1.
>
> CC arch/powerpc/perf/imc-pmu.o
> ../arch/powerpc/perf/imc-pmu.c: In function ‘trace_imc_event_init’:
> ../arch/powerpc/perf/imc-pmu.c:1429:22: error: variable ‘target’ set but not used [-Werror=unused-but-set-variable]
> struct task_struct *target;
> ^~~~~~
>
> Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> arch/powerpc/perf/imc-pmu.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index 62d0b54086f8..9ed4fcccf8a9 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -1426,8 +1426,6 @@ static void trace_imc_event_del(struct perf_event *event, int flags)
>
> static int trace_imc_event_init(struct perf_event *event)
> {
> - struct task_struct *target;
> -
> if (event->attr.type != event->pmu->type)
> return -ENOENT;
>
> @@ -1458,7 +1456,6 @@ static int trace_imc_event_init(struct perf_event *event)
> mutex_unlock(&imc_global_refc.lock);
>
> event->hw.idx = -1;
> - target = event->hw.target;
Reviewed-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
Thanks
Athira
>
> event->pmu->task_ctx_nr = perf_hw_context;
> event->destroy = reset_global_refc;
> --
> 2.25.4
>
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/mce: remove nmi_enter/exit from real mode handler
From: Michal Suchánek @ 2020-09-17 12:20 UTC (permalink / raw)
To: Ganesh Goudar; +Cc: linuxppc-dev, npiggin, mahesh
In-Reply-To: <20200916172228.83271-2-ganeshgr@linux.ibm.com>
Hello,
On Wed, Sep 16, 2020 at 10:52:26PM +0530, Ganesh Goudar wrote:
> Use of nmi_enter/exit in real mode handler causes the kernel to panic
> and reboot on injecting slb mutihit on pseries machine running in hash
> mmu mode, As these calls try to accesses memory outside RMO region in
> real mode handler where translation is disabled.
>
> Add check to not to use these calls on pseries machine running in hash
> mmu mode.
>
> Fixes: 116ac378bb3f ("powerpc/64s: machine check interrupt update NMI accounting")
> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
> ---
> arch/powerpc/kernel/mce.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
> index ada59f6c4298..1d42fe0f5f9c 100644
> --- a/arch/powerpc/kernel/mce.c
> +++ b/arch/powerpc/kernel/mce.c
> @@ -591,10 +591,15 @@ EXPORT_SYMBOL_GPL(machine_check_print_event_info);
> long notrace machine_check_early(struct pt_regs *regs)
> {
> long handled = 0;
> - bool nested = in_nmi();
> + bool nested;
> + bool is_pseries_hpt_guest;
> u8 ftrace_enabled = this_cpu_get_ftrace_enabled();
>
> this_cpu_set_ftrace_enabled(0);
> + is_pseries_hpt_guest = machine_is(pseries) &&
> + mmu_has_feature(MMU_FTR_HPTE_TABLE);
> + /* Do not use nmi_enter/exit for pseries hpte guest */
> + nested = is_pseries_hpt_guest ? true : in_nmi();
As pointed out already in another comment nesting is supported natively
since 69ea03b56ed2c7189ccd0b5910ad39f3cad1df21. You can simply do
nmi_enter and nmi_exit unconditionally - or only based on
is_pseries_hpt_guest.
The other question is what is the value of calling nmi_enter here at
all. It crashes in one case, we simply skip it for that case, and we are
good. Maybe we could skip it altogether?
Thanks
Michal
^ permalink raw reply
* Re: [PATCH] powerpc/perf: Exclude pmc5/6 from the irrelevant PMU group constraints
From: Michael Ellerman @ 2020-09-17 12:13 UTC (permalink / raw)
To: Athira Rajeev; +Cc: maddy, linuxppc-dev
In-Reply-To: <1600257900-2043-1-git-send-email-atrajeev@linux.vnet.ibm.com>
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> PMU counter support functions enforces event constraints for group of
> events to check if all events in a group can be monitored. Incase of
> event codes using PMC5 and PMC6 ( 500fa and 600f4 respectively ),
> not all constraints are applicable, say the threshold or sample bits.
> But current code includes pmc5 and pmc6 in some group constraints (like
> IC_DC Qualifier bits) which is actually not applicable and hence results
> in those events not getting counted when scheduled along with group of
> other events. Patch fixes this by excluding PMC5/6 from constraints
> which are not relevant for it.
>
> Fixes: 7ffd948 ('powerpc/perf: factor out power8 pmu functions')
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
> arch/powerpc/perf/isa207-common.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/powerpc/perf/isa207-common.c b/arch/powerpc/perf/isa207-common.c
> index 964437a..186fad8 100644
> --- a/arch/powerpc/perf/isa207-common.c
> +++ b/arch/powerpc/perf/isa207-common.c
> @@ -288,6 +288,9 @@ int isa207_get_constraint(u64 event, unsigned long *maskp, unsigned long *valp)
>
> mask |= CNST_PMC_MASK(pmc);
> value |= CNST_PMC_VAL(pmc);
> +
> + if (pmc >= 5)
> + goto ebb_bhrb;
This is fairly subtle. Can you please put a block comment above the if
explaining in a few lines what's going on.
cheers
^ permalink raw reply
* Re: [PATCH -next] ide: Fix symbol undeclared warnings
From: Michael Ellerman @ 2020-09-17 12:01 UTC (permalink / raw)
To: Wang Wensheng, davem, benh, paulus, linux-ide, linuxppc-dev,
linux-kernel
In-Reply-To: <20200916092333.77158-1-wangwensheng4@huawei.com>
Wang Wensheng <wangwensheng4@huawei.com> writes:
> Build the object file with `C=2` and get the following warnings:
> make allmodconfig ARCH=powerpc CROSS_COMPILE=powerpc64-linux-gnu-
> make C=2 drivers/ide/pmac.o ARCH=powerpc64
> CROSS_COMPILE=powerpc64-linux-gnu-
>
> drivers/ide/pmac.c:228:23: warning: symbol 'mdma_timings_33' was not
> declared. Should it be static?
> drivers/ide/pmac.c:241:23: warning: symbol 'mdma_timings_33k' was not
> declared. Should it be static?
> drivers/ide/pmac.c:254:23: warning: symbol 'mdma_timings_66' was not
> declared. Should it be static?
> drivers/ide/pmac.c:272:3: warning: symbol 'kl66_udma_timings' was not
> declared. Should it be static?
> drivers/ide/pmac.c:1418:12: warning: symbol 'pmac_ide_probe' was not
> declared. Should it be static?
>
> Signed-off-by: Wang Wensheng <wangwensheng4@huawei.com>
> ---
> drivers/ide/pmac.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
TIL davem maintains IDE?
But I suspect he isn't that interested in this powerpc only driver, so
I'll grab this.
cheers
> diff --git a/drivers/ide/pmac.c b/drivers/ide/pmac.c
> index ea0b064b5f56..6bb2fc6755c2 100644
> --- a/drivers/ide/pmac.c
> +++ b/drivers/ide/pmac.c
> @@ -225,7 +225,7 @@ struct mdma_timings_t {
> int cycleTime;
> };
>
> -struct mdma_timings_t mdma_timings_33[] =
> +static struct mdma_timings_t mdma_timings_33[] =
> {
> { 240, 240, 480 },
> { 180, 180, 360 },
> @@ -238,7 +238,7 @@ struct mdma_timings_t mdma_timings_33[] =
> { 0, 0, 0 }
> };
>
> -struct mdma_timings_t mdma_timings_33k[] =
> +static struct mdma_timings_t mdma_timings_33k[] =
> {
> { 240, 240, 480 },
> { 180, 180, 360 },
> @@ -251,7 +251,7 @@ struct mdma_timings_t mdma_timings_33k[] =
> { 0, 0, 0 }
> };
>
> -struct mdma_timings_t mdma_timings_66[] =
> +static struct mdma_timings_t mdma_timings_66[] =
> {
> { 240, 240, 480 },
> { 180, 180, 360 },
> @@ -265,7 +265,7 @@ struct mdma_timings_t mdma_timings_66[] =
> };
>
> /* KeyLargo ATA-4 Ultra DMA timings (rounded) */
> -struct {
> +static struct {
> int addrSetup; /* ??? */
> int rdy2pause;
> int wrDataSetup;
> @@ -1415,7 +1415,7 @@ static struct pci_driver pmac_ide_pci_driver = {
> };
> MODULE_DEVICE_TABLE(pci, pmac_ide_pci_match);
>
> -int __init pmac_ide_probe(void)
> +static int __init pmac_ide_probe(void)
> {
> int error;
>
> --
> 2.25.0
^ permalink raw reply
* Re: [PATCH v2] powerpc: fix EDEADLOCK redefinition error in uapi/asm/errno.h
From: Michael Ellerman @ 2020-09-17 11:55 UTC (permalink / raw)
To: Tony Ambardar
Cc: linux-arch, Tony Ambardar, Arnd Bergmann, linux-kernel,
Paul Mackerras, Rosen Penev, bpf, linuxppc-dev
In-Reply-To: <20200917000757.1232850-1-Tony.Ambardar@gmail.com>
[ Cc += linux-arch & Arnd ]
Hi Tony,
This looks OK to me, but I'm always a bit nervous about changes in uapi.
I've Cc'ed linux-arch and Arnd who look after the asm-generic headers,
which this is slightly related to, just in case.
One minor comment below.
Tony Ambardar <tony.ambardar@gmail.com> writes:
> A few archs like powerpc have different errno.h values for macros
> EDEADLOCK and EDEADLK. In code including both libc and linux versions of
> errno.h, this can result in multiple definitions of EDEADLOCK in the
> include chain. Definitions to the same value (e.g. seen with mips) do
> not raise warnings, but on powerpc there are redefinitions changing the
> value, which raise warnings and errors (if using "-Werror").
>
> Guard against these redefinitions to avoid build errors like the following,
> first seen cross-compiling libbpf v5.8.9 for powerpc using GCC 8.4.0 with
> musl 1.1.24:
>
> In file included from ../../arch/powerpc/include/uapi/asm/errno.h:5,
> from ../../include/linux/err.h:8,
> from libbpf.c:29:
> ../../include/uapi/asm-generic/errno.h:40: error: "EDEADLOCK" redefined [-Werror]
> #define EDEADLOCK EDEADLK
>
> In file included from toolchain-powerpc_8540_gcc-8.4.0_musl/include/errno.h:10,
> from libbpf.c:26:
> toolchain-powerpc_8540_gcc-8.4.0_musl/include/bits/errno.h:58: note: this is the location of the previous definition
> #define EDEADLOCK 58
>
> cc1: all warnings being treated as errors
>
> Fixes: 95f28190aa01 ("tools include arch: Grab a copy of errno.h for arch's supported by perf")
> Fixes: c3617f72036c ("UAPI: (Scripted) Disintegrate arch/powerpc/include/asm")
I suspect that's not the right commit to tag. It just moved errno.h from
arch/powerpc/include/asm to arch/powerpc/include/uapi/asm. It's content
was almost identical, and entirely identical as far as EDEADLOCK was
concerned.
Prior to that the file lived in asm-powerpc/errno.h, eg:
$ git cat-file -p b8b572e1015f^:include/asm-powerpc/errno.h
Before that it was include/asm-ppc64/errno.h, content still the same.
To go back further we'd have to look at the historical git trees, which
is probably overkill. I'm pretty sure it's always had this problem.
So we should probably drop the Fixes tags and just Cc: stable, that
means please backport it as far back as possible.
cheers
> Reported-by: Rosen Penev <rosenp@gmail.com>
> Signed-off-by: Tony Ambardar <Tony.Ambardar@gmail.com>
> ---
> v1 -> v2:
> * clean up commit description formatting
> ---
> arch/powerpc/include/uapi/asm/errno.h | 1 +
> tools/arch/powerpc/include/uapi/asm/errno.h | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/arch/powerpc/include/uapi/asm/errno.h b/arch/powerpc/include/uapi/asm/errno.h
> index cc79856896a1..4ba87de32be0 100644
> --- a/arch/powerpc/include/uapi/asm/errno.h
> +++ b/arch/powerpc/include/uapi/asm/errno.h
> @@ -2,6 +2,7 @@
> #ifndef _ASM_POWERPC_ERRNO_H
> #define _ASM_POWERPC_ERRNO_H
>
> +#undef EDEADLOCK
> #include <asm-generic/errno.h>
>
> #undef EDEADLOCK
> diff --git a/tools/arch/powerpc/include/uapi/asm/errno.h b/tools/arch/powerpc/include/uapi/asm/errno.h
> index cc79856896a1..4ba87de32be0 100644
> --- a/tools/arch/powerpc/include/uapi/asm/errno.h
> +++ b/tools/arch/powerpc/include/uapi/asm/errno.h
> @@ -2,6 +2,7 @@
> #ifndef _ASM_POWERPC_ERRNO_H
> #define _ASM_POWERPC_ERRNO_H
>
> +#undef EDEADLOCK
> #include <asm-generic/errno.h>
>
> #undef EDEADLOCK
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH] powerpc/64: Make VDSO32 track COMPAT on 64-bit
From: Michael Ellerman @ 2020-09-17 11:28 UTC (permalink / raw)
To: linuxppc-dev, Michael Ellerman; +Cc: christophe.leroy, msuchanek
In-Reply-To: <20200908125850.407939-1-mpe@ellerman.id.au>
On Tue, 8 Sep 2020 22:58:50 +1000, Michael Ellerman wrote:
> When we added the VDSO32 kconfig symbol, which controls building of
> the 32-bit VDSO, we made it depend on CPU_BIG_ENDIAN (for 64-bit).
>
> That was because back then COMPAT was always enabled for 64-bit, so
> depending on it would have left the 32-bit VDSO always enabled, which
> we didn't want.
>
> [...]
Applied to powerpc/next.
[1/1] powerpc/64: Make VDSO32 track COMPAT on 64-bit
https://git.kernel.org/powerpc/c/231b232df8f67e7d37af01259c21f2a131c3911e
cheers
^ permalink raw reply
* [Bug 209297] New: powerpc: MPC10X_BRIDGE violates Kconfig dependency of PPC_INDIRECT_PCI on PCI
From: bugzilla-daemon @ 2020-09-17 11:28 UTC (permalink / raw)
To: linuxppc-dev
https://bugzilla.kernel.org/show_bug.cgi?id=209297
Bug ID: 209297
Summary: powerpc: MPC10X_BRIDGE violates Kconfig dependency of
PPC_INDIRECT_PCI on PCI
Product: Platform Specific/Hardware
Version: 2.5
Kernel Version: 5.9-rc4
Hardware: PPC-32
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: PPC-32
Assignee: platform_ppc-32@kernel-bugs.osdl.org
Reporter: fazilyildiran@gmail.com
CC: mpe@ellerman.id.au, paulus@samba.org
Regression: No
The commit 25635c71e441 ("ppc: Use the indirect_pci.c from
arch/powerpc/sysdev")
introduced PPC_INDIRECT_PCI as a non-visible config that depends on PCI.
Since it is non-visible, the dependency on PCI has no implications other
than leading to Kbuild build warnings if !PCI. For example, disabling PCI
and enabling LINKSTATION leads to the following Kbuild warning since
PPC_INDIRECT_PCI is selected by MPC10X_BRIDGE, which is selected by
LINKSTATION:
WARNING: unmet direct dependencies detected for PPC_INDIRECT_PCI
Depends on [n]: PCI [=n]
Selected by [y]:
- MPC10X_BRIDGE [=y]
There are many other configs that select PPC_INDIRECT_PCI but they seem
to enable PCI one way or another, e.g., enabling FORCE_PCI or selecting
PPC_INDIRECT_PCI if PCI. Among them, only MPC10X_BRIDGE and MV64X60 seem
to not account for the dependency on PCI, and MV64X60 seem to be obsolete:
https://bugzilla.kernel.org/show_bug.cgi?id=209277
I am not sure about the internals of the user subsystem. However, two fix
options I see are: 1) --select FORCE_PCI-- by MPC10X_BRIDGE, 2) select
PPC_INDIRECT_PCI --if PCI-- by MPC10X_BRIDGE.
Thanks,
Necip
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH] powerpc/ps3: make two symbols static
From: Michael Ellerman @ 2020-09-17 11:27 UTC (permalink / raw)
To: Jason Yan, geoff, mpe, paulus, linuxppc-dev, benh, linux-kernel
Cc: Hulk Robot
In-Reply-To: <20200911020121.1464585-1-yanaijie@huawei.com>
On Fri, 11 Sep 2020 10:01:21 +0800, Jason Yan wrote:
> This addresses the following sparse warning:
>
> arch/powerpc/platforms/ps3/spu.c:451:33: warning: symbol
> 'spu_management_ps3_ops' was not declared. Should it be static?
> arch/powerpc/platforms/ps3/spu.c:592:28: warning: symbol
> 'spu_priv1_ps3_ops' was not declared. Should it be static?
Applied to powerpc/next.
[1/1] powerpc/ps3: make two symbols static
https://git.kernel.org/powerpc/c/bbc4f40b5322b3e0b8678619f1c613dadc811669
cheers
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox