Hmac tests #25825

rprakas-gsc · 2025-01-09T06:12:24Z

Added Hmac error conditions test

azure-pipelines · 2025-01-09T19:12:19Z

There was an error handling pipeline event 46043aee-f25d-4a9d-a7e1-0d28141af50f.

rprakas-gsc · 2025-01-14T18:09:45Z

@moidx for visibility

Signed-off-by: Ramesh Prakash <[email protected]>

This reverts commit 607e841. Signed-off-by: Ramesh Prakash <[email protected]>

nasahlpa

Thanks for your work @rprakas-gsc! I've dropped a couple of suggestions.

nasahlpa · 2025-01-23T06:46:50Z

hw/top_earlgrey/data/ip/chip_aes_testplan.hjson

      bazel: ["//sw/device/tests:aes_force_prng_reseed_test"]
    }
+    {


Would it be possible to split this work into two PRs? As far as I understand, the changes in the chip_aes_testplan.hjson in this commit and also in the other commit don't really belong here?

nasahlpa · 2025-01-23T06:47:37Z

sw/device/tests/BUILD

-           "//hw/top_earlgrey:sim_verilator": None,
-           "//hw/top_earlgrey:fpga_cw340_sival": None,
-           "//hw/top_earlgrey:silicon_creator": None,
+            "//hw/top_earlgrey:sim_verilator": None,


I would suggest moving these changes into the same commit as the first commit in this PR?

nasahlpa · 2025-01-23T06:47:53Z

sw/device/tests/hmac_error_conditions_test.c

@@ -58,7 +58,6 @@ static const char kData[142] =
    "one of the few honest people that I have ever "
    "known";

-
 static uint32_t kHmacKey[8] = {


This also applies to this change.

nasahlpa · 2025-01-23T06:53:14Z

hw/top_earlgrey/data/ip/chip_aes_testplan.hjson

@@ -211,7 +211,7 @@
      stage: V2
      si_stage: SV3
      lc_states: ["PROD"]
-      tests: []
+      tests: ["chip_sw_aes_prng_reseed"]


Similar to the other comment, I think it would be better having a separate PR for this. Moreover, it would be good to have a brief explanation why this is needed.

nasahlpa · 2025-01-23T06:58:35Z

sw/device/tests/hmac_error_conditions_test.c

@@ -0,0 +1,361 @@
+// Copyright lowRISC contributors (OpenTitan project).


Could you please add a subject tag to the commit message header?

nasahlpa · 2025-01-23T07:13:07Z

sw/device/tests/hmac_error_conditions_test.c

+}
+static void run_hmac_clear_interrupt(void) {
+  uint32_t intr_state_reg_val_reset = 0x0007;
+  abs_mmio_write32(kHmacBaseAddr + HMAC_INTR_STATE_REG_OFFSET,


@engdoreis is there a suggestion when to use abs_mmio_write32() vs mmio_region_write32()?

nasahlpa · 2025-01-23T07:20:18Z

sw/device/tests/hmac_error_conditions_test.c

+  uint32_t hmacerr_reg =
+      mmio_region_read32(hmac->base_addr, HMAC_ERROR_REG_OFFSET);
+  LOG_INFO("REGISTER HMAC_ERROR_CODE: 0x%x  Expected:0x%x", hmacerr_reg,
+           error_code_expected);


A print (LOG_INFO()) is not sufficient to check whether the test passed or failed. Please add something like:

TRY_CHECK(hmacerr_reg == error_code_expected, "Unexpected error was triggered.");

This applies throughout the entire code.

nasahlpa · 2025-01-23T07:30:40Z

sw/device/tests/hmac_error_conditions_test.c

+static void run_test_pushmsg_when_shadisabled(
+    const dif_hmac_t *hmac, const char *data, size_t len, const uint8_t *key,
+    const dif_hmac_digest_t *expected_digest) {
+  uint32_t error_code_expected = 0x05;


I am a bit suprised that the HMAC DIF driver does not provide error codes. For example, KMAC provides this structure in the DIF:

opentitan/sw/device/lib/dif/dif_kmac.h

Lines 365 to 410 in 276caf8

typedef enum dif_kmac_error {

/**

* No error has occured.

*/

kDifErrorNone = 0,

/**

* The Key Manager has raised an error because the secret key is not valid.

*/

kDifErrorKeyNotValid = 1,

/**

* An attempt was made to write data into the message FIFO but the KMAC unit

* was not in the correct state to receive the data.

*/

kDifErrorSoftwarePushedMessageFifo = 2,

/**

* SW issued a command while a HW application interface was using KMAC.

*/

kDifErrorSoftwareIssuedCommandWhileAppInterfaceActive = 3,

/**

* The entropy wait timer has expired.

*/

kDifErrorEntropyWaitTimerExpired = 4,

/**

* Incorrect entropy mode when entropy is ready.

*/

kDifErrorEntropyModeIncorrect = 5,

kDifErrorUnexpectedModeStrength = 6,

kDifErrorIncorrectFunctionName = 7,

kDifErrorSoftwareCommandSequence = 8,

kDifErrorSoftwareHashingWithoutEntropyReady = 9,

kDifErrorFatalError = 0xC1,

kDifErrorPackerIntegrity = 0xC2,

kDifErrorMsgFifoIntegrity = 0xC3,

} dif_kmac_error_t;

As I think adding this to the DIF is out of scope for this PR, maybe you can add a typedef for the different error codes you are expecting at the beginning of this test? Then it would be clearer what exactly error code 5 means.

nasahlpa · 2025-01-23T07:58:41Z

hw/top_earlgrey/data/ip/chip_hmac_testplan.hjson

@@ -198,7 +198,7 @@
      si_stage: SV3
      lc_states: ["PROD"]
      tests: []
-      bazel: []
+      bazel: ["//sw/device/tests:hmac_error_conditions_test"]


Thanks for adding tests that are not mentioned in the testplan (e.g., run_test_invalidconfig_keylength). Could you please update the testplan to document the additional tests?

nasahlpa · 2025-01-23T08:01:20Z

sw/device/tests/hmac_error_conditions_test.c

+  for (int error_condition = 1; error_condition < 9; error_condition++) {
+    switch (error_condition) {


A for + switch construct seems a bit unnecessary in my opinion.

rprakas-gsc · 2025-01-28T18:30:54Z

@nasahlpa - Thanks for the suggestions. The opentitan master test environment is broken. I will be incorporating these feedback -and checking in this code and filing PR in earlgrey_1.0.0 branch .
The new PR is listed here #26049

nasahlpa · 2025-01-29T06:23:29Z

@rprakas-gsc why is the test environment broken on master? I think you just need to restart the CI or rebase your work.

I don't quite understand why you've opened a second, identical PR (#26049)? I'd prefer merging this one as here the review history remains preserved and it is easier for me to do a second review round.

rprakas-gsc · 2025-01-29T07:00:50Z

I tried rebasing already on the master branch, it didn't help. My issue is more related to detecting the FPGA board. I also wasted quite a bit of time going down this route - hence I started an identical PR on the earlgrey_1.0.0 branch, with this i can test this code on silicon as well.

nasahlpa · 2025-01-29T07:06:06Z

Thanks for your response. Not sure if I understand, but your test executes just fine here in CI:
//sw/device/tests:hmac_error_conditions_test_fpga_cw340_sival PASSED in 24.2s

rprakas-gsc · 2025-01-29T21:25:30Z

the tests which was executed in fpga - happened before I rebased. To fix the merge issues, I rebased it - thats when the FPGA board detection issue started showing up.

rprakas-gsc · 2025-02-07T01:11:09Z

Closing this PR - since this PR has been approved in earlgrey_1.0.0 branch and backported to master

rprakas-gsc requested a review from a team as a code owner January 9, 2025 06:12

rprakas-gsc requested review from alees24 and removed request for a team January 9, 2025 06:12

rprakas-gsc force-pushed the hmac_tests branch 2 times, most recently from 2c4ccd0 to 1b14ede Compare January 9, 2025 19:12

rprakas-gsc force-pushed the hmac_tests branch 3 times, most recently from c108c17 to 4cb7bc3 Compare January 11, 2025 05:36

moidx self-requested a review January 15, 2025 00:14

rprakas-gsc added 2 commits January 14, 2025 21:42

Add Hmac_error_conditions test

1aa8d9d

Signed-off-by: Ramesh Prakash <[email protected]>

Updated bazel target for aes_interrupt

13595e0

Signed-off-by: Ramesh Prakash <[email protected]>

rprakas-gsc force-pushed the hmac_tests branch from 4cb7bc3 to f6b510e Compare January 15, 2025 05:44

Revert "Added bazel target for aes_interrupt_encr"

d066def

This reverts commit 607e841. Signed-off-by: Ramesh Prakash <[email protected]>

rprakas-gsc force-pushed the hmac_tests branch from f6b510e to d066def Compare January 15, 2025 06:17

engdoreis requested review from nasahlpa and andrea-caforio January 22, 2025 17:29

nasahlpa reviewed Jan 23, 2025

View reviewed changes

rprakas-gsc closed this Feb 7, 2025

rprakas-gsc deleted the hmac_tests branch February 7, 2025 01:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hmac tests #25825

Hmac tests #25825

rprakas-gsc commented Jan 9, 2025

azure-pipelines bot commented Jan 9, 2025

rprakas-gsc commented Jan 14, 2025

nasahlpa left a comment

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

nasahlpa Jan 23, 2025

rprakas-gsc commented Jan 28, 2025 •

edited

Loading

nasahlpa commented Jan 29, 2025

rprakas-gsc commented Jan 29, 2025 •

edited

Loading

nasahlpa commented Jan 29, 2025

rprakas-gsc commented Jan 29, 2025

rprakas-gsc commented Feb 7, 2025

		@@ -0,0 +1,361 @@
		// Copyright lowRISC contributors (OpenTitan project).

	typedef enum dif_kmac_error {
	/**
	* No error has occured.
	*/
	kDifErrorNone = 0,

	/**
	* The Key Manager has raised an error because the secret key is not valid.
	*/
	kDifErrorKeyNotValid = 1,

	/**
	* An attempt was made to write data into the message FIFO but the KMAC unit
	* was not in the correct state to receive the data.
	*/
	kDifErrorSoftwarePushedMessageFifo = 2,

	/**
	* SW issued a command while a HW application interface was using KMAC.
	*/
	kDifErrorSoftwareIssuedCommandWhileAppInterfaceActive = 3,

	/**
	* The entropy wait timer has expired.
	*/
	kDifErrorEntropyWaitTimerExpired = 4,

	/**
	* Incorrect entropy mode when entropy is ready.
	*/
	kDifErrorEntropyModeIncorrect = 5,

	kDifErrorUnexpectedModeStrength = 6,

	kDifErrorIncorrectFunctionName = 7,

	kDifErrorSoftwareCommandSequence = 8,

	kDifErrorSoftwareHashingWithoutEntropyReady = 9,

	kDifErrorFatalError = 0xC1,

	kDifErrorPackerIntegrity = 0xC2,

	kDifErrorMsgFifoIntegrity = 0xC3,
	} dif_kmac_error_t;

		for (int error_condition = 1; error_condition < 9; error_condition++) {
		switch (error_condition) {

Hmac tests #25825

Hmac tests #25825

Conversation

rprakas-gsc commented Jan 9, 2025

azure-pipelines bot commented Jan 9, 2025

rprakas-gsc commented Jan 14, 2025

nasahlpa 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rprakas-gsc commented Jan 28, 2025 • edited Loading

nasahlpa commented Jan 29, 2025

rprakas-gsc commented Jan 29, 2025 • edited Loading

nasahlpa commented Jan 29, 2025

rprakas-gsc commented Jan 29, 2025

rprakas-gsc commented Feb 7, 2025

rprakas-gsc commented Jan 28, 2025 •

edited

Loading

rprakas-gsc commented Jan 29, 2025 •

edited

Loading