From 52eebaf6b22b7f547dee73aa3c99c104acf4d314 Mon Sep 17 00:00:00 2001 From: Praveen K Paladugu Date: Thu, 2 May 2024 21:42:57 +0000 Subject: [PATCH] vmm: refactor DeviceManager to use console_info While adding console devices, DeviceManager will now use the FDs in console_info instead of creating them. To reduce the size of this commit, I marked some variables are unused with '_' prefix. All those variables are cleaned up in next commit. Signed-off-by: Praveen K Paladugu --- vmm/src/console_devices.rs | 4 +- vmm/src/device_manager.rs | 215 ++++++++++++++++--------------------- vmm/src/lib.rs | 4 + vmm/src/serial_manager.rs | 28 +++-- vmm/src/vm.rs | 7 +- 5 files changed, 114 insertions(+), 144 deletions(-) diff --git a/vmm/src/console_devices.rs b/vmm/src/console_devices.rs index 93f581171..1e4ff22b5 100644 --- a/vmm/src/console_devices.rs +++ b/vmm/src/console_devices.rs @@ -107,7 +107,7 @@ fn modify_mode( Ok(()) } -pub fn set_raw_mode( +fn set_raw_mode( f: &dyn AsRawFd, original_termios_opt: Arc>>, ) -> ConsoleDeviceResult<()> { @@ -120,7 +120,7 @@ pub fn set_raw_mode( .map_err(ConsoleDeviceError::SetPtyRaw) } -pub fn create_pty() -> io::Result<(File, File, PathBuf)> { +fn create_pty() -> io::Result<(File, File, PathBuf)> { // Try to use /dev/pts/ptmx first then fall back to /dev/ptmx // This is done to try and use the devpts filesystem that // could be available for use in the process's namespace first. diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index eb525c602..c80d917c2 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -13,7 +13,7 @@ use crate::config::{ ConsoleOutputMode, DeviceConfig, DiskConfig, FsConfig, NetConfig, PmemConfig, UserDeviceConfig, VdpaConfig, VhostMode, VmConfig, VsockConfig, }; -use crate::console_devices::{create_pty, set_raw_mode, ConsoleDeviceError}; +use crate::console_devices::{ConsoleDeviceError, ConsoleInfo}; use crate::cpu::{CpuManager, CPU_MANAGER_ACPI_SIZE}; use crate::device_tree::{DeviceNode, DeviceTree}; use crate::interrupt::LegacyUserspaceInterruptManager; @@ -21,7 +21,6 @@ use crate::interrupt::MsiInterruptManager; use crate::memory_manager::{Error as MemoryManagerError, MemoryManager, MEMORY_MANAGER_ACPI_SIZE}; use crate::pci_segment::PciSegment; use crate::serial_manager::{Error as SerialManagerError, SerialManager}; -use crate::sigwinch_listener::listen_for_sigwinch_on_tty; use crate::vm_config::DEFAULT_PCI_SEGMENT_APERTURE_WEIGHT; use crate::GuestRegionMmap; use crate::PciDeviceInfo; @@ -53,7 +52,7 @@ use devices::legacy::Pl011; use devices::{ interrupt_controller, interrupt_controller::InterruptController, AcpiNotificationFlags, }; -use hypervisor::{HypervisorType, IoEventAddress}; +use hypervisor::IoEventAddress; use libc::{ tcsetattr, termios, MAP_NORESERVE, MAP_PRIVATE, MAP_SHARED, O_TMPFILE, PROT_READ, PROT_WRITE, TCSANOW, @@ -69,6 +68,7 @@ use std::collections::{BTreeMap, BTreeSet, HashMap}; use std::fs::{File, OpenOptions}; use std::io::{self, stdout, Seek, SeekFrom}; use std::num::Wrapping; +use std::os::fd::RawFd; use std::os::unix::fs::OpenOptionsExt; use std::os::unix::io::{AsRawFd, FromRawFd}; use std::path::PathBuf; @@ -496,6 +496,12 @@ pub enum DeviceManagerError { /// Cannot start sigwinch listener StartSigwinchListener(std::io::Error), + + // Invalid console info + InvalidConsoleInfo, + + // Invalid console fd + InvalidConsoleFd, } pub type DeviceManagerResult = result::Result; @@ -768,9 +774,6 @@ pub struct AcpiPlatformAddresses { } pub struct DeviceManager { - // The underlying hypervisor - hypervisor_type: HypervisorType, - // Manage address space related to devices address_manager: Arc, @@ -956,7 +959,6 @@ impl DeviceManager { pub fn new( #[cfg(target_arch = "x86_64")] io_bus: Arc, mmio_bus: Arc, - hypervisor_type: HypervisorType, vm: Arc, config: Arc>, memory_manager: Arc>, @@ -1133,7 +1135,6 @@ impl DeviceManager { } let device_manager = DeviceManager { - hypervisor_type, address_manager: Arc::clone(&address_manager), console: Arc::new(Console::default()), interrupt_controller: None, @@ -1230,6 +1231,7 @@ impl DeviceManager { serial_pty: Option, console_pty: Option, debug_console_pty: Option, + console_info: Option, console_resize_pipe: Option, original_termios_opt: Arc>>, ) -> DeviceManagerResult<()> { @@ -1295,6 +1297,7 @@ impl DeviceManager { serial_pty, console_pty, debug_console_pty, + console_info, console_resize_pipe, )?; @@ -1978,81 +1981,53 @@ impl DeviceManager { fn add_virtio_console_device( &mut self, virtio_devices: &mut Vec, - console_pty: Option, + console_fd: Option, resize_pipe: Option, ) -> DeviceManagerResult>> { let console_config = self.config.lock().unwrap().console.clone(); let endpoint = match console_config.mode { ConsoleOutputMode::File => { - let file = File::create(console_config.file.as_ref().unwrap()) - .map_err(DeviceManagerError::ConsoleOutputFileOpen)?; - Endpoint::File(file) + if let Some(file_fd) = console_fd { + // SAFETY: file_fd is guaranteed to be a valid fd from + // pre_create_console_devices() in vmm/src/console_devices.rs + Endpoint::File(unsafe { File::from_raw_fd(file_fd) }) + } else { + return Err(DeviceManagerError::InvalidConsoleFd); + } } ConsoleOutputMode::Pty => { - if let Some(pty) = console_pty { - self.config.lock().unwrap().console.file = Some(pty.path.clone()); - let file = pty.main.try_clone().unwrap(); - self.console_pty = Some(Arc::new(Mutex::new(pty))); + if let Some(pty_fd) = console_fd { + // SAFETY: pty_fd is guaranteed to be a valid fd from + // pre_create_console_devices() in vmm/src/console_devices.rs + let file = unsafe { File::from_raw_fd(pty_fd) }; self.console_resize_pipe = resize_pipe.map(Arc::new); Endpoint::PtyPair(file.try_clone().unwrap(), file) } else { - let (main, sub, path) = - create_pty().map_err(DeviceManagerError::ConsolePtyOpen)?; - set_raw_mode(&sub, self.original_termios_opt.clone()) - .map_err(DeviceManagerError::SetPtyRaw)?; - self.config.lock().unwrap().console.file = Some(path.clone()); - let file = main.try_clone().unwrap(); - assert!(resize_pipe.is_none()); - self.console_resize_pipe = Some(Arc::new( - listen_for_sigwinch_on_tty(sub, &self.seccomp_action, self.hypervisor_type) - .map_err(DeviceManagerError::StartSigwinchListener)?, - )); - self.console_pty = Some(Arc::new(Mutex::new(PtyPair { main, path }))); - Endpoint::PtyPair(file.try_clone().unwrap(), file) + return Err(DeviceManagerError::InvalidConsoleFd); } } ConsoleOutputMode::Tty => { - // Duplicating the file descriptors like this is needed as otherwise - // they will be closed on a reboot and the numbers reused - - // SAFETY: FFI call to dup. Trivially safe. - let stdout = unsafe { libc::dup(libc::STDOUT_FILENO) }; - if stdout == -1 { - return vmm_sys_util::errno::errno_result().map_err(DeviceManagerError::DupFd); - } - // SAFETY: stdout is valid and owned solely by us. - let stdout = unsafe { File::from_raw_fd(stdout) }; - - // Make sure stdout is in raw mode, if it's a terminal. - let _ = set_raw_mode(&stdout, self.original_termios_opt.clone()); - - // SAFETY: FFI call. Trivially safe. - if unsafe { libc::isatty(libc::STDOUT_FILENO) } == 1 { - self.console_resize_pipe = Some(Arc::new( - listen_for_sigwinch_on_tty( - stdout.try_clone().unwrap(), - &self.seccomp_action, - self.hypervisor_type, - ) - .map_err(DeviceManagerError::StartSigwinchListener)?, - )); - } - - // If an interactive TTY then we can accept input - // SAFETY: FFI call. Trivially safe. - if unsafe { libc::isatty(libc::STDIN_FILENO) == 1 } { - // SAFETY: FFI call to dup. Trivially safe. - let stdin = unsafe { libc::dup(libc::STDIN_FILENO) }; - if stdin == -1 { - return vmm_sys_util::errno::errno_result() - .map_err(DeviceManagerError::DupFd); + if let Some(tty_fd) = console_fd { + // SAFETY: tty_fd is guaranteed to be a valid fd from + // pre_create_console_devices() in vmm/src/console_devices.rs + let stdout = unsafe { File::from_raw_fd(tty_fd) }; + // If an interactive TTY then we can accept input + // SAFETY: FFI call. Trivially safe. + if unsafe { libc::isatty(libc::STDIN_FILENO) == 1 } { + // SAFETY: FFI call to dup. Trivially safe. + let stdin = unsafe { libc::dup(libc::STDIN_FILENO) }; + if stdin == -1 { + return vmm_sys_util::errno::errno_result() + .map_err(DeviceManagerError::DupFd); + } + // SAFETY: stdin is valid and owned solely by us. + let stdin = unsafe { File::from_raw_fd(stdin) }; + Endpoint::FilePair(stdout, stdin) + } else { + Endpoint::File(stdout) } - // SAFETY: stdin is valid and owned solely by us. - let stdin = unsafe { File::from_raw_fd(stdin) }; - - Endpoint::FilePair(stdout, stdin) } else { - Endpoint::File(stdout) + return Err(DeviceManagerError::InvalidConsoleFd); } } ConsoleOutputMode::Socket => { @@ -2104,6 +2079,7 @@ impl DeviceManager { }) } + #[allow(clippy::too_many_arguments)] /// Adds all devices that behave like a console with respect to the VM /// configuration. This includes: /// - debug-console @@ -2113,38 +2089,36 @@ impl DeviceManager { &mut self, interrupt_manager: &Arc>, virtio_devices: &mut Vec, - serial_pty: Option, - console_pty: Option, - #[cfg(target_arch = "x86_64")] debug_console_pty: Option, + _serial_pty: Option, + _console_pty: Option, + #[cfg(target_arch = "x86_64")] _debug_console_pty: Option, #[cfg(not(target_arch = "x86_64"))] _: Option, + console_info: Option, console_resize_pipe: Option, ) -> DeviceManagerResult> { let serial_config = self.config.lock().unwrap().serial.clone(); + if console_info.is_none() { + return Err(DeviceManagerError::InvalidConsoleInfo); + } + + // SAFETY: console_info is Some, so it's safe to unwrap. + let console_info = console_info.unwrap(); let serial_writer: Option> = match serial_config.mode { - ConsoleOutputMode::File => Some(Box::new( - File::create(serial_config.file.as_ref().unwrap()) - .map_err(DeviceManagerError::SerialOutputFileOpen)?, - )), - ConsoleOutputMode::Pty => { - if let Some(pty) = serial_pty.clone() { - self.config.lock().unwrap().serial.file = Some(pty.path.clone()); - self.serial_pty = Some(Arc::new(Mutex::new(pty))); - } else { - let (main, sub, path) = - create_pty().map_err(DeviceManagerError::SerialPtyOpen)?; - set_raw_mode(&sub, self.original_termios_opt.clone()) - .map_err(DeviceManagerError::SetPtyRaw)?; - self.config.lock().unwrap().serial.file = Some(path.clone()); - self.serial_pty = Some(Arc::new(Mutex::new(PtyPair { main, path }))); + ConsoleOutputMode::File | ConsoleOutputMode::Tty => { + if console_info.serial_main_fd.is_none() { + return Err(DeviceManagerError::InvalidConsoleInfo); } - None + // SAFETY: serial_main_fd is Some, so it's safe to unwrap. + // SAFETY: serial_main_fd is guaranteed to be a valid fd from + // pre_create_console_devices() in vmm/src/console_devices.rs + Some(Box::new(unsafe { + File::from_raw_fd(console_info.serial_main_fd.unwrap()) + })) } - ConsoleOutputMode::Tty => { - let out = stdout(); - let _ = set_raw_mode(&out, self.original_termios_opt.clone()); - Some(Box::new(out)) - } - ConsoleOutputMode::Off | ConsoleOutputMode::Null | ConsoleOutputMode::Socket => None, + ConsoleOutputMode::Off + | ConsoleOutputMode::Null + | ConsoleOutputMode::Pty + | ConsoleOutputMode::Socket => None, }; if serial_config.mode != ConsoleOutputMode::Off { let serial = self.add_serial_device(interrupt_manager, serial_writer)?; @@ -2152,7 +2126,7 @@ impl DeviceManager { ConsoleOutputMode::Pty | ConsoleOutputMode::Tty | ConsoleOutputMode::Socket => { let serial_manager = SerialManager::new( serial, - self.serial_pty.clone(), + console_info.serial_main_fd, serial_config.mode, serial_config.socket, ) @@ -2177,43 +2151,34 @@ impl DeviceManager { #[cfg(target_arch = "x86_64")] { let debug_console_config = self.config.lock().unwrap().debug_console.clone(); - let debug_console_writer: Option> = match debug_console_config - .mode - { - ConsoleOutputMode::File => Some(Box::new( - File::create(debug_console_config.file.as_ref().unwrap()) - .map_err(DeviceManagerError::DebugconOutputFileOpen)?, - )), - ConsoleOutputMode::Pty => { - if let Some(pty) = debug_console_pty { - self.config.lock().unwrap().debug_console.file = Some(pty.path.clone()); - self.debug_console_pty = Some(Arc::new(Mutex::new(pty))); - } else { - let (main, sub, path) = - create_pty().map_err(DeviceManagerError::DebugconPtyOpen)?; - set_raw_mode(&sub, self.original_termios_opt.clone()) - .map_err(DeviceManagerError::SetPtyRaw)?; - self.config.lock().unwrap().debug_console.file = Some(path.clone()); - self.debug_console_pty = Some(Arc::new(Mutex::new(PtyPair { main, path }))); + let debug_console_writer: Option> = + match debug_console_config.mode { + ConsoleOutputMode::File | ConsoleOutputMode::Tty => { + if console_info.debug_main_fd.is_none() { + return Err(DeviceManagerError::InvalidConsoleInfo); + } + // SAFETY: debug_main_fd is Some, so it's safe to unwrap. + // SAFETY: debug_main_fd is guaranteed to be a valid fd from + // pre_create_console_devices() in vmm/src/console_devices.rs + Some(Box::new(unsafe { + File::from_raw_fd(console_info.debug_main_fd.unwrap()) + })) } - None - } - ConsoleOutputMode::Tty => { - let out = stdout(); - let _ = set_raw_mode(&out, self.original_termios_opt.clone()); - Some(Box::new(out)) - } - ConsoleOutputMode::Off | ConsoleOutputMode::Null | ConsoleOutputMode::Socket => { - None - } - }; + ConsoleOutputMode::Off + | ConsoleOutputMode::Null + | ConsoleOutputMode::Pty + | ConsoleOutputMode::Socket => None, + }; if let Some(writer) = debug_console_writer { let _ = self.add_debug_console_device(writer)?; } } - let console_resizer = - self.add_virtio_console_device(virtio_devices, console_pty, console_resize_pipe)?; + let console_resizer = self.add_virtio_console_device( + virtio_devices, + console_info.console_main_fd, + console_resize_pipe, + )?; Ok(Arc::new(Console { console_resizer })) } diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 6dfb088af..d4751efd2 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -813,6 +813,7 @@ impl Vmm { None, None, None, + None, Arc::clone(&self.original_termios_opt), Some(snapshot), ) @@ -1266,6 +1267,7 @@ impl RequestHandler for Vmm { None, None, None, + self.console_info.clone(), None, Arc::clone(&self.original_termios_opt), None, @@ -1395,6 +1397,7 @@ impl RequestHandler for Vmm { None, None, None, + self.console_info.clone(), None, Arc::clone(&self.original_termios_opt), Some(snapshot), @@ -1501,6 +1504,7 @@ impl RequestHandler for Vmm { serial_pty, console_pty, debug_console_pty, + self.console_info.clone(), console_resize_pipe, Arc::clone(&self.original_termios_opt), None, diff --git a/vmm/src/serial_manager.rs b/vmm/src/serial_manager.rs index 763e6bfae..d3c1069c5 100644 --- a/vmm/src/serial_manager.rs +++ b/vmm/src/serial_manager.rs @@ -4,7 +4,6 @@ // use crate::config::ConsoleOutputMode; -use crate::device_manager::PtyPair; #[cfg(target_arch = "aarch64")] use devices::legacy::Pl011; #[cfg(target_arch = "x86_64")] @@ -14,6 +13,7 @@ use serial_buffer::SerialBuffer; use std::fs::File; use std::io::Read; use std::net::Shutdown; +use std::os::fd::RawFd; use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd}; use std::os::unix::net::{UnixListener, UnixStream}; use std::panic::AssertUnwindSafe; @@ -123,7 +123,7 @@ impl SerialManager { pub fn new( #[cfg(target_arch = "x86_64")] serial: Arc>, #[cfg(target_arch = "aarch64")] serial: Arc>, - pty_pair: Option>>, + main_fd: Option, mode: ConsoleOutputMode, socket: Option, ) -> Result> { @@ -131,13 +131,10 @@ impl SerialManager { let in_file = match mode { ConsoleOutputMode::Pty => { - if let Some(pty_pair) = pty_pair { - pty_pair - .lock() - .unwrap() - .main - .try_clone() - .map_err(Error::FileClone)? + if let Some(pty_main) = main_fd { + // SAFETY: pty_main is guaranteed to be a valid fd from + // pre_create_console_devices() in vmm/src/console_devices.rs + unsafe { File::from_raw_fd(pty_main) } } else { return Ok(None); } @@ -170,12 +167,13 @@ impl SerialManager { } } ConsoleOutputMode::Socket => { - if let Some(path_in_socket) = socket { - socket_path = Some(path_in_socket.clone()); - let listener = UnixListener::bind(path_in_socket.as_path()) - .map_err(Error::BindUnixSocket)?; - // SAFETY: listener is valid and will return valid fd - unsafe { File::from_raw_fd(listener.into_raw_fd()) } + if let Some(socket_fd) = main_fd { + if let Some(path_in_socket) = socket { + socket_path = Some(path_in_socket.clone()); + } + // SAFETY: socke_fd is guaranteed to be a valid fd from + // pre_create_console_devices() in vmm/src/console_devices.rs + unsafe { File::from_raw_fd(socket_fd) } } else { return Ok(None); } diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs index 338a5274c..2df5cc5c5 100644 --- a/vmm/src/vm.rs +++ b/vmm/src/vm.rs @@ -16,7 +16,7 @@ use crate::config::{ UserDeviceConfig, ValidationError, VdpaConfig, VmConfig, VsockConfig, }; use crate::config::{NumaConfig, PayloadConfig}; -use crate::console_devices::ConsoleDeviceError; +use crate::console_devices::{ConsoleDeviceError, ConsoleInfo}; #[cfg(all(target_arch = "x86_64", feature = "guest_debug"))] use crate::coredump::{ CpuElf64Writable, DumpState, Elf64Writable, GuestDebuggable, GuestDebuggableError, NoteDescType, @@ -491,6 +491,7 @@ impl Vm { serial_pty: Option, console_pty: Option, debug_console_pty: Option, + console_info: Option, console_resize_pipe: Option, original_termios: Arc>>, snapshot: Option, @@ -627,7 +628,6 @@ impl Vm { #[cfg(target_arch = "x86_64")] io_bus, mmio_bus, - hypervisor.hypervisor_type(), vm.clone(), config.clone(), memory_manager.clone(), @@ -652,6 +652,7 @@ impl Vm { serial_pty, console_pty, debug_console_pty, + console_info, console_resize_pipe, original_termios, ) @@ -808,6 +809,7 @@ impl Vm { serial_pty: Option, console_pty: Option, debug_console_pty: Option, + console_info: Option, console_resize_pipe: Option, original_termios: Arc>>, snapshot: Option, @@ -888,6 +890,7 @@ impl Vm { serial_pty, console_pty, debug_console_pty, + console_info, console_resize_pipe, original_termios, snapshot,