diff --git a/docs/intel_tdx.md b/docs/intel_tdx.md index 31c5fd9fd..1052d897d 100644 --- a/docs/intel_tdx.md +++ b/docs/intel_tdx.md @@ -61,7 +61,8 @@ meaning it will be printing guest kernel logs to the `virtio-console` device. ```bash ./cloud-hypervisor \ - --tdx firmware=edk2-staging/Build/OvmfCh/RELEASE_GCC5/FV/OVMF.fd \ + --platform tdx=on + --firmware edk2-staging/Build/OvmfCh/RELEASE_GCC5/FV/OVMF.fd \ --cpus boot=1 \ --memory size=1G \ --disk path=tdx_guest_img @@ -72,7 +73,8 @@ firmware: ```bash ./cloud-hypervisor \ - --tdx firmware=edk2-staging/Build/OvmfCh/DEBUG_GCC5/FV/OVMF.fd \ + --platform tdx=on + --firmware edk2-staging/Build/OvmfCh/DEBUG_GCC5/FV/OVMF.fd \ --cpus boot=1 \ --memory size=1G \ --disk path=tdx_guest_img \ @@ -95,7 +97,8 @@ option as well. ```bash ./cloud-hypervisor \ - --tdx firmware=tdshim \ + --platform tdx=on + --firmware tdshim \ --kernel bzImage \ --cmdline "root=/dev/vda3 console=hvc0 rw" --cpus boot=1 \ diff --git a/src/main.rs b/src/main.rs index 8725129a2..c7a43f67f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -400,15 +400,6 @@ fn create_app<'a>( .group("vmm-config"), ); - #[cfg(feature = "tdx")] - let app = app.arg( - Arg::new("tdx") - .long("tdx") - .help("TDX Support: firmware=") - .takes_value(true) - .group("vm-config"), - ); - app } @@ -577,11 +568,6 @@ fn start_vmm(cmd_arguments: ArgMatches) -> Result, Error> { let payload_present = cmd_arguments.is_present("kernel") || cmd_arguments.is_present("firmware"); - // Can't test for "vm-config" group as some have default values. The kernel (or tdx if enabled) - // is the only required option for booting the VM. - #[cfg(feature = "tdx")] - let payload_present = payload_present || cmd_arguments.is_present("tdx"); - if payload_present { let vm_params = config::VmParams::from_arg_matches(&cmd_arguments); let vm_config = config::VmConfig::parse(vm_params).map_err(Error::ParsingConfig)?; @@ -744,8 +730,6 @@ mod unit_tests { sgx_epc: None, numa: None, watchdog: false, - #[cfg(feature = "tdx")] - tdx: None, #[cfg(feature = "gdb")] gdb: false, platform: None, diff --git a/vmm/src/api/openapi/cloud-hypervisor.yaml b/vmm/src/api/openapi/cloud-hypervisor.yaml index 029b4cf08..514aad63e 100644 --- a/vmm/src/api/openapi/cloud-hypervisor.yaml +++ b/vmm/src/api/openapi/cloud-hypervisor.yaml @@ -559,8 +559,6 @@ components: type: array items: $ref: "#/components/schemas/SgxEpcConfig" - tdx: - $ref: "#/components/schemas/TdxConfig" numa: type: array items: @@ -650,6 +648,9 @@ components: type: array items: type: string + tdx: + type: boolean + default: false MemoryZoneConfig: required: @@ -1006,15 +1007,6 @@ components: type: boolean default: false - TdxConfig: - required: - - firmware - type: object - properties: - firmware: - type: string - description: Path to the firmware that will be used to boot the TDx guest up. - NumaDistance: required: - destination diff --git a/vmm/src/config.rs b/vmm/src/config.rs index 9d72f8feb..4f85bb6c4 100644 --- a/vmm/src/config.rs +++ b/vmm/src/config.rs @@ -153,6 +153,9 @@ pub enum ValidationError { /// CPU Hotplug is not permitted with TDX #[cfg(feature = "tdx")] TdxNoCpuHotplug, + /// Missing firmware for TDX + #[cfg(feature = "tdx")] + TdxFirmwareMissing, /// Insuffient vCPUs for queues TooManyQueues, /// Need shared memory for vfio-user @@ -219,6 +222,10 @@ impl fmt::Display for ValidationError { TdxNoCpuHotplug => { write!(f, "CPU hotplug is not permitted with TDX") } + #[cfg(feature = "tdx")] + TdxFirmwareMissing => { + write!(f, "No TDX firmware specified") + } TooManyQueues => { write!(f, "Number of vCPUs is insufficient for number of queues") } @@ -363,8 +370,6 @@ pub struct VmParams<'a> { pub sgx_epc: Option>, pub numa: Option>, pub watchdog: bool, - #[cfg(feature = "tdx")] - pub tdx: Option<&'a str>, #[cfg(feature = "gdb")] pub gdb: bool, pub platform: Option<&'a str>, @@ -397,8 +402,6 @@ impl<'a> VmParams<'a> { let numa: Option> = args.values_of("numa").map(|x| x.collect()); let watchdog = args.is_present("watchdog"); let platform = args.value_of("platform"); - #[cfg(feature = "tdx")] - let tdx = args.value_of("tdx"); #[cfg(feature = "gdb")] let gdb = args.is_present("gdb"); VmParams { @@ -425,8 +428,6 @@ impl<'a> VmParams<'a> { sgx_epc, numa, watchdog, - #[cfg(feature = "tdx")] - tdx, #[cfg(feature = "gdb")] gdb, platform, @@ -642,16 +643,22 @@ pub struct PlatformConfig { pub uuid: Option, #[serde(default)] pub oem_strings: Option>, + #[cfg(feature = "tdx")] + #[serde(default)] + pub tdx: bool, } impl PlatformConfig { pub fn parse(platform: &str) -> Result { let mut parser = OptionParser::new(); - parser.add("num_pci_segments"); - parser.add("iommu_segments"); - parser.add("serial_number"); - parser.add("uuid"); - parser.add("oem_strings"); + parser + .add("num_pci_segments") + .add("iommu_segments") + .add("serial_number") + .add("uuid") + .add("oem_strings"); + #[cfg(feature = "tdx")] + parser.add("tdx"); parser.parse(platform).map_err(Error::ParsePlatform)?; let num_pci_segments: u16 = parser @@ -670,12 +677,20 @@ impl PlatformConfig { .convert::("oem_strings") .map_err(Error::ParsePlatform)? .map(|v| v.0); + #[cfg(feature = "tdx")] + let tdx = parser + .convert::("tdx") + .map_err(Error::ParsePlatform)? + .unwrap_or(Toggle(false)) + .0; Ok(PlatformConfig { num_pci_segments, iommu_segments, serial_number, uuid, oem_strings, + #[cfg(feature = "tdx")] + tdx, }) } @@ -706,6 +721,8 @@ impl Default for PlatformConfig { serial_number: None, uuid: None, oem_strings: None, + #[cfg(feature = "tdx")] + tdx: false, } } } @@ -2083,26 +2100,6 @@ impl VsockConfig { } } -#[cfg(feature = "tdx")] -#[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize, Default)] -pub struct TdxConfig { - pub firmware: PathBuf, -} - -#[cfg(feature = "tdx")] -impl TdxConfig { - pub fn parse(tdx: &str) -> Result { - let mut parser = OptionParser::new(); - parser.add("firmware"); - parser.parse(tdx).map_err(Error::ParseTdx)?; - let firmware = parser - .get("firmware") - .map(PathBuf::from) - .ok_or(Error::FirmwarePathMissing)?; - Ok(TdxConfig { firmware }) - } -} - #[cfg(target_arch = "x86_64")] #[derive(Clone, Debug, PartialEq, Eq, Deserialize, Serialize, Default)] pub struct SgxEpcConfig { @@ -2296,8 +2293,6 @@ pub struct VmConfig { pub numa: Option>, #[serde(default)] pub watchdog: bool, - #[cfg(feature = "tdx")] - pub tdx: Option, #[cfg(feature = "gdb")] pub gdb: bool, pub platform: Option, @@ -2341,16 +2336,16 @@ impl VmConfig { }) } - #[cfg(not(feature = "tdx"))] self.payload .as_ref() .ok_or(ValidationError::KernelMissing)?; #[cfg(feature = "tdx")] { - let tdx_enabled = self.tdx.is_some(); - if !tdx_enabled && self.payload.is_none() { - return Err(ValidationError::KernelMissing); + let tdx_enabled = self.platform.as_ref().map(|p| p.tdx).unwrap_or(false); + // At this point we know payload isn't None. + if tdx_enabled && self.payload.as_ref().unwrap().firmware.is_none() { + return Err(ValidationError::TdxFirmwareMissing); } if tdx_enabled && (self.cpus.max_vcpus != self.cpus.boot_vcpus) { return Err(ValidationError::TdxNoCpuHotplug); @@ -2686,9 +2681,6 @@ impl VmConfig { None }; - #[cfg(feature = "tdx")] - let tdx = vm_params.tdx.map(TdxConfig::parse).transpose()?; - #[cfg(feature = "gdb")] let gdb = vm_params.gdb; @@ -2716,8 +2708,6 @@ impl VmConfig { sgx_epc, numa, watchdog: vm_params.watchdog, - #[cfg(feature = "tdx")] - tdx, #[cfg(feature = "gdb")] gdb, platform, @@ -2725,6 +2715,11 @@ impl VmConfig { config.validate().map_err(Error::Validation)?; Ok(config) } + + #[cfg(feature = "tdx")] + pub fn is_tdx_enabled(&self) -> bool { + self.platform.as_ref().map(|p| p.tdx).unwrap_or(false) + } } #[cfg(test)] @@ -3323,8 +3318,6 @@ mod tests { sgx_epc: None, numa: None, watchdog: false, - #[cfg(feature = "tdx")] - tdx: None, #[cfg(feature = "gdb")] gdb: false, platform: None, diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 9656f3bfc..d2102abd6 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -1419,8 +1419,6 @@ impl Vmm { let vm_config = vm.get_config(); #[cfg(all(feature = "kvm", target_arch = "x86_64"))] let common_cpuid = { - #[cfg(feature = "tdx")] - let tdx_enabled = vm_config.lock().unwrap().tdx.is_some(); let phys_bits = vm::physical_bits(vm_config.lock().unwrap().cpus.max_phys_bits); arch::generate_common_cpuid( hypervisor, @@ -1429,7 +1427,7 @@ impl Vmm { phys_bits, vm_config.lock().unwrap().cpus.kvm_hyperv, #[cfg(feature = "tdx")] - tdx_enabled, + vm_config.lock().unwrap().is_tdx_enabled(), ) .map_err(|e| { MigratableError::MigrateReceive(anyhow!("Error generating common cpuid': {:?}", e)) @@ -1612,8 +1610,6 @@ impl Vmm { let dest_cpuid = &{ let vm_config = &src_vm_config.lock().unwrap(); - #[cfg(feature = "tdx")] - let tdx_enabled = vm_config.tdx.is_some(); let phys_bits = vm::physical_bits(vm_config.cpus.max_phys_bits); arch::generate_common_cpuid( self.hypervisor.clone(), @@ -1622,7 +1618,7 @@ impl Vmm { phys_bits, vm_config.cpus.kvm_hyperv, #[cfg(feature = "tdx")] - tdx_enabled, + vm_config.is_tdx_enabled(), ) .map_err(|e| { MigratableError::MigrateReceive(anyhow!("Error generating common cpuid: {:?}", e)) @@ -2053,8 +2049,6 @@ mod unit_tests { sgx_epc: None, numa: None, watchdog: false, - #[cfg(feature = "tdx")] - tdx: None, #[cfg(feature = "gdb")] gdb: false, platform: None, diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs index 9264b3da7..2fd047501 100644 --- a/vmm/src/vm.rs +++ b/vmm/src/vm.rs @@ -285,6 +285,10 @@ pub enum Error { #[error("Error finalizing TDX VM: {0}")] FinalizeTdx(#[source] hypervisor::HypervisorVmError), + #[cfg(feature = "tdx")] + #[error("TDX firmware missing")] + TdxFirmwareMissing, + #[cfg(feature = "tdx")] #[error("Invalid TDX payload type")] InvalidPayloadType, @@ -515,7 +519,9 @@ impl Vm { Self::create_numa_nodes(config.lock().unwrap().numa.clone(), &memory_manager)?; #[cfg(feature = "tdx")] - let force_iommu = config.lock().unwrap().tdx.is_some(); + let tdx_enabled = config.lock().unwrap().is_tdx_enabled(); + #[cfg(feature = "tdx")] + let force_iommu = tdx_enabled; #[cfg(not(feature = "tdx"))] let force_iommu = false; @@ -559,8 +565,6 @@ impl Vm { }); let exit_evt_clone = exit_evt.try_clone().map_err(Error::EventFdClone)?; - #[cfg(feature = "tdx")] - let tdx_enabled = config.lock().unwrap().tdx.is_some(); let cpus_config = { &config.lock().unwrap().cpus.clone() }; let cpu_manager = cpu::CpuManager::new( cpus_config, @@ -723,7 +727,7 @@ impl Vm { let timestamp = Instant::now(); #[cfg(feature = "tdx")] - let tdx_enabled = config.lock().unwrap().tdx.is_some(); + let tdx_enabled = config.lock().unwrap().is_tdx_enabled(); hypervisor.check_required_extensions().unwrap(); #[cfg(feature = "tdx")] let vm = hypervisor @@ -1136,7 +1140,7 @@ impl Vm { ) -> Result>>> { // Kernel with TDX is loaded in a different manner #[cfg(feature = "tdx")] - if config.lock().unwrap().tdx.is_some() { + if config.lock().unwrap().is_tdx_enabled() { return Ok(None); } @@ -1784,10 +1788,19 @@ impl Vm { #[cfg(feature = "tdx")] fn extract_tdvf_sections(&mut self) -> Result> { use arch::x86_64::tdx::*; + + let firmware_path = self + .config + .lock() + .unwrap() + .payload + .as_ref() + .unwrap() + .firmware + .clone() + .ok_or(Error::TdxFirmwareMissing)?; // The TDVF file contains a table of section as well as code - let mut firmware_file = - File::open(&self.config.lock().unwrap().tdx.as_ref().unwrap().firmware) - .map_err(Error::LoadTdvf)?; + let mut firmware_file = File::open(firmware_path).map_err(Error::LoadTdvf)?; // For all the sections allocate some RAM backing them parse_tdvf_sections(&mut firmware_file).map_err(Error::ParseTdvf) @@ -1882,9 +1895,17 @@ impl Vm { } // The TDVF file contains a table of section as well as code - let mut firmware_file = - File::open(&self.config.lock().unwrap().tdx.as_ref().unwrap().firmware) - .map_err(Error::LoadTdvf)?; + let firmware_path = self + .config + .lock() + .unwrap() + .payload + .as_ref() + .unwrap() + .firmware + .clone() + .ok_or(Error::TdxFirmwareMissing)?; + let mut firmware_file = File::open(firmware_path).map_err(Error::LoadTdvf)?; // The guest memory at this point now has all the required regions so it // is safe to copy from the TDVF file into it. @@ -2115,7 +2136,7 @@ impl Vm { fn create_acpi_tables(&self) -> Option { #[cfg(feature = "tdx")] - if self.config.lock().unwrap().tdx.is_some() { + if self.config.lock().unwrap().is_tdx_enabled() { return None; } @@ -2166,10 +2187,13 @@ impl Vm { // finish. let entry_point = self.entry_point()?; + #[cfg(feature = "tdx")] + let tdx_enabled = self.config.lock().unwrap().is_tdx_enabled(); + // The initial TDX configuration must be done before the vCPUs are // created #[cfg(feature = "tdx")] - if self.config.lock().unwrap().tdx.is_some() { + if tdx_enabled { self.init_tdx()?; } @@ -2181,7 +2205,7 @@ impl Vm { .map_err(Error::CpuManager)?; #[cfg(feature = "tdx")] - let sections = if self.config.lock().unwrap().tdx.is_some() { + let sections = if tdx_enabled { self.extract_tdvf_sections()? } else { Vec::new() @@ -2189,7 +2213,7 @@ impl Vm { // Configuring the TDX regions requires that the vCPUs are created. #[cfg(feature = "tdx")] - let hob_address = if self.config.lock().unwrap().tdx.is_some() { + let hob_address = if tdx_enabled { // TDX sections are written to memory. self.populate_tdx_sections(§ions)? } else { @@ -2715,9 +2739,12 @@ impl Snapshottable for Vm { fn snapshot(&mut self) -> std::result::Result { event!("vm", "snapshotting"); + #[cfg(feature = "tdx")] + let tdx_enabled = self.config.lock().unwrap().is_tdx_enabled(); + #[cfg(feature = "tdx")] { - if self.config.lock().unwrap().tdx.is_some() { + if tdx_enabled { return Err(MigratableError::Snapshot(anyhow!( "Snapshot not possible with TDX VM" ))); @@ -2733,8 +2760,6 @@ impl Snapshottable for Vm { #[cfg(all(feature = "kvm", target_arch = "x86_64"))] let common_cpuid = { - #[cfg(feature = "tdx")] - let tdx_enabled = self.config.lock().unwrap().tdx.is_some(); let phys_bits = physical_bits(self.config.lock().unwrap().cpus.max_phys_bits); arch::generate_common_cpuid( self.hypervisor.clone(),