Message-Id: <b4292adb-fdd2-d257-839a-8de5c3f1f9d5@linux.vnet.ibm.com>
Date:   Wed, 8 Feb 2017 11:07:29 +0530
From:   Anju T Sudhakar <anju@...ux.vnet.ibm.com>
To:     Michael Ellerman <mpe@...erman.id.au>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Cc:     ananth@...ibm.com, naveen.n.rao@...ux.vnet.ibm.com,
        paulus@...ba.org, srikar@...ux.vnet.ibm.com,
        benh@...nel.crashing.org, mahesh@...ux.vnet.ibm.com,
        mhiramat@...nel.org
Subject: Re: [PATCH V3 3/4] arch/powerpc: Implement Optprobes

Hi Michael,


Thank you so much for the review.

On Wednesday 01 February 2017 04:23 PM, Michael Ellerman wrote:
> Anju T Sudhakar <anju@...ux.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, &regs, *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

Powered by blists - more mailing lists