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

Lthreads disentangled from musl #403

Merged
merged 59 commits into from
Aug 14, 2020
Merged

Lthreads disentangled from musl #403

merged 59 commits into from
Aug 14, 2020

Conversation

vtikoo
Copy link
Contributor

@vtikoo vtikoo commented Jun 9, 2020

Summary

  • Remove musl related threading code from lthreads implementation.
  • Add auxiliary vector entries on the elf64 stack created during first ecall.

Fixes #156

Corresponding sgx-lkl-musl PR: lsds/sgx-lkl-musl#18

@vtikoo vtikoo changed the title Lthreads disentangled from musl [WIP] Lthreads disentangled from musl Jun 9, 2020
Copy link
Contributor

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this, looks like a great start.

.gitmodules Outdated Show resolved Hide resolved
src/enclave/enclave_event_channel.c Outdated Show resolved Hide resolved
src/enclave/enclave_init.c Outdated Show resolved Hide resolved
src/include/enclave/lthread.h Outdated Show resolved Hide resolved
src/include/enclave/lthread.h Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
@letmaik
Copy link
Contributor

letmaik commented Jun 10, 2020

@vtikoo Could you set the PR to draft mode if it's not ready for merging yet?

@vtikoo vtikoo marked this pull request as draft June 10, 2020 17:16
@vtikoo vtikoo marked this pull request as ready for review June 23, 2020 02:51
@vtikoo vtikoo marked this pull request as draft June 23, 2020 02:54
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
Copy link
Contributor

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diffs that mostly delete code make me happy!

src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
src/sched/lthread.c Outdated Show resolved Hide resolved
@davidchisnall
Copy link
Contributor

Please can you create a draft musl PR for the musl changes so that I can review them as well? Thanks.

@vtikoo
Copy link
Contributor Author

vtikoo commented Jun 27, 2020

@davidchisnall the musl PR is here - lsds/sgx-lkl-musl#18

@vtikoo vtikoo force-pushed the fix-156 branch 6 times, most recently from 7b06171 to 00b894a Compare July 16, 2020 18:59
@vtikoo vtikoo force-pushed the fix-156 branch 3 times, most recently from 861331b to 62696d6 Compare July 21, 2020 00:50
@vtikoo vtikoo marked this pull request as ready for review July 31, 2020 15:51
Copy link
Collaborator

@mikbras mikbras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vikas. If you can verify that auxv is 16-byte aligned, I will approve. Also I notice that some of the standard AT_* variables are missing in auxv. This is fine for now but when we back out certain musl libc changes, they will be needed to find the elf header, program headers, and the elf entry point.

@@ -90,6 +91,8 @@ static void init_wireguard()

static int startmain(void* args)
{
__init_libc(sgxlkl_enclave_state.elf64_stack.envp,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't worry. My feature.split branch will re-organize this after check in.

@@ -150,9 +156,8 @@ int __libc_init_enclave(int argc, char** argv)
max_lthreads = next_power_of_2(max_lthreads);

newmpmcq(&__scheduler_queue, max_lthreads, 0);

__init_libc(envp, argv[0]);
__init_tls();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just double checking. Has the call to this function gone away completely? (__init_tls)?

{
// By default auxv[AT_RANDOM] points to a buffer with 16 random bytes.
uint64_t *rbuf = (uint64_t*)buf_ptr;
buf_ptr += 16;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this buffer 16-byte aligned? I think the ABI requires it.

auxv[23] = !sgxlkl_in_sw_debug_mode();
auxv[24] = AT_NULL;
auxv[25] = 0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following seem to be missing:

  • AT_BASE -- base address of image
  • AT_SYSINFO_EHDR -- the ELF header address
  • AT_PHDR - the program header address
  • AT_PHNUM - the number of program headers
  • AT_ENTRY - address of the entry point

These are usually passed by the loader. It may work without them for now, but these may be needed later when we want to just call _start().

You can check in without these I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AT_PHDR is needed for unwind libraries. We probably can't throw an exception through qsort without that. That said, I've never actually seen code in the wild that wants to throw exceptions through libc functions outside of test suites: if you're using C++, there are significantly more efficient ways of sorting than calling libc's qsort and very few other libc functions take function pointer arguments.

@vtikoo
Copy link
Contributor Author

vtikoo commented Aug 14, 2020

As discussed with @mikbras, #325 covers the effort to get to full ABI compatibility for the Stack on app entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate userspace TLS from lthreads
5 participants