Patchwork [powerpc] fix unpaired __trace_hcall_entry and __trace_hcall_exit

login
register
mail settings
Submitter Li Zhong
Date Dec. 19, 2011, 2:06 a.m.
Message ID <1324260407.3006.17.camel@ThinkPad-T61>
Download mbox | patch
Permalink /patch/132148/
State Superseded
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Li Zhong - Dec. 19, 2011, 2:06 a.m.
Unpaired calling of __trace_hcall_entry and __trace_hcall_exit could
cause incorrect preempt count. And it might happen as the global
variable hcall_tracepoint_refcount is checked separately before calling
them. 

I don't know much about the powerpc arch. But the idea here is to store
the hcall_tracepoint_refcount locally, so __trace_hcall_entry and
__trace_hcall_exit will be called or not called in pair by checking the
same value. 

Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/pseries/hvCall.S |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)
Benjamin Herrenschmidt - Jan. 3, 2012, 12:54 a.m.
On Mon, 2011-12-19 at 10:06 +0800, Li Zhong wrote:
> Unpaired calling of __trace_hcall_entry and __trace_hcall_exit could
> cause incorrect preempt count. And it might happen as the global
> variable hcall_tracepoint_refcount is checked separately before calling
> them. 
> 
> I don't know much about the powerpc arch. But the idea here is to store
> the hcall_tracepoint_refcount locally, so __trace_hcall_entry and
> __trace_hcall_exit will be called or not called in pair by checking the
> same value. 

Can you re-use an existing spot in the stack frame rather than adding
this entry ? There's plenty of unused spots :-) For example the link
editor doubleword is never going to be used in that function, we could
hijack it safely. It would make the patch (and the code) simpler.

Cheers,
Ben.

> Reported-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/hvCall.S |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/hvCall.S b/arch/powerpc/platforms/pseries/hvCall.S
> index fd05fde..1240bd2 100644
> --- a/arch/powerpc/platforms/pseries/hvCall.S
> +++ b/arch/powerpc/platforms/pseries/hvCall.S
> @@ -14,6 +14,7 @@
>  #include <asm/ptrace.h>
>  	
>  #define STK_PARM(i)     (48 + ((i)-3)*8)
> +#define REG_SIZE        (2*8)
>  
>  #ifdef CONFIG_TRACEPOINTS
>  
> @@ -32,11 +33,12 @@ hcall_tracepoint_refcount:
>   * unconditional cpu feature.
>   */
>  #define HCALL_INST_PRECALL(FIRST_REG)				\
> +	std	r31,-8(r1);					\
>  BEGIN_FTR_SECTION;						\
>  	b	1f;						\
>  END_FTR_SECTION(0, 1);						\
> -	ld      r12,hcall_tracepoint_refcount@toc(r2);		\
> -	cmpdi	r12,0;						\
> +	ld      r31,hcall_tracepoint_refcount@toc(r2);		\
> +	cmpdi	r31,0;						\
>  	beq+	1f;						\
>  	mflr	r0;						\
>  	std	r3,STK_PARM(r3)(r1);				\
> @@ -49,9 +51,9 @@ END_FTR_SECTION(0, 1);						\
>  	std	r10,STK_PARM(r10)(r1);				\
>  	std	r0,16(r1);					\
>  	addi	r4,r1,STK_PARM(FIRST_REG);			\
> -	stdu	r1,-STACK_FRAME_OVERHEAD(r1);			\
> +	stdu	r1,-STACK_FRAME_OVERHEAD-REG_SIZE(r1);		\
>  	bl	.__trace_hcall_entry;				\
> -	addi	r1,r1,STACK_FRAME_OVERHEAD;			\
> +	addi	r1,r1,STACK_FRAME_OVERHEAD+REG_SIZE;		\
>  	ld	r0,16(r1);					\
>  	ld	r3,STK_PARM(r3)(r1);				\
>  	ld	r4,STK_PARM(r4)(r1);				\
> @@ -74,8 +76,7 @@ END_FTR_SECTION(0, 1);						\
>  BEGIN_FTR_SECTION;						\
>  	b	1f;						\
>  END_FTR_SECTION(0, 1);						\
> -	ld      r12,hcall_tracepoint_refcount@toc(r2);		\
> -	cmpdi	r12,0;						\
> +	cmpdi	r31,0;						\
>  	beq+	1f;						\
>  	mflr	r0;						\
>  	ld	r6,STK_PARM(r3)(r1);				\
> @@ -83,13 +84,14 @@ END_FTR_SECTION(0, 1);						\
>  	mr	r4,r3;						\
>  	mr	r3,r6;						\
>  	std	r0,16(r1);					\
> -	stdu	r1,-STACK_FRAME_OVERHEAD(r1);			\
> +	stdu	r1,-STACK_FRAME_OVERHEAD-REG_SIZE(r1);		\
>  	bl	.__trace_hcall_exit;				\
> -	addi	r1,r1,STACK_FRAME_OVERHEAD;			\
> +	addi	r1,r1,STACK_FRAME_OVERHEAD+REG_SIZE;		\
>  	ld	r0,16(r1);					\
>  	ld	r3,STK_PARM(r3)(r1);				\
>  	mtlr	r0;						\
> -1:
> +1:								\
> +	ld	r31,-8(r1);
>  
>  #define HCALL_INST_POSTCALL_NORETS				\
>  	li	r5,0;						\

Patch

diff --git a/arch/powerpc/platforms/pseries/hvCall.S b/arch/powerpc/platforms/pseries/hvCall.S
index fd05fde..1240bd2 100644
--- a/arch/powerpc/platforms/pseries/hvCall.S
+++ b/arch/powerpc/platforms/pseries/hvCall.S
@@ -14,6 +14,7 @@ 
 #include <asm/ptrace.h>
 	
 #define STK_PARM(i)     (48 + ((i)-3)*8)
+#define REG_SIZE        (2*8)
 
 #ifdef CONFIG_TRACEPOINTS
 
@@ -32,11 +33,12 @@  hcall_tracepoint_refcount:
  * unconditional cpu feature.
  */
 #define HCALL_INST_PRECALL(FIRST_REG)				\
+	std	r31,-8(r1);					\
 BEGIN_FTR_SECTION;						\
 	b	1f;						\
 END_FTR_SECTION(0, 1);						\
-	ld      r12,hcall_tracepoint_refcount@toc(r2);		\
-	cmpdi	r12,0;						\
+	ld      r31,hcall_tracepoint_refcount@toc(r2);		\
+	cmpdi	r31,0;						\
 	beq+	1f;						\
 	mflr	r0;						\
 	std	r3,STK_PARM(r3)(r1);				\
@@ -49,9 +51,9 @@  END_FTR_SECTION(0, 1);						\
 	std	r10,STK_PARM(r10)(r1);				\
 	std	r0,16(r1);					\
 	addi	r4,r1,STK_PARM(FIRST_REG);			\
-	stdu	r1,-STACK_FRAME_OVERHEAD(r1);			\
+	stdu	r1,-STACK_FRAME_OVERHEAD-REG_SIZE(r1);		\
 	bl	.__trace_hcall_entry;				\
-	addi	r1,r1,STACK_FRAME_OVERHEAD;			\
+	addi	r1,r1,STACK_FRAME_OVERHEAD+REG_SIZE;		\
 	ld	r0,16(r1);					\
 	ld	r3,STK_PARM(r3)(r1);				\
 	ld	r4,STK_PARM(r4)(r1);				\
@@ -74,8 +76,7 @@  END_FTR_SECTION(0, 1);						\
 BEGIN_FTR_SECTION;						\
 	b	1f;						\
 END_FTR_SECTION(0, 1);						\
-	ld      r12,hcall_tracepoint_refcount@toc(r2);		\
-	cmpdi	r12,0;						\
+	cmpdi	r31,0;						\
 	beq+	1f;						\
 	mflr	r0;						\
 	ld	r6,STK_PARM(r3)(r1);				\
@@ -83,13 +84,14 @@  END_FTR_SECTION(0, 1);						\
 	mr	r4,r3;						\
 	mr	r3,r6;						\
 	std	r0,16(r1);					\
-	stdu	r1,-STACK_FRAME_OVERHEAD(r1);			\
+	stdu	r1,-STACK_FRAME_OVERHEAD-REG_SIZE(r1);		\
 	bl	.__trace_hcall_exit;				\
-	addi	r1,r1,STACK_FRAME_OVERHEAD;			\
+	addi	r1,r1,STACK_FRAME_OVERHEAD+REG_SIZE;		\
 	ld	r0,16(r1);					\
 	ld	r3,STK_PARM(r3)(r1);				\
 	mtlr	r0;						\
-1:
+1:								\
+	ld	r31,-8(r1);
 
 #define HCALL_INST_POSTCALL_NORETS				\
 	li	r5,0;						\