-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Port mbed TLS self test to greentea #2603
Conversation
@andresag01 Sorry for not looking at this sooner! It looks good, but the entropy test fails for targets that don't have the |
@bridadan thanks for having a look at this pull request. Could you please let us know what are the targets that fail? I presume that the reason for the failure is because they do not have entropy source enabled, which part of what we wanted to test. I am not sure what is the intended behaviour in this case, what do @yanesca, @sbutcher-arm and @pjbakker think? |
@adustm Please let me know if you're able to run the tests in this PR against the HW entropy for STM32 that you recently implemented. |
@andresag01 I think that's probably the case, though I admit I'm not all that familiar with mbedTLS and the entropy sources. If that test cannot pass on targets that don't support the entropy source yet, then I would think it doesn't make sense to run the test since there's no way it will pass. Instead it should be ran only when the feature is enabled. |
The test could be gated by the macro as others are
|
Hello,
LGTM Cheers |
@sg- It should be noted that the the For example,
|
@andresag01 Probably should have tagged you in the above message too 😄 |
Hello, is there any news on the topic ? |
We are implementing @sbutcher-arm proposal i.e. have an additional configuration file in mbed TLS that gets included when DEVICE_ENTROPY_SOURCE is not defined in targets.json. We have prepared two PRs for this: Mbed-TLS/mbedtls#615 (mbed TLS) They are still marked as DRAFT because this is still work in progress. |
+1 |
The pull request looks good to me. |
be798c6
to
8e4faa0
Compare
/morph test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Result: FAILUREYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 980 Build failed! |
@0xc0170 Looks like a few targets can't fit this test in memory. Perhaps we need to break this up into a few binaries? |
@andresag01 Can you please have a look at failures, and how we can fix those? |
We had a quick look, the small platforms are having problems. RAM overflows, by >1k and similar sizes, therefore might be considered if those targets are either supported or should be (memory size restrictions). |
Because mbed TLS is a static link library you only need to build in the functions that you need, and you can leave out everything else. If you only want AES - you can link in only AES and exclude all hashing functions, X.509 support and everything else. Also, despite the fact asymmetric crypto may take a very long time on the smaller devices there may be some use cases that still want to do it if they don't care about the time taken to execute. That means that's it's not obvious whether mbed TLS is appropriate or not for some of these tiny devices. Therefore it still makes sense to run all tests on all devices for the moment as we can't second guess all use cases, even if some don't seem to make immediate sense. Therefore, we'll break up the tests into smaller functional entities as @bridadan suggests to allow all tests to run. In this process we might find some of the bigger features still don't fit on the tiniest devices (such as X.509). If this is true we may look again at the feature set and see what makes sense and how to exclude such features. |
@sbutcher-arm I think that's probably the simplest approach for now. The one thing I would recommend is to try pairing some of the lightweight and related functions in the same test binary instead of creating one binary for each function. The reason being is flashing the binary takes the majority of the time for most of theses tests, so the testing time will increase quite a bit if you add 20 binaries. |
8e4faa0
to
5fc271b
Compare
@sbutcher-arm, @yanesca: I have rebased the changes and split the selftests into four functional components as previously discussed, but this is still failing the build test in some platforms. We could split these into even smaller parts. |
/morph test |
Result: FAILUREYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 1005 Build failed! |
@andresag01 I know you said some platforms are failing, just reran the CI to get a feel for what is still affected. |
5fc271b
to
d6f40b6
Compare
Examining the problem, it looks as if some tests (and also functions and features) won't fit on some devices without target specific optimisations. That means that we can't adopt a 'on configuration of tests will suit all targets' approach, and we'll need to find some way to configure tests per target - which we can't do today. Because the immediate need is to test entropy sources, and for the sake of getting into the test framework some tests we can build on, we've removed all self-tests apart from entropy and SHA-2, which hopefully should be small and simple enough to run on all devices. From there we can expand the set of tests in later PR's. |
/morph test |
Result: SUCCESSYour command has finished executing! Here's what you wrote!
Outputmbed Build Number: 1011 All builds and test passed! |
This set of tests is a port of the mbed TLS test program at
mbedtls/programs/test/selftest.c
. The test executes thembedtls_*_self_test()
functions in the library modules and checks the output (PASS, FAIL). This patch ports the self test program to use mbedgt instead but maintains the same functionality.