feat(opentmk): opentmk framework with first testcase #1210
Conversation
feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: opentmk init feat: init 1 feat: init 2 feat: init 1 feat: opentmk feat: opentmk init 3 feat: opentmk init 4 feat: opentmk init 4
mayank-microsoft
force-pushed
the
target-opentmk
branch
from
c3f74c0 to
056bf7d
Compare
| edition.workspace = true | ||
| rust-version.workspace = true | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
nit: sort and switch to dotted syntax for crates that don't need features. Also move all crates to be workspaced.
There was a problem hiding this comment.
@smalis-msft There is one issue where I want to use the serde package with default features off as I want to use a no_std env. Should I disable the feature in workspace?
| /// input/output pages are not being concurrently used elsewhere. For fast | ||
| /// hypercalls, the caller must ensure that there are no output words so that | ||
| /// there is no register corruption. | ||
| #[inline(never)] |
There was a problem hiding this comment.
Right. I need to remove this.
| @@ -0,0 +1,4 @@ | |||
| imports_granularity = "Item" # Expands `use foo::{bar, baz};` into separate `use` lines | |||
There was a problem hiding this comment.
This will need to go away, although if these options are stabilized they could move to our root rustfmt.toml.
There was a problem hiding this comment.
I will remove this before checking in. These options are not stable.
| /// Returns Ok(()) if successful, Err(SendError) if all receivers have been dropped | ||
| pub fn send(&self, value: T) -> Result<(), SendError<T>> { | ||
| // Check if there are any receivers left | ||
| if self.inner.receivers.load(Ordering::SeqCst) == 0 { |
There was a problem hiding this comment.
John would know better than me, but is SeqCst needed for all these atomic operations? It does have some overhead, though I suppose if this is a test-only crate that's not a big deal
There was a problem hiding this comment.
I am currently thinking of it as a test crate we can probably improve on it.
|
|
||
| //! TPM 2.0 Protocol types, as defined in the spec | ||
|
|
||
| //! NOTE: once the `tpm-rs` project matures, this hand-rolled code should be *deleted* and |
There was a problem hiding this comment.
We shouldn't duplicate all of this code. If we can't depend on the tpm crate here we should just move this file to a new tpm_defs or tpm_proto crate that can be shared.
There was a problem hiding this comment.
yeah that makes sense, but we need some changes to work with no std. I want to do that with a lower priority. Lets get the other changes approved and then I can take this change up?
| } | ||
|
|
||
| /// Call before jumping to kernel. | ||
| pub fn uninitialize() { |
There was a problem hiding this comment.
Do we need uninitialize? Do we ever call it and/or jump to an OS kernel?
There was a problem hiding this comment.
Its just for sanity if we do need to uninit the hypercall pages.
|
please add more to the description of the change - rationale, design, etc. |
There was a problem hiding this comment.
I didn't review much of the PR, but i'm missing a lot of design rational and why we have chosen to make the choices we have. Are we adding a writeup on the structure here and what we're doing?
There are many cases where we need to split the unsafe code into smaller chunks as well.
| @@ -0,0 +1,286 @@ | |||
| #![no_std] | |||
There was a problem hiding this comment.
is there really no off-the-shelf crate that does what we want?
There was a problem hiding this comment.
initially we had a requirement where we wanted to peek into the queue (peek and then insert back, so I had implemented the lib) we no longer depend on that. But I am not really aware of any blocking channel implementation for no_std. I am open to changing this if you are aware of something.
| @@ -0,0 +1,286 @@ | |||
| #![no_std] | |||
There was a problem hiding this comment.
missing module header, comments, forbid(unsafe_code)
|
|
||
| /// An unbounded channel implementation with priority send capability. | ||
| /// This implementation works in no_std environments using spin-rs. | ||
| /// It uses a VecDeque as the underlying buffer and provides blocking and non-blocking interfaces. |
There was a problem hiding this comment.
i don't see any indication any methods actually block - this is stale? or what's the actual design here?
| "vm/vmgs/vmgs_lib", | ||
| "vm/vmgs/vmgstool", | ||
| # opentmk | ||
| "opentmk/", |
There was a problem hiding this comment.
what does the trailing slash mean here?
There was a problem hiding this comment.
I'll remove that. It doesn't mean anything.
| windows = "0.59" | ||
| windows-service = "0.7" | ||
| windows-sys = "0.52" | ||
| x86_64 = { version = "0.15.2", default-features = false } |
There was a problem hiding this comment.
what are we getting from this dep that we don't already have in x86defs?
There was a problem hiding this comment.
we use x86_64 heavily for interrupt management. For the IDT structs, helpers and interrupt abi.
| @@ -0,0 +1,5 @@ | |||
| RUST_BACKTRACE=1 CARGO_PROFILE_RELEASE_force_frame_pointers=yes cargo build -p opentmk --target x86_64-unknown-uefi --release #--target-dir ./target/x86_64-unknown-uefi/debug | |||
There was a problem hiding this comment.
This shouldn't live in the repo.
There was a problem hiding this comment.
Right I'll remove this before merging.
| rust-version.workspace = true | ||
|
|
||
| [features] | ||
| default = ["nightly"] |
There was a problem hiding this comment.
don't do this - you will break everyone's rust-analyzer. Are we required to have nightly for this? why?
There was a problem hiding this comment.
Yeah this change got push while locally validating the changes, I'll not push the nightly feature as a dependency for default.
| @@ -0,0 +1,4 @@ | |||
| imports_granularity = "Item" # Expands `use foo::{bar, baz};` into separate `use` lines | |||
There was a problem hiding this comment.
why do we need this here? why are we not using the root repo's one?
There was a problem hiding this comment.
I'll remove this, Its there for aiding the linting. Most of these are not supported on stable.
| cmd: Option<Box<dyn FnOnce(&mut T)>>, | ||
| } | ||
|
|
||
| impl<T> VpExecutor<T> { |
There was a problem hiding this comment.
executor already has a well-defined meaning in the rust ecosystem for async code. Is there another term we can use?
There was a problem hiding this comment.
Sure. I'll work on that.
| @@ -0,0 +1,96 @@ | |||
| use core::alloc::GlobalAlloc; | |||
There was a problem hiding this comment.
It's hard for me to reason why we have our own allocator here instead of either relying on UEFI's, or using an off the shelf crate - why?
|
To be more explicit - I cannot review this code well until we have some documentation in the description about why we are making this change. Help answer the why and what we are doing here, so that I can give better feedback on the overall direction, and what parts need to be scrutinized more. Without it, I can't give useful feedback. I think also this PR is much too large, and we should see if there are ways to split it up, which I can't do without some more context and rationale. |
|
@chris-oo let me work on it and get back to you. I will go over the decisions we have made here and summarise the earlier discussions as well. |
OpenTMK framework for testing guest-based scenarios with a HCL.