Details
- Reviewers
markj mhorne gnn andrew - Commits
- rG07864a8a2466: kinst: port to arm64
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/cddl/dev/kinst/aarch64/kinst_isa.c | ||
|---|---|---|
| 305 | You could simplify control flow and reduce indentation with for (; !found && instr < limit; instr++). Then in the body, use else if and remove the breaks. | |
| 342 | The comment should explain why. | |
| 345 | As in riscv, it'd be nicer to have predicate functions instead of these naked tests. | |
| sys/cddl/dev/kinst/aarch64/kinst_isa.h | ||
| 14 | These should probably go into dtrace.h rather than being copied from FBT. | |
| sys/cddl/dev/kinst/aarch64/kinst_isa.h | ||
|---|---|---|
| 14 | Should the patch value go in dtrace.h as well, something like AARCH64_PATCHVAL? | |
| sys/cddl/dev/kinst/aarch64/kinst_isa.c | ||
|---|---|---|
| 47 | If you shift imm left by 12 bits, then the low 12 bits of the shifted value are 0, so masking 0xfff has no effect. | |
| 403 | This is the root cause of the problem: https://github.com/freebsd/freebsd-src/blob/main/sys/arm64/arm64/exception.S#L147 Basically, when returning from an exception taken in EL1, the handler does not bother restoring callee-saved registers, since it is technically unnecessary. However, adr/adrp might modify one of those registers. In that case, the effect of the emulation is lost, since the exception handler does not reload the register value from the trapframe. That's why kinst::vm_fault:36 crashes your kernel, but :32 does not. Both are adrp instructions, but the former modifies x27 (callee-saved) while the latter modifies x8 (caller-saved). As you noted, the problem only appears to affect adr/adrp, and that makes sense since other emulated instructions do not modify arbitrary registers. So I think we can go ahead with this workaround of simply ignoring such instructions, but the comment should be updated to explain why. | |
| 464 | I believe it is safe to trace do_el0_sync and handle_el0_sync. Those functions handle exceptions from EL0, i.e., user mode, but kinst will only raise exceptions in EL1. | |
| sys/cddl/dev/kinst/aarch64/kinst_isa.c | ||
|---|---|---|
| 403 | Interesting. Would it be wise to get rid of this optimization to let kinst trace those instructions, or is the trade off not worth it, considering the not-so-large amount of users who will want to trace those 2 exact instructions? | |
| sys/cddl/dev/kinst/aarch64/kinst_isa.c | ||
|---|---|---|
| 403 | Well, the problem affects any instruction of the form <adr|adrp> <callee-saved-register>, <label>. I think we should just work around the problem by disabling emulation of adr/adrp for now. I'm not sure how useful that optimization is. Ideally we could disable that optimization only for breakpoint exceptions, but I don't see a clean way to do that at the moment. | |
| sys/cddl/dev/kinst/aarch64/kinst_isa.c | ||
|---|---|---|
| 157 | Why sync the address of the patch point? Only the trampoline is being modified here. kinst_patch_tracepoint() handles modification of the original instruction. | |
Approved.
| sys/cddl/dev/kinst/aarch64/kinst_isa.c | ||
|---|---|---|
| 205 | kinst_jump_next_instr() returns 0, which tells dtrace_invop() to keep looking for a breakpoint handler, but that's not really what we want (though it's relatively harmless). Perhaps, in a follow-up revision, dtrace_invop_start() could interpret a return value of -1 from dtrace_invop() to mean, "just return without trying to emulate an instruction." That assumes that -1 isn't a valid encoding of an instruction which we might want to emulate, so it's not a perfect solution. | |
| sys/cddl/dev/kinst/aarch64/kinst_isa.c | ||
|---|---|---|
| 205 | I've been trying to figure out a clever way to refactor dtrace_invop_start() as well, but a possible modification will indeed go into a follow-up commit, not this one. | |