From 156fd90bc7ab540abd5f309ea09247c56b538f79 Mon Sep 17 00:00:00 2001 From: Chakradhar Kotamraju Date: Wed, 18 Dec 2024 16:51:52 -0700 Subject: [PATCH] TODO TDX comments were removed and corresponding issue items have been created --- openhcl/hcl/src/ioctl.rs | 2 - openhcl/openhcl_boot/src/arch/x86_64/tdx.rs | 2 - openhcl/openhcl_boot/src/main.rs | 8 --- openhcl/underhill_mem/src/lib.rs | 6 -- openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs | 2 - openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs | 13 +--- openhcl/virt_mshv_vtl/src/lib.rs | 9 +-- .../src/processor/hardware_cvm/mod.rs | 1 - .../virt_mshv_vtl/src/processor/tdx/mod.rs | 69 ++----------------- vm/hv1/hv1_emulator/src/cpuid.rs | 1 - vm/x86/tdcall/src/lib.rs | 5 -- vm/x86/x86defs/src/tdx.rs | 2 +- 12 files changed, 10 insertions(+), 110 deletions(-) diff --git a/openhcl/hcl/src/ioctl.rs b/openhcl/hcl/src/ioctl.rs index 967619e89..9663134c6 100644 --- a/openhcl/hcl/src/ioctl.rs +++ b/openhcl/hcl/src/ioctl.rs @@ -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()), @@ -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!(); } diff --git a/openhcl/openhcl_boot/src/arch/x86_64/tdx.rs b/openhcl/openhcl_boot/src/arch/x86_64/tdx.rs index 47858bd3c..33427c554 100644 --- a/openhcl/openhcl_boot/src/arch/x86_64/tdx.rs +++ b/openhcl/openhcl_boot/src/arch/x86_64/tdx.rs @@ -113,8 +113,6 @@ 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. 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 a1a8a306f..45b2aa53b 100644 --- a/openhcl/openhcl_boot/src/main.rs +++ b/openhcl/openhcl_boot/src/main.rs @@ -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; } diff --git a/openhcl/underhill_mem/src/lib.rs b/openhcl/underhill_mem/src/lib.rs index 173bc4355..4e1971573 100644 --- a/openhcl/underhill_mem/src/lib.rs +++ b/openhcl/underhill_mem/src/lib.rs @@ -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) } }; diff --git a/openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs b/openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs index 9da1bbdbd..7ab64b8e7 100644 --- a/openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs +++ b/openhcl/virt_mshv_vtl/src/cvm_cpuid/mod.rs @@ -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, diff --git a/openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs b/openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs index c95088bb1..864be264d 100644 --- a/openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs +++ b/openhcl/virt_mshv_vtl/src/cvm_cpuid/tdx.rs @@ -22,8 +22,6 @@ pub const TDX_REQUIRED_LEAVES: &[(CpuidFunction, Option)] = &[ (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)), @@ -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 } @@ -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, )) @@ -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, @@ -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); } @@ -205,8 +198,6 @@ 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 let vps_per_socket; if !version_and_features_edx.mt_per_socket() { if version_and_features_ebx.lps_per_package() > 1 { @@ -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, diff --git a/openhcl/virt_mshv_vtl/src/lib.rs b/openhcl/virt_mshv_vtl/src/lib.rs index d0258d7c6..d1dede3f2 100644 --- a/openhcl/virt_mshv_vtl/src/lib.rs +++ b/openhcl/virt_mshv_vtl/src/lib.rs @@ -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 { @@ -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); @@ -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 { 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 31aebec67..97c813077 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,6 @@ impl UhHypercallHandler<'_, '_, T, B> { } // TODO GUEST VSM: interrupt rewinding - // TODO TDX GUEST VSM: update execution mode } } diff --git a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs index af78b5970..69b1fe855 100644 --- a/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs +++ b/openhcl/virt_mshv_vtl/src/processor/tdx/mod.rs @@ -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; @@ -538,9 +537,6 @@ impl BackingPrivate for TdxBacked { params: super::private::BackingParams<'_, '_, Self>, _shared: &TdxBackedShared, ) -> Result { - // 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() = @@ -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 @@ -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; @@ -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 @@ -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, @@ -828,7 +812,6 @@ impl BackingPrivate for TdxBacked { this: &mut UhProcessor<'_, Self>, _dev: &impl CpuIo, ) -> Result { - // TODO TDX GUEST VSM this.hcvm_handle_cross_vtl_interrupts(|_this, _vtl, _check_rflags| false) } @@ -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() @@ -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, @@ -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(); @@ -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)) @@ -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 { @@ -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), @@ -1900,14 +1860,7 @@ impl UhProcessor<'_, TdxBacked> { } fn read_msr_cvm(&self, msr: u32, vtl: GuestVtl) -> Result { - // 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, @@ -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(()) } @@ -2101,7 +2052,6 @@ impl 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: [ @@ -2205,7 +2155,6 @@ impl EmulatorSupport for UhEmulationState<'_, '_, T, TdxBacked> { _gpa: u64, _mode: TranslateMode, ) -> Result<(), virt_support_x86emu::emulate::EmuCheckVtlAccessError> { - // TODO TDX: VTL1 not supported // Lock Vtl TLB Ok(()) } @@ -2590,9 +2539,6 @@ impl 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, ] ); @@ -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, }) } @@ -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; @@ -2887,14 +2833,13 @@ 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, + fixed: [0; 11], + variable: [0; 16], }) } fn set_mtrrs(&mut self, _value: &vp::Mtrrs) -> Result<(), Self::Error> { - // TODO TDX: MTRRs Ok(()) } diff --git a/vm/hv1/hv1_emulator/src/cpuid.rs b/vm/hv1/hv1_emulator/src/cpuid.rs index 29d1dfb7b..5c2ef8382 100644 --- a/vm/hv1/hv1_emulator/src/cpuid.rs +++ b/vm/hv1/hv1_emulator/src/cpuid.rs @@ -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); diff --git a/vm/x86/tdcall/src/lib.rs b/vm/x86/tdcall/src/lib.rs index c3a3ac1af..e109a9d77 100644 --- a/vm/x86/tdcall/src/lib.rs +++ b/vm/x86/tdcall/src/lib.rs @@ -357,8 +357,6 @@ pub fn tdcall_page_attr_wr( let output = call.tdcall(input); - // TODO TDX: RCX and RDX also contain info that could be returned - match output.rax.code() { TdCallResultCode::SUCCESS => Ok(()), val => Err(val), @@ -392,7 +390,6 @@ fn set_page_attr( /// The error returned by [`accept_pages`]. #[derive(Debug)] pub enum AcceptPagesError { - // TODO TDX: better error types /// Unknown error type. Unknown(TdCallResultCode), /// Setting page attributes failed after accepting, @@ -574,8 +571,6 @@ pub fn tdcall_map_gpa( let output = call.tdcall(input); - // TODO TDX: check rax return code - let result = TdVmCallR10Result(output.r10); match result { diff --git a/vm/x86/x86defs/src/tdx.rs b/vm/x86/x86defs/src/tdx.rs index 2e047cafd..cfab14119 100644 --- a/vm/x86/x86defs/src/tdx.rs +++ b/vm/x86/x86defs/src/tdx.rs @@ -502,7 +502,7 @@ 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 info: u32, pub length: u32, }