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

Add std-only examples and polish doc for main #144

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Conversation

DemesneGH
Copy link
Contributor

  • Add examples: serde, message_passing_interface, tcp_client, udp_socket, tls_client, tls_server
  • Add test scripts and CI for those examples
  • Add migration guide from master branch
  • Polish README in main page


### Step 1: Migrate Old Code to the New Environment

To migrate an example project (e.g., `tls_server-rs` in `examples/`), you

Choose a reason for hiding this comment

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

The current migration strategy seems to ask developers to duplicate the hello-world example and manually copy their source code into the corresponding subdirectories. However, this approach doesn't clearly highlight what has actually changed and what developers need to focus on during the migration.

The target audience for this section is developers currently using the master branch who have their own code repositories. The key point we need to emphasize is that no code changes are required during the migration—only the build scripts need to be updated.

The message we want to convey is simple: developers should be aware that the build process has changed compared to the legacy master branch, and the build scripts will need to be replaced. Could we clearly outline the differences in the build scripts (e.g. Cargo.toml, build.rs, Makefile, etc), step-by-step, and perhaps reference a specific commit for further clarification? This way, developers can focus on adapting their build environment without being concerned about unnecessary code modifications.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO, we should rather be describing the build environment for the main branch as to how it's structured. This will help developers writing new TAs or updating existing TAs to the build environment on main branch.

A few words for each file under following directory structure should be sufficient:

examples/acipher-rs/
├── host
│   ├── Cargo.toml
│   ├── Makefile
│   └── src
│       └── main.rs
├── Makefile
├── proto
│   ├── build.rs
│   ├── Cargo.toml
│   └── src
│       └── lib.rs
├── ta
│   ├── build.rs
│   ├── Cargo.toml
│   ├── Makefile
│   ├── src
│   │   └── main.rs
│   ├── ta_static.rs
│   └── Xargo.toml
└── uuid.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doc updated, thanks for all comments

CROSS_COMPILE_HOST=$(CROSS_COMPILE_HOST)
$(q)make -C ta TARGET_TA=$(TARGET_TA) \
CROSS_COMPILE_TA=$(CROSS_COMPILE_TA)

Choose a reason for hiding this comment

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

I see that at the top level, TARGET_HOST and TARGET_TA are used to differentiate between the build parameters for the host and trusted application (TA), which makes sense. However, at the sub-level (host/Makefile and ta/Makefile), it may be unnecessary to keep differentiating these parameters. The sub-level Makefiles could rely on the variables passed down from the top-level Makefile and use more generic variable names, such as TARGET and CROSS_COMPILE, similar as the @CROSS_COMPILE usage for building ring.

Instead of maintaining the separate TARGET_HOST and TARGET_TA at each level, we could streamline the build process by using:

@make -C host TARGET=$(TARGET_HOST) CROSS_COMPILE=$(CROSS_COMPILE_HOST)
@make -C ta TARGET=$(TARGET_TA) CROSS_COMPILE=$(CROSS_COMPILE_TA)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sub-level Makefiles could rely on the variables passed down from the top-level Makefile and use more generic variable names, such as TARGET and CROSS_COMPILE, similar as the @CROSS_COMPILE usage for building ring.

That makes sense because the CROSS_COMPILE is needed for building inner C libraries for TA. But I have some consideration for independent building.
Keeping the CROSS_COMPILE_{HOST, TA} suffix helps to build CA or TA independently. The scenario is common, like the CA is finished and we're developing TA and willing to rebuild it.

Currently, the CROSS_COMPILE_{HOST, TA} is set by source environment on env setup stage of our SDK (https://github.com/DemesneGH/rust-optee-trustzone-sdk/blob/a84e96d4199289844285d545ac5916fb35f1e9fb/environment) .
After setup we can build only TA for one example: e.g. run make -C examples/hello_world-rs/ta.

If changed to this:

@make -C host TARGET=$(TARGET_HOST) CROSS_COMPILE=$(CROSS_COMPILE_HOST)
@make -C ta TARGET=$(TARGET_TA) CROSS_COMPILE=$(CROSS_COMPILE_TA)

The example/*/ta/Makefile needs to read CROSS_COMPILE set by example/*/Makefile.
If we want to build TA only, we have to build the entire example (using example/*/Makefile) to set correct CROSS_COMPILE, or set the CROSS_COMPILE explicitly.

Choose a reason for hiding this comment

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

make host, make ta should be the recommended way for independent building.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

examples/message_passing_interface-rs/ta/ta_static.rs Outdated Show resolved Hide resolved
- remove wildcard imports: "use core::ffi::*;"
- since "c_size_t" is an unstable feature for current Rustc
  (2024-05-14), replace them with "usize", which is equivalent
  on ARM32 and ARM64.
@DemesneGH DemesneGH force-pushed the main branch 2 times, most recently from d57ad65 to 3ccfadc Compare October 18, 2024 05:44
OUT_DIR := $(CURDIR)/target/$(TARGET)/release


all: host strip
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's a client towards STD TA, there is no value in building it without ${STD} enabled, so we should gate it as well as we do for STD TAs. This holds true for other STD TAs clients as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Updated in new commits, thanks for your comment


#![no_main]
// this is the workaround for the error:
// error[E0658]: use of unstable library feature 'c_size_t'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment redundant now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in new commit

@b49020
Copy link
Contributor

b49020 commented Oct 18, 2024

Apart from few comments above, the overall cleanup looks good to me.

Host and TA read `TARGET` and `CROSS_COMPILE` from the top level
Makefile of the example. That meets the env need of building inner
C libraries such as ring for TA.
Run `make -C examples/xxxx ta` will build TA independently.
@DemesneGH DemesneGH merged commit 7f9c0c2 into apache:main Oct 25, 2024
7 checks 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.

3 participants