[PATCH v2 0/2] MIPS: ftrace: Fix icache flush issue

[PATCH v2 0/2] MIPS: ftrace: Fix icache flush issue

[Viller Hsiao]

In 32-bit mode, the start address of flushing icache is wrong because
of error address calculation. It causes system crash at boot when
dynamic function trace is enabled. This issue existed since linux-3.8.

In the patch set, I fixed the flushing range and refined the macros
used by it to pass compilation.

Patch 1 is tried to improve the usability of some macros such that
we can make patch 2 cleaner. Patch 2 fixes this issue.

This patch set is based on commit 7d3f1a5 of mips-for-linux-next branch.

Viller Hsiao (2):
  MIPS: ftrace: Tweak safe_load()/safe_store() macros
  MIPS: ftrace: Fix icache flush range error

 arch/mips/include/asm/ftrace.h | 20 ++++++++++----------
 arch/mips/kernel/ftrace.c      |  5 ++---
 2 files changed, 12 insertions(+), 13 deletions(-)

-- 
1.8.4.3

[PATCH v2 1/2] MIPS: ftrace: Tweak safe_load()/safe_store() macros

[Viller Hsiao]

Due to name collision in ftrace safe_load and safe_store macros,
these macros cannot take expressions as operands.

For example, compiler will complain for a macro call like the following:
  safe_store_code(new_code2, ip + 4, faulted);

  arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
     : [dst] "r" (dst), [src] "r" (src)\
        ^
  arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
    safe_store_code(new_code2, ip + 4, faulted);
    ^
  arch/mips/kernel/ftrace.c:118:32: error: undefined named operand 'ip + 4'
    safe_store_code(new_code2, ip + 4, faulted);
                                  ^
  arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
     : [dst] "r" (dst), [src] "r" (src)\
        ^
  arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
    safe_store_code(new_code2, ip + 4, faulted);
    ^

This patch tweaks variable naming in those macros to allow flexible
operands.

Signed-off-by: Viller Hsiao <villerhsiao@gmail.com>
---
 arch/mips/include/asm/ftrace.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/mips/include/asm/ftrace.h b/arch/mips/include/asm/ftrace.h
index ce35c9a..434321c 100644
--- a/arch/mips/include/asm/ftrace.h
+++ b/arch/mips/include/asm/ftrace.h
@@ -22,12 +22,12 @@ extern void _mcount(void);
 #define safe_load(load, src, dst, error)		\
 do {							\
 	asm volatile (					\
-		"1: " load " %[" STR(dst) "], 0(%[" STR(src) "])\n"\
-		"   li %[" STR(error) "], 0\n"		\
+		"1: " load " %[dest], 0(%[source])\n"	\
+		"   li %[err], 0\n"			\
 		"2:\n"					\
 							\
 		".section .fixup, \"ax\"\n"		\
-		"3: li %[" STR(error) "], 1\n"		\
+		"3: li %[err], 1\n"			\
 		"   j 2b\n"				\
 		".previous\n"				\
 							\
@@ -35,8 +35,8 @@ do {							\
 		STR(PTR) "\t1b, 3b\n\t"			\
 		".previous\n"				\
 							\
-		: [dst] "=&r" (dst), [error] "=r" (error)\
-		: [src] "r" (src)			\
+		: [dest] "=&r" (dst), [err] "=r" (error)\
+		: [source] "r" (src)			\
 		: "memory"				\
 	);						\
 } while (0)
@@ -44,12 +44,12 @@ do {							\
 #define safe_store(store, src, dst, error)	\
 do {						\
 	asm volatile (				\
-		"1: " store " %[" STR(src) "], 0(%[" STR(dst) "])\n"\
-		"   li %[" STR(error) "], 0\n"	\
+		"1: " store " %[source], 0(%[dest])\n"\
+		"   li %[err], 0\n"		\
 		"2:\n"				\
 						\
 		".section .fixup, \"ax\"\n"	\
-		"3: li %[" STR(error) "], 1\n"	\
+		"3: li %[err], 1\n"		\
 		"   j 2b\n"			\
 		".previous\n"			\
 						\
@@ -57,8 +57,8 @@ do {						\
 		STR(PTR) "\t1b, 3b\n\t"		\
 		".previous\n"			\
 						\
-		: [error] "=r" (error)		\
-		: [dst] "r" (dst), [src] "r" (src)\
+		: [err] "=r" (error)		\
+		: [dest] "r" (dst), [source] "r" (src)\
 		: "memory"			\
 	);					\
 } while (0)
-- 
1.8.4.3

Re: [PATCH v2 1/2] MIPS: ftrace: Tweak safe_load()/safe_store() macros

[Ralf Baechle]

On Sat, Feb 22, 2014 at 03:46:48PM +0800, Viller Hsiao wrote:

> Due to name collision in ftrace safe_load and safe_store macros,
> these macros cannot take expressions as operands.
> 
> For example, compiler will complain for a macro call like the following:
>   safe_store_code(new_code2, ip + 4, faulted);
> 
>   arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
>      : [dst] "r" (dst), [src] "r" (src)\
>         ^
>   arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
>     safe_store_code(new_code2, ip + 4, faulted);
>     ^
>   arch/mips/kernel/ftrace.c:118:32: error: undefined named operand 'ip + 4'
>     safe_store_code(new_code2, ip + 4, faulted);
>                                   ^
>   arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
>      : [dst] "r" (dst), [src] "r" (src)\
>         ^
>   arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
>     safe_store_code(new_code2, ip + 4, faulted);
>     ^
> 
> This patch tweaks variable naming in those macros to allow flexible
> operands.

Interesting catch - and while I think your patch indeed is an improvment
nobody seems to have observed this in a kernel tree, so I'm going to treat
this as a non-urgent improvment and queue it for 3.15.

If this can be triggered in any -stable or v3.14-rc7 tree, please let me
know.

Thanks,

  Ralf

Re: [PATCH v2 1/2] MIPS: ftrace: Tweak safe_load()/safe_store() macros

[Viller Hsiao]

On Mon, Mar 17, 2014 at 10:56 PM, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Sat, Feb 22, 2014 at 03:46:48PM +0800, Viller Hsiao wrote:
>
>> Due to name collision in ftrace safe_load and safe_store macros,
>> these macros cannot take expressions as operands.
>>
>> For example, compiler will complain for a macro call like the following:
>>   safe_store_code(new_code2, ip + 4, faulted);
>>
>>   arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
>>      : [dst] "r" (dst), [src] "r" (src)\
>>         ^
>>   arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
>>     safe_store_code(new_code2, ip + 4, faulted);
>>     ^
>>   arch/mips/kernel/ftrace.c:118:32: error: undefined named operand 'ip + 4'
>>     safe_store_code(new_code2, ip + 4, faulted);
>>                                   ^
>>   arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
>>      : [dst] "r" (dst), [src] "r" (src)\
>>         ^
>>   arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
>>     safe_store_code(new_code2, ip + 4, faulted);
>>     ^
>>
>> This patch tweaks variable naming in those macros to allow flexible
>> operands.
>
> Interesting catch - and while I think your patch indeed is an improvment
> nobody seems to have observed this in a kernel tree, so I'm going to treat
> this as a non-urgent improvment and queue it for 3.15.
>
> If this can be triggered in any -stable or v3.14-rc7 tree, please let me
> know.
>
> Thanks,
>
>   Ralf

Hi Ralf,

If you get plan to merge it later, please help to handle another patch
in the patchset at the same time,
   [PATCH v2 2/2] MIPS: ftrace: Fix icache flush range error

Or the 2nd one might have compilation error shown in this commit
message. Sorry for the inconvenience.

Viller

Re: [PATCH v2 1/2] MIPS: ftrace: Tweak safe_load()/safe_store() macros

[Viller Hsiao]

On Tue, Mar 18, 2014 at 8:04 AM, Viller Hsiao <villerhsiao@gmail.com> wrote:
> On Mon, Mar 17, 2014 at 10:56 PM, Ralf Baechle <ralf@linux-mips.org> wrote:
>> On Sat, Feb 22, 2014 at 03:46:48PM +0800, Viller Hsiao wrote:
>>
>>> Due to name collision in ftrace safe_load and safe_store macros,
>>> these macros cannot take expressions as operands.
>>>
>>> For example, compiler will complain for a macro call like the following:
>>>   safe_store_code(new_code2, ip + 4, faulted);
>>>
>>>   arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
>>>      : [dst] "r" (dst), [src] "r" (src)\
>>>         ^
>>>   arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
>>>     safe_store_code(new_code2, ip + 4, faulted);
>>>     ^
>>>   arch/mips/kernel/ftrace.c:118:32: error: undefined named operand 'ip + 4'
>>>     safe_store_code(new_code2, ip + 4, faulted);
>>>                                   ^
>>>   arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
>>>      : [dst] "r" (dst), [src] "r" (src)\
>>>         ^
>>>   arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
>>>     safe_store_code(new_code2, ip + 4, faulted);
>>>     ^
>>>
>>> This patch tweaks variable naming in those macros to allow flexible
>>> operands.
>>
>> Interesting catch - and while I think your patch indeed is an improvment
>> nobody seems to have observed this in a kernel tree, so I'm going to treat
>> this as a non-urgent improvment and queue it for 3.15.
>>
>> If this can be triggered in any -stable or v3.14-rc7 tree, please let me
>> know.
>>
>> Thanks,
>>
>>   Ralf
>
> Hi Ralf,
>
> If you get plan to merge it later, please help to handle another patch
> in the patchset at the same time,
>    [PATCH v2 2/2] MIPS: ftrace: Fix icache flush range error
>
> Or the 2nd one might have compilation error shown in this commit
> message. Sorry for the inconvenience.
>
> Viller

Hi Ralf,

The operand name and variable name are a little ambiguous in PATCH v2.
Therefore I tweaked and submitted PATCH v3. Please help to use them if
possible.

Thanks,
Viller

Re: [PATCH v2 1/2] MIPS: ftrace: Tweak safe_load()/safe_store() macros

[Ralf Baechle]

On Tue, Mar 18, 2014 at 03:53:00PM +0800, Viller Hsiao wrote:

> The operand name and variable name are a little ambiguous in PATCH v2.
> Therefore I tweaked and submitted PATCH v3. Please help to use them if
> possible.

Too late, the older versions of your patch were pulled by Linus
yesterday.  If you have further improvments beyond those version in
Linus' tree now, please submit a new patch.

Thanks!

  Ralf

Re: [PATCH v2 1/2] MIPS: ftrace: Tweak safe_load()/safe_store() macros

[Tony Wu]

On Tue, Mar 18, 2014 at 8:04 PM, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Tue, Mar 18, 2014 at 03:53:00PM +0800, Viller Hsiao wrote:
>
>> The operand name and variable name are a little ambiguous in PATCH v2.
>> Therefore I tweaked and submitted PATCH v3. Please help to use them if
>> possible.
>
> Too late, the older versions of your patch were pulled by Linus
> yesterday.  If you have further improvments beyond those version in
> Linus' tree now, please submit a new patch.
>
> Thanks!
>
>   Ralf
>

Ralf,

This patch is required for the other ftrace patch you cherry-picked to
linux-3.x-stable tree. The safe_store macros cannot take expression as
operand without this patch.

Thanks,
Tony

Re: [PATCH v2 1/2] MIPS: ftrace: Tweak safe_load()/safe_store() macros

[Geert Uytterhoeven]

Hi Ralf,

On Mon, Mar 17, 2014 at 3:56 PM, Ralf Baechle <ralf@linux-mips.org> wrote:
> On Sat, Feb 22, 2014 at 03:46:48PM +0800, Viller Hsiao wrote:
>
>> Due to name collision in ftrace safe_load and safe_store macros,
>> these macros cannot take expressions as operands.
>>
>> For example, compiler will complain for a macro call like the following:
>>   safe_store_code(new_code2, ip + 4, faulted);
>>
>>   arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
>>      : [dst] "r" (dst), [src] "r" (src)\
>>         ^
>>   arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
>>     safe_store_code(new_code2, ip + 4, faulted);
>>     ^
>>   arch/mips/kernel/ftrace.c:118:32: error: undefined named operand 'ip + 4'
>>     safe_store_code(new_code2, ip + 4, faulted);
>>                                   ^
>>   arch/mips/include/asm/ftrace.h:61:6: note: in definition of macro 'safe_store'
>>      : [dst] "r" (dst), [src] "r" (src)\
>>         ^
>>   arch/mips/kernel/ftrace.c:118:2: note: in expansion of macro 'safe_store_code'
>>     safe_store_code(new_code2, ip + 4, faulted);
>>     ^
>>
>> This patch tweaks variable naming in those macros to allow flexible
>> operands.
>
> Interesting catch - and while I think your patch indeed is an improvment
> nobody seems to have observed this in a kernel tree, so I'm going to treat
> this as a non-urgent improvment and queue it for 3.15.
>
> If this can be triggered in any -stable or v3.14-rc7 tree, please let me
> know.

Mips/allmodconfig started to fail _after_ v3.14-rc7:

http://kisskb.ellerman.id.au/kisskb/buildresult/10807340/

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 2/2] MIPS: ftrace: Fix icache flush range error

[Viller Hsiao]

In 32-bit mode, the start address passed to flush_icache_range is
shifted by 4 bytes before the second safe_store_code() call.

This causes system crash from time to time because the first 4 bytes
might not be flushed properly. This bug exists since linux-3.8.

Also remove obsoleted comment while at it.

Signed-off-by: Viller Hsiao <villerhsiao@gmail.com>
---
 arch/mips/kernel/ftrace.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index ddcc350..74fe735 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -115,11 +115,10 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1,
 	safe_store_code(new_code1, ip, faulted);
 	if (unlikely(faulted))
 		return -EFAULT;
-	ip += 4;
-	safe_store_code(new_code2, ip, faulted);
+	safe_store_code(new_code2, ip + 4, faulted);
 	if (unlikely(faulted))
 		return -EFAULT;
-	flush_icache_range(ip, ip + 8); /* original ip + 12 */
+	flush_icache_range(ip, ip + 8);
 	return 0;
 }
 #endif
-- 
1.8.4.3

RE: [PATCH v2 0/2] MIPS: ftrace: Fix icache flush issue

[Qais Yousef]

> -----Original Message-----
> From: Viller Hsiao [mailto:villerhsiao@gmail.com]
> Sent: 22 February 2014 07:47
> To: linux-mips@linux-mips.org
> Cc: rostedt@goodmis.org; fweisbec@gmail.com; mingo@redhat.com; ralf@linux-
> mips.org; Qais Yousef; Viller Hsiao
> Subject: [PATCH v2 0/2] MIPS: ftrace: Fix icache flush issue
> 
> In 32-bit mode, the start address of flushing icache is wrong because of error
> address calculation. It causes system crash at boot when dynamic function trace is
> enabled. This issue existed since linux-3.8.
> 
> In the patch set, I fixed the flushing range and refined the macros used by it to
> pass compilation.
> 
> Patch 1 is tried to improve the usability of some macros such that we can make
> patch 2 cleaner. Patch 2 fixes this issue.
> 
> This patch set is based on commit 7d3f1a5 of mips-for-linux-next branch.
> 
> Viller Hsiao (2):
>   MIPS: ftrace: Tweak safe_load()/safe_store() macros
>   MIPS: ftrace: Fix icache flush range error
> 

Both patches look good to me. Thanks for the fixes.

Reviewed-by: Qais Yousef <qais.yousef@imgtec.com>

Qais

>  arch/mips/include/asm/ftrace.h | 20 ++++++++++----------
>  arch/mips/kernel/ftrace.c      |  5 ++---
>  2 files changed, 12 insertions(+), 13 deletions(-)
> 
> --
> 1.8.4.3