* [PATCH] MIPS: ftrace: Fix icache flush range error
@ 2014-02-19 14:54 Viller Hsiao
2014-02-19 15:51 ` Qais Yousef
0 siblings, 1 reply; 4+ messages in thread
From: Viller Hsiao @ 2014-02-19 14:54 UTC (permalink / raw)
To: linux-mips
Cc: Viller Hsiao, Steven Rostedt, Frederic Weisbecker, Ingo Molnar,
Ralf Baechle
In 32-bit machine, the start address of flushing icache is wrong after
calculated address of 2nd modified instruction in function tracer. The
start address is shifted 4 bytes from ordinary calculation.
This causes problem when the address of 1st instruction is the last
word of one cache line. It will not be flushed at this case.
Signed-off-by: Viller Hsiao <villerhsiao@gmail.com>
---
arch/mips/kernel/ftrace.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
index 185ba25..5bdc535 100644
--- a/arch/mips/kernel/ftrace.c
+++ b/arch/mips/kernel/ftrace.c
@@ -107,12 +107,12 @@ static int ftrace_modify_code_2(unsigned long ip, unsigned int new_code1,
unsigned int new_code2)
{
int faulted;
+ unsigned long ip2 = ip + 4;
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, ip2, faulted);
if (unlikely(faulted))
return -EFAULT;
flush_icache_range(ip, ip + 8); /* original ip + 12 */
--
1.8.4.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH] MIPS: ftrace: Fix icache flush range error
2014-02-19 14:54 [PATCH] MIPS: ftrace: Fix icache flush range error Viller Hsiao
@ 2014-02-19 15:51 ` Qais Yousef
2014-02-20 2:53 ` Viller Hsiao
0 siblings, 1 reply; 4+ messages in thread
From: Qais Yousef @ 2014-02-19 15:51 UTC (permalink / raw)
To: Viller Hsiao
Cc: Steven Rostedt, Frederic Weisbecker, Ingo Molnar, Ralf Baechle,
linux-mips
> -----Original Message-----
> From: linux-mips-bounce@linux-mips.org [mailto:linux-mips-bounce@linux-
> mips.org] On Behalf Of Viller Hsiao
> Sent: 19 February 2014 14:55
> To: linux-mips@linux-mips.org
> Cc: Viller Hsiao; Steven Rostedt; Frederic Weisbecker; Ingo Molnar; Ralf Baechle
> Subject: [PATCH] MIPS: ftrace: Fix icache flush range error
>
>
> In 32-bit machine, the start address of flushing icache is wrong after calculated
> address of 2nd modified instruction in function tracer. The start address is shifted
> 4 bytes from ordinary calculation.
>
> This causes problem when the address of 1st instruction is the last word of one
> cache line. It will not be flushed at this case.
>
> Signed-off-by: Viller Hsiao <villerhsiao@gmail.com>
> ---
> arch/mips/kernel/ftrace.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c index
> 185ba25..5bdc535 100644
> --- a/arch/mips/kernel/ftrace.c
> +++ b/arch/mips/kernel/ftrace.c
> @@ -107,12 +107,12 @@ static int ftrace_modify_code_2(unsigned long ip,
> unsigned int new_code1,
> unsigned int new_code2)
> {
> int faulted;
> + unsigned long ip2 = ip + 4;
I think better to omit this variable...
>
> 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, ip2, faulted);
And just do the addition directly here instead.
> if (unlikely(faulted))
> return -EFAULT;
> flush_icache_range(ip, ip + 8); /* original ip + 12 */
Care to fix this comment by removing it? I can't rationalise it and made me confused for a bit.
If you do remove it please mention the change in the commit message.
Nice catch by the way.
Cheers,
Qais
> --
> 1.8.4.3
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] MIPS: ftrace: Fix icache flush range error
2014-02-19 15:51 ` Qais Yousef
@ 2014-02-20 2:53 ` Viller Hsiao
2014-02-20 9:39 ` Qais Yousef
0 siblings, 1 reply; 4+ messages in thread
From: Viller Hsiao @ 2014-02-20 2:53 UTC (permalink / raw)
To: Qais Yousef
Cc: Steven Rostedt, Frederic Weisbecker, Ingo Molnar, Ralf Baechle,
linux-mips
On Wed, Feb 19, 2014 at 11:51 PM, Qais Yousef <Qais.Yousef@imgtec.com> wrote:
>> -----Original Message-----
>> From: linux-mips-bounce@linux-mips.org [mailto:linux-mips-bounce@linux-
>> mips.org] On Behalf Of Viller Hsiao
>> Sent: 19 February 2014 14:55
>> To: linux-mips@linux-mips.org
>> Cc: Viller Hsiao; Steven Rostedt; Frederic Weisbecker; Ingo Molnar; Ralf Baechle
>> Subject: [PATCH] MIPS: ftrace: Fix icache flush range error
>>
>>
>> In 32-bit machine, the start address of flushing icache is wrong after calculated
>> address of 2nd modified instruction in function tracer. The start address is shifted
>> 4 bytes from ordinary calculation.
>>
>> This causes problem when the address of 1st instruction is the last word of one
>> cache line. It will not be flushed at this case.
>>
>> Signed-off-by: Viller Hsiao <villerhsiao@gmail.com>
>> ---
>> arch/mips/kernel/ftrace.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c index
>> 185ba25..5bdc535 100644
>> --- a/arch/mips/kernel/ftrace.c
>> +++ b/arch/mips/kernel/ftrace.c
>> @@ -107,12 +107,12 @@ static int ftrace_modify_code_2(unsigned long ip,
>> unsigned int new_code1,
>> unsigned int new_code2)
>> {
>> int faulted;
>> + unsigned long ip2 = ip + 4;
>
> I think better to omit this variable...
>
>>
>> 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, ip2, faulted);
>
> And just do the addition directly here instead.
>
Replacing ip2 to (ip + 4) causes compilation error because of the same
naming of symbolic operand and its input variable in safe_store().
----
arch/mips/kernel/ftrace.c:114:29: error: expected identifier before '(' token
safe_store_code(new_code2, (ip + 4), faulted);
^
/home/villerhsiao/official/linux-torvalds/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:114:2: note: in expansion of macro 'safe_store_code'
safe_store_code(new_code2, (ip + 4), faulted);
^
/home/villerhsiao/official/linux-torvalds/arch/mips/include/asm/ftrace.h:61:11:
error: expected ':' or ')' before string constant
: [dst] "r" (dst), [src] "r" (src)\
^
/home/villerhsiao/official/linux-torvalds/arch/mips/include/asm/ftrace.h:69:2:
note: in expansion of macro 'safe_store'
safe_store(STR(sw), src, dst, error)
^
arch/mips/kernel/ftrace.c:114:2: note: in expansion of macro 'safe_store_code'
safe_store_code(new_code2, (ip + 4), faulted);
^
----
If so, I will suggest to add the following patch to resolve it, (not
verified yet and I will do it before upload)
diff --git a/arch/mips/include/asm/ftrace.h b/arch/mips/include/asm/ftrace.h
index ce35c9a..ec031e8 100644
--- a/arch/mips/include/asm/ftrace.h
+++ b/arch/mips/include/asm/ftrace.h
@@ -19,15 +19,15 @@
extern void _mcount(void);
#define mcount _mcount
-#define safe_load(load, src, dst, error) \
+#define safe_load(load, source, dest, error) \
do { \
asm volatile ( \
"1: " load " %[" STR(dst) "], 0(%[" STR(src) "])\n"\
- " li %[" STR(error) "], 0\n" \
+ " li %[" STR(err) "], 0\n" \
"2:\n" \
\
".section .fixup, \"ax\"\n" \
- "3: li %[" STR(error) "], 1\n" \
+ "3: li %[" STR(err) "], 1\n" \
" j 2b\n" \
".previous\n" \
\
@@ -35,21 +35,21 @@ do { \
STR(PTR) "\t1b, 3b\n\t" \
".previous\n" \
\
- : [dst] "=&r" (dst), [error] "=r" (error)\
- : [src] "r" (src) \
+ : [dst] "=&r" (dest), [err] "=r" (error)\
+ : [src] "r" (source) \
: "memory" \
); \
} while (0)
-#define safe_store(store, src, dst, error) \
+#define safe_store(store, source, dest, error) \
do { \
asm volatile ( \
"1: " store " %[" STR(src) "], 0(%[" STR(dst) "])\n"\
- " li %[" STR(error) "], 0\n" \
+ " li %[" STR(err) "], 0\n" \
"2:\n" \
\
".section .fixup, \"ax\"\n" \
- "3: li %[" STR(error) "], 1\n" \
+ "3: li %[" STR(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) \
+ : [dst] "r" (dest), [src] "r" (source)\
: "memory" \
); \
} while (0)
>> if (unlikely(faulted))
>> return -EFAULT;
>> flush_icache_range(ip, ip + 8); /* original ip + 12 */
>
> Care to fix this comment by removing it? I can't rationalise it and made me confused for a bit.
> If you do remove it please mention the change in the commit message.
Ok. I will check it.
>
> Nice catch by the way.
>
> Cheers,
> Qais
>
>> --
>> 1.8.4.3
>>
>
Best Regards,
Viller
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH] MIPS: ftrace: Fix icache flush range error
2014-02-20 2:53 ` Viller Hsiao
@ 2014-02-20 9:39 ` Qais Yousef
0 siblings, 0 replies; 4+ messages in thread
From: Qais Yousef @ 2014-02-20 9:39 UTC (permalink / raw)
To: Viller Hsiao
Cc: Steven Rostedt, Frederic Weisbecker, Ingo Molnar, Ralf Baechle,
linux-mips
> -----Original Message-----
> From: Viller Hsiao [mailto:villerhsiao@gmail.com]
> Sent: 20 February 2014 02:54
> To: Qais Yousef
> Cc: Steven Rostedt; Frederic Weisbecker; Ingo Molnar; Ralf Baechle; linux-
> mips@linux-mips.org
> Subject: Re: [PATCH] MIPS: ftrace: Fix icache flush range error
>
> On Wed, Feb 19, 2014 at 11:51 PM, Qais Yousef <Qais.Yousef@imgtec.com>
> wrote:
> >> -----Original Message-----
> >> From: linux-mips-bounce@linux-mips.org
> >> [mailto:linux-mips-bounce@linux- mips.org] On Behalf Of Viller Hsiao
> >> Sent: 19 February 2014 14:55
> >> To: linux-mips@linux-mips.org
> >> Cc: Viller Hsiao; Steven Rostedt; Frederic Weisbecker; Ingo Molnar;
> >> Ralf Baechle
> >> Subject: [PATCH] MIPS: ftrace: Fix icache flush range error
> >>
> >>
> >> In 32-bit machine, the start address of flushing icache is wrong
> >> after calculated address of 2nd modified instruction in function
> >> tracer. The start address is shifted
> >> 4 bytes from ordinary calculation.
> >>
> >> This causes problem when the address of 1st instruction is the last
> >> word of one cache line. It will not be flushed at this case.
> >>
> >> Signed-off-by: Viller Hsiao <villerhsiao@gmail.com>
> >> ---
> >> arch/mips/kernel/ftrace.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
> >> index
> >> 185ba25..5bdc535 100644
> >> --- a/arch/mips/kernel/ftrace.c
> >> +++ b/arch/mips/kernel/ftrace.c
> >> @@ -107,12 +107,12 @@ static int ftrace_modify_code_2(unsigned long
> >> ip, unsigned int new_code1,
> >> unsigned int new_code2) {
> >> int faulted;
> >> + unsigned long ip2 = ip + 4;
> >
> > I think better to omit this variable...
> >
> >>
> >> 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, ip2, faulted);
> >
> > And just do the addition directly here instead.
> >
>
> Replacing ip2 to (ip + 4) causes compilation error because of the same
> naming of symbolic operand and its input variable in safe_store().
> ----
> arch/mips/kernel/ftrace.c:114:29: error: expected identifier before '(' token
> safe_store_code(new_code2, (ip + 4), faulted);
> ^
> /home/villerhsiao/official/linux-torvalds/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:114:2: note: in expansion of macro 'safe_store_code'
> safe_store_code(new_code2, (ip + 4), faulted);
> ^
> /home/villerhsiao/official/linux-torvalds/arch/mips/include/asm/ftrace.h:61:11:
> error: expected ':' or ')' before string constant
> : [dst] "r" (dst), [src] "r" (src)\
> ^
> /home/villerhsiao/official/linux-torvalds/arch/mips/include/asm/ftrace.h:69:2:
> note: in expansion of macro 'safe_store'
> safe_store(STR(sw), src, dst, error)
> ^
> arch/mips/kernel/ftrace.c:114:2: note: in expansion of macro 'safe_store_code'
> safe_store_code(new_code2, (ip + 4), faulted);
> ^
> ----
> If so, I will suggest to add the following patch to resolve it, (not
> verified yet and I will do it before upload)
>
It looks good to me generally. Please apply this patch before your original fix and resend.
Thanks,
Qais
> diff --git a/arch/mips/include/asm/ftrace.h b/arch/mips/include/asm/ftrace.h
> index ce35c9a..ec031e8 100644
> --- a/arch/mips/include/asm/ftrace.h
> +++ b/arch/mips/include/asm/ftrace.h
> @@ -19,15 +19,15 @@
> extern void _mcount(void);
> #define mcount _mcount
>
> -#define safe_load(load, src, dst, error) \
> +#define safe_load(load, source, dest, error) \
> do { \
> asm volatile ( \
> "1: " load " %[" STR(dst) "], 0(%[" STR(src) "])\n"\
> - " li %[" STR(error) "], 0\n" \
> + " li %[" STR(err) "], 0\n" \
> "2:\n" \
> \
> ".section .fixup, \"ax\"\n" \
> - "3: li %[" STR(error) "], 1\n" \
> + "3: li %[" STR(err) "], 1\n" \
> " j 2b\n" \
> ".previous\n" \
> \
> @@ -35,21 +35,21 @@ do { \
> STR(PTR) "\t1b, 3b\n\t" \
> ".previous\n" \
> \
> - : [dst] "=&r" (dst), [error] "=r" (error)\
> - : [src] "r" (src) \
> + : [dst] "=&r" (dest), [err] "=r" (error)\
> + : [src] "r" (source) \
> : "memory" \
> ); \
> } while (0)
>
> -#define safe_store(store, src, dst, error) \
> +#define safe_store(store, source, dest, error) \
> do { \
> asm volatile ( \
> "1: " store " %[" STR(src) "], 0(%[" STR(dst) "])\n"\
> - " li %[" STR(error) "], 0\n" \
> + " li %[" STR(err) "], 0\n" \
> "2:\n" \
> \
> ".section .fixup, \"ax\"\n" \
> - "3: li %[" STR(error) "], 1\n" \
> + "3: li %[" STR(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) \
> + : [dst] "r" (dest), [src] "r" (source)\
> : "memory" \
> ); \
> } while (0)
>
> >> if (unlikely(faulted))
> >> return -EFAULT;
> >> flush_icache_range(ip, ip + 8); /* original ip + 12 */
> >
> > Care to fix this comment by removing it? I can't rationalise it and made me
> confused for a bit.
> > If you do remove it please mention the change in the commit message.
> Ok. I will check it.
>
> >
> > Nice catch by the way.
> >
> > Cheers,
> > Qais
> >
> >> --
> >> 1.8.4.3
> >>
> >
>
> Best Regards,
> Viller
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-02-20 9:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-02-19 14:54 [PATCH] MIPS: ftrace: Fix icache flush range error Viller Hsiao
2014-02-19 15:51 ` Qais Yousef
2014-02-20 2:53 ` Viller Hsiao
2014-02-20 9:39 ` Qais Yousef
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.