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 AArch64 support for RSEQ. #50

Merged
merged 1 commit into from
Oct 19, 2020
Merged

Conversation

avieira-arm
Copy link
Contributor

Hi all,

This is a patch for initial RSEQ support for AArch64.

Tested on Ubuntu with 5.1.8+ kernel:
CC=clang bazel test -c dbg --copt "-mcpu=neoverse-n1" --linkopt "-fuse-ld=lld" --sandbox_debug --override_repository="tcmalloc=src/tcmalloc" -k //tcmalloc/..
CC=clang bazel test -c opt --copt "-mcpu=neoverse-n1" --linkopt "-fuse-ld=lld" --sandbox_debug --override_repository="tcmalloc=src/tcmalloc" -k //tcmalloc/..

On both occasions all tests ran and percpu_tcmalloc_test ran without skipping for slow path.

Is this OK?

I am currently looking at adding BTI to protect the branch targets and maybe even PAC for when we spill the LR onto the stack in START_RSEQ. Do you have any objections to this? I noticed the other ports don't use anything similar.

Another question I had is whether you might be open to re-using linux headers to specify these RSEQ sequences, like https://github.com/compudj/librseq/blob/master/include/rseq/rseq-arm64.h

I believe they lead to pretty much the same, but might help make it easier to rewrite the code in case things change in the future?

Kind regards,
Andre

@google-cla
Copy link

google-cla bot commented Oct 2, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Oct 2, 2020
Copy link
Contributor

@resistor resistor left a comment

Choose a reason for hiding this comment

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

I'm not an expert on rseq, but I've highlighted a few areas where I believe the asm sequences could be tighter.

tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
tcmalloc/internal/BUILD Outdated Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
@ckennelly
Copy link
Collaborator

We'll also need to get the CLA fixed before merging.

@avieira-arm
Copy link
Contributor Author

I have fixed the indentation problem pointed out by Owen. The other comments Owen provided are either blocked because of the need to not clobber argument registers in the critical section or me not having found a valid addressing mode that would improve performance.

Copy link
Contributor

@resistor resistor left a comment

Choose a reason for hiding this comment

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

LGTM

tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
* size_t len (w3) {
* uint64_t x8 = __rseq_abi.cpu_id
* uint64_t* x8 = CpuMemoryStart(x0, r8)
* Header* hdr = x8 + w1 * 8
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Let's note that we're using x4 for hdr

tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
tcmalloc/internal/percpu_rseq_aarch64.S Outdated Show resolved Hide resolved
@ckennelly
Copy link
Collaborator

During some internal testing, I noticed a need to make sure TCMalloc is defining its own rseq-related macros (rather than requiring internal functions to follow TCMalloc's implementation's in lock-step). I should have a commit landed in the tree soon to rename those, but that'll require rebasing...

@avieira-arm
Copy link
Contributor Author

I'll wait for that commit then.

@avieira-arm
Copy link
Contributor Author

Had messed up the condition, now this should work, skip the test if the version < 9. For versions 9 or newer it will still check it is using the small memory model and error otherwise.

tcmalloc/internal/BUILD Outdated Show resolved Hide resolved
@ckennelly ckennelly merged commit d5738eb into google:master Oct 19, 2020
@ckennelly ckennelly linked an issue Oct 19, 2020 that may be closed by this pull request
copybara-service bot pushed a commit that referenced this pull request Oct 19, 2020
PiperOrigin-RevId: 337894411
Change-Id: I372c99c367bd44ae6d096fc1f3219359f3395a94
@ckennelly ckennelly linked an issue Oct 19, 2020 that may be closed by this pull request
suiliangchu added a commit to suiliangchu/tcmalloc that referenced this pull request Aug 29, 2022
PiperOrigin-RevId: 337894411
Change-Id: I372c99c367bd44ae6d096fc1f3219359f3395a94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RSEQ support for AArch64 Plans to support ARM64 as architecture?
4 participants