libteec: Move OP-TEE defined fields into an imp struct by JLLinaro · Pull Request #349 · OP-TEE/optee_client

JLLinaro · 2023-07-14T09:39:39Z

GlobalPlatform TEE Client API Specification v1.0 specifies that the structs TEEC_Context, TEEC_Session, TEEC_SharedMemory, and TEEC_Operation shall have a user defined struct named imp. In OP-TEE the struct is not there and instead the user defined fields are declared directly in the top structs.
This commit introduces the imp struct to better support using different implementations. The imp fields now represent the implementation defined parts of the structs that was previously declared directly in the top struct. All previously available parameters are preserved in the imp struct.

Link: #348
Reported-by: Tom Hebb tommyhebb@gmail.com

JLLinaro · 2023-07-14T10:22:05Z

The automatic tests fail but that has been solved in the following pull request OP-TEE/optee_test#684

jforissier · 2023-07-17T11:46:02Z

This LGTM, but as mentioned in #348, it changes the layout of TEEC_SharedMemory and therefore is an ABI breakage. So the libteec version should be bumped from 1.0.0 to 2.0.0.

github-actions · 2023-08-17T01:17:21Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

github-actions · 2023-09-21T01:20:41Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

jenswi-linaro · 2023-09-26T07:56:22Z

@JLLinaro would you mind updating this PR so we can merge it?

JLLinaro · 2023-09-29T07:07:26Z

@jenswi-linaro version is now updated to 2.0.0

jforissier · 2023-09-29T07:30:52Z

We need MAJOR_VERSION := 2 in libteec/Makefile, too.

etienne-lms

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> for commit
"libteec: Move OP-TEE defined fields into an imp struct"
(with indentation issue fixed)

etienne-lms · 2023-09-29T07:49:36Z

+			bool dummy;
+			uint8_t flags;
+		} internal;
+    } imp;


fix indentation

jforissier · 2023-10-27T12:41:29Z

Please squash the second commit into the first one (and keep subject "libteec: Move OP-TEE defined fields into an imp struct").
With that:
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2023-10-27T13:38:09Z

There is a conflict, would you please rebase onto master, and also add the Acked-by: tags from Etienne and me?

jenswi-linaro · 2023-11-02T09:39:18Z

 	/* Implementation defined */
-	TEEC_Context *ctx;
-	uint32_t session_id;
+	struct{


Please add a space ( ), between struct and {.

@JLLinaro comment not addressed

jforissier · 2023-11-27T10:27:58Z

Ping @JLLinaro

vincent-mailhol · 2023-11-29T07:51:00Z

+		union {
+			bool dummy;
+			uint8_t flags;
+		} internal;


Is the dummy field still needed following this change? dummy was introduced in 4f3d4cb not to break the ABI. Here we are breaking it anyway, so while at it, I suggest some clean up:

Suggested change

union {

bool dummy;

uint8_t flags;

} internal;

uint32_t flags;

N.B.: If there is a wish to keep more flag bits for future use, then just declare flags as uint32_t (the structure will be padded anyway, so you will not make it any bigger).

I agree. uint32_t flags; LGTM.

@JLLinaro comment not addressed

github-actions · 2023-12-30T01:27:55Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

jforissier · 2024-01-09T08:38:38Z

@JLLinaro any chance this could make it into the next release? (-rc1 is in 3 days).

On the other hand, I am now wondering if this is a good idea. Who cares about this? Sure it is in the spec, but what difference does it make? Apps are not supposed to manipulate the imp field anyways. And changing it now will have impacts on downstream users (version change).

jenswi-linaro · 2024-01-10T07:18:19Z

The answer to your question is in #348

jforissier · 2024-01-10T07:44:46Z

The answer to your question is in #348

Ah, yes. It would be nice to mention in the commit description that the imp struct makes it easier to create a binding for Rust.

github-actions · 2024-02-10T01:18:20Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

jenswi-linaro · 2024-02-12T14:22:40Z

I think we need to explain to @JLLinaro that it's going to cost him a beer each time github reminds us that it's not done yet. :-)

github-actions · 2024-03-14T01:20:17Z

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

jforissier · 2024-03-28T11:41:03Z

+		union {
+			uint32_t flags;
+		} internal;


The union should not be needed, just make this uint32_t flags;.

GlobalPlatform TEE Client API Specification v1.0 specifies that the structs TEEC_Context, TEEC_Session, TEEC_SharedMemory, and TEEC_Operation shall have a user defined struct named imp. In OP-TEE the struct is not there and instead the user defined fields are declared directly in the top structs. This commit introduces the imp struct to better support using different implementations. The imp fields now represent the implementation defined parts of the structs that was previously declared directly in the top struct. All previously available parameters are preserved in the imp struct. The updated version of the imp structure makes it easier to create a binding for Rust. Adding the missing imp struct to the structs in OP-TEE is an ABI breakage which requires a version major update of libteec as explained by Jerome Forissier. Link: OP-TEE#348 Reported-by: Tom Hebb <tommyhebb@gmail.com> Signed-off-by: Julianus Larson <julianus.larson@linaro.org> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org> Fixed spacing Signed-off-by: Julianus Larson <julianus.larson@linaro.org>

jforissier · 2024-04-02T08:32:21Z

Merged manually (with commit message slightly edited).

JLLinaro mentioned this pull request Jul 14, 2023

regression_1025: use the imp field in TEEC_Context OP-TEE/optee_test#684

Closed

github-actions Bot added the Stale label Aug 17, 2023

jforissier removed the Stale label Aug 21, 2023

github-actions Bot added the Stale label Sep 21, 2023

github-actions Bot closed this Sep 26, 2023

jenswi-linaro reopened this Sep 26, 2023

jenswi-linaro removed the Stale label Sep 26, 2023

etienne-lms reviewed Sep 29, 2023

View reviewed changes

JLLinaro force-pushed the fix_imp_part_of_types_in_tee_client_api branch from dbe8282 to c004dbb Compare October 27, 2023 13:16

JLLinaro force-pushed the fix_imp_part_of_types_in_tee_client_api branch from c004dbb to b3c0606 Compare October 27, 2023 14:09

jenswi-linaro reviewed Nov 2, 2023

View reviewed changes

vincent-mailhol reviewed Nov 29, 2023

View reviewed changes

github-actions Bot added the Stale label Dec 30, 2023

github-actions Bot closed this Jan 4, 2024

jenswi-linaro reopened this Jan 4, 2024

github-actions Bot removed the Stale label Jan 10, 2024

github-actions Bot added the Stale label Feb 10, 2024

jenswi-linaro removed the Stale label Feb 12, 2024

github-actions Bot added the Stale label Mar 14, 2024

github-actions Bot closed this Mar 19, 2024

jforissier reopened this Mar 28, 2024

JLLinaro force-pushed the fix_imp_part_of_types_in_tee_client_api branch 3 times, most recently from 6e9f710 to 7d2763f Compare March 28, 2024 10:58

jforissier reviewed Mar 28, 2024

View reviewed changes

JLLinaro force-pushed the fix_imp_part_of_types_in_tee_client_api branch from 7d2763f to ea8d960 Compare March 28, 2024 13:47

jforissier mentioned this pull request Mar 28, 2024

Updates for the new 'imp' field in TEEC_Session and TEEC_Context OP-TEE/optee_test#736

Merged

jforissier closed this Apr 2, 2024

jenswi-linaro mentioned this pull request Sep 6, 2024

Type definitions in tee_client_api.h violate the GlobalPlatform TEE Client API spec #348

Closed

Conversation

JLLinaro commented Jul 14, 2023

Uh oh!

JLLinaro commented Jul 14, 2023

Uh oh!

jforissier commented Jul 17, 2023

Uh oh!

github-actions Bot commented Aug 17, 2023

Uh oh!

github-actions Bot commented Sep 21, 2023

Uh oh!

jenswi-linaro commented Sep 26, 2023

Uh oh!

JLLinaro commented Sep 29, 2023

Uh oh!

jforissier commented Sep 29, 2023

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

etienne-lms Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

jforissier commented Oct 27, 2023

Uh oh!

jforissier commented Oct 27, 2023

Uh oh!

jenswi-linaro Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

jforissier Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

jforissier commented Nov 27, 2023

Uh oh!

vincent-mailhol Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jforissier Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

jforissier Mar 28, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Dec 30, 2023

Uh oh!

jforissier commented Jan 9, 2024

Uh oh!

jenswi-linaro commented Jan 10, 2024

Uh oh!

jforissier commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Feb 10, 2024

Uh oh!

jenswi-linaro commented Feb 12, 2024

Uh oh!

github-actions Bot commented Mar 14, 2024

Uh oh!

jforissier Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jforissier commented Apr 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vincent-mailhol Nov 29, 2023 •

edited

Loading

jforissier commented Jan 10, 2024 •

edited

Loading

jforissier Mar 28, 2024 •

edited

Loading