jprobe question

jprobe question

[Zhang, Yanmin]

Mostly, jprobe handler has parameters. If the parameters are changed in
the jprobe handler, should the original function use the changed values?


For example, function store_online calls cpu_down. If we register a
jprobe at cpu_down, see below function.


int cpu_down_handler(unsigned int cpu)
{
	cpu += 1;
	return cpu;
}

Assume store_online calls cpu_down with parameter cpu=2, when
cpu_down_handler exits back to original function cpu_down, should
cpu_down use the new parameter value 3(orig_cpu +1) instead of the 2
(the original value)?

My answer is no. Because c compiler might change the parameter values
even though we don't change them in c codes sometimes.

What's your idea?

Yanmin

Re: jprobe question

[Ananth N Mavinakayanahalli]

On Tue, Nov 29, 2005 at 11:54:44AM +0800, Zhang, Yanmin wrote:
> Mostly, jprobe handler has parameters. If the parameters are changed in
> the jprobe handler, should the original function use the changed values?
> 
> 
> For example, function store_online calls cpu_down. If we register a
> jprobe at cpu_down, see below function.
> 
> 
> int cpu_down_handler(unsigned int cpu)
> {
> 	cpu += 1;
> 	return cpu;
> }
> 
> Assume store_online calls cpu_down with parameter cpu=2, when
> cpu_down_handler exits back to original function cpu_down, should
> cpu_down use the new parameter value 3(orig_cpu +1) instead of the 2
> (the original value)?

It shouldn't and it won't

> My answer is no. Because c compiler might change the parameter values
> even though we don't change them in c codes sometimes.
> 
> What's your idea?

If you look closely at the jprobes code, care is taken to reset the
stack/pt_regs to the original values after returning from the jprobe
handler. Changes to the parameters in the handlers should not affect
the original parameter values.

Ananth

RE: jprobe question

[Keshavamurthy, Anil S]

>[YM] unwind costs too much time because it will create a 
>switch_stack and unwind frame on the top of the stack. Anyway, 
>I will work out a new patch using unwind.

Yes, I understand the unwind cost, but at the 
same time hate quick and dirty way to store bsp 
in the pt_reg reserved area. For now don't
worry about the unwind cost. Let's 
push saving bsp patch separately later and at
that time we can optimize jprobes.

Also more comment before you cook up the new patch, 
some people on ia64 mailing list don't like adding 
asm volatile ("....") instruction in the C file.
If possible avoid this asm in the C file.

Post your new patch onto Ia64 mailing list and Cc'ing systemtap.

-Anil

RE: jprobe question

[Zhang, Yanmin]

>>-----Original Message-----
>>From: Keshavamurthy, Anil S
>>Sent: 2005年12月1日 4:56
>>To: Zhang, Yanmin
>>Cc: 'systemtap@sources.redhat.com'; Mao, Bibo
>>Subject: RE: jprobe question
>>
>>>Here are the patches. In function non_syscall, I save the
>>>ar.bsp (after instruction cover) to the scratch area below
>>>pt_regs, then calls preserve_scratch_area.
>>>preserve_scratch_area will reserve 1 new 16-byte area for next
>>>call to ia64_bad_break. Later on, kprobe handler uses
>>>pt_regs->cr_ifs and the saved ar.bsp to get the real ar.bsp
>>>and parameter number. This approach also considers user space
>>>probe. kprobe_save_bsp patch has no any impact on critical fault patch.
>>>The jprobe patch is to save the function parameters when the
>>>jprobe is hit and restore them after break.
>>
>>Yanmin,
>>	I am not sure whether using the scratch area below the pt_regs
>>to save ar.bsp would be the right thing to do and more over it does look like
>>a hack.
>>Clean solution would be to have a field in pt_regs to save ar.bsp,
[YM] Yes. But some bigwigs might be unhappy to add new members into pt_regs.

 until we
>>have it, here is what you can do for now.
>>
>>In the setjmp_pre_handler() get the ar.bsp by unwinding the stack
>>and then calling unw_get_bsp() to get the ar.bsp. Save this ar.bsp in the kcb
>>structure and later
>>in longjmp_break_handler() you can use this ar.bsp without unwinding the stack.
[YM] unwind costs too much time because it will create a switch_stack and unwind frame on the top of the stack. Anyway, I will work out a new patch using unwind.

RE: jprobe question

[Keshavamurthy, Anil S]

>Here are the patches. In function non_syscall, I save the 
>ar.bsp (after instruction cover) to the scratch area below 
>pt_regs, then calls preserve_scratch_area. 
>preserve_scratch_area will reserve 1 new 16-byte area for next 
>call to ia64_bad_break. Later on, kprobe handler uses 
>pt_regs->cr_ifs and the saved ar.bsp to get the real ar.bsp 
>and parameter number. This approach also considers user space 
>probe. kprobe_save_bsp patch has no any impact on critical fault patch.
>The jprobe patch is to save the function parameters when the 
>jprobe is hit and restore them after break.

Yanmin,
	I am not sure whether using the scratch area below the pt_regs 
to save ar.bsp would be the right thing to do and more over it does look
like a hack.
Clean solution would be to have a field in pt_regs to save ar.bsp, until
we 
have it, here is what you can do for now.

In the setjmp_pre_handler() get the ar.bsp by unwinding the stack 
and then calling unw_get_bsp() to get the ar.bsp. Save this ar.bsp in
the kcb structure and later 
in longjmp_break_handler() you can use this ar.bsp without unwinding the
stack.

Let me know your thoughts and with you next version of the patch please
cc IA64 mailing list too.


Cheers,
Anil Keshavamurthy

RE: jprobe question

[Zhang, Yanmin]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

>>-----Original Message-----
>>From: Keshavamurthy Anil S [mailto:anil.s.keshavamurthy@intel.com]
>>Sent: 2005年11月30日 2:54
>>To: Zhang, Yanmin
>>Cc: systemtap@sources.redhat.com; Keshavamurthy, Anil S; Mao, Bibo
>>Subject: Re: jprobe question
>>
>>On Mon, Nov 28, 2005 at 07:53:12PM -0800, Zhang, Yanmin wrote:
>>>
>>>    Mostly,  jprobe  handler has parameters. If the parameters are changed
>>>    in  the  jprobe  handler, should the original function use the changed
>>>    values?
>>
>> Good question.
>>
>>>    My  answer is no. Because c compiler might change the parameter values
>>>    even though we don't change them in c codes sometimes.
>>>
>>>    What's your idea?
>>>
>>
>>You are correct, gcc assumes that the callee owns the argument space and
>>could overwrite it. If you see the code in function setjmp_pre_handler()
>>(for i386 & x86_64), we are saving this area and restoring it back before
>>passing the control back to the probed(original) function.
>>
>>For Ia64, I was under the assumption that this might not be the case, but
>>you proved it wrong. So for Ia64 also we need to implement the similar logic
>>of
>>saving and restoring the register stack space.
>>
>>Will open a bugzilla entry for this to track this bug.
>>
>>Patch welcome.

Here are the patches. In function non_syscall, I save the ar.bsp (after instruction cover) to the scratch area below pt_regs, then calls preserve_scratch_area. preserve_scratch_area will reserve 1 new 16-byte area for next call to ia64_bad_break. Later on, kprobe handler uses pt_regs->cr_ifs and the saved ar.bsp to get the real ar.bsp and parameter number. This approach also considers user space probe. kprobe_save_bsp patch has no any impact on critical fault patch.
The jprobe patch is to save the function parameters when the jprobe is hit and restore them after break.


[-- Attachment #2: jprobe_protect_out_reg_ia64.patch --]
[-- Type: application/octet-stream, Size: 2641 bytes --]

diff -Nraup linux-2.6.14_mm1/arch/ia64/kernel/kprobes.c linux-2.6.14_mm1_jprobe/arch/ia64/kernel/kprobes.c
--- linux-2.6.14_mm1/arch/ia64/kernel/kprobes.c	2005-11-09 19:08:51.000000000 +0800
+++ linux-2.6.14_mm1_jprobe/arch/ia64/kernel/kprobes.c	2005-11-24 16:24:07.000000000 +0800
@@ -764,9 +764,15 @@ int __kprobes setjmp_pre_handler(struct 
 	struct jprobe *jp = container_of(p, struct jprobe, kp);
 	unsigned long addr = ((struct fnptr *)(jp->entry))->ip;
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+	unsigned long *ar_bsp = (unsigned long *)*((unsigned long *)regs - 2);
+	int param_num = regs->cr_ifs & 0x3f;
 
 	/* save architectural state */
 	kcb->jprobe_saved_regs = *regs;
+	asm volatile("{\n\tflushrs\n\t\n\t\n\t}":::"memory");
+	memcpy( kcb->jprobes_saved_stacked_regs,
+		ia64_rse_skip_regs(ar_bsp, -param_num),
+		REAL_PARAM_RSE_SIZE(regs)*sizeof(unsigned long) );
 
 	/* after rfi, execute the jprobe instrumented function */
 	regs->cr_iip = addr & ~0xFULL;
@@ -785,8 +791,21 @@ int __kprobes setjmp_pre_handler(struct 
 int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 {
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+	unsigned long *ar_bsp = (unsigned long *)*((unsigned long *)regs - 2);
+	int param_num = regs->cr_ifs & 0x3f;
+	unsigned long rsc_save = 0;
 
 	*regs = kcb->jprobe_saved_regs;
+	asm volatile("{\n\tflushrs\n\t\n\t\n\t}":::"memory");
+	memcpy(	ia64_rse_skip_regs(ar_bsp, -param_num),
+		kcb->jprobes_saved_stacked_regs,
+		REAL_PARAM_RSE_SIZE(regs)*sizeof(unsigned long) );
+	asm volatile( "mov %0=ar.rsc;;\n\t"
+			"mov ar.rsc=0;;\n\t"
+			"{\n\tloadrs;;\n\t\n\t\n\t}\n\t"
+			"mov ar.rsc=%1\n\t"
+			:"=r" (rsc_save):"r" (rsc_save):"memory");
+
 	preempt_enable_no_resched();
 	return 1;
 }
diff -Nraup linux-2.6.14_mm1/include/asm-ia64/kprobes.h linux-2.6.14_mm1_jprobe/include/asm-ia64/kprobes.h
--- linux-2.6.14_mm1/include/asm-ia64/kprobes.h	2005-11-09 19:08:43.000000000 +0800
+++ linux-2.6.14_mm1_jprobe/include/asm-ia64/kprobes.h	2005-11-23 23:36:30.000000000 +0800
@@ -68,10 +68,19 @@ struct prev_kprobe {
 	unsigned long status;
 };
 
+#define	MAX_PARAM_RSE_SIZE	(0x80+0x80/0x3f)
+#define REAL_PARAM_RSE_SIZE(regs)	({	\
+			unsigned long *ar_bsp;					\
+			int param_num = regs->cr_ifs & 0x3f;			\
+			ar_bsp = (unsigned long *)*((unsigned long *)regs - 2);	\
+			ar_bsp - ia64_rse_skip_regs(ar_bsp, -param_num);	\
+		})
+
 /* per-cpu kprobe control block */
 struct kprobe_ctlblk {
 	unsigned long kprobe_status;
 	struct pt_regs jprobe_saved_regs;
+	unsigned long jprobes_saved_stacked_regs[MAX_PARAM_RSE_SIZE];
 	struct prev_kprobe prev_kprobe;
 };
 

[-- Attachment #3: kprobe_save_bsp_to_scratch_area_2.6.14_mm1.patch --]
[-- Type: application/octet-stream, Size: 2268 bytes --]

diff -Nraup linux-2.6.14_mm1/arch/ia64/kernel/ivt.S linux-2.6.14_mm1_bsp/arch/ia64/kernel/ivt.S
--- linux-2.6.14_mm1/arch/ia64/kernel/ivt.S	2005-11-09 19:08:51.000000000 +0800
+++ linux-2.6.14_mm1_bsp/arch/ia64/kernel/ivt.S	2005-11-24 01:04:03.000000000 +0800
@@ -1042,10 +1042,24 @@ END(dispatch_illegal_op_fault)
 	DBG_FAULT(17)
 	FAULT(17)
 
+ENTRY(preserve_scratch_area)
+	alloc r14=ar.pfs,0,0,2,0
+	adds r12=-16,r12
+	;;
+	br.call.sptk.many rp=ia64_bad_break	// avoid WAW on CFM
+	;;
+	movl r15=ia64_leave_kernel
+	adds r12=16,r12
+	;;
+	mov rp=r15
+	;;
+	br.ret.sptk.many rp
+END(preserve_scratch_area)
+
 ENTRY(non_syscall)
 	mov ar.rsc=r27			// restore ar.rsc before SAVE_MIN_WITH_COVER
 	;;
-	SAVE_MIN_WITH_COVER
+	SAVE_MIN_WITH_COVER_BSP
 
 	// There is no particular reason for this code to be here, other than that
 	// there happens to be space here that would go unused otherwise.  If this
@@ -1062,12 +1076,10 @@ ENTRY(non_syscall)
 	srlz.i				// guarantee that interruption collection is on
 	;;
 (p15)	ssm psr.i			// restore psr.i
-	movl r15=ia64_leave_kernel
 	;;
 	SAVE_REST
-	mov rp=r15
 	;;
-	br.call.sptk.many b6=ia64_bad_break	// avoid WAW on CFM and ignore return addr
+	br.call.sptk.many b6=preserve_scratch_area	// avoid WAW on CFM and ignore return addr
 END(non_syscall)
 
 	.org ia64_ivt+0x4800
diff -Nraup linux-2.6.14_mm1/arch/ia64/kernel/minstate.h linux-2.6.14_mm1_bsp/arch/ia64/kernel/minstate.h
--- linux-2.6.14_mm1/arch/ia64/kernel/minstate.h	2005-11-09 19:08:51.000000000 +0800
+++ linux-2.6.14_mm1_bsp/arch/ia64/kernel/minstate.h	2005-11-23 22:45:49.000000000 +0800
@@ -196,3 +196,21 @@
 #define SAVE_MIN_WITH_COVER	DO_SAVE_MIN(cover, mov r30=cr.ifs,)
 #define SAVE_MIN_WITH_COVER_R19	DO_SAVE_MIN(cover, mov r30=cr.ifs, mov r15=r19)
 #define SAVE_MIN		DO_SAVE_MIN(     , mov r30=r0, )
+
+#ifdef	CONFIG_KPROBES
+/*
+ * Here we save ar.bsp (after cover) to the scratch area below pt_regs for kprobe purpose.
+ * Kprobe uses it to locate the function parameters on RBS in kprobe handler.
+ *
+ */
+#define	EXTRA_SAVE_BSP				\
+	mov r23=ar.bsp				\
+	;;					\
+	st8 [r12]=r23
+
+#define	SAVE_MIN_WITH_COVER_BSP	DO_SAVE_MIN(cover, mov r30=cr.ifs, EXTRA_SAVE_BSP)
+
+#else
+#define	SAVE_MIN_WITH_COVER_BSP	SAVE_MIN_WITH_COVER
+#endif
+

Re: jprobe question

[Keshavamurthy Anil S]

On Mon, Nov 28, 2005 at 07:53:12PM -0800, Zhang, Yanmin wrote:
> 
>    Mostly,  jprobe  handler has parameters. If the parameters are changed
>    in  the  jprobe  handler, should the original function use the changed
>    values?

 Good question.

>    My  answer is no. Because c compiler might change the parameter values
>    even though we don't change them in c codes sometimes.
> 
>    What's your idea?
> 

You are correct, gcc assumes that the callee owns the argument space and
could overwrite it. If you see the code in function setjmp_pre_handler()
(for i386 & x86_64), we are saving this area and restoring it back before 
passing the control back to the probed(original) function.

For Ia64, I was under the assumption that this might not be the case, but
you proved it wrong. So for Ia64 also we need to implement the similar logic of
saving and restoring the register stack space.

Will open a bugzilla entry for this to track this bug.

Patch welcome.

Thanks,
-Anil Keshavamurthy