[aes] Add shadow CTRL register, interface with alert handler #2152

vogelpi · 2020-05-07T14:45:12Z

This PR adds a shadow CTRL register according to @tjaychen's Shadow Register RFC discussed in #150.

More specifically, two more copies are added for the CTRL register: a shadow register and a staged register. An update to the CTRL register now always takes two subsequent writes. The first write goes into the staged register and the second potentially to the actual and the shadow register.

If the content of the staged register and the value written to the actual register do not match, this causes an update error and triggers an alert. The register is not updated.
If the contents of actual and shadow registers differ, this causes a storage error and triggers an alert.
Invalid mode field values are resolved to the new reset value AES_NONE = 4'b1000.
The cipher core is not allowed to start or finish unless the mode field is set to a valid value.

~~What is missing is the adaption of the UVM-based DV infrastructure.~~ The changes have been successfully tested using AES DV, my own Verilator framework and the aes_test.

tjaychen · 2020-05-08T04:52:24Z

hw/ip/aes/rtl/aes_core.sv

+  //////////////////////
+  // Control Register //
+  //////////////////////
+


this looks great! although..do you want to turn this into a prim so others can make use of it also?

I actually wanted to get this in quickly to have it ready for bronze. But due to the high impact on DV it has to wait. So I can indeed work out a prim now.

sounds good. Is the high DV impact because the sequences all have to change for a double write..?

No that is the simple part. A lot of work lies in adding another interface (the alert handler), feeding back alerts into the sequences, dealing with it in the scoreboard and things like that. Since Rasmus has a ton of changes almost ready, it is more efficient to first get his changes in.

@vogelpi the fact that it "wants to go quickly" means it probably should just wait for bronze modifications. :) would love to see this as a primitive fully integrated into reggen with auto-reg testing included (@sriyerg @cindychip )

I have factored out the shadow register primitive into a separate PR here: #2293
This PR is now using the primitive.

tjaychen · 2020-05-08T04:53:17Z

@weicaiyang

rasmus-madsen · 2020-05-08T09:37:26Z

I rather not review the code specifically - to avoid having a deeper understanding of how it works.
so I avoid aliasing the RTL implementation into my dv effort.
I will probably look into it after dv moves to V2. to see if I can spot some vulnerabilities.

but I do have a clearification question.
Do AES dv need to verify the new shadowreg and stage reg or can I assume them to be "pre verified"

but I have questions

vogelpi · 2020-05-08T10:46:27Z

I just talked to @rasmus-madsen and must admit that I underestimated the implications of this on DV. For this reason, I have to split this up. I will add the altert handler interface on v1-bronze directly as this needs to go in now.

@rasmus-madsen will soon push a major DV update. I will first await that one before pushing this PR for master any further. This gives me the opportunity to create a prim for the shadow register as suggested by @tjaychen .

Regarding DV: Modifying the sequences to properly handle the shadow register is low effort. But adding the alert handler interface has a high impact on DV. There will be a lot of work required to get the interface integrated into the environment properly. Finally, adding any new interface will probably set us back to V0. This we might need to discuss at some point @weicaiyang , @sriyerg , @sjgitty .

cdgori · 2020-05-18T18:00:30Z

hw/ip/aes/rtl/aes_core.sv

+      AES_128: ctrl_d.key_len = AES_128;
+      AES_256: ctrl_d.key_len = AES_256;
+      AES_192: ctrl_d.key_len = AES192Enable ? AES_192 : AES_128;
+      default: ctrl_d.key_len = AES_128; // unsupported values are mapped to AES_128


Thinking about this a bit more: hould the default/unsupported mapping be AES_128 or AES_256?

I believe that it could be a tiny bit safer to have AES_256 (for this one and the second-half of the AES192 case above), since there is then less chance of using a key with reduced rounds (?). This is a pretty minor thing though, and I'm not sure if it actually affects anything.

For sure it does not hurt to default to AES_256. Thus, I made the switch. Thanks for the suggestion.

vogelpi · 2020-05-21T14:46:42Z

Update: implemented @cdgori comment's and factored out the shadow register into a new primitive here #2293. I suggest we discuss this new primitive over there.

weicaiyang · 2020-05-21T18:23:37Z

but I do have a clearification question.
Do AES dv need to verify the new shadowreg and stage reg or can I assume them to be "pre verified"

we should have a common CSR seq or add the support in csr_rw seq to test shadow registers.

weicaiyang · 2020-05-21T18:30:20Z

Regarding DV: Modifying the sequences to properly handle the shadow register is low effort. But adding the alert handler interface has a high impact on DV. There will be a lot of work required to get the interface integrated into the environment properly. Finally, adding any new interface will probably set us back to V0. This we might need to discuss at some point @weicaiyang , @sriyerg , @sjgitty .

perhaps we can keep it in V1 as long as we keep sanity passing and update testplan accordingly.

sjgitty · 2020-05-22T01:00:55Z

... adding any new interface will probably set us back to V0. This we might need to discuss at some point @weicaiyang , @sriyerg , @sjgitty .

Those alerts are coming, sooner or later, so we'll have to "face the effects" regardless. I don't think it means going back to V0 if we keep the sanity going like @weicaiyang suggested. But I recognize that it will set us back some time.

I thought that it was known alerts were eventually coming, which makes me wonder if there are other surprises we should make sure are discussed. For instance the eventual port to the key manager for side loading.

msfschaffner

This looks good @vogelpi, thanks! I just have a few minor comments below.

msfschaffner · 2020-06-06T01:20:08Z

hw/ip/aes/data/aes.hjson

        hwaccess: "hrw",
        desc:  '''
-          3-bit one-hot field to select AES block cipher mode: ECB (3'b001), CBC (3'b010) or CTR (3'b100).
-          Invalid input values, i.e., value with multiple bits set, and value 3'b000 are not supported are mapped to 3'b001.
+          4-bit one-hot field to select AES block cipher mode: ECB (4'b0001), CBC (4'b0010), CTR (4'b0100) or NONE (4'b1000).


you may want to consider making these multibit fields for mode selection enum types. the reggen tool creates a nice table with a row for each value. see, e.g.

opentitan/hw/ip/pinmux/data/pinmux.hjson

Lines 210 to 226 in 5c323ef

enum: [

{ value: "0",

name: "Tie-Low",

desc: "The pin is driven actively to zero in deep sleep mode."

},

{ value: "1",

name: "Tie-High",

desc: "The pin is driven actively to one in deep sleep mode."

},

{ value: "2",

name: "High-Z",

desc: '''

The pin is left undriven in deep sleep mode. Note that the actual

driving behavior during deep sleep will then depend on the pull-up/-down

configuration of padctrl.

'''

},

Good points, I switched to enums for both mode and key length fields.

msfschaffner · 2020-06-06T01:22:57Z

hw/ip/aes/rtl/aes_pkg.sv

@@ -6,15 +6,19 @@

 package aes_pkg;

+parameter int NumAlerts = 1;
+parameter logic [NumAlerts-1:0] AlertAsyncOn = NumAlerts'(1'b1);


I think for integration purposes, this parameter should be a module parameter such that topgen can easily set it when instantiating the module in top_earlgrey. this is one of the aspects that is not fully automated in topgen yet.

Thanks @msfschaffner for your review. Should we change this in a follow-up PR? I think HMAC does currently the same...

Sure, just leave it be for now. It's probably better to do this as part of a topgen update...

btw, since you guys brought this up, what do we like as the general approach?

Do we want to pass parameters as part of instantiation, or do we prefer to have a top_pkg.sv of sorts, which is templatized, and that all the ips can reference? I guess this doesn't work at all when there are multiple instances of the same module...but it's for example what i'm doing in flash.

Is it worth making this more uniform in your opinion?

Sorry I am pretty late to the party, I wonder if there is any conclusion or follow up about this discussion?
From the DV automation point of view, I think it would be useful to specify "which alert the shadow_reg error is connected to" in the register hjson file (if possible). In DV csr test, we are trying to automatically generate update_err and storage error, then check if the corresponding alert is triggered. It would be very helpful if we can know which alert reflects the shadow_reg errors.

Thanks for the reminder @cindychip. I forgot a bit about this but now took a note in the AES tracking issue. I had in mind to add a second alert. One would be minor for update errors and other stuff. One would be major and require a reset (storage error). This would also require a small change to make every storage error fatal. Right now, they are only fatal if the module is busy (ctrl register not writable, cipher core will never finish with invalid config). Does that sound reasonable to you?

hw/ip/prim/rtl/prim_subreg_shadow.sv

vogelpi · 2020-06-12T13:14:15Z

Update: this PR has been rebased to include the latest improvements to the AES DV framework and also @cindychip 's work for shadow regs (#2377) .

tjaychen · 2020-06-13T01:23:17Z

hw/top_earlgrey/ip/alert_handler/data/autogen/alert_handler.hjson

@@ -53,7 +53,7 @@
    { name: "AsyncOn",
      desc: "Number of peripheral outputs",


is the description here correct?

rasmus-madsen

hi @vogelpi LGTM
thank you for updating the Dv also - I added a few nit comment.
one of which I will just update on my side.
by choice i skipped the aes RTL files so I did not review those

rasmus-madsen · 2020-06-17T10:47:41Z

hw/dv/sv/dv_base_reg/dv_base_reg.sv

+
+  function void set_en_shadow_wr(bit val);
+    if (en_shadow_wr == 0 && val && shadow_wr_staged) begin
+      `uvm_info(`gfn, "unable to set en_shadow_wr due to register already complete first write",


nit: missing d - completed

rasmus-madsen · 2020-06-22T09:05:12Z

hw/ip/aes/doc/_index.md

@@ -74,7 +74,7 @@ The AES unit automatically starts the encryption/decryption of the next data blo
 The order in which the input registers are written does not matter.
 Every input register must be written at least once for the AES unit to automatically start encryption/decryption.


Do you mean input data register?
if not, it this also true when running ECB mode - it does not make sense if the user have to write the IV registers for ECB

Yes, this section is about the input data registers.

rasmus-madsen · 2020-06-22T09:45:37Z

hw/ip/aes/dv/env/aes_scoreboard.sv

+            4'b0010:  input_item.mode = AES_CBC;
+            4'b0100:  input_item.mode = AES_CTR;
+            4'b1000:  input_item.mode = AES_NONE;
+            default:  input_item.mode = AES_ECB;


Should we make this none to match the RTL?
I can do that.

If this is an invalid mode, is that actually allowed by the module spec? If not, surely the test code should just throw an error?

yes this mode is allowed by the RTL
so the dv should also go into this state.
the question here is since the RTL goes into this as a default state - the DV should also.

Yep, AES_NONE is the reset value in the RTL and every invalid setting is mapped to AES_NONE. I wasn't fully sure how to correctly adapt DV for this. If you could change that @rasmus-madsen this would be great. This should possibly be a follow-up PR?

yep let's do that in a follow up PR

vogelpi · 2020-06-29T16:34:40Z

Update: rebased on master to get latest improvements to AES DV from #2665.

vogelpi · 2020-07-20T08:45:29Z

Update: rebased on master, removed draft status.

@cindychip 's DV PR #2377 will hopefully get merged soon. This is ready for review. I already got approval from DV. What is now needed is:

Approval from security
Approval from software
Approval from design

lenary

LGTM from the software side.

These changes will need to be replicated into the AES DIF in #2413 (but this doesn't block the current PR).

lenary · 2020-07-20T09:07:41Z

sw/device/lib/aes.c

@@ -101,7 +105,7 @@ void aes_clear(void) {
  }

  // Disable autostart
-  REG32(AES_CTRL(0)) = 0x1u << AES_CTRL_MANUAL_OPERATION;
+  REG32(AES_CTRL_SHADOWED(0)) = 0x1u << AES_CTRL_SHADOWED_MANUAL_OPERATION;


This write needs duplicating, surely?

Oh thanks, good catch Sam! I just fixed it.

lenary · 2020-07-20T09:07:47Z

sw/device/lib/aes.c

+  REG32(AES_CTRL_SHADOWED(0)) = cfg_val;
+  REG32(AES_CTRL_SHADOWED(0)) = cfg_val;


lenary · 2020-07-20T09:08:14Z

hw/ip/aes/doc/_index.md

+  REG32(AES_CTRL_SHADOWED(0)) = aes_ctrl_val;
+  REG32(AES_CTRL_SHADOWED(0)) = aes_ctrl_val;


lenary · 2020-07-20T09:09:27Z

sw/device/lib/aes.h

+ * encoding. NONE is not a supported mode but the reset value of the hardware.
+ * The hardware resolves invalid mode values to NONE.


This is useful.

rasmus-madsen

LGTM

cdgori · 2020-07-20T21:19:46Z

hw/ip/aes/data/aes.hjson

-          disabled at compile time) are mapped to 3'b001.
+        desc: '''
+          3-bit one-hot field to select AES key length.
+          Invalid input values, i.e., values with multiple bits set, value 3'b000, and value 3'b010 in case 192-bit keys are not supported (because disabled at compile time) are mapped to AES_256 (3'b100).


Thanks for your review @cdgori . I switched to 1 sentence per line on purpose here. Actually I do this switch for every block that I change.

cdgori

LGTM!

@tjaychen

This commit adds a shadow CTRL register according to @tjaychen's Shadow Register RFC: - If the content of the staged register and the value written to the actual register do not match, this causes an update error and triggers an alert. The register is not updated. - If the contents of actual and shadow registers differ, this causes a storage error and triggers an alert. - Invalid mode field values are resolved to the new reset value AES_NONE = 4'b1000. - The cipher core is not allowed to start or finish unless the mode field is set to a valid value. Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>

vogelpi requested review from tjaychen, eunchan, cdgori, martin-lueker, rasmus-madsen and msfschaffner May 7, 2020 14:45

tjaychen reviewed May 8, 2020

View reviewed changes

vogelpi mentioned this pull request May 8, 2020

[aes] Add intermodule connections and estimated gates for hardening #2140

Merged

cdgori reviewed May 18, 2020

View reviewed changes

vogelpi force-pushed the aes-add-shadow-ctrl branch 3 times, most recently from 09f1e15 to 38d8c0a Compare May 21, 2020 14:42

vogelpi force-pushed the aes-add-shadow-ctrl branch 2 times, most recently from 20fc176 to 9014aac Compare May 21, 2020 17:08

vogelpi force-pushed the aes-add-shadow-ctrl branch from 9014aac to db1bc25 Compare May 22, 2020 09:50

msfschaffner reviewed Jun 6, 2020

View reviewed changes

vogelpi force-pushed the aes-add-shadow-ctrl branch from db1bc25 to 5603c8a Compare June 12, 2020 12:13

vogelpi mentioned this pull request Jun 12, 2020

[dv/common] initial support for shadow register #2377

Merged

vogelpi force-pushed the aes-add-shadow-ctrl branch from 5603c8a to 2dd4848 Compare June 12, 2020 13:09

tjaychen reviewed Jun 13, 2020

View reviewed changes

rasmus-madsen approved these changes Jun 22, 2020

View reviewed changes

vogelpi force-pushed the aes-add-shadow-ctrl branch from 2dd4848 to e94a4a0 Compare June 26, 2020 13:02

vogelpi force-pushed the aes-add-shadow-ctrl branch from e94a4a0 to 0882c89 Compare June 29, 2020 16:32

cindychip mentioned this pull request Jul 7, 2020

[prim] Add shadow register primitive #2293

Merged

vogelpi force-pushed the aes-add-shadow-ctrl branch from 0882c89 to 090fd49 Compare July 16, 2020 06:40

vogelpi marked this pull request as ready for review July 20, 2020 08:36

vogelpi requested review from gkelly, lenary, mcy, moidx, sriyerg and weicaiyang as code owners July 20, 2020 08:36

vogelpi force-pushed the aes-add-shadow-ctrl branch from 090fd49 to 2736707 Compare July 20, 2020 08:42

vogelpi requested review from tjaychen, cdgori, msfschaffner and silvestrst July 20, 2020 08:45

lenary approved these changes Jul 20, 2020

View reviewed changes

vogelpi force-pushed the aes-add-shadow-ctrl branch from 2736707 to 88f57a8 Compare July 20, 2020 09:19

rasmus-madsen approved these changes Jul 20, 2020

View reviewed changes

cdgori reviewed Jul 20, 2020

View reviewed changes

cdgori approved these changes Jul 20, 2020

View reviewed changes

vogelpi mentioned this pull request Jul 21, 2020

[aes:rtl] AES produces wrong result when data is interleaved with key #2913

Closed

vogelpi force-pushed the aes-add-shadow-ctrl branch from 88f57a8 to f961347 Compare July 22, 2020 06:34

vogelpi force-pushed the aes-add-shadow-ctrl branch from f961347 to 2546b2b Compare July 22, 2020 08:05

vogelpi merged commit be4bcb7 into lowRISC:master Jul 22, 2020

vogelpi deleted the aes-add-shadow-ctrl branch July 22, 2020 09:14

vogelpi mentioned this pull request Jul 28, 2020

[aes] Tracking requested changes and work items #1005

Closed

35 tasks

	enum: [
	{ value: "0",
	name: "Tie-Low",
	desc: "The pin is driven actively to zero in deep sleep mode."
	},
	{ value: "1",
	name: "Tie-High",
	desc: "The pin is driven actively to one in deep sleep mode."
	},
	{ value: "2",
	name: "High-Z",
	desc: '''
	The pin is left undriven in deep sleep mode. Note that the actual
	driving behavior during deep sleep will then depend on the pull-up/-down
	configuration of padctrl.
	'''
	},

		@@ -53,7 +53,7 @@
		{ name: "AsyncOn",
		desc: "Number of peripheral outputs",

		@@ -74,7 +74,7 @@ The AES unit automatically starts the encryption/decryption of the next data blo
		The order in which the input registers are written does not matter.
		Every input register must be written at least once for the AES unit to automatically start encryption/decryption.

		REG32(AES_CTRL_SHADOWED(0)) = cfg_val;
		REG32(AES_CTRL_SHADOWED(0)) = cfg_val;

		REG32(AES_CTRL_SHADOWED(0)) = aes_ctrl_val;
		REG32(AES_CTRL_SHADOWED(0)) = aes_ctrl_val;

		* encoding. NONE is not a supported mode but the reset value of the hardware.
		* The hardware resolves invalid mode values to NONE.

[aes] Add shadow CTRL register, interface with alert handler #2152

[aes] Add shadow CTRL register, interface with alert handler #2152

Conversation

vogelpi commented May 7, 2020 • edited Loading

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

tjaychen commented May 8, 2020

rasmus-madsen commented May 8, 2020

vogelpi commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vogelpi commented May 21, 2020

weicaiyang commented May 21, 2020

weicaiyang commented May 21, 2020

sjgitty commented May 22, 2020

msfschaffner left a 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 Jun 12, 2020

Choose a reason for hiding this comment

rasmus-madsen left a 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 Jun 29, 2020

vogelpi commented Jul 20, 2020

lenary left a 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

rasmus-madsen left a comment

Choose a reason for hiding this comment

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 May 7, 2020 •

edited

Loading