[top] Add AES unit #576
[top] Add AES unit #576
Conversation
vogelpi
requested review from
msfschaffner,
sriyerg and
weicaiyang
as code owners
|
Also pinging @rasmus-madsen and @martin-lueker to keep them in the loop. |
|
Regarding DV: Basic functionality has been verified using a Verilator-based framework that is not in the tree currently. The real DV is work in progress by Rasmus. |
Yes I am working on this, the current status is that I have been debugging a register issue that was rootcaused to a problem in the one of the autogen scripts. Weicai has solved the issue and I am in the process of rebasing my branch to get the updates. next issue is there is no active support for multiregs as I understand it. but I will see if I can't get it to work. |
There was a problem hiding this comment.
LGTM, but I think @eunchan should give it a quick look as well.
Did you run a top-level simulation or FPGA build to check whether it impacts the existing functionality? |
|
I ran FPGA synthesis to evaluate the cost of enabling support for AES-192 (currently around 25%) but I did not run simulations or the like. I guess CI will do both, right? |
|
Since you're adding to the top, it might be worth adding a sanity C test
under sw/tests. It can be a quick check that nothing is majorly broken at
the top.
We can also add that to the verilator based CI, which runs everything under
sw/tests.
…On Wed, Oct 23, 2019, 08:32 Pirmin Vogel ***@***.***> wrote:
I ran FPGA synthesis to evaluate the cost of enabling support for AES-192
(currently around 25%) but I did not run simulations or the like. I guess
CI will do both, right?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#576?email_source=notifications&email_token=AAH2RSUC6YUAJAWITFHGVZTQQBU7XA5CNFSM4JD6CGHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECB3GXQ#issuecomment-545502046>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAH2RSQC7OXPWB4WVEMBXQ3QQBU7XANCNFSM4JD6CGHA>
.
|
|
The OpenOCD test suite runs through as usual. @tjaychen That's a good idea. I will implement and add a simple sanity test for the AES module. |
There was a problem hiding this comment.
LGTM Thanks!
I think @weicaiyang should be aware of this change as he is focusing on TL-UL crossbar DV.
|
I have created a driver for the AES unit and a test application. It works. What is missing is the intermediate layer, i.e., the actual HW crypto API that calls the AES driver/DIF underneath (similar to hw_hmac). Anyway, this does not belong into this PR here. I will create another one for the software, possibly a draft one since there is more urgent stuff around at the moment than implementing the crypto API for AES. Bottom line: I would be happy to get this approved and merged, before it's getting stale. |
|
With the reviews you have my impression is it's good to go in - perhaps with a final ok from @sjgitty? Or is there a particular review you're waiting on? |
|
Thanks for approving @msfschaffner ! I wasn't waiting for a particular review, but wanted to first write a basic test that makes the core use the module. This test is now in the following draft PR: #627. Given the high load of most people at the moment, I will no longer wait and merge now. |
This commit changes the `clock_primary` field to `clk_i` and introduces two parameters to define the `count` field of the multiregs.
This input port is usually not exposed to the IP interface but statically driven inside the top-level IP wrapper.
This commit adds an assertion to check that outputs always have a known value after reset. This is related to #405.
|
Sorry, I just saw this thread and it answers all of the questions/comments
I made attached to #627. This is much clearer to me now.
(However, one of my questions does sort of still stand, do we need a
similar sw / verilator test for HMAC or is that superfluous now that there
is more DV infrastructure around HMAC? Would we need to do the same for
RSA/elliptic curve for any HW-accelerator that we might implement?
Basically trying to understand the usage of this versus the full DV suites.)
…On Fri, Oct 25, 2019 at 2:11 AM Pirmin Vogel ***@***.***> wrote:
Thanks for approving @msfschaffner <https://github.com/msfschaffner> !
I wasn't waiting for a particular review, but wanted to first write a
basic test that makes the core use the module. This test is now in the
following draft PR: #627 <#627>.
Given the high load of most people at the moment, I will no longer wait
and merge now.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#576?email_source=notifications&email_token=AJBZS7ENZBML2H7YVZICD5LQQKZ2LA5CNFSM4JD6CGHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECHXKOY#issuecomment-546272571>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJBZS7ECY24JPV4R2ZWVWTDQQKZ2LANCNFSM4JD6CGHA>
.
|
|
Hi @cdgori , In the end, I think it would be useful to have such an example for every accelerator as it can also show how to actually use the module. However, if we want to provide examples, we might want to discuss first how software should interface the libraries and how to handle keys etc. Let's continue this discussion in in #627 or a separate issue. |
|
Great, this is 100% clear to me then. Sorry that I missed the similar
example for HMAC. We can continue in #627 if needed.
…On Wed, Oct 30, 2019 at 2:20 AM Pirmin Vogel ***@***.***> wrote:
Hi @cdgori <https://github.com/cdgori> ,
there is already a similar example for HMAC to what I am doing in #627
<#627>. This is independent of
the actual DV. The idea is to have a quick sanity test to make sure the
module is properly integrated into the top and accessible to software.
In the end, I think it would be useful to have such an example for every
accelerator as it can also show how to actually use the module. However, if
we want to provide examples, we might want to discuss first how software
should interface the libraries and how to handle keys etc. Let's continue
this discussion in in #627 <#627>
or a separate issue.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#576?email_source=notifications&email_token=AJBZS7CIBAWPR4UWSY3L45LQRFGUTA5CNFSM4JD6CGHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECTOM2Q#issuecomment-547808874>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AJBZS7G2EC5UII2EOOR3ZC3QRFGUTANCNFSM4JD6CGHA>
.
|
| @@ -19,7 +32,7 @@ | |||
| starting encryption/decryption of the next block. Can only be updated | |||
| when the AES unit is idle. | |||
There was a problem hiding this comment.
Can you mention what happens if the engine is not idle?
"writes to the register while the engine is active are <dropped? cause error?>"
There was a problem hiding this comment.
Currently, it is just not safe to do. The current block will use the old value and next block will use the new/intermediate value if the key is not fully configured. I would like to change this according to what has been done for HMAC in #1014, i.e., ignore key updates while the unit is busy.
There was a problem hiding this comment.
The corresponding change is in PR #1053.
This PR adds the AES unit to the top-level of earlgrey.