[aes] Rework register interface #1053

vogelpi · 2019-11-22T13:00:04Z

This PR contains a series of commits to implement the following two main changes:

Input data and initial key registers can now be cleared using a bit in the trigger register (like the output data register). At the moment, these registers are cleared to 0. At a later point, an LFSR will be inserted to clear those registers with pseudo-random data. Previously those registers had to be cleared by software.
Writes to the key and control registers are ignored if the AES unit is not idle. This similar to the behavior of the HMAC module changed in PR [hmac] Block the configuration change while processing #1014 .

Hand in hand with these changes goes a restructuring/cleanup of the AES core (not the cipher, but the aes_core.sv file connecting the data paths with the registers).

The changes have been successfully tested using my Verilator test framework.

hw/ip/aes/rtl/aes_control.sv

hw/ip/aes/data/aes.hjson

cdgori · 2019-11-22T19:50:54Z

hw/ip/aes/rtl/aes_control.sv

@@ -374,10 +404,11 @@ module aes_control #(

  // Detect new key, new input, output read
  // Edge detectors are cleared by the FSM
-  assign key_init_new_d = dec_key_gen ? '0 : key_init_new_q | key_init_qe_i;
+  assign key_init_clear = (key_init_sel_o == KEY_INIT_CLEAR) & (&key_init_we_o);
+  assign key_init_new_d = dec_key_gen | key_init_clear ? '0 : key_init_new_q | key_init_qe_i;


I personally prefer to parenthesize any compound conditions or assignments, even if the order of operations is correct, just to avoid silly editing/copy-paste errors that could occur later, but I believe this way is allowed by the style guide.

I am suggesting: ... = (dec_key_gen | key_init_clear) ? '0 : (key_init_new_q | key_init_qe_i);

But I am pretty liberal with parentheses, maybe too much so.

I think that is a valid point. I added the parentheses.

hw/ip/aes/rtl/aes_control.sv

cdgori

LGTM with a handful of nits/clarifications. I appreciate how much cleaner it looks with the for loops in some of the sections.

vogelpi · 2019-11-25T20:55:45Z

Thanks @cdgori for your feedback. I implemented your suggestions.

eunchan · 2019-11-25T21:39:13Z

hw/ip/aes/rtl/aes_control.sv

-            data_out_we_o       = 1'b1;
-            data_out_clear_we_o = 1'b1;
-          end
+        end else if (key_clear_i || data_in_clear_i || data_out_clear_i) begin


Is the signal pulse or level?

Nevermind I found the code below (they are level)

It seems like this would work better as a wo where the qe was used to do the clearing, else
SW will need to write first to 1 then to 0, no?

It seems like this would work better as a wo where the qe was used to do the clearing, else
SW will need to write first to 1 then to 0, no?

Right. But the signals shouldn't be pulse as those signal used in other state too. But having it write-only and let hardware clear them if they are processed is good as I mentioned below.

I will make it wo for software, this makes sense.

But to clarify:

Software only needs to write 1 (already now). The hardware will then clear it back to 0.

The hardware should not use the qe signal to trigger the clearing as pointed out by @eunchan. Writes to the TRIGGER register are not ignored when the unit is not idle. If the unit is busy while e.g. software triggers a clear of the input register, this clear would not be triggered after finishing operation when using the qe signal.

eunchan · 2019-11-25T21:43:50Z

hw/ip/aes/rtl/aes_control.sv

+  output logic                    data_in_clear_o,
+  output logic                    data_in_clear_we_o,
  output logic                    data_out_clear_o,


This could trigger the CSR test failure as TRIGGER register has RW for software access. @weicaiyang @sriyerg to give us clear answer.

Thanks @eunchan for your reviewn and the comment. Do you suggest to make TRIGGER write-only for software?

If that works. Yes :)

You can merge as it is. It can be done in following PR after getting the answer from DV engineers,

Alright. I'll merge it now.

Actually, I would need the green light from someone with write permission. Would you mind @eunchan?

sjgitty · 2019-11-27T21:12:35Z

hw/ip/aes/doc/_index.md

-2. Clear the configured initial key by overwriting the Initial Key registers {{< regref "KEY0" >}} - {{< regref "KEY7" >}}.
-3. Clear the previous input data by overwriting the Input Data registers {{< regref "DATA_IN0" >}} - {{< regref "DATA_IN3" >}}.
-4. Clear the internal key registers and the Output Data register by setting the KEY_CLEAR and DATA_OUT_CLEAR bits in {{< regref "TRIGGER" >}} to `1`.
+2. Clear all key registers as well as the Input Data and the Output Data registers by setting the KEY_CLEAR, DATA_IN_CLEAR and DATA_OUT_CLEAR bits in {{< regref "TRIGGER" >}} to `1`.


just a .md pro tip: you can number them all 1. and markdown will keep track of the emnumeration.

Let's get pro then ;-) I changed it here and will keep it in mind for future changes to the doc.

sjgitty · 2019-11-27T21:15:20Z

hw/ip/aes/rtl/aes_control.sv

-            data_out_we_o       = 1'b1;
-            data_out_clear_we_o = 1'b1;
-          end
+        end else if (key_clear_i || data_in_clear_i || data_out_clear_i) begin


It seems like this would work better as a wo where the qe was used to do the clearing, else
SW will need to write first to 1 then to 0, no?

vogelpi · 2019-11-28T09:16:19Z

I added another commit to change the software access permissions of the TRIGGER register from rw to wo as suggested.

The reason I made the trigger register rw for software in the first place was to allow software to check if a requested clear has been performed. Let me know if you think this is necessary or not. I can still remove the commit.

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

This commit modifies the input data and initial key registers as well as the control FSM to allow the hardware to clear those registers based on the content of the trigger register. Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

Software does not need to be able to read back the content of the trigger register. This commit removes read access for software. Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

vogelpi · 2019-12-02T17:02:06Z

Thanks @eunchan for approving. I am merging this now.

vogelpi requested review from tjaychen, eunchan, cdgori and sjgitty November 22, 2019 13:00

This was referenced Nov 22, 2019

[top] Add AES unit #576

Merged

[aes] Tracking requested changes and work items #1005

Closed

[WIP] [aes/dv] Verilator testbench #1054

Closed

cdgori reviewed Nov 22, 2019

View reviewed changes

hw/ip/aes/rtl/aes_control.sv Outdated Show resolved Hide resolved

cdgori reviewed Nov 22, 2019

View reviewed changes

hw/ip/aes/data/aes.hjson Outdated Show resolved Hide resolved

cdgori reviewed Nov 22, 2019

View reviewed changes

hw/ip/aes/data/aes.hjson Outdated Show resolved Hide resolved

cdgori reviewed Nov 22, 2019

View reviewed changes

hw/ip/aes/rtl/aes_control.sv Outdated Show resolved Hide resolved

cdgori approved these changes Nov 22, 2019

View reviewed changes

vogelpi force-pushed the aes-rework-registers branch from d34597c to 2dab587 Compare November 25, 2019 20:54

eunchan reviewed Nov 25, 2019

View reviewed changes

vogelpi force-pushed the aes-rework-registers branch from 2dab587 to 82ea5a0 Compare November 26, 2019 07:58

sjgitty reviewed Nov 27, 2019

View reviewed changes

vogelpi force-pushed the aes-rework-registers branch from 82ea5a0 to 3db2f8e Compare November 28, 2019 08:54

vogelpi added 7 commits November 28, 2019 16:27

[aes/rtl] Add headers, restructure AES core

c9c0e8d

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

[aes/rtl] Update register tool

25aedc0

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

[aes/rtl] Beautify input/output handling

ba1cc27

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

[aes] Ignore writes to key register when not idle

5837ec9

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

[aes] Ignore writes to control register when not idle

bcfb213

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

[aes] Make the trigger register write-only for software

16b082c

Software does not need to be able to read back the content of the trigger register. This commit removes read access for software. Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

vogelpi force-pushed the aes-rework-registers branch from 8b48540 to 16b082c Compare November 28, 2019 15:41

eunchan approved these changes Dec 2, 2019

View reviewed changes

vogelpi merged commit 76463db into lowRISC:master Dec 2, 2019

vogelpi deleted the aes-rework-registers branch December 3, 2019 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[aes] Rework register interface #1053

[aes] Rework register interface #1053

vogelpi commented Nov 22, 2019

cdgori Nov 22, 2019

vogelpi Nov 25, 2019

cdgori left a comment

vogelpi commented Nov 25, 2019

eunchan Nov 25, 2019

eunchan Nov 25, 2019

sjgitty Nov 27, 2019

eunchan Nov 27, 2019

vogelpi Nov 28, 2019

eunchan Nov 25, 2019

vogelpi Nov 27, 2019

eunchan Nov 27, 2019

eunchan Nov 27, 2019

vogelpi Nov 27, 2019

vogelpi Nov 27, 2019

sjgitty Nov 27, 2019

vogelpi Nov 28, 2019

sjgitty Nov 27, 2019

vogelpi commented Nov 28, 2019

vogelpi commented Dec 2, 2019

[aes] Rework register interface #1053

[aes] Rework register interface #1053

Conversation

vogelpi commented Nov 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdgori left a comment

Choose a reason for hiding this comment

vogelpi commented Nov 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogelpi commented Nov 28, 2019

vogelpi commented Dec 2, 2019