* Re: [PATCH v5 5/9] powerpc/mm/book3s64: Update tlb flush routines to take a page walk cache flush argument
From: Segher Boessenkool @ 2021-05-19 0:45 UTC (permalink / raw)
To: Michael Ellerman
Cc: Aneesh Kumar K.V, npiggin, linux-mm, kaleshsingh, joel, akpm,
linuxppc-dev, Guenter Roeck
In-Reply-To: <87a6ork1qp.fsf@mpe.ellerman.id.au>
On Wed, May 19, 2021 at 10:26:22AM +1000, Michael Ellerman wrote:
> Guenter Roeck <linux@roeck-us.net> writes:
> > Ah, sorry. I wasn't aware that the following is valid C code
> >
> > void f1()
> > {
> > return f2();
> > ^^^^^^
> > }
> >
> > as long as f2() is void as well. Confusing, but we live and learn.
>
> It might be valid, but it's still bad IMHO.
>
> It's confusing to readers, and serves no useful purpose.
And it actually explicitly is undefined behaviour in C90 already
(3.6.6.4 in C90, 6.8.6.4 in C99 and later).
Segher
^ permalink raw reply
* Re: [PATCH v5 5/9] powerpc/mm/book3s64: Update tlb flush routines to take a page walk cache flush argument
From: Guenter Roeck @ 2021-05-19 1:08 UTC (permalink / raw)
To: Michael Ellerman, Aneesh Kumar K.V
Cc: npiggin, linux-mm, kaleshsingh, joel, akpm, linuxppc-dev
In-Reply-To: <87a6ork1qp.fsf@mpe.ellerman.id.au>
On 5/18/21 5:26 PM, Michael Ellerman wrote:
[ ... ]
>>> That was the generic header change in the patch. I was commenting about the
>>> ppc64 specific change causing build failures.
>>
>> Ah, sorry. I wasn't aware that the following is valid C code
>>
>> void f1()
>> {
>> return f2();
>> ^^^^^^
>> }
>>
>> as long as f2() is void as well. Confusing, but we live and learn.
>
> It might be valid, but it's still bad IMHO.
>
> It's confusing to readers, and serves no useful purpose.
>
Agreed, but it is surprisingly wide-spread. Try to run the coccinelle
script below, just for fun. The script doesn't even catch instances
in include files, yet there are more than 450 hits.
Guenter
---
virtual report
@d@
identifier f;
expression e;
position p;
@@
void f(...)
{
<...
return e@p;
...>
}
@script:python depends on report@
f << d.f;
p << d.p;
@@
print "void function %s:%s() with non-void return in line %s" % (p[0].file, f, p[0].line)
^ permalink raw reply
* Re: [PATCH v6 2/3] powerpc: Move script to check relocations at compile time in scripts/
From: Michael Ellerman @ 2021-05-19 1:09 UTC (permalink / raw)
To: Alexandre Ghiti, Benjamin Herrenschmidt, Paul Mackerras,
Paul Walmsley, Palmer Dabbelt, Albert Ou, linuxppc-dev,
linux-kernel, linux-riscv
Cc: Anup Patel, Alexandre Ghiti
In-Reply-To: <20210518101252.1484465-3-alex@ghiti.fr>
Alexandre Ghiti <alex@ghiti.fr> writes:
> Relocating kernel at runtime is done very early in the boot process, so
> it is not convenient to check for relocations there and react in case a
> relocation was not expected.
>
> Powerpc architecture has a script that allows to check at compile time
> for such unexpected relocations: extract the common logic to scripts/
> so that other architectures can take advantage of it.
>
> Signed-off-by: Alexandre Ghiti <alex@ghiti.fr>
> Reviewed-by: Anup Patel <anup@brainfault.org>
> ---
> arch/powerpc/tools/relocs_check.sh | 18 ++----------------
> scripts/relocs_check.sh | 20 ++++++++++++++++++++
> 2 files changed, 22 insertions(+), 16 deletions(-)
> create mode 100755 scripts/relocs_check.sh
I'm not sure that script is really big/complicated enough to warrant
sharing vs just copying, but I don't mind either.
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
> diff --git a/arch/powerpc/tools/relocs_check.sh b/arch/powerpc/tools/relocs_check.sh
> index 014e00e74d2b..e367895941ae 100755
> --- a/arch/powerpc/tools/relocs_check.sh
> +++ b/arch/powerpc/tools/relocs_check.sh
> @@ -15,21 +15,8 @@ if [ $# -lt 3 ]; then
> exit 1
> fi
>
> -# Have Kbuild supply the path to objdump and nm so we handle cross compilation.
> -objdump="$1"
> -nm="$2"
> -vmlinux="$3"
> -
> -# Remove from the bad relocations those that match an undefined weak symbol
> -# which will result in an absolute relocation to 0.
> -# Weak unresolved symbols are of that form in nm output:
> -# " w _binary__btf_vmlinux_bin_end"
> -undef_weak_symbols=$($nm "$vmlinux" | awk '$1 ~ /w/ { print $2 }')
> -
> bad_relocs=$(
> -$objdump -R "$vmlinux" |
> - # Only look at relocation lines.
> - grep -E '\<R_' |
> +${srctree}/scripts/relocs_check.sh "$@" |
> # These relocations are okay
> # On PPC64:
> # R_PPC64_RELATIVE, R_PPC64_NONE
> @@ -43,8 +30,7 @@ R_PPC_ADDR16_LO
> R_PPC_ADDR16_HI
> R_PPC_ADDR16_HA
> R_PPC_RELATIVE
> -R_PPC_NONE' |
> - ([ "$undef_weak_symbols" ] && grep -F -w -v "$undef_weak_symbols" || cat)
> +R_PPC_NONE'
> )
>
> if [ -z "$bad_relocs" ]; then
> diff --git a/scripts/relocs_check.sh b/scripts/relocs_check.sh
> new file mode 100755
> index 000000000000..137c660499f3
> --- /dev/null
> +++ b/scripts/relocs_check.sh
> @@ -0,0 +1,20 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +# Get a list of all the relocations, remove from it the relocations
> +# that are known to be legitimate and return this list to arch specific
> +# script that will look for suspicious relocations.
> +
> +objdump="$1"
> +nm="$2"
> +vmlinux="$3"
> +
> +# Remove from the possible bad relocations those that match an undefined
> +# weak symbol which will result in an absolute relocation to 0.
> +# Weak unresolved symbols are of that form in nm output:
> +# " w _binary__btf_vmlinux_bin_end"
> +undef_weak_symbols=$($nm "$vmlinux" | awk '$1 ~ /w/ { print $2 }')
> +
> +$objdump -R "$vmlinux" |
> + grep -E '\<R_' |
> + ([ "$undef_weak_symbols" ] && grep -F -w -v "$undef_weak_symbols" || cat)
> --
> 2.30.2
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply
* [Bug 213069] kernel BUG at arch/powerpc/include/asm/book3s/64/hash-4k.h:147! Oops: Exception in kernel mode, sig: 5 [#1]
From: bugzilla-daemon @ 2021-05-19 1:56 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-213069-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=213069
Michael Ellerman (michael@ellerman.id.au) changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution|--- |CODE_FIX
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [PATCH net-next] ibmveth: fix kobj_to_dev.cocci warnings
From: YueHaibing @ 2021-05-19 2:28 UTC (permalink / raw)
To: benh, paulus, cforno12, davem, kuba, dwilder
Cc: netdev, YueHaibing, linuxppc-dev, linux-kernel
Use kobj_to_dev() instead of container_of()
Generated by: scripts/coccinelle/api/kobj_to_dev.cocci
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/ethernet/ibm/ibmveth.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 7fea9ae60f13..bc67a7ee872b 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -1799,8 +1799,7 @@ static ssize_t veth_pool_store(struct kobject *kobj, struct attribute *attr,
struct ibmveth_buff_pool *pool = container_of(kobj,
struct ibmveth_buff_pool,
kobj);
- struct net_device *netdev = dev_get_drvdata(
- container_of(kobj->parent, struct device, kobj));
+ struct net_device *netdev = dev_get_drvdata(kobj_to_dev(kobj->parent));
struct ibmveth_adapter *adapter = netdev_priv(netdev);
long value = simple_strtol(buf, NULL, 10);
long rc;
--
2.17.1
^ permalink raw reply related
* Re: Linux powerpc new system call instruction and ABI
From: Nicholas Piggin @ 2021-05-19 2:50 UTC (permalink / raw)
To: Dmitry V. Levin, Michael Ellerman
Cc: libc-alpha, Matheus Castanho, linux-api, musl, libc-dev,
linuxppc-dev
In-Reply-To: <20210518231331.GA8464@altlinux.org>
Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am:
> Hi,
>
> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
> [...]
>> - Error handling: The consensus among kernel, glibc, and musl is to move to
>> using negative return values in r3 rather than CR0[SO]=1 to indicate error,
>> which matches most other architectures, and is closer to a function call.
>
> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was
> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and
> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used.
> This includes syscall_get_error() and all its users including
> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable
> when scv is used.
>
> See also https://bugzilla.redhat.com/1929836
I see, thanks. Using latest strace from github.com, the attached kernel
patch makes strace -k check results a lot greener.
Some of the remaining failing tests look like this (I didn't look at all
of them yet):
signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
) = 50
signal(SIGUSR1, SIG_IGN) = 0xfacefeeddeadbeef
write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown errno 559038737) = 41
write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737
) = 26
exit_group(1) = ?
I think the problem is glibc testing for -ve, but it should be comparing
against -4095 (+cc Matheus)
#define RET_SCV \
cmpdi r3,0; \
bgelr+; \
neg r3,r3;
With this patch, I think the ptrace ABI should mostly be fixed. I think
a problem remains with applications that look at system call return
registers directly and have powerpc specific error cases. Those probably
will just need to be updated unfortunately. Michael thought it might be
possible to return an indication via ptrace somehow that the syscall is
using a new ABI, so such apps can be updated to test for it. I don't
know how that would be done.
Thanks,
Nick
--
diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
index 9c9ab2746168..b476a685f066 100644
--- a/arch/powerpc/include/asm/ptrace.h
+++ b/arch/powerpc/include/asm/ptrace.h
@@ -19,6 +19,7 @@
#ifndef _ASM_POWERPC_PTRACE_H
#define _ASM_POWERPC_PTRACE_H
+#include <linux/err.h>
#include <uapi/asm/ptrace.h>
#include <asm/asm-const.h>
@@ -152,25 +153,6 @@ extern unsigned long profile_pc(struct pt_regs *regs);
long do_syscall_trace_enter(struct pt_regs *regs);
void do_syscall_trace_leave(struct pt_regs *regs);
-#define kernel_stack_pointer(regs) ((regs)->gpr[1])
-static inline int is_syscall_success(struct pt_regs *regs)
-{
- return !(regs->ccr & 0x10000000);
-}
-
-static inline long regs_return_value(struct pt_regs *regs)
-{
- if (is_syscall_success(regs))
- return regs->gpr[3];
- else
- return -regs->gpr[3];
-}
-
-static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
-{
- regs->gpr[3] = rc;
-}
-
#ifdef __powerpc64__
#define user_mode(regs) ((((regs)->msr) >> MSR_PR_LG) & 0x1)
#else
@@ -235,6 +217,31 @@ static __always_inline void set_trap_norestart(struct pt_regs *regs)
regs->trap |= 0x1;
}
+#define kernel_stack_pointer(regs) ((regs)->gpr[1])
+static inline int is_syscall_success(struct pt_regs *regs)
+{
+ if (trap_is_scv(regs))
+ return !IS_ERR_VALUE((unsigned long)regs->gpr[3]);
+ else
+ return !(regs->ccr & 0x10000000);
+}
+
+static inline long regs_return_value(struct pt_regs *regs)
+{
+ if (trap_is_scv(regs))
+ return regs->gpr[3];
+
+ if (is_syscall_success(regs))
+ return regs->gpr[3];
+ else
+ return -regs->gpr[3];
+}
+
+static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
+{
+ regs->gpr[3] = rc;
+}
+
#define arch_has_single_step() (1)
#define arch_has_block_step() (true)
#define ARCH_HAS_USER_SINGLE_STEP_REPORT
diff --git a/arch/powerpc/include/asm/syscall.h b/arch/powerpc/include/asm/syscall.h
index fd1b518eed17..e8b40149bf7e 100644
--- a/arch/powerpc/include/asm/syscall.h
+++ b/arch/powerpc/include/asm/syscall.h
@@ -41,11 +41,20 @@ static inline void syscall_rollback(struct task_struct *task,
static inline long syscall_get_error(struct task_struct *task,
struct pt_regs *regs)
{
- /*
- * If the system call failed,
- * regs->gpr[3] contains a positive ERRORCODE.
- */
- return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
+ if (trap_is_scv(regs)) {
+ unsigned long error = regs->gpr[3];
+
+ if (task_is_32bit(task))
+ error = (long)(int)error;
+
+ return IS_ERR_VALUE(error) ? error : 0;
+ } else {
+ /*
+ * If the system call failed,
+ * regs->gpr[3] contains a positive ERRORCODE.
+ */
+ return (regs->ccr & 0x10000000UL) ? -regs->gpr[3] : 0;
+ }
}
static inline long syscall_get_return_value(struct task_struct *task,
@@ -58,18 +67,22 @@ static inline void syscall_set_return_value(struct task_struct *task,
struct pt_regs *regs,
int error, long val)
{
- /*
- * In the general case it's not obvious that we must deal with CCR
- * here, as the syscall exit path will also do that for us. However
- * there are some places, eg. the signal code, which check ccr to
- * decide if the value in r3 is actually an error.
- */
- if (error) {
- regs->ccr |= 0x10000000L;
- regs->gpr[3] = error;
+ if (trap_is_scv(regs)) {
+ regs->gpr[3] = (long) error ?: val;
} else {
- regs->ccr &= ~0x10000000L;
- regs->gpr[3] = val;
+ /*
+ * In the general case it's not obvious that we must deal with
+ * CCR here, as the syscall exit path will also do that for us.
+ * However there are some places, eg. the signal code, which
+ * check ccr to decide if the value in r3 is actually an error.
+ */
+ if (error) {
+ regs->ccr |= 0x10000000L;
+ regs->gpr[3] = error;
+ } else {
+ regs->ccr &= ~0x10000000L;
+ regs->gpr[3] = val;
+ }
}
}
diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index b4ec6c7dd72e..c4bb9f305eaf 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -165,8 +165,10 @@ static inline bool test_thread_local_flags(unsigned int flags)
#ifdef CONFIG_COMPAT
#define is_32bit_task() (test_thread_flag(TIF_32BIT))
+#define task_is_32bit(task) (test_tsk_thread_flag(task, TIF_32BIT))
#else
#define is_32bit_task() (IS_ENABLED(CONFIG_PPC32))
+#define task_is_32bit(task) (IS_ENABLED(CONFIG_PPC32))
#endif
#if defined(CONFIG_PPC64)
^ permalink raw reply related
* Re: [PATCH v2 0/2] mm: unify the allocation of pglist_data instances
From: Mike Rapoport @ 2021-05-19 3:48 UTC (permalink / raw)
To: Miles Chen
Cc: Baoquan He, Jonathan Corbet, linux-doc, kexec, linuxppc-dev,
linux-mm, Paul Mackerras, linux-arm-kernel, linux-mediatek,
Andrew Morton, Dave Young, Vivek Goyal
In-Reply-To: <1621383126.12301.4.camel@mtkswgap22>
On Wed, May 19, 2021 at 08:12:06AM +0800, Miles Chen wrote:
> On Tue, 2021-05-18 at 19:09 +0300, Mike Rapoport wrote:
> > Hello Miles,
> >
> > On Tue, May 18, 2021 at 05:24:44PM +0800, Miles Chen wrote:
> > > This patches is created to fix the __pa() warning messages when
> > > CONFIG_DEBUG_VIRTUAL=y by unifying the allocation of pglist_data
> > > instances.
> > >
> > > In current implementation of node_data, if CONFIG_NEED_MULTIPLE_NODES=y,
> > > pglist_data is allocated by a memblock API. If CONFIG_NEED_MULTIPLE_NODES=n,
> > > we use a global variable named "contig_page_data".
> > >
> > > If CONFIG_DEBUG_VIRTUAL is not enabled. __pa() can handle both
> > > allocation and symbol cases. But if CONFIG_DEBUG_VIRTUAL is set,
> > > we will have the "virt_to_phys used for non-linear address" warning
> > > when booting.
> > >
> > > To fix the warning, always allocate pglist_data by memblock APIs and
> > > remove the usage of contig_page_data.
> >
> > Somehow I was sure that we can allocate pglist_data before it is accessed
> > in sparse_init() somewhere outside mm/sparse.c. It's really not the case
> > and having two places that may allocated this structure is surely worth
> > than your previous suggestion.
> >
> > Sorry about that.
>
> Do you mean taht to call allocation function arch/*, somewhere after
> paging_init() (so we can access pglist_data) and before sparse_init()
> and free_area_init()?
No, I meant that your original patch is better than adding allocation of
NODE_DATA(0) in two places.
> Miles
>
> >
> > > Warning message:
> > > [ 0.000000] ------------[ cut here ]------------
> > > [ 0.000000] virt_to_phys used for non-linear address: (____ptrval____) (contig_page_data+0x0/0x1c00)
> > > [ 0.000000] WARNING: CPU: 0 PID: 0 at arch/arm64/mm/physaddr.c:15 __virt_to_phys+0x58/0x68
> > > [ 0.000000] Modules linked in:
> > > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Tainted: G W 5.13.0-rc1-00074-g1140ab592e2e #3
> > > [ 0.000000] Hardware name: linux,dummy-virt (DT)
> > > [ 0.000000] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO BTYPE=--)
> > > [ 0.000000] pc : __virt_to_phys+0x58/0x68
> > > [ 0.000000] lr : __virt_to_phys+0x54/0x68
> > > [ 0.000000] sp : ffff800011833e70
> > > [ 0.000000] x29: ffff800011833e70 x28: 00000000418a0018 x27: 0000000000000000
> > > [ 0.000000] x26: 000000000000000a x25: ffff800011b70000 x24: ffff800011b70000
> > > [ 0.000000] x23: fffffc0001c00000 x22: ffff800011b70000 x21: 0000000047ffffb0
> > > [ 0.000000] x20: 0000000000000008 x19: ffff800011b082c0 x18: ffffffffffffffff
> > > [ 0.000000] x17: 0000000000000000 x16: ffff800011833bf9 x15: 0000000000000004
> > > [ 0.000000] x14: 0000000000000fff x13: ffff80001186a548 x12: 0000000000000000
> > > [ 0.000000] x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
> > > [ 0.000000] x8 : ffff8000115c9000 x7 : 737520737968705f x6 : ffff800011b62ef8
> > > [ 0.000000] x5 : 0000000000000000 x4 : 0000000000000001 x3 : 0000000000000000
> > > [ 0.000000] x2 : 0000000000000000 x1 : ffff80001159585e x0 : 0000000000000058
> > > [ 0.000000] Call trace:
> > > [ 0.000000] __virt_to_phys+0x58/0x68
> > > [ 0.000000] check_usemap_section_nr+0x50/0xfc
> > > [ 0.000000] sparse_init_nid+0x1ac/0x28c
> > > [ 0.000000] sparse_init+0x1c4/0x1e0
> > > [ 0.000000] bootmem_init+0x60/0x90
> > > [ 0.000000] setup_arch+0x184/0x1f0
> > > [ 0.000000] start_kernel+0x78/0x488
> > > [ 0.000000] ---[ end trace f68728a0d3053b60 ]---
> > >
> > > [1] https://urldefense.com/v3/__https://lore.kernel.org/patchwork/patch/1425110/__;!!CTRNKA9wMg0ARbw!x-wGFEC1wLzXho2kI1CrC2fjXNaQm5f-n0ADQyJDckCOKZHAP_q055DCSWYcQ7Zdcw$
> > >
> > > Change since v1:
> > > - use memblock_alloc() to create pglist_data when CONFIG_NUMA=n
> > >
> > > Miles Chen (2):
> > > mm: introduce prepare_node_data
> > > mm: replace contig_page_data with node_data
> > >
> > > Documentation/admin-guide/kdump/vmcoreinfo.rst | 13 -------------
> > > arch/powerpc/kexec/core.c | 5 -----
> > > include/linux/gfp.h | 3 ---
> > > include/linux/mm.h | 2 ++
> > > include/linux/mmzone.h | 4 ++--
> > > kernel/crash_core.c | 1 -
> > > mm/memblock.c | 3 +--
> > > mm/page_alloc.c | 16 ++++++++++++++++
> > > mm/sparse.c | 2 ++
> > > 9 files changed, 23 insertions(+), 26 deletions(-)
> > >
> > >
> > > base-commit: 8ac91e6c6033ebc12c5c1e4aa171b81a662bd70f
> > > --
> > > 2.18.0
> > >
> >
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [PATCH v2 0/2] mm: unify the allocation of pglist_data instances
From: Miles Chen @ 2021-05-19 3:55 UTC (permalink / raw)
To: Mike Rapoport
Cc: Baoquan He, Jonathan Corbet, linux-doc, kexec, linuxppc-dev,
linux-mm, Paul Mackerras, linux-arm-kernel, linux-mediatek,
Andrew Morton, Dave Young, Vivek Goyal
In-Reply-To: <YKSKq68E9Ompn0vE@kernel.org>
On Wed, 2021-05-19 at 06:48 +0300, Mike Rapoport wrote:
> On Wed, May 19, 2021 at 08:12:06AM +0800, Miles Chen wrote:
> > On Tue, 2021-05-18 at 19:09 +0300, Mike Rapoport wrote:
> > > Hello Miles,
> > >
> > > On Tue, May 18, 2021 at 05:24:44PM +0800, Miles Chen wrote:
> > > > This patches is created to fix the __pa() warning messages when
> > > > CONFIG_DEBUG_VIRTUAL=y by unifying the allocation of pglist_data
> > > > instances.
> > > >
> > > > In current implementation of node_data, if CONFIG_NEED_MULTIPLE_NODES=y,
> > > > pglist_data is allocated by a memblock API. If CONFIG_NEED_MULTIPLE_NODES=n,
> > > > we use a global variable named "contig_page_data".
> > > >
> > > > If CONFIG_DEBUG_VIRTUAL is not enabled. __pa() can handle both
> > > > allocation and symbol cases. But if CONFIG_DEBUG_VIRTUAL is set,
> > > > we will have the "virt_to_phys used for non-linear address" warning
> > > > when booting.
> > > >
> > > > To fix the warning, always allocate pglist_data by memblock APIs and
> > > > remove the usage of contig_page_data.
> > >
> > > Somehow I was sure that we can allocate pglist_data before it is accessed
> > > in sparse_init() somewhere outside mm/sparse.c. It's really not the case
> > > and having two places that may allocated this structure is surely worth
> > > than your previous suggestion.
> > >
> > > Sorry about that.
> >
> > Do you mean taht to call allocation function arch/*, somewhere after
> > paging_init() (so we can access pglist_data) and before sparse_init()
> > and free_area_init()?
>
> No, I meant that your original patch is better than adding allocation of
> NODE_DATA(0) in two places.
Got it. will you re-review the original patch?
>
> > Miles
> >
> > >
> > > > Warning message:
> > > > [ 0.000000] ------------[ cut here ]------------
> > > > [ 0.000000] virt_to_phys used for non-linear address: (____ptrval____) (contig_page_data+0x0/0x1c00)
> > > > [ 0.000000] WARNING: CPU: 0 PID: 0 at arch/arm64/mm/physaddr.c:15 __virt_to_phys+0x58/0x68
> > > > [ 0.000000] Modules linked in:
> > > > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Tainted: G W 5.13.0-rc1-00074-g1140ab592e2e #3
> > > > [ 0.000000] Hardware name: linux,dummy-virt (DT)
> > > > [ 0.000000] pstate: 600000c5 (nZCv daIF -PAN -UAO -TCO BTYPE=--)
> > > > [ 0.000000] pc : __virt_to_phys+0x58/0x68
> > > > [ 0.000000] lr : __virt_to_phys+0x54/0x68
> > > > [ 0.000000] sp : ffff800011833e70
> > > > [ 0.000000] x29: ffff800011833e70 x28: 00000000418a0018 x27: 0000000000000000
> > > > [ 0.000000] x26: 000000000000000a x25: ffff800011b70000 x24: ffff800011b70000
> > > > [ 0.000000] x23: fffffc0001c00000 x22: ffff800011b70000 x21: 0000000047ffffb0
> > > > [ 0.000000] x20: 0000000000000008 x19: ffff800011b082c0 x18: ffffffffffffffff
> > > > [ 0.000000] x17: 0000000000000000 x16: ffff800011833bf9 x15: 0000000000000004
> > > > [ 0.000000] x14: 0000000000000fff x13: ffff80001186a548 x12: 0000000000000000
> > > > [ 0.000000] x11: 0000000000000000 x10: 00000000ffffffff x9 : 0000000000000000
> > > > [ 0.000000] x8 : ffff8000115c9000 x7 : 737520737968705f x6 : ffff800011b62ef8
> > > > [ 0.000000] x5 : 0000000000000000 x4 : 0000000000000001 x3 : 0000000000000000
> > > > [ 0.000000] x2 : 0000000000000000 x1 : ffff80001159585e x0 : 0000000000000058
> > > > [ 0.000000] Call trace:
> > > > [ 0.000000] __virt_to_phys+0x58/0x68
> > > > [ 0.000000] check_usemap_section_nr+0x50/0xfc
> > > > [ 0.000000] sparse_init_nid+0x1ac/0x28c
> > > > [ 0.000000] sparse_init+0x1c4/0x1e0
> > > > [ 0.000000] bootmem_init+0x60/0x90
> > > > [ 0.000000] setup_arch+0x184/0x1f0
> > > > [ 0.000000] start_kernel+0x78/0x488
> > > > [ 0.000000] ---[ end trace f68728a0d3053b60 ]---
> > > >
> > > > [1] https://urldefense.com/v3/__https://lore.kernel.org/patchwork/patch/1425110/__;!!CTRNKA9wMg0ARbw!x-wGFEC1wLzXho2kI1CrC2fjXNaQm5f-n0ADQyJDckCOKZHAP_q055DCSWYcQ7Zdcw$
> > > >
> > > > Change since v1:
> > > > - use memblock_alloc() to create pglist_data when CONFIG_NUMA=n
> > > >
> > > > Miles Chen (2):
> > > > mm: introduce prepare_node_data
> > > > mm: replace contig_page_data with node_data
> > > >
> > > > Documentation/admin-guide/kdump/vmcoreinfo.rst | 13 -------------
> > > > arch/powerpc/kexec/core.c | 5 -----
> > > > include/linux/gfp.h | 3 ---
> > > > include/linux/mm.h | 2 ++
> > > > include/linux/mmzone.h | 4 ++--
> > > > kernel/crash_core.c | 1 -
> > > > mm/memblock.c | 3 +--
> > > > mm/page_alloc.c | 16 ++++++++++++++++
> > > > mm/sparse.c | 2 ++
> > > > 9 files changed, 23 insertions(+), 26 deletions(-)
> > > >
> > > >
> > > > base-commit: 8ac91e6c6033ebc12c5c1e4aa171b81a662bd70f
> > > > --
> > > > 2.18.0
> > > >
> > >
> >
>
^ permalink raw reply
* Re: [PATCH v5 3/9] mm/mremap: Use pmd/pud_poplulate to update page table entries
From: Aneesh Kumar K.V @ 2021-05-19 4:46 UTC (permalink / raw)
To: Nathan Chancellor
Cc: npiggin, linux-mm, kaleshsingh, joel, akpm, linuxppc-dev
In-Reply-To: <YKQdxpHVYB9H0M0j@Ryzen-9-3900X.localdomain>
Nathan Chancellor <nathan@kernel.org> writes:
> Hi Aneesh,
>
> On Thu, Apr 22, 2021 at 11:13:17AM +0530, Aneesh Kumar K.V wrote:
>> pmd/pud_populate is the right interface to be used to set the respective
>> page table entries. Some architectures like ppc64 do assume that set_pmd/pud_at
>> can only be used to set a hugepage PTE. Since we are not setting up a hugepage
>> PTE here, use the pmd/pud_populate interface.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>> mm/mremap.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/mremap.c b/mm/mremap.c
>> index ec8f840399ed..574287f9bb39 100644
>> --- a/mm/mremap.c
>> +++ b/mm/mremap.c
>> @@ -26,6 +26,7 @@
>>
>> #include <asm/cacheflush.h>
>> #include <asm/tlbflush.h>
>> +#include <asm/pgalloc.h>
>>
>> #include "internal.h"
>>
>> @@ -257,9 +258,8 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,
>> pmd_clear(old_pmd);
>>
>> VM_BUG_ON(!pmd_none(*new_pmd));
>> + pmd_populate(mm, new_pmd, (pgtable_t)pmd_page_vaddr(pmd));
>>
>> - /* Set the new pmd */
>> - set_pmd_at(mm, new_addr, new_pmd, pmd);
>> flush_tlb_range(vma, old_addr, old_addr + PMD_SIZE);
>> if (new_ptl != old_ptl)
>> spin_unlock(new_ptl);
>> @@ -306,8 +306,7 @@ static bool move_normal_pud(struct vm_area_struct *vma, unsigned long old_addr,
>>
>> VM_BUG_ON(!pud_none(*new_pud));
>>
>> - /* Set the new pud */
>> - set_pud_at(mm, new_addr, new_pud, pud);
>> + pud_populate(mm, new_pud, (pmd_t *)pud_page_vaddr(pud));
>> flush_tlb_range(vma, old_addr, old_addr + PUD_SIZE);
>> if (new_ptl != old_ptl)
>> spin_unlock(new_ptl);
>> --
>> 2.30.2
>>
>>
>
> This commit causes my WSL2 VM to close when compiling something memory
> intensive, such as an x86_64_defconfig + CONFIG_LTO_CLANG_FULL=y kernel
> or LLVM/Clang. Unfortunately, I do not have much further information to
> provide since I do not see any sort of splat in dmesg right before it
> closes and I have found zero information about getting the previous
> kernel message in WSL2 (custom init so no systemd or anything).
>
> The config file is the stock one from Microsoft:
>
> https://github.com/microsoft/WSL2-Linux-Kernel/blob/a571dc8cedc8e0e56487c0dc93243e0b5db8960a/Microsoft/config-wsl
>
> I have attached my .config anyways, which includes CONFIG_DEBUG_VM,
> which does not appear to show anything out of the ordinary. I have also
> attached a dmesg just in case anything sticks out. I am happy to provide
> any additional information or perform additional debugging steps as
> needed.
>
Can you try this change?
modified mm/mremap.c
@@ -279,7 +279,7 @@ static bool move_normal_pmd(struct vm_area_struct *vma, unsigned long old_addr,
pmd_clear(old_pmd);
VM_BUG_ON(!pmd_none(*new_pmd));
- pmd_populate(mm, new_pmd, (pgtable_t)pmd_page_vaddr(pmd));
+ pmd_populate(mm, new_pmd, pmd_pgtable(pmd));
if (new_ptl != old_ptl)
spin_unlock(new_ptl);
^ permalink raw reply
* Re: Linux powerpc new system call instruction and ABI
From: Nicholas Piggin @ 2021-05-19 5:01 UTC (permalink / raw)
To: Dmitry V. Levin, Michael Ellerman
Cc: libc-alpha, Matheus Castanho, linux-api, musl, libc-dev,
linuxppc-dev
In-Reply-To: <1621385544.nttlk5qugb.astroid@bobo.none>
Excerpts from Nicholas Piggin's message of May 19, 2021 12:50 pm:
> Excerpts from Dmitry V. Levin's message of May 19, 2021 9:13 am:
>> Hi,
>>
>> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
>> [...]
>>> - Error handling: The consensus among kernel, glibc, and musl is to move to
>>> using negative return values in r3 rather than CR0[SO]=1 to indicate error,
>>> which matches most other architectures, and is closer to a function call.
>>
>> Apparently, the patchset merged by commit v5.9-rc1~100^2~164 was
>> incomplete: all functions defined in arch/powerpc/include/asm/ptrace.h and
>> arch/powerpc/include/asm/syscall.h that use ccr are broken when scv is used.
>> This includes syscall_get_error() and all its users including
>> PTRACE_GET_SYSCALL_INFO API, which in turn makes strace unusable
>> when scv is used.
>>
>> See also https://bugzilla.redhat.com/1929836
>
> I see, thanks. Using latest strace from github.com, the attached kernel
> patch makes strace -k check results a lot greener.
>
> Some of the remaining failing tests look like this (I didn't look at all
> of them yet):
>
> signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
> write(1, "signal(SIGUSR1, 0xfacefeeddeadbe"..., 50signal(SIGUSR1, 0xfacefeeddeadbeef) = 0 (SIG_DFL)
> ) = 50
> signal(SIGUSR1, SIG_IGN) = 0xfacefeeddeadbeef
> write(2, "errno2name.c:461: unknown errno "..., 41errno2name.c:461: unknown errno 559038737) = 41
> write(2, ": Unknown error 559038737\n", 26: Unknown error 559038737
> ) = 26
> exit_group(1) = ?
>
> I think the problem is glibc testing for -ve, but it should be comparing
> against -4095 (+cc Matheus)
>
> #define RET_SCV \
> cmpdi r3,0; \
> bgelr+; \
> neg r3,r3;
This glibc patch at least gets that signal test working. Haven't run the
full suite yet because of trouble making it work with a local glibc
install...
Thanks,
Nick
---
diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h
index c57bb1c05d..1ea4c3b917 100644
--- a/sysdeps/powerpc/powerpc64/sysdep.h
+++ b/sysdeps/powerpc/powerpc64/sysdep.h
@@ -398,8 +398,9 @@ LT_LABELSUFFIX(name,_name_end): ; \
#endif
#define RET_SCV \
- cmpdi r3,0; \
- bgelr+; \
+ li r9,-4095; \
+ cmpld r3,r9; \
+ bltlr+; \
neg r3,r3;
#define RET_SC \
^ permalink raw reply related
* Re: [PATCH net-next] ibmveth: fix kobj_to_dev.cocci warnings
From: Lijun Pan @ 2021-05-19 5:20 UTC (permalink / raw)
To: YueHaibing
Cc: Cristobal Forno, dwilder, linux-kernel, paulus, netdev,
Jakub Kicinski, linuxppc-dev, David Miller
In-Reply-To: <20210519022849.12752-1-yuehaibing@huawei.com>
> On May 18, 2021, at 9:28 PM, YueHaibing <yuehaibing@huawei.com> wrote:
>
> Use kobj_to_dev() instead of container_of()
>
> Generated by: scripts/coccinelle/api/kobj_to_dev.cocci
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
Acked-by: Lijun Pan <lijunp213@gmail.com>
> drivers/net/ethernet/ibm/ibmveth.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
> index 7fea9ae60f13..bc67a7ee872b 100644
> --- a/drivers/net/ethernet/ibm/ibmveth.c
> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> @@ -1799,8 +1799,7 @@ static ssize_t veth_pool_store(struct kobject *kobj, struct attribute *attr,
> struct ibmveth_buff_pool *pool = container_of(kobj,
> struct ibmveth_buff_pool,
> kobj);
> - struct net_device *netdev = dev_get_drvdata(
> - container_of(kobj->parent, struct device, kobj));
> + struct net_device *netdev = dev_get_drvdata(kobj_to_dev(kobj->parent));
> struct ibmveth_adapter *adapter = netdev_priv(netdev);
> long value = simple_strtol(buf, NULL, 10);
> long rc;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3] powerpc/papr_scm: Reduce error severity if nvdimm stats inaccessible
From: kajoljain @ 2021-05-19 7:06 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev, linux-nvdimm
Cc: Aneesh Kumar K . V, Santosh Sivaraj, Ira Weiny, Dan Williams
In-Reply-To: <20210508043642.114076-1-vaibhav@linux.ibm.com>
On 5/8/21 10:06 AM, Vaibhav Jain wrote:
> Currently drc_pmem_qeury_stats() generates a dev_err in case
> "Enable Performance Information Collection" feature is disabled from
> HMC or performance stats are not available for an nvdimm. The error is
> of the form below:
>
> papr_scm ibm,persistent-memory:ibm,pmemory@44104001: Failed to query
> performance stats, Err:-10
>
> This error message confuses users as it implies a possible problem
> with the nvdimm even though its due to a disabled/unavailable
> feature. We fix this by explicitly handling the H_AUTHORITY and
> H_UNSUPPORTED errors from the H_SCM_PERFORMANCE_STATS hcall.
>
> In case of H_AUTHORITY error an info message is logged instead of an
> error, saying that "Permission denied while accessing performance
> stats" and an EPERM error is returned back.
>
> In case of H_UNSUPPORTED error we return a EOPNOTSUPP error back from
> drc_pmem_query_stats() indicating that performance stats-query
> operation is not supported on this nvdimm.
>
> Fixes: 2d02bf835e57('powerpc/papr_scm: Fetch nvdimm performance stats from PHYP')
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Changelog
>
> v3:
> * Return EOPNOTSUPP error in case of H_UNSUPPORTED [ Ira ]
> * Return EPERM in case of H_AUTHORITY [ Ira ]
> * Updated patch description
>
Patch looks good to me.
Reviewed-By: Kajol Jain<kjain@linux.ibm.com>
Thanks,
Kajol Jain
> v2:
> * Updated the message logged in case of H_AUTHORITY error [ Ira ]
> * Switched from dev_warn to dev_info in case of H_AUTHORITY error.
> * Instead of -EPERM return -EACCESS for H_AUTHORITY error.
> * Added explicit handling of H_UNSUPPORTED error.
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index ef26fe40efb0..e2b69cc3beaf 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -310,6 +310,13 @@ static ssize_t drc_pmem_query_stats(struct papr_scm_priv *p,
> dev_err(&p->pdev->dev,
> "Unknown performance stats, Err:0x%016lX\n", ret[0]);
> return -ENOENT;
> + } else if (rc == H_AUTHORITY) {
> + dev_info(&p->pdev->dev,
> + "Permission denied while accessing performance stats");
> + return -EPERM;
> + } else if (rc == H_UNSUPPORTED) {
> + dev_dbg(&p->pdev->dev, "Performance stats unsupported\n");
> + return -EOPNOTSUPP;
> } else if (rc != H_SUCCESS) {
> dev_err(&p->pdev->dev,
> "Failed to query performance stats, Err:%lld\n", rc);
>
^ permalink raw reply
* Re: Linux powerpc new system call instruction and ABI
From: Joakim Tjernlund @ 2021-05-19 7:33 UTC (permalink / raw)
To: ldv@altlinux.org, mpe@ellerman.id.au, npiggin@gmail.com
Cc: libc-dev@lists.llvm.org, linux-api@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, musl@lists.openwall.com
In-Reply-To: <20210518231331.GA8464@altlinux.org>
On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote:
> Hi,
>
> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
> [...]
> > - Error handling: The consensus among kernel, glibc, and musl is to move to
> > using negative return values in r3 rather than CR0[SO]=1 to indicate error,
> > which matches most other architectures, and is closer to a function call.
What about syscalls like times(2) which can return -1 without it being an error?
Jocke
^ permalink raw reply
* Re: Linux powerpc new system call instruction and ABI
From: Nicholas Piggin @ 2021-05-19 7:55 UTC (permalink / raw)
To: Joakim Tjernlund, ldv@altlinux.org, mpe@ellerman.id.au
Cc: libc-dev@lists.llvm.org, musl@lists.openwall.com,
linuxppc-dev@lists.ozlabs.org, linux-api@vger.kernel.org
In-Reply-To: <9b5ea7059c5a5d4e9ccccd7d73ce2c66b2203f52.camel@infinera.com>
Excerpts from Joakim Tjernlund's message of May 19, 2021 5:33 pm:
> On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote:
>> Hi,
>>
>> On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
>> [...]
>> > - Error handling: The consensus among kernel, glibc, and musl is to move to
>> > using negative return values in r3 rather than CR0[SO]=1 to indicate error,
>> > which matches most other architectures, and is closer to a function call.
>
> What about syscalls like times(2) which can return -1 without it being an error?
They do become errors / indistinguishable and have to be dealt with by
libc or userspace. Which does follow what most architectures do (all
except ia64, mips, sparc, and powerpc actually).
Interesting question though, it should have been noted.
Thanks,
Nick
^ permalink raw reply
* Re: [FSL P50x0] KVM HV doesn't work anymore
From: Nicholas Piggin @ 2021-05-19 7:57 UTC (permalink / raw)
To: Christophe Leroy, Christian Zigotzky, kvm-ppc@vger.kernel.org,
linuxppc-dev
Cc: Darren Stevens, R.T.Dickinson, mad skateman, Christian Zigotzky
In-Reply-To: <e6ed7674-3df9-ec3e-8bcf-dcd8ff0fecf8@xenosoft.de>
Excerpts from Christian Zigotzky's message of May 17, 2021 7:42 pm:
> On 17 May 2021 at 09:42am, Nicholas Piggin wrote:
>> Excerpts from Christian Zigotzky's message of May 15, 2021 11:46 pm:
>>> On 15 May 2021 at 12:08pm Christophe Leroy wrote:
>>>>
>>>> Le 15/05/2021 à 11:48, Christian Zigotzky a écrit :
>>>>> Hi All,
>>>>>
>>>>> I bisected today [1] and the bisecting itself was OK but the
>>>>> reverting of the bad commit doesn't solve the issue. Do you have an
>>>>> idea which commit could be resposible for this issue? Maybe the
>>>>> bisecting wasn't successful. I will look in the kernel git log. Maybe
>>>>> there is a commit that affected KVM HV on FSL P50x0 machines.
>>>> If the uImage doesn't load, it may be because of the size of uImage.
>>>>
>>>> See https://github.com/linuxppc/issues/issues/208
>>>>
>>>> Is there a significant size difference with and without KVM HV ?
>>>>
>>>> Maybe you can try to remove another option to reduce the size of the
>>>> uImage.
>>> I tried it but it doesn't solve the issue. The uImage works without KVM
>>> HV in a virtual e5500 QEMU machine.
>> Any more progress with this? I would say that bisect might have just
>> been a bit unstable and maybe by chance some things did not crash so
>> it's pointing to the wrong patch.
>>
>> Upstream merge of powerpc-5.13-1 was good and powerpc-5.13-2 was bad?
>>
>> Between that looks like some KVM MMU rework. You could try the patch
>> before this one b1c5356e873c ("KVM: PPC: Convert to the gfn-based MMU
>> notifier callbacks"). That won't revert cleanly so just try run the
>> tree at that point. If it works, test the patch and see if it fails.
>>
>> Thanks,
>> Nick
> Hi Nick,
>
> Thanks a lot for your answer. Yes, there is a little bit of progress.
> The RC2 of kernel 5.13 successfully boots with -smp 3 in a virtual e5500
> QEMU machine.
> -smp 4 doesn't work anymore since the PowerPC updates 5.13-2. I used
> -smp 4 before 5.13 because my FSL P5040 machine has 4 cores.
>
> Could you please post a patch for reverting the commit before
> b1c5356e873c ("KVM: PPC: Convert to the gfn-based MMU notifier callbacks")?
You could `git checkout b1c5356e873c~1`
Thanks,
Nick
>
^ permalink raw reply
* Re: Linux powerpc new system call instruction and ABI
From: Joakim Tjernlund @ 2021-05-19 8:08 UTC (permalink / raw)
To: ldv@altlinux.org, mpe@ellerman.id.au, npiggin@gmail.com
Cc: libc-dev@lists.llvm.org, linux-api@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, musl@lists.openwall.com
In-Reply-To: <1621410291.c7si38sa9q.astroid@bobo.none>
On Wed, 2021-05-19 at 17:55 +1000, Nicholas Piggin wrote:
> Excerpts from Joakim Tjernlund's message of May 19, 2021 5:33 pm:
> > On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote:
> > > Hi,
> > >
> > > On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
> > > [...]
> > > > - Error handling: The consensus among kernel, glibc, and musl is to move to
> > > > using negative return values in r3 rather than CR0[SO]=1 to indicate error,
> > > > which matches most other architectures, and is closer to a function call.
> >
> > What about syscalls like times(2) which can return -1 without it being an error?
>
> They do become errors / indistinguishable and have to be dealt with by
> libc or userspace. Which does follow what most architectures do (all
> except ia64, mips, sparc, and powerpc actually).
>
> Interesting question though, it should have been noted.
>
> Thanks,
> Nick
I always figured the ppc way was superior. It begs the question if not the other archs should
change instead?
Jocke
^ permalink raw reply
* Re: [PATCH v8 27/30] powerpc/kprobes: Don't allow breakpoints on suffixes
From: Naveen N. Rao @ 2021-05-19 8:11 UTC (permalink / raw)
To: Christophe Leroy, Jordan Niethe, linuxppc-dev
Cc: alistair, npiggin, bala24, dja
In-Reply-To: <cda38a1c-a78f-9eb5-8395-ce23caa2c81d@csgroup.eu>
Christophe Leroy wrote:
>
>
> Le 06/05/2020 à 05:40, Jordan Niethe a écrit :
>> Do not allow inserting breakpoints on the suffix of a prefix instruction
>> in kprobes.
>>
>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>> ---
>> v8: Add this back from v3
>> ---
>> arch/powerpc/kernel/kprobes.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
>> index 33d54b091c70..227510df8c55 100644
>> --- a/arch/powerpc/kernel/kprobes.c
>> +++ b/arch/powerpc/kernel/kprobes.c
>> @@ -106,7 +106,9 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
>> int arch_prepare_kprobe(struct kprobe *p)
>> {
>> int ret = 0;
>> + struct kprobe *prev;
>> struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
>> + struct ppc_inst prefix = ppc_inst_read((struct ppc_inst *)(p->addr - 1));
>
> What if p->addr is the first word of a page and the previous page is
> not mapped ?
Good catch!
I think we can just skip validation if the instruction is at the
beginning of a page. I have a few cleanups in this area - I will post a
patchset soon.
>
>>
>> if ((unsigned long)p->addr & 0x03) {
>> printk("Attempt to register kprobe at an unaligned address\n");
>> @@ -114,6 +116,17 @@ int arch_prepare_kprobe(struct kprobe *p)
>> } else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
>> printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
>> ret = -EINVAL;
>> + } else if (ppc_inst_prefixed(prefix)) {
>
> If p->addr - 2 contains a valid prefixed instruction, then p->addr - 1 contains the suffix of that
> prefixed instruction. Are we sure a suffix can never ever be misinterpreted as the prefix of a
> prefixed instruction ?
Yes. Per the ISA:
Bits 0:5 of all prefixes are assigned the primary opcode
value 0b000001. 0b000001 is not available for use as a
primary opcode for either word instructions or suffixes
of prefixed instructions.
- Naveen
^ permalink raw reply
* Re: Linux powerpc new system call instruction and ABI
From: Nicholas Piggin @ 2021-05-19 8:42 UTC (permalink / raw)
To: Joakim Tjernlund, ldv@altlinux.org, mpe@ellerman.id.au
Cc: libc-dev@lists.llvm.org, musl@lists.openwall.com,
linuxppc-dev@lists.ozlabs.org, linux-api@vger.kernel.org
In-Reply-To: <fb9b6a6099855bd00efc6ffe540ccad14dd9a365.camel@infinera.com>
Excerpts from Joakim Tjernlund's message of May 19, 2021 6:08 pm:
> On Wed, 2021-05-19 at 17:55 +1000, Nicholas Piggin wrote:
>> Excerpts from Joakim Tjernlund's message of May 19, 2021 5:33 pm:
>> > On Wed, 2021-05-19 at 02:13 +0300, Dmitry V. Levin wrote:
>> > > Hi,
>> > >
>> > > On Thu, Jun 11, 2020 at 06:12:01PM +1000, Nicholas Piggin wrote:
>> > > [...]
>> > > > - Error handling: The consensus among kernel, glibc, and musl is to move to
>> > > > using negative return values in r3 rather than CR0[SO]=1 to indicate error,
>> > > > which matches most other architectures, and is closer to a function call.
>> >
>> > What about syscalls like times(2) which can return -1 without it being an error?
>>
>> They do become errors / indistinguishable and have to be dealt with by
>> libc or userspace. Which does follow what most architectures do (all
>> except ia64, mips, sparc, and powerpc actually).
>>
>> Interesting question though, it should have been noted.
>>
>> Thanks,
>> Nick
>
> I always figured the ppc way was superior. It begs the question if not the other archs should
> change instead?
It is superior in some ways, not enough to be worth being different.
Other archs are unlikely to change because it would be painful for
not much benefit. New system calls just should be made to not return
error numbers. If we ever had a big new version of syscall ABI in
Linux, we can always use another scv vector number for it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v3 5/6] sched/fair: Consider SMT in ASYM_PACKING load balance
From: Peter Zijlstra @ 2021-05-19 9:59 UTC (permalink / raw)
To: Ricardo Neri
Cc: Juri Lelli, Len Brown, Quentin Perret, Vincent Guittot,
Ravi V. Shankar, linux-kernel, Aubrey Li,
Daniel Bristot de Oliveira, Ricardo Neri, Steven Rostedt,
Dietmar Eggemann, Ben Segall, linuxppc-dev, Mel Gorman,
Srinivas Pandruvada, Joel Fernandes (Google), Tim Chen,
Ingo Molnar, Aubrey Li
In-Reply-To: <20210518190740.GA15251@ranerica-svr.sc.intel.com>
On Tue, May 18, 2021 at 12:07:40PM -0700, Ricardo Neri wrote:
> On Fri, May 14, 2021 at 07:14:15PM -0700, Ricardo Neri wrote:
> > On Fri, May 14, 2021 at 11:47:45AM +0200, Peter Zijlstra wrote:
> > > So I'm thinking that this is a property of having ASYM_PACKING at a core
> > > level, rather than some arch special. Wouldn't something like this be
> > > more appropriate?
> > Thanks Peter for the quick review! This makes sense to me. The only
> > reason we proposed arch_asym_check_smt_siblings() is because we were
> > about breaking powerpc (I need to study how they set priorities for SMT,
> > if applicable). If you think this is not an issue I can post a
> > v4 with this update.
>
> As far as I can see, priorities in powerpc are set by the CPU number.
> However, I am not sure how CPUs are enumerated? If CPUs in brackets are
> SMT sibling, Does an enumeration looks like A) [0, 1], [2, 3] or B) [0, 2],
> [1, 3]? I guess B is the right answer. Otherwise, both SMT siblings of a
> core would need to be busy before a new core is used.
>
> Still, I think the issue described in the cover letter may be
> reproducible in powerpc as well. If CPU3 is offlined, and [0, 2] pulled
> tasks from [1, -] so that both CPU0 and CPU2 become busy, CPU1 would not be
> able to help since CPU0 has the highest priority.
>
> I am cc'ing the linuxppc list to get some feedback.
IIRC the concern with Power is that their Cores can go faster if the
higher SMT siblings are unused.
That is, suppose you have an SMT4 Core with only a single active task,
then if only SMT0 is used it can reach max performance, but if the
active sibling is SMT1 it can not reach max performance, and if the only
active sibling is SMT2 it goes slower still.
So they need to pack the tasks to the lowest SMT siblings, and have the
highest SMT siblings idle (where possible) in order to increase
performance.
^ permalink raw reply
* Re: Linux powerpc new system call instruction and ABI
From: Dmitry V. Levin @ 2021-05-19 10:24 UTC (permalink / raw)
To: Nicholas Piggin
Cc: libc-alpha, Matheus Castanho, musl, linux-api, libc-dev,
linuxppc-dev
In-Reply-To: <1621385544.nttlk5qugb.astroid@bobo.none>
On Wed, May 19, 2021 at 12:50:24PM +1000, Nicholas Piggin wrote:
[...]
> With this patch, I think the ptrace ABI should mostly be fixed. I think
> a problem remains with applications that look at system call return
> registers directly and have powerpc specific error cases. Those probably
> will just need to be updated unfortunately. Michael thought it might be
> possible to return an indication via ptrace somehow that the syscall is
> using a new ABI, so such apps can be updated to test for it. I don't
> know how that would be done.
Is there any sane way for these applications to handle the scv case?
How can they tell that the scv semantics is being used for the given
syscall invocation? Can this information be obtained e.g. from struct
pt_regs?
For example, in strace we have the following powerpc-specific code used
for syscall tampering:
$ cat src/linux/powerpc/set_error.c
/*
* Copyright (c) 2016-2021 The strace developers.
* All rights reserved.
*
* SPDX-License-Identifier: LGPL-2.1-or-later
*/
static int
arch_set_r3_ccr(struct tcb *tcp, const unsigned long r3,
const unsigned long ccr_set, const unsigned long ccr_clear)
{
if (ptrace_syscall_info_is_valid() &&
upeek(tcp, sizeof(long) * PT_CCR, &ppc_regs.ccr))
return -1;
const unsigned long old_ccr = ppc_regs.ccr;
ppc_regs.gpr[3] = r3;
ppc_regs.ccr |= ccr_set;
ppc_regs.ccr &= ~ccr_clear;
if (ppc_regs.ccr != old_ccr &&
upoke(tcp, sizeof(long) * PT_CCR, ppc_regs.ccr))
return -1;
return upoke(tcp, sizeof(long) * (PT_R0 + 3), ppc_regs.gpr[3]);
}
static int
arch_set_error(struct tcb *tcp)
{
return arch_set_r3_ccr(tcp, tcp->u_error, 0x10000000, 0);
}
static int
arch_set_success(struct tcb *tcp)
{
return arch_set_r3_ccr(tcp, tcp->u_rval, 0, 0x10000000);
}
--
ldv
^ permalink raw reply
* [PATCH 0/5] powerpc/kprobes: fixes and cleanups
From: Naveen N. Rao @ 2021-05-19 10:47 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Jordan Niethe; +Cc: linuxppc-dev
Various fixes and some code refactoring for kprobes on powerpc. The
first patch fixes an invalid access if probing the first instruction in
a kernel module. The rest are small cleanups. More details in the
individual patches.
- Naveen
Naveen N. Rao (5):
powerpc/kprobes: Fix validation of prefixed instructions across page
boundary
powerpc/kprobes: Roll IS_RFI() macro into IS_RFID()
powerpc/kprobes: Check instruction validity during kprobe registration
powerpc/kprobes: Refactor arch_prepare_kprobe()
powerpc/kprobes: Warn if instruction patching failed
arch/powerpc/include/asm/sstep.h | 7 +-
arch/powerpc/kernel/kprobes.c | 112 +++++++++++--------------------
2 files changed, 43 insertions(+), 76 deletions(-)
base-commit: 3a81c0495fdb91fd9a9b4f617098c283131eeae1
--
2.30.2
^ permalink raw reply
* [PATCH 1/5] powerpc/kprobes: Fix validation of prefixed instructions across page boundary
From: Naveen N. Rao @ 2021-05-19 10:47 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Jordan Niethe; +Cc: linuxppc-dev
In-Reply-To: <cover.1621416666.git.naveen.n.rao@linux.vnet.ibm.com>
When checking if the probed instruction is the suffix of a prefixed
instruction, we access the instruction at the previous word. If the
probed instruction is the very first word of a module, we can end up
trying to access an invalid page. Fix this by skipping the check for all
instructions at the beginning of a page. Prefixed instructions cannot
cross a 64-byte boundary and as such, preventing probing on such
instructions is not worthwhile.
Cc: stable@vger.kernel.org # v5.8+
Fixes: b4657f7650babc ("powerpc/kprobes: Don't allow breakpoints on suffixes")
Reported-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/kprobes.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 01ab2163659e4b..f611d9eb3562d7 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -108,7 +108,6 @@ int arch_prepare_kprobe(struct kprobe *p)
int ret = 0;
struct kprobe *prev;
struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
- struct ppc_inst prefix = ppc_inst_read((struct ppc_inst *)(p->addr - 1));
if ((unsigned long)p->addr & 0x03) {
printk("Attempt to register kprobe at an unaligned address\n");
@@ -116,7 +115,8 @@ int arch_prepare_kprobe(struct kprobe *p)
} else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
ret = -EINVAL;
- } else if (ppc_inst_prefixed(prefix)) {
+ } else if ((unsigned long)p->addr & ~PAGE_MASK &&
+ ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)(p->addr - 1)))) {
printk("Cannot register a kprobe on the second word of prefixed instruction\n");
ret = -EINVAL;
}
--
2.30.2
^ permalink raw reply related
* [PATCH 2/5] powerpc/kprobes: Roll IS_RFI() macro into IS_RFID()
From: Naveen N. Rao @ 2021-05-19 10:47 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Jordan Niethe; +Cc: linuxppc-dev
In-Reply-To: <cover.1621416666.git.naveen.n.rao@linux.vnet.ibm.com>
In kprobes and xmon, we should exclude both 32-bit and 64-bit variants
of mtmsr and rfi instructions from being stepped. Have IS_RFID() also
detect a rfi instruction similar to IS_MTMSRD().
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/sstep.h | 7 +++----
arch/powerpc/kernel/kprobes.c | 4 ++--
2 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 972ed0df154d60..1df867c2e054e5 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -13,12 +13,11 @@ struct pt_regs;
* we don't allow putting a breakpoint on an mtmsrd instruction.
* Similarly we don't allow breakpoints on rfid instructions.
* These macros tell us if an instruction is a mtmsrd or rfid.
- * Note that IS_MTMSRD returns true for both an mtmsr (32-bit)
- * and an mtmsrd (64-bit).
+ * Note that these return true for both mtmsr/rfi (32-bit)
+ * and mtmsrd/rfid (64-bit).
*/
#define IS_MTMSRD(instr) ((ppc_inst_val(instr) & 0xfc0007be) == 0x7c000124)
-#define IS_RFID(instr) ((ppc_inst_val(instr) & 0xfc0007fe) == 0x4c000024)
-#define IS_RFI(instr) ((ppc_inst_val(instr) & 0xfc0007fe) == 0x4c000064)
+#define IS_RFID(instr) ((ppc_inst_val(instr) & 0xfc0007be) == 0x4c000024)
enum instruction_type {
COMPUTE, /* arith/logical/CR op, etc. */
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index f611d9eb3562d7..b7b20875d34d91 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -112,8 +112,8 @@ int arch_prepare_kprobe(struct kprobe *p)
if ((unsigned long)p->addr & 0x03) {
printk("Attempt to register kprobe at an unaligned address\n");
ret = -EINVAL;
- } else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) {
- printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n");
+ } else if (IS_MTMSRD(insn) || IS_RFID(insn)) {
+ printk("Cannot register a kprobe on mtmsr[d]/rfi[d]\n");
ret = -EINVAL;
} else if ((unsigned long)p->addr & ~PAGE_MASK &&
ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)(p->addr - 1)))) {
--
2.30.2
^ permalink raw reply related
* [PATCH 3/5] powerpc/kprobes: Check instruction validity during kprobe registration
From: Naveen N. Rao @ 2021-05-19 10:47 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Jordan Niethe; +Cc: linuxppc-dev
In-Reply-To: <cover.1621416666.git.naveen.n.rao@linux.vnet.ibm.com>
In trap-based (classic) kprobes, we try to emulate the probed
instruction so as to avoid having to single step it. We use a flag to
determine if the probed instruction was successfully emulated, so that we
can speed up subsequent probe hits.
However, emulate_step() doesn't differentiate between unknown
instructions and an emulation attempt that failed. As such, the current
heuristic is not of much use. Instead, use analyse_instr() during kprobe
registration to determine if the probed instruction can be decoded by
our instruction emulation infrastructure. For unknown instructions, we
can then directly single-step while for other instructions, we can
attempt to emulate and fall back to single stepping if that fails.
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/kprobes.c | 62 +++++++++--------------------------
1 file changed, 16 insertions(+), 46 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index b7b20875d34d91..bbef9e918ecb39 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -107,6 +107,8 @@ int arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
struct kprobe *prev;
+ struct pt_regs regs;
+ struct instruction_op op;
struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
if ((unsigned long)p->addr & 0x03) {
@@ -140,9 +142,18 @@ int arch_prepare_kprobe(struct kprobe *p)
if (!ret) {
patch_instruction((struct ppc_inst *)p->ainsn.insn, insn);
p->opcode = ppc_inst_val(insn);
+
+ /* Check if this is an instruction we recognise */
+ p->ainsn.boostable = 0;
+ memset(®s, 0, sizeof(struct pt_regs));
+ regs.nip = (unsigned long)p->addr;
+ regs.msr = MSR_KERNEL;
+ ret = analyse_instr(&op, ®s, insn);
+ if (ret == 1 || (ret == 0 && GETTYPE(op.type) != UNKNOWN))
+ p->ainsn.boostable = 1;
+ ret = 0;
}
- p->ainsn.boostable = 0;
return ret;
}
NOKPROBE_SYMBOL(arch_prepare_kprobe);
@@ -225,47 +236,6 @@ void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs)
}
NOKPROBE_SYMBOL(arch_prepare_kretprobe);
-static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
-{
- int ret;
- struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->ainsn.insn);
-
- /* regs->nip is also adjusted if emulate_step returns 1 */
- ret = emulate_step(regs, insn);
- if (ret > 0) {
- /*
- * Once this instruction has been boosted
- * successfully, set the boostable flag
- */
- if (unlikely(p->ainsn.boostable == 0))
- p->ainsn.boostable = 1;
- } else if (ret < 0) {
- /*
- * We don't allow kprobes on mtmsr(d)/rfi(d), etc.
- * So, we should never get here... but, its still
- * good to catch them, just in case...
- */
- printk("Can't step on instruction %s\n", ppc_inst_as_str(insn));
- BUG();
- } else {
- /*
- * If we haven't previously emulated this instruction, then it
- * can't be boosted. Note it down so we don't try to do so again.
- *
- * If, however, we had emulated this instruction in the past,
- * then this is just an error with the current run (for
- * instance, exceptions due to a load/store). We return 0 so
- * that this is now single-stepped, but continue to try
- * emulating it in subsequent probe hits.
- */
- if (unlikely(p->ainsn.boostable != 1))
- p->ainsn.boostable = -1;
- }
-
- return ret;
-}
-NOKPROBE_SYMBOL(try_to_emulate);
-
int kprobe_handler(struct pt_regs *regs)
{
struct kprobe *p;
@@ -334,8 +304,8 @@ int kprobe_handler(struct pt_regs *regs)
set_current_kprobe(p, regs, kcb);
kprobes_inc_nmissed_count(p);
kcb->kprobe_status = KPROBE_REENTER;
- if (p->ainsn.boostable >= 0) {
- ret = try_to_emulate(p, regs);
+ if (p->ainsn.boostable) {
+ ret = emulate_step(regs, ppc_inst_read((struct ppc_inst *)p->ainsn.insn));
if (ret > 0) {
restore_previous_kprobe(kcb);
@@ -356,8 +326,8 @@ int kprobe_handler(struct pt_regs *regs)
return 1;
}
- if (p->ainsn.boostable >= 0) {
- ret = try_to_emulate(p, regs);
+ if (p->ainsn.boostable) {
+ ret = emulate_step(regs, ppc_inst_read((struct ppc_inst *)p->ainsn.insn));
if (ret > 0) {
if (p->post_handler)
--
2.30.2
^ permalink raw reply related
* [PATCH 4/5] powerpc/kprobes: Refactor arch_prepare_kprobe()
From: Naveen N. Rao @ 2021-05-19 10:47 UTC (permalink / raw)
To: Michael Ellerman, Christophe Leroy, Jordan Niethe; +Cc: linuxppc-dev
In-Reply-To: <cover.1621416666.git.naveen.n.rao@linux.vnet.ibm.com>
Clean up the function to look sane:
- return immediately on error, rather than pointlessly setting the
return value
- pr_info() instead of printk()
- check return value of patch_instruction()
- and to top it all of: a reverse christmas tree!
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/kprobes.c | 64 +++++++++++++++++------------------
1 file changed, 31 insertions(+), 33 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index bbef9e918ecb39..7195162362941f 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -105,56 +105,54 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
int arch_prepare_kprobe(struct kprobe *p)
{
- int ret = 0;
+ struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
+ struct instruction_op op;
struct kprobe *prev;
struct pt_regs regs;
- struct instruction_op op;
- struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr);
+ int ret = 0;
if ((unsigned long)p->addr & 0x03) {
- printk("Attempt to register kprobe at an unaligned address\n");
- ret = -EINVAL;
+ pr_info("Attempt to register kprobe at an unaligned address\n");
+ return -EINVAL;
} else if (IS_MTMSRD(insn) || IS_RFID(insn)) {
- printk("Cannot register a kprobe on mtmsr[d]/rfi[d]\n");
- ret = -EINVAL;
+ pr_info("Cannot register a kprobe on mtmsr[d]/rfi[d]\n");
+ return -EINVAL;
} else if ((unsigned long)p->addr & ~PAGE_MASK &&
ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)(p->addr - 1)))) {
- printk("Cannot register a kprobe on the second word of prefixed instruction\n");
- ret = -EINVAL;
+ pr_info("Cannot register a kprobe on the second word of prefixed instruction\n");
+ return -EINVAL;
}
+
+ /* Check if the previous instruction is a prefix instruction with an active kprobe */
preempt_disable();
prev = get_kprobe(p->addr - 1);
preempt_enable_no_resched();
- if (prev &&
- ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)prev->ainsn.insn))) {
- printk("Cannot register a kprobe on the second word of prefixed instruction\n");
- ret = -EINVAL;
+ if (prev && ppc_inst_prefixed(ppc_inst_read((struct ppc_inst *)prev->ainsn.insn))) {
+ pr_info("Cannot register a kprobe on the second word of prefixed instruction\n");
+ return -EINVAL;
}
/* insn must be on a special executable page on ppc64. This is
* not explicitly required on ppc32 (right now), but it doesn't hurt */
- if (!ret) {
- p->ainsn.insn = get_insn_slot();
- if (!p->ainsn.insn)
- ret = -ENOMEM;
- }
+ p->ainsn.insn = get_insn_slot();
+ if (!p->ainsn.insn)
+ return -ENOMEM;
- if (!ret) {
- patch_instruction((struct ppc_inst *)p->ainsn.insn, insn);
- p->opcode = ppc_inst_val(insn);
-
- /* Check if this is an instruction we recognise */
- p->ainsn.boostable = 0;
- memset(®s, 0, sizeof(struct pt_regs));
- regs.nip = (unsigned long)p->addr;
- regs.msr = MSR_KERNEL;
- ret = analyse_instr(&op, ®s, insn);
- if (ret == 1 || (ret == 0 && GETTYPE(op.type) != UNKNOWN))
- p->ainsn.boostable = 1;
- ret = 0;
- }
+ if (patch_instruction((struct ppc_inst *)p->ainsn.insn, insn))
+ return -EFAULT;
- return ret;
+ p->opcode = ppc_inst_val(insn);
+
+ /* Check if this is an instruction we recognise */
+ p->ainsn.boostable = 0;
+ memset(®s, 0, sizeof(struct pt_regs));
+ regs.nip = (unsigned long)p->addr;
+ regs.msr = MSR_KERNEL;
+ ret = analyse_instr(&op, ®s, insn);
+ if (ret == 1 || (ret == 0 && GETTYPE(op.type) != UNKNOWN))
+ p->ainsn.boostable = 1;
+
+ return 0;
}
NOKPROBE_SYMBOL(arch_prepare_kprobe);
--
2.30.2
^ permalink raw reply related
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