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

Support Sendable sessions for use in multithreaded code #120

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maurer
Copy link

@maurer maurer commented Feb 3, 2024

No description provided.

The existing bindings require a `&mut` reference to the context,
indicating that only one session may be alive at a time per context.
However, GlobalPlatform v1.0 says:

However, it is valid for the Client Application to concurrently use
these functions to initialize or finalize different objects; in the
above example two threads could initialize different TEEC_Session
structures.

This indicates both:
1. It should be possible to have two simultaneous live sessions.
2. If synchronization is necessary on the `TEE_Context` object, it is
   the responsibility of the C library to provide that synchronization.

This means that we do not require exclusive access to the `TEE_Context`,
either at creation time or during use of the session, so we can take a
`&` instead.
By default, Sessions contain a borrow into their Context. This is
correct, because the underlying implementation is allowed to have
pointer pointing back at their context.

However, this makes them harder to use from multiple threads, as a
non-static reference is inherently not Send.

This provides `OwnedSession`, behind the `owned` feature, which keeps a
reference counted hold on the context backing the session to prevent it
from being moved or dropped until the session leaves scope. This way,
the `OwnedSession` can be transferred to another thread which does not
know how long the original thread would keep the context around.
@DemesneGH
Copy link
Contributor

The CI has been fixed, please rebase on the latest commit, thanks!

self_cell = { version = "1.0", optional = true }

[features]
owned = ["self_cell"]
Copy link
Contributor

Choose a reason for hiding this comment

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

how about change the feature name to the more accurate name such as owned_session?

/// let uuid = Uuid::parse_str("8abcf200-2450-11e4-abe2-0002a5d5c51b").unwrap();
/// let p0 = ParamValue(42, 0, ParamType::ValueInout);
/// let mut operation = Operation::new(0, p0, ParamNone, ParamNone, ParamNone);
/// let session = ctx.open_session_with_operation(uuid, operation).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

ctx.open_session_with_operation to ctx.open_owned_session_with_operation?

// sessions with 'static lifetime should be Send.
//
// A few ways to construct a session with static lifetime:
// 1. With the "owned" feature, use an `OwnedSession`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a hello-world example for multi-thread session? It would be great if the example can be tested on our CI

@b49020
Copy link
Contributor

b49020 commented Feb 7, 2024

@maurer Can you target these changes for no-std branch instead since that will be the main development branch going forward?

@DemesneGH I hope that's something we agreed upon. We will merge std TAs support there once the custom downstream OP-TEE toolchain target makes it way upstream.

@DemesneGH
Copy link
Contributor

@b49020 For previous users developed on the master(with std) it's okay to open PR upon this branch. If it's ready to be merged we can sync the change to no-std branch then. I think developers should not be tangled in open PRs on which branch, we can handle that.

@b49020
Copy link
Contributor

b49020 commented Feb 8, 2024

@DemesneGH I think the better way to handle this is to rather create separate git repos for optee-utee and optee-teec crates which both std and no-std examples can reuse. All the contributions happens towards those git repos instead, see: #123. It would also allow us to easily publish those crates to crates.io too.

@x1a0b0
Copy link

x1a0b0 commented Oct 10, 2024

Any update to this PR?
It's meaningful for integrate CA to normal rust applictions with multithread support

@DemesneGH
Copy link
Contributor

@x1a0b0 Thanks for your contribution! Since the master branch is outdated, could you rebase on the main branch and open a new PR? Please also see the review comments I left before, thanks!

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