diff --git a/openhcl/hcl/src/ioctl.rs b/openhcl/hcl/src/ioctl.rs index e983f84f8f..bdf20ac36e 100644 --- a/openhcl/hcl/src/ioctl.rs +++ b/openhcl/hcl/src/ioctl.rs @@ -2819,7 +2819,7 @@ 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. + // TODO TDX: Figure out what these values should be. (Issue #560) IsolationType::Tdx => hvdef::HvRegisterVsmCapabilities::new() .with_deny_lower_vtl_startup(caps.deny_lower_vtl_startup()) .with_intercept_page_available(caps.intercept_page_available()), @@ -2920,7 +2920,7 @@ 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 SNP TODO TDX - required for vmbus relay monitor page support (Issue #559) todo!(); } diff --git a/openhcl/openhcl_boot/src/arch/x86_64/tdx.rs b/openhcl/openhcl_boot/src/arch/x86_64/tdx.rs index 47858bd3cd..28bda12484 100644 --- a/openhcl/openhcl_boot/src/arch/x86_64/tdx.rs +++ b/openhcl/openhcl_boot/src/arch/x86_64/tdx.rs @@ -113,8 +113,7 @@ pub fn get_tdx_tsc_reftime() -> Option { // 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. + // TODO TDX: Explore the option of getting frequency from CPUID. (Issue #432) TSC_FREQUENCY.set(read_msr_tdcall(hvdef::HV_X64_MSR_TSC_FREQUENCY)); } diff --git a/openhcl/openhcl_boot/src/main.rs b/openhcl/openhcl_boot/src/main.rs index 8f24903336..b92490b2cb 100644 --- a/openhcl/openhcl_boot/src/main.rs +++ b/openhcl/openhcl_boot/src/main.rs @@ -796,14 +796,15 @@ 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 + // TODO TDX SNP: check and make sure the vp index ordering is in agreement with + // the host and hypervisor. 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. + // certain if there are places where we depend on this mapping today. (Issue #572) return; } diff --git a/openhcl/underhill_mem/src/lib.rs b/openhcl/underhill_mem/src/lib.rs index 5948416dfe..9d19a7faad 100644 --- a/openhcl/underhill_mem/src/lib.rs +++ b/openhcl/underhill_mem/src/lib.rs @@ -1137,11 +1137,11 @@ mod mapping { } } IsolationType::Tdx => { - // TODO TDX GUEST VSM: implement acceptor.vtl_permissions + // TODO TDX: 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. + // guest memory. (Issue #573) GpaVtlPermissions::new(IsolationType::Tdx, vtl, HV_MAP_GPA_PERMISSIONS_ALL) } diff --git a/openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs b/openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs index 9da1bbdbde..b6c48cc7a0 100644 --- a/openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs +++ b/openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs @@ -60,7 +60,7 @@ trait CpuidArchInitializer { /// 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) + // signature to pass CpuidResults as a mutable reference. Issue #556) fn process_extended_state_subleaves( &self, results: &mut CpuidSubtable, diff --git a/openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs b/openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs index c95088bb1a..d2ca0edc40 100644 --- a/openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs +++ b/openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs @@ -23,7 +23,7 @@ pub const TDX_REQUIRED_LEAVES: &[(CpuidFunction, Option)] = &[ (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 + // they're required for TDX (Issue #562) (CpuidFunction::CacheAndTlbInformation, None), (CpuidFunction::ExtendedFeatures, Some(1)), (CpuidFunction::CacheParameters, Some(0)), @@ -51,7 +51,7 @@ impl CpuidArchInitializer for TdxCpuidInitializer { } fn extended_max_function(&self) -> u32 { - // TODO TDX: Check if this is the same value in the OS repo + // TODO TDX: Check if this is the same value in the OS repo (Issue #556) CpuidFunction::ExtendedIntelMaximum.0 } @@ -111,7 +111,7 @@ impl CpuidArchInitializer for TdxCpuidInitializer { } CpuidFunction::TmulInformation => { if subleaf == 0 { - // TODO TDX: does this actually have subleaves? the spec says 1+ are reserved + // TODO TDX: does this actually have subleaves? the spec says 1+ are reserved (Issue #556) Some(CpuidResultMask::new( 0xffffffff, 0xffffffff, 0xffffffff, 0xffffffff, true, )) @@ -164,7 +164,7 @@ impl CpuidArchInitializer for TdxCpuidInitializer { results: &mut CpuidSubtable, extended_state_mask: u64, ) -> Result<(), CpuidResultsError> { - // TODO TDX: See HvlpPopulateExtendedStateCpuid + // TODO TDX: See HvlpPopulateExtendedStateCpuid (Issue #556) let xfd_supported = if let Some(support) = results.get(&1).map( |CpuidResult { eax, @@ -188,7 +188,7 @@ impl CpuidArchInitializer for TdxCpuidInitializer { 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 + // TODO TDX: update some maximum xfd value; see HvlpMaximumXfd (Issue #556) } results.insert(i, result); @@ -205,8 +205,7 @@ impl CpuidArchInitializer for TdxCpuidInitializer { _address_space_sizes_ecx: cpuid::ExtendedAddressSpaceSizesEcx, _processor_topology_ebx: Option, // Will be None for Intel ) -> Result { - // TODO TDX: see HvlpInitializeCpuidTopologyIntel - // TODO TDX: fix returned errors + // TODO TDX: see HvlpInitializeCpuidTopologyIntel and fix returned errors (Issue #556) let vps_per_socket; if !version_and_features_edx.mt_per_socket() { if version_and_features_ebx.lps_per_package() > 1 { @@ -220,8 +219,7 @@ impl CpuidArchInitializer for TdxCpuidInitializer { vps_per_socket = version_and_features_ebx.lps_per_package(); } - // TODO TDX: validation of leaf 0xB - + // TODO TDX: validation of leaf 0xB (Issue #556) Ok(super::ExtendedTopologyResult { subleaf0: None, subleaf1: None, diff --git a/openhcl/virt_mshv_vtl/src/lib.rs b/openhcl/virt_mshv_vtl/src/lib.rs index 8183a2a93c..1a5b8e567a 100644 --- a/openhcl/virt_mshv_vtl/src/lib.rs +++ b/openhcl/virt_mshv_vtl/src/lib.rs @@ -866,7 +866,7 @@ 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. + // implemented yet. (Issue #575) if self.inner.isolation.is_hardware_isolated() { None } else { @@ -1391,7 +1391,7 @@ impl<'a> UhProtoPartition<'a> { // 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. + // register for these intercepts itself. (Issue #558) if params.intercept_debug_exceptions { if !cfg!(feature = "gdb") { return Err(Error::InvalidDebugConfiguration); @@ -1745,7 +1745,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, // TODO TDX GUEST_VSM (Issue #557) #[cfg(guest_arch = "x86_64")] IsolationType::Snp => { if !params.env_cvm_guest_vsm { diff --git a/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs b/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs index 794c923196..12943f1b7e 100644 --- a/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs @@ -709,7 +709,7 @@ impl UhHypercallHandler<'_, '_, T, B> { } // TODO GUEST VSM: interrupt rewinding - // TODO TDX GUEST VSM: update execution mode + // TODO TDX GUEST VSM: update execution mode (Issue #687) } } diff --git a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs index db527bae4c..1318020d78 100644 --- a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs @@ -500,7 +500,7 @@ impl HardwareIsolatedBacking for TdxBacked { _source_vtl: GuestVtl, _target_vtl: GuestVtl, ) { - // TODO TDX GUEST VSM + // TODO TDX GUEST VSM (Issue #685) todo!() } @@ -564,7 +564,7 @@ impl BackingPrivate for TdxBacked { _shared: &TdxBackedShared, ) -> Result { // TODO TDX: TDX shares the vp context page for xmm registers only. It - // should probably move to its own page. + // should probably move to its own page. (Issue #555) // // FX regs and XMM registers are zero-initialized by the kernel. Set // them to the arch default. @@ -590,15 +590,13 @@ 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? - - // TODO TDX GUEST VSM: Presumably we need to duplicate much of this work + // TODO TDX: Implement the following for vp context (Issue #555) + // ssp is for shadow stack + // direct overlay like snp? + // lapic / APIC setup? + // see ValInitializeVplc + // XCR_XFMEM setup? + // GUEST VSM: Presumably we need to duplicate much of this work // when VTL 1 is enabled. // Configure L2 controls to permit shared memory. @@ -649,7 +647,7 @@ 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. + // ioctl. (Issue #555) let read_cr4 = hcl.read_vmx_cr4_fixed1(); let allowed_cr4_bits = (ShadowedRegister::Cr4.guest_owned_mask() | X64_CR4_MCE) & read_cr4; @@ -689,7 +687,7 @@ impl BackingPrivate for TdxBacked { let pfns = pfns_handle.base_pfn()..pfns_handle.base_pfn() + pfns_handle.size_pages(); let overlays: Vec<_> = pfns.collect(); - // TODO TDX: This needs to come from a private pool + // TODO TDX: This needs to come from a private pool (Issue #555) let flush_page = params .partition .shared_vis_pages_pool @@ -765,7 +763,7 @@ impl BackingPrivate for TdxBacked { fn init(this: &mut UhProcessor<'_, Self>) { // TODO TDX GUEST VSM: Presumably we need to duplicate much of this work - // when VTL 1 is enabled. + // when VTL 1 is enabled. (Issue #555) // Configure the synic overlays. let pfns = &this.backing.direct_overlays_pfns; @@ -862,7 +860,7 @@ impl BackingPrivate for TdxBacked { this: &mut UhProcessor<'_, Self>, _dev: &impl CpuIo, ) -> Result { - // TODO TDX GUEST VSM + // TODO TDX GUEST VSM (Issue #686) this.hcvm_handle_cross_vtl_interrupts(|_this, _vtl, _check_rflags| false) } @@ -888,7 +886,7 @@ impl UhProcessor<'_, TdxBacked> { /// poll retried. fn try_poll_apic(&mut self, vtl: GuestVtl, scan_irr: bool) -> Result { // Check for interrupt requests from the host and kernel IPI offload. - // TODO TDX GUEST VSM supporting VTL 1 proxy irrs requires kernel changes + // TODO TDX GUEST VSM supporting VTL 1 proxy irrs requires kernel changes (Issue #691) if vtl == GuestVtl::Vtl0 { if let Some(irr) = self.runner.proxy_irr() { // We can't put the interrupts directly on the APIC page because we might need @@ -1463,7 +1461,7 @@ impl UhProcessor<'_, TdxBacked> { // 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. + // lose this interrupt. (Issue #553) if next_interruption.valid() { tracing::debug!( ?next_interruption, @@ -1692,7 +1690,7 @@ impl UhProcessor<'_, TdxBacked> { // TODO TDX: see lots of these exits while waiting at frontpage. // Probably expected, given we will still get L1 timer - // interrupts? + // interrupts? (Issue #688) self.clear_interrupt_shadow(intercepted_vtl); self.advance_to_next_instruction(); &mut self.backing.vtls[intercepted_vtl].exit_stats.hlt @@ -1768,10 +1766,9 @@ impl UhProcessor<'_, TdxBacked> { &mut self.backing.vtls[intercepted_vtl].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. + // TODO TDX: If this is an access to a shared gpa, then this + // is the result of hypervisor unable to handle it and + // delivered an intercept message for this GPA. (Issue #689) // // However, at this point the kernel has cleared that // information so some kind of redesign is required to figure @@ -2042,14 +2039,14 @@ impl UhProcessor<'_, TdxBacked> { } fn read_msr_cvm(&self, msr: u32, vtl: GuestVtl) -> Result { - // TODO TDX: port remaining tdx and common values + // TODO TDX: port remaining tdx and common values (Issue #551) // - // TODO TDX: consider if this can be shared with SnpBacked's + // 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 + // TODO TDX: LIFTED FROM WHP (Issue #551) x86defs::X86X_IA32_MSR_PLATFORM_ID => { // Windows requires accessing this to boot. WHP // used to pass this through to the hardware, @@ -2206,7 +2203,7 @@ 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? + // TODO TDX: cache CS into last exit because last exit contains CS optionally? (Issue #550) Ok(()) } @@ -2361,7 +2358,7 @@ impl X86EmulatorSupport for UhEmulationState<'_, '_, T, TdxBacked> { _mode: TranslateMode, ) -> Result<(), virt_support_x86emu::emulate::EmuCheckVtlAccessError> { // Lock Vtl TLB - // TODO TDX GUEST VSM: VTL1 not yet supported + // TODO TDX GUEST VSM: VTL1 not yet supported (Issue #549) Ok(()) } @@ -2744,7 +2741,7 @@ impl UhHypercallHandler<'_, '_, T, TdxBacked> { hv1_hypercall::HvPostMessage, hv1_hypercall::HvSignalEvent, hv1_hypercall::HvExtQueryCapabilities, - // TODO TDX: copied from SNP, enable individually as needed. + // TODO TDX: copied from SNP, enable individually as needed. (Issue #548) // hv1_hypercall::HvGetVpRegisters, // hv1_hypercall::HvEnablePartitionVtl, ] @@ -2949,8 +2946,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, // TODO TDX (Issue #547) + pending_interruption: None, // TODO TDX (Issue #547) }) } @@ -2960,8 +2957,8 @@ impl AccessVpState for UhVpStateAccess<'_, '_, TdxBacked> { nmi_pending, nmi_masked, interrupt_shadow, - pending_event: _, // TODO TDX - pending_interruption: _, // TODO TDX + pending_event: _, // TODO TDX (Issue #547) + pending_interruption: _, // TODO TDX (Issue #547) } = value; self.vp.backing.cvm.lapics[self.vtl].activity = mp_state; self.vp.backing.cvm.lapics[self.vtl].nmi_pending = nmi_pending; @@ -3031,14 +3028,14 @@ impl AccessVpState for UhVpStateAccess<'_, '_, TdxBacked> { fn mtrrs(&mut self) -> Result { 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, // TODO TDX: MTRRs (Issue #547) + fixed: [0; 11], // TODO TDX: MTRRs (Issue #547) + variable: [0; 16], // TODO TDX: MTRRs (Issue #547) }) } fn set_mtrrs(&mut self, _value: &vp::Mtrrs) -> Result<(), Self::Error> { - // TODO TDX: MTRRs + // TODO TDX: MTRRs (Issue #547) Ok(()) } diff --git a/vm/hv1/hv1_emulator/src/cpuid.rs b/vm/hv1/hv1_emulator/src/cpuid.rs index 29d1dfb7b9..6e0459f3b5 100644 --- a/vm/hv1/hv1_emulator/src/cpuid.rs +++ b/vm/hv1/hv1_emulator/src/cpuid.rs @@ -129,7 +129,7 @@ pub fn hv_cpuid_leaves( .with_use_apic_msrs(use_apic_msrs); if hardware_isolated { - // TODO TDX too when it's ready + // TODO TDX: Process TDX isolation type when it's ready (Issue #546) if isolation == IsolationType::Snp { enlightenments = enlightenments .with_use_hypercall_for_remote_flush_and_local_flush_entire(true); diff --git a/vm/x86/tdcall/src/lib.rs b/vm/x86/tdcall/src/lib.rs index c3a3ac1afd..91a2b8a5bc 100644 --- a/vm/x86/tdcall/src/lib.rs +++ b/vm/x86/tdcall/src/lib.rs @@ -357,7 +357,7 @@ pub fn tdcall_page_attr_wr( let output = call.tdcall(input); - // TODO TDX: RCX and RDX also contain info that could be returned + // TODO TDX: RCX and RDX also contain info that could be returned (Issue #545) match output.rax.code() { TdCallResultCode::SUCCESS => Ok(()), @@ -392,7 +392,7 @@ fn set_page_attr( /// The error returned by [`accept_pages`]. #[derive(Debug)] pub enum AcceptPagesError { - // TODO TDX: better error types + // TODO TDX: Add better error types (Issue #545) /// Unknown error type. Unknown(TdCallResultCode), /// Setting page attributes failed after accepting, @@ -574,7 +574,7 @@ pub fn tdcall_map_gpa( let output = call.tdcall(input); - // TODO TDX: check rax return code + // TODO TDX: check rax return code (Issue #545) let result = TdVmCallR10Result(output.r10); diff --git a/vm/x86/x86defs/src/tdx.rs b/vm/x86/x86defs/src/tdx.rs index 2e047cafd6..904e819a56 100644 --- a/vm/x86/x86defs/src/tdx.rs +++ b/vm/x86/x86defs/src/tdx.rs @@ -502,8 +502,8 @@ pub struct TdxExtendedFieldCode { /// Instruction info returned in r11 for a TDG.VP.ENTER call. #[bitfield(u64)] pub struct TdxInstructionInfo { - pub info: u32, // TODO TDX: what is this - pub length: u32, + pub info: u32, // Information about the instruction that caused VM exit. Refer Intel SDM 28.2 + pub length: u32, // Length of the instruction that caused VM exit. } #[bitfield(u64)]