Updated the code with the review feedback #24574

github-gcontributor · 2024-09-14T02:15:05Z

No description provided.

github-gcontributor · 2024-09-14T02:18:34Z

Updated with feedback and as per format/style.
https://github.com/github-gcontributor/opentitan/blob/vt_checkins/sw/device/tests/aes_force_prng_reseed_test.c
Please review.

moidx · 2024-09-15T19:36:22Z

sw/device/tests/aes_force_prng_reseed_test.c

+  // Disable EDN and CSRNG to simulate stalling
+  LOG_INFO("Disabling EDN0, EDN1...");
+  CHECK_DIF_OK(dif_edn_stop(&edn0));
+  CHECK_DIF_OK(dif_edn_stop(&edn1));
+  busy_spin_micros(500);


Use the following function from entropy_testutils:

TRY(entropy_testutils_stop_all());

moidx · 2024-09-15T19:37:47Z

sw/device/tests/aes_force_prng_reseed_test.c

+  LOG_INFO("Conditionally checking AES status after stopping EDN and CSRNG.");
+  LOG_INFO("Waiting for AES to stall after PRNG reseed request...");
+  bool stall = false;
+  CHECK_DIF_OK(dif_aes_get_status(&aes, kDifAesStatusStall, &stall));


The stall condition will only occur after you switch the key. You may have to load a couple of different keys because the PRNG may already be reseeded the first time you call it.

This check should go below after dif_aes_start(...).

Noted. Pushing "waiting for the stall" logic after dif_aes_start()

For loading keys more than once.
Steps will be as below:
generate first key = key
Generate key_share0[i] = kAesModesKey128[i] ^ kKeyShare1[i]
Load first key using dif_aes_start()
Then dif_aes_trigger()

generate second key = key2
Generate key2_share0[i] = kAesModesKey128[i] ^ kKeyShare1[i]+1 --> Different key generation

wait for stall
then enable entropy complex
load data

Load second key using dif_aes_start() later after enabling
Then dif_aes_trigger()
Please advice.

moidx · 2024-09-15T19:39:39Z

sw/device/tests/aes_force_prng_reseed_test.c

+  LOG_INFO("Disabling CSRNG...");
+  CHECK_DIF_OK(dif_csrng_stop(&csrng));
+  CHECK_DIF_OK(dif_entropy_src_stop(&entropy_src));
+
+  // Re-enable Entropy Source and CSRNG in the proper sequence
+  LOG_INFO("Re-initializing Entropy Source and CSRNG...");
+
+  // Initialize and configure Entropy Source
+  dif_entropy_src_config_t entropy_src_config =
+      entropy_testutils_config_default();
+  CHECK_DIF_OK(dif_entropy_src_configure(&entropy_src, entropy_src_config,
+                                         kDifToggleEnabled));
+
+  // Initialize and start CSRNG
+  CHECK_DIF_OK(dif_csrng_configure(&csrng));
+  // set_edn_auto_mode();
+  LOG_INFO("Setting EDN to auto mode...");
+  TRY(entropy_testutils_auto_mode_init());


All this code can be replaced for a single call:

TRY(entropy_testutils_auto_mode_init());

moidx · 2024-09-15T19:40:57Z

sw/device/tests/aes_force_prng_reseed_test.c

+  // Trigger PRNG reseed request
+  LOG_INFO("Triggering PRNG reseed...");
+  CHECK_DIF_OK(dif_aes_trigger(&aes, kDifAesTriggerPrngReseed));


I would expect to see a stall if entropy complex is disabled and you call this command. It may be a good idea to test this as well.

moidx · 2024-09-15T19:45:10Z

sw/device/tests/aes_force_prng_reseed_test.c

+  // T8: Load the plain text to trigger the encryption operation
+  CHECK_DIF_OK(dif_aes_load_data(&aes, in_data_plain));
+
+  // Trigger AES reseed request, specific for manual operation case


kDifAesTriggerStart is not the same as kDifAesTriggerPrngReseed.

Yes. Can you clarify more on this review comment wrt code?

sw/device/tests/aes_force_prng_reseed_test.c

moidx · 2024-09-15T19:49:36Z

Hi @github-gcontributor, Can you also do the following:

Add Bazel target to sw/device/tests/BUILD
Update the commit title and text to describe the changes being introduced in this PR.
Update the PR title and description to follow the commit details.
You will also have to setup your Name and Email in gitgub to be able to sign your commit. This is the reason one of the CI checks is failing. This is needed to meet the CLA guidelines.

github-gcontributor · 2024-09-16T23:14:04Z

@moidx,
Thanks for the review.
Made changes to the code as per the comments.
i.e. The Test checks for AES Stall with Disabled Entropy Complex (and not just edn)
Added BUILD file with target

Please provide feedback on the changes..

github-gcontributor · 2024-09-20T18:58:35Z

Hi @github-gcontributor, Can you also do the following:

Add Bazel target to sw/device/tests/BUILD

Update the commit title and text to describe the changes being introduced in this PR.

Update the PR title and description to follow the commit details.

You will also have to setup your Name and Email in gitgub to be able to sign your commit. This is the reason one of the CI checks is failing. This is needed to meet the CLA guidelines.

Regarding point 4,
I tried making the changes as per the instruction. Using my corporate id, does not allow me to proceed checkin as it would expose the privacy. As a second step, I added "[email protected]" as mentioned in my github steps to protect privacy. However it still asking showing up issue in quick lint check.

Still issue is persistent:
"""
remote: error: GH007: Your push would publish a private email address.
remote: You can make your email public or disable this protection by visiting:
remote: http://github.com/settings/emails
To github.com:github-gcontributor/opentitan.git
! [remote rejected] vt_checkins -> vt_checkins (push declined due to email privacy restrictions)
error: failed to push some refs to 'github.com:github-gcontributor/opentitan.git'
"""
I added my personal id.
With non corporate id, I am disabling the "Keep my email addresses private" option now. Looks like it is allowing me to chekcin now. Waiting for the quick lint

a-will · 2024-09-20T20:00:15Z

You will also have to setup your Name and Email in gitgub to be able to sign your commit. This is the reason one of the CI checks is failing. This is needed to meet the CLA guidelines.

Regarding point 4, I tried making the changes as per the instruction. Using my corporate id, does not allow me to proceed checkin as it would expose the privacy. As a second step, I added "[email protected]" as mentioned in my github steps to protect privacy. However it still asking showing up issue in quick lint check.

Still issue is persistent: """ remote: error: GH007: Your push would publish a private email address. remote: You can make your email public or disable this protection by visiting: remote: http://github.com/settings/emails To github.com:github-gcontributor/opentitan.git ! [remote rejected] vt_checkins -> vt_checkins (push declined due to email privacy restrictions) error: failed to push some refs to 'github.com:github-gcontributor/opentitan.git' """ I added my personal id. With non corporate id, I am disabling the "Keep my email addresses private" option now. Looks like it is allowing me to chekcin now. Waiting for the quick lint

You are contributing to a public repo with a CLA from your company. The corporate email must be exposed in commits, else the contribution cannot be accepted. You must uncheck "Block command line pushes that expose my email" (or else get someone else from your organization to submit the change).

Some people choose to have separate accounts for corporate work, and this enabled them to keep this safeguard for their personal account. I'm not sure if there is a way to make it more granular (like by repo or something).

github-gcontributor · 2024-09-20T22:08:32Z

You will also have to setup your Name and Email in gitgub to be able to sign your commit. This is the reason one of the CI checks is failing. This is needed to meet the CLA guidelines.

Regarding point 4, I tried making the changes as per the instruction. Using my corporate id, does not allow me to proceed checkin as it would expose the privacy. As a second step, I added "[email protected]" as mentioned in my github steps to protect privacy. However it still asking showing up issue in quick lint check.
Still issue is persistent: """ remote: error: GH007: Your push would publish a private email address. remote: You can make your email public or disable this protection by visiting: remote: http://github.com/settings/emails To github.com:github-gcontributor/opentitan.git ! [remote rejected] vt_checkins -> vt_checkins (push declined due to email privacy restrictions) error: failed to push some refs to 'github.com:github-gcontributor/opentitan.git' """ I added my personal id. With non corporate id, I am disabling the "Keep my email addresses private" option now. Looks like it is allowing me to chekcin now. Waiting for the quick lint

You are contributing to a public repo with a CLA from your company. The corporate email must be exposed in commits, else the contribution cannot be accepted. You must uncheck "Block command line pushes that expose my email" (or else get someone else from your organization to submit the change).

Some people choose to have separate accounts for corporate work, and this enabled them to keep this safeguard for their personal account. I'm not sure if there is a way to make it more granular (like by repo or something).

Sure, in that case let me change my id to corporate email.
After disabling "Keep my email addresses private" I am able to checkin with my corporate id. Waiting for quick lint now...

Signed-off-by: Varunkumar Trivedi <[email protected]>

github-gcontributor requested a review from a team as a code owner September 14, 2024 02:15

github-gcontributor requested review from moidx and removed request for a team September 14, 2024 02:15

moidx reviewed Sep 15, 2024

View reviewed changes

github-gcontributor force-pushed the vt_checkins branch 2 times, most recently from 0e063c2 to 21a2971 Compare September 16, 2024 23:04

github-gcontributor changed the title ~~Initial Checkins~~ Updated the code with the review feedback Sep 16, 2024

github-gcontributor force-pushed the vt_checkins branch from 21a2971 to d244b08 Compare September 20, 2024 18:22

github-gcontributor force-pushed the vt_checkins branch 3 times, most recently from b62a3ec to 50fd1cf Compare September 20, 2024 19:46

github-gcontributor force-pushed the vt_checkins branch from 50fd1cf to b735a02 Compare September 20, 2024 21:42

github-gcontributor force-pushed the vt_checkins branch 3 times, most recently from 85dd686 to cb56857 Compare September 20, 2024 23:04

Initial checkin for prng_reseed,

e8aeba7

Signed-off-by: Varunkumar Trivedi <[email protected]>

github-gcontributor force-pushed the vt_checkins branch from cb56857 to e8aeba7 Compare September 21, 2024 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated the code with the review feedback #24574

Updated the code with the review feedback #24574

github-gcontributor commented Sep 14, 2024

github-gcontributor commented Sep 14, 2024

moidx Sep 15, 2024

github-gcontributor Sep 16, 2024

moidx Sep 15, 2024

github-gcontributor Sep 16, 2024

github-gcontributor Sep 16, 2024

moidx Sep 15, 2024

github-gcontributor Sep 16, 2024

moidx Sep 15, 2024

moidx Sep 15, 2024

github-gcontributor Sep 16, 2024

moidx commented Sep 15, 2024

github-gcontributor commented Sep 16, 2024

github-gcontributor commented Sep 20, 2024 •

edited

Loading

a-will commented Sep 20, 2024

github-gcontributor commented Sep 20, 2024 •

edited

Loading

Updated the code with the review feedback #24574

Are you sure you want to change the base?

Updated the code with the review feedback #24574

Conversation

github-gcontributor commented Sep 14, 2024

github-gcontributor commented Sep 14, 2024

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

moidx commented Sep 15, 2024

github-gcontributor commented Sep 16, 2024

github-gcontributor commented Sep 20, 2024 • edited Loading

a-will commented Sep 20, 2024

github-gcontributor commented Sep 20, 2024 • edited Loading

github-gcontributor commented Sep 20, 2024 •

edited

Loading

github-gcontributor commented Sep 20, 2024 •

edited

Loading