Excerpts from Nicholas Piggin's message of May 24, 2020 9:56 am: > Excerpts from Michael Ellerman's message of May 22, 2020 11:33 pm: >> 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. >> >> Although it would seem natural for the value to be passed by value, >> that makes it too easy to write a loop that will read off the end of a >> page, eg: >> >> for (; src < end; src = ppc_inst_next(src, *src), >> dest = ppc_inst_next(dest, *dest)) >> >> As noticed by Christophe and Jordan, if end is the exact end of a >> page, and the next page is not mapped, this will fault, because *dest >> will read 8 bytes, 4 bytes into the next page. >> >> So value is passed by reference, so the helper can be careful to use >> ppc_inst_read() on it. >> >> Signed-off-by: Michael Ellerman <m...@ellerman.id.au> >> --- >> arch/powerpc/include/asm/inst.h | 13 +++++++++++++ >> arch/powerpc/kernel/uprobes.c | 2 +- >> arch/powerpc/lib/feature-fixups.c | 15 ++++++++------- >> arch/powerpc/xmon/xmon.c | 2 +- >> 4 files changed, 23 insertions(+), 9 deletions(-) >> >> v2: Pass the value as a pointer and use ppc_inst_read() on it. >> >> diff --git a/arch/powerpc/include/asm/inst.h >> b/arch/powerpc/include/asm/inst.h >> index d82e0c99cfa1..5b756ba77ed2 100644 >> --- a/arch/powerpc/include/asm/inst.h >> +++ b/arch/powerpc/include/asm/inst.h >> @@ -100,6 +100,19 @@ 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) >> +{ >> + struct ppc_inst tmp; >> + >> + tmp = ppc_inst_read(value); >> + >> + return location + ppc_inst_len(tmp); >> +} >> + >> 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..d200e7df7167 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; > > AFAIKS except for here, there is no need for the void *location arg. > > I would prefer to drop that and keep this unchanged (it's a slightly > special case anyway).
Ooops, I didn't read the last thread. I don't think insert_bpts needs it though, only this case. Anyway it's a minor nitpick so if it's already been considered, fine. Thanks, Nick