On Wed, May 20, 2020 at 9:44 PM Michael Ellerman <m...@ellerman.id.au> wrote: > > In a few places we want to calculate the address of the next > instruction. Previously that was simple, we just added 4 bytes, or if > using a u32 * we incremented that pointer by 1. > > But prefixed instructions make it more complicated, we need to advance > by either 4 or 8 bytes depending on the actual instruction. We also > can't do pointer arithmetic using struct ppc_inst, because it is > always 8 bytes in size on 64-bit, even though we might only need to > advance by 4 bytes. > > So add a ppc_inst_next() helper which calculates the location of the > next instruction, if the given instruction was located at the given > address. Note the instruction doesn't need to actually be at the > address in memory. > > Convert several locations to use it. > > Signed-off-by: Michael Ellerman <m...@ellerman.id.au> > --- > arch/powerpc/include/asm/inst.h | 9 +++++++++ > arch/powerpc/kernel/uprobes.c | 2 +- > arch/powerpc/lib/feature-fixups.c | 10 +++++----- > arch/powerpc/xmon/xmon.c | 2 +- > 4 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h > index d82e0c99cfa1..7d5ee1309b92 100644 > --- a/arch/powerpc/include/asm/inst.h > +++ b/arch/powerpc/include/asm/inst.h > @@ -100,6 +100,15 @@ static inline int ppc_inst_len(struct ppc_inst x) > return ppc_inst_prefixed(x) ? 8 : 4; > } > > +/* > + * Return the address of the next instruction, if the instruction @value was > + * located at @location. > + */ > +static inline struct ppc_inst *ppc_inst_next(void *location, struct ppc_inst > value) > +{ > + return location + ppc_inst_len(value); > +} I think this is a good idea. I tried something similar in the initial post for an instruction type. I had: +#define PPC_INST_NEXT(ptr) ((ptr) += PPC_INST_LEN(DEREF_PPC_INST_PTR((ptr)))) but how you've got it is much more clear/usable. I wonder why not +static inline struct ppc_inst *ppc_inst_next(void *location) +{ + return location + ppc_inst_len(ppc_inst_read((struct ppc_inst *)location); +}
> + > int probe_user_read_inst(struct ppc_inst *inst, > struct ppc_inst __user *nip); > > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > index 83e883e1a42d..683ba76919a7 100644 > --- a/arch/powerpc/kernel/uprobes.c > +++ b/arch/powerpc/kernel/uprobes.c > @@ -112,7 +112,7 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, > struct pt_regs *regs) > * support doesn't exist and have to fix-up the next instruction > * to be executed. > */ > - regs->nip = utask->vaddr + > ppc_inst_len(ppc_inst_read(&auprobe->insn)); > + regs->nip = (unsigned long)ppc_inst_next((void *)utask->vaddr, > auprobe->insn); > > user_disable_single_step(current); > return 0; > diff --git a/arch/powerpc/lib/feature-fixups.c > b/arch/powerpc/lib/feature-fixups.c > index 80f320c2e189..0ad01eebf112 100644 > --- a/arch/powerpc/lib/feature-fixups.c > +++ b/arch/powerpc/lib/feature-fixups.c > @@ -84,13 +84,13 @@ static int patch_feature_section(unsigned long value, > struct fixup_entry *fcur) > src = alt_start; > dest = start; > > - for (; src < alt_end; src = (void *)src + > ppc_inst_len(ppc_inst_read(src)), > - (dest = (void *)dest + ppc_inst_len(ppc_inst_read(dest)))) { > + for (; src < alt_end; src = ppc_inst_next(src, *src), > + dest = ppc_inst_next(dest, *dest)) { The reason to maybe use ppc_inst_read() in the helper instead of just *dest would be we don't always need to read 8 bytes. > if (patch_alt_instruction(src, dest, alt_start, alt_end)) > return 1; > } > > - for (; dest < end; dest = (void *)dest + > ppc_inst_len(ppc_inst(PPC_INST_NOP))) > + for (; dest < end; dest = ppc_inst_next(dest, ppc_inst(PPC_INST_NOP))) But then you wouldn't be able to do this as easily I guess. > raw_patch_instruction(dest, ppc_inst(PPC_INST_NOP)); > > return 0; > @@ -405,8 +405,8 @@ static void do_final_fixups(void) > while (src < end) { > inst = ppc_inst_read(src); > raw_patch_instruction(dest, inst); > - src = (void *)src + ppc_inst_len(inst); > - dest = (void *)dest + ppc_inst_len(inst); > + src = ppc_inst_next(src, *src); > + dest = ppc_inst_next(dest, *dest); > } > #endif > } > diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c > index fb135f2cd6b0..aa123f56b7d4 100644 > --- a/arch/powerpc/xmon/xmon.c > +++ b/arch/powerpc/xmon/xmon.c > @@ -939,7 +939,7 @@ static void insert_bpts(void) > } > > patch_instruction(bp->instr, instr); > - patch_instruction((void *)bp->instr + ppc_inst_len(instr), > + patch_instruction(ppc_inst_next(bp->instr, instr), > ppc_inst(bpinstr)); > if (bp->enabled & BP_CIABR) > continue; > -- > 2.25.1 >