[PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions

[Jordan Niethe]

On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin at 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