DTrace support for AArch64

[br]

Summary

basic stuff implemented

[emaste]

sys/arm64/arm64/db_trace.c

77 ↗

(On Diff #5965)

I think the changes in this file could be committed independently

[br]

move ddb-related changes to separate patch

[br]

sys/arm64/arm64/db_trace.c

77 ↗

(On Diff #5965)

sounds reasonable, thanks!

[andrew]

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]

unmagic

[br]

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.

[andrew]

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.

[br]

sys/arm64/arm64/exception.S

71–82

thanks! - it is much easier now

[andrew]

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?

[br]

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]

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

[br]

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

[br]

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]

regenerate patch

[emaste]

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.

[emaste]

lib/libproc/proc_bkpt.c

64

See https://gcc.gnu.org/ml/gcc-patches/2013-11/msg02228.html

[br]

Set 20 as IMM16 value for BRK instruction

[andrew]

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]

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]

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

[br]

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