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

Implement SM3 Example and Testing #68

Merged
merged 7 commits into from
Feb 22, 2021
Merged

Conversation

HCPauKaifler
Copy link
Contributor

Closes #15

@ben-marshall
Copy link
Member

Hi @HCPauKaifler - this is great, thank you very much for contributing.
I tried to build and run the benchmarks, but I think the file benchamarks/test/test_hash_sm3.c1 is missing?
When trying to run, I get:

work@ben-TF:~/riscv/riscv-crypto$ make -B -C benchmarks/ build-test-sm3_reference build-test-sm3_zscrypto_rv64 run-test-sm3_reference run-test-sm3_zscrypto_rv64 
make: Entering directory '/home/work/riscv/riscv-crypto/benchmarks'
sha3/zscrypto_rv64/Makefile.in:10: warning: overriding recipe for target '/home/work/riscv/riscv-crypto/build/benchmarks/rv64-zscrypto/obj/sha3/fips202.o'
sha3/reference/Makefile.in:6: warning: ignoring old recipe for target '/home/work/riscv/riscv-crypto/build/benchmarks/rv64-zscrypto/obj/sha3/fips202.o'
sha3/zscrypto_rv64/Makefile.in:10: warning: overriding recipe for target '/home/work/riscv/riscv-crypto/build/benchmarks/rv64-zscrypto/dis/sha3/fips202.dis'
sha3/reference/Makefile.in:6: warning: ignoring old recipe for target '/home/work/riscv/riscv-crypto/build/benchmarks/rv64-zscrypto/dis/sha3/fips202.dis'
sha3/zscrypto_rv64/Makefile.in:10: warning: overriding recipe for target '/home/work/riscv/riscv-crypto/build/benchmarks/rv64-zscrypto/dis/sha3/fips202.size'
sha3/reference/Makefile.in:6: warning: ignoring old recipe for target '/home/work/riscv/riscv-crypto/build/benchmarks/rv64-zscrypto/dis/sha3/fips202.size'
make: *** No rule to make target 'test/test_hash_sm3.c', needed by '/home/work/riscv/riscv-crypto/build/benchmarks/rv64-zscrypto/bin/test/test_hash_sm3-sm3_reference.elf'. Stop.
make: Leaving directory '/home/work/riscv/riscv-crypto/benchmarks'

If you add the missing file, it looks like everything should work?
Cheers,
Ben

@mjosaarinen
Copy link
Collaborator

mjosaarinen commented Jan 3, 2021

Hi,

I'm pretty sure this code would benefit from some optimization if intended as a benchmark -- this is not really how SM3 or other MD hash functions are usually implemented.

Ideally, the instruction counts should match or be better than those at https://github.com/mjosaarinen/lwsha_isa#sm3

Currently, the state words are rotated in a way that can't easily be handled by a compiler. Furthermore, it expands the message, generating many unnecessary loads and stores. A RISC-V benchmark can leverage the large register file which allows the entire 16-word message block to be stored in the register file.

See e.g. the SM3 reference code at https://github.com/mjosaarinen/lwsha_isa/blob/master/sm3_rv32_cf.c

However, that earlier code will need to adopt the appropriate intrinsics for P0, P1, GREV, ROTATE, etc.

Cheers,

  • markku

@ben-marshall
Copy link
Member

Hi @HCPauKaifler

Apologies I took so long to get around to this again, I'm back from leave now and can give it some attention.

I've pulled your latest commit with the testbench file and everything builds and runs well, so thanks for that! I'm sorry I didn't actually get to review the code until now.

Broadly though, Markku is right, there are some optimisations to be done. Concretely, I'd say:

  • Inlining sm3_expand into sm3_compress and avoiding the extra loads and stores.
  • Updating the REVERSE_BITS_32 macros to use the Bitmanip grevi instructions.
  • This may just be my preference, but inlining memcpy where it is used. I don't think the overhead of calling it is worth while for such small data sizes.
  • In terms of when to stop, Markku's lwsha repo is what to aim for in terms of instruction counts.

If you don't expect you'll have time to do these optimisations, that's no problem, just let me know and I'm happy to merge this in as-is, and do the various optimisations when I (or someone else) gets the chance. Everything fits into the existing benchmark framework well and just having a solid base / set of tests to start optimising from is really good.

Cheers,
Ben

This commit optimizes the RV32 SM3 implementation to yield a speedup of
about 2.6x the original implementation.

The RV64 version is faster now too, but the toolchain seems to be
broken as grev and rol don't seem to compile. So it's still a lot slower.
@HCPauKaifler
Copy link
Contributor Author

Hello,

thanks for your tips @ben-marshall and @mjosaarinen!
I have implemented them for RV32 and got a speed-up of about 2.6x.
There seem to be some issues with the toolchain, so GREV and ROL don't seem to compile with RV64.

With Test 4 I am now hashing about 63 IPB using the reference, and 33 IPB with GREV, ROL, P0 and P1 enabled on RV32.

Best regards,
Pau

@ben-marshall
Copy link
Member

ben-marshall commented Feb 16, 2021

Hi @HCPauKaifler
You are right, there was a bug in what instructions the assembler would accept. I've pushed a fix to the dev/next-release branch. Does that work for you?
Great to hear about the performance improvements!
Cheers,
Ben

@HCPauKaifler
Copy link
Contributor Author

HCPauKaifler commented Feb 16, 2021

Hello @ben-marshall
Thanks for the fix on RV64!
Now GREV and ROL work for RV64. Now however GREV and ROL no longer work on RV32. Spike just hangs up when it gets to them.

Best regards,
Pau Kaifler

@ben-marshall ben-marshall merged commit 934e1e7 into riscv:master Feb 22, 2021
@ben-marshall
Copy link
Member

Hi @HCPauKaifler
The cause of Spike hanging was my fault. I fixed it a few days ago, but only just got around to testing your branch. Everything works, and though there are still some performance improvements to be made, it's better than before and I've merged it in. I also don't want to keep you on the hook with this any longer, since I know it's not your primary focus right now!

Thanks a lot for your efforts!
Cheers,
Ben

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.

SM3 example code
3 participants