Re: [PATCH v4 09/16] powerpc: Use a function for reading instructions

Jordan Niethe's on March 23, 2020 8:09 pm:

On Mon, Mar 23, 2020 at 7:03 PM Nicholas Piggin [off-list ref] wrote:

quoted

Jordan Niethe's on March 20, 2020 3:18 pm:

quoted

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.

Signed-off-by: Jordan Niethe <redacted>
---
v4: New to series
---
 arch/powerpc/include/asm/uprobes.h |  4 ++--
 arch/powerpc/kernel/kprobes.c      |  8 ++++----
 arch/powerpc/kernel/mce_power.c    |  2 +-
 arch/powerpc/kernel/optprobes.c    |  6 +++---
 arch/powerpc/kernel/trace/ftrace.c | 33 +++++++++++++++++++-----------
 arch/powerpc/kernel/uprobes.c      |  2 +-
 arch/powerpc/lib/code-patching.c   | 22 ++++++++++----------
 arch/powerpc/lib/feature-fixups.c  |  6 +++---
 arch/powerpc/xmon/xmon.c           |  6 +++---
 9 files changed, 49 insertions(+), 40 deletions(-)

diff --git a/arch/powerpc/include/asm/uprobes.h b/arch/powerpc/include/asm/uprobes.h
index 2bbdf27d09b5..fff3c5fc90b5 100644
--- a/arch/powerpc/include/asm/uprobes.h
+++ b/arch/powerpc/include/asm/uprobes.h

@@ -23,8 +23,8 @@ typedef ppc_opcode_t uprobe_opcode_t;

 struct arch_uprobe {
      union {
-             u32     insn;
-             u32     ixol;
+             u8      insn[MAX_UINSN_BYTES];
+             u8      ixol[MAX_UINSN_BYTES];
      };
 };

Hmm. I wonder if this should be a different patch. Not sure if raw
bytes is a good idea here. ppc probes also has a ppc_opcode_t, maybe
could be replaced with ppc_insn_t and used here instead?

You are right this should not really be in this patch. I felt bytes
made sense as we have  MAX_UINSN_BYTES, which could be updated once
prefixed instructions were introduced.

Okay.

By replace do you mean define uprobe_opcode_t as ppc_inst_t instead of
ppc_opcode_t? That will not really work with the arch indep code in
places like:

bool __weak is_swbp_insn(uprobe_opcode_t *insn)
{
    return *insn == UPROBE_SWBP_INSN;
}

Ah, yeah I did mean that, you probably told me that already.

Or do you mean something like this:
  struct arch_uprobe {
       union {
 -             u32     insn;
 -             u32     ixol;
 +             pcc_inst_t     insn;
 +             ppc_inst_t     ixol;
       };
 };

I didn't mean that, but it would be nicer than the change you've got,
if it works.

quoted

Also can't find where you define ppc_inst_read.

quoted

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 7614a9f537fd..ad451205f268 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c

@@ -41,6 +41,12 @@
 #define      NUM_FTRACE_TRAMPS       8
 static unsigned long ftrace_tramps[NUM_FTRACE_TRAMPS];

+static long
+read_inst(ppc_inst *inst, const void *src)
+{
+     return probe_kernel_read((void *)inst, src, MCOUNT_INSN_SIZE);
+}

Humbly suggest probe_kernel_inst_read.

The other probe_kernel_*  functions were from generic code so I
thought it might be misleading to call it that.

It's probably not too bad, you could add a __ or ppc_ prefix if
it would help?

Thanks,
Nick