Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add simple test script to test all modes supported by capture.py #150

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

vogelpi
Copy link
Collaborator

@vogelpi vogelpi commented Aug 10, 2023

This PR adds a very simple script to test all capture modes currently supported by capture.py. What it does is running the capture commands and copy the trace sample figures under tmp for visual inspection. This is useful for a couple of things we need to do soon:

  1. Updating ChipWhisperer API
  2. Adding CI support
  3. Refactoring capture.py

The script is not beautiful nor smart but it does what it needs. We can improve / rewrite as we do the things above.

done

# OTBN
# TODO!
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wettermo and @vrozic, would you mind providing me the OTBN commands we're currently supporting? I don't have a good overview of what's working and known broken atm. Thanks!

Copy link
Collaborator

@wettermo wettermo Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vogelpi basically there are just two otbn commands:

otbn_test_list["otbn-vertical"]=100
otbn_test_list["otbn-vertical-batch"]=1000

The differentiation between keygen and modinv is done through an "app" parameter in the relevant capture config files

Comment on lines 41 to 53
aes_test_list["aes-random"]=100
aes_test_list["aes-random-batch"]=1000
aes_test_list["aes-fvsr-key"]=100
aes_test_list["aes-fvsr-key-batch"]=1000

for test in ${!aes_test_list[@]}; do
echo Testing ${test} on ${BOARD}
echo Testing ${test} on ${BOARD} - `date` >> "tmp/${BOARD}_test_capture.log"
if [ "${test}" = "aes-random" ]; then
ARGS="--force-program-bitstream"
else
ARGS=""
fi
Copy link
Contributor

@vrozic vrozic Aug 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are not always executed in the order they're listed here. Consequently, argument --force-program-bitstream is not always set for the first test that is executed. For this reason, some tests end up running using a wrong bitstream.
I would fix this by moving ARGS="--force-program-bitstream" before the for loop and ARGS="" at the end of the loop body.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vrozic for your suggestion, this is now changed.

Comment on lines 64 to 74
kmac_test_list["kmac-random"]=100
kmac_test_list["kmac-fvsr-key"]=100
kmac_test_list["kmac-fvsr-key-batch"]=1000

for test in ${!kmac_test_list[@]}; do
echo Testing ${test} on ${BOARD}
echo Testing ${test} on ${BOARD} - `date` >> "tmp/${BOARD}_test_capture.log"
if [ "${test}" = "kmac-random" ]; then
ARGS="--force-program-bitstream"
else
ARGS=""
fi
NUM_TRACES=${kmac_test_list[${test}]}
./capture.py --cfg-file capture_${MODE}_${BOARD}.yaml capture ${test} \
--num-traces ${NUM_TRACES} ${ARGS} &>> "tmp/${BOARD}_test_capture.log"
mv projects/sample_traces_${MODE}.html tmp/${BOARD}_${test}_traces.html
done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Comment on lines 85 to 121
sha3_test_list["sha3-fvsr-data"]=100
sha3_test_list["sha3-fvsr-data-batch"]=1000

for test in ${!sha3_test_list[@]}; do
echo Testing ${test} on ${BOARD}
echo Testing ${test} on ${BOARD} - `date` >> "tmp/${BOARD}_test_capture.log"
if [ "${test}" = "sha3-fvsr-data" ]; then
ARGS="--force-program-bitstream"
else
ARGS=""
fi
NUM_TRACES=${sha3_test_list[${test}]}
./capture.py --cfg-file capture_${MODE}_${BOARD}.yaml capture ${test} \
--num-traces ${NUM_TRACES} ${ARGS} &>> "tmp/${BOARD}_test_capture.log"
mv projects/sample_traces_${MODE}.html tmp/${BOARD}_${test}_traces.html
done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

Comment on lines 102 to 103
# OTBN
# TODO!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# OTBN
# TODO!
# OTBN keygen
MODE="otbn_vertical_keygen"
declare -A otbn_keygen_test_list
otbn_keygen_test_list["otbn-vertical"]=100
otbn_keygen_test_list["otbn-vertical-batch"]=1000
ARGS="--force-program-bitstream"
for test in ${!otbn_keygen_test_list[@]}; do
echo Testing ${test} on ${BOARD}
echo Testing ${test} on ${BOARD} - `date` >> "tmp/${BOARD}_test_capture.log"
NUM_TRACES=${otbn_keygen_test_list[${test}]}
./capture.py --cfg-file capture_${MODE}.yaml capture ${test} \
--num-traces ${NUM_TRACES} ${ARGS} &>> "tmp/${BOARD}_test_capture.log"
mv projects/sample_traces_ecdsa_keygen.html tmp/${BOARD}_${test}_traces.html
ARGS=""
done
# OTBN modinv
MODE="otbn_vertical_modinv"
declare -A otbn_modinv_test_list
otbn_modinv_test_list["otbn-vertical"]=100
for test in ${!otbn_modinv_test_list[@]}; do
echo Testing ${test} on ${BOARD}
echo Testing ${test} on ${BOARD} - `date` >> "tmp/${BOARD}_test_capture.log"
NUM_TRACES=${otbn_modinv_test_list[${test}]}
./capture.py --cfg-file capture_${MODE}.yaml capture ${test} \
--num-traces ${NUM_TRACES} ${ARGS} &>> "tmp/${BOARD}_test_capture.log"
mv projects/sample_traces_ecdsa_modinv.html tmp/${BOARD}_${test}_traces.html
done

Something like this works well for testing otbn vertical keygen and modinv.
A couple of caveats:

  1. pll_frequency in capture_otbn_vertical_modinv.yaml needs to be reduced to 91 MHz
  2. parameter test_type: (KEY or SEED) cannot be overridden through command line. This is currently only testing for test_type: KEY.
  3. OTBN commands don't follow the same naming strategy used in the rest of the script. As a result we end up with two tests named otbn_vertical. We should come up with a convention for naming capture commands and modes. I'll open an issue for this.
  4. This doesn't include capture of ECDSA256 and ECDSA384. I suggest to add those later after we close [objs] update ECC384 binaries #149

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vrozic and @wettermo for your input. I've included this as is and it seems to work okay. Just needed to add something to distinguish the result figures for keygen and modinv.

Regarding the points you've raised @vrozic :

  1. This seems to be handled by the yaml file entirely and I believe it's fine as is
  2. Good point, I believe testing KEY is sufficient. IIRC, this is what we really care about here.
  3. Thanks, sounds good.
  4. Thanks, I've added a TODO in the script and will update/merge that other PR in OpenTitan.

Signed-off-by: Pirmin Vogel <vogelpi@lowrisc.org>
@vogelpi vogelpi force-pushed the add-simple-capture-test-script branch from 3f12af4 to 4e93b4c Compare September 8, 2023 14:14
@vogelpi vogelpi requested a review from vrozic September 8, 2023 14:28
Copy link
Contributor

@vrozic vrozic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vogelpi for the update.
I've tested the latest changes and the script works well.
All graphs look correct except for the sha3-fvsr-data (non-batch) where it seems that the wrong offset is used. This is a pre-existing problem.

In my view this is ready to merge.

@vogelpi
Copy link
Collaborator Author

vogelpi commented Sep 8, 2023

Thanks @vrozic , yes I've noted the thing with the sha3 non-batch capture as well.

@vogelpi vogelpi merged commit afea793 into lowRISC:master Sep 8, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants