On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npig...@gmail.com> wrote:
>
> Jordan Niethe's on February 26, 2020 2:07 pm:
> > @@ -136,11 +148,14 @@ int arch_prepare_kprobe(struct kprobe *p)
> > }
> >
> > if (!ret) {
> > - patch_instruction(p->ainsn.insn, *p->addr);
> > + patch_instruction(&p->ainsn.insn[0], p->addr[0]);
> > + if (IS_PREFIX(insn))
> > + patch_instruction(&p->ainsn.insn[1], p->addr[1]);
> > p->opcode = *p->addr;
>
> Not to single out this hunk or this patch even, but what do you reckon
> about adding an instruction data type, and then use that in all these
> call sites rather than adding the extra arg or doing the extra copy
> manually in each place depending on prefix?
>
> instrs_are_equal, get_user_instr, analyse_instr, patch_instruction,
> etc., would all take this new instr. Places that open code a memory
> access like your MCE change need some accessor
>
> instr = *(unsigned int *)(instr_addr);
> - if (!analyse_instr(&op, &tmp, instr, PPC_NO_SUFFIX)) {
> + if (IS_PREFIX(instr))
> + suffix = *(unsigned int *)(instr_addr + 4);
>
> Becomes
> read_instr(instr_addr, &instr);
> if (!analyse_instr(&op, &tmp, instr)) ...
>
> etc.
Daniel Axtens also talked about this and my reasons not to do so were
pretty unconvincing, so I started trying something like this. One
thing I have been wondering is how pervasive should the new type be.
Below is data type I have started using, which I think works
reasonably for replacing unsigned ints everywhere (like within
code-patching.c). In a few architecture independent places such as
uprobes which want to do ==, etc the union type does not work so well.
I will have the next revision of the series start using a type.
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
new file mode 100644
index 000000000000..50adb3dbdeb4
--- /dev/null
+++ b/arch/powerpc/include/asm/inst.h
@@ -0,0 +1,87 @@
+
+#ifndef _ASM_INST_H
+#define _ASM_INST_H
+
+#ifdef __powerpc64__
+
+/* 64 bit Instruction */
+
+typedef struct {
+ unsigned int prefix;
+ unsigned int suffix;
+} __packed ppc_prefixed_inst;
+
+typedef union ppc_inst {
+ unsigned int w;
+ ppc_prefixed_inst p;
+} ppc_inst;
+
+#define PPC_INST_IS_PREFIXED(inst) (((inst).w >> 26) == 1)
+#define PPC_INST_LEN(inst) (PPC_INST_IS_PREFIXED((inst)) ?
sizeof((inst).p) : sizeof((inst).w))
+
+#define PPC_INST_NEW_WORD(x) ((ppc_inst) { .w = (x) })
+#define PPC_INST_NEW_WORD_PAD(x) ((ppc_inst) { .p.prefix = (x),
.p.suffix = (0x60000000) })
+#define PPC_INST_NEW_PREFIXED(x, y) ((ppc_inst) { .p.prefix = (x),
.p.suffix = (y) })
+
+#define PPC_INST_WORD(x) ((x).w)
+#define PPC_INST_PREFIX(x) (x.p.prefix)
+#define PPC_INST_SUFFIX(x) (x.p.suffix)
+#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
+
+#define DEREF_PPC_INST_PTR(ptr) \
+({ \
+ ppc_inst __inst; \
+ __inst.w = *(unsigned int *)(ptr); \
+ if (PPC_INST_IS_PREFIXED(__inst)) \
+ __inst.p = *(ppc_prefixed_inst *)(ptr); \
+ __inst; \
+})
+
+#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
+#define PPC_INST_PREV(ptr) ((ptr) -= PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr))))
+
+#define PPC_INST_EQ(x, y) \
+({ \
+ long pic_ret = 0; \
+ pic_ret = (PPC_INST_PREFIX(x) == PPC_INST_PREFIX(y)); \
+ if (pic_ret) { \
+ if (PPC_INST_IS_PREFIXED(x) && PPC_INST_IS_PREFIXED(y)) { \
+ pic_ret = (PPC_INST_SUFFIX(x) == PPC_INST_SUFFIX(y)); \
+ } else { \
+ pic_ret = 0; \
+ } \
+ } \
+ pic_ret; \
+})
+
+#else /* !__powerpc64__ */
+
+/* 32 bit Instruction */
+
+typedef unsigned int ppc_inst;
+
+#define PPC_INST_IS_PREFIXED(inst) (0)
+#define PPC_INST_LEN(inst) (4)
+
+#define PPC_INST_NEW_WORD(x) (x)
+#define PPC_INST_NEW_WORD_PAD(x) (x)
+#define PPC_INST_NEW_PREFIXED(x, y) (x)
+
+#define PPC_INST_WORD(x) (x)
+#define PPC_INST_PREFIX(x) (x)
+#define PPC_INST_SUFFIX(x) (0)
+#define PPC_INST_EMPTY(x) (PPC_INST_WORD(x) == 0)
+
+#define DEREF_PPC_INST_PTR(ptr) (*ptr)
+
+#define PPC_INST_NEXT(ptr) ((ptr) += 4)
+#define PPC_INST_PREV(ptr) ((ptr) -= 4)
+
+#define PPC_INST_EQ(x, y) ((x) == (y))
+
+#endif /* __powerpc64__ */
+
+
+#endif /* _ASM_INST_H */
>
> Thanks,
> Nick
