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.

Sign up for GitHub

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

TODO TDX comments were removed #538

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

TODO TDX comments were removed #538

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
156fd90
TODO TDX comments were removed and corresponding
ckotamra Dec 18, 2024
868819f
Merge remote-tracking branch 'upstream/main' into remove-todotdx-comm…
ckotamra Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions openhcl/hcl/src/ioctl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2783,7 +2783,6 @@ impl Hcl {
IsolationType::Snp => hvdef::HvRegisterVsmCapabilities::new()
.with_deny_lower_vtl_startup(caps.deny_lower_vtl_startup())
.with_intercept_page_available(caps.intercept_page_available()),
// TODO TDX: Figure out what these values should be.
IsolationType::Tdx => hvdef::HvRegisterVsmCapabilities::new()
.with_deny_lower_vtl_startup(caps.deny_lower_vtl_startup())
.with_intercept_page_available(caps.intercept_page_available()),
Expand Down Expand Up @@ -2884,7 +2883,6 @@ impl Hcl {
target_vtl: HvInputVtl,
) -> Result<(), ApplyVtlProtectionsError> {
if self.isolation.is_hardware_isolated() {
// TODO SNP TODO TDX - required for vmbus relay monitor page support
todo!();
}

Expand Down
2 changes: 0 additions & 2 deletions openhcl/openhcl_boot/src/arch/x86_64/tdx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,6 @@ pub fn get_tdx_tsc_reftime() -> Option<u64> {
// This is first called by the BSP from openhcl_boot and the frequency
// is saved in this gloabal variable. Subsequent calls use the global variable.
if TSC_FREQUENCY.get() == 0 {
// TODO TDX: Getting tsc frequency from HV currently. Explore the option
// of getting it from more reliable source such as CPUID.
TSC_FREQUENCY.set(read_msr_tdcall(hvdef::HV_X64_MSR_TSC_FREQUENCY));
}

Expand Down
8 changes: 0 additions & 8 deletions openhcl/openhcl_boot/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,14 +790,6 @@ fn validate_vp_hw_ids(partition_info: &PartitionInfo) {
use hypercall::HwId;

if partition_info.isolation.is_hardware_isolated() {
// TODO TDX SNP: we don't have a GHCB/GHCI page set up to communicate
// with the hypervisor here, so we can't easily perform the check. Since
// there is no security impact to this check, we can skip it for now; if
// the VM fails to boot, then this is due to a host contract violation.
//
// For TDX, we could use ENUM TOPOLOGY to validate that the TD VCPU
// indexes correspond to the APIC IDs in the right order. I am not
// certain if there are places where we depend on this mapping today.
return;
}

Expand Down
6 changes: 0 additions & 6 deletions openhcl/underhill_mem/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1139,12 +1139,6 @@ mod mapping {
}
}
IsolationType::Tdx => {
// TODO TDX GUEST VSM: implement acceptor.vtl_permissions
// For now, since guest vsm isn't enabled (therefore no VTL
// 1), and VTL 0 can't change its own permissions, the
// permissions should be the same as when VTL 2 initialized
// guest memory.

GpaVtlPermissions::new(IsolationType::Tdx, vtl, HV_MAP_GPA_PERMISSIONS_ALL)
}
};
Expand Down
2 changes: 0 additions & 2 deletions openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ trait CpuidArchInitializer {
/// Processes extended state enumeration subleaves 2+. result is a helper
/// for retrieving the result of a given subleaf.
//
// (TODO TDX: This will be to populate them, will need to update the
// signature to pass CpuidResults as a mutable reference)
fn process_extended_state_subleaves(
&self,
results: &mut CpuidSubtable,
Expand Down
13 changes: 1 addition & 12 deletions openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ pub const TDX_REQUIRED_LEAVES: &[(CpuidFunction, Option<u32>)] = &[
(CpuidFunction::TileInformation, Some(0)),
(CpuidFunction::TileInformation, Some(1)),
(CpuidFunction::TmulInformation, Some(0)),
// TODO TDX: The following aren't required from AMD. Need to double-check if
// they're required for TDX
(CpuidFunction::CacheAndTlbInformation, None),
(CpuidFunction::ExtendedFeatures, Some(1)),
(CpuidFunction::CacheParameters, Some(0)),
Expand Down Expand Up @@ -51,7 +49,6 @@ impl CpuidArchInitializer for TdxCpuidInitializer {
}

fn extended_max_function(&self) -> u32 {
// TODO TDX: Check if this is the same value in the OS repo
CpuidFunction::ExtendedIntelMaximum.0
}

Expand Down Expand Up @@ -111,7 +108,6 @@ impl CpuidArchInitializer for TdxCpuidInitializer {
}
CpuidFunction::TmulInformation => {
if subleaf == 0 {
// TODO TDX: does this actually have subleaves? the spec says 1+ are reserved
Some(CpuidResultMask::new(
0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, true,
))
Expand Down Expand Up @@ -164,7 +160,6 @@ impl CpuidArchInitializer for TdxCpuidInitializer {
results: &mut CpuidSubtable,
extended_state_mask: u64,
) -> Result<(), CpuidResultsError> {
// TODO TDX: See HvlpPopulateExtendedStateCpuid
let xfd_supported = if let Some(support) = results.get(&1).map(
|CpuidResult {
eax,
Expand All @@ -187,9 +182,7 @@ impl CpuidArchInitializer for TdxCpuidInitializer {
if (1 << i) & summary_mask != 0 {
let result = Self::cpuid(CpuidFunction::ExtendedStateEnumeration.0, i);
let result_xfd = cpuid::ExtendedStateEnumerationSubleafNEcx::from(result.ecx).xfd();
if xfd_supported && result_xfd {
// TODO TDX: update some maximum xfd value; see HvlpMaximumXfd
}
if xfd_supported && result_xfd {}

results.insert(i, result);
}
Expand All @@ -205,8 +198,6 @@ impl CpuidArchInitializer for TdxCpuidInitializer {
_address_space_sizes_ecx: cpuid::ExtendedAddressSpaceSizesEcx,
_processor_topology_ebx: Option<cpuid::ProcessorTopologyDefinitionEbx>, // Will be None for Intel
) -> Result<super::ExtendedTopologyResult, CpuidResultsError> {
// TODO TDX: see HvlpInitializeCpuidTopologyIntel
// TODO TDX: fix returned errors
let vps_per_socket;
if !version_and_features_edx.mt_per_socket() {
if version_and_features_ebx.lps_per_package() > 1 {
Expand All @@ -220,8 +211,6 @@ impl CpuidArchInitializer for TdxCpuidInitializer {
vps_per_socket = version_and_features_ebx.lps_per_package();
}

// TODO TDX: validation of leaf 0xB

Ok(super::ExtendedTopologyResult {
subleaf0: None,
subleaf1: None,
Expand Down
9 changes: 1 addition & 8 deletions openhcl/virt_mshv_vtl/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,9 +819,6 @@ impl virt::Synic for UhPartition {
}

fn monitor_support(&self) -> Option<&dyn virt::SynicMonitor> {
// TODO TDX TODO SNP: Disable monitor support for TDX and SNP as support
// for VTL2 protections is needed to emulate this page, which is not
// implemented yet.
if self.inner.isolation.is_hardware_isolated() {
None
} else {
Expand Down Expand Up @@ -1343,10 +1340,6 @@ impl<'a> UhProtoPartition<'a> {
let is_hardware_isolated = isolation.is_hardware_isolated();

// Intercept Debug Exceptions
// TODO TDX: This currently works on TDX because all Underhill TDs today
// have the debug policy bit set, allowing the hypervisor to install the
// intercept on behalf of the guest. In the future, Underhill should
// register for these intercepts itself.
if params.intercept_debug_exceptions {
if !cfg!(feature = "gdb") {
return Err(Error::InvalidDebugConfiguration);
Expand Down Expand Up @@ -1698,7 +1691,7 @@ impl UhProtoPartition<'_> {
match params.isolation {
IsolationType::None | IsolationType::Vbs => {}
#[cfg(guest_arch = "x86_64")]
IsolationType::Tdx => return false, // TODO TDX GUEST_VSM
IsolationType::Tdx => return false,
#[cfg(guest_arch = "x86_64")]
IsolationType::Snp => {
if !params.env_cvm_guest_vsm {
Expand Down
1 change: 0 additions & 1 deletion openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,6 @@ impl<T, B: HardwareIsolatedBacking> UhHypercallHandler<'_, '_, T, B> {
}

// TODO GUEST VSM: interrupt rewinding
// TODO TDX GUEST VSM: update execution mode
}
}

Expand Down
69 changes: 7 additions & 62 deletions openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,6 @@ impl HardwareIsolatedBacking for TdxBacked {
this: &UhProcessor<'_, Self>,
vtl: GuestVtl,
) -> TranslationRegisters {
// TODO TDX GUEST VSM: use vtl for all registers
let cr0 = this.backing.cr0.read(&this.runner);
let cr4 = this.backing.cr4.read(&this.runner);
let efer = this.backing.efer;
Expand Down Expand Up @@ -538,9 +537,6 @@ impl BackingPrivate for TdxBacked {
params: super::private::BackingParams<'_, '_, Self>,
_shared: &TdxBackedShared,
) -> Result<Self, crate::Error> {
// TODO TDX: TDX shares the vp context page for xmm registers only. It
// should probably move to its own page.
//
// FX regs and XMM registers are zero-initialized by the kernel. Set
// them to the arch default.
*params.runner.fx_state_mut() =
Expand All @@ -565,14 +561,6 @@ impl BackingPrivate for TdxBacked {
*rflags = regs.rflags;
*rip = regs.rip;

// TODO TDX: ssp is for shadow stack

// TODO TDX: direct overlay like snp?
// TODO TDX: lapic / APIC setup?

// TODO TDX: see ValInitializeVplc
// TODO TDX: XCR_XFMEM setup?

// Configure L2 controls to permit shared memory.
//
// Ideally we would disable this when `hide_isolation` is set, but
Expand Down Expand Up @@ -621,8 +609,6 @@ impl BackingPrivate for TdxBacked {

// Allowed cr4 bits depend on the values allowed by the SEAM.
//
// TODO TDX: Consider just using MSR kernel module instead of explicit
// ioctl.
let read_cr4 = hcl.read_vmx_cr4_fixed1();
let allowed_cr4_bits = (ShadowedRegister::Cr4.guest_owned_mask() | X64_CR4_MCE) & read_cr4;

Expand Down Expand Up @@ -677,7 +663,6 @@ impl BackingPrivate for TdxBacked {
)
.into();

// TODO TDX: This needs to come from a private pool
let flush_page = params
.partition
.shared_vis_pages_pool
Expand Down Expand Up @@ -787,7 +772,6 @@ impl BackingPrivate for TdxBacked {
this.run_vp_tdx(dev).await
}

// TODO TDX GUEST VSM
fn poll_apic(
this: &mut UhProcessor<'_, Self>,
_vtl: GuestVtl,
Expand Down Expand Up @@ -828,7 +812,6 @@ impl BackingPrivate for TdxBacked {
this: &mut UhProcessor<'_, Self>,
_dev: &impl CpuIo,
) -> Result<bool, UhRunVpError> {
// TODO TDX GUEST VSM
this.hcvm_handle_cross_vtl_interrupts(|_this, _vtl, _check_rflags| false)
}

Expand Down Expand Up @@ -856,7 +839,6 @@ impl UhProcessor<'_, TdxBacked> {
// Check for interrupt requests from the host.
let mut update_rvi = false;
if let Some(irr) = self.runner.proxy_irr() {
// TODO TDX: filter proxy IRRs.
if self.backing.cvm.lapics[GuestVtl::Vtl0]
.lapic
.can_offload_irr()
Expand Down Expand Up @@ -1332,11 +1314,6 @@ impl UhProcessor<'_, TdxBacked> {
// otherwise the interrupt will be lost and the guest left in a bad
// state.
//
// TODO TDX: Unclear what kind of exits these would be, but they
// should be spurious EPT exits. Can we validate or assert that
// somehow? If we were to somehow call some other path which would
// set interruption_information before we inject this one, we would
// lose this interrupt.
if next_interruption.valid() {
tracing::debug!(
?next_interruption,
Expand Down Expand Up @@ -1498,7 +1475,6 @@ impl UhProcessor<'_, TdxBacked> {
let subleaf = enter_state.rcx() as u32;
let xfem = self
.runner
// TODO TDX GUEST VSM
.get_vp_register(GuestVtl::Vtl0, HvX64RegisterName::Xfem)
.map_err(|err| VpHaltReason::Hypervisor(UhRunVpError::EmulationState(err)))?
.as_u64();
Expand Down Expand Up @@ -1610,7 +1586,6 @@ impl UhProcessor<'_, TdxBacked> {
})
{
self.runner
// TODO TDX GUEST VSM
.set_vp_register(GuestVtl::Vtl0, HvX64RegisterName::Xfem, value.into())
.map_err(|err| {
VpHaltReason::Hypervisor(UhRunVpError::EmulationState(err))
Expand All @@ -1634,20 +1609,6 @@ impl UhProcessor<'_, TdxBacked> {
&mut self.backing.exit_stats.wbinvd
}
VmxExit::EPT_VIOLATION => {
// TODO TDX: If this is an access to a shared gpa, we need to
// check the intercept page to see if this is a real exit or
// spurious. This exit is only real if the hypervisor has
// delivered an intercept message for this GPA.
//
// However, at this point the kernel has cleared that
// information so some kind of redesign is required to figure
// this out.
//
// For now, we instead treat EPTs on readable RAM as spurious
// and log appropriately. This check is also not entirely
// sufficient, as it may be a write access where the page is
// protected, but we don't yet support MNF/guest VSM so this is
// okay enough.
let is_readable_ram =
self.partition.gm[intercepted_vtl].check_gpa_readable(exit_info.gpa());
if is_readable_ram {
Expand Down Expand Up @@ -1880,7 +1841,6 @@ impl UhProcessor<'_, TdxBacked> {
// so that the hypervisor can directly inject events.
if matches!(msr, hvdef::HV_X64_MSR_SINT0..=hvdef::HV_X64_MSR_SINT15) {
if let Err(err) = self.runner.set_vp_register(
// TODO TDX GUEST VSM
GuestVtl::Vtl0,
HvX64RegisterName(
HvX64RegisterName::Sint0.0 + (msr - hvdef::HV_X64_MSR_SINT0),
Expand All @@ -1900,14 +1860,7 @@ impl UhProcessor<'_, TdxBacked> {
}

fn read_msr_cvm(&self, msr: u32, vtl: GuestVtl) -> Result<u64, MsrError> {
// TODO TDX: port remaining tdx and common values
//
// TODO TDX: consider if this can be shared with SnpBacked's
// implementation. For the most part other than Intel/TDX specific
// registers, MSR handling should be the same.

match msr {
// TODO TDX: LIFTED FROM WHP
x86defs::X86X_IA32_MSR_PLATFORM_ID => {
// Windows requires accessing this to boot. WHP
// used to pass this through to the hardware,
Expand Down Expand Up @@ -2064,8 +2017,6 @@ impl UhProcessor<'_, TdxBacked> {
self.runner
.write_vmcs32(vtl, seg.attributes(), !0, attributes.into());

// TODO TDX: cache CS into last exit because last exit contains CS optionally?

Ok(())
}

Expand Down Expand Up @@ -2101,7 +2052,6 @@ impl<T: CpuIo> EmulatorSupport for UhEmulationState<'_, '_, T, TdxBacked> {
let cs = TdxExit(self.vp.runner.tdx_vp_enter_exit_info()).cs();
let enter_state = self.vp.runner.tdx_enter_guest_state();

// TODO TDX: Only supports VTL0
Ok(x86emu::CpuState {
gps: enter_state.gps,
segs: [
Expand Down Expand Up @@ -2205,7 +2155,6 @@ impl<T: CpuIo> EmulatorSupport for UhEmulationState<'_, '_, T, TdxBacked> {
_gpa: u64,
_mode: TranslateMode,
) -> Result<(), virt_support_x86emu::emulate::EmuCheckVtlAccessError<Self::Error>> {
// TODO TDX: VTL1 not supported
// Lock Vtl TLB
Ok(())
}
Expand Down Expand Up @@ -2590,9 +2539,6 @@ impl<T: CpuIo> UhHypercallHandler<'_, '_, T, TdxBacked> {
hv1_hypercall::HvPostMessage,
hv1_hypercall::HvSignalEvent,
hv1_hypercall::HvExtQueryCapabilities,
// TODO TDX: copied from SNP, enable individually as needed.
// hv1_hypercall::HvGetVpRegisters,
// hv1_hypercall::HvEnablePartitionVtl,
]
);

Expand Down Expand Up @@ -2806,8 +2752,8 @@ impl AccessVpState for UhVpStateAccess<'_, '_, TdxBacked> {
nmi_masked: interruptibility.blocked_by_nmi(),
interrupt_shadow: interruptibility.blocked_by_sti()
|| interruptibility.blocked_by_movss(),
pending_event: None, // TODO TDX
pending_interruption: None, // TODO TDX
pending_event: None,
pending_interruption: None,
})
}

Expand All @@ -2817,8 +2763,8 @@ impl AccessVpState for UhVpStateAccess<'_, '_, TdxBacked> {
nmi_pending,
nmi_masked,
interrupt_shadow,
pending_event: _, // TODO TDX
pending_interruption: _, // TODO TDX
pending_event: _,
pending_interruption: _,
} = value;
self.vp.backing.cvm.lapics[self.vtl].activity = mp_state;
self.vp.backing.cvm.lapics[self.vtl].nmi_pending = nmi_pending;
Expand Down Expand Up @@ -2887,14 +2833,13 @@ impl AccessVpState for UhVpStateAccess<'_, '_, TdxBacked> {

fn mtrrs(&mut self) -> Result<vp::Mtrrs, Self::Error> {
Ok(vp::Mtrrs {
msr_mtrr_def_type: 0, // TODO TDX: MTRRs
fixed: [0; 11], // TODO TDX: MTRRs
variable: [0; 16], // TODO TDX: MTRRs
msr_mtrr_def_type: 0,
fixed: [0; 11],
variable: [0; 16],
})
}

fn set_mtrrs(&mut self, _value: &vp::Mtrrs) -> Result<(), Self::Error> {
// TODO TDX: MTRRs
Ok(())
}

Expand Down
1 change: 0 additions & 1 deletion vm/hv1/hv1_emulator/src/cpuid.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ pub fn hv_cpuid_leaves(
.with_use_apic_msrs(use_apic_msrs);

if hardware_isolated {
// TODO TDX too when it's ready
if isolation == IsolationType::Snp {
enlightenments = enlightenments
.with_use_hypercall_for_remote_flush_and_local_flush_entire(true);
Expand Down
Loading
Loading