On Sat, Oct 23, 2010 at 5:31 AM, David Daney <ddaney@xxxxxxxxxxxxxxxxxx> wrote:
> On 10/22/2010 01:58 PM, Wu Zhangjin wrote:
>>
>> From: Wu Zhangjin<wuzhangjin@xxxxxxxxx>
>>
>> The module space may be the same as the kernel space sometimes(with
>> related kernel config and gcc options), but the current in_module()
>> assume they are always different, so, it should be fixed.
>>
>> As we know, for the limitation of the fixed 32bit long instruction of
>> MIPS, the "jal target" can only jump to an address whose offset from the
>> jal instruction is smaller than 2^28=256M, which means if the address is
>> in kernel space(the kernel image should be always smaller than 256M), no
>> long call is needed to jump from the address to _mcount, and further, if
>> the module use the same space as kernel space, the situation for module
>> will be the same as the kernel. but currently for MIPS, in most of the
>> situations, module has different space(0xc0000000) from the kernel
>> space(0x80000000) and the offset is bigger than 256M, a register is
>> needed to load the address of _mcount and another instruction "jal
>> <register>" is needed to jump from an address to _mcount.
>>
>> The above can be explained as:
>>
>> if (in_kernel_space(addr)) {
>> jal _mcount;
>> } else {
>> load the address of _mcount to a register
>> jalr<register>
>> }
>>
>
> Why can't we just do code examination to determine which case we are
> patching? There is a very limited and fixed set of instruction sequences
> that GCC will emit for _mcount calls. We can easily check for all of them.
>
> The mcount call sequence unconditionally emits:
>
> MOVE $1,$31
>
> In the JALR case (for -mlong-call), the instruction immediately before this
> will have the target register be '$3'.
>
Yes, we can, but we care about the performance here:
The current in_kernel_space() only need to check the address itself
but checking the code at (address-4) need an extra memory access .
Because ftrace_make_{nop,call} may be called to cope with a large
number of _mcount calling sites, considering the low frequency of the
MIPS, using a working and light-weight method here is more important,
so ... why not checking the address directly and the point is it works
well.
> I would rather directly check which case we are patching rather than trying
> to infer it from the address.
>
> Is there any case where a 'stock' kernel currently fails? If not, this
> patch seems like unneeded code churn.
Hmm, not found such a failure yet, but theoretically the failure may
happen for the old implementation(The in_module() in
arch/mips/kernel/ftrace.c) had the assumption of module would always
be compiled with -mlong-calls, but the method(in_kernel_space) used in
this patch discards the assumption and therefore it is more generic.
Thanks,
Wu Zhangjin
>
>
>
>> As we can see, the above explanation covers all of the situations well
>> and reflect the reality, so, we decide to add a new in_kernel_space()
>> instead of the old in_module().
>>
>> Based on core_kernel_text() from kernel/extable.c, in_kernel_space() is
>> easily to be added. Because all of the functions in the init sections is
>> anotated with notrace, so, differ from core_kernel_text(),
>> in_kernel_space() doesn't care about init section.
>>
>> With this new in_kernel_space(), the ftrace_make_{nop,call} can be
>> explained as following.
>>
>> ftrace_make_call() ftrace_make_nop()
>>
>> if (in_kernel_space(addr)) {
>> jal _mcount; (jal ftrace_caller) <--> nop
>> } else {
>> load the address of _mcount to a register<--> b label
>> jalr<register>
>> label:
>> }
>>
>> Signed-off-by: Wu Zhangjin<wuzhangjin@xxxxxxxxx>
>> ---
>> arch/mips/kernel/ftrace.c | 66
>> ++++++++++++++++++++++++--------------------
>> 1 files changed, 36 insertions(+), 30 deletions(-)
>>
>> diff --git a/arch/mips/kernel/ftrace.c b/arch/mips/kernel/ftrace.c
>> index 65f1949..6ff5a54 100644
>> --- a/arch/mips/kernel/ftrace.c
>> +++ b/arch/mips/kernel/ftrace.c
>> @@ -17,21 +17,7 @@
>> #include<asm/cacheflush.h>
>> #include<asm/uasm.h>
>>
>> -/*
>> - * If the Instruction Pointer is in module space (0xc0000000), return
>> true;
>> - * otherwise, it is in kernel space (0x80000000), return false.
>> - *
>> - * FIXME: This will not work when the kernel space and module space are
>> the
>> - * same. If they are the same, we need to modify scripts/recordmcount.pl,
>> - * ftrace_make_nop/call() and the other related parts to ensure the
>> - * enabling/disabling of the calling site to _mcount is right for both
>> kernel
>> - * and module.
>> - */
>> -
>> -static inline int in_module(unsigned long ip)
>> -{
>> - return ip& 0x40000000;
>> -}
>> +#include<asm-generic/sections.h>
>>
>> #ifdef CONFIG_DYNAMIC_FTRACE
>>
>> @@ -69,6 +55,20 @@ static inline void ftrace_dyn_arch_init_insns(void)
>> #endif
>> }
>>
>> +/*
>> + * Check if the address is in kernel space
>> + *
>> + * Clone core_kernel_text() from kernel/extable.c, but don't call
>> + * init_kernel_text() for Ftrace don't trace functions in init sections.
>> + */
>> +static inline int in_kernel_space(unsigned long ip)
>> +{
>> + if (ip>= (unsigned long)_stext&&
>> + ip<= (unsigned long)_etext)
>> + return 1;
>> + return 0;
>> +}
>> +
>> static int ftrace_modify_code(unsigned long ip, unsigned int new_code)
>> {
>> int faulted;
>> @@ -91,10 +91,16 @@ int ftrace_make_nop(struct module *mod,
>> unsigned long ip = rec->ip;
>>
>> /*
>> - * We have compiled module with -mlong-calls, but compiled the
>> kernel
>> - * without it, we need to cope with them respectively.
>> + * If ip is in kernel space, long call is not needed, otherwise,
>> it is
>> + * needed.
>> */
>> - if (in_module(ip)) {
>> + if (in_kernel_space(ip)) {
>> + /*
>> + * move at, ra
>> + * jal _mcount --> nop
>> + */
>> + new = INSN_NOP;
>> + } else {
>> #if defined(KBUILD_MCOUNT_RA_ADDRESS)&& defined(CONFIG_32BIT)
>> /*
>> * lui v1, hi_16bit_of_mcount --> b 1f (0x10000005)
>> @@ -117,12 +123,6 @@ int ftrace_make_nop(struct module *mod,
>> */
>> new = INSN_B_1F_4;
>> #endif
>> - } else {
>> - /*
>> - * move at, ra
>> - * jal _mcount --> nop
>> - */
>> - new = INSN_NOP;
>> }
>> return ftrace_modify_code(ip, new);
>> }
>> @@ -132,8 +132,12 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned
>> long addr)
>> unsigned int new;
>> unsigned long ip = rec->ip;
>>
>> - /* ip, module: 0xc0000000, kernel: 0x80000000 */
>> - new = in_module(ip) ? insn_lui_v1_hi16_mcount :
>> insn_jal_ftrace_caller;
>> + /*
>> + * If ip is in kernel space, long call is not needed, otherwise,
>> it is
>> + * needed.
>> + */
>> + new = in_kernel_space(ip) ? insn_jal_ftrace_caller :
>> + insn_lui_v1_hi16_mcount;
>>
>> return ftrace_modify_code(ip, new);
>> }
>> @@ -200,11 +204,13 @@ unsigned long ftrace_get_parent_addr(unsigned long
>> self_addr,
>> int faulted;
>>
>> /*
>> - * For module, move the ip from calling site of mcount after the
>> - * instruction "lui v1, hi_16bit_of_mcount"(offset is 24), but for
>> - * kernel, move after the instruction "move ra, at"(offset is 16)
>> + * If self_addr is in kernel space, long call is not needed, only
>> need
>> + * to move ip after the instruction "move ra, at"(offset is 16),
>> + * otherwise, long call is needed, need to move ip from calling
>> site of
>> + * mcount after the instruction "lui v1,
>> hi_16bit_of_mcount"(offset is
>> + * 24).
>> */
>> - ip = self_addr - (in_module(self_addr) ? 24 : 16);
>> + ip = self_addr - (in_kernel_space(self_addr) ? 16 : 24);
>>
>> /*
>> * search the text until finding the non-store instruction or
>> "s{d,w}
>
>
