Jordan Niethe's on March 23, 2020 8:09 pm:
> On Mon, Mar 23, 2020 at 7:03 PM Nicholas Piggin <npig...@gmail.com> wrote:
>>
>> Jordan Niethe's on March 20, 2020 3:18 pm:
>> > 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 <jniet...@gmail.com>
>> > ---
>> > 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.
>> Also can't find where you define ppc_inst_read.
>>
>> > 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
