[sival,kmac] Add kmac_error_conditions_test #24765

nasahlpa · 2024-10-10T14:01:43Z

This adds the new kmac_error_conditions_test test that is defined in the chip_sw_kmac_error_conditions chip level test.

nasahlpa · 2024-10-10T15:58:20Z

@vogelpi the test for the ErrWaitTimerExpired and ErrIncorrectEntropyMode is not working on the CW310 FPGA as we turn by default the masks off. I've marked the test as broken and referenced to #15530. Is this OK for you?

engdoreis · 2024-10-18T16:08:11Z

sw/device/tests/kmac_error_conditions_test.c

+                                      /*processed=*/NULL, /*capacity=*/NULL);
+
+  // It is OK to get kDifError at this point because of possible timeout.
+  CHECK(res == kDifOk || res == kDifError);


Nit.

Suggested change

CHECK(res == kDifOk || res == kDifError);

TRY_CHECK(res == kDifOk || res == kDifError);

engdoreis · 2024-10-18T16:08:31Z

sw/device/tests/kmac_error_conditions_test.c

+
+  // Check if there is a new error.
+  bool irq_err_pending;
+  CHECK_DIF_OK(


Suggested change

CHECK_DIF_OK(

TRY(

engdoreis · 2024-10-18T16:08:59Z

sw/device/tests/kmac_error_conditions_test.c

+  if (irq_err_pending) {
+    dif_kmac_error_t err_status;
+    uint32_t err_info;
+    CHECK_DIF_OK(dif_kmac_get_error(&kmac, &err_status, &err_info));


This would apply to the others below.

Suggested change

CHECK_DIF_OK(dif_kmac_get_error(&kmac, &err_status, &err_info));

TRY(dif_kmac_get_error(&kmac, &err_status, &err_info));

nasahlpa · 2024-10-21T05:45:35Z

Thanks for your review, @engdoreis! As suggested, I've changed CHECK_DIF_OK to TRY.

vogelpi

Thanks for the PR @nasahlpa !

The test implementation looks mostly good.

Can you please elaborate on

ErrSwIssuedCmdInAppActive could not be implemented in SW as triggering a SW command while the app interface is active is not possible.

There are two error codes which we can't test related to this limitation. It would be good understand what the underlying issue is. I we agree to not test this, we should update the testplan accordingly.

Checking kmac_pkg.sv I noted there are some other currently untested error codes that we should actually be able to test:

ErrSwHashingWithoutEntropyReady - triggering a hash operation without setting the entropy mode first. This probably doesn't work in ROM_EXT though.
ErrShadowRegUpdate - triggering a shadow register update error. This should be super straight forward.

The remaining three are related to FI countermeasures and escalations. We probably can't test this using this test anyway.

sw/device/tests/BUILD

sw/device/tests/kmac_error_conditions_test.c

vogelpi · 2024-10-21T12:02:57Z

sw/device/tests/kmac_error_conditions_test.c

+ * the KMAC block. Note that this test is not exhaustive, i.e., the test does
+ * not trying to reach the ErrorSoftwareCommandSequence error state from each
+ * other state.


Thanks for the comment, I think this is okay but maybe it would be good for someone from the SiVal team to confirm @luismarques , @engdoreis ?

sw/device/tests/kmac_error_conditions_test.c

engdoreis

Thanks @nasahlpa for the PR, You are using the mmio API on the test code to change the registers directly, isn't it worth creating DIFs to abstracts these registers access

sw/device/tests/kmac_error_conditions_test.c

engdoreis · 2024-11-07T09:42:39Z

sw/device/tests/kmac_error_conditions_test.c

+
+  // Check whether the KMAC ERR bit was set in the interrupt state register.
+  TRY(dif_kmac_has_error_occurred(kmac, &error));
+  CHECK(error, "No error was triggered.");


This applies throughout this file.

Suggested change

CHECK(error, "No error was triggered.");

TRY_CHECK(error, "No error was triggered.");

As far as I see, the difference between CHECK and TRY_CHECK is that the former aborts the test and the latter continues but returns a status_t? Wouldn't it be better to abort the test (so using CHECK)?

sw/device/tests/kmac_error_conditions_test.c

This adds the new `kmac_error_conditions_test` test that is defined in the `chip_sw_kmac_error_conditions` chip level test. Signed-off-by: Pascal Nasahl <[email protected]>

nasahlpa · 2024-11-07T13:58:31Z

Thanks for your review, @vogelpi and @engdoreis

I've made quite some significant changes to this PR to address your review, @vogelpi:

Executed the test on CW340 - this works now and the test also gets executed in CI. Thanks @jwnrt for setting this up.
Worked on testing ErrShadowRegUpdate. I've noticed that this ErrShadowRegUpdate (and the corresponding kDifErrorShadowRegisterUpdate) is never used in HW and SW. Instead, according to the documentation, a shadow register update error is indicated by a recoverable alert. I've added a test to test whether the HW complies to the documentation. Do we want to remove ErrShadowRegUpdate and kDifErrorShadowRegisterUpdate from the HW and software in a separate PR?
Implemented the ErrSwIssuedCmdInAppActive test.
Implemented the ErrSwPushedMsgFifo test according to the test plan.
Implemented the ErrSwHashingWithoutEntropyReady test.

@engdoreis I just have one question regarding the difference CHECK <-> TRY_CHECK (please see in the comment above). Regarding the DIFs, I don't think that we should add these register writes to the DIF as they are only required to trigger an error behavior. Adding them could cause some unintended misuse by others, hence, I prefer leaving it as it is.

vogelpi · 2024-11-07T15:19:34Z

This sounds all great, excellent work @nasahlpa !

About the ErrShadowRegUpdate error code: I see the corresponding alert condition is also signaled via STATUS register. So software will learn about having done something wrong, even if it's not via ERR_CODE register. And your test checks this which is very nice! I agree that we should remove ErrShadowRegUpdate and kDifErrorShadowRegisterUpdate from the HW and SW in a separate PR.

sw/device/tests/kmac_error_conditions_test.c

nasahlpa force-pushed the chip_sw_kmac_error_conditions branch 2 times, most recently from e8c149d to 2f957c2 Compare October 10, 2024 15:56

nasahlpa force-pushed the chip_sw_kmac_error_conditions branch 2 times, most recently from 281e16c to 0134925 Compare October 11, 2024 06:33

nasahlpa marked this pull request as ready for review October 17, 2024 05:27

nasahlpa requested a review from a team as a code owner October 17, 2024 05:27

nasahlpa requested review from HU90m, vogelpi and engdoreis and removed request for a team and HU90m October 17, 2024 05:27

engdoreis reviewed Oct 18, 2024

View reviewed changes

nasahlpa force-pushed the chip_sw_kmac_error_conditions branch from 0134925 to a2882d9 Compare October 21, 2024 05:45

vogelpi reviewed Oct 21, 2024

View reviewed changes

nasahlpa force-pushed the chip_sw_kmac_error_conditions branch from a2882d9 to 3517dc9 Compare November 6, 2024 19:10

nasahlpa marked this pull request as draft November 6, 2024 20:04

engdoreis reviewed Nov 7, 2024

View reviewed changes

nasahlpa force-pushed the chip_sw_kmac_error_conditions branch 3 times, most recently from 0b55457 to ef9514c Compare November 7, 2024 13:53

[sival,kmac] Add kmac_error_conditions_test

d18142a

This adds the new `kmac_error_conditions_test` test that is defined in the `chip_sw_kmac_error_conditions` chip level test. Signed-off-by: Pascal Nasahl <[email protected]>

nasahlpa force-pushed the chip_sw_kmac_error_conditions branch from ef9514c to d18142a Compare November 7, 2024 13:59

nasahlpa marked this pull request as ready for review November 7, 2024 13:59

vogelpi approved these changes Nov 7, 2024

View reviewed changes

sw/device/tests/kmac_error_conditions_test.c Show resolved Hide resolved

vogelpi added the CherryPick:earlgrey_1.0.0 This PR should be cherry-picked to earlgrey_1.0.0 label Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sival,kmac] Add kmac_error_conditions_test #24765

[sival,kmac] Add kmac_error_conditions_test #24765

nasahlpa commented Oct 10, 2024 •

edited

Loading

nasahlpa commented Oct 10, 2024

engdoreis Oct 18, 2024

engdoreis Oct 18, 2024

engdoreis Oct 18, 2024

nasahlpa commented Oct 21, 2024

vogelpi left a comment

vogelpi Oct 21, 2024

engdoreis left a comment

engdoreis Nov 7, 2024

nasahlpa Nov 7, 2024

nasahlpa commented Nov 7, 2024

vogelpi commented Nov 7, 2024

	CHECK(res == kDifOk \|\| res == kDifError);
	TRY_CHECK(res == kDifOk \|\| res == kDifError);

	CHECK_DIF_OK(dif_kmac_get_error(&kmac, &err_status, &err_info));
	TRY(dif_kmac_get_error(&kmac, &err_status, &err_info));

	CHECK(error, "No error was triggered.");
	TRY_CHECK(error, "No error was triggered.");

[sival,kmac] Add kmac_error_conditions_test #24765

Are you sure you want to change the base?

[sival,kmac] Add kmac_error_conditions_test #24765

Conversation

nasahlpa commented Oct 10, 2024 • edited Loading

nasahlpa commented Oct 10, 2024

engdoreis Oct 18, 2024

Choose a reason for hiding this comment

engdoreis Oct 18, 2024

Choose a reason for hiding this comment

engdoreis Oct 18, 2024

Choose a reason for hiding this comment

nasahlpa commented Oct 21, 2024

vogelpi left a comment

Choose a reason for hiding this comment

vogelpi Oct 21, 2024

Choose a reason for hiding this comment

engdoreis left a comment

Choose a reason for hiding this comment

engdoreis Nov 7, 2024

Choose a reason for hiding this comment

nasahlpa Nov 7, 2024

Choose a reason for hiding this comment

nasahlpa commented Nov 7, 2024

vogelpi commented Nov 7, 2024

nasahlpa commented Oct 10, 2024 •

edited

Loading