All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] powerpc: Add ppc_inst_next()
@ 2020-05-20 11:44 Michael Ellerman
  2020-05-20 12:21 ` Jordan Niethe
  0 siblings, 1 reply; 3+ messages in thread
From: Michael Ellerman @ 2020-05-20 11:44 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: christophe.leroy, jniethe5

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc: Add ppc_inst_next()
  2020-05-20 11:44 [PATCH] powerpc: Add ppc_inst_next() Michael Ellerman
@ 2020-05-20 12:21 ` Jordan Niethe
  2020-05-20 12:30   ` Christophe Leroy
  0 siblings, 1 reply; 3+ messages in thread
From: Jordan Niethe @ 2020-05-20 12:21 UTC (permalink / raw)
  To: Michael Ellerman; +Cc: Christophe Leroy, linuxppc-dev

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] powerpc: Add ppc_inst_next()
  2020-05-20 12:21 ` Jordan Niethe
@ 2020-05-20 12:30   ` Christophe Leroy
  0 siblings, 0 replies; 3+ messages in thread
From: Christophe Leroy @ 2020-05-20 12:30 UTC (permalink / raw)
  To: Jordan Niethe, Michael Ellerman; +Cc: Christophe Leroy, linuxppc-dev



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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-20 12:34 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 11:44 [PATCH] powerpc: Add ppc_inst_next() Michael Ellerman
2020-05-20 12:21 ` Jordan Niethe
2020-05-20 12:30   ` Christophe Leroy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.