This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: Regarding systemtap support for AArch64
- From: Masami Hiramatsu <masami dot hiramatsu dot pt at hitachi dot com>
- To: Sandeepa Prabhu <sandeepa dot prabhu at linaro dot org>
- Cc: William Cohen <wcohen at redhat dot com>, systemtap at sourceware dot org, Deepak Saxena <dsaxena at linaro dot org>
- Date: Thu, 03 Oct 2013 22:00:58 +0900
- Subject: Re: Regarding systemtap support for AArch64
- Authentication-results: sourceware.org; auth=none
- References: <CA+b37P3S4adOJe+S1RWKVDEzeVLG2Oa4EFqYgeH4cU6SNmvtEQ at mail dot gmail dot com> <1380011243 dot 3958 dot 11921 dot camel at bordewijk dot wildebeest dot org> <52432F3B dot 4020503 at redhat dot com> <CA+b37P13t44vQfS3RwxkCowgqYBAHyUHCNJQtGqxmrqnt_rw6Q at mail dot gmail dot com> <5248E391 dot 3060306 at hitachi dot com> <52496A50 dot 9090904 at redhat dot com> <CA+b37P31Zz3F0SGJt_M_3T2GxCm6zn5K4b56oeoR-qMBF=wjDg at mail dot gmail dot com> <524C025B dot 1060402 at hitachi dot com> <CA+b37P0i8Ms8u=BcTAMfGGm+bSAYQO+OM-+qTiHSPRysMRMHfg at mail dot gmail dot com>
(2013/10/03 12:12), Sandeepa Prabhu wrote:
> On 2 October 2013 16:54, Masami Hiramatsu
> <masami.hiramatsu.pt@hitachi.com> wrote:
>> (2013/10/02 13:17), Sandeepa Prabhu wrote:
>>> Hi all,
>>>
>>> I have uploaded ARM64 kprobes work on Linaro public git:
>>> git://git.linaro.org/people/sandeepa.prabhu/linux-aarch64.git Branch:
>>> kprobes_devel_v8. Patches are published on LAKML too. This is based
>>> on v8 upstream kernel (3.12-rc1) right now, and works with linaro
>>> boot-wrapper and fast model setup, though, not sure what it takes to
>>> build for fedora.
>>
>> Thank you for the great work! :)
>>
>> At a glance, I have some comments (almost formatting issues) on it.
>> - kprobes def config patch should be moved after the
>> kprobe support.
> Yes ideally, but it is not part of the patchset, I only added this new
> config in my repo for development testing (for sample modules), actual
> changes should go in in arm/arm64/defconfig or any other configs that
> can enable it.
Ah, I see. :)
>> - Is it really need to use spinlock to protect break_hook?
> Any cpu can remove breakpoint hooks right, and traversal happen in
> debug exception context so mutex are not safe (can sleep/schedule out)
> in debug exception.
I don't think we need to remove the breakpoint hooks after starting
up the kernel. If we use the spinlock there, we'll pay a big cost
because of the lock contention.
>> Why can't we use rcu_lock as step_hook?
> Need to check, but we need protection for list traversal only (if hook
> removed from another cpu)
>
>> - patch_text should check the alignment in itself and return an error.
> Yes, perhaps kprobe also should check the alignment before handling,
> if debug addr (ELR_EL1) is not 32-bit aligned, it's an error and
> should be a BUG() since this it cannot be handled, can fix this in
> next version.
Right, for the similar illegal address, we already use -EILSEQ in
arch_prepare_kprobe() on x86. You can do same thing on arm64.
>> - For ease of bisect debugging, I'd recommend you to split decoder and
>> simulator, and to reorder it as below.
>> 1. decoder patch
>> 2. kprobes basic patch (this doesn't support insns which need simulator)
>> 3. simulator & kprobes enhancement patch
> Good point, I will refine in next set of patches, I thought it is
> difficult to split that way since we have unified decode table. One
> basic question with this way of splitting, "is it expected that 1.
> and 2. (without simulate patch-3) can build and execute?"
Yes, we can just reject the instructions which require simulation at
that point ;) Patch 3. enables kprobes to support such instructions.
>> - probes-*.c is not good name for the simulator. those should have
>> better name.
> May be decode-arm64.c? Originally I had decode-* but the logic is
> limited to kprobes and uprobes only so renamed that way. Other cases
> like jump_labels, use different decoding, and may not share same code.
The decoding table will be different from other usecase, but I think
simulator code can be shared (and must be). It may just get the address,
instruction, and registers, not the kprobe.
>> - kprobes-arm64.c is also not good name. I think we can have
>> arch/arm64/kernel/kprobes/ directory on arm64 as same as x86.
>> we may have core.c and insn_map.c.
> Hmmm, with our design uprobes should share probes-* or decode-* files,
> then uprobes code also reside in arch/arm64/kernel/kprobes/ folder
> itself. Please suggest if this is fine? folder name may be
> misleading.
Ah, I see. On x86, both uprobes and kprobes have own decode(actually
for classifying instructions) table. Those are a bit different.
At this point, I think you don't need to consider so much about
uprobes. Just focus on kprobes ;) We can evolve the code when we need.
>> - probes-common.c looks only for kprobes. This should be go into kprobes.c.
> No, this file is common for kprobes 64-bit kernel, uprobes 32-bit
> userspace, and uprobes 64-bit userspace, so needs to be in separate
> file.
Right, I misunderstood. This is a part of the simulator.
>> - It seems that a part of kretprobe logic is in the kprobes' patch.
> Hmm I can refine it in next patchset, may be jprobes support too go in
> as separate patch?
Yeah, feel free to add jprobes support too. However, jprobe is not common
feature in kernel now, I mean it is not high priority.
>> IMHO, the simulator/decoder will be useful for other cases (KVM, mmiotrace etc.)
>> So I recommend you to keep it away from kprobes as far as possible.
>>
>> And please CC the series to me next time
>> (since I don't subscribe LAKML...).
> Sorry, I did not CC you last time, I will add you from v2.
Thanks ;)
> Masami,
> Couple of questions:
> 1. My changes reside in arm/arm64 only right now, does it make sense
> to publish on LKML so that core kprobes developers can review?
Of course, it is always helpful for us :)
> 2. Wanted to change sample/kprobes/kprobe_example.c to check for the
> missing case of ARM & ARM64 and print the relevant info. Can this
> change be as part of same series (if going on LKML)?
Yeah, it is better to update it within the series.
> Thanks a lot for your review, I would also need help on validation for
> my work, please let me know if our repo holds good for systemtap
> validation. I am interested in using fedora on v8 model, please
> provide me some instructions to get the packages.
Thank you !
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com