Support Xtensa, RISC-V and ESP-IDF #1506
Conversation
|
This patch would also be extremely enabling for Xous on Precursor! We're trying to add secure messaging to our RISC-V mobile platform and getting Thanks for opening the PR @MabezDev, I really appreciate it! |
|
We are also currently using a patched version of ring for Espressif based platforms - would really love to see this getting upstreamed! My thoughts regarding the dynamically sized arrays: |
|
@briansmith would you mind taking a look at this PR when you get a few moments? It would be really great to get some feedback :). |
|
Just have seen https://twitter.com/feistyduck/status/1577644279740387328: |
|
I can confirm that this patch fixed building ring for RISC-V for me. I also had to apply this patch: --- a/build.rs
+++ b/build.rs
@@ -236,6 +236,13 @@ const ASM_TARGETS: &[AsmTarget] = &[
asm_extension: "S",
preassemble: true,
},
+ AsmTarget {
+ oss: LINUX_ABI,
+ arch: "riscv64",
+ perlasm_format: "linux64",
+ asm_extension: "S",
+ preassemble: false,
+ },
];
struct AsmTarget {I was also building for a non-standard RISC-V arch and had to do: export CRATE_CC_NO_DEFAULTS=1before calling cargo build. |
There was a problem hiding this comment.
Thanks for the PR. Please see the inline comments. I'd like to separate the PR that adds support for the OS from the PR that adds the fallback implementation, and I'd like us to implement the approach outlined in #1455 if at all practical.
We should explore some alternate solution to the '-Winlineissue as I understand why it is problematic for you, but also I do find the warnings to be very helpful. Perhaps we should just remove-Winline` for now.
| @@ -229,6 +230,21 @@ mod sysrand_chunk { | |||
| } | |||
| } | |||
|
|
|||
| #[cfg(target_os = "espidf")] | |||
There was a problem hiding this comment.
This part should be updated to just add target_os = "espidf" to the allowlist in the new version of rand.rs, after reviewing that the getrandom crate implements this functionality for this operating system correctly. Please do this change in a separate PR.
|
|
||
| #if !defined(OPENSSL_X86) && !defined(OPENSSL_X86_64) && \ | ||
| !defined(OPENSSL_ARM) && !defined(OPENSSL_AARCH64) | ||
| void bn_mul_mont(BN_ULONG *rp, const BN_ULONG *ap, const BN_ULONG *bp, |
There was a problem hiding this comment.
Let's solve this in a PR that isn't specific to any OS/architecture. I will contact the original author of this code to get it submitted so there's no IPR concerns.
| (&[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"), | ||
| (&[], "crypto/crypto.c"), |
There was a problem hiding this comment.
I don't think a change for crypto.c is needed.
| @@ -167,7 +167,7 @@ untrusted = { version = "0.9" } | |||
| [target.'cfg(any(target_arch = "x86",target_arch = "x86_64", all(any(target_arch = "aarch64", target_arch = "arm"), any(target_os = "android", target_os = "fuchsia", target_os = "linux", target_os = "windows"))))'.dependencies] | |||
| spin = { version = "0.9.2", default-features = false, features = ["once"] } | |||
|
|
|||
| [target.'cfg(any(target_os = "android", target_os = "linux"))'.dependencies] | |||
| [target.'cfg(any(target_os = "android", target_os = "linux", target_os = "espidf"))'.dependencies] | |||
There was a problem hiding this comment.
I don't think this change will be needed once you update the rand.rs changes.
| @@ -201,6 +201,7 @@ slow_tests = [] | |||
| std = ["alloc"] | |||
| test_logging = [] | |||
| wasm32_unknown_unknown_js = ["web-sys"] | |||
| size_optimized = [] | |||
There was a problem hiding this comment.
I'd rather not control this with a feature flag. Perhaps you can work around this with CFLAGS in your build environment for now?
| #elif defined(__riscv) && __riscv_xlen == 64 | ||
| #define OPENSSL_64_BIT | ||
| #elif defined(__riscv) && __riscv_xlen == 32 | ||
| #define OPENSSL_32_BIT |
There was a problem hiding this comment.
Please reconsider this in light of the approach I suggest in #1455.
|
@MabezDev Thanks again for the PR. Because my comments are tantamount to "redo the whole thing and split it up into multiple PRs," and because you aren't the original author of the |
|
@MabezDev Been wanting to try use rustls in my ESP32S3 project. First had to port these changes to version InvestigatingobjdumpHere I notice Seems like Removed Put back Had wrong function name. I guess it was renamed in 0.17. Changed It compiles now, but at link time I get a bit more linking errors for undefined references. If I don't try to use my TcpAcceptor with SSL it compiles fine. I guess it requires nistz256 methods and as we don't have them created for xtensa it fails at link time. linker errorsFrom what I see we don't have it implemented in 0.17 version as well. Seems like Seems like EC algs are impacted. While I manage to stick with RSA and not link and code related to EC it should work |
This is a continuation of the upstreaming effort from #1459.
We would really like some feedback on this PR, we wish to use
ringto secure millions of Espressif devices. If could let us know what reservations you have about this PR, I would be more than happy to address them.Quoting from the original PR for comments about the PR itself:
A few points: