On Tue, 23 Apr 2019 20:15:49 +0200 Nicolai Stange <nstange@xxxxxxx> wrote: > > This can be made much simpler by making a hardcoded ftrace_int3_tramp > > that does the following: > > > > ftrace_int3_tramp > > push %r11 > > jmp ftrace_caller > > > But wouldn't this mean that ftrace_caller could nest if the breakpoint > in question happened to be placed at ftrace_call? Infinite recursion set > aside, the ip value determined from inner calls based on the on-stack > return address would be wrong, AFAICS. Or am I missing something here? I had a reply here, but I think you explained what I just explained (and then deleted) below ;-) > > > > The ftrace_caller will either call a single ftrace callback, if there's > > only a single ops registered, or it will call the loop function that > > iterates over all the ftrace_ops registered and will call each function > > that matches the ftrace_ops hashes. > > > > In either case, it's what we want. > > Ok, under the assumption that my above point is valid, this patch could > still get simplified a lot by having two trampolines: > > 1.) Your ftrace_int3_tramp from above, to be used if the patched insn is > some mcount call site. The live patching fops will need > ftrace_regs_caller though. So, > > ftrace_int3_tramp_regs_caller: > push %r11 > jmp ftrace_regs_caller > > 2.) Another one redirecting control flow to ftrace_ops_list_func(). It's > going to be used when the int3 is found at ftrace_call or > ftrace_regs_call resp. their counterparts in some ftrace_ops' > trampoline. > > ftrace_int3_tramp_list_func: > push %r11 > jmp ftrace_ops_list_func Yes, I wrote a reply basically stating something similar, but then deleted it after reading what you wrote here! > > ftrace_int3_handler() would then distinguish the following cases: > 1.) ip == ftrace_graph_call => ignore, i.e. skip the insn OK, because this transition would be from "call function graph" to "nop" or the other way. Either case, one would always be a nop. > 2.) is_ftrace_caller(ip) => ftrace_int3_tramp_list_func > 3.) ftrace_location(ip) => ftrace_int3_tramp_regs_caller > > ftrace_location() is getting invoked from ftrace_int3_handler() already, > so there wouldn't be any additional cost. > > If that makes sense to you, I'd prepare a patch... Yes it does. > > > > The ftrace_int3_tramp will simply simulate the call ftrace_caller that > > would be there as the default caller if more than one function is set > > to it. > > > > For 32 bit, we could add 4 variables on the thread_info and make 4 > > trampolines, one for each context (normal, softirq, irq and NMI), and > > have them use the variable stored in the thread_info for that location: > > > > ftrace_int3_tramp_normal > > push %eax # just for space > > push %eax > > mov whatever_to_get_thread_info, %eax > > mov normal(%eax), %eax > > mov %eax, 4(%esp) > > pop %eax > > jmp ftrace_caller > > > > ftrace_int3_tramp_softiqr > > push %eax # just for space > > push %eax > > mov whatever_to_get_thread_info, %eax > > mov softirq(%eax), %eax > > mov %eax, 4(%esp) > > pop %eax > > jmp ftrace_caller > > > > etc.. > > > > > > A bit crazier for 32 bit but it can still be done. ;-) > > Ok, but currently CONFIG_HAVE_LIVEPATCH=n for x86 && !x86_64, > so I'd rather not invest too much energy into screwing 32 bit up ;) > Probably not something you care about, but something that I do. Which means it will have to go on my TODO list. I care about missed functions being called. This means, if you have something tracing a function, and then enable something else to trace that same function, you might miss the first one. -- Steve
