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

Create a skeleton UEFI app #2605

Merged
merged 14 commits into from
Mar 21, 2022
Merged

Create a skeleton UEFI app #2605

merged 14 commits into from
Mar 21, 2022

Conversation

andrisaar
Copy link
Collaborator

@andrisaar andrisaar commented Mar 10, 2022

This adds a skeleton UEFI "hello world" The whole goal is to have something that we can start integrating into our build/CI systems.

Case in point, for now experimental/uefi needs to be excluded in the root Cargo.toml because commands that execute from the root directory (eg cargo udeps) fail. This is because Cargo will only read the .cargo/config.toml file from the current durectory, but right now the only way to set all of it up properly is to use a .cargo/config.toml under experimental/uefi as we need some experimental features. This means that if you run, say, cargo doc from experimental/uefi it will succeed, but running it from the root directory will fail.

There is hope that the per-package-target Cargo feature will (eventually) help us, but we'll need to wait for rust-lang/cargo#9451 (or something equivalent) to be submitted before we can start using it.

@andrisaar andrisaar force-pushed the uefi branch 2 times, most recently from b69dcb9 to baf8fe1 Compare March 10, 2022 17:28
@andrisaar andrisaar changed the title Create a skeleton for a UEFI app Create a skeleton UEFI app Mar 10, 2022
@andrisaar andrisaar marked this pull request as ready for review March 15, 2022 12:19
Copy link
Collaborator

@tiziano88 tiziano88 left a comment

Choose a reason for hiding this comment

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

Thanks Andri, cool stuff!

use uefi::{prelude::*, table::runtime::ResetType, ResultExt};

#[entry]
fn _start(_handle: Handle, mut system_table: SystemTable<Boot>) -> Status {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to be named _start or is it just a convention? Either way, could you link to some reference material?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just a convention. I've added some comments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, thanks! This stuff is pretty new for everyone, including people familiar with "vanilla" Rust, so expect to have to over communicate / document things :)

main(_handle, &mut system_table)
};

system_table
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some comment here to explain what's happening (and why)?

#![feature(abi_efiapi)]
#![feature(custom_test_frameworks)]
#![test_runner(crate::test_runner)]
#![reexport_test_harness_main = "test_main"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does the test_main string come from? Is it important that it stays like that, or can it be changed? Please add some details

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I assume it can be changed; I've added a link to the blog post this code draws upon heavily for the testing infrastructure.


let status = writeln!(system_table.stdout(), "Hello World!");

match status {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be expressed more "functionally"? e.g. status.map(|_| SUCCESS).or(DEVICE_ERROR) or something like that

Copy link
Collaborator Author

@andrisaar andrisaar Mar 16, 2022

Choose a reason for hiding this comment

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

I could do status.map_or_else(|_| DEVICE_ERROR, |_| SUCCESS) but quite frankly I think that's more opaque and hard to read than that simple match.

Ideally you'd have something like Status::from(status), but it doesn't look that trait is implemented by default. The opposite (turning a Status into a Result) works, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, map_or_else is quite unreadable unless you know exactly which way around the args are.

I think the version of what I suggested that actually works would be status.map(|_| SUCCESS).unwrap_or(ERROR) (which I personally think reads like idiomatic Rust).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's much better. Done.

Comment on lines +26 to +34
#[entry]
fn _start(_handle: Handle, mut system_table: SystemTable<Boot>) -> Status {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I imagine some of this will end up being part of some generic harness library, while some other parts will be Oak-specific. Not in this PR, but it may be worth start thinking about that split going forward.

xtask/src/main.rs Show resolved Hide resolved
use uefi::{prelude::*, table::runtime::ResetType, ResultExt};

#[entry]
fn _start(_handle: Handle, mut system_table: SystemTable<Boot>) -> Status {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great, thanks! This stuff is pretty new for everyone, including people familiar with "vanilla" Rust, so expect to have to over communicate / document things :)

Copy link
Collaborator

@conradgrobler conradgrobler left a comment

Choose a reason for hiding this comment

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

Nice, thanks.

@andrisaar andrisaar merged commit cc7a6b3 into project-oak:main Mar 21, 2022
@github-actions
Copy link

Reproducibility Index:

70f60debbe969656ee5486c1bfecad28c2eedcf45a0546b03c8db73a641b261a  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
2619b250a929ecc3fda9d4c1f761b7a151e5bc892426d0ff70024512a1c2e86b  ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe

Reproducibility Index diff:

@jul-sh
Copy link
Contributor

jul-sh commented Mar 21, 2022

nice, was interesting read :)

@jul-sh jul-sh self-requested a review March 21, 2022 14:18
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