Re: [PATCH] powerpc: Add ppc_inst_next()

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 20/05/2020 à 14:21, Jordan Niethe a écrit :
> On Wed, May 20, 2020 at 9:44 PM Michael Ellerman <mpe@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 <mpe@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.

Yes I agree

> 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);
> +}

Because as Michael explains, the instruction to be skipped might not yet 
be at the pointed memory location (for instance in insert_bpts() )

> 
>> +
>>   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.

And reading 8 bytes might trigger a page fault if we are reading the 
very last non prefixed instruction of the last page.

>>                  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
>>

Christophe