| Message ID | 20200506034050.24806-13-jniethe5@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f8faaffaa7d99028e457ef2d1dcb43a98f736938 |
| Headers | show |
| Series | Initial Prefixed Instruction support | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (1bc92fe3175eb26ff37e580c0383d7a9abe06835) |
| snowpatch_ozlabs/checkpatch | warning | total: 2 errors, 0 warnings, 0 checks, 188 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
Le 06/05/2020 à 05:40, Jordan Niethe a écrit : > Prefixed instructions will mean there are instructions of different > length. As a result dereferencing a pointer to an instruction will not > necessarily give the desired result. Introduce a function for reading > instructions from memory into the instruction data type. Shouldn't this function be used in mmu_patch_addis() in mm/nohash/8xx.c ? Christophe > > Reviewed-by: Alistair Popple <alistair@popple.id.au> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> > --- > v4: New to series > v5: - Rename read_inst() -> probe_kernel_read_inst() > - No longer modify uprobe probe type in this patch > v6: - feature-fixups.c: do_final_fixups(): Use here > - arch_prepare_kprobe(): patch_instruction(): no longer part of this > patch > - Move probe_kernel_read_inst() out of this patch > - Use in uprobes > v8: style > --- > arch/powerpc/include/asm/inst.h | 5 +++++ > arch/powerpc/kernel/kprobes.c | 6 +++--- > arch/powerpc/kernel/mce_power.c | 2 +- > arch/powerpc/kernel/optprobes.c | 4 ++-- > arch/powerpc/kernel/trace/ftrace.c | 4 ++-- > arch/powerpc/kernel/uprobes.c | 2 +- > arch/powerpc/lib/code-patching.c | 26 ++++++++++++++------------ > arch/powerpc/lib/feature-fixups.c | 4 ++-- > arch/powerpc/xmon/xmon.c | 6 +++--- > 9 files changed, 33 insertions(+), 26 deletions(-) > > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h > index 19d8bb7a1c2b..552e953bf04f 100644 > --- a/arch/powerpc/include/asm/inst.h > +++ b/arch/powerpc/include/asm/inst.h > @@ -27,6 +27,11 @@ static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x) > return ppc_inst(swab32(ppc_inst_val(x))); > } > > +static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr) > +{ > + return *ptr; > +} > + > static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y) > { > return ppc_inst_val(x) == ppc_inst_val(y); > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c > index a08ae5803622..f64312dca84f 100644 > --- a/arch/powerpc/kernel/kprobes.c > +++ b/arch/powerpc/kernel/kprobes.c > @@ -106,7 +106,7 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) > int arch_prepare_kprobe(struct kprobe *p) > { > int ret = 0; > - struct ppc_inst insn = *(struct ppc_inst *)p->addr; > + struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr); > > if ((unsigned long)p->addr & 0x03) { > printk("Attempt to register kprobe at an unaligned address\n"); > @@ -127,7 +127,7 @@ int arch_prepare_kprobe(struct kprobe *p) > if (!ret) { > memcpy(p->ainsn.insn, p->addr, > MAX_INSN_SIZE * sizeof(kprobe_opcode_t)); > - p->opcode = *p->addr; > + p->opcode = ppc_inst_val(insn); > flush_icache_range((unsigned long)p->ainsn.insn, > (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t)); > } > @@ -217,7 +217,7 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe); > static int try_to_emulate(struct kprobe *p, struct pt_regs *regs) > { > int ret; > - struct ppc_inst insn = *(struct ppc_inst *)p->ainsn.insn; > + struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->ainsn.insn); > > /* regs->nip is also adjusted if emulate_step returns 1 */ > ret = emulate_step(regs, insn); > diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c > index cd23218c60bb..45c51ba0071b 100644 > --- a/arch/powerpc/kernel/mce_power.c > +++ b/arch/powerpc/kernel/mce_power.c > @@ -374,7 +374,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr, > pfn = addr_to_pfn(regs, regs->nip); > if (pfn != ULONG_MAX) { > instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK); > - instr = *(struct ppc_inst *)(instr_addr); > + instr = ppc_inst_read((struct ppc_inst *)instr_addr); > if (!analyse_instr(&op, &tmp, instr)) { > pfn = addr_to_pfn(regs, op.ea); > *addr = op.ea; > diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c > index 5a71fef71c22..52c1ab3f85aa 100644 > --- a/arch/powerpc/kernel/optprobes.c > +++ b/arch/powerpc/kernel/optprobes.c > @@ -100,9 +100,9 @@ static unsigned long can_optimize(struct kprobe *p) > * Ensure that the instruction is not a conditional branch, > * and that can be emulated. > */ > - if (!is_conditional_branch(*(struct ppc_inst *)p->ainsn.insn) && > + if (!is_conditional_branch(ppc_inst_read((struct ppc_inst *)p->ainsn.insn)) && > analyse_instr(&op, ®s, > - *(struct ppc_inst *)p->ainsn.insn) == 1) { > + ppc_inst_read((struct ppc_inst *)p->ainsn.insn)) == 1) { > emulate_update_regs(®s, &op); > nip = regs.nip; > } > diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c > index 3117ed675735..acd5b889815f 100644 > --- a/arch/powerpc/kernel/trace/ftrace.c > +++ b/arch/powerpc/kernel/trace/ftrace.c > @@ -848,7 +848,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func) > struct ppc_inst old, new; > int ret; > > - old = *(struct ppc_inst *)&ftrace_call; > + old = ppc_inst_read((struct ppc_inst *)&ftrace_call); > new = ftrace_call_replace(ip, (unsigned long)func, 1); > ret = ftrace_modify_code(ip, old, new); > > @@ -856,7 +856,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func) > /* Also update the regs callback function */ > if (!ret) { > ip = (unsigned long)(&ftrace_regs_call); > - old = *(struct ppc_inst *)&ftrace_regs_call; > + old = ppc_inst_read((struct ppc_inst *)&ftrace_regs_call); > new = ftrace_call_replace(ip, (unsigned long)func, 1); > ret = ftrace_modify_code(ip, old, new); > } > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > index 31c870287f2b..6893d40a48c5 100644 > --- a/arch/powerpc/kernel/uprobes.c > +++ b/arch/powerpc/kernel/uprobes.c > @@ -174,7 +174,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) > * emulate_step() returns 1 if the insn was successfully emulated. > * For all other cases, we need to single-step in hardware. > */ > - ret = emulate_step(regs, auprobe->insn); > + ret = emulate_step(regs, ppc_inst_read(&auprobe->insn)); > if (ret > 0) > return true; > > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c > index 1dff9d9d6645..435fc8e9f45d 100644 > --- a/arch/powerpc/lib/code-patching.c > +++ b/arch/powerpc/lib/code-patching.c > @@ -348,9 +348,9 @@ static unsigned long branch_bform_target(const struct ppc_inst *instr) > > unsigned long branch_target(const struct ppc_inst *instr) > { > - if (instr_is_branch_iform(*instr)) > + if (instr_is_branch_iform(ppc_inst_read(instr))) > return branch_iform_target(instr); > - else if (instr_is_branch_bform(*instr)) > + else if (instr_is_branch_bform(ppc_inst_read(instr))) > return branch_bform_target(instr); > > return 0; > @@ -358,7 +358,8 @@ unsigned long branch_target(const struct ppc_inst *instr) > > int instr_is_branch_to_addr(const struct ppc_inst *instr, unsigned long addr) > { > - if (instr_is_branch_iform(*instr) || instr_is_branch_bform(*instr)) > + if (instr_is_branch_iform(ppc_inst_read(instr)) || > + instr_is_branch_bform(ppc_inst_read(instr))) > return branch_target(instr) == addr; > > return 0; > @@ -368,13 +369,14 @@ int translate_branch(struct ppc_inst *instr, const struct ppc_inst *dest, > const struct ppc_inst *src) > { > unsigned long target; > - > target = branch_target(src); > > - if (instr_is_branch_iform(*src)) > - return create_branch(instr, dest, target, ppc_inst_val(*src)); > - else if (instr_is_branch_bform(*src)) > - return create_cond_branch(instr, dest, target, ppc_inst_val(*src)); > + if (instr_is_branch_iform(ppc_inst_read(src))) > + return create_branch(instr, dest, target, > + ppc_inst_val(ppc_inst_read(src))); > + else if (instr_is_branch_bform(ppc_inst_read(src))) > + return create_cond_branch(instr, dest, target, > + ppc_inst_val(ppc_inst_read(src))); > > return 1; > } > @@ -598,7 +600,7 @@ static void __init test_translate_branch(void) > patch_instruction(q, instr); > check(instr_is_branch_to_addr(p, addr)); > check(instr_is_branch_to_addr(q, addr)); > - check(ppc_inst_equal(*q, ppc_inst(0x4a000000))); > + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x4a000000))); > > /* Maximum positive case, move x to x - 32 MB + 4 */ > p = buf + 0x2000000; > @@ -609,7 +611,7 @@ static void __init test_translate_branch(void) > patch_instruction(q, instr); > check(instr_is_branch_to_addr(p, addr)); > check(instr_is_branch_to_addr(q, addr)); > - check(ppc_inst_equal(*q, ppc_inst(0x49fffffc))); > + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x49fffffc))); > > /* Jump to x + 16 MB moved to x + 20 MB */ > p = buf; > @@ -655,7 +657,7 @@ static void __init test_translate_branch(void) > patch_instruction(q, instr); > check(instr_is_branch_to_addr(p, addr)); > check(instr_is_branch_to_addr(q, addr)); > - check(ppc_inst_equal(*q, ppc_inst(0x43ff8000))); > + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x43ff8000))); > > /* Maximum positive case, move x to x - 32 KB + 4 */ > p = buf + 0x8000; > @@ -667,7 +669,7 @@ static void __init test_translate_branch(void) > patch_instruction(q, instr); > check(instr_is_branch_to_addr(p, addr)); > check(instr_is_branch_to_addr(q, addr)); > - check(ppc_inst_equal(*q, ppc_inst(0x43ff7ffc))); > + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x43ff7ffc))); > > /* Jump to x + 12 KB moved to x + 20 KB */ > p = buf; > diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c > index fb6e8e8abf4e..c0d3ed4efb7e 100644 > --- a/arch/powerpc/lib/feature-fixups.c > +++ b/arch/powerpc/lib/feature-fixups.c > @@ -48,7 +48,7 @@ static int patch_alt_instruction(struct ppc_inst *src, struct ppc_inst *dest, > int err; > struct ppc_inst instr; > > - instr = *src; > + instr = ppc_inst_read(src); > > if (instr_is_relative_branch(*src)) { > struct ppc_inst *target = (struct ppc_inst *)branch_target(src); > @@ -403,7 +403,7 @@ static void do_final_fixups(void) > length = (__end_interrupts - _stext) / sizeof(struct ppc_inst); > > while (length--) { > - raw_patch_instruction(dest, *src); > + raw_patch_instruction(dest, ppc_inst_read(src)); > src++; > dest++; > } > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index e0132d6d24d0..68e0b05d9226 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -702,13 +702,13 @@ static int xmon_core(struct pt_regs *regs, int fromipi) > if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) == (MSR_IR|MSR_64BIT)) { > bp = at_breakpoint(regs->nip); > if (bp != NULL) { > - int stepped = emulate_step(regs, bp->instr[0]); > + int stepped = emulate_step(regs, ppc_inst_read(bp->instr)); > if (stepped == 0) { > regs->nip = (unsigned long) &bp->instr[0]; > atomic_inc(&bp->ref_count); > } else if (stepped < 0) { > printf("Couldn't single-step %s instruction\n", > - (IS_RFID(bp->instr[0])? "rfid": "mtmsrd")); > + IS_RFID(ppc_inst_read(bp->instr))? "rfid": "mtmsrd"); > } > } > } > @@ -949,7 +949,7 @@ static void remove_bpts(void) > if (mread(bp->address, &instr, 4) == 4 > && ppc_inst_equal(instr, ppc_inst(bpinstr)) > && patch_instruction( > - (struct ppc_inst *)bp->address, bp->instr[0]) != 0) > + (struct ppc_inst *)bp->address, ppc_inst_read(bp->instr)) != 0) > printf("Couldn't remove breakpoint at %lx\n", > bp->address); > } >
On Sun, May 17, 2020 at 4:39 AM Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > > Le 06/05/2020 à 05:40, Jordan Niethe a écrit : > > Prefixed instructions will mean there are instructions of different > > length. As a result dereferencing a pointer to an instruction will not > > necessarily give the desired result. Introduce a function for reading > > instructions from memory into the instruction data type. > > > Shouldn't this function be used in mmu_patch_addis() in mm/nohash/8xx.c ? > > Christophe Yes, that would be a good idea. mpe here is a fix, along with one I'll post for [PATCH v8 11/30] powerpc: Use a datatype for instructions. diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c --- a/arch/powerpc/mm/nohash/8xx.c +++ b/arch/powerpc/mm/nohash/8xx.c @@ -98,7 +98,7 @@ static void mmu_patch_cmp_limit(s32 *site, unsigned long mapped) static void mmu_patch_addis(s32 *site, long simm) { - struct ppc_inst instr = *(struct ppc_inst *)patch_site_addr(site); + struct ppc_inst instr = ppc_inst_read((struct ppc_inst *)patch_site_addr(site)); unsigned int val = ppc_inst_val(instr); val &= 0xffff0000;
Jordan Niethe <jniethe5@gmail.com> writes: > On Sun, May 17, 2020 at 4:39 AM Christophe Leroy > <christophe.leroy@csgroup.eu> wrote: >> >> Le 06/05/2020 à 05:40, Jordan Niethe a écrit : >> > Prefixed instructions will mean there are instructions of different >> > length. As a result dereferencing a pointer to an instruction will not >> > necessarily give the desired result. Introduce a function for reading >> > instructions from memory into the instruction data type. >> >> >> Shouldn't this function be used in mmu_patch_addis() in mm/nohash/8xx.c ? >> >> Christophe > Yes, that would be a good idea. > mpe here is a fix, along with one I'll > post for [PATCH v8 11/30] powerpc: Use a datatype for instructions. I didn't fold this in because I'd prefer one of you send me a patch on top of the series that converts that code to use the new type. That way it can be tested separately from this big series. cheers > diff --git a/arch/powerpc/mm/nohash/8xx.c b/arch/powerpc/mm/nohash/8xx.c > --- a/arch/powerpc/mm/nohash/8xx.c > +++ b/arch/powerpc/mm/nohash/8xx.c > @@ -98,7 +98,7 @@ static void mmu_patch_cmp_limit(s32 *site, unsigned > long mapped) > > static void mmu_patch_addis(s32 *site, long simm) > { > - struct ppc_inst instr = *(struct ppc_inst *)patch_site_addr(site); > + struct ppc_inst instr = ppc_inst_read((struct ppc_inst > *)patch_site_addr(site)); > unsigned int val = ppc_inst_val(instr); > > val &= 0xffff0000; > -- >> >> > >> > Reviewed-by: Alistair Popple <alistair@popple.id.au> >> > Signed-off-by: Jordan Niethe <jniethe5@gmail.com> >> > --- >> > v4: New to series >> > v5: - Rename read_inst() -> probe_kernel_read_inst() >> > - No longer modify uprobe probe type in this patch >> > v6: - feature-fixups.c: do_final_fixups(): Use here >> > - arch_prepare_kprobe(): patch_instruction(): no longer part of this >> > patch >> > - Move probe_kernel_read_inst() out of this patch >> > - Use in uprobes >> > v8: style >> > --- >> > arch/powerpc/include/asm/inst.h | 5 +++++ >> > arch/powerpc/kernel/kprobes.c | 6 +++--- >> > arch/powerpc/kernel/mce_power.c | 2 +- >> > arch/powerpc/kernel/optprobes.c | 4 ++-- >> > arch/powerpc/kernel/trace/ftrace.c | 4 ++-- >> > arch/powerpc/kernel/uprobes.c | 2 +- >> > arch/powerpc/lib/code-patching.c | 26 ++++++++++++++------------ >> > arch/powerpc/lib/feature-fixups.c | 4 ++-- >> > arch/powerpc/xmon/xmon.c | 6 +++--- >> > 9 files changed, 33 insertions(+), 26 deletions(-) >> > >> > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h >> > index 19d8bb7a1c2b..552e953bf04f 100644 >> > --- a/arch/powerpc/include/asm/inst.h >> > +++ b/arch/powerpc/include/asm/inst.h >> > @@ -27,6 +27,11 @@ static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x) >> > return ppc_inst(swab32(ppc_inst_val(x))); >> > } >> > >> > +static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr) >> > +{ >> > + return *ptr; >> > +} >> > + >> > static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y) >> > { >> > return ppc_inst_val(x) == ppc_inst_val(y); >> > diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c >> > index a08ae5803622..f64312dca84f 100644 >> > --- a/arch/powerpc/kernel/kprobes.c >> > +++ b/arch/powerpc/kernel/kprobes.c >> > @@ -106,7 +106,7 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) >> > int arch_prepare_kprobe(struct kprobe *p) >> > { >> > int ret = 0; >> > - struct ppc_inst insn = *(struct ppc_inst *)p->addr; >> > + struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr); >> > >> > if ((unsigned long)p->addr & 0x03) { >> > printk("Attempt to register kprobe at an unaligned address\n"); >> > @@ -127,7 +127,7 @@ int arch_prepare_kprobe(struct kprobe *p) >> > if (!ret) { >> > memcpy(p->ainsn.insn, p->addr, >> > MAX_INSN_SIZE * sizeof(kprobe_opcode_t)); >> > - p->opcode = *p->addr; >> > + p->opcode = ppc_inst_val(insn); >> > flush_icache_range((unsigned long)p->ainsn.insn, >> > (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t)); >> > } >> > @@ -217,7 +217,7 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe); >> > static int try_to_emulate(struct kprobe *p, struct pt_regs *regs) >> > { >> > int ret; >> > - struct ppc_inst insn = *(struct ppc_inst *)p->ainsn.insn; >> > + struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->ainsn.insn); >> > >> > /* regs->nip is also adjusted if emulate_step returns 1 */ >> > ret = emulate_step(regs, insn); >> > diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c >> > index cd23218c60bb..45c51ba0071b 100644 >> > --- a/arch/powerpc/kernel/mce_power.c >> > +++ b/arch/powerpc/kernel/mce_power.c >> > @@ -374,7 +374,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr, >> > pfn = addr_to_pfn(regs, regs->nip); >> > if (pfn != ULONG_MAX) { >> > instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK); >> > - instr = *(struct ppc_inst *)(instr_addr); >> > + instr = ppc_inst_read((struct ppc_inst *)instr_addr); >> > if (!analyse_instr(&op, &tmp, instr)) { >> > pfn = addr_to_pfn(regs, op.ea); >> > *addr = op.ea; >> > diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c >> > index 5a71fef71c22..52c1ab3f85aa 100644 >> > --- a/arch/powerpc/kernel/optprobes.c >> > +++ b/arch/powerpc/kernel/optprobes.c >> > @@ -100,9 +100,9 @@ static unsigned long can_optimize(struct kprobe *p) >> > * Ensure that the instruction is not a conditional branch, >> > * and that can be emulated. >> > */ >> > - if (!is_conditional_branch(*(struct ppc_inst *)p->ainsn.insn) && >> > + if (!is_conditional_branch(ppc_inst_read((struct ppc_inst *)p->ainsn.insn)) && >> > analyse_instr(&op, ®s, >> > - *(struct ppc_inst *)p->ainsn.insn) == 1) { >> > + ppc_inst_read((struct ppc_inst *)p->ainsn.insn)) == 1) { >> > emulate_update_regs(®s, &op); >> > nip = regs.nip; >> > } >> > diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c >> > index 3117ed675735..acd5b889815f 100644 >> > --- a/arch/powerpc/kernel/trace/ftrace.c >> > +++ b/arch/powerpc/kernel/trace/ftrace.c >> > @@ -848,7 +848,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func) >> > struct ppc_inst old, new; >> > int ret; >> > >> > - old = *(struct ppc_inst *)&ftrace_call; >> > + old = ppc_inst_read((struct ppc_inst *)&ftrace_call); >> > new = ftrace_call_replace(ip, (unsigned long)func, 1); >> > ret = ftrace_modify_code(ip, old, new); >> > >> > @@ -856,7 +856,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func) >> > /* Also update the regs callback function */ >> > if (!ret) { >> > ip = (unsigned long)(&ftrace_regs_call); >> > - old = *(struct ppc_inst *)&ftrace_regs_call; >> > + old = ppc_inst_read((struct ppc_inst *)&ftrace_regs_call); >> > new = ftrace_call_replace(ip, (unsigned long)func, 1); >> > ret = ftrace_modify_code(ip, old, new); >> > } >> > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c >> > index 31c870287f2b..6893d40a48c5 100644 >> > --- a/arch/powerpc/kernel/uprobes.c >> > +++ b/arch/powerpc/kernel/uprobes.c >> > @@ -174,7 +174,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) >> > * emulate_step() returns 1 if the insn was successfully emulated. >> > * For all other cases, we need to single-step in hardware. >> > */ >> > - ret = emulate_step(regs, auprobe->insn); >> > + ret = emulate_step(regs, ppc_inst_read(&auprobe->insn)); >> > if (ret > 0) >> > return true; >> > >> > diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c >> > index 1dff9d9d6645..435fc8e9f45d 100644 >> > --- a/arch/powerpc/lib/code-patching.c >> > +++ b/arch/powerpc/lib/code-patching.c >> > @@ -348,9 +348,9 @@ static unsigned long branch_bform_target(const struct ppc_inst *instr) >> > >> > unsigned long branch_target(const struct ppc_inst *instr) >> > { >> > - if (instr_is_branch_iform(*instr)) >> > + if (instr_is_branch_iform(ppc_inst_read(instr))) >> > return branch_iform_target(instr); >> > - else if (instr_is_branch_bform(*instr)) >> > + else if (instr_is_branch_bform(ppc_inst_read(instr))) >> > return branch_bform_target(instr); >> > >> > return 0; >> > @@ -358,7 +358,8 @@ unsigned long branch_target(const struct ppc_inst *instr) >> > >> > int instr_is_branch_to_addr(const struct ppc_inst *instr, unsigned long addr) >> > { >> > - if (instr_is_branch_iform(*instr) || instr_is_branch_bform(*instr)) >> > + if (instr_is_branch_iform(ppc_inst_read(instr)) || >> > + instr_is_branch_bform(ppc_inst_read(instr))) >> > return branch_target(instr) == addr; >> > >> > return 0; >> > @@ -368,13 +369,14 @@ int translate_branch(struct ppc_inst *instr, const struct ppc_inst *dest, >> > const struct ppc_inst *src) >> > { >> > unsigned long target; >> > - >> > target = branch_target(src); >> > >> > - if (instr_is_branch_iform(*src)) >> > - return create_branch(instr, dest, target, ppc_inst_val(*src)); >> > - else if (instr_is_branch_bform(*src)) >> > - return create_cond_branch(instr, dest, target, ppc_inst_val(*src)); >> > + if (instr_is_branch_iform(ppc_inst_read(src))) >> > + return create_branch(instr, dest, target, >> > + ppc_inst_val(ppc_inst_read(src))); >> > + else if (instr_is_branch_bform(ppc_inst_read(src))) >> > + return create_cond_branch(instr, dest, target, >> > + ppc_inst_val(ppc_inst_read(src))); >> > >> > return 1; >> > } >> > @@ -598,7 +600,7 @@ static void __init test_translate_branch(void) >> > patch_instruction(q, instr); >> > check(instr_is_branch_to_addr(p, addr)); >> > check(instr_is_branch_to_addr(q, addr)); >> > - check(ppc_inst_equal(*q, ppc_inst(0x4a000000))); >> > + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x4a000000))); >> > >> > /* Maximum positive case, move x to x - 32 MB + 4 */ >> > p = buf + 0x2000000; >> > @@ -609,7 +611,7 @@ static void __init test_translate_branch(void) >> > patch_instruction(q, instr); >> > check(instr_is_branch_to_addr(p, addr)); >> > check(instr_is_branch_to_addr(q, addr)); >> > - check(ppc_inst_equal(*q, ppc_inst(0x49fffffc))); >> > + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x49fffffc))); >> > >> > /* Jump to x + 16 MB moved to x + 20 MB */ >> > p = buf; >> > @@ -655,7 +657,7 @@ static void __init test_translate_branch(void) >> > patch_instruction(q, instr); >> > check(instr_is_branch_to_addr(p, addr)); >> > check(instr_is_branch_to_addr(q, addr)); >> > - check(ppc_inst_equal(*q, ppc_inst(0x43ff8000))); >> > + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x43ff8000))); >> > >> > /* Maximum positive case, move x to x - 32 KB + 4 */ >> > p = buf + 0x8000; >> > @@ -667,7 +669,7 @@ static void __init test_translate_branch(void) >> > patch_instruction(q, instr); >> > check(instr_is_branch_to_addr(p, addr)); >> > check(instr_is_branch_to_addr(q, addr)); >> > - check(ppc_inst_equal(*q, ppc_inst(0x43ff7ffc))); >> > + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x43ff7ffc))); >> > >> > /* Jump to x + 12 KB moved to x + 20 KB */ >> > p = buf; >> > diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c >> > index fb6e8e8abf4e..c0d3ed4efb7e 100644 >> > --- a/arch/powerpc/lib/feature-fixups.c >> > +++ b/arch/powerpc/lib/feature-fixups.c >> > @@ -48,7 +48,7 @@ static int patch_alt_instruction(struct ppc_inst *src, struct ppc_inst *dest, >> > int err; >> > struct ppc_inst instr; >> > >> > - instr = *src; >> > + instr = ppc_inst_read(src); >> > >> > if (instr_is_relative_branch(*src)) { >> > struct ppc_inst *target = (struct ppc_inst *)branch_target(src); >> > @@ -403,7 +403,7 @@ static void do_final_fixups(void) >> > length = (__end_interrupts - _stext) / sizeof(struct ppc_inst); >> > >> > while (length--) { >> > - raw_patch_instruction(dest, *src); >> > + raw_patch_instruction(dest, ppc_inst_read(src)); >> > src++; >> > dest++; >> > } >> > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c >> > index e0132d6d24d0..68e0b05d9226 100644 >> > --- a/arch/powerpc/xmon/xmon.c >> > +++ b/arch/powerpc/xmon/xmon.c >> > @@ -702,13 +702,13 @@ static int xmon_core(struct pt_regs *regs, int fromipi) >> > if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) == (MSR_IR|MSR_64BIT)) { >> > bp = at_breakpoint(regs->nip); >> > if (bp != NULL) { >> > - int stepped = emulate_step(regs, bp->instr[0]); >> > + int stepped = emulate_step(regs, ppc_inst_read(bp->instr)); >> > if (stepped == 0) { >> > regs->nip = (unsigned long) &bp->instr[0]; >> > atomic_inc(&bp->ref_count); >> > } else if (stepped < 0) { >> > printf("Couldn't single-step %s instruction\n", >> > - (IS_RFID(bp->instr[0])? "rfid": "mtmsrd")); >> > + IS_RFID(ppc_inst_read(bp->instr))? "rfid": "mtmsrd"); >> > } >> > } >> > } >> > @@ -949,7 +949,7 @@ static void remove_bpts(void) >> > if (mread(bp->address, &instr, 4) == 4 >> > && ppc_inst_equal(instr, ppc_inst(bpinstr)) >> > && patch_instruction( >> > - (struct ppc_inst *)bp->address, bp->instr[0]) != 0) >> > + (struct ppc_inst *)bp->address, ppc_inst_read(bp->instr)) != 0) >> > printf("Couldn't remove breakpoint at %lx\n", >> > bp->address); >> > } >> >
Le 19/05/2020 à 06:05, Michael Ellerman a écrit : > Jordan Niethe <jniethe5@gmail.com> writes: >> On Sun, May 17, 2020 at 4:39 AM Christophe Leroy >> <christophe.leroy@csgroup.eu> wrote: >>> >>> Le 06/05/2020 à 05:40, Jordan Niethe a écrit : >>>> Prefixed instructions will mean there are instructions of different >>>> length. As a result dereferencing a pointer to an instruction will not >>>> necessarily give the desired result. Introduce a function for reading >>>> instructions from memory into the instruction data type. >>> >>> >>> Shouldn't this function be used in mmu_patch_addis() in mm/nohash/8xx.c ? >>> >>> Christophe > >> Yes, that would be a good idea. > >> mpe here is a fix, along with one I'll >> post for [PATCH v8 11/30] powerpc: Use a datatype for instructions. > > I didn't fold this in because I'd prefer one of you send me a patch on > top of the series that converts that code to use the new type. > > That way it can be tested separately from this big series. > All this code is going away with the series implementing the use of hugepages for kernel mappings on 8xx (https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=176094) that I hope will go in 5.8, so there is no point in sending a patch to change that I guess. Is there anything special I need to do to secure the merging of that series in 5.8 ? Christophe
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 19/05/2020 à 06:05, Michael Ellerman a écrit : >> Jordan Niethe <jniethe5@gmail.com> writes: >>> On Sun, May 17, 2020 at 4:39 AM Christophe Leroy >>> <christophe.leroy@csgroup.eu> wrote: >>>> >>>> Le 06/05/2020 à 05:40, Jordan Niethe a écrit : >>>>> Prefixed instructions will mean there are instructions of different >>>>> length. As a result dereferencing a pointer to an instruction will not >>>>> necessarily give the desired result. Introduce a function for reading >>>>> instructions from memory into the instruction data type. >>>> >>>> Shouldn't this function be used in mmu_patch_addis() in mm/nohash/8xx.c ? >> >>> Yes, that would be a good idea. >> >>> mpe here is a fix, along with one I'll >>> post for [PATCH v8 11/30] powerpc: Use a datatype for instructions. >> >> I didn't fold this in because I'd prefer one of you send me a patch on >> top of the series that converts that code to use the new type. >> >> That way it can be tested separately from this big series. >> > > All this code is going away with the series implementing the use of > hugepages for kernel mappings on 8xx > (https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=176094) > that I hope will go in 5.8, so there is no point in sending a patch to > change that I guess. OK. > Is there anything special I need to do to secure the merging of that > series in 5.8 ? Review it for me :P As long as it is only touching 8xx I'm happy to defer to you. So I guess any parts that affect other platforms will be where I need to spend more effort reviewing it. I'll try and get it into my test branch today and get some testing done on it. cheers
diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h index 19d8bb7a1c2b..552e953bf04f 100644 --- a/arch/powerpc/include/asm/inst.h +++ b/arch/powerpc/include/asm/inst.h @@ -27,6 +27,11 @@ static inline struct ppc_inst ppc_inst_swab(struct ppc_inst x) return ppc_inst(swab32(ppc_inst_val(x))); } +static inline struct ppc_inst ppc_inst_read(const struct ppc_inst *ptr) +{ + return *ptr; +} + static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y) { return ppc_inst_val(x) == ppc_inst_val(y); diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c index a08ae5803622..f64312dca84f 100644 --- a/arch/powerpc/kernel/kprobes.c +++ b/arch/powerpc/kernel/kprobes.c @@ -106,7 +106,7 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset) int arch_prepare_kprobe(struct kprobe *p) { int ret = 0; - struct ppc_inst insn = *(struct ppc_inst *)p->addr; + struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->addr); if ((unsigned long)p->addr & 0x03) { printk("Attempt to register kprobe at an unaligned address\n"); @@ -127,7 +127,7 @@ int arch_prepare_kprobe(struct kprobe *p) if (!ret) { memcpy(p->ainsn.insn, p->addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t)); - p->opcode = *p->addr; + p->opcode = ppc_inst_val(insn); flush_icache_range((unsigned long)p->ainsn.insn, (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t)); } @@ -217,7 +217,7 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe); static int try_to_emulate(struct kprobe *p, struct pt_regs *regs) { int ret; - struct ppc_inst insn = *(struct ppc_inst *)p->ainsn.insn; + struct ppc_inst insn = ppc_inst_read((struct ppc_inst *)p->ainsn.insn); /* regs->nip is also adjusted if emulate_step returns 1 */ ret = emulate_step(regs, insn); diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c index cd23218c60bb..45c51ba0071b 100644 --- a/arch/powerpc/kernel/mce_power.c +++ b/arch/powerpc/kernel/mce_power.c @@ -374,7 +374,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs, uint64_t *addr, pfn = addr_to_pfn(regs, regs->nip); if (pfn != ULONG_MAX) { instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK); - instr = *(struct ppc_inst *)(instr_addr); + instr = ppc_inst_read((struct ppc_inst *)instr_addr); if (!analyse_instr(&op, &tmp, instr)) { pfn = addr_to_pfn(regs, op.ea); *addr = op.ea; diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c index 5a71fef71c22..52c1ab3f85aa 100644 --- a/arch/powerpc/kernel/optprobes.c +++ b/arch/powerpc/kernel/optprobes.c @@ -100,9 +100,9 @@ static unsigned long can_optimize(struct kprobe *p) * Ensure that the instruction is not a conditional branch, * and that can be emulated. */ - if (!is_conditional_branch(*(struct ppc_inst *)p->ainsn.insn) && + if (!is_conditional_branch(ppc_inst_read((struct ppc_inst *)p->ainsn.insn)) && analyse_instr(&op, ®s, - *(struct ppc_inst *)p->ainsn.insn) == 1) { + ppc_inst_read((struct ppc_inst *)p->ainsn.insn)) == 1) { emulate_update_regs(®s, &op); nip = regs.nip; } diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c index 3117ed675735..acd5b889815f 100644 --- a/arch/powerpc/kernel/trace/ftrace.c +++ b/arch/powerpc/kernel/trace/ftrace.c @@ -848,7 +848,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func) struct ppc_inst old, new; int ret; - old = *(struct ppc_inst *)&ftrace_call; + old = ppc_inst_read((struct ppc_inst *)&ftrace_call); new = ftrace_call_replace(ip, (unsigned long)func, 1); ret = ftrace_modify_code(ip, old, new); @@ -856,7 +856,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func) /* Also update the regs callback function */ if (!ret) { ip = (unsigned long)(&ftrace_regs_call); - old = *(struct ppc_inst *)&ftrace_regs_call; + old = ppc_inst_read((struct ppc_inst *)&ftrace_regs_call); new = ftrace_call_replace(ip, (unsigned long)func, 1); ret = ftrace_modify_code(ip, old, new); } diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c index 31c870287f2b..6893d40a48c5 100644 --- a/arch/powerpc/kernel/uprobes.c +++ b/arch/powerpc/kernel/uprobes.c @@ -174,7 +174,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) * emulate_step() returns 1 if the insn was successfully emulated. * For all other cases, we need to single-step in hardware. */ - ret = emulate_step(regs, auprobe->insn); + ret = emulate_step(regs, ppc_inst_read(&auprobe->insn)); if (ret > 0) return true; diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index 1dff9d9d6645..435fc8e9f45d 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -348,9 +348,9 @@ static unsigned long branch_bform_target(const struct ppc_inst *instr) unsigned long branch_target(const struct ppc_inst *instr) { - if (instr_is_branch_iform(*instr)) + if (instr_is_branch_iform(ppc_inst_read(instr))) return branch_iform_target(instr); - else if (instr_is_branch_bform(*instr)) + else if (instr_is_branch_bform(ppc_inst_read(instr))) return branch_bform_target(instr); return 0; @@ -358,7 +358,8 @@ unsigned long branch_target(const struct ppc_inst *instr) int instr_is_branch_to_addr(const struct ppc_inst *instr, unsigned long addr) { - if (instr_is_branch_iform(*instr) || instr_is_branch_bform(*instr)) + if (instr_is_branch_iform(ppc_inst_read(instr)) || + instr_is_branch_bform(ppc_inst_read(instr))) return branch_target(instr) == addr; return 0; @@ -368,13 +369,14 @@ int translate_branch(struct ppc_inst *instr, const struct ppc_inst *dest, const struct ppc_inst *src) { unsigned long target; - target = branch_target(src); - if (instr_is_branch_iform(*src)) - return create_branch(instr, dest, target, ppc_inst_val(*src)); - else if (instr_is_branch_bform(*src)) - return create_cond_branch(instr, dest, target, ppc_inst_val(*src)); + if (instr_is_branch_iform(ppc_inst_read(src))) + return create_branch(instr, dest, target, + ppc_inst_val(ppc_inst_read(src))); + else if (instr_is_branch_bform(ppc_inst_read(src))) + return create_cond_branch(instr, dest, target, + ppc_inst_val(ppc_inst_read(src))); return 1; } @@ -598,7 +600,7 @@ static void __init test_translate_branch(void) patch_instruction(q, instr); check(instr_is_branch_to_addr(p, addr)); check(instr_is_branch_to_addr(q, addr)); - check(ppc_inst_equal(*q, ppc_inst(0x4a000000))); + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x4a000000))); /* Maximum positive case, move x to x - 32 MB + 4 */ p = buf + 0x2000000; @@ -609,7 +611,7 @@ static void __init test_translate_branch(void) patch_instruction(q, instr); check(instr_is_branch_to_addr(p, addr)); check(instr_is_branch_to_addr(q, addr)); - check(ppc_inst_equal(*q, ppc_inst(0x49fffffc))); + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x49fffffc))); /* Jump to x + 16 MB moved to x + 20 MB */ p = buf; @@ -655,7 +657,7 @@ static void __init test_translate_branch(void) patch_instruction(q, instr); check(instr_is_branch_to_addr(p, addr)); check(instr_is_branch_to_addr(q, addr)); - check(ppc_inst_equal(*q, ppc_inst(0x43ff8000))); + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x43ff8000))); /* Maximum positive case, move x to x - 32 KB + 4 */ p = buf + 0x8000; @@ -667,7 +669,7 @@ static void __init test_translate_branch(void) patch_instruction(q, instr); check(instr_is_branch_to_addr(p, addr)); check(instr_is_branch_to_addr(q, addr)); - check(ppc_inst_equal(*q, ppc_inst(0x43ff7ffc))); + check(ppc_inst_equal(ppc_inst_read(q), ppc_inst(0x43ff7ffc))); /* Jump to x + 12 KB moved to x + 20 KB */ p = buf; diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c index fb6e8e8abf4e..c0d3ed4efb7e 100644 --- a/arch/powerpc/lib/feature-fixups.c +++ b/arch/powerpc/lib/feature-fixups.c @@ -48,7 +48,7 @@ static int patch_alt_instruction(struct ppc_inst *src, struct ppc_inst *dest, int err; struct ppc_inst instr; - instr = *src; + instr = ppc_inst_read(src); if (instr_is_relative_branch(*src)) { struct ppc_inst *target = (struct ppc_inst *)branch_target(src); @@ -403,7 +403,7 @@ static void do_final_fixups(void) length = (__end_interrupts - _stext) / sizeof(struct ppc_inst); while (length--) { - raw_patch_instruction(dest, *src); + raw_patch_instruction(dest, ppc_inst_read(src)); src++; dest++; } diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c index e0132d6d24d0..68e0b05d9226 100644 --- a/arch/powerpc/xmon/xmon.c +++ b/arch/powerpc/xmon/xmon.c @@ -702,13 +702,13 @@ static int xmon_core(struct pt_regs *regs, int fromipi) if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) == (MSR_IR|MSR_64BIT)) { bp = at_breakpoint(regs->nip); if (bp != NULL) { - int stepped = emulate_step(regs, bp->instr[0]); + int stepped = emulate_step(regs, ppc_inst_read(bp->instr)); if (stepped == 0) { regs->nip = (unsigned long) &bp->instr[0]; atomic_inc(&bp->ref_count); } else if (stepped < 0) { printf("Couldn't single-step %s instruction\n", - (IS_RFID(bp->instr[0])? "rfid": "mtmsrd")); + IS_RFID(ppc_inst_read(bp->instr))? "rfid": "mtmsrd"); } } } @@ -949,7 +949,7 @@ static void remove_bpts(void) if (mread(bp->address, &instr, 4) == 4 && ppc_inst_equal(instr, ppc_inst(bpinstr)) && patch_instruction( - (struct ppc_inst *)bp->address, bp->instr[0]) != 0) + (struct ppc_inst *)bp->address, ppc_inst_read(bp->instr)) != 0) printf("Couldn't remove breakpoint at %lx\n", bp->address); }