| Message ID | 20200226040716.32395-12-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Initial Prefixed Instruction support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (65b2623f395a4e25ab3ff4cff1c9c7623619a22d) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (71c3a888cbcaf453aecf8d2f8fb003271d28073f) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (c5f86891185c408b2241ba9a82ae8622d8386aff) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (9eb425b2e04e0e3006adffea5bf5f227a896f128) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (f3fef7e990dae5fcd7cd8ccbd9b2a98bdb481da8) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
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. Thanks, Nick
On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@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
Jordan Niethe's on February 27, 2020 10:58 am: > On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@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? > 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? > +} __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): 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). > + > +#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. > + > +#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); > +#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) > +#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)); } If all accessors and initalisers are such that the padding gets set, that'll work. Thanks, Nick
Nicholas Piggin's on February 28, 2020 11:47 am: > Jordan Niethe's on February 27, 2020 10:58 am: >> On Wed, Feb 26, 2020 at 6:18 PM Nicholas Piggin <npiggin@gmail.com> wrote: >> + >> +#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); Probably inst = ppc_inst_read(mem), rather. Thanks, Nick
On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin <npiggin@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@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
Jordan Niethe's on February 28, 2020 1:23 pm: > On Fri, Feb 28, 2020 at 12:48 PM Nicholas Piggin <npiggin@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@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. Sounds good. >> > 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? Hmm, it might be. On the other hand if ppc32 can use the same struct ppc_insn struct it might help share code without too many macros. I guess it's up to Christophe and Michael and you I won't complain any more (so long as they're lower case and inlines where possible :)) Thanks, Nick
diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h index 66b3f2983b22..0d44ce8a3163 100644 --- a/arch/powerpc/include/asm/kprobes.h +++ b/arch/powerpc/include/asm/kprobes.h @@ -38,12 +38,13 @@ extern kprobe_opcode_t optprobe_template_entry[]; extern kprobe_opcode_t optprobe_template_op_address[]; extern kprobe_opcode_t optprobe_template_call_handler[]; extern kprobe_opcode_t optprobe_template_insn[]; +extern kprobe_opcode_t optprobe_template_suffix[]; extern kprobe_opcode_t optprobe_template_call_emulate[]; extern kprobe_opcode_t optprobe_template_ret[]; extern kprobe_opcode_t optprobe_template_end[]; -/* Fixed instruction size for powerpc */ -#define MAX_INSN_SIZE 1 +/* Prefixed instructions are two words */ +#define MAX_INSN_SIZE 2 #define MAX_OPTIMIZED_LENGTH sizeof(kprobe_opcode_t) /* 4 bytes */ #define MAX_OPTINSN_SIZE (optprobe_template_end - optprobe_template_entry) #define RELATIVEJUMP_SIZE sizeof(kprobe_opcode_t) /* 4 bytes */ diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index 6b2e9e37f12b..9ccf1b9a1275 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -117,16 +117,28 @@ void *alloc_insn_page(void) int arch_prepare_kprobe(struct kprobe *p) { int ret = 0; + struct kprobe *prev; kprobe_opcode_t insn = *p->addr; + kprobe_opcode_t prefix = *(p->addr - 1); + preempt_disable(); if ((unsigned long)p->addr & 0x03) { printk("Attempt to register kprobe at an unaligned address\n"); ret = -EINVAL; } else if (IS_MTMSRD(insn) || IS_RFID(insn) || IS_RFI(insn)) { printk("Cannot register a kprobe on rfi/rfid or mtmsr[d]\n"); ret = -EINVAL; + } else if (IS_PREFIX(prefix)) { + printk("Cannot register a kprobe on the second word of prefixed instruction\n"); + ret = -EINVAL; + } + prev = get_kprobe(p->addr - 1); + if (prev && IS_PREFIX(*prev->ainsn.insn)) { + printk("Cannot register a kprobe on the second word of prefixed instruction\n"); + ret = -EINVAL; } + /* insn must be on a special executable page on ppc64. This is * not explicitly required on ppc32 (right now), but it doesn't hurt */ if (!ret) { @@ -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; } p->ainsn.boostable = 0; + preempt_enable_no_resched(); return ret; } NOKPROBE_SYMBOL(arch_prepare_kprobe); @@ -225,10 +240,11 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe); static int try_to_emulate(struct kprobe *p, struct pt_regs *regs) { int ret; - unsigned int insn = *p->ainsn.insn; + unsigned int insn = p->ainsn.insn[0]; + unsigned int suffix = p->ainsn.insn[1]; /* regs->nip is also adjusted if emulate_step returns 1 */ - ret = emulate_step(regs, insn, PPC_NO_SUFFIX); + ret = emulate_step(regs, insn, suffix); if (ret > 0) { /* * Once this instruction has been boosted @@ -242,7 +258,11 @@ static int try_to_emulate(struct kprobe *p, struct pt_regs *regs) * So, we should never get here... but, its still * good to catch them, just in case... */ - printk("Can't step on instruction %x\n", insn); + if (!IS_PREFIX(insn)) + printk("Can't step on instruction %x\n", insn); + else + printk("Can't step on instruction %x:%x\n", insn, + suffix); BUG(); } else { /* @@ -284,7 +304,7 @@ int kprobe_handler(struct pt_regs *regs) if (kprobe_running()) { p = get_kprobe(addr); if (p) { - kprobe_opcode_t insn = *p->ainsn.insn; + kprobe_opcode_t insn = p->ainsn.insn[0]; if (kcb->kprobe_status == KPROBE_HIT_SS && is_trap(insn)) { /* Turn off 'trace' bits */ @@ -457,9 +477,10 @@ static int trampoline_probe_handler(struct kprobe *p, struct pt_regs *regs) * the link register properly so that the subsequent 'blr' in * kretprobe_trampoline jumps back to the right instruction. * - * For nip, we should set the address to the previous instruction since - * we end up emulating it in kprobe_handler(), which increments the nip - * again. + * To keep the nip at the correct address we need to counter the + * increment that happens when we emulate the kretprobe_trampoline noop + * in kprobe_handler(). We do this by decrementing the address by the + * length of the noop which is always 4 bytes. */ regs->nip = orig_ret_address - 4; regs->link = orig_ret_address; @@ -487,12 +508,14 @@ int kprobe_post_handler(struct pt_regs *regs) { struct kprobe *cur = kprobe_running(); struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); + kprobe_opcode_t insn; if (!cur || user_mode(regs)) return 0; + insn = *cur->ainsn.insn; /* make sure we got here for instruction we have a kprobe on */ - if (((unsigned long)cur->ainsn.insn + 4) != regs->nip) + if ((unsigned long)cur->ainsn.insn + PPC_INST_LENGTH(insn) != regs->nip) return 0; if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) { @@ -501,7 +524,7 @@ int kprobe_post_handler(struct pt_regs *regs) } /* Adjust nip to after the single-stepped instruction */ - regs->nip = (unsigned long)cur->addr + 4; + regs->nip = (unsigned long)cur->addr + PPC_INST_LENGTH(insn); regs->msr |= kcb->kprobe_saved_msr; /*Restore back the original saved kprobes variables and continue. */ diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c index f908d9422557..60cf8e8485ab 100644 --- a/arch/powerpc/kernel/optprobes.c +++ b/arch/powerpc/kernel/optprobes.c @@ -27,6 +27,8 @@ (optprobe_template_op_address - optprobe_template_entry) #define TMPL_INSN_IDX \ (optprobe_template_insn - optprobe_template_entry) +#define TMPL_SUFX_IDX \ + (optprobe_template_suffix - optprobe_template_entry) #define TMPL_END_IDX \ (optprobe_template_end - optprobe_template_entry) @@ -100,8 +102,8 @@ static unsigned long can_optimize(struct kprobe *p) * and that can be emulated. */ if (!is_conditional_branch(*p->ainsn.insn) && - analyse_instr(&op, ®s, *p->ainsn.insn, - PPC_NO_SUFFIX) == 1) { + analyse_instr(&op, ®s, p->ainsn.insn[0], + p->ainsn.insn[1]) == 1) { emulate_update_regs(®s, &op); nip = regs.nip; } @@ -141,27 +143,27 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op) } /* - * emulate_step() requires insn to be emulated as - * second parameter. Load register 'r4' with the - * instruction. + * emulate_step() requires insn to be emulated as second parameter, and the + * suffix as the third parameter. Load these into registers. */ -void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr) +static void patch_imm32_load_insns(int reg, unsigned int val, + kprobe_opcode_t *addr) { - /* addis r4,0,(insn)@h */ - patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(4) | + /* addis reg,0,(insn)@h */ + patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(reg) | ((val >> 16) & 0xffff)); addr++; - /* ori r4,r4,(insn)@l */ - patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(4) | - ___PPC_RS(4) | (val & 0xffff)); + /* ori reg,reg,(insn)@l */ + patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(reg) | + ___PPC_RS(reg) | (val & 0xffff)); } /* * Generate instructions to load provided immediate 64-bit value * to register 'r3' and patch these instructions at 'addr'. */ -void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr) +static void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr) { /* lis r3,(op)@highest */ patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(3) | @@ -267,9 +269,11 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p) patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step); /* - * 3. load instruction to be emulated into relevant register, and + * 3. load instruction and suffix to be emulated into the relevant + * registers, and */ - patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX); + patch_imm32_load_insns(4, p->ainsn.insn[0], buff + TMPL_INSN_IDX); + patch_imm32_load_insns(5, p->ainsn.insn[1], buff + TMPL_SUFX_IDX); /* * 4. branch back from trampoline diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S index cf383520843f..395d1643f59d 100644 --- a/arch/powerpc/kernel/optprobes_head.S +++ b/arch/powerpc/kernel/optprobes_head.S @@ -95,6 +95,12 @@ optprobe_template_insn: nop nop + .global optprobe_template_suffix +optprobe_template_suffix: + /* Pass suffix to be emulated in r5 */ + nop + nop + .global optprobe_template_call_emulate optprobe_template_call_emulate: /* Branch to emulate_step() */
A prefixed instruction is composed of a word prefix followed by a word suffix. It does not make sense to be able to have a kprobe on the suffix of a prefixed instruction, so make this impossible. Kprobes work by replacing an instruction with a trap and saving that instruction to be single stepped out of place later. Currently there is not enough space allocated to keep a prefixed instruction for single stepping. Increase the amount of space allocated for holding the instruction copy. kprobe_post_handler() expects all instructions to be 4 bytes long which means that it does not function correctly for prefixed instructions. Add checks for prefixed instructions which will use a length of 8 bytes instead. For optprobes we normally patch in loading the instruction we put a probe on into r4 before calling emulate_step(). We now make space and patch in loading the suffix into r5 as well. Signed-off-by: Jordan Niethe <jniethe5@gmail.com> --- v3: - Base on top of https://patchwork.ozlabs.org/patch/1232619/ - Change printing format to %x:%x --- arch/powerpc/include/asm/kprobes.h | 5 ++-- arch/powerpc/kernel/kprobes.c | 43 +++++++++++++++++++++------- arch/powerpc/kernel/optprobes.c | 32 ++++++++++++--------- arch/powerpc/kernel/optprobes_head.S | 6 ++++ 4 files changed, 60 insertions(+), 26 deletions(-)