Messages in this thread | | | Subject | Re: [PATCH V3 3/4] arch/powerpc: Implement Optprobes | From | Anju T Sudhakar <> | Date | Wed, 8 Feb 2017 11:07:29 +0530 |
| |
Hi Michael,
Thank you so much for the review.
On Wednesday 01 February 2017 04:23 PM, Michael Ellerman wrote: > Anju T Sudhakar <anju@linux.vnet.ibm.com> writes: > >> Detour buffer contains instructions to create an in memory pt_regs. >> After the execution of the pre-handler, a call is made for instruction emulation. >> The NIP is determined in advanced through dummy instruction emulation and a branch >> instruction is created to the NIP at the end of the trampoline. > That's good detail, but it's hard to follow for someone who isn't > familiar with with kprobes/optprobes etc. You don't even tell us what an > optprobe is :) > > So can you provide a bit more background before diving into the specific > details.
Sure. I will provide sufficient details here.
>> Instruction slot for detour buffer is allocated from the reserved area. >> For the time being, 64KB is reserved in memory for this purpose. >> >> Instructions which can be emulated using analyse_instr() are suppliants > ^ > candidates ?
:-) yes 'candidates' will be more appropriate here.
> >> diff --git a/Documentation/features/debug/optprobes/arch-support.txt b/Documentation/features/debug/optprobes/arch-support.txt >> index b8999d8..45bc99d 100644 >> --- a/Documentation/features/debug/optprobes/arch-support.txt >> +++ b/Documentation/features/debug/optprobes/arch-support.txt >> @@ -27,7 +27,7 @@ >> | nios2: | TODO | >> | openrisc: | TODO | >> | parisc: | TODO | >> - | powerpc: | TODO | >> + | powerpc: | ok | > We don't support them for modules yet, so maybe it's premature to flip > that?
ok.
>> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c >> new file mode 100644 >> index 0000000..fb5e62d >> --- /dev/null >> +++ b/arch/powerpc/kernel/optprobes.c >> @@ -0,0 +1,331 @@ >> +/* >> + * Code for Kernel probes Jump optimization. >> + * >> + * Copyright 2016, Anju T, IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * as published by the Free Software Foundation; either version >> + * 2 of the License, or (at your option) any later version. >> + */ >> + >> +#include <linux/kprobes.h> >> +#include <linux/jump_label.h> >> +#include <linux/types.h> >> +#include <linux/slab.h> >> +#include <linux/list.h> >> +#include <asm/kprobes.h> >> +#include <asm/ptrace.h> >> +#include <asm/cacheflush.h> >> +#include <asm/code-patching.h> >> +#include <asm/sstep.h> >> +#include <asm/ppc-opcode.h> >> + >> +#define TMPL_CALL_HDLR_IDX \ >> + (optprobe_template_call_handler - optprobe_template_entry) >> +#define TMPL_EMULATE_IDX \ >> + (optprobe_template_call_emulate - optprobe_template_entry) >> +#define TMPL_RET_IDX \ >> + (optprobe_template_ret - optprobe_template_entry) >> +#define TMPL_OP_IDX \ >> + (optprobe_template_op_address - optprobe_template_entry) >> +#define TMPL_INSN_IDX \ >> + (optprobe_template_insn - optprobe_template_entry) >> +#define TMPL_END_IDX \ >> + (optprobe_template_end - optprobe_template_entry) >> + >> +DEFINE_INSN_CACHE_OPS(ppc_optinsn); >> + >> +static bool insn_page_in_use; >> + >> +static void *__ppc_alloc_insn_page(void) >> +{ >> + if (insn_page_in_use) >> + return NULL; >> + insn_page_in_use = true; > This sets off my "no-locking-visible" detector. I assume there's some > locking somewhere else that makes this work?
Actually we don't need a lock here, since this function is invoked by __get_insn_slot() (in kernel/kprobes.c). __get_insn_slot() already have lock on kprobe_insn_cache.
>> + return &optinsn_slot; >> +} >> + >> +static void __ppc_free_insn_page(void *page __maybe_unused) >> +{ >> + insn_page_in_use = false; >> +} >> + >> +struct kprobe_insn_cache kprobe_ppc_optinsn_slots = { >> + .mutex = __MUTEX_INITIALIZER(kprobe_ppc_optinsn_slots.mutex), >> + .pages = LIST_HEAD_INIT(kprobe_ppc_optinsn_slots.pages), >> + /* insn_size initialized later */ >> + .alloc = __ppc_alloc_insn_page, >> + .free = __ppc_free_insn_page, >> + .nr_garbage = 0, >> +}; >> + >> +/* >> + * Check if we can optimize this probe. Returns NIP post-emulation if this can >> + * be optimized and 0 otherwise. >> + */ >> +static unsigned long can_optimize(struct kprobe *p) >> +{ >> + struct pt_regs regs; >> + struct instruction_op op; >> + unsigned long nip = 0; >> + >> + /* >> + * kprobe placed for kretprobe during boot time >> + * is not optimizing now. >> + * >> + * TODO: Optimize kprobe in kretprobe_trampoline >> + */ >> + if (p->addr == (kprobe_opcode_t *)&kretprobe_trampoline) >> + return 0; >> + >> + /* >> + * We only support optimizing kernel addresses, but not >> + * module addresses. > That probably warrants a TODO or FIXME.
sure.
>> + */ >> + if (!is_kernel_addr((unsigned long)p->addr)) >> + return 0; >> + >> + regs.nip = (unsigned long)p->addr; >> + regs.trap = 0x0; >> + regs.msr = MSR_KERNEL; > It may not matter in practice, but leaving most of regs uninitialised > seems a bit fishy. I'd prefer if we zeroed it first.
yes. Will initialize the regs to zero here.
>> + /* >> + * Ensure that the instruction is not a conditional branch, > Can you spell out why it can't be a conditional branch.
We are not optimizing conditional branches here, because we can't predict the nip prior with dummy pt_regs and can not ensure that the return branch from detour buffer falls in the range of address i.e 32 MB.
I will add proper comments here.
>> + * and that can be emulated. >> + */ >> + if (!is_conditional_branch(*p->ainsn.insn) && >> + analyse_instr(&op, ®s, *p->ainsn.insn)) >> + nip = regs.nip; >> + >> + return nip; >> +} >> + >> +static void optimized_callback(struct optimized_kprobe *op, >> + struct pt_regs *regs) >> +{ >> + struct kprobe_ctlblk *kcb = get_kprobe_ctlblk(); >> + unsigned long flags; >> + >> + /* This is possible if op is under delayed unoptimizing */ >> + if (kprobe_disabled(&op->kp)) >> + return; >> + >> + local_irq_save(flags); > What is that protecting against? Because on powerpc it doesn't actually > disable interrupts, it just masks some of them, the perf interrupt for > example can still run.
As pointed out by Naveen, we need hard_irq_disable() here. Thanks for the catch.
:-)
>> + >> + if (kprobe_running()) { >> + kprobes_inc_nmissed_count(&op->kp); >> + } else { >> + __this_cpu_write(current_kprobe, &op->kp); >> + regs->nip = (unsigned long)op->kp.addr; >> + kcb->kprobe_status = KPROBE_HIT_ACTIVE; >> + opt_pre_handler(&op->kp, regs); >> + __this_cpu_write(current_kprobe, NULL); >> + } >> + local_irq_restore(flags); >> +} >> +NOKPROBE_SYMBOL(optimized_callback); >> + >> +void arch_remove_optimized_kprobe(struct optimized_kprobe *op) >> +{ >> + if (op->optinsn.insn) { >> + free_ppc_optinsn_slot(op->optinsn.insn, 1); >> + op->optinsn.insn = NULL; >> + } >> +} >> + >> +/* >> + * emulate_step() requires insn to be emulated as >> + * second parameter. Load register 'r4' with the >> + * instruction. >> + */ >> +void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr) >> +{ >> + /* addis r4,0,(insn)@h */ >> + *addr++ = PPC_INST_ADDIS | ___PPC_RT(4) | >> + ((val >> 16) & 0xffff); >> + >> + /* ori r4,r4,(insn)@l */ >> + *addr = PPC_INST_ORI | ___PPC_RA(4) | ___PPC_RS(4) | >> + (val & 0xffff); >> +} >> + >> +/* >> + * Generate instructions to load provided immediate 64-bit value >> + * to register 'r3' and patch these instructions at 'addr'. >> + */ >> +void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr) >> +{ >> + /* lis r3,(op)@highest */ >> + *addr++ = PPC_INST_ADDIS | ___PPC_RT(3) | >> + ((val >> 48) & 0xffff); >> + >> + /* ori r3,r3,(op)@higher */ >> + *addr++ = PPC_INST_ORI | ___PPC_RA(3) | ___PPC_RS(3) | >> + ((val >> 32) & 0xffff); >> + >> + /* rldicr r3,r3,32,31 */ >> + *addr++ = PPC_INST_RLDICR | ___PPC_RA(3) | ___PPC_RS(3) | >> + __PPC_SH64(32) | __PPC_ME64(31); >> + >> + /* oris r3,r3,(op)@h */ >> + *addr++ = PPC_INST_ORIS | ___PPC_RA(3) | ___PPC_RS(3) | >> + ((val >> 16) & 0xffff); >> + >> + /* ori r3,r3,(op)@l */ >> + *addr = PPC_INST_ORI | ___PPC_RA(3) | ___PPC_RS(3) | >> + (val & 0xffff); >> +} > I can't say I love those patch functions. A PC-relative load would be > cleaner, but I guess that's ugly/expensive on current CPUs. > >> diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S >> new file mode 100644 >> index 0000000..c86976b >> --- /dev/null >> +++ b/arch/powerpc/kernel/optprobes_head.S >> @@ -0,0 +1,135 @@ >> +/* >> + * Code to prepare detour buffer for optprobes in Kernel. >> + * >> + * Copyright 2016, Anju T, IBM Corp. >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * as published by the Free Software Foundation; either version >> + * 2 of the License, or (at your option) any later version. >> + */ >> + >> +#include <asm/ppc_asm.h> >> +#include <asm/ptrace.h> >> +#include <asm/asm-offsets.h> >> + >> +#define OPT_SLOT_SIZE 65536 >> + >> + .balign 4 >> + >> + /* >> + * Reserve an area to allocate slots for detour buffer. >> + * This is part of .text section (rather than vmalloc area) >> + * as this needs to be within 32MB of the probed address. >> + */ >> + .global optinsn_slot >> +optinsn_slot: >> + .space OPT_SLOT_SIZE >> + >> + /* >> + * Optprobe template: >> + * This template gets copied into one of the slots in optinsn_slot >> + * and gets fixed up with real optprobe structures et al. >> + */ >> + .global optprobe_template_entry >> +optprobe_template_entry: >> + /* Create an in-memory pt_regs */ >> + stdu 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) >> + SAVE_10GPRS(2,r1) >> + SAVE_10GPRS(12,r1) >> + SAVE_10GPRS(22,r1) >> + /* Save SPRS */ >> + mfmsr r5 >> + std r5,_MSR(r1) >> + li r5,0x700 >> + std r5,_TRAP(r1) >> + li r5,0 >> + std r5,ORIG_GPR3(r1) >> + std r5,RESULT(r1) >> + mfctr r5 >> + std r5,_CTR(r1) >> + mflr r5 >> + std r5,_LINK(r1) >> + mfspr r5,SPRN_XER >> + std r5,_XER(r1) >> + mfcr r5 >> + std r5,_CCR(r1) >> + lbz r5,PACASOFTIRQEN(r13) >> + std r5,SOFTE(r1) >> + mfdar r5 >> + std r5,_DAR(r1) >> + mfdsisr r5 >> + std r5,_DSISR(r1) > So this is what made me originally reply to this patch. This > save/restore sequence. > > I'm not clear on why this is what we need to save/restore. > > Aren't we essentially just interposing a function call? If so do we need > to save/restore all of these? eg. MSR/DAR/DSISR. Non-volatile GPRs? And > why are we pretending there was a 0x700 trap? > > Is it because we're going to end up emulating the instruction and so we > need everything in pt_regs ? >
Yes. we need the pt_regs to be saved in the detour buffer.
>> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c >> index 3362299..895dcdd 100644 >> --- a/arch/powerpc/lib/sstep.c >> +++ b/arch/powerpc/lib/sstep.c >> @@ -618,6 +618,27 @@ static int __kprobes trap_compare(long v1, long v2) >> } >> >> /* >> + * Helper to check if a given instruction is a conditional branch >> + * Derived from the conditional checks in analyse_instr() >> + */ >> +bool __kprobes is_conditional_branch(unsigned int instr) >> +{ >> + unsigned int opcode = instr >> 26; >> + > We already have some similar routines in code_patching.c/h. Can you move > this in there instead?
sure.
> cheers >
Thanks and regards, Anju
| |