add a subtest for non-NULL memref of size 0 + misc. cleanup #708

vincent-mailhol · 2023-11-17T16:38:30Z

This is a series of four patches. The first five patches:

regression 1033: remove trailing space characters
ta: os_test: undefine TA2TA_BUF_SIZE
ta: os_test: return TEE_ERROR_BAD_PARAMETERS for incorrect parameters
ta: os_test: do not print caller function name in EMSG()
ta: os_test: do not call TEE_CloseTASession() if session is not opened

are clean-ups (details in patch description).

The final patch:

regression 1016: add a subtest for non-NULL memref of size 0

adds a test case for OP-TEE/optee_os#6405 as requested by @jenswi-linaro.

Note that OP-TEE/optee_os#6405 is a prerequisite of this PR. Without it, the test will fail.

etienne-lms

Comments for commit "regression 1033: remove trailing whitespace":
Prefer s/whitespace/space characters/
s/opte/OP-TEE/
Remove the empty line between Link: tag and the other tags.
For the Link: tag, maybe prefer:

- [1] Linux kernel coding style §Indentation
- Link: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation 
+Link: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation [1]

etienne-lms

Acked-by: Etienne Carriere <[email protected]> for commit
"ta: os_test: undefine TA2TA_BUF_SIZE".

etienne-lms

Reviewed-by: Etienne Carriere <[email protected]> for commit
"ta: os_test: do not call TEE_CloseTASession() if session is not opened".

vincent-mailhol · 2023-11-17T18:10:30Z

Comments for commit "regression 1033: remove trailing whitespace": Prefer s/whitespace/space characters/ s/opte/OP-TEE/ Remove the empty line between Link: tag and the other tags. For the Link: tag, maybe prefer:

Done, except of the whitespace occurrence in the verbatim quote which I left untouched.

- [1] Linux kernel coding style §Indentation
- Link: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation 
+Link: https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation [1]

Replaced by

[1] https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation

etienne-lms · 2023-11-17T18:27:31Z

Done, except of the whitespace occurrence in the verbatim quote which I left untouched.

Ok

[1] https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation

Checkpatch would complain. It requests message lines to be 72 char max and a Link: tag to be straight followed by an URL.

vincent-mailhol · 2023-11-18T14:34:06Z

[1] https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation

Checkpatch would complain. It requests message lines to be 72 char max and a Link: tag to be straight followed by an URL.

The limit used to be 72 but is now 75: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl#n3264

Yes, it will complain. But checkpatch.pl is known to yield false positives. Contributors are normally asked to "be able to justify all violations that remain in your patch". Ref: point 5 of the Linux Kernel patch submission checklist. So let me justify my violation.

First, because I am well aware of that specific checkpatch rule, I normally use the syntax:

[tag] Title and paragraph names
Link: URL

which do not yield any warnings and allow to give extra details (some URL are not self explanatory). I stole this syntax from someone else, do not remember who, and, since then, never received any complains on the kernel mailing.

You complained about this syntax. Maybe it is too verbose: fair. So I changed it to use:

[tag] URL

This syntax is popular and often used. On my local linux kernel tree:

$ git log --format=format:%B | egrep "^\[[0-9]+\]:? https?://\S+$" | wc -l
8477

You suggested the alternate syntax:

Link: URL [tag]

This is not conventional. If I look at Markdown footnotes, Wikipedia footnote, Latex bibtex, or any other document in my bookshelf the [tag] or any other reference index comes always first, not last.
Putting the tag at the end is a less popular practice. Some people use this syntax as a workaround of checkpatch warning, but they are the minority:

$ git log --format=format:%B | egrep "^Link: https?://\S+ \[[0-9]+\]$" | wc -l
1049

So here, despite of the checkpatch warning, I am just following the majority. I hope that the above is enough to justify why I purposely violated the 75 column limit in the second iteration of my patch.

While at it, I did not comment on your previous request to change whitespace into space characters because I do not like to argue on nitpicks and I did not want to spend more time on this trivial patch. Taking the linux kernel tree as a reference:

$ git log | egrep -i "white ?space" | wc -l
7309
$ git log | egrep -i "space character" | wc -l
101

As you can see, "whitespace" is the norm, "space character" is marginal. Honestly, I am really surprised to receive nitpicks on some wording or formatting that are well accepted elsewhere.

etienne-lms

Honestly, I am really surprised to receive nitpicks on some wording or formatting that are well accepted elsewhere.

Maybe because I may be wrong. I often am, allow me to be. Your arguments make sense (I'm sorry you felt the need for such a long explanation but I thank you for the details). There's a checkpatch CI test on optee_test P-Rs. We'll ask our dear maintainers to enable it for you P-R and see its result.

For commit "regression 1016: add a subtest for non-NULL memref of size 0":
The overall test looks good to me but for few minor issues to address, see my review comments.

For commit "regression 1033: remove trailing space characters":
Acked-by: Etienne Carriere <[email protected]>

etienne-lms · 2023-11-18T21:13:08Z

host/xtest/regression_1000.c

@@ -1377,6 +1377,7 @@ static void xtest_tee_test_1016(ADBG_Case_t *c)
 	TEEC_Session session = { };
 	TEEC_Operation op = TEEC_OPERATION_INITIALIZER;
 	uint32_t ret_orig = 0;
+	int dummy;


Right. As per Linux coding standard, not initializing it would have been fine. But I forgot that OP-TEE introduced an extra rule to force such default initialization: https://optee.readthedocs.io/en/latest/general/coding_standards.html#coding-standards.

I will pay more attention on this point.

etienne-lms · 2023-11-18T21:14:59Z

ta/os_test/os_test.c

+	static const TEE_UUID test_uuid = TA_OS_TEST_UUID;
+	TEE_TASessionHandle sess = TEE_HANDLE_NULL;
+	uint32_t ret_orig = 0;
+	TEE_Result res;


= TEE_ERROR_GENERIC (or any TEE_xxx result value)

TEE_ERROR_GENERIC seems best.

etienne-lms · 2023-11-18T21:15:29Z

ta/os_test/os_test.c

+		return TEE_ERROR_GENERIC;
+
+	/*
+	 * This test expects all buffer to be non-NULL but all sizes


s/buffer/buffer pointers/

I changed from buffer to memory references to get closer to the global platform vocabulary.

etienne-lms · 2023-11-18T21:16:27Z

ta/os_test/os_test.c

+	     !params[2].memref.buffer) ||
+	    (params[0].memref.size || params[1].memref.size ||
+	     params[2].memref.size))
+		return TEE_ERROR_GENERIC;


Prefer TEE_ERROR_BAD_PARAMETERS since these are not the expected input parameters.
Ditto at line 1187.

I guess you took examples from the 3 above functions. Actually they should be fixed to use TEE_ERROR_BAD_PARAMETERS also but that out of the scope of your change.

Yes, this is a copy/paste issue. Fixed.

that out of the scope of your change.

Well, regardless, this series already have three clean-up patches, so while at it: 35dfc81.

etienne-lms · 2023-11-18T21:16:46Z

ta/os_test/os_test.c

+	if ((!params[0].memref.buffer || !params[1].memref.buffer ||
+	     !params[2].memref.buffer) ||
+	    (params[0].memref.size || params[1].memref.size ||
+	     params[2].memref.size))


can remove inner parentheses

Allow me another nitpicking suggestion:

if (!params[0].memref.buffer || params[0].memref.size || !params[1].memref.buffer || params[1].memref.size || !params[2].memref.buffer || params[2].memref.size) return TEE_ERROR_BAD_PARAMETERS;

That's not a strong opinion of mine, keep your ordering if you rather like.

can remove inner parentheses

Yes, the grouping was an attend to reflect the semantic of the comment above:

if (!((all memory references are non-NULL) && (all sizes are zero)))

equivalent to:

if ((!(all memory references are non-NULL)) || (!(all sizes are zero))))

I do not mind removing those extra brackets.

Allow me another nitpicking suggestion:

if (!params[0].memref.buffer || params[0].memref.size || !params[1].memref.buffer || params[1].memref.size || !params[2].memref.buffer || params[2].memref.size) return TEE_ERROR_BAD_PARAMETERS;

That's not a strong opinion of mine, keep your ordering if you rather like.

Yep, I actually also thought about this syntax before submitting the patch. OK with me, suggestion applied.

etienne-lms · 2023-11-18T21:18:33Z

ta/os_test/os_test.c

+	res = TEE_OpenTASession(&test_uuid, TEE_TIMEOUT_INFINITE, 0, NULL,
+				&sess, &ret_orig);
+	if (res != TEE_SUCCESS) {
+		EMSG("%s: TEE_OpenTASession failed", __func__);


no need for __func__ argument. EMSG() prints the function name (and the line number).
I suggest to print res value. Could be helpful: EMSG("TEE_OpenTASession failed: %#"PRIx32, res);

Ah, I was confused by the other use of EMSG() in the file. Example in test_mem_access_right() line 537:

EMSG("test_mem_access_right: TEE_OpenTASession failed\n");

thus assuming the function name was optional. Fixed.

While at it, I added a patch to the series to fix it: e305c14

etienne-lms · 2023-11-18T23:06:47Z

There's a checkpatch CI test on optee_test P-Rs. We'll ask our dear maintainers to enable it for you P-R and see its result.

Shame on me, I was wrong again, there's no checkpatch run for optee_test CI tests. Obviously not a big issue that said.

etienne-lms

For commit "ta: os_test: return TEE_ERROR_BAD_PARAMETERS for incorrect parameters":
A Reported-by: tag should be followed by a Closes: or a Link: tag. Either add
Link: https://github.com/OP-TEE/optee_test/pull/708#discussion_r1398273025 or remove the Reported-by: tag (i don't think it's really needed here).
With that addressed: Reviewed-by: Etienne Carriere <[email protected]>.

Reviewed-by: Etienne Carriere <[email protected]> for commits
"ta: os_test: do not print caller function name in EMSG()" and
"regression 1016: add a subtest for non-NULL memref of size 0".

etienne-lms · 2023-11-19T09:47:38Z

ta/os_test/os_test.c

-		     "res 0x%x ret_orig 0x%x\n", (unsigned int)res,
-		     (unsigned int)ret_orig);
+		EMSG("TEE_InvokeTACommand: res 0x%x ret_orig 0x%x\n",
+		     (unsigned int)res, (unsigned int)ret_orig);


While you're at it, maybe you can further cleanup these message traces: remove \n terminal character (here and there) and use here PRIx32 specifier instead of "x" for res and ret_orig to avoid the cast.

~~However, since it's also out of the scope of your initial P-R, you can discard my comment, we'll address that with another P-R.~~

(edited: discard this comment, there are so many of such occurrences to address, we can definitely fix them in a dedicated P-R)

Removing the \n is something I will never do manually. Scripts are better for that. Example:

$ echo 'EMSG("test\n")' | sed -r 's/(MSG\(".*)\\n"/\1"/g' EMSG("test")

To apply tree wide:

git ls-files -z | xargs -0 sed -i -r 's/(MSG\(".*)\\n"/\1"/g'

It will miss a few patterns, for example if the string format is on a new line, and it may break the indentation of the newline escape in multi-lines macro. But aside of that, it should be pretty robust.

I you want to apply, please do it on top of this branch, I do not want to rebase (too lazy).

For the PRIx32, I will let someone else take care of it :)

Follow-up here.

I you want to apply, please do it on top of this branch, I do not want to rebase (too lazy).

For the PRIx32, I will let someone else take care of it :)

Never mind, I'll take care. Thanks for the hints, they remind me unix-like shell commands are sometimes pretty or ugly depending on the viewer's eyes.

vincent-mailhol · 2023-11-19T12:56:34Z

For commit "ta: os_test: return TEE_ERROR_BAD_PARAMETERS for incorrect parameters":
A Reported-by: tag should be followed by a Closes: or a Link: tag. Either add
Link: https://github.com/OP-TEE/optee_test/pull/708#discussion_r1398273025 or remove the Reported-by: tag (i don't think it's really needed here).
With that addressed: Reviewed-by: Etienne Carriere <[email protected]>.

You are right, such a rule exists (ref). It is rarely enforced so I actually did not know about it. Well, I learnt something today \o/.

I discarded the Reported-by: tag.

etienne-lms

The series LGTM. Thanks Vincent.
I rely on our dear maintainers whether or not they're ok with commit message for commit "regression 1033: remove trailing space characters".

jenswi-linaro · 2023-11-20T09:07:14Z

ta/os_test/ta_entry.c

@@ -92,6 +92,9 @@ TEE_Result TA_InvokeCommandEntryPoint(void *pSessionContext,
 	case TA_OS_TEST_CMD_TA2TA_MEMREF:
 		return ta_entry_ta2ta_memref(nParamTypes, pParams);

+	case TA_OS_TEST_CMD_TA2TA_MEMREF_SIZE0:
+		return ta_entry_ta2ta_memref_empty(nParamTypes, pParams);


How about naming this function ta_entry_ta2ta_memref_size0() to match the TA command ID?

Suggestion applied in 63786dc. Thanks.

jenswi-linaro · 2023-11-20T11:01:43Z

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

As advised by the Linux kernel coding style [1] which OP-TEE follows: [...] don't leave whitespace at the end of lines. [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#indentation Signed-off-by: Vincent Mailhol <[email protected]> Acked-by: Etienne Carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

TA2TA_BUF_SIZE is used to defined some test arrays. Using a macro instead of a const int declaration is good as it forces the declared arrays not to be variable length arrays. However, TA2TA_BUF_SIZE is defined in the middle of the translation unit. Because its use is local to ta_entry_ta2ta_memref(), undefine it after the function to make the visibility of the macro scoped. Signed-off-by: Vincent Mailhol <[email protected]> Acked-by: Etienne Carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

The TEE_ERROR_BAD_PARAMETERS return code should be prefered over TEE_ERROR_GENERIC for any failed check on the parameters. Signed-off-by: Vincent Mailhol <[email protected]> Reviewed-by: Etienne Carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

EMSG() already prints the caller function name (and the line number). As such, no need to hardcode it. Remove all usage of caller function name in EMSG(). Signed-off-by: Vincent Mailhol <[email protected]> Reviewed-by: Etienne Carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

The Global Platform specification [1] tells us that the session parameter of TEE_CloseTASession() is: An opened session handle The behaviour is unspecified if the session handle is not opened. Make sure not to call TEE_CloseTASession() with an invalid session handle when TEE_OpenTASession() fails by either: - doing an early return - adding an additional cleanup label [1] TEE Internal Core API Specification – Public Release v1.3.1, §4.9.2 "TEE_CloseTASession" Signed-off-by: Vincent Mailhol <[email protected]> Reviewed-by: Etienne Carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

Add a subtest to assert that the implementation allows to forward non-NULL memref from a TA to another TA. Regression 1016 already contains tests related to forwarding memref between TAs. Thus extend this existing test with the subtest described above instead of writing a new one. The Global Platform specification allows this, however, at the time of writing, optee-os will panic. A fix is proposed at [1]. [1] core: tee_svc.c: allow to pass non-NULL memref of size 0 Link: OP-TEE/optee_os#6405 Signed-off-by: Vincent Mailhol <[email protected]> Reviewed-by: Etienne Carriere <[email protected]> Reviewed-by: Jens Wiklander <[email protected]>

vincent-mailhol mentioned this pull request Nov 17, 2023

core: tee_svc.c: allow to pass non-NULL memref of size 0 OP-TEE/optee_os#6405

Merged

etienne-lms reviewed Nov 17, 2023

View reviewed changes

vincent-mailhol force-pushed the ta2ta-memref-size0 branch from 4df2897 to dcefc81 Compare November 17, 2023 17:58

etienne-lms reviewed Nov 18, 2023

View reviewed changes

vincent-mailhol force-pushed the ta2ta-memref-size0 branch from dcefc81 to 96d46f2 Compare November 19, 2023 04:32

etienne-lms reviewed Nov 19, 2023

View reviewed changes

vincent-mailhol force-pushed the ta2ta-memref-size0 branch from 96d46f2 to d4ef7d4 Compare November 19, 2023 12:57

vincent-mailhol mentioned this pull request Nov 19, 2023

No newline in *MSG() macros #709

Merged

etienne-lms approved these changes Nov 19, 2023

View reviewed changes

jenswi-linaro reviewed Nov 20, 2023

View reviewed changes

vincent-mailhol force-pushed the ta2ta-memref-size0 branch from d4ef7d4 to 63786dc Compare November 20, 2023 10:58

vincent-mailhol added 6 commits November 20, 2023 21:19

vincent-mailhol force-pushed the ta2ta-memref-size0 branch from 63786dc to 3aea377 Compare November 20, 2023 12:20

jforissier merged commit da741cd into OP-TEE:master Nov 20, 2023
2 checks passed

vincent-mailhol deleted the ta2ta-memref-size0 branch November 20, 2023 23:58

etienne-lms mentioned this pull request Nov 21, 2023

ta: cleanup trace messages in test TAs #710

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a subtest for non-NULL memref of size 0 + misc. cleanup #708

add a subtest for non-NULL memref of size 0 + misc. cleanup #708

vincent-mailhol commented Nov 17, 2023 •

edited

Loading

etienne-lms left a comment

etienne-lms left a comment

etienne-lms left a comment

vincent-mailhol commented Nov 17, 2023

etienne-lms commented Nov 17, 2023

vincent-mailhol commented Nov 18, 2023

etienne-lms left a comment

etienne-lms Nov 18, 2023

vincent-mailhol Nov 19, 2023

etienne-lms Nov 18, 2023

vincent-mailhol Nov 19, 2023

etienne-lms Nov 18, 2023

vincent-mailhol Nov 19, 2023

etienne-lms Nov 18, 2023

vincent-mailhol Nov 19, 2023

vincent-mailhol Nov 19, 2023

etienne-lms Nov 18, 2023

vincent-mailhol Nov 19, 2023

etienne-lms Nov 18, 2023

vincent-mailhol Nov 19, 2023

vincent-mailhol Nov 19, 2023

etienne-lms commented Nov 18, 2023 •

edited

Loading

etienne-lms left a comment

etienne-lms Nov 19, 2023 •

edited

Loading

vincent-mailhol Nov 19, 2023

vincent-mailhol Nov 19, 2023 •

edited

Loading

etienne-lms Nov 19, 2023

vincent-mailhol commented Nov 19, 2023

etienne-lms left a comment

jenswi-linaro Nov 20, 2023

vincent-mailhol Nov 20, 2023

jenswi-linaro commented Nov 20, 2023

add a subtest for non-NULL memref of size 0 + misc. cleanup #708

add a subtest for non-NULL memref of size 0 + misc. cleanup #708

Conversation

vincent-mailhol commented Nov 17, 2023 • edited Loading

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

vincent-mailhol commented Nov 17, 2023

etienne-lms commented Nov 17, 2023

vincent-mailhol commented Nov 18, 2023

etienne-lms left a 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

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

etienne-lms commented Nov 18, 2023 • edited Loading

etienne-lms left a comment

Choose a reason for hiding this comment

etienne-lms Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincent-mailhol Nov 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincent-mailhol commented Nov 19, 2023

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Nov 20, 2023

vincent-mailhol commented Nov 17, 2023 •

edited

Loading

etienne-lms commented Nov 18, 2023 •

edited

Loading

etienne-lms Nov 19, 2023 •

edited

Loading

vincent-mailhol Nov 19, 2023 •

edited

Loading