Re: Regarding systemtap support for AArch64

[Sandeepa Prabhu <sandeepa dot prabhu at linaro dot org>]

On 3 October 2013 18:30, Masami Hiramatsu
<masami.hiramatsu.pt@hitachi.com> wrote:
> (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.
Not in kprobes. But kgdb can remove breakpoint handler and use same
API. or atleast while providing an api we should not assume race
cannot happen right?
And there wont be much lock contention, i'ts only if the debug
framework (like kgdb) is wrapping-up, not is normal use-case.

>
>>>    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.
Hmm, I will change this part.

>
>>>  - 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.
Got it, and can be done cleanly :-)

>
>>>  - 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.
When we wrote at Linaro, our plan was to share simulation calls
between kprobes and uprobes, and planned to use 'struct kprobes' for
both the frameworks, this is how it was done on "arch/arm/kernel/"
effectively.
If more frameworks can use it (as it seems) I change it to accept
opcode, pc value and saved pt_regs and avoid kprobe struct altogether.
Also, we are starting on uprobes at Linaro, so it won't be too long
before we start thinking about that too ;-)

>
>>>  - 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.
OK, I will do that, and CC LKML from v2.

>
>> 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 !

Question:
I am working on v2 patchset based on comments, for next week to post,
do you have basic aarch64 setup (fast-model/hardware), ARM v8-ARM etc?
I mean, how about sharing some efforts with me(Linaro) going further?
Most work shall go through LAKML so you may have to subscribe to that
;),  but do you mind working on Linaro hosted public git ? we can lay
out a plan then. After kprobes, we have much work on uprobes in queue
(both 32-bit and 64-bit user-space) and your insights help us, since
you are one of the maintainers of both subsystems.

Cheers,
Sandeepa
>
>
>
> --
> Masami HIRAMATSU
> IT Management Research Dept. Linux Technology Center
> Hitachi, Ltd., Yokohama Research Laboratory
> E-mail: masami.hiramatsu.pt@hitachi.com
>
>