Page MenuHomeFreeBSD

Add support for CloudABI on ARM64.
ClosedPublic

Authored by ed on Oct 16 2015, 10:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 22 2024, 9:44 AM
Unknown Object (File)
Feb 15 2024, 11:17 PM
Unknown Object (File)
Feb 15 2024, 11:17 PM
Unknown Object (File)
Feb 15 2024, 11:17 PM
Unknown Object (File)
Feb 15 2024, 11:17 PM
Unknown Object (File)
Feb 15 2024, 11:17 PM
Unknown Object (File)
Feb 15 2024, 11:17 PM
Unknown Object (File)
Feb 15 2024, 11:17 PM
Subscribers

Details

Summary

It turns out that it is pretty easy to make CloudABI work on ARM64. We
essentially only need to copy over the sysvec from AMD64 and ensure that
we use ARM64 specific registers.

As there is an overlap between function argument and return registers,
we do need to extend cloudabi64_schedtail() to only set its values if
we're actually forking. Not when we're creating a new thread.

Test Plan

All cloudlibc tests seem to pass, except one caused due to a small bug
in Clang.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ed retitled this revision from to Add support for CloudABI on ARM64..
ed updated this object.
ed edited the test plan for this revision. (Show Details)
ed added a reviewer: andrew.
ed edited edge metadata.

Use MAXARGS instead of hardcoding 8.

sys/arm64/cloudabi64/cloudabi64_sysvec.c
79 ↗(On Diff #9447)

This should no longer be needed after rS289502.

199 ↗(On Diff #9447)

This feels like it would be fragile, but the only other was I can see to handle fork vs thread creation would be to store the syscall value somewhere or not trash it in exec_setregs.

sys/arm64/cloudabi64/cloudabi64_sysvec.c
64 ↗(On Diff #9447)

Better use sysent->pv_usrstack.

83 ↗(On Diff #9447)

FreeBSD uses single source for freebsd_fixup on all architectures, and even manages to compile 32 and 64 bit versions from single source.

Could you reduce the duplication for cloudabi ? Sources for arm64 and amd64 are identical.

159 ↗(On Diff #9447)

Why not td_retval[1] = tf_x[1] ?

199 ↗(On Diff #9447)

Indeed, this is fragile. Usurp a thread private flags bit (tf_pflags) and use it to indicate that either the thread forked or was created in existing process. The flag could be cleaned up in fork_exit() right after sv_schedtail().

256 ↗(On Diff #9447)

I suggest that everything in this file after the line should be also de-duplicated.

ed marked 3 inline comments as done.
ed edited edge metadata.

Move code shared between amd64 and arm64 into a separate file.

Introduce cloudabi64_module.c that will contain the module declarations
for cloudabi64. Also move the stack initialization routines into this
source file, so that they can be used by cloudabi64_sysvec.c.

While there, don't use USRSTACK in cloudabi64_copyout_strings().

This doesn't change the code to properly distinguish between fork() and
thread creation yet. I should be able to look into that tomorrow.

ed edited edge metadata.

Run arc diff again?

It looks like cloudabi64_module.c isn't uploaded.

ed edited edge metadata.

Add an assertion to make sure TDP_FORKING is never left set.

ed edited edge metadata.

Hopefully final version of this change.

  • Sync up with r289747 and r289748, reducing this change to just the ARM64 specific bits.
  • Rebase cloudabi64_sysvec.c on top of the latest version of the amd64 version, to make the diff more obvious.
  • Bump the man page date.
ed edited edge metadata.
ed marked an inline comment as done.Oct 22 2015, 9:40 AM

Sorry for all the noise caused by decomposing this change into separate ones. I think we're now getting closer...

sys/arm64/cloudabi64/cloudabi64_sysvec.c
159 ↗(On Diff #9447)

If my understanding of the ARM64 calling convention is correct (which FreeBSD and CloudABI follow for the system call convention as well), there is no need to preserve x0...xN. This is why it's set to some constant, zero in this case. We also do this for FreeBSD in sys/arm64/arm64/vm_machdep.c.

I'm also fine with setting it to tf_x[1], but CloudABI's userspace won't care.

kib edited edge metadata.
kib added inline comments.
sys/arm64/cloudabi64/cloudabi64_sysvec.c
68 ↗(On Diff #9607)

Traditionally, FreeBSD syscalls did not changed the registers, except the subset which was used to return values. This was partially broken for amd64 in an attempt to micro-optimize the syscall timings, but even there, we preserve arg 1 and 2 (%rdi and %rsi). It is a traditional behaviour, possibly a question of the implementation quality, preserving the state where is no reason to destroy it.

110 ↗(On Diff #9607)

BTW, what is a use for the tid in the child process ?

This revision is now accepted and ready to land.Oct 22 2015, 10:11 AM
ed edited edge metadata.

Preserve x[1].

This revision now requires review to proceed.Oct 22 2015, 10:26 AM
sys/arm64/cloudabi64/cloudabi64_sysvec.c
68 ↗(On Diff #9607)

That's a good point. Let's preserve x1. It keeps more options open for the future.

110 ↗(On Diff #9607)

That's a good question.

The new thread in the new process needs to know this so that it can update the state used by the pthread library. It needs to know the thread ID for lock acquisitions later on, but also to update all of the locks that were held at the time the process forked.

https://github.com/NuxiNL/cloudlibc/blob/master/src/libc/sys/procdesc/pdfork.c#L47-L57

This is needed, as there is no dedicated gettid() system call. The C library always keeps track of the right value.

kib edited edge metadata.
This revision is now accepted and ready to land.Oct 22 2015, 11:06 AM
This revision was automatically updated to reflect the committed changes.