From: Christophe Leroy <christophe.leroy@csgroup.eu> To: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Michael Ellerman <mpe@ellerman.id.au>, Paul Mackerras <paulus@samba.org> Cc: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v1 2/2] powerpc: Enable OPTPROBES on PPC32 Date: Tue, 20 Apr 2021 15:37:06 +0200 [thread overview] Message-ID: <db3b71f3-2554-69b0-6ef9-a70e31d034b6@csgroup.eu> (raw) In-Reply-To: <1618900760.son2fwciv1.naveen@linux.ibm.com> Le 20/04/2021 à 08:51, Naveen N. Rao a écrit : > Christophe Leroy wrote: >> For that, create a 32 bits version of patch_imm64_load_insns() >> and create a patch_imm_load_insns() which calls >> patch_imm32_load_insns() on PPC32 and patch_imm64_load_insns() >> on PPC64. >> >> Adapt optprobes_head.S for PPC32. Use PPC_LL/PPC_STL macros instead >> of raw ld/std, opt out things linked to paca and use stmw/lmw to >> save/restore registers. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/Kconfig | 2 +- >> arch/powerpc/kernel/optprobes.c | 24 +++++++++++++-- >> arch/powerpc/kernel/optprobes_head.S | 46 +++++++++++++++++++--------- >> 3 files changed, 53 insertions(+), 19 deletions(-) > > Thanks for adding support for ppc32. It is good to see that it works without too many changes. > >> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig >> index c1344c05226c..49b538e54efb 100644 >> --- a/arch/powerpc/Kconfig >> +++ b/arch/powerpc/Kconfig >> @@ -227,7 +227,7 @@ config PPC >> select HAVE_MOD_ARCH_SPECIFIC >> select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S) >> select HAVE_HARDLOCKUP_DETECTOR_ARCH if PPC64 && PPC_BOOK3S && SMP >> - select HAVE_OPTPROBES if PPC64 >> + select HAVE_OPTPROBES >> select HAVE_PERF_EVENTS >> select HAVE_PERF_EVENTS_NMI if PPC64 >> select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && >> !HAVE_HARDLOCKUP_DETECTOR_ARCH >> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c >> index 58fdb9f66e0f..cdf87086fa33 100644 >> --- a/arch/powerpc/kernel/optprobes.c >> +++ b/arch/powerpc/kernel/optprobes.c >> @@ -141,11 +141,21 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op) >> } >> } >> >> +static void patch_imm32_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr) >> +{ >> + patch_instruction((struct ppc_inst *)addr, >> + ppc_inst(PPC_RAW_LIS(reg, IMM_H(val)))); >> + addr++; >> + >> + patch_instruction((struct ppc_inst *)addr, >> + ppc_inst(PPC_RAW_ORI(reg, reg, IMM_L(val)))); >> +} >> + >> /* >> * Generate instructions to load provided immediate 64-bit value >> * to register 'reg' and patch these instructions at 'addr'. >> */ >> -static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr) >> +static void patch_imm64_load_insns(unsigned long long val, int reg, kprobe_opcode_t *addr) > > Do you really need this? Without it I get: from arch/powerpc/kernel/optprobes.c:8: arch/powerpc/kernel/optprobes.c: In function 'patch_imm64_load_insns': arch/powerpc/kernel/optprobes.c:163:14: error: right shift count >= width of type [-Werror=shift-count-overflow] 163 | ((val >> 48) & 0xffff))); | ^~ ./arch/powerpc/include/asm/inst.h:69:48: note: in definition of macro 'ppc_inst' 69 | #define ppc_inst(x) ((struct ppc_inst){ .val = x }) | ^ arch/powerpc/kernel/optprobes.c:169:31: error: right shift count >= width of type [-Werror=shift-count-overflow] 169 | ___PPC_RS(reg) | ((val >> 32) & 0xffff))); | ^~ ./arch/powerpc/include/asm/inst.h:69:48: note: in definition of macro 'ppc_inst' 69 | #define ppc_inst(x) ((struct ppc_inst){ .val = x }) | ^ > >> { >> /* lis reg,(op)@highest */ >> patch_instruction((struct ppc_inst *)addr, >> @@ -177,6 +187,14 @@ static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t * >> ___PPC_RS(reg) | (val & 0xffff))); >> } >> >> +static void patch_imm_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr) >> +{ >> + if (IS_ENABLED(CONFIG_PPC64)) >> + patch_imm64_load_insns(val, reg, addr); >> + else >> + patch_imm32_load_insns(val, reg, addr); >> +} >> + >> int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p) >> { >> struct ppc_inst branch_op_callback, branch_emulate_step, temp; >> @@ -230,7 +248,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p) >> * Fixup the template with instructions to: >> * 1. load the address of the actual probepoint >> */ >> - patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX); >> + patch_imm_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX); >> >> /* >> * 2. branch to optimized_callback() and emulate_step() >> @@ -264,7 +282,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p) >> * 3. load instruction to be emulated into relevant register, and >> */ >> temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn); >> - patch_imm64_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX); >> + patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX); >> >> /* >> * 4. branch back from trampoline >> diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S >> index ff8ba4d3824d..49f31e554573 100644 >> --- a/arch/powerpc/kernel/optprobes_head.S >> +++ b/arch/powerpc/kernel/optprobes_head.S >> @@ -30,39 +30,47 @@ optinsn_slot: >> .global optprobe_template_entry >> optprobe_template_entry: >> /* Create an in-memory pt_regs */ >> - stdu r1,-INT_FRAME_SIZE(r1) >> + PPC_STLU r1,-INT_FRAME_SIZE(r1) >> SAVE_GPR(0,r1) >> /* Save the previous SP into stack */ >> addi r0,r1,INT_FRAME_SIZE >> - std r0,GPR1(r1) >> + PPC_STL r0,GPR1(r1) >> +#ifdef CONFIG_PPC64 >> SAVE_10GPRS(2,r1) >> SAVE_10GPRS(12,r1) >> SAVE_10GPRS(22,r1) >> +#else >> + stmw r2, GPR2(r1) >> +#endif >> /* Save SPRS */ >> mfmsr r5 >> - std r5,_MSR(r1) >> + PPC_STL r5,_MSR(r1) >> li r5,0x700 >> - std r5,_TRAP(r1) >> + PPC_STL r5,_TRAP(r1) >> li r5,0 >> - std r5,ORIG_GPR3(r1) >> - std r5,RESULT(r1) >> + PPC_STL r5,ORIG_GPR3(r1) >> + PPC_STL r5,RESULT(r1) >> mfctr r5 >> - std r5,_CTR(r1) >> + PPC_STL r5,_CTR(r1) >> mflr r5 >> - std r5,_LINK(r1) >> + PPC_STL r5,_LINK(r1) >> mfspr r5,SPRN_XER >> - std r5,_XER(r1) >> + PPC_STL r5,_XER(r1) >> mfcr r5 >> - std r5,_CCR(r1) >> + PPC_STL r5,_CCR(r1) >> +#ifdef CONFIG_PPC64 >> lbz r5,PACAIRQSOFTMASK(r13) >> std r5,SOFTE(r1) >> +#endif >> >> /* >> * We may get here from a module, so load the kernel TOC in r2. >> * The original TOC gets restored when pt_regs is restored >> * further below. >> */ >> +#ifdef CONFIG_PPC64 >> ld r2,PACATOC(r13) >> +#endif > > Are the ISA and ABI documents for ppc32 available publicly? ABI: https://wiki.raptorcs.com/w/images/0/03/Power-Arch-32-bit-ABI-supp-1.0-Unified.pdf ISA: https://www.nxp.com/files-static/product/doc/MPCFPE32B.pdf > I would have thought that the TOC issues > apply to ppc32 as well, but want to understand why this isn't a problem there. r2 is the pointer to 'current' on PPC32. No TOC. > >> >> .global optprobe_template_op_address >> optprobe_template_op_address: >> @@ -72,9 +80,11 @@ optprobe_template_op_address: >> */ >> nop >> nop >> +#ifdef CONFIG_PPC64 >> nop >> nop >> nop >> +#endif >> /* 2. pt_regs pointer in r4 */ >> addi r4,r1,STACK_FRAME_OVERHEAD >> >> @@ -94,9 +104,11 @@ optprobe_template_insn: >> /* 2, Pass instruction to be emulated in r4 */ >> nop >> nop >> +#ifdef CONFIG_PPC64 >> nop >> nop >> nop >> +#endif > > It would be nice to put these behind a macro so as to avoid these #ifdef blocks here, as well as > with the register save/restore sequence. > Will see what I can do Christophe
WARNING: multiple messages have this Message-ID (diff)
From: Christophe Leroy <christophe.leroy@csgroup.eu> To: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Michael Ellerman <mpe@ellerman.id.au>, Paul Mackerras <paulus@samba.org> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 2/2] powerpc: Enable OPTPROBES on PPC32 Date: Tue, 20 Apr 2021 15:37:06 +0200 [thread overview] Message-ID: <db3b71f3-2554-69b0-6ef9-a70e31d034b6@csgroup.eu> (raw) In-Reply-To: <1618900760.son2fwciv1.naveen@linux.ibm.com> Le 20/04/2021 à 08:51, Naveen N. Rao a écrit : > Christophe Leroy wrote: >> For that, create a 32 bits version of patch_imm64_load_insns() >> and create a patch_imm_load_insns() which calls >> patch_imm32_load_insns() on PPC32 and patch_imm64_load_insns() >> on PPC64. >> >> Adapt optprobes_head.S for PPC32. Use PPC_LL/PPC_STL macros instead >> of raw ld/std, opt out things linked to paca and use stmw/lmw to >> save/restore registers. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- >> arch/powerpc/Kconfig | 2 +- >> arch/powerpc/kernel/optprobes.c | 24 +++++++++++++-- >> arch/powerpc/kernel/optprobes_head.S | 46 +++++++++++++++++++--------- >> 3 files changed, 53 insertions(+), 19 deletions(-) > > Thanks for adding support for ppc32. It is good to see that it works without too many changes. > >> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig >> index c1344c05226c..49b538e54efb 100644 >> --- a/arch/powerpc/Kconfig >> +++ b/arch/powerpc/Kconfig >> @@ -227,7 +227,7 @@ config PPC >> select HAVE_MOD_ARCH_SPECIFIC >> select HAVE_NMI if PERF_EVENTS || (PPC64 && PPC_BOOK3S) >> select HAVE_HARDLOCKUP_DETECTOR_ARCH if PPC64 && PPC_BOOK3S && SMP >> - select HAVE_OPTPROBES if PPC64 >> + select HAVE_OPTPROBES >> select HAVE_PERF_EVENTS >> select HAVE_PERF_EVENTS_NMI if PPC64 >> select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI && >> !HAVE_HARDLOCKUP_DETECTOR_ARCH >> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c >> index 58fdb9f66e0f..cdf87086fa33 100644 >> --- a/arch/powerpc/kernel/optprobes.c >> +++ b/arch/powerpc/kernel/optprobes.c >> @@ -141,11 +141,21 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op) >> } >> } >> >> +static void patch_imm32_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr) >> +{ >> + patch_instruction((struct ppc_inst *)addr, >> + ppc_inst(PPC_RAW_LIS(reg, IMM_H(val)))); >> + addr++; >> + >> + patch_instruction((struct ppc_inst *)addr, >> + ppc_inst(PPC_RAW_ORI(reg, reg, IMM_L(val)))); >> +} >> + >> /* >> * Generate instructions to load provided immediate 64-bit value >> * to register 'reg' and patch these instructions at 'addr'. >> */ >> -static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr) >> +static void patch_imm64_load_insns(unsigned long long val, int reg, kprobe_opcode_t *addr) > > Do you really need this? Without it I get: from arch/powerpc/kernel/optprobes.c:8: arch/powerpc/kernel/optprobes.c: In function 'patch_imm64_load_insns': arch/powerpc/kernel/optprobes.c:163:14: error: right shift count >= width of type [-Werror=shift-count-overflow] 163 | ((val >> 48) & 0xffff))); | ^~ ./arch/powerpc/include/asm/inst.h:69:48: note: in definition of macro 'ppc_inst' 69 | #define ppc_inst(x) ((struct ppc_inst){ .val = x }) | ^ arch/powerpc/kernel/optprobes.c:169:31: error: right shift count >= width of type [-Werror=shift-count-overflow] 169 | ___PPC_RS(reg) | ((val >> 32) & 0xffff))); | ^~ ./arch/powerpc/include/asm/inst.h:69:48: note: in definition of macro 'ppc_inst' 69 | #define ppc_inst(x) ((struct ppc_inst){ .val = x }) | ^ > >> { >> /* lis reg,(op)@highest */ >> patch_instruction((struct ppc_inst *)addr, >> @@ -177,6 +187,14 @@ static void patch_imm64_load_insns(unsigned long val, int reg, kprobe_opcode_t * >> ___PPC_RS(reg) | (val & 0xffff))); >> } >> >> +static void patch_imm_load_insns(unsigned long val, int reg, kprobe_opcode_t *addr) >> +{ >> + if (IS_ENABLED(CONFIG_PPC64)) >> + patch_imm64_load_insns(val, reg, addr); >> + else >> + patch_imm32_load_insns(val, reg, addr); >> +} >> + >> int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p) >> { >> struct ppc_inst branch_op_callback, branch_emulate_step, temp; >> @@ -230,7 +248,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p) >> * Fixup the template with instructions to: >> * 1. load the address of the actual probepoint >> */ >> - patch_imm64_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX); >> + patch_imm_load_insns((unsigned long)op, 3, buff + TMPL_OP_IDX); >> >> /* >> * 2. branch to optimized_callback() and emulate_step() >> @@ -264,7 +282,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p) >> * 3. load instruction to be emulated into relevant register, and >> */ >> temp = ppc_inst_read((struct ppc_inst *)p->ainsn.insn); >> - patch_imm64_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX); >> + patch_imm_load_insns(ppc_inst_as_ulong(temp), 4, buff + TMPL_INSN_IDX); >> >> /* >> * 4. branch back from trampoline >> diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S >> index ff8ba4d3824d..49f31e554573 100644 >> --- a/arch/powerpc/kernel/optprobes_head.S >> +++ b/arch/powerpc/kernel/optprobes_head.S >> @@ -30,39 +30,47 @@ optinsn_slot: >> .global optprobe_template_entry >> optprobe_template_entry: >> /* Create an in-memory pt_regs */ >> - stdu r1,-INT_FRAME_SIZE(r1) >> + PPC_STLU r1,-INT_FRAME_SIZE(r1) >> SAVE_GPR(0,r1) >> /* Save the previous SP into stack */ >> addi r0,r1,INT_FRAME_SIZE >> - std r0,GPR1(r1) >> + PPC_STL r0,GPR1(r1) >> +#ifdef CONFIG_PPC64 >> SAVE_10GPRS(2,r1) >> SAVE_10GPRS(12,r1) >> SAVE_10GPRS(22,r1) >> +#else >> + stmw r2, GPR2(r1) >> +#endif >> /* Save SPRS */ >> mfmsr r5 >> - std r5,_MSR(r1) >> + PPC_STL r5,_MSR(r1) >> li r5,0x700 >> - std r5,_TRAP(r1) >> + PPC_STL r5,_TRAP(r1) >> li r5,0 >> - std r5,ORIG_GPR3(r1) >> - std r5,RESULT(r1) >> + PPC_STL r5,ORIG_GPR3(r1) >> + PPC_STL r5,RESULT(r1) >> mfctr r5 >> - std r5,_CTR(r1) >> + PPC_STL r5,_CTR(r1) >> mflr r5 >> - std r5,_LINK(r1) >> + PPC_STL r5,_LINK(r1) >> mfspr r5,SPRN_XER >> - std r5,_XER(r1) >> + PPC_STL r5,_XER(r1) >> mfcr r5 >> - std r5,_CCR(r1) >> + PPC_STL r5,_CCR(r1) >> +#ifdef CONFIG_PPC64 >> lbz r5,PACAIRQSOFTMASK(r13) >> std r5,SOFTE(r1) >> +#endif >> >> /* >> * We may get here from a module, so load the kernel TOC in r2. >> * The original TOC gets restored when pt_regs is restored >> * further below. >> */ >> +#ifdef CONFIG_PPC64 >> ld r2,PACATOC(r13) >> +#endif > > Are the ISA and ABI documents for ppc32 available publicly? ABI: https://wiki.raptorcs.com/w/images/0/03/Power-Arch-32-bit-ABI-supp-1.0-Unified.pdf ISA: https://www.nxp.com/files-static/product/doc/MPCFPE32B.pdf > I would have thought that the TOC issues > apply to ppc32 as well, but want to understand why this isn't a problem there. r2 is the pointer to 'current' on PPC32. No TOC. > >> >> .global optprobe_template_op_address >> optprobe_template_op_address: >> @@ -72,9 +80,11 @@ optprobe_template_op_address: >> */ >> nop >> nop >> +#ifdef CONFIG_PPC64 >> nop >> nop >> nop >> +#endif >> /* 2. pt_regs pointer in r4 */ >> addi r4,r1,STACK_FRAME_OVERHEAD >> >> @@ -94,9 +104,11 @@ optprobe_template_insn: >> /* 2, Pass instruction to be emulated in r4 */ >> nop >> nop >> +#ifdef CONFIG_PPC64 >> nop >> nop >> nop >> +#endif > > It would be nice to put these behind a macro so as to avoid these #ifdef blocks here, as well as > with the register save/restore sequence. > Will see what I can do Christophe
next prev parent reply other threads:[~2021-04-20 13:37 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-06 9:38 [PATCH v1 1/2] powerpc/inst: ppc_inst_as_u64() becomes ppc_inst_as_ulong() Christophe Leroy 2021-04-06 9:38 ` Christophe Leroy 2021-04-06 9:38 ` [PATCH v1 2/2] powerpc: Enable OPTPROBES on PPC32 Christophe Leroy 2021-04-06 9:38 ` Christophe Leroy 2021-04-20 6:51 ` Naveen N. Rao 2021-04-20 6:51 ` Naveen N. Rao 2021-04-20 13:37 ` Christophe Leroy [this message] 2021-04-20 13:37 ` Christophe Leroy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=db3b71f3-2554-69b0-6ef9-a70e31d034b6@csgroup.eu \ --to=christophe.leroy@csgroup.eu \ --cc=benh@kernel.crashing.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mpe@ellerman.id.au \ --cc=naveen.n.rao@linux.vnet.ibm.com \ --cc=paulus@samba.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.