tillitis / tillitis-key1

Board designs, FPGA verilog, firmware for TKey, the flexible and open USB security key 🔑
https://www.tillitis.se
400 stars 26 forks source link

picorv32 built without DIVision, but extension "m" includes div/rem (rv32imc) #12

Closed quite closed 2 years ago

quite commented 2 years ago

@bjoto wrote:

The picorv32 on the key is built with ENABLE_DIV=0, and we're compiling the source (fw/app) with the "m extension" enabled. The m-ext includes div/rem so we're just lucky that we're not getting those in the app/fw. Two options:

  • Enable division in the picorv32 (more LUTs)
  • Use the Zmmul extension instead of m (recent clangs has support): "RISC-V Zmmul Multiply Only enables low-cost implementations that require multiplication operations but not division, and is part of the RISC-V Unprivileged Specification."

Thank you. We currently use clang 14 and it does not have Zmmul, checked with llc -march=riscv32 -mcpu=help |& grep -i zmmul. For catching this early and right now we could do llvm-objdump | grep (div|rem) or "look into the exception handling of illegal instruction in picorv32... maybe it's possible to trap and blink the leds aggresively!"

quite commented 2 years ago

@secworks I tried building with ENABLE_DIV=1, result is this increased device utilisation, from:

 Info:            ICESTORM_LC:  4446/ 5280    84%

to

 Info:            ICESTORM_LC:  5189/ 5280    98%

Maybe this is not great at all, considering that the space might be needed for other things, like a blake2 in hw? Or what else we've had on the table.

quite commented 2 years ago

Maybe we can just avoid DIV/REM being used? If so, then eating up more space in the FPGA seems like really bad idea. In this case, would it be fine for now to check for DIV/REM in the final ELF, and fail if they are present? And once we get onto clang 16 (or what is needed), we can benefit from Zmmul extension.

quite commented 2 years ago

Ah yes, we should try to use a DIV and see what happens, crash or not?

secworks commented 2 years ago

@secworks I tried building with ENABLE_DIV=1, result is this increased device utilisation, from:

Info:            ICESTORM_LC:  4446/ 5280    84%

to

 Info:            ICESTORM_LC:  5189/ 5280    98%

Maybe this is not great at all, considering that the space might be needed for other things, like a blake2 in hw? Or what else we've had on the table.

Interesting, I don't see the change in utilization when I enable DIV (which I expect to get).

To answer the the use of resources - if apps crash when using a div instruction that would make this a high priority, and thus a good use of resources. As long as we get good clock frequency with 98% fill rate I would be happy. I don't see any things more important to add right now.

Blake2s in HW will require more resources that we have as it is before enabling DIV.

secworks commented 2 years ago

Fixed in commit https://github.com/tillitis/tillitis-key1/commit/f09ff87f9e5d5ed9940f1550ae7fae4b6486aabf