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

Jordan Niethe's on February 27, 2020 10:58 am:

On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin [off-list ref] wrote:

quoted

Jordan Niethe's on February 26, 2020 2:07 pm:

quoted

@@ -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

Okay.

thing I have been wondering is how pervasive should the new type be.

I wouldn't mind it being quite pervasive. We have to be careful not
to copy it directly to/from memory now, but if we have accessors to
do all that with, I think it should be okay.

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.

There will be some places you have to make the boundary. I would start
by just making it a powerpc thing, but possibly there is or could be
some generic helpers. How does something like x86 cope with this?

I will have the next revision of the series start using a type.

Thanks for doing that.

u32?

I'd make it a struct and use nameless structs/unions inside it (with
appropriate packed annotation):

struct ppc_inst {
    union {
        struct {
            u32 word;
	    u32 pad;
	};
	struct {
	    u32 prefix;
	    u32 suffix;
	};
    };
};

+
+#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))

Good accessors, I'd make them all C inline functions and lower case.

+
+#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) })

If these are widely used, I'd make them a bit shorter

#define PPC_INST(x)
#define PPC_INST_PREFIXED(x)

I'd also set padding to something invalid 0 or 0xffffffff for the first
case, and have your accessors check that rather than carrying around
another type of ppc_inst (prefixed, padded, non-padded).

I'd avoid simple accessors like this completely. If they have any use
it would be to ensure you don't try to use prefix/suffix on a 
non-prefixed instruction for example. If you want to do that I'd use
inline functions for it.

I'd go an inline with shorter lowercase names. ppc_inst_read(&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))))

Wouldn't bother with this accessor, caller could ptr += ppc_inst_len(inst)

static inline bool ppc_inst_eq(struct ppc_inst x, struct ppc_inst y)
{
	return !memcmp(&x, &y, sizeof(struct ppc_inst));
}

If all accessors and initalisers are such that the padding gets set,
that'll work.

Thanks,
Nick