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

syscall: Fix SuccessU32U64 format #3175

Merged
merged 1 commit into from Aug 29, 2022
Merged

syscall: Fix SuccessU32U64 format #3175

merged 1 commit into from Aug 29, 2022

Conversation

dcz-self
Copy link

TRD104 specifies that the 32-bit value goes before, not after the 64-bit one.

Pull Request Overview

This pull request fixes syscall format to match TRD104 3.2 Return Values

Testing Strategy

This pull request was tested by matching with libtock-rs in a test project.

TODO or Help Wanted

This pull request still needs review.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

TRD104 specifies that the 32-bit value goes before, not after the 64-bit one.
@bradjc bradjc added the release-blocker Issue or PR that must be resolved before the next release label Aug 29, 2022
Copy link
Contributor

@hudson-ayers hudson-ayers left a comment

Choose a reason for hiding this comment

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

I submitted tock/libtock-c#294 to match this change

bors bot added a commit to tock/libtock-c that referenced this pull request Aug 29, 2022
294: fix SuccessU32U64 syscall return variant name to match TRD104 r=ppannuto a=hudson-ayers

Matches updates in tock/tock#3175

Co-authored-by: Hudson Ayers <hayers@stanford.edu>
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

Nice catch. Mercifully nothing (in-tree at least) is using this yet, so hopefully no impact from the syscall behavior technically changing. I'm comfortable calling this a bugfix to match documented intent.

@ppannuto
Copy link
Member

bors r+

@lschuermann
Copy link
Member

Do we want to merge this before 2.1? I know we're technically changing ABI semantics w.r.t. allow and subscribe already, but this seems like it would constitute a major change in the system call ABI, would it not? I know we're probably not strictly following semantic versioning, but as far as I know changing the system call encoding was one of the arguments for performing a major version bump with 2.0. Alternatively, we might just be able to update the TRD instead, to match the current implementation?

@lschuermann
Copy link
Member

Okay, didn't see @ppannuto's comment. If nothing is using this yet, I'm fine with this change in 2.1.

@ppannuto
Copy link
Member

Yeah, IIRC the TRD was designed with some understanding of architectural calling conventions here: namely, when passing a 64-bit value, ARM will pass it in {r0,r1} or {r2,r3}, so aligning it this way allows the whole return to live in registers; while the other way would require some manipulation in the low-level bits of libtock-c after the syscall once things are types again.

@brghena
Copy link
Contributor

brghena commented Aug 29, 2022

@ppannuto That was exactly the information I was looking for: whether the TRD or implementation were correct given real-world constraints. It looks like the TRD was correct, which makes this indeed a bugfix.

@brghena
Copy link
Contributor

brghena commented Aug 29, 2022

In the future though, this seems like the type of thing that is at least potentially a big enough change to warrant staying open for at least 24 hours to give people a chance to think and comment.

@bradjc
Copy link
Contributor

bradjc commented Aug 29, 2022

I don't see how we can merge this without approval from either of the two lead architects of the 2.0 syscalls.

@bors
Copy link
Contributor

bors bot commented Aug 29, 2022

@bors bors bot merged commit 6e41531 into tock:master Aug 29, 2022
@ppannuto
Copy link
Member

Sorry, I got trigger happy with our goals of trying to get the release out the door.

@lschuermann
Copy link
Member

lschuermann commented Aug 29, 2022

Yeah, IIRC the TRD was designed with some understanding of architectural calling conventions here: namely, when passing a 64-bit value, ARM will pass it in {r0,r1} or {r2,r3}, so aligning it this way allows the whole return to live in registers; while the other way would require some manipulation in the low-level bits of libtock-c after the syscall once things are types again.

@ppannuto This makes sense, and I recall the discussions around these issues, which makes me wonder why we haven't implemented the proper encoding. One very likely explanation is indeed that we haven't used this particular encoding yet. I was on the fence about this change when I saw it, which also motivated my above comment, since -- even though this wasn't used by us (i.e. capsules) -- it's still a change to the implemented ABI contract. It is a breaking change for downstream users of the kernel for sure. I'd say that with this change merged, we should at the very least take a look at the ASM on the kernel and userspace side of code using this particular return value encoding, and perform some basic sanity tests for good measure before 2.1, just like we did when developing 2.0.

tyler-potyondy pushed a commit to tyler-potyondy/libtock-c that referenced this pull request Mar 13, 2024
294: fix SuccessU32U64 syscall return variant name to match TRD104 r=ppannuto a=hudson-ayers

Matches updates in tock/tock#3175

Co-authored-by: Hudson Ayers <hayers@stanford.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel release-blocker Issue or PR that must be resolved before the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants