LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "liuqi (BA)" <liuqi115@huawei.com>
To: Amit Kachhap <amit.kachhap@arm.com>,
	Linuxarm <linuxarm@huawei.com>, <catalin.marinas@arm.com>,
	<will@kernel.org>, <naveen.n.rao@linux.ibm.com>,
	<anil.s.keshavamurthy@intel.com>, <davem@davemloft.net>,
	<mhiramat@kernel.org>, <linux-arm-kernel@lists.infradead.org>
Cc: <song.bao.hua@hisilicon.com>, <prime.zeng@hisilicon.com>,
	<robin.murphy@arm.com>, <f.fangjian@huawei.com>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] arm64: kprobe: Enable OPTPROBE for arm64
Date: Mon, 9 Aug 2021 15:00:18 +0800	[thread overview]
Message-ID: <c2aa748e-14a9-1e5f-c4f2-5f6e1c140f5e@huawei.com> (raw)
In-Reply-To: <2fd3c78f-741a-4693-acae-4ea5465ad798@arm.com>


Hi Amit,
On 2021/8/9 12:15, Amit Kachhap wrote:
> Hi Liu,
> 
> I am few minor comments.
> 
> On 8/4/21 11:32 AM, Qi Liu wrote:
>> This patch introduce optprobe for ARM64. In optprobe, probed
>> instruction is replaced by a branch instruction to detour
>> buffer. Detour buffer contains trampoline code and a call to
>> optimized_callback(). optimized_callback() calls opt_pre_handler()
>> to execute kprobe handler.
>>
>> Limitations:
>> - We only support !CONFIG_RANDOMIZE_MODULE_REGION_FULL case to
>> guarantee the offset between probe point and kprobe pre_handler
>> is not larger than 128MiB.
>>
>> Performance of optprobe on Hip08 platform is test using kprobe
>> example module[1] to analyze the latency of a kernel function,
>> and here is the result:
>>
>> [1] 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/kprobes/kretprobe_example.c 
>>
> 
>> +     *
>> +     * kprobe opt use a 'b' instruction to branch to optinsn.insn.
>> +     * According to ARM manual, branch instruction is:
>> +     *
>> +     *   31  30                  25              0
>> +     *  +----+---+---+---+---+---+---------------+
>> +     *  |cond| 0 | 0 | 1 | 0 | 1 |     imm26     |
>> +     *  +----+---+---+---+---+---+---------------+
>> +     *
>> +     * imm26 is a signed 26 bits integer. The real branch offset is 
>> computed
>> +     * by: imm64 = SignExtend(imm26:'00', 64);
>> +     *
>> +     * So the maximum forward branch should be:
>> +     *   (0x01ffffff << 2) = 1720x07fffffc =  0x07fffffc
> 
> 7120x07fffffc ? Shouldn't it be just 0x07fffffc.
> 

Emmm yes, 7120x07fffffc can be dropped, thanks.
>> +     * The maximum backward branch should be:
>> +     *   (0xfe000000 << 2) = 0xFFFFFFFFF8000000 = -0x08000000
>> +     *
>> +     * We can simply check (rel & 0xf8000003):
>> +     *  if rel is positive, (rel & 0xf8000003) should be 0
>> +     *  if rel is negitive, (rel & 0xf8000003) should be 0xf8000000
>> +     *  the last '3' is used for alignment checking.
>> +     */
>> +    return (offset >= -0x08000000 && offset <= 0x07fffffc && !(offset 
>> & 0x3));
>> +}
>> +
>> +int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct 
>> kprobe *orig)
>> +{
>> +    kprobe_opcode_t *code;
>> +    u32 insn;
>> +    int ret, i;
>> +    void *addrs[TMPL_END_IDX];
>> +    void *addr;
>> +
>> +    code = get_optinsn_slot();
>> +    if (!code)
>> +        return -ENOMEM;
>> +
>> +    if (!is_offset_in_range((unsigned long)code,
>> +                (unsigned long)orig->addr + 8))
>> +        goto error;
>> +
>> +    if (!is_offset_in_range((unsigned long)code + TMPL_CALL_BACK,
>> +                (unsigned long)optimized_callback))
>> +        goto error;
>> +
>> +    if (!is_offset_in_range((unsigned long)&code[TMPL_RESTORE_END],
>> +                (unsigned long)op->kp.addr + 4))
>> +        goto error;
>> +
>> +    /* Setup template */
>> +    for (i = 0; i < TMPL_END_IDX; i++)
>> +        addrs[i] = code + i;
>> +
>> +    ret = aarch64_insn_patch_text(addrs, optprobe_template_entry,
>> +                      TMPL_END_IDX);
> 
> Here it copies nops for restore_orig_insn, restore_orig_insn and
> again overwrites them with new values below.
> 

Yes, as Masami said, I could prepare another writable buffer and build 
the trampoline code on it, like x86 optprobe. thanks.

>> +    if (ret < 0)
>> +        goto error;
>> +
[...]
>> +    mrs x1, CurrentEL
>> +    orr x0, x0, x1
>> +    mrs x1, SPSel
>> +    orr x0, x0, x1
>> +    stp xzr, x0, [sp, #S_PC]
>> +    /* Get parameters to optimized_callback() */
>> +    ldr    x0, 1f
>> +    mov    x1, sp
> 
> Most of the codes here are same as save_all_base_regs macro in file
> arch/arm64/kernel/probes/kprobes_trampoline.S. May be they
> can be made common macros and kept in asm/assembler.h
> 
Sounds reasonable, will fix this next time.

Thanks,
Qi
>> +    /* Branch to optimized_callback() */
>> +    .global optprobe_template_call
>> +optprobe_template_call:
>> +    nop
>> +        /* Restore registers */
>> +    ldr x0, [sp, #S_PSTATE]
>> +    and x0, x0, #(PSR_N_BIT | PSR_Z_BIT | PSR_C_BIT | PSR_V_BIT)
>> +    msr nzcv, x0
>> +    ldp x0, x1, [sp, #S_X0]
>> +    ldp x2, x3, [sp, #S_X2]
>> +    ldp x4, x5, [sp, #S_X4]
>> +    ldp x6, x7, [sp, #S_X6]
>> +    ldp x8, x9, [sp, #S_X8]
>> +    ldp x10, x11, [sp, #S_X10]
>> +    ldp x12, x13, [sp, #S_X12]
>> +    ldp x14, x15, [sp, #S_X14]
>> +    ldp x16, x17, [sp, #S_X16]
>> +    ldp x18, x19, [sp, #S_X18]
>> +    ldp x20, x21, [sp, #S_X20]
>> +    ldp x22, x23, [sp, #S_X22]
>> +    ldp x24, x25, [sp, #S_X24]
>> +    ldp x26, x27, [sp, #S_X26]
>> +    ldp x28, x29, [sp, #S_X28]
>> +    ldr lr, [sp, #S_LR]
>> +        add sp, sp, #PT_REGS_SIZE
> 
> Same as previous comment.
> 
> BR.
> //Amit
> 
>> +    .global optprobe_template_restore_orig_insn
>> +optprobe_template_restore_orig_insn:
>> +    nop
>> +    .global optprobe_template_restore_end
>> +optprobe_template_restore_end:
>> +    nop
>> +    .global optprobe_template_end
>> +optprobe_template_end:
>> +    .global optprobe_template_val
>> +optprobe_template_val:
>> +    1:    .long 0
>> +        .long 0
>>
> .


      reply	other threads:[~2021-08-09  7:00 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-04  6:02 Qi Liu
2021-08-05  1:54 ` Masami Hiramatsu
2021-08-05  9:25   ` liuqi (BA)
2021-08-05 16:44     ` Masami Hiramatsu
2021-08-09  6:33       ` liuqi (BA)
2021-08-05  8:55 ` Song Bao Hua (Barry Song)
2021-08-09  6:44   ` liuqi (BA)
2021-08-09  4:15 ` Amit Kachhap
2021-08-09  7:00   ` liuqi (BA) [this message]

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=c2aa748e-14a9-1e5f-c4f2-5f6e1c140f5e@huawei.com \
    --to=liuqi115@huawei.com \
    --cc=amit.kachhap@arm.com \
    --cc=anil.s.keshavamurthy@intel.com \
    --cc=catalin.marinas@arm.com \
    --cc=davem@davemloft.net \
    --cc=f.fangjian@huawei.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@huawei.com \
    --cc=mhiramat@kernel.org \
    --cc=naveen.n.rao@linux.ibm.com \
    --cc=prime.zeng@hisilicon.com \
    --cc=robin.murphy@arm.com \
    --cc=song.bao.hua@hisilicon.com \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v2] arm64: kprobe: Enable OPTPROBE for arm64' \
    /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: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).