Messages in this thread |  | | Date | Fri, 15 Nov 2013 19:11:57 +0900 | From | Masami Hiramatsu <> | Subject | Re: Re: Re: [PATCH v2 10/13] kprobes: Remove uneeded kernel dependency on struct arch_specific_insn |
| |
(2013/11/14 23:15), Jon Medhurst (Tixy) wrote: > On Thu, 2013-11-14 at 11:02 +0900, Masami Hiramatsu wrote: >> (2013/11/14 2:13), Jon Medhurst (Tixy) wrote: >>> On Tue, 2013-10-15 at 17:04 -0400, David Long wrote: >>>> From: "David A. Long" <dave.long@linaro.org> >>>> >>>> Instead of depending on include/asm/kprobes.h to provide a dummy definition >>>> for struct arch_specific_insn, do so in include/linux/kprobes.h. >>> >>> That change description doesn't quite seem to quite make sense to me. >>> >>> Anyway, what we're trying to do with this patch is to allow us to use >>> arch_specific_insn for purposes additional to implementing kprobes. This >>> patch enables that but I'm wary that the kprobes code assumes that ainsn >>> is a struct arch_specific_insn, e.g. in linux/kernel/kprobes.c we have: >>> >>> memcpy(&p->ainsn, &ap->ainsn, sizeof(struct arch_specific_insn)); >>> >>> Now, that code isn't compiled when kprobes isn't configured, but it >>> seams to me to be safer if that was also changed to >>> >>> memcpy(&p->ainsn, &ap->ainsn, sizeof(p->ainsn)); >> >> This kind of cleanup looks good for me, but I don't agree to change >> the type of the member (removing is OK) by Kconfig. > > Wouldn't that still require an #ifdef CONFIG_KPROBES around ainsn? > Admittedly a less ugly one than one to change its type to an int.
Yeah, that's the point.
> >> If you want to >> change the framework of kprobes and uprobes itself (unification), >> I'm appreciate to discuss with you and uprobes people, because it >> will involve all arch dependent code change, *NOT ONLY* the ARM issue. > > Well, I don't think the goal wasn't unification as such. For kprobes on > ARM we have to decode and simulate pretty much the entire instruction > set(s) and the attempt to implement uprobes on ARM have tried to make > use of as much of that as possible. The tricky bit has been as to where > to try and draw the level of abstraction, and it seems this may well be > leaking out of the arch specific arena.
I see, I've heard that from Sandeepa who are working on arm64 kprobes. His patch series now has generic interface of decoder/simulator.
> Bit of background, Dave Long has been working on ARM uprobes based on > Rabin Vincent's earlier work, and I, as author of a large part of the > current ARM kprobes code, have been reviewing (not very satisfactorily I > admit) the bits that impact that. One of my motivations has been to push > the kprobes instruction decoding to be more generic, rather than special > casing things to cope with uprobes. This is because I'm aware of the > reoccurring theme on the ARM lists that it would be good to not have all > the different methods of instruction decoding, for probes, debug and > simulation, etc. (I'm sceptical that a one-size-fits-all is possible, > but consolidation where practical is good).
Same as x86, we still have different code base of kprobes and uprobes Fortunately, x86 instruction decoder is separated, but single-stepping and other parts are not well shared.
>>> However, I also wonder if we should instead leave arch_specific_insn as >>> a kprobes specific structure and on ARM define it in terms of a new more >>> generic 'struct probe_insn'? The drawback with that is that we'd >>> probably end up with a struct just containing a single member which >>> seems a bit redundant: >>> >>> struct arch_specific_insn { >>> struct probe_insn pinsn; >>> }; >> >> I also disagree it. If you have a plan to integrate uprobes and kprobes >> arch specific code, please share it with us. > > There's not really a 'plan', just an attempt to reuse the instruction > decoding code used by kprobes in the implementation of uprobes, i.e. the > patch series [1] which this mail thread is in reply to. > > [1] http://thread.gmane.org/gmane.linux.kernel/1579985
OK, and I think similar method we can use on x86 too. :) In that case, we may be able to simplify the arch_specific_insn.
Thank you,
-- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@hitachi.com
|  |