Page MenuHomeFreeBSD

DTrace support for AArch64
ClosedPublic

Authored by br on Jun 5 2015, 3:15 PM.
Tags
None
Referenced Files
F103712970: D2738.diff
Thu, Nov 28, 9:06 AM
Unknown Object (File)
Mon, Nov 25, 3:17 PM
Unknown Object (File)
Mon, Nov 25, 12:49 PM
Unknown Object (File)
Mon, Nov 25, 9:18 AM
Unknown Object (File)
Fri, Nov 22, 5:25 PM
Unknown Object (File)
Fri, Nov 22, 1:50 PM
Unknown Object (File)
Fri, Nov 22, 1:39 PM
Unknown Object (File)
Thu, Nov 21, 9:50 PM
Subscribers

Details

Reviewers
None
Group Reviewers
ARM
DTrace
Commits
rS285009: First cut of DTrace for AArch64.
Summary

basic stuff implemented

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

br retitled this revision from to DTrace support for AArch64.
br updated this object.
br edited the test plan for this revision. (Show Details)
br added reviewers: ARM, DTrace.
sys/arm64/arm64/db_trace.c
77 ↗(On Diff #5965)

I think the changes in this file could be committed independently

move ddb-related changes to separate patch

br marked an inline comment as done.Jun 6 2015, 11:01 AM
br added inline comments.
sys/arm64/arm64/db_trace.c
77 ↗(On Diff #5965)

sounds reasonable, thanks!

andrew added inline comments.
lib/libproc/proc_regs.c
64

This file could be split out and committed as is (although it won't work as we don't have ptrace support).

sys/arm64/arm64/exception.S
71–82

what is this change for?

sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
11887–11888

This should be sorted, aarch64 should be before amd64.

sys/cddl/dev/dtrace/aarch64/dtrace_asm.S
59–62

This doesn't disable interrupts, only sets them to be disabled when returning from an exception. To disable them you need the daif register.

70–74

See dtrace_interrupt_disable above

82

Why not ldrb w0, [x0]? Then there is no need for the mov.

124–127

This is wrong, it loads & stores data in 4-byte chunks, but then increments the address by 1. I expect you were intendign to use ldrb and strb here.

166–167

You will need tow versions of this as sizeof(uint32_t) != sizeof(void *)

sys/cddl/dev/fbt/aarch64/fbt_isa.c
62

This coment is incorrect, the 5th parameter is in x4.

br marked an inline comment as done.
br removed a subscriber: andrew.

unmagic

sys/arm64/arm64/exception.S
71–82

so in order to emulate 'store pair' instruction we have to reserve space (minimum required 96 bytes, I set 128).

in case frame->tf_sp not changed during trap (say dtrace turned off), we have to skip 128 bytes reserved space. If frame->tf_sp changed, it will be used as new SP.

sys/arm64/arm64/exception.S
71–82

I've reworked the code to simplify this. frame->tf_sp is the value of sp when entering the exception handler, you can adjust it as needed in save_registers after the mov x18, sp instruction.

sys/arm64/arm64/exception.S
71–82

thanks! - it is much easier now

cddl/contrib/opensolaris/lib/libdtrace/aarch64/dt_isadep.c
83

What is 0x3 for? If it's to check an alignment we should add a macro for this.

lib/libproc/proc_bkpt.c
64

Is this just used in userland? We should allocate a unique value for it.

sys/cddl/dev/dtrace/aarch64/dtrace_subr.c
260

Why 128?

261–262

This looks wrong for the 32-bit store, it will write to sp and sp + 8 bytes, where it should write to sp and sp + 4 bytes.

266–267

This has the same issue as above with the 32-bit instructions. do we need them? I would only expect to see them used when using the 32-bit ABI in userland.

276

We need #define INSN_SIZE 4 in a header for use here.

sys/cddl/dev/fbt/aarch64/fbt_isa.c
157

Why are you searching for ldp and not ret?

sys/cddl/dev/dtrace/aarch64/dtrace_asm.S
60–63

ok thanks - I changed to daifset, daifclr

sys/cddl/dev/dtrace/aarch64/dtrace_subr.c
262–263

ok,thanks. Lets leave STP/LDP 64bit version only

267–268

ok leave 64-bit versions only

br added inline comments.
cddl/contrib/opensolaris/lib/libdtrace/aarch64/dt_isadep.c
84

ok I reused ALIGNED_POINTER macro from arm64/include/param.h

sys/cddl/dev/dtrace/aarch64/dtrace_asm.S
125–128

right, fixed

sys/cddl/dev/dtrace/aarch64/dtrace_asm.S
83

right

sys/cddl/dev/dtrace/aarch64/dtrace_subr.c
261

ok I eliminated this number

277

header added

sys/cddl/dev/fbt/aarch64/fbt_isa.c
63

comment removed

sys/cddl/dev/dtrace/aarch64/dtrace_asm.S
167–168

splitted for two versions

sys/cddl/dev/fbt/aarch64/fbt_isa.c
158

ok lets try with ret instead of ldp. Looking on trace I see it does not really matters

br marked 14 inline comments as done.

regenerate patch

cddl/contrib/opensolaris/lib/libdtrace/common/dt_link.c
230–236

What do we need to do here (for arm/mips in general, not just aarch64)?

432–437

and what do we need to do here?

lib/libproc/proc_regs.c
64

Yes, I'd like to see this committed as is. The aarch64 #ifs should be resorted to be first though to keep alpha order.

Set 20 as IMM16 value for BRK instruction

cddl/contrib/opensolaris/lib/libdtrace/common/dt_link.c
236

This should be in alphabetical order, aarch64 should be before arm.

436

And this

848

And this

cddl/lib/Makefile
29–31

This should be sorted, it was missed when arm was added, but that shouldn't mean we do the same with AArch64.

cddl/lib/libdtrace/Makefile
86

Alphabetical

lib/Makefile
219

Alphabetical

lib/libproc/proc_bkpt.c
63

Alphabetical

64

Can you use a different brk value? It might also pay to document them on the wiki.

sys/arm64/arm64/trap.c
267

You should also check the breakpoint instruction has the correct value so we don't hit the ddb brk.

sys/cddl/dev/dtrace/aarch64/dtrace_asm.S
157–158

Why not replace these two instructions with cbnz?

172–173

And here

sys/cddl/dev/fbt/aarch64/fbt_isa.c
44

This should be a unique value in the EL1 range.

117

Does this function exist on arm64?

124

This list should be checked in some central location.

168

There is no branch delay slot on arm. It's also a bug in the 32-bit dtrace code.

196

POPM? Shouldn't it be named DTRACE_INVOP_RET?

br marked 11 inline comments as done.
br marked 15 inline comments as done.Jun 24 2015, 1:29 PM
br added inline comments.
lib/libproc/proc_bkpt.c
58

So I set 0x14 for libproc and 0x40d for dtrace

64

I have set 20 as BRK value. See BREAKPOINT_INSTR define

sys/cddl/dev/fbt/aarch64/fbt_isa.c
45

Ok let it be 0x40d

118

not sure

125

what you mean?

169

right, thanks

br marked an inline comment as done.

move checks if function is excluded from instrumentation to some central location

br marked an inline comment as done.

set 0xd as brk value (instead of 20) for libproc

This revision was automatically updated to reflect the committed changes.