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 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
>> > ---
>> > 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
Re: [PATCH v4 09/16] powerpc: Use a function for reading instructions
On Mon, Mar 23, 2020 at 7:03 PM Nicholas Piggin 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
> > ---
> > 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.
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;
}
Or do you mean something like this:
struct arch_uprobe {
union {
- u32 insn;
- u32 ixol;
+ pcc_inst_t insn;
+ ppc_inst_t ixol;
};
};
>
> 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.
>
> Thanks,
> Nick
>
Re: [PATCH v4 09/16] powerpc: Use a function for reading instructions
On Mon, 2020-03-23 at 18:00 +1000, Nicholas Piggin 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
> > ---
> > 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?
>
> Also can't find where you define ppc_inst_read.
Nick, ppc_inst_read and macro PPC_INST you have asked in patch 4 are defined in
asm/inst.h with patch 3 (powerpc: Use a datatype for instructions)
-- Bala
>
> > 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 @@
> > #defineNUM_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.
>
> Thanks,
> Nick
>
Re: [PATCH v4 09/16] powerpc: Use a function for reading instructions
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
> ---
> 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?
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.
Thanks,
Nick
[PATCH v4 09/16] powerpc: Use a function for reading instructions
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
---
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];
};
};
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 0c600b6e4ead..f142d11d7b48 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -117,7 +117,7 @@ void *alloc_insn_page(void)
int arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
- kprobe_opcode_t insn = *p->addr;
+ ppc_inst insn = ppc_inst_read(p->addr);
if ((unsigned long)p->addr & 0x03) {
printk("Attempt to register kprobe at an unaligned address\n");
@@ -136,8 +136,8 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- patch_instruction(p->ainsn.insn, *p->addr);
- p->opcode = *p->addr;
+ patch_instruction(p->ainsn.insn, insn);
+ p->opcode = ppc_inst_word(insn);
}
p->ainsn.boostable = 0;
@@ -225,7 +225,7 @@ NOKPROBE_SYMBOL(arch_prepare_kretprobe);
static int try_to_emulate(struct kprobe *p, struct pt_regs *regs)
{
int ret;
- ppc_inst insn = *p->ainsn.insn;
+ ppc_inst insn = ppc_inst_read((ppc_inst *)p->ainsn.insn);
/* regs->nip is also adjusted if emulate_step returns 1 */
ret = emulate_step(regs, insn);
diff --git a/arch/powerpc/kernel/mce_power.c b/arch/powerpc/kernel/mce_power.c
index e65616bb3a3e..d1fdb5105d32 100644
--- a/arch/powerpc/kernel/mce_power.c
+++ b/arch/powerpc/kernel/mce_power.c
@@ -374,7 +374,7 @@ static int mce_find_instr_ea_and_phys(struct pt_regs *regs,
uint64_t *addr,
pfn = addr_to_pfn(regs, regs->nip);
if (pfn != ULONG_MAX) {
instr_addr = (pfn << PAGE_SHIFT) + (regs->nip & ~PAGE_MASK);
- instr = *(ppc_inst *)(instr_addr);
+ instr = ppc_inst_read((ppc_inst *)instr_addr);
if (!analyse_instr(&op, &tmp, instr)) {
pfn = addr_to_pfn(regs, op.ea);
*addr = op.ea;
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 6027425a85f2..5b53c373373b 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -99,8 +99,8 @@ static unsigned long can_optimize(struct kprobe *p)
* Ensure that the instruction is not a conditional branch,
* and that can be emulated.
*/
- if (!is_conditional_branch(*p->ainsn.insn) &&
- analyse_instr(&op, ®s, *p->ainsn.insn) == 1) {
+ if (!is_conditional_branch(ppc_inst_read(p->ainsn.insn)) &&
+ analyse_instr(&op, ®s, ppc_inst_read(p->ainsn.insn))
== 1) {
emulate_update_regs(®s, &op);
nip = regs.nip;
}
@@ -268,7 +268,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe
*op, struct kprobe *p)
/*
* 3. load instruction to be emulated into relevant register, and
*/
- patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
+ patch_imm32_load_insns(*(unsigned int *)p->ainsn.insn, buff +
TMPL_INSN_IDX);
/*
* 4. branch back from trampoline
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 @@
#defineNUM_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);
+}
+
static ppc_inst
ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
{
@@ -68,7 +74,7 @@ ftrace_modify_code(unsig
