[PATCH v3 11/14] powerpc/kprobes: Support kprobes on prefixed instructions
Jordan Niethe
jniethe5 at gmail.com
Fri Feb 28 14:23:46 AEDT 2020
On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin <npiggin at gmail.com> wrote:
>
> Jordan Niethe's on February 27, 2020 10:58 am:
> > 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
>
> 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?
One of the places I was thinking of was is_swbp_insn() in
kernel/events/uprobes.c. The problem was I wanted to typedef
uprobe_opcode_t as ppc_insn type which was probably the wrong thing to
do. x86 typedef's it as u8 (size of the trap they use). So we probably
can do the same thing and just keep it as a u32.
>
> > I will have the next revision of the series start using a type.
>
> Thanks for doing that.
>
> >
> > 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;
>
> u32?
Sure.
>
> > +} __packed ppc_prefixed_inst;
> > +
> > +typedef union ppc_inst {
> > + unsigned int w;
> > + ppc_prefixed_inst p;
> > +} ppc_inst;
>
> I'd make it a struct and use nameless structs/unions inside it (with
> appropriate packed annotation):
Yeah that will be nicer.
>
> 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.
Will change.
>
> > +
> > +#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)
Good idea, they do end up used quite widely.
>
> 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).
>
> > +
> > +#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)
>
> 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.
What I was thinking with these macros was that they would let us keep
the instruction type as a simple u32 on 32 bit ppc. Is it worth trying
to do that?
>
> > +
> > +#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; \
> > +})
>
> I'd go an inline with shorter lowercase names. ppc_inst_read(&inst);
Will do.
>
> > +#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)
Good point.
>
>
> > +#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; \
> > +})
>
> static inline bool ppc_inst_eq(struct ppc_inst x, struct ppc_inst y)
> {
> return !memcmp(&x, &y, sizeof(struct ppc_inst));
> }
Well that is definitely cleaner!
Thank you for the feedback.
>
> If all accessors and initalisers are such that the padding gets set,
> that'll work.
>
> Thanks,
> Nick
More information about the Linuxppc-dev
mailing list