Re: Regarding systemtap support for AArch64

[Masami Hiramatsu <masami dot hiramatsu dot pt at hitachi 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