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

ta: os_test: fetch system time after the one second delay for proper comparaison #703

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

clementfaure
Copy link
Contributor

…time comparaison

In the regression_1006 test, the wrapped TA time is compared to the system time. The time delay for this test is one second. The system time is fetched at the very begining of the test function.

For this test to pass, it is assumed that the system time fetched is at least one second. That might not be the case in a system where the test is started right after a deep sleep state where the system counter gets reset.

To solve this issue, fetch the system counter after the one second delay.

TEE_GetSystemTime(&sys_t);
printf("system time %u.%03u\n", (unsigned int)sys_t.seconds,
(unsigned int)sys_t.millis);

if (t.seconds > sys_t.seconds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test seems a bit strange. What are we testing against here, a time that has wrapped too much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this test checks if the TA time wrapped correctly?
If the TA time wraps correctly after the one second delay, the TA time should be inferior to one second. That TA time will always be inferior to the system time (with that patch).
If the TA time does not wrap and stays to its max value, the TA time will be superior to system time. There is still a chance where TA time and system time are at the maximum value at the very same time ... but that's unlikely.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the test checks that TA time has indeed wrapped. Maybe testing against 0 should be sufficient.

@etienne-lms
Copy link
Contributor

For this test to pass, it is assumed that the system time fetched is at least one second. That might not be the case in a system where the test is started right after a deep sleep state where the system counter gets reset.

It seems the issue is rather that System Time got reset during a low power state transition whereas it should not.
That said, I think testing TA wrapped time against 0 is enough for this test.

@clementfaure
Copy link
Contributor Author

For this test to pass, it is assumed that the system time fetched is at least one second. That might not be the case in a system where the test is started right after a deep sleep state where the system counter gets reset.

It seems the issue is rather that System Time got reset during a low power state transition whereas it should not. That said, I think testing TA wrapped time against 0 is enough for this test.

I agree that a system counter reset should not be expected, we are investigating. However, I still think the logic of this test is wrong. Let's imagine you system boots super-fast and run xtest, this test will fail.

@etienne-lms
Copy link
Contributor

I agree that there is no need to test TA wrapped time against TEE system time. We just need to test it wrapped. As we expect only few seconds have passed, testing t_seconds < 2 should be enough, or maybe 5.
Testing against 0 as I previously suggested is a non-sense 😲.

@clementfaure clementfaure force-pushed the upstream/fix_sys_time branch from a3b45fe to 23089c0 Compare November 6, 2023 08:52
Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Acked-by: Etienne Carriere <[email protected]>

@clementfaure clementfaure force-pushed the upstream/fix_sys_time branch from 23089c0 to 6c4217b Compare November 6, 2023 09:41
@clementfaure
Copy link
Contributor Author

Tag applied

@jenswi-linaro
Copy link
Contributor

Acked-by: Jens Wiklander <[email protected]>

In the TA time wrap test, the TA time is set to its maximum value. After
one second delay, the TA time was compared to the system time.

The comparaison with the system time might not be pertinent as it is
most likely more than dozen seconds.

Compare the wrapped time to a fixed value instead.

Signed-off-by: Clement Faure <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
@clementfaure clementfaure force-pushed the upstream/fix_sys_time branch from 6c4217b to 89cc1d7 Compare November 6, 2023 13:10
@clementfaure
Copy link
Contributor Author

Tags applied

@jforissier jforissier merged commit a2c1ce3 into OP-TEE:master Nov 6, 2023
1 check 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.

4 participants