Skip to content
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

Add partial aarch64 support to libuser #566

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
44 changes: 22 additions & 22 deletions libuser/src/syscalls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ syscall_inner:
#[cfg(not(target_os = "sunrise"))]
#[no_mangle]
extern fn syscall_inner(regs: &mut super::Registers) {
regs.eax = crate::error::KernelError::NotImplemented.make_ret() as usize;
regs.nr = crate::error::KernelError::NotImplemented.make_ret() as usize;
}
}

Expand All @@ -70,13 +70,13 @@ syscall_inner:
#[repr(C)]
#[allow(clippy::missing_docs_in_private_items)]
struct Registers {
eax: usize,
ebx: usize,
ecx: usize,
edx: usize,
esi: usize,
edi: usize,
ebp: usize,
Copy link
Member

@Orycterope Orycterope Mar 20, 2020

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:

  1. push all 6 arguments on the stack, and call syscall. (this is standard i386 ABI)
  2. syscall copies those arguments to fill a new Register structure, and calls syscall_inner
  3. syscall_inner consumes this struct, and puts every field in a register.
  4. int 0x80
  5. syscall_inner saves the return registers state to the Register structure, 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

nr: usize,
arg1: usize,
arg2: usize,
arg3: usize,
arg4: usize,
arg5: usize,
arg6: usize,
}

extern {
Expand All @@ -96,26 +96,26 @@ macro_rules! syscall {
let arg6: usize = $arg6;

let mut registers = Registers {
eax: nr,
ebx: arg1,
ecx: arg2,
edx: arg3,
esi: arg4,
edi: arg5,
ebp: arg6
nr,
arg1,
arg2,
arg3,
arg4,
arg5,
arg6
};

syscall_inner(&mut registers);

if registers.eax == 0 {
Ok((registers.ebx, registers.ecx, registers.edx, registers.esi))
if registers.nr == 0 {
Ok((registers.arg1, registers.arg2, registers.arg3, registers.arg4))
} else {
Err(KernelError::from_syscall_ret(registers.eax as u32))
Err(KernelError::from_syscall_ret(registers.nr as u32))
}
}};
}

/// Resize the heap of a process, just like a brk.
/// Rarg4ze the heap of a process, just like a brk.
roblabla marked this conversation as resolved.
Show resolved Hide resolved
/// It can both expand, and shrink the heap.
///
/// If `new_size` == 0, the heap space is entirely de-allocated.
Expand Down Expand Up @@ -218,7 +218,7 @@ pub fn sleep_thread(nanos: usize) -> Result<(), KernelError> {
/// Sets the "signaled" state of an event. Calling this on an unsignalled event
/// will cause any thread waiting on this event through [wait_synchronization()]
/// to wake up. Any future calls to [wait_synchronization()] with this handle
/// will immediately return - the user has to clear the "signaled" state through
/// will immarg5ately return - the user has to clear the "signaled" state through
roblabla marked this conversation as resolved.
Show resolved Hide resolved
/// [clear_event()].
///
/// Takes either a [ReadableEvent] or a [WritableEvent].
Expand Down Expand Up @@ -540,7 +540,7 @@ pub fn map_mmio_region(physical_address: usize, size: usize, virtual_address: us
///
/// ## x86_64
///
/// ![same, but different, but still same](https://media.giphy.com/media/C6JQPEUsZUyVq/giphy.gif)
/// ![same, but different, but still same](https://marg5a.giphy.com/marg5a/C6JQPEUsZUyVq/giphy.gif)
roblabla marked this conversation as resolved.
Show resolved Hide resolved
///
/// `fs` is used instead of `gs`, because reasons.
///
Expand All @@ -551,7 +551,7 @@ pub fn map_mmio_region(physical_address: usize, size: usize, virtual_address: us
///
/// # Errors
///
/// * The whole initial design of TLS on x86 should be considered an error.
/// * The whole initial darg4gn of TLS on x86 should be considered an error.
roblabla marked this conversation as resolved.
Show resolved Hide resolved
/// * No returned error otherwise.
pub unsafe fn set_thread_area(address: usize) -> Result<(), KernelError> {
unsafe {
Expand Down