-
Notifications
You must be signed in to change notification settings - Fork 106
[RTE-709] amd-sev runner + cargo runner #863
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
base: thomasvs/RTE-710-elf2uki
Are you sure you want to change the base?
Conversation
3aff654 to
eeaaa9d
Compare
| } | ||
|
|
||
| impl FortanixVmeConfig { | ||
| const DEFAULT_CPU_COUNT: isize = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: defaults removed here because I set them in the runner. Made more sense to me that way, and avoids Options or duplicate defaults.
1410dea to
346730d
Compare
8ee6765 to
0b285d0
Compare
346730d to
eb3021c
Compare
eb3021c to
31707e4
Compare
| let memory_size = format!("{}M", memory_mib); | ||
|
|
||
| // TODO (RTE-740): id-block | ||
| let mut command = Command::new("sudo"); // TODO: look at "sudo" - needed for amd + `/dev/sev`? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when running in stimulator, we should not need sudo right ?
| firmware_image_path: Option<PathBuf>, | ||
|
|
||
| /// Name for the enclave in the runner | ||
| #[arg(long, default_value = "FortanixVm")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could we make the default name more informative by including AmdSev?
| pub fn help() -> String { | ||
| String::from("Usage: <ftxvme-runner-cargo> [--simulate] [--verbose] <elf_path> [others]*") | ||
| impl CargoArgs { | ||
| pub fn eif_path(&self) -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo?
eif -> elf
| } else { | ||
| Ok(cli) | ||
| } | ||
| pub fn uki_path(&self) -> PathBuf { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this function code is identical to eif_path
| }; | ||
| let uki_path = amd_sev_snp_args.uki_path(); | ||
|
|
||
| // TODO: we can assume this is installed right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we can add code here to check if this tool exist, if not pop up error msg and guide user the way to install it
| let mut ftxvme_elf2uki = Command::new("ftxvme-elf2uki"); | ||
| ftxvme_elf2uki | ||
| .arg("--app") | ||
| .arg(&amd_sev_snp_args.elf_path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I mentioned above, with code above, amd_sev_snp_args.elf_path will be same to uki_path
are you intential to just inplace generate the uki?
1ca2fbc to
087721d
Compare
| let uki_path = amd_sev_snp_args.uki_path(); | ||
|
|
||
| // TODO: we can assume this is installed right? | ||
| let mut ftxvme_elf2uki = Command::new("ftxvme-elf2uki"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, i missed that but, we're not usigx ftx* prefix anymore, so this should be fortanix-vme-elf2uki.
| } | ||
| } | ||
|
|
||
| fn run_to_completion<P: Platform + 'static>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, it would be nice if run_to_completion & create_runner functions (maybe everything with no relation to Cli related types) move to fortanix-vme-runner crate (aka lib). They will be needed in common runner in the mono repo.
| /// https://docs.aws.amazon.com/enclaves/latest/user/cmd-nitro-run-enclave.html#cmd-nitro-run-enclave-options | ||
| /// https://docs.aws.amazon.com/enclaves/latest/user/cmd-nitro-build-enclave.html#cmd-nitro-build-enclave-options | ||
| struct FortanixVmeConfig { | ||
| // TODO: unused - remove this? Then we don't need to make `FortanixVmeConfig` generic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be passed to fortanix-vme-runner. It is used by nitro, we may discard it for amd sev/snp.
No description provided.