RustCrypto / asm-hashes

Assembly implementations of cryptographic hash functions
46 stars 26 forks source link

s390x support for asm-hashes #44

Closed vishwabmc closed 2 years ago

vishwabmc commented 2 years ago

Hello,

While building https://github.com/filecoin-project/lotus on s390x platform on a Big Endian architecture, saw an error message https://github.com/filecoin-project/lotus/issues/1779 and I followed the suggestions to build libfilcrypto.so from the source.

While doing so, the build failed because it was depending on asm-hashes and it did not have support for s390x platform.

To get going, we have generated .S based on the https://www.nayuki.io/res/fast-sha2-hashes-in-x86-assembly/sha256.c for example. This got us going with building libfilcrypto for s390x. However, not sure if this is the right thing to do since we could have used .c file directly anyway.

Would you have any plans on providing an implementation for s390x ?

vishwabmc commented 2 years ago

https://github.com/udai05/asm-hashes is what we used to get going.

newpavlov commented 2 years ago

Is there a noticeable performance improvement on this platform with the asm backend? If not, I suggest simply to not enable the asm feature.

vishwabmc commented 2 years ago

Is there a noticeable performance improvement on this platform with the asm backend? If not, I suggest simply to not enable the asm feature.

Thanks for the quick response @newpavlov . We just finished building it and setting up the tests and hence don't have the perf numbers yet.

Would you please help on how to disable asm feature ?. Thank you.

newpavlov commented 2 years ago

Would you please help on how to disable asm feature ?

It's disabled by default, so either your project or one of your dependencies enable it. If it's the latter, I suggest fixing it, since this feature should not be enabled by library crates.

vishwabmc commented 2 years ago

Would you please help on how to disable asm feature ?

It's disabled by default, so either your project or one of your dependencies enable it. If it's the latter, I suggest fixing it, since this feature should not be enabled by library crates.

Thank you. However, I have not enabled it. All I did was this:

As you mentioned, one of the dependencies might be enabling it.

cargo tree shows sha2 v0.9.9 is what is pulling it.

├── sha2 v0.9.9 │ │ ├── block-buffer v0.9.0 │ │ │ └── generic-array v0.14.5 () │ │ ├── cfg-if v1.0.0 │ │ ├── digest v0.9.0 () │ │ ├── opaque-debug v0.3.0 │ │ └── sha2-asm v0.6.2

Another path that is pulling it is sha2raw

├── filecoin-proofs v11.0.0 │ │ ├── storage-proofs-porep v11.0.0 │ │ │ ├── sha2raw v6.0.0 │ │ │ │ ├── block-buffer v0.9.0 () │ │ │ │ ├── byteorder v1.4.3 │ │ │ │ ├── digest v0.9.0 () │ │ │ │ ├── fake-simd v0.1.2 │ │ │ │ ├── lazy_static v1.4.0 │ │ │ │ ├── opaque-debug v0.3.0 │ │ │ │ └──sha2-asm v0.6.2

newpavlov commented 2 years ago

It looks like storage-proofs-porep conditionally enables on ARM. Maybe it affects other targets as well? Have you tried to use resolver = "2"?

vishwabmc commented 2 years ago

It looks like storage-proofs-porep conditionally enables on ARM. Maybe it affects other targets as well? Have you tried to use resolver = "2"?

Okay.. I have not tried resolver = "2" before and I did that on the lotus/extern/filecoin-ffi/rust/Cargo.toml and it did not do the trick.

cargo-features = ["edition2021"] [package] name = "filcrypto" ..... ..... edition = "2021" publish = false resolver = "2"

That said, I am not well versed in this. Could you help with what change needed in which Cargo.toml ?

newpavlov commented 2 years ago

I only can say that you have to find the crate which is responsible for enabling the asm feature. Try to patch storage-proofs-porep by removing the linked lines. Also cargo tree -e features could help as well.

vishwabmc commented 2 years ago

I only can say that you have to find the crate which is responsible for enabling the asm feature. Try to patch storage-proofs-porep by removing the linked lines. Also cargo tree -e features could help as well.

Wow.. this is awesome.. cargo tree -e features really helped me narrow this down to rust-fil-proofs/sha2raw

I commented these below and things are working fine without the need of sha2-asm

-sha2-asm = { version = "0.6", optional = true } -[features] -default = ["asm"] -asm = ["sha2-asm"]

THANK YOU very much @newpavlov . Really appreciate your quick help on this.

vishwabmc commented 2 years ago

For the documentation, here are list of changes I did:

On lotus/extern/filecoin-ffi/rust/Cargo.toml :

--- a/rust/Cargo.toml
+++ b/rust/Cargo.toml
@@ -1,3 +1,4 @@
+cargo-features = ["edition2021"]
 [package]
 name = "filcrypto"
@@ -10,8 +11,9 @@ authors = [
...........

-edition = "2018"
+edition = "2021"
 publish = false
+resolver = "2"

+[patch.crates-io]
+sha2raw = { path = "/data/vishwa/new-lotus/rust-fil-proofs/sha2raw" }

On /data/vishwa/new-lotus/rust-fil-proofs/sha2raw/Cargo.toml

-sha2-asm = { version = "0.6", optional = true }
+#sha2-asm = { version = "0.6", optional = true }

-[features]
-default = ["asm"]
-asm = ["sha2-asm"]
+#[features]
+#default = ["asm"]
+#asm = ["sha2-asm"]

Followed by cargo update and then cargo build

newpavlov commented 2 years ago

You could simply remove the default = ["asm"] line or use default-features = false on sha2raw in storage-proofs-porep.

Do you still plan to measure performance of the asm backend? If not, I think we can close this issue.

vishwabmc commented 2 years ago

You could simply remove the default = ["asm"] line or use default-features = false on sha2raw in storage-proofs-porep.

Do you still plan to measure performance of the asm backend? If not, I think we can close this issue.

@newpavlov If I don't put the resolver="2" and related changes, it still brings sha2-asm as part of sha2-v0.9.9 and I am unable to patch it. Have tried couple ways to fix it, but have not been successful.

I cloned: https://github.com/RustCrypto/hashes and edited sha2/Cargo.toml and commented these lines:

[package] name = "sha2" version = "0.9.9"

[dependencies]

[features] default = ["std"] std = ["digest/std"]

And put sha2 under [patch.crates-io] in the top level Cargo.toml

When I do cargo update, I see this :

warning: Patch `sha2 v0.9.9 (/data/vishwa/new-lotus/hashes/sha2)` was not used in the crate graph.
Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.

if I remove updates to sha2/Cargo.toml, then cargo update goes through, but that does not help.

├── bellperson v0.18.2
│   ├── sha2 feature "default"
│   │   ├── sha2 v0.9.9 (/data/vishwa/new-lotus/hashes/sha2)
│   │   │   ├── cfg-if feature "default" (*)
│   │   │   ├── digest feature "default" (*)
│   │   │   ├── block-buffer feature "default"
│   │   │   │   └── block-buffer v0.9.0
│   │   │   │       └── generic-array feature "default" (*)
│   │   │   ├── opaque-debug feature "default"
│   │   │   │   └── opaque-debug v0.3.0
│   │   │   └── sha2-asm feature "default"
│   │   │       └── sha2-asm v0.6.2
│   │   │           [build-dependencies]
│   │   │           └── cc feature "default" (*)
newpavlov commented 2 years ago

It's not an issue with our crates, so I think the best course of action will be to update the filecoin crates to work properly around the asm feature (in particular, it should not be enabled by default in library crates). For further questions, I suggest using URLO.

vishwabmc commented 2 years ago

@newpavlov Agreed. For now, I am going ahead closing this. Post our test results, I will come back on this.

Again, thank you very much. Really appreciate the support !