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

Implement TDVF and INIT_VCPU #7

Merged
merged 7 commits into from
Jun 10, 2024

Conversation

jakecorrenti
Copy link
Member

Adds a module that will provide an API for TDVF (TDX Virtual Firmware).

Adds the API to initialize a VCPU for TDX.

@auto-assign auto-assign bot requested a review from tylerfanelli May 23, 2024 18:53
@jakecorrenti jakecorrenti force-pushed the tdvf-and-init-vcpu branch 2 times, most recently from fe25d9d to 9e1a7a1 Compare May 23, 2024 18:57
Copy link
Member

@tylerfanelli tylerfanelli left a comment

Choose a reason for hiding this comment

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

Nice work, I've suggested a few changes.

Side note: There are a lot of functional changes here for this to be only one commit (IMO it can be easily broken into 4+ commits). While its fine for now and you don't need to change, it is good practice to create commits with small modular changes that either introduce new functionality or fix something. This can ease the burden on a reviewer when trying to review code submitted.

src/launch/mod.rs Show resolved Hide resolved
src/launch/mod.rs Outdated Show resolved Hide resolved
src/launch/mod.rs Outdated Show resolved Hide resolved
src/tdvf/mod.rs Outdated Show resolved Hide resolved
src/tdvf/mod.rs Outdated Show resolved Hide resolved
tests/launch.rs Outdated Show resolved Hide resolved
Copy link
Member

@tylerfanelli tylerfanelli left a comment

Choose a reason for hiding this comment

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

One small change, looks good otherwise.

fn try_from(fd: &'a mut kvm_ioctls::VcpuFd) -> Result<Self, Self::Error> {
// need to enable the X2APIC bit for CPUID[0x1] so that the kernel can call
// KVM_SET_MSRS(MSR_IA32_APIC_BASE) without failing
let kvm = Kvm::new()?;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should be re-using the Kvm that we initialized the vCPU with. We should include that in this.

impl<'a> TryFrom<(&'a mut kvm_ioctls::VcpuFd, &'a mut kvm_ioctls::Kvm)> for TdxVcpu<'a>

Copy link
Member Author

Choose a reason for hiding this comment

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

the TryFrom trait only allows one argument from what I can see

Copy link
Member Author

Choose a reason for hiding this comment

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

the TryFrom trait only allows one argument from what I can see

I misread your code snippet, please disregard

Copy link
Member Author

Choose a reason for hiding this comment

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

should be fixed.

Signed-off-by: Jake Correnti <[email protected]>
Copy link
Member

@tylerfanelli tylerfanelli left a comment

Choose a reason for hiding this comment

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

Nice work!

@tylerfanelli tylerfanelli merged commit 81c10e3 into virtee:main Jun 10, 2024
2 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.

2 participants