You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@teaclave.apache.org by "ivila (via GitHub)" <gi...@apache.org> on 2025/04/14 07:17:39 UTC
[PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
ivila opened a new pull request, #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179
This PR fix the systest of optee-teec, which solves part of #168
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "ivila (via GitHub)" <gi...@apache.org>.
ivila commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046513152
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
I prefer using `unsafe { std::mem::zeroed() }` here. While Rust marks this as unsafe because zero isn't a valid bit pattern for all types, we're only applying it to C structs where a zeroed value is always valid (consistent with how we use `memset` in C).
This also avoids unnecessary changes to the `imp` field. While this field (in `TEEC_OPERATION`, `TEEC_Session`, etc) may change over time, we explicitly shouldn't—and don't need to—care about their internal representation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046540649
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
Let's have a SAFETY mark above this line, the format is:
// SAFETY:
// explaination:xxxx
let mut raw_ctx = unsafe { std::mem::zeroed() };
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046496148
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
Since usage of `unsafe()` would need a explanation when we enable `cargo clippy` in the future. I think we need to unify the unsafe usage.
Will all similar initializations use `unsafe { std::mem::zeroed() }` or just fill in default values?
What's your opinion on it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "ivila (via GitHub)" <gi...@apache.org>.
ivila commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046557447
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
Add for `TEEC_Context` and `TEEC_Session`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046540649
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
Let's have a SAFETY mark above this line, the format is:
// SAFETY:
// some explanation xxxx
let mut raw_ctx = unsafe { std::mem::zeroed() };
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046496148
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
Since usage of `unsafe()` would need a explanation when we enable `cargo clippy` in the future. I think we need to unify the unsafe usage.
Will all similar initializations use `unsafe { std::mem::zeroed() }` or just fill in default values like this original code?
What's your opinion on it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046311740
##########
.github/workflows/ci.yml:
##########
@@ -211,6 +211,28 @@ jobs:
make -j3 toolchains &&
make -j`nproc` RUST_ENABLE=y COMPILE_S_USER=32 check
+ systest:
+ runs-on: ubuntu-24.04-arm
+ container: ghcr.io/ivila/teaclave-trustzone-sdk-ci-runner:ubuntu-24.04
Review Comment:
Seems you're using a new image, does our Dockerfile need to change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046540649
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
Let's have a SAFETY mark above this line, the format is:
// SAFETY:
// some explaination xxxx
let mut raw_ctx = unsafe { std::mem::zeroed() };
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "ivila (via GitHub)" <gi...@apache.org>.
ivila commented on PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#issuecomment-2808788112
> BTW could the ARM64 image also help on #160?
I'm currently working on this (stuck on the QEMU aarch64 memory expansion issue).
You can review my changes here:: https://github.com/ivila/incubator-teaclave-trustzone-sdk/commit/72145f162dc12ef059f21511d2c2136f49d4c62d
The CI is currently failing due to the expanded memory problem: https://github.com/ivila/incubator-teaclave-trustzone-sdk/actions/runs/14465993903
I’ve already fixed this locally after following the documentation you told me here: https://github.com/apache/incubator-teaclave-trustzone-sdk/blob/main/docs/expanding-ta-secure-memory-on-qemuv8.md
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046559685
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#issuecomment-2808766768
BTW could the ARM64 image also help on https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/160?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046496148
##########
optee-teec/src/context.rs:
##########
@@ -57,11 +57,7 @@ impl Context {
/// ```
pub fn new() -> Result<Context> {
// define an empty TEEC_Context
- let mut raw_ctx = raw::TEEC_Context {
- fd: 0,
- reg_mem: false,
- memref_null: false,
- };
+ let mut raw_ctx = unsafe { std::mem::zeroed() };
Review Comment:
Since usage of `unsafe{}` would need a explanation of the safety assumption when we enable `cargo clippy` in the future. I think we need to unify the unsafe usage.
Will all similar initializations use `unsafe { std::mem::zeroed() }` or just fill in default values like this original code?
What's your opinion on it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "ivila (via GitHub)" <gi...@apache.org>.
ivila merged PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "ivila (via GitHub)" <gi...@apache.org>.
ivila commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046319991
##########
.github/workflows/ci.yml:
##########
@@ -211,6 +211,28 @@ jobs:
make -j3 toolchains &&
make -j`nproc` RUST_ENABLE=y COMPILE_S_USER=32 check
+ systest:
+ runs-on: ubuntu-24.04-arm
+ container: ghcr.io/ivila/teaclave-trustzone-sdk-ci-runner:ubuntu-24.04
Review Comment:
> Seems you're using a new image, does our Dockerfile need to change?
No, it uses the same Dockerfile, but this one is built for `linux/arm64`.
It seems that `yuanz0/teaclave-trustzone-sdk:ubuntu-24.04` only built for platform `linux/amd64`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on code in PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#discussion_r2046348812
##########
.github/workflows/ci.yml:
##########
@@ -211,6 +211,28 @@ jobs:
make -j3 toolchains &&
make -j`nproc` RUST_ENABLE=y COMPILE_S_USER=32 check
+ systest:
+ runs-on: ubuntu-24.04-arm
+ container: ghcr.io/ivila/teaclave-trustzone-sdk-ci-runner:ubuntu-24.04
Review Comment:
Got it. Could you help to add some comments on the CI tasks for clarification? Such as:
```
# Cross-compile on AMD64 and run tests in QEMU
build-and-test-examples:
# Run systest of optee_teec and build systest of optee_utee on ARM64
systest:
```
It would help us avoid confusion in the future, as we might forget the differences.
##########
optee-utee/optee-utee-sys/src/tee_api.rs:
##########
@@ -241,7 +238,7 @@ extern "C" {
pub fn TEE_TruncateObjectData(object: TEE_ObjectHandle, size: usize) -> TEE_Result;
pub fn TEE_SeekObjectData(
object: TEE_ObjectHandle,
- offset: i32, //intmax_t
+ offset: intmax_t, //intmax_t
Review Comment:
Remove the redundant comment `//intmax_t`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org
Re: [PR] Fix teec ctest zc [incubator-teaclave-trustzone-sdk]
Posted by "DemesneGH (via GitHub)" <gi...@apache.org>.
DemesneGH commented on PR #179:
URL: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/179#issuecomment-2808758265
After the tiny comments resolved please feel free to add:
`Reviewed-by: Yuan Zhuang <yu...@apache.org>`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@teaclave.apache.org
For additional commands, e-mail: dev-help@teaclave.apache.org