> -----Original Message----- > From: Viller Hsiao [mailto:villerhsiao@xxxxxxxxx] > Sent: 20 February 2014 02:54 > To: Qais Yousef > Cc: Steven Rostedt; Frederic Weisbecker; Ingo Molnar; Ralf Baechle; linux- > mips@xxxxxxxxxxxxxx > Subject: Re: [PATCH] MIPS: ftrace: Fix icache flush range error > > On Wed, Feb 19, 2014 at 11:51 PM, Qais Yousef <Qais.Yousef@xxxxxxxxxx> > wrote: > >> -----Original Message----- > >> From: linux-mips-bounce@xxxxxxxxxxxxxx > >> [mailto:linux-mips-bounce@linux- mips.org] On Behalf Of Viller Hsiao > >> Sent: 19 February 2014 14:55 > >> To: linux-mips@xxxxxxxxxxxxxx > >> 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@xxxxxxxxx> > >> --- > >> 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
