2021-11-29 17:52:01

by Christophe Leroy

Subject: [PATCH v5 1/5] powerpc/inst: Refactor ___get_user_instr()

PPC64 version of ___get_user_instr() can be used for PPC32 as well,
by simply disabling the suffix part with IS_ENABLED(CONFIG_PPC64).

Signed-off-by: Christophe Leroy <[email protected]>
---
v5: Force use of 'y' in ppc_inst_prefix on PPC32 to avoid 'use variable' warning with W=1
---
arch/powerpc/include/asm/inst.h | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index b11c0e2f9639..10a5c1b76ca0 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -4,8 +4,6 @@

#include <asm/ppc-opcode.h>

-#ifdef CONFIG_PPC64
-
#define ___get_user_instr(gu_op, dest, ptr) \
({ \
long __gui_ret; \
@@ -16,7 +14,7 @@
__chk_user_ptr(ptr); \
__gui_ret = gu_op(__prefix, __gui_ptr); \
if (__gui_ret == 0) { \
- if ((__prefix >> 26) == OP_PREFIX) { \
+ if (IS_ENABLED(CONFIG_PPC64) && (__prefix >> 26) == OP_PREFIX) { \
__gui_ret = gu_op(__suffix, __gui_ptr + 1); \
__gui_inst = ppc_inst_prefix(__prefix, __suffix); \
} else { \
@@ -27,13 +25,6 @@
} \
__gui_ret; \
})
-#else /* !CONFIG_PPC64 */
-#define ___get_user_instr(gu_op, dest, ptr) \
-({ \
- __chk_user_ptr(ptr); \
- gu_op((dest).val, (u32 __user *)(ptr)); \
-})
-#endif /* CONFIG_PPC64 */

#define get_user_instr(x, ptr) ___get_user_instr(get_user, x, ptr)

@@ -71,7 +62,7 @@ static inline u32 ppc_inst_suffix(struct ppc_inst x)
}

#else
-#define ppc_inst_prefix(x, y) ppc_inst(x)
+#define ppc_inst_prefix(x, y) ((void)y, ppc_inst(x))

static inline u32 ppc_inst_suffix(struct ppc_inst x)
{
--
2.33.1

2021-11-29 17:54:05

by Christophe Leroy

Subject: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

copy_inst_from_kernel_nofault() uses copy_from_kernel_nofault() to
copy one or two 32bits words. This means calling an out-of-line
function which itself calls back copy_from_kernel_nofault_allowed()
then performs a generic copy with loops.

Rewrite copy_inst_from_kernel_nofault() to do everything at a
single place and use __get_kernel_nofault() directly to perform
single accesses without loops.

Allthough the generic function uses pagefault_disable(), it is not
required on powerpc because do_page_fault() bails earlier when a
kernel mode fault happens on a kernel address.

As the function has now become very small, inline it.

With this change, on an 8xx the time spent in the loop in
ftrace_replace_code() is reduced by 23% at function tracer activation
and 27% at nop tracer activation.
The overall time to activate function tracer (measured with shell
command 'time') is 570ms before the patch and 470ms after the patch.

Even vmlinux size is reduced (by 152 instruction).

Before the patch:

00000018 <copy_inst_from_kernel_nofault>:
18: 94 21 ff e0 stwu r1,-32(r1)
1c: 7c 08 02 a6 mflr r0
20: 38 a0 00 04 li r5,4
24: 93 e1 00 1c stw r31,28(r1)
28: 7c 7f 1b 78 mr r31,r3
2c: 38 61 00 08 addi r3,r1,8
30: 90 01 00 24 stw r0,36(r1)
34: 48 00 00 01 bl 34 <copy_inst_from_kernel_nofault+0x1c>
34: R_PPC_REL24 copy_from_kernel_nofault
38: 2c 03 00 00 cmpwi r3,0
3c: 40 82 00 0c bne 48 <copy_inst_from_kernel_nofault+0x30>
40: 81 21 00 08 lwz r9,8(r1)
44: 91 3f 00 00 stw r9,0(r31)
48: 80 01 00 24 lwz r0,36(r1)
4c: 83 e1 00 1c lwz r31,28(r1)
50: 38 21 00 20 addi r1,r1,32
54: 7c 08 03 a6 mtlr r0
58: 4e 80 00 20 blr

After the patch (before inlining):

00000018 <copy_inst_from_kernel_nofault>:
18: 3d 20 b0 00 lis r9,-20480
1c: 7c 04 48 40 cmplw r4,r9
20: 7c 69 1b 78 mr r9,r3
24: 41 80 00 14 blt 38 <copy_inst_from_kernel_nofault+0x20>
28: 81 44 00 00 lwz r10,0(r4)
2c: 38 60 00 00 li r3,0
30: 91 49 00 00 stw r10,0(r9)
34: 4e 80 00 20 blr

38: 38 60 ff de li r3,-34
3c: 4e 80 00 20 blr
40: 38 60 ff f2 li r3,-14
44: 4e 80 00 20 blr

Signed-off-by: Christophe Leroy <[email protected]>
---
v4: Inline and remove pagefault_disable()

v3: New
---
arch/powerpc/include/asm/inst.h | 21 ++++++++++++++++++++-
arch/powerpc/mm/maccess.c | 17 -----------------
2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 53a40faf362a..631436f3f5c3 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -4,6 +4,8 @@

#include <asm/ppc-opcode.h>
#include <asm/reg.h>
+#include <asm/disassemble.h>
+#include <asm/uaccess.h>

#define ___get_user_instr(gu_op, dest, ptr) \
({ \
@@ -148,6 +150,23 @@ static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], ppc_inst_t x)
__str; \
})

-int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src);
+static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
+{
+ unsigned int val, suffix;
+
+ if (unlikely(!is_kernel_addr((unsigned long)src)))
+ return -ERANGE;
+
+ __get_kernel_nofault(&val, src, u32, Efault);
+ if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
+ __get_kernel_nofault(&suffix, src + 1, u32, Efault);
+ *inst = ppc_inst_prefix(val, suffix);
+ } else {
+ *inst = ppc_inst(val);
+ }
+ return 0;
+Efault:
+ return -EFAULT;
+}

#endif /* _ASM_POWERPC_INST_H */
diff --git a/arch/powerpc/mm/maccess.c b/arch/powerpc/mm/maccess.c
index 5abae96b2b46..ea821d0ffe16 100644
--- a/arch/powerpc/mm/maccess.c
+++ b/arch/powerpc/mm/maccess.c
@@ -11,20 +11,3 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
{
return is_kernel_addr((unsigned long)unsafe_src);
}
-
-int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
-{
- unsigned int val, suffix;
- int err;
-
- err = copy_from_kernel_nofault(&val, src, sizeof(val));
- if (err)
- return err;
- if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
- err = copy_from_kernel_nofault(&suffix, src + 1, sizeof(suffix));
- *inst = ppc_inst_prefix(val, suffix);
- } else {
- *inst = ppc_inst(val);
- }
- return err;
-}
--
2.33.1

2021-11-29 17:56:09

by Christophe Leroy

Subject: [PATCH v5 3/5] powerpc/inst: Define ppc_inst_t as u32 on PPC32

Unlike PPC64 ABI, PPC32 uses the stack to pass a parameter defined
as a struct, even when the struct has a single simple element.

To avoid that, define ppc_inst_t as u32 on PPC32.

Keep it as 'struct ppc_inst' when __CHECKER__ is defined so that
sparse can perform type checking.

Also revert commit 511eea5e2ccd ("powerpc/kprobes: Fix Oops by passing
ppc_inst as a pointer to emulate_step() on ppc32") as now the
instruction to be emulated is passed as a register to emulate_step().

Signed-off-by: Christophe Leroy <[email protected]>
---
v2: Make it work with kprobes
---
arch/powerpc/include/asm/inst.h | 15 +++++++++++++--
arch/powerpc/kernel/optprobes.c | 8 ++------
2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index b3502f21e0f4..7ef5fd3bb167 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -34,6 +34,7 @@
* Instruction data type for POWER
*/

+#if defined(CONFIG_PPC64) || defined(__CHECKER__)
typedef struct {
u32 val;
#ifdef CONFIG_PPC64
@@ -46,13 +47,23 @@ static inline u32 ppc_inst_val(ppc_inst_t x)
return x.val;
}

+#define ppc_inst(x) ((ppc_inst_t){ .val = (x) })
+
+#else
+typedef u32 ppc_inst_t;
+
+static inline u32 ppc_inst_val(ppc_inst_t x)
+{
+ return x;
+}
+#define ppc_inst(x) (x)
+#endif
+
static inline int ppc_inst_primary_opcode(ppc_inst_t x)
{
return ppc_inst_val(x) >> 26;
}

-#define ppc_inst(x) ((ppc_inst_t){ .val = (x) })
-
#ifdef CONFIG_PPC64
#define ppc_inst_prefix(x, y) ((ppc_inst_t){ .val = (x), .suffix = (y) })

diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 378db980ded3..3b1c2236cbee 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -228,12 +228,8 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
/*
* 3. load instruction to be emulated into relevant register, and
*/
- if (IS_ENABLED(CONFIG_PPC64)) {
- temp = ppc_inst_read(p->ainsn.insn);
- patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);
- } else {
- patch_imm_load_insns((unsigned long)p->ainsn.insn, 4, buff + TMPL_INSN_IDX);
- }
+ temp = ppc_inst_read(p->ainsn.insn);
+ patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX);

/*
* 4. branch back from trampoline
--
2.33.1

2021-11-29 17:58:09

by Christophe Leroy

Subject: [PATCH v5 4/5] powerpc/inst: Move ppc_inst_t definition in asm/reg.h

Because of circular inclusion of asm/hw_breakpoint.h, we
need to move definition of asm/reg.h outside of inst.h
so that asm/hw_breakpoint.h gets it without including
asm/inst.h

Also remove asm/inst.h from asm/uprobes.h as it's not
needed anymore.

Signed-off-by: Christophe Leroy <[email protected]>
---
v4: New to support inlining of copy_inst_from_kernel_nofault() in following patch.
---
arch/powerpc/include/asm/hw_breakpoint.h | 1 -
arch/powerpc/include/asm/inst.h | 10 +---------
arch/powerpc/include/asm/reg.h | 12 ++++++++++++
arch/powerpc/include/asm/uprobes.h | 1 -
4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index 88053d3c68e6..84d39fd42f71 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -10,7 +10,6 @@
#define _PPC_BOOK3S_64_HW_BREAKPOINT_H

#include <asm/cpu_has_feature.h>
-#include <asm/inst.h>

#ifdef __KERNEL__
struct arch_hw_breakpoint {
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 7ef5fd3bb167..53a40faf362a 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -3,6 +3,7 @@
#define _ASM_POWERPC_INST_H

#include <asm/ppc-opcode.h>
+#include <asm/reg.h>

#define ___get_user_instr(gu_op, dest, ptr) \
({ \
@@ -35,13 +36,6 @@
*/

#if defined(CONFIG_PPC64) || defined(__CHECKER__)
-typedef struct {
- u32 val;
-#ifdef CONFIG_PPC64
- u32 suffix;
-#endif
-} __packed ppc_inst_t;
-
static inline u32 ppc_inst_val(ppc_inst_t x)
{
return x.val;
@@ -50,8 +44,6 @@ static inline u32 ppc_inst_val(ppc_inst_t x)
#define ppc_inst(x) ((ppc_inst_t){ .val = (x) })

#else
-typedef u32 ppc_inst_t;
-
static inline u32 ppc_inst_val(ppc_inst_t x)
{
return x;
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index e9d27265253b..85501181f929 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1366,6 +1366,18 @@

/* Macros for setting and retrieving special purpose registers */
#ifndef __ASSEMBLY__
+
+#if defined(CONFIG_PPC64) || defined(__CHECKER__)
+typedef struct {
+ u32 val;
+#ifdef CONFIG_PPC64
+ u32 suffix;
+#endif
+} __packed ppc_inst_t;
+#else
+typedef u32 ppc_inst_t;
+#endif
+
#define mfmsr() ({unsigned long rval; \
asm volatile("mfmsr %0" : "=r" (rval) : \
: "memory"); rval;})
diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index fe683371336f..a7ae1860115a 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h
@@ -11,7 +11,6 @@

#include <linux/notifier.h>
#include <asm/probes.h>
-#include <asm/inst.h>

typedef ppc_opcode_t uprobe_opcode_t;

--
2.33.1

2021-11-29 18:00:12

by Christophe Leroy

Subject: [PATCH v5 2/5] powerpc/inst: Define ppc_inst_t

In order to stop using 'struct ppc_inst' on PPC32,
define a ppc_inst_t typedef.

Signed-off-by: Christophe Leroy <[email protected]>
---
v3: Rebased and resolved conflicts

v2: Anonymise the structure so that only the typedef can be used
---
arch/powerpc/include/asm/code-patching.h | 18 +++----
arch/powerpc/include/asm/hw_breakpoint.h | 4 +-
arch/powerpc/include/asm/inst.h | 36 ++++++-------
arch/powerpc/include/asm/sstep.h | 4 +-
arch/powerpc/kernel/align.c | 4 +-
arch/powerpc/kernel/epapr_paravirt.c | 2 +-
arch/powerpc/kernel/hw_breakpoint.c | 4 +-
.../kernel/hw_breakpoint_constraints.c | 4 +-
arch/powerpc/kernel/kprobes.c | 4 +-
arch/powerpc/kernel/mce_power.c | 2 +-
arch/powerpc/kernel/optprobes.c | 4 +-
arch/powerpc/kernel/process.c | 2 +-
arch/powerpc/kernel/setup_32.c | 2 +-
arch/powerpc/kernel/trace/ftrace.c | 54 +++++++++----------
arch/powerpc/kernel/vecemu.c | 2 +-
arch/powerpc/lib/code-patching.c | 38 ++++++-------
arch/powerpc/lib/feature-fixups.c | 4 +-
arch/powerpc/lib/sstep.c | 4 +-
arch/powerpc/lib/test_emulate_step.c | 10 ++--
arch/powerpc/mm/maccess.c | 2 +-
arch/powerpc/perf/8xx-pmu.c | 2 +-
arch/powerpc/xmon/xmon.c | 14 ++---
arch/powerpc/xmon/xmon_bpts.h | 4 +-
23 files changed, 112 insertions(+), 112 deletions(-)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index 4ba834599c4d..46e8c5a8ce51 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -24,20 +24,20 @@

bool is_offset_in_branch_range(long offset);
bool is_offset_in_cond_branch_range(long offset);
-int create_branch(struct ppc_inst *instr, const u32 *addr,
+int create_branch(ppc_inst_t *instr, const u32 *addr,
unsigned long target, int flags);
-int create_cond_branch(struct ppc_inst *instr, const u32 *addr,
+int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
unsigned long target, int flags);
int patch_branch(u32 *addr, unsigned long target, int flags);
-int patch_instruction(u32 *addr, struct ppc_inst instr);
-int raw_patch_instruction(u32 *addr, struct ppc_inst instr);
+int patch_instruction(u32 *addr, ppc_inst_t instr);
+int raw_patch_instruction(u32 *addr, ppc_inst_t instr);

static inline unsigned long patch_site_addr(s32 *site)
{
return (unsigned long)site + *site;
}

-static inline int patch_instruction_site(s32 *site, struct ppc_inst instr)
+static inline int patch_instruction_site(s32 *site, ppc_inst_t instr)
{
return patch_instruction((u32 *)patch_site_addr(site), instr);
}
@@ -58,11 +58,11 @@ static inline int modify_instruction_site(s32 *site, unsigned int clr, unsigned
return modify_instruction((unsigned int *)patch_site_addr(site), clr, set);
}

-int instr_is_relative_branch(struct ppc_inst instr);
-int instr_is_relative_link_branch(struct ppc_inst instr);
+int instr_is_relative_branch(ppc_inst_t instr);
+int instr_is_relative_link_branch(ppc_inst_t instr);
unsigned long branch_target(const u32 *instr);
-int translate_branch(struct ppc_inst *instr, const u32 *dest, const u32 *src);
-extern bool is_conditional_branch(struct ppc_inst instr);
+int translate_branch(ppc_inst_t *instr, const u32 *dest, const u32 *src);
+bool is_conditional_branch(ppc_inst_t instr);
#ifdef CONFIG_PPC_BOOK3E_64
void __patch_exception(int exc, unsigned long addr);
#define patch_exception(exc, name) do { \
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index abebfbee5b1c..88053d3c68e6 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -56,11 +56,11 @@ 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,
+bool wp_check_constraints(struct pt_regs *regs, ppc_inst_t 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,
+void wp_get_instr_detail(struct pt_regs *regs, ppc_inst_t *instr,
int *type, int *size, unsigned long *ea);

#ifdef CONFIG_HAVE_HW_BREAKPOINT
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 10a5c1b76ca0..b3502f21e0f4 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -8,7 +8,7 @@
({ \
long __gui_ret; \
u32 __user *__gui_ptr = (u32 __user *)ptr; \
- struct ppc_inst __gui_inst; \
+ ppc_inst_t __gui_inst; \
unsigned int __prefix, __suffix; \
\
__chk_user_ptr(ptr); \
@@ -34,29 +34,29 @@
* Instruction data type for POWER
*/

-struct ppc_inst {
+typedef struct {
u32 val;
#ifdef CONFIG_PPC64
u32 suffix;
#endif
-} __packed;
+} __packed ppc_inst_t;

-static inline u32 ppc_inst_val(struct ppc_inst x)
+static inline u32 ppc_inst_val(ppc_inst_t x)
{
return x.val;
}

-static inline int ppc_inst_primary_opcode(struct ppc_inst x)
+static inline int ppc_inst_primary_opcode(ppc_inst_t x)
{
return ppc_inst_val(x) >> 26;
}

-#define ppc_inst(x) ((struct ppc_inst){ .val = (x) })
+#define ppc_inst(x) ((ppc_inst_t){ .val = (x) })

#ifdef CONFIG_PPC64
-#define ppc_inst_prefix(x, y) ((struct ppc_inst){ .val = (x), .suffix = (y) })
+#define ppc_inst_prefix(x, y) ((ppc_inst_t){ .val = (x), .suffix = (y) })

-static inline u32 ppc_inst_suffix(struct ppc_inst x)
+static inline u32 ppc_inst_suffix(ppc_inst_t x)
{
return x.suffix;
}
@@ -64,14 +64,14 @@ static inline u32 ppc_inst_suffix(struct ppc_inst x)
#else
#define ppc_inst_prefix(x, y) ((void)y, ppc_inst(x))

-static inline u32 ppc_inst_suffix(struct ppc_inst x)
+static inline u32 ppc_inst_suffix(ppc_inst_t x)
{
return 0;
}

#endif /* CONFIG_PPC64 */

-static inline struct ppc_inst ppc_inst_read(const u32 *ptr)
+static inline ppc_inst_t ppc_inst_read(const u32 *ptr)
{
if (IS_ENABLED(CONFIG_PPC64) && (*ptr >> 26) == OP_PREFIX)
return ppc_inst_prefix(*ptr, *(ptr + 1));
@@ -79,17 +79,17 @@ static inline struct ppc_inst ppc_inst_read(const u32 *ptr)
return ppc_inst(*ptr);
}

-static inline bool ppc_inst_prefixed(struct ppc_inst x)
+static inline bool ppc_inst_prefixed(ppc_inst_t x)
{
return IS_ENABLED(CONFIG_PPC64) && ppc_inst_primary_opcode(x) == OP_PREFIX;
}

-static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x)
+static inline ppc_inst_t ppc_inst_swab(ppc_inst_t x)
{
return ppc_inst_prefix(swab32(ppc_inst_val(x)), swab32(ppc_inst_suffix(x)));
}

-static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
+static inline bool ppc_inst_equal(ppc_inst_t x, ppc_inst_t y)
{
if (ppc_inst_val(x) != ppc_inst_val(y))
return false;
@@ -98,7 +98,7 @@ static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
return ppc_inst_suffix(x) == ppc_inst_suffix(y);
}

-static inline int ppc_inst_len(struct ppc_inst x)
+static inline int ppc_inst_len(ppc_inst_t x)
{
return ppc_inst_prefixed(x) ? 8 : 4;
}
@@ -109,14 +109,14 @@ static inline int ppc_inst_len(struct ppc_inst x)
*/
static inline u32 *ppc_inst_next(u32 *location, u32 *value)
{
- struct ppc_inst tmp;
+ ppc_inst_t tmp;

tmp = ppc_inst_read(value);

return (void *)location + ppc_inst_len(tmp);
}

-static inline unsigned long ppc_inst_as_ulong(struct ppc_inst x)
+static inline unsigned long ppc_inst_as_ulong(ppc_inst_t x)
{
if (IS_ENABLED(CONFIG_PPC32))
return ppc_inst_val(x);
@@ -128,7 +128,7 @@ static inline unsigned long ppc_inst_as_ulong(struct ppc_inst x)

#define PPC_INST_STR_LEN sizeof("00000000 00000000")

-static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], struct ppc_inst x)
+static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], ppc_inst_t x)
{
if (ppc_inst_prefixed(x))
sprintf(str, "%08x %08x", ppc_inst_val(x), ppc_inst_suffix(x));
@@ -145,6 +145,6 @@ static inline char *__ppc_inst_as_str(char str[PPC_INST_STR_LEN], struct ppc_ins
__str; \
})

-int copy_inst_from_kernel_nofault(struct ppc_inst *inst, u32 *src);
+int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src);

#endif /* _ASM_POWERPC_INST_H */
diff --git a/arch/powerpc/include/asm/sstep.h b/arch/powerpc/include/asm/sstep.h
index 1df867c2e054..50950deedb87 100644
--- a/arch/powerpc/include/asm/sstep.h
+++ b/arch/powerpc/include/asm/sstep.h
@@ -145,7 +145,7 @@ union vsx_reg {
* otherwise.
*/
extern int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
- struct ppc_inst instr);
+ ppc_inst_t instr);

/*
* Emulate an instruction that can be executed just by updating
@@ -162,7 +162,7 @@ void emulate_update_regs(struct pt_regs *reg, struct instruction_op *op);
* 0 if it could not be emulated, or -1 for an instruction that
* should not be emulated (rfid, mtmsrd clearing MSR_RI, etc.).
*/
-extern int emulate_step(struct pt_regs *regs, struct ppc_inst instr);
+int emulate_step(struct pt_regs *regs, ppc_inst_t instr);

/*
* Emulate a load or store instruction by reading/writing the
diff --git a/arch/powerpc/kernel/align.c b/arch/powerpc/kernel/align.c
index bf96b954a4eb..3e37ece06739 100644
--- a/arch/powerpc/kernel/align.c
+++ b/arch/powerpc/kernel/align.c
@@ -105,7 +105,7 @@ static struct aligninfo spe_aligninfo[32] = {
* so we don't need the address swizzling.
*/
static int emulate_spe(struct pt_regs *regs, unsigned int reg,
- struct ppc_inst ppc_instr)
+ ppc_inst_t ppc_instr)
{
union {
u64 ll;
@@ -300,7 +300,7 @@ static int emulate_spe(struct pt_regs *regs, unsigned int reg,

int fix_alignment(struct pt_regs *regs)
{
- struct ppc_inst instr;
+ ppc_inst_t instr;
struct instruction_op op;
int r, type;

diff --git a/arch/powerpc/kernel/epapr_paravirt.c b/arch/powerpc/kernel/epapr_paravirt.c
index 93b0f3ec8fb0..d4b8aff20815 100644
--- a/arch/powerpc/kernel/epapr_paravirt.c
+++ b/arch/powerpc/kernel/epapr_paravirt.c
@@ -37,7 +37,7 @@ static int __init early_init_dt_scan_epapr(unsigned long node,
return -1;

for (i = 0; i < (len / 4); i++) {
- struct ppc_inst inst = ppc_inst(be32_to_cpu(insts[i]));
+ ppc_inst_t inst = ppc_inst(be32_to_cpu(insts[i]));
patch_instruction(epapr_hypercall_start + i, inst);
#if !defined(CONFIG_64BIT) || defined(CONFIG_PPC_BOOK3E_64)
patch_instruction(epapr_ev_idle_start + i, inst);
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 91a3be14808b..2669f80b3a49 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -523,7 +523,7 @@ static void larx_stcx_err(struct perf_event *bp, struct arch_hw_breakpoint *info

static bool stepping_handler(struct pt_regs *regs, struct perf_event **bp,
struct arch_hw_breakpoint **info, int *hit,
- struct ppc_inst instr)
+ ppc_inst_t instr)
{
int i;
int stepped;
@@ -616,7 +616,7 @@ int hw_breakpoint_handler(struct die_args *args)
int hit[HBP_NUM_MAX] = {0};
int nr_hit = 0;
bool ptrace_bp = false;
- struct ppc_inst instr = ppc_inst(0);
+ ppc_inst_t instr = ppc_inst(0);
int type = 0;
int size = 0;
unsigned long ea;
diff --git a/arch/powerpc/kernel/hw_breakpoint_constraints.c b/arch/powerpc/kernel/hw_breakpoint_constraints.c
index 42b967e3d85c..a74623025f3a 100644
--- a/arch/powerpc/kernel/hw_breakpoint_constraints.c
+++ b/arch/powerpc/kernel/hw_breakpoint_constraints.c
@@ -80,7 +80,7 @@ static bool check_dawrx_constraints(struct pt_regs *regs, int type,
* 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,
+bool wp_check_constraints(struct pt_regs *regs, ppc_inst_t instr,
unsigned long ea, int type, int size,
struct arch_hw_breakpoint *info)
{
@@ -127,7 +127,7 @@ bool wp_check_constraints(struct pt_regs *regs, struct ppc_inst instr,
return false;
}

-void wp_get_instr_detail(struct pt_regs *regs, struct ppc_inst *instr,
+void wp_get_instr_detail(struct pt_regs *regs, ppc_inst_t *instr,
int *type, int *size, unsigned long *ea)
{
struct instruction_op op;
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 86d77ff056a6..9a492fdec1df 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -124,7 +124,7 @@ int arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
struct kprobe *prev;
- struct ppc_inst insn = ppc_inst_read(p->addr);
+ ppc_inst_t insn = ppc_inst_read(p->addr);

if ((unsigned long)p->addr & 0x03) {
printk("Attempt to register kprobe at an unaligned address\n");
@@ -244,7 +244,7 @@ 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(p->ainsn.insn);
+ ppc_inst_t insn = ppc_inst_read(p->ainsn.insn);

/* regs->nip is also adjusted if emulate_step returns 1 */
ret = emulate_step(regs, insn);
diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
index c2f55fe7092d..f2dd4daeddf8 100644
--- a/arch/powerpc/kernel/mce_power.c
+++ b/arch/powerpc/kernel/mce_power.c
@@ -455,7 +455,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr,
* in real-mode is tricky and can lead to recursive
* faults
*/
- struct ppc_inst instr;
+ ppc_inst_t instr;
unsigned long pfn, instr_addr;
struct instruction_op op;
struct pt_regs tmp = *regs;
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index ce1903064031..378db980ded3 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -153,7 +153,7 @@ static void patch_imm_load_insns(unsigned long val, int reg, kprobe_opcode_t *ad

int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
{
- struct ppc_inst branch_op_callback, branch_emulate_step, temp;
+ ppc_inst_t branch_op_callback, branch_emulate_step, temp;
unsigned long op_callback_addr, emulate_step_addr;
kprobe_opcode_t *buff;
long b_offset;
@@ -269,7 +269,7 @@ int arch_check_optimized_kprobe(struct optimized_kprobe *op)

void arch_optimize_kprobes(struct list_head *oplist)
{
- struct ppc_inst instr;
+ ppc_inst_t instr;
struct optimized_kprobe *op;
struct optimized_kprobe *tmp;

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 5d2333d2a283..c60abfc397a5 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -628,7 +628,7 @@ 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);
+ ppc_inst_t instr = ppc_inst(0);
int type = 0;
int size = 0;
unsigned long ea;
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index 7ec5c47fce0e..4017b05ef643 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -75,7 +75,7 @@ EXPORT_SYMBOL(DMA_MODE_WRITE);
notrace void __init machine_init(u64 dt_ptr)
{
u32 *addr = (u32 *)patch_site_addr(&patch__memset_nocache);
- struct ppc_inst insn;
+ ppc_inst_t insn;

/* Configure static keys first, now that we're relocated. */
setup_feature_keys();
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index d89c5df4f206..f293294ef5da 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -41,10 +41,10 @@
#define NUM_FTRACE_TRAMPS 8
static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];

-static struct ppc_inst
+static ppc_inst_t
ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
{
- struct ppc_inst op;
+ ppc_inst_t op;

addr = ppc_function_entry((void *)addr);

@@ -55,9 +55,9 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
}

static int
-ftrace_modify_code(unsigned long ip, struct ppc_inst old, struct ppc_inst new)
+ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
{
- struct ppc_inst replaced;
+ ppc_inst_t replaced;

/*
* Note:
@@ -90,24 +90,24 @@ ftrace_modify_code(unsigned long ip, struct ppc_inst old, struct ppc_inst new)
*/
static int test_24bit_addr(unsigned long ip, unsigned long addr)
{
- struct ppc_inst op;
+ ppc_inst_t op;
addr = ppc_function_entry((void *)addr);

/* use the create_branch to verify that this offset can be branched */
return create_branch(&op, (u32 *)ip, addr, 0) == 0;
}

-static int is_bl_op(struct ppc_inst op)
+static int is_bl_op(ppc_inst_t op)
{
return (ppc_inst_val(op) & 0xfc000003) == 0x48000001;
}

-static int is_b_op(struct ppc_inst op)
+static int is_b_op(ppc_inst_t op)
{
return (ppc_inst_val(op) & 0xfc000003) == 0x48000000;
}

-static unsigned long find_bl_target(unsigned long ip, struct ppc_inst op)
+static unsigned long find_bl_target(unsigned long ip, ppc_inst_t op)
{
int offset;

@@ -127,7 +127,7 @@ __ftrace_make_nop(struct module *mod,
{
unsigned long entry, ptr, tramp;
unsigned long ip = rec->ip;
- struct ppc_inst op, pop;
+ ppc_inst_t op, pop;

/* read where this goes */
if (copy_inst_from_kernel_nofault(&op, (void *)ip)) {
@@ -221,7 +221,7 @@ static int
__ftrace_make_nop(struct module *mod,
struct dyn_ftrace *rec, unsigned long addr)
{
- struct ppc_inst op;
+ ppc_inst_t op;
unsigned int jmp[4];
unsigned long ip = rec->ip;
unsigned long tramp;
@@ -291,7 +291,7 @@ __ftrace_make_nop(struct module *mod,
static unsigned long find_ftrace_tramp(unsigned long ip)
{
int i;
- struct ppc_inst instr;
+ ppc_inst_t instr;

/*
* We have the compiler generated long_branch tramps at the end
@@ -329,9 +329,9 @@ static int add_ftrace_tramp(unsigned long tramp)
static int setup_mcount_compiler_tramp(unsigned long tramp)
{
int i;
- struct ppc_inst op;
+ ppc_inst_t op;
unsigned long ptr;
- struct ppc_inst instr;
+ ppc_inst_t instr;
static unsigned long ftrace_plt_tramps[NUM_FTRACE_TRAMPS];

/* Is this a known long jump tramp? */
@@ -396,7 +396,7 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
{
unsigned long tramp, ip = rec->ip;
- struct ppc_inst op;
+ ppc_inst_t op;

/* Read where this goes */
if (copy_inst_from_kernel_nofault(&op, (void *)ip)) {
@@ -436,7 +436,7 @@ int ftrace_make_nop(struct module *mod,
struct dyn_ftrace *rec, unsigned long addr)
{
unsigned long ip = rec->ip;
- struct ppc_inst old, new;
+ ppc_inst_t old, new;

/*
* If the calling address is more that 24 bits away,
@@ -489,7 +489,7 @@ int ftrace_make_nop(struct module *mod,
*/
#ifndef CONFIG_MPROFILE_KERNEL
static int
-expected_nop_sequence(void *ip, struct ppc_inst op0, struct ppc_inst op1)
+expected_nop_sequence(void *ip, ppc_inst_t op0, ppc_inst_t op1)
{
/*
* We expect to see:
@@ -507,7 +507,7 @@ expected_nop_sequence(void *ip, struct ppc_inst op0, struct ppc_inst op1)
}
#else
static int
-expected_nop_sequence(void *ip, struct ppc_inst op0, struct ppc_inst op1)
+expected_nop_sequence(void *ip, ppc_inst_t op0, ppc_inst_t op1)
{
/* look for patched "NOP" on ppc64 with -mprofile-kernel */
if (!ppc_inst_equal(op0, ppc_inst(PPC_RAW_NOP())))
@@ -519,8 +519,8 @@ expected_nop_sequence(void *ip, struct ppc_inst op0, struct ppc_inst op1)
static int
__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
- struct ppc_inst op[2];
- struct ppc_inst instr;
+ ppc_inst_t op[2];
+ ppc_inst_t instr;
void *ip = (void *)rec->ip;
unsigned long entry, ptr, tramp;
struct module *mod = rec->arch.mod;
@@ -588,7 +588,7 @@ static int
__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
int err;
- struct ppc_inst op;
+ ppc_inst_t op;
u32 *ip = (u32 *)rec->ip;

/* read where this goes */
@@ -626,7 +626,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)

static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
{
- struct ppc_inst op;
+ ppc_inst_t op;
void *ip = (void *)rec->ip;
unsigned long tramp, entry, ptr;

@@ -674,7 +674,7 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
unsigned long ip = rec->ip;
- struct ppc_inst old, new;
+ ppc_inst_t old, new;

/*
* If the calling address is more that 24 bits away,
@@ -713,7 +713,7 @@ static int
__ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
unsigned long addr)
{
- struct ppc_inst op;
+ ppc_inst_t op;
unsigned long ip = rec->ip;
unsigned long entry, ptr, tramp;
struct module *mod = rec->arch.mod;
@@ -807,7 +807,7 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
unsigned long addr)
{
unsigned long ip = rec->ip;
- struct ppc_inst old, new;
+ ppc_inst_t old, new;

/*
* If the calling address is more that 24 bits away,
@@ -847,7 +847,7 @@ int ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
int ftrace_update_ftrace_func(ftrace_func_t func)
{
unsigned long ip = (unsigned long)(&ftrace_call);
- struct ppc_inst old, new;
+ ppc_inst_t old, new;
int ret;

old = ppc_inst_read((u32 *)&ftrace_call);
@@ -932,7 +932,7 @@ int ftrace_enable_ftrace_graph_caller(void)
unsigned long ip = (unsigned long)(&ftrace_graph_call);
unsigned long addr = (unsigned long)(&ftrace_graph_caller);
unsigned long stub = (unsigned long)(&ftrace_graph_stub);
- struct ppc_inst old, new;
+ ppc_inst_t old, new;

old = ftrace_call_replace(ip, stub, 0);
new = ftrace_call_replace(ip, addr, 0);
@@ -945,7 +945,7 @@ int ftrace_disable_ftrace_graph_caller(void)
unsigned long ip = (unsigned long)(&ftrace_graph_call);
unsigned long addr = (unsigned long)(&ftrace_graph_caller);
unsigned long stub = (unsigned long)(&ftrace_graph_stub);
- struct ppc_inst old, new;
+ ppc_inst_t old, new;

old = ftrace_call_replace(ip, addr, 0);
new = ftrace_call_replace(ip, stub, 0);
diff --git a/arch/powerpc/kernel/vecemu.c b/arch/powerpc/kernel/vecemu.c
index ae632569446f..fd9432875ebc 100644
--- a/arch/powerpc/kernel/vecemu.c
+++ b/arch/powerpc/kernel/vecemu.c
@@ -261,7 +261,7 @@ static unsigned int rfin(unsigned int x)

int emulate_altivec(struct pt_regs *regs)
{
- struct ppc_inst instr;
+ ppc_inst_t instr;
unsigned int i, word;
unsigned int va, vb, vc, vd;
vector128 *vrs;
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index c5ed98823835..312324a26df3 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -18,7 +18,7 @@
#include <asm/setup.h>
#include <asm/inst.h>

-static int __patch_instruction(u32 *exec_addr, struct ppc_inst instr, u32 *patch_addr)
+static int __patch_instruction(u32 *exec_addr, ppc_inst_t instr, u32 *patch_addr)
{
if (!ppc_inst_prefixed(instr)) {
u32 val = ppc_inst_val(instr);
@@ -39,7 +39,7 @@ static int __patch_instruction(u32 *exec_addr, struct ppc_inst instr, u32 *patch
return -EFAULT;
}

-int raw_patch_instruction(u32 *addr, struct ppc_inst instr)
+int raw_patch_instruction(u32 *addr, ppc_inst_t instr)
{
return __patch_instruction(addr, instr, addr);
}
@@ -141,7 +141,7 @@ static inline int unmap_patch_area(unsigned long addr)
return 0;
}

-static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
+static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
{
int err;
u32 *patch_addr = NULL;
@@ -180,14 +180,14 @@ static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
}
#else /* !CONFIG_STRICT_KERNEL_RWX */

-static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
+static int do_patch_instruction(u32 *addr, ppc_inst_t instr)
{
return raw_patch_instruction(addr, instr);
}

#endif /* CONFIG_STRICT_KERNEL_RWX */

-int patch_instruction(u32 *addr, struct ppc_inst instr)
+int patch_instruction(u32 *addr, ppc_inst_t instr)
{
/* Make sure we aren't patching a freed init section */
if (init_mem_is_free && init_section_contains(addr, 4)) {
@@ -200,7 +200,7 @@ NOKPROBE_SYMBOL(patch_instruction);

int patch_branch(u32 *addr, unsigned long target, int flags)
{
- struct ppc_inst instr;
+ ppc_inst_t instr;

create_branch(&instr, addr, target, flags);
return patch_instruction(addr, instr);
@@ -237,7 +237,7 @@ bool is_offset_in_cond_branch_range(long offset)
* Helper to check if a given instruction is a conditional branch
* Derived from the conditional checks in analyse_instr()
*/
-bool is_conditional_branch(struct ppc_inst instr)
+bool is_conditional_branch(ppc_inst_t instr)
{
unsigned int opcode = ppc_inst_primary_opcode(instr);

@@ -255,7 +255,7 @@ bool is_conditional_branch(struct ppc_inst instr)
}
NOKPROBE_SYMBOL(is_conditional_branch);

-int create_branch(struct ppc_inst *instr, const u32 *addr,
+int create_branch(ppc_inst_t *instr, const u32 *addr,
unsigned long target, int flags)
{
long offset;
@@ -275,7 +275,7 @@ int create_branch(struct ppc_inst *instr, const u32 *addr,
return 0;
}

-int create_cond_branch(struct ppc_inst *instr, const u32 *addr,
+int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
unsigned long target, int flags)
{
long offset;
@@ -294,22 +294,22 @@ int create_cond_branch(struct ppc_inst *instr, const u32 *addr,
return 0;
}

-static unsigned int branch_opcode(struct ppc_inst instr)
+static unsigned int branch_opcode(ppc_inst_t instr)
{
return ppc_inst_primary_opcode(instr) & 0x3F;
}

-static int instr_is_branch_iform(struct ppc_inst instr)
+static int instr_is_branch_iform(ppc_inst_t instr)
{
return branch_opcode(instr) == 18;
}

-static int instr_is_branch_bform(struct ppc_inst instr)
+static int instr_is_branch_bform(ppc_inst_t instr)
{
return branch_opcode(instr) == 16;
}

-int instr_is_relative_branch(struct ppc_inst instr)
+int instr_is_relative_branch(ppc_inst_t instr)
{
if (ppc_inst_val(instr) & BRANCH_ABSOLUTE)
return 0;
@@ -317,7 +317,7 @@ int instr_is_relative_branch(struct ppc_inst instr)
return instr_is_branch_iform(instr) || instr_is_branch_bform(instr);
}

-int instr_is_relative_link_branch(struct ppc_inst instr)
+int instr_is_relative_link_branch(ppc_inst_t instr)
{
return instr_is_relative_branch(instr) && (ppc_inst_val(instr) & BRANCH_SET_LINK);
}
@@ -364,7 +364,7 @@ unsigned long branch_target(const u32 *instr)
return 0;
}

-int translate_branch(struct ppc_inst *instr, const u32 *dest, const u32 *src)
+int translate_branch(ppc_inst_t *instr, const u32 *dest, const u32 *src)
{
unsigned long target;
target = branch_target(src);
@@ -417,7 +417,7 @@ static void __init test_trampoline(void)
static void __init test_branch_iform(void)
{
int err;
- struct ppc_inst instr;
+ ppc_inst_t instr;
u32 tmp[2];
u32 *iptr = tmp;
unsigned long addr = (unsigned long)tmp;
@@ -499,7 +499,7 @@ static void __init test_create_function_call(void)
{
u32 *iptr;
unsigned long dest;
- struct ppc_inst instr;
+ ppc_inst_t instr;

/* Check we can create a function call */
iptr = (u32 *)ppc_function_entry(test_trampoline);
@@ -513,7 +513,7 @@ static void __init test_branch_bform(void)
{
int err;
unsigned long addr;
- struct ppc_inst instr;
+ ppc_inst_t instr;
u32 tmp[2];
u32 *iptr = tmp;
unsigned int flags;
@@ -591,7 +591,7 @@ static void __init test_translate_branch(void)
{
unsigned long addr;
void *p, *q;
- struct ppc_inst instr;
+ ppc_inst_t instr;
void *buf;

buf = vmalloc(PAGE_ALIGN(0x2000000 + 1));
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index c3e06922468b..57c6bb802f6c 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -47,7 +47,7 @@ static u32 *calc_addr(struct fixup_entry *fcur, long offset)
static int patch_alt_instruction(u32 *src, u32 *dest, u32 *alt_start, u32 *alt_end)
{
int err;
- struct ppc_inst instr;
+ ppc_inst_t instr;

instr = ppc_inst_read(src);

@@ -624,7 +624,7 @@ void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end)
static void do_final_fixups(void)
{
#if defined(CONFIG_PPC64) && defined(CONFIG_RELOCATABLE)
- struct ppc_inst inst;
+ ppc_inst_t inst;
u32 *src, *dest, *end;

if (PHYSICAL_START == 0)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index 86f49e3e7cf5..a94b0cd0bdc5 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1354,7 +1354,7 @@ static nokprobe_inline int trap_compare(long v1, long v2)
* otherwise.
*/
int analyse_instr(struct instruction_op *op, const struct pt_regs *regs,
- struct ppc_inst instr)
+ ppc_inst_t instr)
{
#ifdef CONFIG_PPC64
unsigned int suffixopcode, prefixtype, prefix_r;
@@ -3578,7 +3578,7 @@ NOKPROBE_SYMBOL(emulate_loadstore);
* or -1 if the instruction is one that should not be stepped,
* such as an rfid, or a mtmsrd that would clear MSR_RI.
*/
-int emulate_step(struct pt_regs *regs, struct ppc_inst instr)
+int emulate_step(struct pt_regs *regs, ppc_inst_t instr)
{
struct instruction_op op;
int r, err, type;
diff --git a/arch/powerpc/lib/test_emulate_step.c b/arch/powerpc/lib/test_emulate_step.c
index 8b4f6b3e96c4..4f141daafcff 100644
--- a/arch/powerpc/lib/test_emulate_step.c
+++ b/arch/powerpc/lib/test_emulate_step.c
@@ -792,7 +792,7 @@ static void __init test_lxvpx_stxvpx(void)
#ifdef CONFIG_VSX
static void __init test_plxvp_pstxvp(void)
{
- struct ppc_inst instr;
+ ppc_inst_t instr;
struct pt_regs regs;
union {
vector128 a;
@@ -906,7 +906,7 @@ struct compute_test {
struct {
char *descr;
unsigned long flags;
- struct ppc_inst instr;
+ ppc_inst_t instr;
struct pt_regs regs;
} subtests[MAX_SUBTESTS + 1];
};
@@ -1600,7 +1600,7 @@ static struct compute_test compute_tests[] = {
};

static int __init emulate_compute_instr(struct pt_regs *regs,
- struct ppc_inst instr,
+ ppc_inst_t instr,
bool negative)
{
int analysed;
@@ -1627,7 +1627,7 @@ static int __init emulate_compute_instr(struct pt_regs *regs,
}

static int __init execute_compute_instr(struct pt_regs *regs,
- struct ppc_inst instr)
+ ppc_inst_t instr)
{
extern int exec_instr(struct pt_regs *regs);

@@ -1658,7 +1658,7 @@ static void __init run_tests_compute(void)
struct compute_test *test;
struct pt_regs *regs, exp, got;
unsigned int i, j, k;
- struct ppc_inst instr;
+ ppc_inst_t instr;
bool ignore_gpr, ignore_xer, ignore_ccr, passed, rc, negative;

for (i = 0; i < ARRAY_SIZE(compute_tests); i++) {
diff --git a/arch/powerpc/mm/maccess.c b/arch/powerpc/mm/maccess.c
index aad7c47e0030..5abae96b2b46 100644
--- a/arch/powerpc/mm/maccess.c
+++ b/arch/powerpc/mm/maccess.c
@@ -12,7 +12,7 @@ bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size)
return is_kernel_addr((unsigned long)unsafe_src);
}

-int copy_inst_from_kernel_nofault(struct ppc_inst *inst, u32 *src)
+int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
{
unsigned int val, suffix;
int err;
diff --git a/arch/powerpc/perf/8xx-pmu.c b/arch/powerpc/perf/8xx-pmu.c
index f970d1510d3d..4738c4dbf567 100644
--- a/arch/powerpc/perf/8xx-pmu.c
+++ b/arch/powerpc/perf/8xx-pmu.c
@@ -153,7 +153,7 @@ static void mpc8xx_pmu_read(struct perf_event *event)

static void mpc8xx_pmu_del(struct perf_event *event, int flags)
{
- struct ppc_inst insn = ppc_inst(PPC_RAW_MFSPR(10, SPRN_SPRG_SCRATCH2));
+ ppc_inst_t insn = ppc_inst(PPC_RAW_MFSPR(10, SPRN_SPRG_SCRATCH2));

mpc8xx_pmu_read(event);

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 83100c6524cc..eac2957c6614 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -125,7 +125,7 @@ static unsigned bpinstr = 0x7fe00008; /* trap */
static int cmds(struct pt_regs *);
static int mread(unsigned long, void *, int);
static int mwrite(unsigned long, void *, int);
-static int mread_instr(unsigned long, struct ppc_inst *);
+static int mread_instr(unsigned long, ppc_inst_t *);
static int handle_fault(struct pt_regs *);
static void byterev(unsigned char *, int);
static void memex(void);
@@ -908,7 +908,7 @@ static struct bpt *new_breakpoint(unsigned long a)
static void insert_bpts(void)
{
int i;
- struct ppc_inst instr, instr2;
+ ppc_inst_t instr, instr2;
struct bpt *bp, *bp2;

bp = bpts;
@@ -988,7 +988,7 @@ static void remove_bpts(void)
{
int i;
struct bpt *bp;
- struct ppc_inst instr;
+ ppc_inst_t instr;

bp = bpts;
for (i = 0; i < NBPTS; ++i, ++bp) {
@@ -1204,7 +1204,7 @@ static int do_step(struct pt_regs *regs)
*/
static int do_step(struct pt_regs *regs)
{
- struct ppc_inst instr;
+ ppc_inst_t instr;
int stepped;

force_enable_xmon();
@@ -1459,7 +1459,7 @@ csum(void)
*/
static long check_bp_loc(unsigned long addr)
{
- struct ppc_inst instr;
+ ppc_inst_t instr;

addr &= ~3;
if (!is_kernel_addr(addr)) {
@@ -2306,7 +2306,7 @@ mwrite(unsigned long adrs, void *buf, int size)
}

static int
-mread_instr(unsigned long adrs, struct ppc_inst *instr)
+mread_instr(unsigned long adrs, ppc_inst_t *instr)
{
volatile int n;

@@ -3026,7 +3026,7 @@ generic_inst_dump(unsigned long adr, long count, int praddr,
{
int nr, dotted;
unsigned long first_adr;
- struct ppc_inst inst, last_inst = ppc_inst(0);
+ ppc_inst_t inst, last_inst = ppc_inst(0);

dotted = 0;
for (first_adr = adr; count > 0; --count, adr += ppc_inst_len(inst)) {
diff --git a/arch/powerpc/xmon/xmon_bpts.h b/arch/powerpc/xmon/xmon_bpts.h
index 57e6fb03de48..377068f52edb 100644
--- a/arch/powerpc/xmon/xmon_bpts.h
+++ b/arch/powerpc/xmon/xmon_bpts.h
@@ -5,8 +5,8 @@
#define NBPTS 256
#ifndef __ASSEMBLY__
#include <asm/inst.h>
-#define BPT_SIZE (sizeof(struct ppc_inst) * 2)
-#define BPT_WORDS (BPT_SIZE / sizeof(struct ppc_inst))
+#define BPT_SIZE (sizeof(ppc_inst_t) * 2)
+#define BPT_WORDS (BPT_SIZE / sizeof(ppc_inst_t))

extern unsigned int bpt_table[NBPTS * BPT_WORDS];
#endif /* __ASSEMBLY__ */
--
2.33.1

2021-11-29 22:57:53

by kernel test robot

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Hi Christophe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.16-rc3 next-20211129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/[email protected]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from arch/powerpc/kernel/asm-offsets.c:71:
In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
*inst = ppc_inst(val);
^~~
arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
#define ppc_inst(x) (x)
^
arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
unsigned int val, suffix;
^
= 0
1 warning generated.
<stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
#warning syscall futex_waitv not implemented
^
1 warning generated.
arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs'
.stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x:
^
arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs'
.stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x:
^
make[2]: *** [arch/powerpc/kernel/vdso32/Makefile:55: arch/powerpc/kernel/vdso32/gettimeofday.o] Error 1
make[2]: Target 'include/generated/vdso32-offsets.h' not remade because of errors.
make[1]: *** [arch/powerpc/Makefile:421: vdso_prepare] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:219: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.


vim +/val +165 arch/powerpc/include/asm/inst.h

152
153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
154 {
155 unsigned int val, suffix;
156
157 if (unlikely(!is_kernel_addr((unsigned long)src)))
158 return -ERANGE;
159
160 __get_kernel_nofault(&val, src, u32, Efault);
161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
162 __get_kernel_nofault(&suffix, src + 1, u32, Efault);
163 *inst = ppc_inst_prefix(val, suffix);
164 } else {
> 165 *inst = ppc_inst(val);
166 }
167 return 0;
168 Efault:
169 return -EFAULT;
170 }
171

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

2021-11-30 05:58:31

by Christophe Leroy

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Le 29/11/2021 à 23:55, kernel test robot a écrit :
> Hi Christophe,
>
> I love your patch! Perhaps something to improve:
>
> [auto build test WARNING on powerpc/next]
> [also build test WARNING on v5.16-rc3 next-20211129]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/[email protected]/config)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # install powerpc cross compiling tool for clang build
> # apt-get install binutils-powerpc-linux-gnu
> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> # save the config file to linux build tree
> mkdir build_dir
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <[email protected]>
>
> All warnings (new ones prefixed by >>):
>
> In file included from arch/powerpc/kernel/asm-offsets.c:71:
> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> *inst = ppc_inst(val);
> ^~~
> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
> #define ppc_inst(x) (x)
> ^
> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
> unsigned int val, suffix;
> ^
> = 0

I can't understand what's wrong here.

We have

__get_kernel_nofault(&val, src, u32, Efault);
if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
__get_kernel_nofault(&suffix, src + 1, u32, Efault);
*inst = ppc_inst_prefix(val, suffix);
} else {
*inst = ppc_inst(val);
}

With

#define __get_kernel_nofault(dst, src, type, err_label) \
__get_user_size_goto(*((type *)(dst)), \
(__force type __user *)(src), sizeof(type), err_label)


And

#define __get_user_size_goto(x, ptr, size, label) \
do { \
BUILD_BUG_ON(size > sizeof(x)); \
switch (size) { \
case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
default: x = 0; BUILD_BUG(); \
} \
} while (0)

And

#define __get_user_asm_goto(x, addr, label, op) \
asm_volatile_goto( \
"1: "op"%U1%X1 %0, %1 # get_user\n" \
EX_TABLE(1b, %l2) \
: "=r" (x) \
: "m<>" (*addr) \
: \
: label)


I see no possibility, no alternative path where val wouldn't be set. The
asm clearly has *addr as an output param so it is always set.

> 1 warning generated.
> <stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
> #warning syscall futex_waitv not implemented
> ^
> 1 warning generated.
> arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs'
> .stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x:
> ^
> arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs'
> .stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x:

How should we fix that ? Will clang support .stabs in the future ?


> ^
> make[2]: *** [arch/powerpc/kernel/vdso32/Makefile:55: arch/powerpc/kernel/vdso32/gettimeofday.o] Error 1
> make[2]: Target 'include/generated/vdso32-offsets.h' not remade because of errors.
> make[1]: *** [arch/powerpc/Makefile:421: vdso_prepare] Error 2
> make[1]: Target 'prepare' not remade because of errors.
> make: *** [Makefile:219: __sub-make] Error 2
> make: Target 'prepare' not remade because of errors.
>
>
> vim +/val +165 arch/powerpc/include/asm/inst.h
>
> 152
> 153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
> 154 {
> 155 unsigned int val, suffix;
> 156
> 157 if (unlikely(!is_kernel_addr((unsigned long)src)))
> 158 return -ERANGE;
> 159
> 160 __get_kernel_nofault(&val, src, u32, Efault);
> 161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> 162 __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> 163 *inst = ppc_inst_prefix(val, suffix);
> 164 } else {
> > 165 *inst = ppc_inst(val);
> 166 }
> 167 return 0;
> 168 Efault:
> 169 return -EFAULT;
> 170 }
> 171
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/[email protected]
>

2021-11-30 11:25:49

by Michael Ellerman

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Christophe Leroy <[email protected]> writes:
> Le 29/11/2021 à 23:55, kernel test robot a écrit :
>> Hi Christophe,
>>
>> I love your patch! Perhaps something to improve:
>>
>> [auto build test WARNING on powerpc/next]
>> [also build test WARNING on v5.16-rc3 next-20211129]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
>> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
>> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/[email protected]/config)
>> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
>> reproduce (this is a W=1 build):
>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> # install powerpc cross compiling tool for clang build
>> # apt-get install binutils-powerpc-linux-gnu
>> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
>> git remote add linux-review https://github.com/0day-ci/linux
>> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
>> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
>> # save the config file to linux build tree
>> mkdir build_dir
>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <[email protected]>
>>
>> All warnings (new ones prefixed by >>):
>>
>> In file included from arch/powerpc/kernel/asm-offsets.c:71:
>> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
>> *inst = ppc_inst(val);
>> ^~~
>> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
>> #define ppc_inst(x) (x)
>> ^
>> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
>> unsigned int val, suffix;
>> ^
>> = 0
>
> I can't understand what's wrong here.
>
> We have
>
> __get_kernel_nofault(&val, src, u32, Efault);
> if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> *inst = ppc_inst_prefix(val, suffix);
> } else {
> *inst = ppc_inst(val);
> }
>
> With
>
> #define __get_kernel_nofault(dst, src, type, err_label) \
> __get_user_size_goto(*((type *)(dst)), \
> (__force type __user *)(src), sizeof(type), err_label)
>
>
> And
>
> #define __get_user_size_goto(x, ptr, size, label) \
> do { \
> BUILD_BUG_ON(size > sizeof(x)); \
> switch (size) { \
> case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
> case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
> case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
> case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
> default: x = 0; BUILD_BUG(); \
> } \
> } while (0)
>
> And
>
> #define __get_user_asm_goto(x, addr, label, op) \
> asm_volatile_goto( \
> "1: "op"%U1%X1 %0, %1 # get_user\n" \
> EX_TABLE(1b, %l2) \
> : "=r" (x) \
> : "m<>" (*addr) \
> : \
> : label)
>
>
> I see no possibility, no alternative path where val wouldn't be set. The
> asm clearly has *addr as an output param so it is always set.

I guess clang can't convince itself of that?

>> 1 warning generated.
>> <stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
>> #warning syscall futex_waitv not implemented
>> ^
>> 1 warning generated.
>> arch/powerpc/kernel/vdso32/gettimeofday.S:72:8: error: unsupported directive '.stabs'
>> .stabs "_restgpr_31_x:F-1",36,0,0,_restgpr_31_x; .globl _restgpr_31_x; _restgpr_31_x:
>> ^
>> arch/powerpc/kernel/vdso32/gettimeofday.S:73:8: error: unsupported directive '.stabs'
>> .stabs "_rest32gpr_31_x:F-1",36,0,0,_rest32gpr_31_x; .globl _rest32gpr_31_x; _rest32gpr_31_x:
>
> How should we fix that ? Will clang support .stabs in the future ?

I think we should drop any stabs annotations / support. AFAICT none of
the toolchains we support produce it anymore.

cheers

2021-11-30 18:17:35

by Nathan Chancellor

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
> Christophe Leroy <[email protected]> writes:
> > Le 29/11/2021 ? 23:55, kernel test robot a ?crit?:
> >> Hi Christophe,
> >>
> >> I love your patch! Perhaps something to improve:
> >>
> >> [auto build test WARNING on powerpc/next]
> >> [also build test WARNING on v5.16-rc3 next-20211129]
> >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> >> And when submitting patch, we suggest to use '--base' as documented in
> >> https://git-scm.com/docs/git-format-patch]
> >>
> >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/[email protected]/config)
> >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> >> reproduce (this is a W=1 build):
> >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >> chmod +x ~/bin/make.cross
> >> # install powerpc cross compiling tool for clang build
> >> # apt-get install binutils-powerpc-linux-gnu
> >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> >> git remote add linux-review https://github.com/0day-ci/linux
> >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> >> # save the config file to linux build tree
> >> mkdir build_dir
> >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
> >>
> >> If you fix the issue, kindly add following tag as appropriate
> >> Reported-by: kernel test robot <[email protected]>
> >>
> >> All warnings (new ones prefixed by >>):
> >>
> >> In file included from arch/powerpc/kernel/asm-offsets.c:71:
> >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
> >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> >> *inst = ppc_inst(val);
> >> ^~~
> >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
> >> #define ppc_inst(x) (x)
> >> ^
> >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
> >> unsigned int val, suffix;
> >> ^
> >> = 0
> >
> > I can't understand what's wrong here.
> >
> > We have
> >
> > __get_kernel_nofault(&val, src, u32, Efault);
> > if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> > __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> > *inst = ppc_inst_prefix(val, suffix);
> > } else {
> > *inst = ppc_inst(val);
> > }
> >
> > With
> >
> > #define __get_kernel_nofault(dst, src, type, err_label) \
> > __get_user_size_goto(*((type *)(dst)), \
> > (__force type __user *)(src), sizeof(type), err_label)
> >
> >
> > And
> >
> > #define __get_user_size_goto(x, ptr, size, label) \
> > do { \
> > BUILD_BUG_ON(size > sizeof(x)); \
> > switch (size) { \
> > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
> > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
> > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
> > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
> > default: x = 0; BUILD_BUG(); \
> > } \
> > } while (0)
> >
> > And
> >
> > #define __get_user_asm_goto(x, addr, label, op) \
> > asm_volatile_goto( \
> > "1: "op"%U1%X1 %0, %1 # get_user\n" \
> > EX_TABLE(1b, %l2) \
> > : "=r" (x) \
> > : "m<>" (*addr) \
> > : \
> > : label)
> >
> >
> > I see no possibility, no alternative path where val wouldn't be set. The
> > asm clearly has *addr as an output param so it is always set.
>
> I guess clang can't convince itself of that?

A simplified reproducer:

$ cat test.c
static inline int copy_inst_from_kernel_nofault(unsigned int *inst,
unsigned int *src)
{
unsigned int val;

asm goto("1: lwz %U1%X1 %0, %1 # get_user\n"
".section __ex_table,\"a\";"
".balign 4;"
".long (1b) - . ;"
".long (%l2) - . ;"
".previous"
: "=r" (*(unsigned int *)(&val))
: "m<>" (*(unsigned int *)(src))
:
: Efault);

*inst = val;
return 0;

Efault:
return -14; /* -EFAULT */
}

$ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c
test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
*inst = val;
^~~
test.c:4:18: note: initialize the variable 'val' to silence this warning
unsigned int val;
^
= 0
1 warning generated.

It certainly looks like there is something wrong with how clang is
tracking the initialization of the variable because it looks to me like
val is only used in the fallthrough path, which happens after it is
initialized via lwz. Perhaps something is wrong with the logic of
https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
being migrated from Bugzilla to GitHub Issues right now so I cannot file
this upstream at the moment).

Cheers,
Nathan

2021-11-30 18:39:11

by Bill Wendling

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <[email protected]> wrote:
>
> On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
> > Christophe Leroy <[email protected]> writes:
> > > Le 29/11/2021 à 23:55, kernel test robot a écrit :
> > >> Hi Christophe,
> > >>
> > >> I love your patch! Perhaps something to improve:
> > >>
> > >> [auto build test WARNING on powerpc/next]
> > >> [also build test WARNING on v5.16-rc3 next-20211129]
> > >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> > >> And when submitting patch, we suggest to use '--base' as documented in
> > >> https://git-scm.com/docs/git-format-patch]
> > >>
> > >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> > >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> > >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/[email protected]/config)
> > >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> > >> reproduce (this is a W=1 build):
> > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > >> chmod +x ~/bin/make.cross
> > >> # install powerpc cross compiling tool for clang build
> > >> # apt-get install binutils-powerpc-linux-gnu
> > >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> > >> git remote add linux-review https://github.com/0day-ci/linux
> > >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> > >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> > >> # save the config file to linux build tree
> > >> mkdir build_dir
> > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
> > >>
> > >> If you fix the issue, kindly add following tag as appropriate
> > >> Reported-by: kernel test robot <[email protected]>
> > >>
> > >> All warnings (new ones prefixed by >>):
> > >>
> > >> In file included from arch/powerpc/kernel/asm-offsets.c:71:
> > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
> > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> > >> *inst = ppc_inst(val);
> > >> ^~~
> > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
> > >> #define ppc_inst(x) (x)
> > >> ^
> > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
> > >> unsigned int val, suffix;
> > >> ^
> > >> = 0
> > >
> > > I can't understand what's wrong here.
> > >
> > > We have
> > >
> > > __get_kernel_nofault(&val, src, u32, Efault);
> > > if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> > > __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> > > *inst = ppc_inst_prefix(val, suffix);
> > > } else {
> > > *inst = ppc_inst(val);
> > > }
> > >
> > > With
> > >
> > > #define __get_kernel_nofault(dst, src, type, err_label) \
> > > __get_user_size_goto(*((type *)(dst)), \
> > > (__force type __user *)(src), sizeof(type), err_label)
> > >
> > >
> > > And
> > >
> > > #define __get_user_size_goto(x, ptr, size, label) \
> > > do { \
> > > BUILD_BUG_ON(size > sizeof(x)); \
> > > switch (size) { \
> > > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
> > > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
> > > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
> > > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
> > > default: x = 0; BUILD_BUG(); \
> > > } \
> > > } while (0)
> > >
> > > And
> > >
> > > #define __get_user_asm_goto(x, addr, label, op) \
> > > asm_volatile_goto( \
> > > "1: "op"%U1%X1 %0, %1 # get_user\n" \
> > > EX_TABLE(1b, %l2) \
> > > : "=r" (x) \
> > > : "m<>" (*addr) \
> > > : \
> > > : label)
> > >
> > >
> > > I see no possibility, no alternative path where val wouldn't be set. The
> > > asm clearly has *addr as an output param so it is always set.
> >
> > I guess clang can't convince itself of that?
>
> A simplified reproducer:
>
> $ cat test.c
> static inline int copy_inst_from_kernel_nofault(unsigned int *inst,
> unsigned int *src)
> {
> unsigned int val;
>
> asm goto("1: lwz %U1%X1 %0, %1 # get_user\n"
> ".section __ex_table,\"a\";"
> ".balign 4;"
> ".long (1b) - . ;"
> ".long (%l2) - . ;"
> ".previous"
> : "=r" (*(unsigned int *)(&val))
> : "m<>" (*(unsigned int *)(src))
> :
> : Efault);
>
> *inst = val;
> return 0;
>
> Efault:
> return -14; /* -EFAULT */
> }
>
> $ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c
> test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> *inst = val;
> ^~~
> test.c:4:18: note: initialize the variable 'val' to silence this warning
> unsigned int val;
> ^
> = 0
> 1 warning generated.
>
> It certainly looks like there is something wrong with how clang is
> tracking the initialization of the variable because it looks to me like
> val is only used in the fallthrough path, which happens after it is
> initialized via lwz. Perhaps something is wrong with the logic of
> https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
> being migrated from Bugzilla to GitHub Issues right now so I cannot file
> this upstream at the moment).
>
If I remove the casts of "val" the warning doesn't appear. I suspect
that when I wrote that patch I forgot to remove those when checking.
#include "Captain_Picard_facepalm.h"

I'll look into it.

-bw

2021-11-30 18:44:55

by Bill Wendling

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <[email protected]> wrote:
>
> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <[email protected]> wrote:
> >
> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
> > > Christophe Leroy <[email protected]> writes:
> > > > Le 29/11/2021 à 23:55, kernel test robot a écrit :
> > > >> Hi Christophe,
> > > >>
> > > >> I love your patch! Perhaps something to improve:
> > > >>
> > > >> [auto build test WARNING on powerpc/next]
> > > >> [also build test WARNING on v5.16-rc3 next-20211129]
> > > >> [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > >> And when submitting patch, we suggest to use '--base' as documented in
> > > >> https://git-scm.com/docs/git-format-patch]
> > > >>
> > > >> url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> > > >> base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> > > >> config: powerpc-randconfig-r023-20211129 (https://download.01.org/0day-ci/archive/20211130/[email protected]/config)
> > > >> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
> > > >> reproduce (this is a W=1 build):
> > > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > > >> chmod +x ~/bin/make.cross
> > > >> # install powerpc cross compiling tool for clang build
> > > >> # apt-get install binutils-powerpc-linux-gnu
> > > >> # https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> > > >> git remote add linux-review https://github.com/0day-ci/linux
> > > >> git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
> > > >> git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
> > > >> # save the config file to linux build tree
> > > >> mkdir build_dir
> > > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare
> > > >>
> > > >> If you fix the issue, kindly add following tag as appropriate
> > > >> Reported-by: kernel test robot <[email protected]>
> > > >>
> > > >> All warnings (new ones prefixed by >>):
> > > >>
> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71:
> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> > > >> *inst = ppc_inst(val);
> > > >> ^~~
> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
> > > >> #define ppc_inst(x) (x)
> > > >> ^
> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
> > > >> unsigned int val, suffix;
> > > >> ^
> > > >> = 0
> > > >
> > > > I can't understand what's wrong here.
> > > >
> > > > We have
> > > >
> > > > __get_kernel_nofault(&val, src, u32, Efault);
> > > > if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> > > > __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> > > > *inst = ppc_inst_prefix(val, suffix);
> > > > } else {
> > > > *inst = ppc_inst(val);
> > > > }
> > > >
> > > > With
> > > >
> > > > #define __get_kernel_nofault(dst, src, type, err_label) \
> > > > __get_user_size_goto(*((type *)(dst)), \
> > > > (__force type __user *)(src), sizeof(type), err_label)
> > > >
> > > >
> > > > And
> > > >
> > > > #define __get_user_size_goto(x, ptr, size, label) \
> > > > do { \
> > > > BUILD_BUG_ON(size > sizeof(x)); \
> > > > switch (size) { \
> > > > case 1: __get_user_asm_goto(x, (u8 __user *)ptr, label, "lbz"); break; \
> > > > case 2: __get_user_asm_goto(x, (u16 __user *)ptr, label, "lhz"); break; \
> > > > case 4: __get_user_asm_goto(x, (u32 __user *)ptr, label, "lwz"); break; \
> > > > case 8: __get_user_asm2_goto(x, (u64 __user *)ptr, label); break; \
> > > > default: x = 0; BUILD_BUG(); \
> > > > } \
> > > > } while (0)
> > > >
> > > > And
> > > >
> > > > #define __get_user_asm_goto(x, addr, label, op) \
> > > > asm_volatile_goto( \
> > > > "1: "op"%U1%X1 %0, %1 # get_user\n" \
> > > > EX_TABLE(1b, %l2) \
> > > > : "=r" (x) \
> > > > : "m<>" (*addr) \
> > > > : \
> > > > : label)
> > > >
> > > >
> > > > I see no possibility, no alternative path where val wouldn't be set. The
> > > > asm clearly has *addr as an output param so it is always set.
> > >
> > > I guess clang can't convince itself of that?
> >
> > A simplified reproducer:
> >
> > $ cat test.c
> > static inline int copy_inst_from_kernel_nofault(unsigned int *inst,
> > unsigned int *src)
> > {
> > unsigned int val;
> >
> > asm goto("1: lwz %U1%X1 %0, %1 # get_user\n"
> > ".section __ex_table,\"a\";"
> > ".balign 4;"
> > ".long (1b) - . ;"
> > ".long (%l2) - . ;"
> > ".previous"
> > : "=r" (*(unsigned int *)(&val))
> > : "m<>" (*(unsigned int *)(src))
> > :
> > : Efault);
> >
> > *inst = val;
> > return 0;
> >
> > Efault:
> > return -14; /* -EFAULT */
> > }
> >
> > $ clang --target=powerpc-linux-gnu -Wuninitialized -fsyntax-only test.c
> > test.c:17:10: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> > *inst = val;
> > ^~~
> > test.c:4:18: note: initialize the variable 'val' to silence this warning
> > unsigned int val;
> > ^
> > = 0
> > 1 warning generated.
> >
> > It certainly looks like there is something wrong with how clang is
> > tracking the initialization of the variable because it looks to me like
> > val is only used in the fallthrough path, which happens after it is
> > initialized via lwz. Perhaps something is wrong with the logic of
> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
> > being migrated from Bugzilla to GitHub Issues right now so I cannot file
> > this upstream at the moment).
> >
> If I remove the casts of "val" the warning doesn't appear. I suspect
> that when I wrote that patch I forgot to remove those when checking.
> #include "Captain_Picard_facepalm.h"
>
> I'll look into it.
>
Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")

2021-12-01 06:46:14

by kernel test robot

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Hi Christophe,

I love your patch! Perhaps something to improve:

[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.16-rc3 next-20211201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-randconfig-r021-20211201 (https://download.01.org/0day-ci/archive/20211201/[email protected]/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 25eb7fa01d7ebbe67648ea03841cda55b4239ab2)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/fb7bff30cc0efc7e4df1b48bb69de1f325eee826
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Christophe-Leroy/powerpc-inst-Refactor-___get_user_instr/20211130-015346
git checkout fb7bff30cc0efc7e4df1b48bb69de1f325eee826
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc prepare

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:143:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:558:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:145:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:559:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:147:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:560:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:21:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:619:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:616:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:149:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:561:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from arch/powerpc/kernel/asm-offsets.c:71:
In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
arch/powerpc/include/asm/inst.h:161:41: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
^~~
arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
unsigned int val, suffix;
^
= 0
>> arch/powerpc/include/asm/inst.h:163:32: warning: variable 'suffix' is uninitialized when used here [-Wuninitialized]
*inst = ppc_inst_prefix(val, suffix);
^~~~~~
arch/powerpc/include/asm/inst.h:62:69: note: expanded from macro 'ppc_inst_prefix'
#define ppc_inst_prefix(x, y) ((ppc_inst_t){ .val = (x), .suffix = (y) })
^
arch/powerpc/include/asm/inst.h:155:26: note: initialize the variable 'suffix' to silence this warning
unsigned int val, suffix;
^
= 0
14 warnings generated.
<stdin>:1559:2: warning: syscall futex_waitv not implemented [-W#warnings]
#warning syscall futex_waitv not implemented
^
1 warning generated.
/usr/bin/ld: unrecognised emulation mode: elf64ppc
Supported emulations: elf_x86_64 elf32_x86_64 elf_i386 elf_iamcu elf_l1om elf_k1om i386pep i386pe
clang-14: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [arch/powerpc/kernel/vdso64/Makefile:44: arch/powerpc/kernel/vdso64/vdso64.so.dbg] Error 1
make[2]: Target 'include/generated/vdso64-offsets.h' not remade because of errors.
make[1]: *** [arch/powerpc/Makefile:422: vdso_prepare] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [Makefile:219: __sub-make] Error 2
make: Target 'prepare' not remade because of errors.


vim +/suffix +163 arch/powerpc/include/asm/inst.h

152
153 static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
154 {
155 unsigned int val, suffix;
156
157 if (unlikely(!is_kernel_addr((unsigned long)src)))
158 return -ERANGE;
159
160 __get_kernel_nofault(&val, src, u32, Efault);
161 if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
162 __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> 163 *inst = ppc_inst_prefix(val, suffix);
164 } else {
165 *inst = ppc_inst(val);
166 }
167 return 0;
168 Efault:
169 return -EFAULT;
170 }
171

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

2021-12-07 03:38:02

by Michael Ellerman

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Bill Wendling <[email protected]> writes:
> On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <[email protected]> wrote:
>> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <[email protected]> wrote:
>> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
>> > > Christophe Leroy <[email protected]> writes:
>> > > > Le 29/11/2021 à 23:55, kernel test robot a écrit :
...
>> > > >> All warnings (new ones prefixed by >>):
>> > > >>
>> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71:
>> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
>> > > >> *inst = ppc_inst(val);
>> > > >> ^~~
>> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
>> > > >> #define ppc_inst(x) (x)
>> > > >> ^
>> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
>> > > >> unsigned int val, suffix;
>> > > >> ^
>> > > >> = 0
>> > > >
>> > > > I can't understand what's wrong here.
...
>> > > >
>> > > > I see no possibility, no alternative path where val wouldn't be set. The
>> > > > asm clearly has *addr as an output param so it is always set.
>> > >
>> > > I guess clang can't convince itself of that?
...
>> >
>> > It certainly looks like there is something wrong with how clang is
>> > tracking the initialization of the variable because it looks to me like
>> > val is only used in the fallthrough path, which happens after it is
>> > initialized via lwz. Perhaps something is wrong with the logic of
>> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
>> > being migrated from Bugzilla to GitHub Issues right now so I cannot file
>> > this upstream at the moment).
>> >
>> If I remove the casts of "val" the warning doesn't appear. I suspect
>> that when I wrote that patch I forgot to remove those when checking.
>> #include "Captain_Picard_facepalm.h"
>>
>> I'll look into it.
>>
> Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")

I guess for now I'll just squash this in as a workaround?


diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 631436f3f5c3..5b591c51fec9 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h
@@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
if (unlikely(!is_kernel_addr((unsigned long)src)))
return -ERANGE;

+#ifdef CONFIG_CC_IS_CLANG
+ val = suffix = 0;
+#endif
__get_kernel_nofault(&val, src, u32, Efault);
if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
__get_kernel_nofault(&suffix, src + 1, u32, Efault);



cheers

2021-12-07 04:49:08

by Nathan Chancellor

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:
> Bill Wendling <[email protected]> writes:
> > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <[email protected]> wrote:
> >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <[email protected]> wrote:
> >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
> >> > > Christophe Leroy <[email protected]> writes:
> >> > > > Le 29/11/2021 ? 23:55, kernel test robot a ?crit :
> ...
> >> > > >> All warnings (new ones prefixed by >>):
> >> > > >>
> >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71:
> >> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
> >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> >> > > >> *inst = ppc_inst(val);
> >> > > >> ^~~
> >> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
> >> > > >> #define ppc_inst(x) (x)
> >> > > >> ^
> >> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
> >> > > >> unsigned int val, suffix;
> >> > > >> ^
> >> > > >> = 0
> >> > > >
> >> > > > I can't understand what's wrong here.
> ...
> >> > > >
> >> > > > I see no possibility, no alternative path where val wouldn't be set. The
> >> > > > asm clearly has *addr as an output param so it is always set.
> >> > >
> >> > > I guess clang can't convince itself of that?
> ...
> >> >
> >> > It certainly looks like there is something wrong with how clang is
> >> > tracking the initialization of the variable because it looks to me like
> >> > val is only used in the fallthrough path, which happens after it is
> >> > initialized via lwz. Perhaps something is wrong with the logic of
> >> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
> >> > being migrated from Bugzilla to GitHub Issues right now so I cannot file
> >> > this upstream at the moment).
> >> >
> >> If I remove the casts of "val" the warning doesn't appear. I suspect
> >> that when I wrote that patch I forgot to remove those when checking.
> >> #include "Captain_Picard_facepalm.h"
> >>
> >> I'll look into it.
> >>
> > Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")
>
> I guess for now I'll just squash this in as a workaround?
>
>
> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> index 631436f3f5c3..5b591c51fec9 100644
> --- a/arch/powerpc/include/asm/inst.h
> +++ b/arch/powerpc/include/asm/inst.h
> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
> if (unlikely(!is_kernel_addr((unsigned long)src)))
> return -ERANGE;

Could we add a version check to this and a link to our bug tracker:

/* https://github.com/ClangBuiltLinux/linux/issues/1521 */
#if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000

> +#ifdef CONFIG_CC_IS_CLANG
> + val = suffix = 0;
> +#endif
> __get_kernel_nofault(&val, src, u32, Efault);
> if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> __get_kernel_nofault(&suffix, src + 1, u32, Efault);
>
>
>
> cheers

2021-12-07 05:45:13

by Christophe Leroy

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Le 07/12/2021 à 05:48, Nathan Chancellor a écrit :
> On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:
>> Bill Wendling <[email protected]> writes:
>>> On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <[email protected]> wrote:
>>>> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <[email protected]> wrote:
>>>>> On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
>>>>>> Christophe Leroy <[email protected]> writes:
>>>>>>> Le 29/11/2021 à 23:55, kernel test robot a écrit :
>> ...
>>>>>>>> All warnings (new ones prefixed by >>):
>>>>>>>>
>>>>>>>> In file included from arch/powerpc/kernel/asm-offsets.c:71:
>>>>>>>> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>>>>>>>>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
>>>>>>>> *inst = ppc_inst(val);
>>>>>>>> ^~~
>>>>>>>> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
>>>>>>>> #define ppc_inst(x) (x)
>>>>>>>> ^
>>>>>>>> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
>>>>>>>> unsigned int val, suffix;
>>>>>>>> ^
>>>>>>>> = 0
>>>>>>>
>>>>>>> I can't understand what's wrong here.
>> ...
>>>>>>>
>>>>>>> I see no possibility, no alternative path where val wouldn't be set. The
>>>>>>> asm clearly has *addr as an output param so it is always set.
>>>>>>
>>>>>> I guess clang can't convince itself of that?
>> ...
>>>>>
>>>>> It certainly looks like there is something wrong with how clang is
>>>>> tracking the initialization of the variable because it looks to me like
>>>>> val is only used in the fallthrough path, which happens after it is
>>>>> initialized via lwz. Perhaps something is wrong with the logic of
>>>>> https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
>>>>> being migrated from Bugzilla to GitHub Issues right now so I cannot file
>>>>> this upstream at the moment).
>>>>>
>>>> If I remove the casts of "val" the warning doesn't appear. I suspect
>>>> that when I wrote that patch I forgot to remove those when checking.
>>>> #include "Captain_Picard_facepalm.h"
>>>>
>>>> I'll look into it.
>>>>
>>> Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")
>>
>> I guess for now I'll just squash this in as a workaround?
>>
>>
>> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>> index 631436f3f5c3..5b591c51fec9 100644
>> --- a/arch/powerpc/include/asm/inst.h
>> +++ b/arch/powerpc/include/asm/inst.h
>> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
>> if (unlikely(!is_kernel_addr((unsigned long)src)))
>> return -ERANGE;
>
> Could we add a version check to this and a link to our bug tracker:
>
> /* https://github.com/ClangBuiltLinux/linux/issues/1521 */
> #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000

The robot reported the problem on:

compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)

Should it be CONFIG_CLANG_VERSION <= 140000 ?

>
>> +#ifdef CONFIG_CC_IS_CLANG
>> + val = suffix = 0;
>> +#endif
>> __get_kernel_nofault(&val, src, u32, Efault);
>> if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
>> __get_kernel_nofault(&suffix, src + 1, u32, Efault);
>>

Christophe

2021-12-07 06:41:51

by Nathan Chancellor

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

On Tue, Dec 07, 2021 at 05:45:08AM +0000, Christophe Leroy wrote:
>
>
> Le 07/12/2021 ? 05:48, Nathan Chancellor a ?crit?:
> > On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:
> >> Bill Wendling <[email protected]> writes:
> >>> On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <[email protected]> wrote:
> >>>> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <[email protected]> wrote:
> >>>>> On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
> >>>>>> Christophe Leroy <[email protected]> writes:
> >>>>>>> Le 29/11/2021 ? 23:55, kernel test robot a ?crit :
> >> ...
> >>>>>>>> All warnings (new ones prefixed by >>):
> >>>>>>>>
> >>>>>>>> In file included from arch/powerpc/kernel/asm-offsets.c:71:
> >>>>>>>> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
> >>>>>>>>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
> >>>>>>>> *inst = ppc_inst(val);
> >>>>>>>> ^~~
> >>>>>>>> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
> >>>>>>>> #define ppc_inst(x) (x)
> >>>>>>>> ^
> >>>>>>>> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
> >>>>>>>> unsigned int val, suffix;
> >>>>>>>> ^
> >>>>>>>> = 0
> >>>>>>>
> >>>>>>> I can't understand what's wrong here.
> >> ...
> >>>>>>>
> >>>>>>> I see no possibility, no alternative path where val wouldn't be set. The
> >>>>>>> asm clearly has *addr as an output param so it is always set.
> >>>>>>
> >>>>>> I guess clang can't convince itself of that?
> >> ...
> >>>>>
> >>>>> It certainly looks like there is something wrong with how clang is
> >>>>> tracking the initialization of the variable because it looks to me like
> >>>>> val is only used in the fallthrough path, which happens after it is
> >>>>> initialized via lwz. Perhaps something is wrong with the logic of
> >>>>> https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
> >>>>> being migrated from Bugzilla to GitHub Issues right now so I cannot file
> >>>>> this upstream at the moment).
> >>>>>
> >>>> If I remove the casts of "val" the warning doesn't appear. I suspect
> >>>> that when I wrote that patch I forgot to remove those when checking.
> >>>> #include "Captain_Picard_facepalm.h"
> >>>>
> >>>> I'll look into it.
> >>>>
> >>> Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")
> >>
> >> I guess for now I'll just squash this in as a workaround?
> >>
> >>
> >> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
> >> index 631436f3f5c3..5b591c51fec9 100644
> >> --- a/arch/powerpc/include/asm/inst.h
> >> +++ b/arch/powerpc/include/asm/inst.h
> >> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
> >> if (unlikely(!is_kernel_addr((unsigned long)src)))
> >> return -ERANGE;
> >
> > Could we add a version check to this and a link to our bug tracker:
> >
> > /* https://github.com/ClangBuiltLinux/linux/issues/1521 */
> > #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000
>
> The robot reported the problem on:
>
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
> df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
>
> Should it be CONFIG_CLANG_VERSION <= 140000 ?

The robot tests clang from tip of tree, rebuilding every week or so. The
fix is getting ready to land so it will be released in 14.0.0 final. We
have always written tip of tree version checks with the expectation that
if people are testing tip of tree clang, they are frequently rebuilding.
If that is not true, they need to be using released/stable versions,
otherwise the model is broken.

If that is too problematic, we could add a version check to Kconfig
(cannot think of a great name for the config off the top of my head)
that checks for this issue and ifdef on that. That might be nice in
case another instance of this crops up in the future.

Cheers,
Nathan

> >
> >> +#ifdef CONFIG_CC_IS_CLANG
> >> + val = suffix = 0;
> >> +#endif
> >> __get_kernel_nofault(&val, src, u32, Efault);
> >> if (IS_ENABLED(CONFIG_PPC64) && get_op(val) == OP_PREFIX) {
> >> __get_kernel_nofault(&suffix, src + 1, u32, Efault);
> >>
>
> Christophe

2021-12-07 07:55:48

by Christophe Leroy

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Le 07/12/2021 à 07:41, Nathan Chancellor a écrit :
> On Tue, Dec 07, 2021 at 05:45:08AM +0000, Christophe Leroy wrote:
>>
>>
>> Le 07/12/2021 à 05:48, Nathan Chancellor a écrit :
>>> On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:
>>>> Bill Wendling <[email protected]> writes:
>>>>> On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <[email protected]> wrote:
>>>>>> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <[email protected]> wrote:
>>>>>>> On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
>>>>>>>> Christophe Leroy <[email protected]> writes:
>>>>>>>>> Le 29/11/2021 à 23:55, kernel test robot a écrit :
>>>> ...
>>>>>>>>>> All warnings (new ones prefixed by >>):
>>>>>>>>>>
>>>>>>>>>> In file included from arch/powerpc/kernel/asm-offsets.c:71:
>>>>>>>>>> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>>>>>>>>>>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
>>>>>>>>>> *inst = ppc_inst(val);
>>>>>>>>>> ^~~
>>>>>>>>>> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
>>>>>>>>>> #define ppc_inst(x) (x)
>>>>>>>>>> ^
>>>>>>>>>> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
>>>>>>>>>> unsigned int val, suffix;
>>>>>>>>>> ^
>>>>>>>>>> = 0
>>>>>>>>>
>>>>>>>>> I can't understand what's wrong here.
>>>> ...
>>>>>>>>>
>>>>>>>>> I see no possibility, no alternative path where val wouldn't be set. The
>>>>>>>>> asm clearly has *addr as an output param so it is always set.
>>>>>>>>
>>>>>>>> I guess clang can't convince itself of that?
>>>> ...
>>>>>>>
>>>>>>> It certainly looks like there is something wrong with how clang is
>>>>>>> tracking the initialization of the variable because it looks to me like
>>>>>>> val is only used in the fallthrough path, which happens after it is
>>>>>>> initialized via lwz. Perhaps something is wrong with the logic of
>>>>>>> https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
>>>>>>> being migrated from Bugzilla to GitHub Issues right now so I cannot file
>>>>>>> this upstream at the moment).
>>>>>>>
>>>>>> If I remove the casts of "val" the warning doesn't appear. I suspect
>>>>>> that when I wrote that patch I forgot to remove those when checking.
>>>>>> #include "Captain_Picard_facepalm.h"
>>>>>>
>>>>>> I'll look into it.
>>>>>>
>>>>> Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")
>>>>
>>>> I guess for now I'll just squash this in as a workaround?
>>>>
>>>>
>>>> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>>>> index 631436f3f5c3..5b591c51fec9 100644
>>>> --- a/arch/powerpc/include/asm/inst.h
>>>> +++ b/arch/powerpc/include/asm/inst.h
>>>> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
>>>> if (unlikely(!is_kernel_addr((unsigned long)src)))
>>>> return -ERANGE;
>>>
>>> Could we add a version check to this and a link to our bug tracker:
>>>
>>> /* https://github.com/ClangBuiltLinux/linux/issues/1521 */
>>> #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000
>>
>> The robot reported the problem on:
>>
>> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project
>> df08b2fe8b35cb63dfb3b49738a3494b9b4e6f8e)
>>
>> Should it be CONFIG_CLANG_VERSION <= 140000 ?
>
> The robot tests clang from tip of tree, rebuilding every week or so. The
> fix is getting ready to land so it will be released in 14.0.0 final. We
> have always written tip of tree version checks with the expectation that
> if people are testing tip of tree clang, they are frequently rebuilding.
> If that is not true, they need to be using released/stable versions,
> otherwise the model is broken.
>
> If that is too problematic, we could add a version check to Kconfig
> (cannot think of a great name for the config off the top of my head)
> that checks for this issue and ifdef on that. That might be nice in
> case another instance of this crops up in the future.
>

It's fine for me. I didn't know robot was using prereleases with the
same name as the future release.

Christophe

2021-12-07 11:19:13

by Michael Ellerman

Subject: Re: [PATCH v5 5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()

Nathan Chancellor <[email protected]> writes:
> On Tue, Dec 07, 2021 at 02:37:26PM +1100, Michael Ellerman wrote:
>> Bill Wendling <[email protected]> writes:
>> > On Tue, Nov 30, 2021 at 10:38 AM Bill Wendling <[email protected]> wrote:
>> >> On Tue, Nov 30, 2021 at 10:17 AM Nathan Chancellor <[email protected]> wrote:
>> >> > On Tue, Nov 30, 2021 at 10:25:43PM +1100, Michael Ellerman wrote:
>> >> > > Christophe Leroy <[email protected]> writes:
>> >> > > > Le 29/11/2021 à 23:55, kernel test robot a écrit :
>> ...
>> >> > > >> All warnings (new ones prefixed by >>):
>> >> > > >>
>> >> > > >> In file included from arch/powerpc/kernel/asm-offsets.c:71:
>> >> > > >> In file included from arch/powerpc/kernel/../xmon/xmon_bpts.h:7:
>> >> > > >>>> arch/powerpc/include/asm/inst.h:165:20: warning: variable 'val' is uninitialized when used here [-Wuninitialized]
>> >> > > >> *inst = ppc_inst(val);
>> >> > > >> ^~~
>> >> > > >> arch/powerpc/include/asm/inst.h:53:22: note: expanded from macro 'ppc_inst'
>> >> > > >> #define ppc_inst(x) (x)
>> >> > > >> ^
>> >> > > >> arch/powerpc/include/asm/inst.h:155:18: note: initialize the variable 'val' to silence this warning
>> >> > > >> unsigned int val, suffix;
>> >> > > >> ^
>> >> > > >> = 0
>> >> > > >
>> >> > > > I can't understand what's wrong here.
>> ...
>> >> > > >
>> >> > > > I see no possibility, no alternative path where val wouldn't be set. The
>> >> > > > asm clearly has *addr as an output param so it is always set.
>> >> > >
>> >> > > I guess clang can't convince itself of that?
>> ...
>> >> >
>> >> > It certainly looks like there is something wrong with how clang is
>> >> > tracking the initialization of the variable because it looks to me like
>> >> > val is only used in the fallthrough path, which happens after it is
>> >> > initialized via lwz. Perhaps something is wrong with the logic of
>> >> > https://reviews.llvm.org/D71314? I've added Bill to CC (LLVM issues are
>> >> > being migrated from Bugzilla to GitHub Issues right now so I cannot file
>> >> > this upstream at the moment).
>> >> >
>> >> If I remove the casts of "val" the warning doesn't appear. I suspect
>> >> that when I wrote that patch I forgot to remove those when checking.
>> >> #include "Captain_Picard_facepalm.h"
>> >>
>> >> I'll look into it.
>> >>
>> > Small retraction. It's the "*(<cast>)&val" that's the issue. (I.e. the "*&")
>>
>> I guess for now I'll just squash this in as a workaround?
>>
>>
>> diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
>> index 631436f3f5c3..5b591c51fec9 100644
>> --- a/arch/powerpc/include/asm/inst.h
>> +++ b/arch/powerpc/include/asm/inst.h
>> @@ -157,6 +157,9 @@ static inline int copy_inst_from_kernel_nofault(ppc_inst_t *inst, u32 *src)
>> if (unlikely(!is_kernel_addr((unsigned long)src)))
>> return -ERANGE;
>
> Could we add a version check to this and a link to our bug tracker:
>
> /* https://github.com/ClangBuiltLinux/linux/issues/1521 */
> #if defined(CONFIG_CC_IS_CLANG) && CONFIG_CLANG_VERSION < 140000

Yep, thanks I'll use that.

cheers

2021-12-15 00:27:03

by Michael Ellerman

Subject: Re: [PATCH v5 1/5] powerpc/inst: Refactor ___get_user_instr()

On Mon, 29 Nov 2021 18:49:37 +0100, Christophe Leroy wrote:
> PPC64 version of ___get_user_instr() can be used for PPC32 as well,
> by simply disabling the suffix part with IS_ENABLED(CONFIG_PPC64).
>
>

Applied to powerpc/next.

[1/5] powerpc/inst: Refactor ___get_user_instr()
https://git.kernel.org/powerpc/c/3261d99adba269a024d0e55737beeedec5eba00e
[2/5] powerpc/inst: Define ppc_inst_t
https://git.kernel.org/powerpc/c/c545b9f040f341038d5228932140fb17e0c156e2
[3/5] powerpc/inst: Define ppc_inst_t as u32 on PPC32
https://git.kernel.org/powerpc/c/07b863aef5b682a482474b524f3df4957d2862ac
[4/5] powerpc/inst: Move ppc_inst_t definition in asm/reg.h
https://git.kernel.org/powerpc/c/9b307576f37136d37d5e42b1d8713ec34a601a62
[5/5] powerpc/inst: Optimise copy_inst_from_kernel_nofault()
https://git.kernel.org/powerpc/c/0d76914a4c99ab5658f3fb07cdf3799d28e2eab3

cheers