Page MenuHomeFreeBSD

kinst: port to arm64
ClosedPublic

Authored by christos on May 30 2023, 3:40 PM.
Referenced Files
Unknown Object (File)
Tue, Jun 4, 6:18 AM
Unknown Object (File)
Mon, May 20, 11:57 AM
Unknown Object (File)
Fri, May 17, 12:54 PM
Unknown Object (File)
Thu, May 16, 1:25 PM
Unknown Object (File)
Thu, May 16, 1:25 PM
Unknown Object (File)
Thu, May 16, 1:25 PM
Unknown Object (File)
Thu, May 16, 1:25 PM
Unknown Object (File)
Thu, May 16, 1:25 PM
Subscribers

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?

christos added inline comments.
sys/cddl/dev/kinst/aarch64/kinst_isa.h
14
christos retitled this revision from kinst: port to ARM64 to kinst: port to arm64.May 31 2023, 8:24 PM
christos edited the summary of this revision. (Show Details)

WIP.

christos edited the summary of this revision. (Show Details)

Updated description, still some unsolved bugs, but mostly done.

christos edited the summary of this revision. (Show Details)

Use per-probe trampolines. Depends on D40962

christos edited the summary of this revision. (Show Details)

Disable adr/adrp emulation.

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.

christos marked 4 inline comments as done.

Address Mark's comments. Depends on D40962.

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.

christos marked an inline comment as done.

Address comments, fix stuff that was also fixed in riscv.

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.

This revision is now accepted and ready to land.Jul 19 2023, 1:47 PM
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.

This revision was automatically updated to reflect the committed changes.