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 MIPS support #1277

Closed
wants to merge 4 commits into from
Closed

Add MIPS support #1277

wants to merge 4 commits into from

Conversation

bltavares
Copy link

Built on top of #1120

This commit ports the changes from #1181 into the updated codebase

The following command is working:

cross build --target mipsel-unknown-linux-musl

But tests are still not working:

cross test --target mipsel-unknown-linux-musl

LinusU and others added 2 commits May 3, 2021 22:13
This commit ports the changes from briansmith#1181 into the updated codebase

The following command is working:
> cross build --target mipsel-unknown-linux-musl

But tests are still not working:
> cross test --target mipsel-unknown-linux-musl
Cargo.toml Outdated
@@ -58,6 +58,7 @@ include = [
"crypto/fipsmodule/bn/asm/x86-mont.pl",
"crypto/fipsmodule/bn/asm/x86_64-mont.pl",
"crypto/fipsmodule/bn/asm/x86_64-mont5.pl",
"crypto/fipsmodule/bn/asm/mips-mont.pl",
Copy link
Owner

Choose a reason for hiding this comment

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

I understand why you're trying to do it this way, but this is exactly what I'm hoping to avoid. Let's find a way to do it without bringing in any MIPS assembly code. It's just not realistic to maintain the MIPS assembly code given current constraints. I'd rather concentrate on improving the C (in the near future, Rust) code instead.

Copy link
Author

Choose a reason for hiding this comment

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

I'm mostly reverse-engineering the error messages which complained about bn_mul_mont not existing, and I found this file defines it. I'm ok with removing it if I can figure out another way to overcome the error message.

build.rs Outdated
(&[AARCH64, ARM, X86_64, X86], "crypto/fipsmodule/ec/gfp_p256.c"),
(&[AARCH64, ARM, X86_64, X86], "crypto/fipsmodule/ec/gfp_p384.c"),
(&[AARCH64, ARM, X86_64, X86], "crypto/fipsmodule/ec/p256.c"),
(&[AARCH64, ARM, MIPS, MIPS64, X86_64, X86], "crypto/crypto.c"),
Copy link
Owner

Choose a reason for hiding this comment

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

crypto.c isn't relevant to MIPS so this should be reverted.

build.rs Outdated
(&[AARCH64, ARM, MIPS, MIPS64, X86_64, X86], "crypto/fipsmodule/ec/ecp_nistz.c"),
(&[AARCH64, ARM, MIPS, MIPS64, X86_64, X86], "crypto/fipsmodule/ec/gfp_p256.c"),
(&[AARCH64, ARM, MIPS, MIPS64, X86_64, X86], "crypto/fipsmodule/ec/gfp_p384.c"),
(&[AARCH64, ARM, MIPS, MIPS64, X86_64, X86], "crypto/fipsmodule/ec/p256.c"),
Copy link
Owner

Choose a reason for hiding this comment

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

Just use &[] for all of these. An empty list means "every platform."

build.rs Outdated
@@ -200,6 +204,20 @@ const ASM_TARGETS: &[AsmTarget] = &[
asm_extension: "S",
preassemble: false,
},
AsmTarget {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove the AsmTarget stuff for MIPS since my goal is to not have any assembly code for MIPS.

build.rs Outdated
@@ -322,6 +341,7 @@ fn ring_build_rs_main() {

let target = Target {
arch,
endian,
Copy link
Owner

Choose a reason for hiding this comment

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

No need to store endian in the target since no big-endian targets are supported.

build.rs Outdated
@@ -391,6 +412,10 @@ fn build_c_code(target: &Target, pregenerated: PathBuf, out_dir: &Path, ring_cor
}
}

if target.arch == "mips" && target.endian == "big" {
Copy link
Owner

Choose a reason for hiding this comment

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

No need for target.arch == "mips" because no big-endian targets are supported yet.

build.rs Outdated
@@ -391,6 +412,10 @@ fn build_c_code(target: &Target, pregenerated: PathBuf, out_dir: &Path, ring_cor
}
}

if target.arch == "mips" && target.endian == "big" {
panic!("MIPS Big-Endian detected. Stoping compilation as BoringSSL code are not available for this platform");
Copy link
Owner

Choose a reason for hiding this comment

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

I would just write "Big-endian targets are not supported yet."

* this file except in compliance with the License. You can obtain a copy
* in the file LICENSE in the source distribution or at
* https://www.openssl.org/source/license.html
*/
Copy link
Owner

Choose a reason for hiding this comment

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

I think (hope) we can avoid adding this file.

target_arch = "x86"
target_arch = "x86",
target_arch = "mips",
target_arch = "mips64",
Copy link
Owner

Choose a reason for hiding this comment

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

Everywhere in this file, I would expect no conditional logic for MIPS because there is a default implementation for platforms for which we don't have assembly. If those default implementations are not working for MIPS, let's investigate together why not.

@bltavares
Copy link
Author

As is, the linker fails to find the bn_mul_mont function:

 = note: /target/mipsel-unknown-linux-musl/debug/deps/libring-f34856797b3553d1.rlib(gfp_p384.o): In function `elem_mul_mont':
          /project/crypto/fipsmodule/ec/gfp_p384.c:162: undefined reference to `ring_core_0_17_0_alpha_9_bn_mul_mont'
          /project/crypto/fipsmodule/ec/gfp_p384.c:162: undefined reference to `ring_core_0_17_0_alpha_9_bn_mul_mont'
          collect2: error: ld returned 1 exit status

Running nm on the output dir, we can see that the object file contains the expect function:

% docker run -ti --rm -v ${PWD}:/project:Z rustembedded/cross:mipsel-unknown-linux-musl-0.2.1  nm /project/target/mipsel-unknown-linux-musl/debug/build/ring-9ee2ef63756708a4/out/mips-mont-elf.o

00000000 T bn_mul_mont
00000020 t bn_mul_mont_internal

The output folder also contains fils for ring_core_generated\prefix_symbols{,_asm}.h , and both contains:

#define bn_mul_mont ring_core_0_17_0_alpha_9_bn_mul_mont

I'm not sure how to further figure out the missing link

@bltavares
Copy link
Author

@briansmith I'm not sure how we'll make certain C files compile wihout adding the assembly definition for bn_mul_mont.
Currently, gfp_p256.c and gfp_p384.c files refer this function.

  = note: /target/mipsel-unknown-linux-musl/debug/deps/libring-f34856797b3553d1.rlib(gfp_p384.o): In function `elem_mul_mont':
          /project/crypto/fipsmodule/ec/gfp_p384.c:162: undefined reference to `ring_core_0_17_0_alpha_9_bn_mul_mont'
          /project/crypto/fipsmodule/ec/gfp_p384.c:162: undefined reference to `ring_core_0_17_0_alpha_9_bn_mul_mont'
          collect2: error: ld returned 1 exit status

@briansmith
Copy link
Owner

OK, I see the problem. I was mistaken about bn_mul_mont. It turns out that I didn't implement a fallback for bn_mul_mont in the way I expected. I had implemented a fallback in the Rust wrapper, but not one that can be called by C code. Here the build fails because bn_mul_mont is still being used by p256_scalar_mul_mont, p256_scalar_sqr_rep_mont, and the gfp_p384.c implementation of elem_mul_mont.

The best thing to do is make a separate PR that transliterates those C functions to the Rust equivalent. Note that those functions are very simple. The Rust implementations of those functions should use the Rust function limbs_mont_mul instead of bn_mul_mont. For targets archs other than the big 4, limbs_mont_mul has a fallback implementation (in Rust, parts in C) that will work on MIPS.

Then, we can rebase this PR on top of that PR and everything should work.

@briansmith
Copy link
Owner

Note that in order for the MIPS assembly to work, we'd need to also bring in the MIPS perlasm translator, and then also patch it with whatever patches would be needed. That's pretty error-prone and I'd rather focus on replacing gfp_p256.c and gfp_p384.c with Rust code, which will solve the problem with less assembly and less C and less Perl.

@bltavares
Copy link
Author

bltavares commented May 4, 2021

Does it mean to keep gfp_p256.c and gfp_p384.c as part of the build for the big 4 or fully replace it for all platforms? I'm not familiar with any sort of crypto code (but I'm comfortable fighting build tools hehe)

I can attempt it later on the weekend to replace the functions with Rust functions.

@briansmith
Copy link
Owner

Does it mean to keep gfp_p256.c and gfp_p384.c as part of the build for the big 4 or fully replace it for all platforms?

  1. Delete the code for the functions in those files that call bn_mul_mont.
  2. In the case of gfp_p256.c, that will be the whole file, but not for gfp_p384.c.
  3. Transliterate the implementations of the deleted functions into Rust, substituting limbs_mont_mul for bn_mul_mont. The replacement for the gfp_p256.c functions goes in p256.rs and the replacements for the functions in gfp_p384.c go in p384.rs. Note that the C constant N is COMMON_OPS.n in Rust and the C constant Q is COMMON_OPS.q in Rust.

I'm not familiar with any sort of crypto code (but I'm comfortable fighting build tools hehe)

Understandable. These functions don't implement any tricky crypto, however I understand that if you're not familiar with the code then it is going to be hard. I will try to find some time on the weekend to do it.

@vDorst
Copy link

vDorst commented Jul 26, 2021

Any news?

I would like to cross compile a project for a mipsel device that is using rust-lts.
But now it fails to compile because:

error: failed to run custom build command for `ring v0.16.20`

Caused by:
  process didn't exit successfully: `/target/release/build/ring-fbbd3758c762a457/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /cargo/registry/src/github.com-1ecc6299db9ec823/ring-0.16.20/build.rs:358:10
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

target: mipsel-unknown-linux-musl

@lancethepants
Copy link

Any news?

I would like to cross compile a project for a mipsel device that is using rust-lts. But now it fails to compile because:

error: failed to run custom build command for `ring v0.16.20`

Caused by:
  process didn't exit successfully: `/target/release/build/ring-fbbd3758c762a457/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /cargo/registry/src/github.com-1ecc6299db9ec823/ring-0.16.20/build.rs:358:10
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...

target: mipsel-unknown-linux-musl

Also encountering this issue with mipsel-unknown-linux-uclibc

@briansmith
Copy link
Owner

@bltavares Can you rebase this on top of #1558. Considering the ideas in #1555 and the work in #1297, I think the main thing to take from this PR is the changes to the scripts in mk/ so that once the general issues with endianness are addressed, the build system will fully support the big-endian MIPS too. If you could help figure out how to get Rust toolchains for MIPS to add the profiler builtins, e.g. by cribbing from rust-lang/rust#104304, that would also be awesome because then we could use the mipsel-unknown-linux-gnu for the little-endian code coverage measurement.

@briansmith
Copy link
Owner

Thanks for your help. MIPS support was added in another PR and then temporarily removed in PR #1637; see #562 (comment) for what needs to be done to add it back.

Closing this due to inactivity.

@briansmith briansmith closed this Sep 30, 2023
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.

5 participants