driver-only ECDSA: fix testing disparities in ecp, random, se_driver_hal

[valeriosetti]

Description

The goal of this PR is to solve test disparities with and without driver implementation when ECDSA is accelerated. Full description is provided in #6856.
Resolves #6856

NOTES

• This PR depends on Driver-only ECDSA starter #6855 so there might be CI failures before that happens. The actual implementation of this PR starts at c26af7b
• The main reason for which this PR has been created is to show @mpg and @gilles-peskine-arm a change to analyze_outcomes.py which was mentioned in the discussion of driver-only ECDSA: fix testing disparities in ecp, random, se_driver_hal #6856 here.

Gatekeeper checklist

• changelog not required
• backport not required
• tests provided

[valeriosetti]

Hi @gilles-peskine-arm, I'm adding you as reviewer here since we started a discussion on the original issue and I thought you might be interested in this.
Let's also continue the discussion here for simplicity ;)

[valeriosetti]

Compared to my previous attempt and following @gilles-peskine-arm suggestion I did the following:

• instead of adding a new possible option for depends_on in the .data file, which didn't look very good to me, I preferred to add a new "line" to the .data file called test_options
• this line is parsed from the generate_test_code.py script
• several test options can be specified there, but so far only EXCLUDE_FROM_DRIVER_TEST_COVERAGE_PARITY is supported
• if EXCLUDE_FROM_DRIVER_TEST_COVERAGE_PARITY is defined for that specific test and if the script is invoked with --driver_coverage_test command line option (from the Makefile in our case), then that test is skipped

As a consequence:

• there is no need to further modify analyze_outcomes.py since the "critical" test is not present so it cannot cause disparities
• the "critical" test is not always skipped but only when the DRIVER_COVERAGE_TEST environment variable is defined when invoking the makefile from the command line (i.e. DRIVER_COVERAGE_TEST=1 make tests)

Wdyt?

[mpg]

This needs rebasing now that #6855 has been merged. Also, since this is the first follow-up to #6855, can you fix the two typos that @mprse spotted in his review?

[valeriosetti]

This needs rebasing now that #6855 has been merged.

yes, I'll do it immediately

Also, since this is the first follow-up to #6855, can you fix #6855 (review)?

sure!

Only 1 last question on my side. Concerning the discussion in the original issue and in particular to this part:

what do you think about my point that referring to a case by its description would be acceptable in the specific case of an ignore list, since any change in the description would be noticed as a result of the script failing?

That's not great, but it's acceptable.

it seems to me that I can return back to the original implementation I proposed, right? It was based on a "simple" check of the test case description in order to understand which test was to be skipped in the driver's coverage analysis

[mpg]

it seems to me that I can return back to the original implementation I proposed, right? It was based on a "simple" check of the test case description in order to understand which test was to be skipped in the driver's coverage analysis

Yes, if that's not too much trouble.

What I like with the original implementation is that all the information about what to ignore is centralized in one data structure in analyze_outcomes.py. Also, it allows trivially to specify what can be ignored per reference-driver pair. Even in terms of code, I think it's better if the changes are confined to analyze_outcomes.py, rather than having to change generate_test_code.py - if we want to change the architecture for driver parity testing in the future, it will be easier if its implementation is well centralized.

[valeriosetti]

Yes, if that's not too much trouble.

This is not a problem at all! I asked mostly to be sure what to do ;)

Even in terms of code, I think it's better if the changes are confined to analyze_outcomes.py

I agree. As you might have seen yesterday I tried to fix also other issues (in other python scripts) coming from the fact that I changed something in the test generator script. The original implementation was definitely more limited in scope as number of changes

[valeriosetti]

This last forced push is to cancel out the previous implementation, which attempted to modify the test generator script, and to return to a more simple solution which only acts on analyze_outcomes.py.
Now it should be ready for review (if the CI is happy)

[mpg]

Looks all good to me! Thanks!

[mprse]

LGTM. Left only one minor proposition for improvement.
Tested locally with positive results.

[mprse]

Minor: I think these lines can be move to line 87, so we have everything together.
Additionally if we continue loop because test suite was ignored(line 82) or test was not executed(line 86) it is not needed.

[mpg]

@valeriosetti Both Przemek and I left optional comments. Since the CI results have expired in the meantime, I think it would make sense for you to push an updated version, so the CI will run again tonight and we can re-review and hopefully merge tomorrow.

[mpg]

LGTM

[mprse]

LGTM