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

Added MIPS (big-endian, 32-bit) support #1859

Closed

Conversation

DavidHorton5339
Copy link
Contributor

Now that ring has generic fallback code, we can compile it for new targets by allowing them through target.h checks.
I'd like to add MIPS support. This works well (all unit tests pass, and our application works on the target).
Unfortunately the Rust compiler recently got broken for MIPS, so version 1.71 or earlier is required.

@briansmith
Copy link
Owner

  1. Please squash all these commits into one commit.
  2. Could you copy/paste/modify the existing mips stuff in mk/cargo.sh and mk/install-build-tools.sh so mk/cargo.sh test --target=whatever-the-target-triple-is works?

Copy link

codecov bot commented Dec 19, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c3fda8b) 96.00% compared to head (d8a5e4c) 96.03%.
Report is 99 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1859      +/-   ##
==========================================
+ Coverage   96.00%   96.03%   +0.02%     
==========================================
  Files         138      136       -2     
  Lines       20746    20761      +15     
  Branches      226      226              
==========================================
+ Hits        19918    19938      +20     
+ Misses        790      789       -1     
+ Partials       38       34       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brocaar
Copy link

brocaar commented Dec 20, 2023

@briansmith @DavidHorton5339 I think the last remaining action is to squash all commits into a single commit before this can be merged, is that correct?

The failing tests seem to be related to the Codecov API returning an error, not to the code changes in this PR.

@DavidHorton5339
Copy link
Contributor Author

Would a squash on merge be acceptable? Git seems unable to squash the commits - perhaps because they span both main and PR branches in my fork repo and aren't all on one branch. Furthermore, I'd like to keep the 0.16-compatible version within the series of commits, in case I want to rebuild a past version of my project.

@DavidHorton5339
Copy link
Contributor Author

@briansmith Happy New Year to you.
Can you comment on whether a squash on merge be acceptable, as squashing existing commits across multiple branches probably won't work? If you'd rather, I can raise a new PR on a clean branch.

@vavrecan
Copy link

vavrecan commented Jan 9, 2024

Hello, what is the status of this please? looking to compile some package to embedded device and pretty much getting this error cargo:warning=include/ring-core/target.h:63:2: error: #error "Unknown target CPU"

cargo:warning= 63 | #error "Unknown target CPU"

cargo:warning= | ^~~~~

@DavidHorton5339
Copy link
Contributor Author

#error "Unknown target CPU" means that the target isn't supported. My change allows support for MIPS 32-bit big-endian, so if this is what you're trying, my proposed change would eliminate this problem.
Meanwhile, as a temporary measure, this patch in your project's top level Cargo.toml will point it to my version of the code, until the PR is approved.

[patch.crates-io]
ring = { git = "https://github.com/CyberHive/ring", branch = "mips_be_32_support_only" }

@vavrecan
Copy link

vavrecan commented Jan 9, 2024

Thank you that helped! hovewer there are plenty of warnings like this:
cargo:warning=In function 'aes_nohw_mix_columns',
cargo:warning= inlined from 'aes_nohw_encrypt_batch' at crypto/fipsmodule/aes/aes_nohw.c:757:5:
cargo:warning=crypto/fipsmodule/aes/aes_nohw.c:691:26: error: inlining failed in call to 'aes_nohw_rotate_rows_twice': call is unlikely and code size would grow [-Werror=inline]

@DavidHorton5339
Copy link
Contributor Author

@vavrecan This is an optimisation issue. Please refer to #1866 to address the problem.

@briansmith
Copy link
Owner

Can you comment on whether a squash on merge be acceptable, as squashing existing commits across multiple branches probably won't work? If you'd rather, I can raise a new PR on a clean branch.

The only merge commits we (intentionally) do are the merges from BoringSSL. All other commits we do are rebase commits. Also, in our commit history we want every commit to be a "clean" commit that can stand on its own, since the commit history serves as the changelog for this project. Since you are able to do a new "clean" PR, I would prefer you to do so.

My plan is to merge #1885 and then ask for this and all other endian-related PRs to be rebased on top of that. Could you please review PR #1885? Then I will prioritize merging your big-endian MIPS support and I will do the release immediately afterwords.

@briansmith
Copy link
Owner

@vavrecan @DavidHorton5339 Regarding the inlining warnings/errors, I will comment further in the issue #1866.

@brocaar
Copy link

brocaar commented Jan 10, 2024

@briansmith

Also, in our commit history we want every commit to be a "clean" commit that can stand on its own, since the commit history serves as the changelog for this project.

I believe with a squash merge as @DavidHorton5339 proposes you get exactly that? It will combine all the commits from the pull-request as a single (new) commit on top of the branch in which it gets merged (https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-commits).

@DavidHorton5339
Copy link
Contributor Author

@brocaar I think you are right about squash merge. The only reason to avoid a squash merge that I know of is that it messes up signed commits (as the final squashed commit ends up coming from github, not from me).
In any case I'm happy to do a fresh PR with this simple change, it's 5 mins work. I'll do this when now-approved https://github.com/briansmith/ring/pull/1885 is merged to avoid any rebasing/merge conflicts etc.

@brocaar
Copy link

brocaar commented Jan 10, 2024

@DavidHorton5339 ah yes, you are right about signed commits :-) I'm looking forward to getting this work merged in, thanks for you work 👍

@briansmith
Copy link
Owner

#1885 is merged.

@DavidHorton5339
Copy link
Contributor Author

New PR raised here #1894. Closing this one.

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.

4 participants