From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933090AbcE3Ixw (ORCPT ); Mon, 30 May 2016 04:53:52 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:12365 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932978AbcE3Ixf (ORCPT ); Mon, 30 May 2016 04:53:35 -0400 Message-Id: <201605300853.u4U8mdVO003525@mx0a-001b2d01.pphosted.com> X-IBM-Helo: d23dlp02.au.ibm.com X-IBM-MailFrom: naveen.n.rao@linux.vnet.ibm.com X-IBM-RcptTo: linux-kernel@vger.kernel.org Date: Mon, 30 May 2016 14:21:57 +0530 From: "Naveen N. Rao" To: Madhavan Srinivasan Cc: Anju T , linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, ananth@in.ibm.com, mahesh@linux.vnet.ibm.com, paulus@samba.org, mhiramat@kernel.org, hemant@linux.vnet.ibm.com, srikar@linux.vnet.ibm.com Subject: Re: [RFC PATCH v2 1/3] arch/powerpc : Add detour buffer support for optprobes References: <1463670640-25701-1-git-send-email-anju@linux.vnet.ibm.com> <1463670640-25701-2-git-send-email-anju@linux.vnet.ibm.com> <201605241015.u4OAFAnB038915@mx0a-001b2d01.pphosted.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201605241015.u4OAFAnB038915@mx0a-001b2d01.pphosted.com> User-Agent: Mutt/1.6.0 (2016-04-01) X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16053008-0032-0000-0000-0000019A32E8 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16053008-0033-0000-0000-00000616366A X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-05-30_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=29 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1604210000 definitions=main-1605300111 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2016/05/24 03:45PM, Madhavan Srinivasan wrote: > > > On Thursday 19 May 2016 08:40 PM, Anju T wrote: > > Detour buffer contains instructions to create an in memory pt_regs. > > After the execution of prehandler a call is made for instruction emulation. > > The NIP is decided after the probed instruction is executed. Hence a branch > > instruction is created to the NIP returned by emulate_step(). > > > > Instruction slot for detour buffer is allocated from > > the reserved area. For the time being 64KB is reserved > > in memory for this purpose. > > > > Signed-off-by: Anju T > > --- > > arch/powerpc/include/asm/kprobes.h | 25 ++++++++ > > arch/powerpc/kernel/optprobes_head.S | 108 +++++++++++++++++++++++++++++++++++ > > 2 files changed, 133 insertions(+) > > create mode 100644 arch/powerpc/kernel/optprobes_head.S > > > > diff --git a/arch/powerpc/include/asm/kprobes.h b/arch/powerpc/include/asm/kprobes.h > > index 039b583..3e4c998 100644 > > --- a/arch/powerpc/include/asm/kprobes.h > > +++ b/arch/powerpc/include/asm/kprobes.h > > @@ -38,7 +38,25 @@ struct pt_regs; > > struct kprobe; > > > > typedef ppc_opcode_t kprobe_opcode_t; > > + > > +extern kprobe_opcode_t optinsn_slot; > > +/* Optinsn template address */ > > +extern kprobe_opcode_t optprobe_template_entry[]; > > +extern kprobe_opcode_t optprobe_template_call_handler[]; > > +extern kprobe_opcode_t optprobe_template_call_emulate[]; > > +extern kprobe_opcode_t optprobe_template_ret_branch[]; > > +extern kprobe_opcode_t optprobe_template_ret[]; > > +extern kprobe_opcode_t optprobe_template_insn[]; > > +extern kprobe_opcode_t optprobe_template_op_address1[]; > > +extern kprobe_opcode_t optprobe_template_op_address2[]; > > +extern kprobe_opcode_t optprobe_template_end[]; > > + > > #define MAX_INSN_SIZE 1 > > +#define MAX_OPTIMIZED_LENGTH 4 > > +#define MAX_OPTINSN_SIZE \ > > + ((unsigned long)&optprobe_template_end - \ > > + (unsigned long)&optprobe_template_entry) > > +#define RELATIVEJUMP_SIZE 4 > > > > #ifdef CONFIG_PPC64 > > #if defined(_CALL_ELF) && _CALL_ELF == 2 > > @@ -129,5 +147,12 @@ struct kprobe_ctlblk { > > extern int kprobe_exceptions_notify(struct notifier_block *self, > > unsigned long val, void *data); > > extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr); > > + > > +struct arch_optimized_insn { > > + kprobe_opcode_t copied_insn[1]; > > + /* detour buffer */ > > + kprobe_opcode_t *insn; > > +}; > > + > > #endif /* __KERNEL__ */ > > #endif /* _ASM_POWERPC_KPROBES_H */ > > diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S > > new file mode 100644 > > index 0000000..ce32aec > > --- /dev/null > > +++ b/arch/powerpc/kernel/optprobes_head.S > > @@ -0,0 +1,108 @@ > > +/* > > + * 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 > > +#include > > +#include > > + You should also add an align directive here: .align 2 > > +.global optinsn_slot > > +optinsn_slot: > > + /* Reserve an area to allocate slots for detour buffer */ > > + .space 65536 > > Can we replace "65536" with SLOT_SIZE? Yes, a macro will be good to have. > > > +.global optprobe_template_entry > > +optprobe_template_entry: > > + stdu r1,-INT_FRAME_SIZE(r1) > > + SAVE_GPR(0,r1) > > + /* Save the previous SP into stack */ > > + addi r0,r1,INT_FRAME_SIZE > > + std 0,GPR1(r1) > > + SAVE_2GPRS(2,r1) > > + SAVE_8GPRS(4,r1) > > + SAVE_10GPRS(12,r1) > > + SAVE_10GPRS(22,r1) > > + /* Save SPRS */ > > + mfcfar r5 > > + std r5,_NIP(r1) > > + mfmsr r5 > > + std r5,_MSR(r1) > > + mfctr r5 > > + std r5,_CTR(r1) > > + mflr r5 > > + std r5,_LINK(r1) > > + mfspr r5,SPRN_XER > > + std r5,_XER(r1) > > + li r5,0 > > + std r5,_TRAP(r1) > > + mfdar r5 > > + std r5,_DAR(r1) > > + mfdsisr r5 > > + std r5,_DSISR(r1) > > We are missing _CCR (CR register)? Yes, and I think we need to ensure we fill reasonable values for all registers in pt_regs including softe (from paca), orig_gpr3 (0?) and result (0). > > Why tab space for the comment? and > a new line before the command will be good ... new line before *each section* will be good :) > > > + /* Pass parameters for optimized_callback */ > > +.global optprobe_template_op_address1 > > +optprobe_template_op_address1: So, the general style is to align labels at the start of a line and to align everything else as per the code indentation. In this case though, I think it's best to use a macro for better readability. Something like: #define _GLOB(name) .global name; \ name: - Naveen