Page MenuHomeFreeBSD
Differential D3917

Add support for CloudABI on ARM64.
ClosedPublic
Actions

Authored by ed on Oct 16 2015, 10:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Feb 28, 5:07 PM
Unknown Object (File)
Mon, Feb 23, 3:23 AM
Unknown Object (File)
Wed, Feb 18, 5:37 PM
Unknown Object (File)
Wed, Feb 18, 2:01 PM
Unknown Object (File)
Wed, Feb 18, 2:00 PM
Unknown Object (File)
Wed, Feb 18, 10:25 AM
Unknown Object (File)
Wed, Feb 18, 10:25 AM
Unknown Object (File)
Tue, Feb 17, 4:11 PM
View All Files
Subscribers
emaste
imp

Details

Reviewers
kib
andrew
Group Reviewers
manpages
Commits
rS289752: Add support for CloudABI on ARM64.
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 Passed
Unit
No Test Coverage
Build Status
Buildable 878
Build 878: arc lint + arc unit

Event Timeline

ed updated this revision to Diff 9445.Oct 16 2015, 10:43 AM
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.
Herald added a reviewer: manpages. · View Herald TranscriptOct 16 2015, 10:43 AM
Herald added subscribers: emaste, imp. · View Herald Transcript
ed updated this revision to Diff 9447.Oct 16 2015, 11:14 AM
ed edited edge metadata.

Use MAXARGS instead of hardcoding 8.

Herald edited edge metadata. · View Herald TranscriptOct 16 2015, 11:14 AM
andrew added inline comments.Oct 18 2015, 1:27 PM
sys/arm64/cloudabi64/cloudabi64_sysvec.c
60

This should no longer be needed after rS289502.

108

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.

andrew added a reviewer: kib.Oct 18 2015, 1:29 PM
kib added inline comments.Oct 19 2015, 6:22 AM
sys/arm64/cloudabi64/cloudabi64_sysvec.c
49–50

Better use sysent->pv_usrstack.

49–50

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.

68

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

108

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().

162

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

ed updated this revision to Diff 9582.Oct 21 2015, 5:07 PM
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.

Herald edited edge metadata. · View Herald TranscriptOct 21 2015, 5:07 PM
ed updated this revision to Diff 9583.Oct 21 2015, 5:10 PM
ed edited edge metadata.

Run arc diff again?

It looks like cloudabi64_module.c isn't uploaded.

Herald edited edge metadata. · View Herald TranscriptOct 21 2015, 5:10 PM
ed updated this revision to Diff 9604.Oct 22 2015, 9:21 AM
ed edited edge metadata.

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

Herald edited edge metadata. · View Herald TranscriptOct 22 2015, 9:21 AM
ed updated this revision to Diff 9607.Oct 22 2015, 9:35 AM
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.
Herald edited edge metadata. · View Herald TranscriptOct 22 2015, 9:35 AM
ed updated this object.Oct 22 2015, 9:38 AM
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
68

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 accepted this revision.Oct 22 2015, 10:11 AM
kib edited edge metadata.
kib added inline comments.
sys/arm64/cloudabi64/cloudabi64_sysvec.c
68

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

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 updated this revision to Diff 9608.Oct 22 2015, 10:26 AM
ed edited edge metadata.

Preserve x[1].

This revision now requires review to proceed.Oct 22 2015, 10:26 AM
Herald edited edge metadata. · View Herald TranscriptOct 22 2015, 10:26 AM
ed added inline comments.Oct 22 2015, 10:26 AM
sys/arm64/cloudabi64/cloudabi64_sysvec.c
68

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

110

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 accepted this revision.Oct 22 2015, 11:06 AM
kib edited edge metadata.
This revision is now accepted and ready to land.Oct 22 2015, 11:06 AM
Closed by commit rS289752: Add support for CloudABI on ARM64. (authored by ed). · Explain WhyOct 22 2015, 11:09 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
share/
man/
man4/
7 lines
sys/
amd64/
cloudabi64/
119 lines
arm64/
arm64/
cloudabi64/
amd64/
cloudabi64/
166 lines
compat/
cloudabi64/
10 lines
conf/
1 line
1 line
modules/
5 lines
cloudabi64/
6 lines

Diff 9582

View Options

share/man/man4/cloudabi.4

Loading...
View Options

sys/amd64/cloudabi64/cloudabi64_sysvec.c

Loading...
View Options

sys/arm64/cloudabi64/

Loading...
View Options

sys/arm64/cloudabi64/cloudabi64_sysvec.c

Loading...
View Options

sys/compat/cloudabi64/cloudabi64_module.c

Loading...
View Options

sys/compat/cloudabi64/cloudabi64_util.h

Loading...
View Options

sys/conf/files

Loading...
View Options

sys/conf/files.arm64

Loading...
View Options

sys/modules/Makefile

Loading...
View Options

sys/modules/cloudabi64/Makefile

Loading...