-
Notifications
You must be signed in to change notification settings - Fork 12
Add partial aarch64 support to libuser #566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
aarch64 requires syscall numbers be immediates, so a standard function can't be used here.
This was created by diffing the i386 JSON with i686-unknown-linux-gnu, and applying relevant changes to aarch64-unknown-linux-gnu.
|
I've implemented a syscall stub, it was easier than I thought. |
roblabla
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good!
Orycterope
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omg thanks so much
libuser/src/threads.rs
Outdated
| tls | ||
| } | ||
|
|
||
| /// Get a pointer to this thread's [TLS] region pointed to by `fs`, translated to the flat-memory model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the comment
| /// | ||
| /// The routine to call and its argument are expected to be found in this `ThreadContext`. | ||
| #[cfg(not(target_arch = "x86"))] | ||
| extern fn thread_trampoline(thread_context_addr: usize) -> ! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost certainly not valid for aarch64.
This deserves a huge
// todo: figure out aarch64 elf TLS
| eax: usize, | ||
| ebx: usize, | ||
| ecx: usize, | ||
| edx: usize, | ||
| esi: usize, | ||
| edi: usize, | ||
| ebp: usize, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roblabla , I need your confirmation on this, but IIRC the sole reason we use a global asm for the syscall is because we need to pass ebp as our sixth argument, but trying to backup+set/restore it in an inline asm block was unfeasible, because Rust needs this register to exit the inline asm block and restore regular context (or something like that).
This struct is only used as an argument to the global asm syscall_iner. This is pretty inefficient, since to do a syscall you must:
- push all 6 arguments on the stack, and call
syscall. (this is standard i386 ABI) syscallcopies those arguments to fill a newRegisterstructure, and callssyscall_innersyscall_innerconsumes this struct, and puts every field in a register.int 0x80syscall_innersaves the return registers state to theRegisterstructure, and returns.
On aarch64, where we have plenty of registers, this whole dance seems unnecessary, and I'd like to see the syscall function simply be a wrapper around a single inline asm instruction, that define its input/output registers properly (e.g. nr -> r0) and let llvm do all the copying for us.
In this context, the Register struct is very i386 specific, and doesn't need to be made generic.
I'd like to hear roblabla's opinion this, but if he agrees with me I'd recommend dropping this commit entirely
libuser/src/syscalls.rs
Outdated
| #[cfg(target_arch = "aarch64")] | ||
| asm!( | ||
| "svc $7" | ||
| : "={x0}"(registers.nr), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow, you did exactly what I was discussing in the previous comment 😅
For aarch64, I don't see the need for copying arguments to the Registers struct. Can't you just directly use nr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought Registers was in the public API? If it isn't, that's unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was, but I think that's a mistake, it should be defined only for i386
This reverts commit 253cacd.
figure out aarch64 TLSSunriseOS/libuser/src/threads.rs Lines 394 to 404 in 708cb35
This comment was generated by todo based on a
|
This is able to make libuser compile with the included target JSON. It does not yet include a crt0 or syscall stub, though preparations for that have been made (I'm not experienced enough with aarch64 assembly).