briansmith / ring

Safe, fast, small crypto using Rust
Other
3.74k stars 704 forks source link

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

Closed DavidHorton5339 closed 9 months ago

DavidHorton5339 commented 11 months ago

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 commented 11 months ago
  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?
codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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 ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

brocaar commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

#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 commented 10 months ago

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 commented 10 months ago

@vavrecan This is an optimisation issue. Please refer to https://github.com/briansmith/ring/issues/1866 to address the problem.

briansmith commented 10 months ago

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 https://github.com/briansmith/ring/pull/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 commented 10 months ago

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

brocaar commented 9 months ago

@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 commented 9 months ago

@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 commented 9 months ago

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

briansmith commented 9 months ago

1885 is merged.

DavidHorton5339 commented 9 months ago

New PR raised here https://github.com/briansmith/ring/pull/1894. Closing this one.