From 3b4fbfa799889417e1d0f4c45196c0123acd7a93 Mon Sep 17 00:00:00 2001 From: Elie Kheirallah Date: Thu, 12 Oct 2023 21:51:59 +0000 Subject: [PATCH] devices: bus: Add BusType to Bus for better debugging. With snapshot/restore, we are snapshotting and restoring all Bus devices on crosvm (MMIO and IO). However there's no message differentiating which bus was snapshotting/restoring, and the message only displays that the action succeeded. Add the type to Bus device and to the logs. BUG=N/A TEST=presubmit Change-Id: Iec4f97f6f7502bfadc96026cc90710c97d351a83 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4936779 Commit-Queue: Elie Kheirallah Reviewed-by: Frederick Mayle Reviewed-by: Daniel Verkamp --- aarch64/src/lib.rs | 5 ++-- devices/src/bus.rs | 21 ++++++++++------ devices/src/lib.rs | 39 +++++++++++++++++++++-------- devices/src/pci/pci_root.rs | 4 +-- devices/tests/irqchip/kvm/x86_64.rs | 9 ++++--- devices/tests/irqchip/userspace.rs | 9 ++++--- riscv64/src/lib.rs | 5 ++-- src/crosvm/sys/linux.rs | 11 ++++++-- x86_64/src/lib.rs | 22 ++++++++-------- x86_64/tests/integration/main.rs | 6 +++-- 10 files changed, 86 insertions(+), 45 deletions(-) diff --git a/aarch64/src/lib.rs b/aarch64/src/lib.rs index 8e084e15ac..60dfb8961a 100644 --- a/aarch64/src/lib.rs +++ b/aarch64/src/lib.rs @@ -28,6 +28,7 @@ use devices::vmwdt::VMWDT_DEFAULT_TIMEOUT_SEC; use devices::Bus; use devices::BusDeviceObj; use devices::BusError; +use devices::BusType; use devices::IrqChip; use devices::IrqChipAArch64; use devices::IrqEventSource; @@ -532,10 +533,10 @@ impl arch::LinuxArch for AArch64 { } } - let mmio_bus = Arc::new(devices::Bus::new()); + let mmio_bus = Arc::new(devices::Bus::new(BusType::Mmio)); // ARM doesn't really use the io bus like x86, so just create an empty bus. - let io_bus = Arc::new(devices::Bus::new()); + let io_bus = Arc::new(devices::Bus::new(BusType::Io)); // Event used by PMDevice to notify crosvm that // guest OS is trying to suspend. diff --git a/devices/src/bus.rs b/devices/src/bus.rs index c7eb3ee9e2..0cd3016320 100644 --- a/devices/src/bus.rs +++ b/devices/src/bus.rs @@ -342,19 +342,26 @@ pub struct Bus { access_id: usize, #[cfg(feature = "stats")] pub stats: Arc>, + bus_type: BusType, } impl Bus { /// Constructs an a bus with an empty address space. - pub fn new() -> Bus { + pub fn new(bus_type: BusType) -> Bus { Bus { devices: Arc::new(Mutex::new(BTreeMap::new())), access_id: 0, #[cfg(feature = "stats")] stats: Arc::new(Mutex::new(BusStatistics::new())), + bus_type, } } + /// Gets the bus type + pub fn get_bus_type(&self) -> BusType { + self.bus_type + } + /// Sets the id that will be used for BusAccessInfo. pub fn set_access_id(&mut self, id: usize) { self.access_id = id; @@ -702,7 +709,7 @@ impl Bus { impl Default for Bus { fn default() -> Self { - Self::new() + Self::new(BusType::Io) } } @@ -808,7 +815,7 @@ mod tests { #[test] fn bus_insert() { - let bus = Bus::new(); + let bus = Bus::new(BusType::Io); let dummy = Arc::new(Mutex::new(DummyDevice)); assert!(bus.insert(dummy.clone(), 0x10, 0).is_err()); assert!(bus.insert(dummy.clone(), 0x10, 0x10).is_ok()); @@ -825,7 +832,7 @@ mod tests { #[test] fn bus_insert_full_addr() { - let bus = Bus::new(); + let bus = Bus::new(BusType::Io); let dummy = Arc::new(Mutex::new(DummyDevice)); assert!(bus.insert(dummy.clone(), 0x10, 0).is_err()); assert!(bus.insert(dummy.clone(), 0x10, 0x10).is_ok()); @@ -842,7 +849,7 @@ mod tests { #[test] fn bus_read_write() { - let bus = Bus::new(); + let bus = Bus::new(BusType::Io); let dummy = Arc::new(Mutex::new(DummyDevice)); assert!(bus.insert(dummy, 0x10, 0x10).is_ok()); assert!(bus.read(0x10, &mut [0, 0, 0, 0])); @@ -859,7 +866,7 @@ mod tests { #[test] fn bus_read_write_values() { - let bus = Bus::new(); + let bus = Bus::new(BusType::Io); let dummy = Arc::new(Mutex::new(ConstantDevice { uses_full_addr: false, })); @@ -876,7 +883,7 @@ mod tests { #[test] fn bus_read_write_full_addr_values() { - let bus = Bus::new(); + let bus = Bus::new(BusType::Io); let dummy = Arc::new(Mutex::new(ConstantDevice { uses_full_addr: true, })); diff --git a/devices/src/lib.rs b/devices/src/lib.rs index 91150ce7fc..34565c0b10 100644 --- a/devices/src/lib.rs +++ b/devices/src/lib.rs @@ -228,12 +228,13 @@ pub fn create_devices_worker_thread( fn sleep_devices(bus: &Bus) -> anyhow::Result<()> { match bus.sleep_devices() { Ok(_) => { - info!("Devices slept successfully"); + info!("Devices slept successfully on {:?} Bus", bus.get_bus_type()); Ok(()) } Err(e) => Err(anyhow!( - "Failed to sleep all devices: {}. Waking up sleeping devices.", - e + "Failed to sleep all devices: {} on {:?} Bus. Waking up sleeping devices.", + e, + bus.get_bus_type(), )), } } @@ -241,15 +242,19 @@ fn sleep_devices(bus: &Bus) -> anyhow::Result<()> { fn wake_devices(bus: &Bus) { match bus.wake_devices() { Ok(_) => { - info!("Devices awoken successfully"); + info!( + "Devices awoken successfully on {:?} Bus", + bus.get_bus_type() + ); } Err(e) => { // Some devices may have slept. Eternally. // Recovery - impossible. // Shut down VM. panic!( - "Failed to wake devices: {}. VM panicked to avoid unexpected behavior", - e + "Failed to wake devices: {} on {:?} Bus. VM panicked to avoid unexpected behavior", + e, + bus.get_bus_type(), ) } } @@ -296,12 +301,19 @@ fn snapshot_devices( ) -> anyhow::Result<()> { match bus.snapshot_devices(add_snapshot) { Ok(_) => { - info!("Devices snapshot successfully"); + info!( + "Devices snapshot successfully for {:?} Bus", + bus.get_bus_type() + ); Ok(()) } Err(e) => { // If snapshot fails, wake devices and return error - error!("failed to snapshot devices: {}", e); + error!( + "failed to snapshot devices: {:#} on {:?} Bus", + e, + bus.get_bus_type() + ); Err(e) } } @@ -313,12 +325,19 @@ fn restore_devices( ) -> anyhow::Result<()> { match bus.restore_devices(devices_map) { Ok(_) => { - info!("Devices restore successfully"); + info!( + "Devices restore successfully for {:?} Bus", + bus.get_bus_type() + ); Ok(()) } Err(e) => { // If restore fails, wake devices and return error - error!("failed to restore devices: {:#}", e); + error!( + "failed to restore devices: {:#} on {:?} Bus", + e, + bus.get_bus_type() + ); Err(e) } } diff --git a/devices/src/pci/pci_root.rs b/devices/src/pci/pci_root.rs index d77e572388..a8f1da9274 100644 --- a/devices/src/pci/pci_root.rs +++ b/devices/src/pci/pci_root.rs @@ -758,8 +758,8 @@ mod tests { use crate::suspendable_tests; fn create_pci_root() -> Arc> { - let io_bus = Arc::new(Bus::new()); - let mmio_bus = Arc::new(Bus::new()); + let io_bus = Arc::new(Bus::new(BusType::Io)); + let mmio_bus = Arc::new(Bus::new(BusType::Mmio)); let root_bus = Arc::new(Mutex::new(PciBus::new(0, 0, false))); Arc::new(Mutex::new(PciRoot::new( diff --git a/devices/tests/irqchip/kvm/x86_64.rs b/devices/tests/irqchip/kvm/x86_64.rs index 6649cf410e..af430f7867 100644 --- a/devices/tests/irqchip/kvm/x86_64.rs +++ b/devices/tests/irqchip/kvm/x86_64.rs @@ -7,6 +7,7 @@ use base::EventWaitResult; use base::Tube; use devices::Bus; +use devices::BusType; use devices::CrosvmDeviceId; use devices::DeviceId; use devices::IrqChip; @@ -233,8 +234,8 @@ fn irq_event_tokens() { fn finalize_devices() { let mut chip = get_split_chip(); - let mmio_bus = Bus::new(); - let io_bus = Bus::new(); + let mmio_bus = Bus::new(BusType::Mmio); + let io_bus = Bus::new(BusType::Io); let mut resources = SystemAllocator::new( SystemAllocatorConfig { io: Some(AddressRange { @@ -372,8 +373,8 @@ fn get_external_interrupt() { fn broadcast_eoi() { let mut chip = get_split_chip(); - let mmio_bus = Bus::new(); - let io_bus = Bus::new(); + let mmio_bus = Bus::new(BusType::Mmio); + let io_bus = Bus::new(BusType::Io); let mut resources = SystemAllocator::new( SystemAllocatorConfig { io: Some(AddressRange { diff --git a/devices/tests/irqchip/userspace.rs b/devices/tests/irqchip/userspace.rs index 5166f0b472..8889e15c1e 100644 --- a/devices/tests/irqchip/userspace.rs +++ b/devices/tests/irqchip/userspace.rs @@ -16,6 +16,7 @@ use base::Tube; use devices::Bus; use devices::BusAccessInfo; use devices::BusDeviceSync; +use devices::BusType; use devices::CrosvmDeviceId; use devices::DestinationShorthand; use devices::DeviceId; @@ -203,8 +204,8 @@ fn irq_event_tokens() { fn finalize_devices() { let mut chip = get_chip(1); - let mmio_bus = Bus::new(); - let io_bus = Bus::new(); + let mmio_bus = Bus::new(BusType::Mmio); + let io_bus = Bus::new(BusType::Io); let mut resources = SystemAllocator::new( SystemAllocatorConfig { io: Some(AddressRange { @@ -459,8 +460,8 @@ fn lowest_priority_destination() { fn broadcast_eoi() { let mut chip = get_chip(1); - let mmio_bus = Bus::new(); - let io_bus = Bus::new(); + let mmio_bus = Bus::new(BusType::Mmio); + let io_bus = Bus::new(BusType::Io); let mut resources = SystemAllocator::new( SystemAllocatorConfig { io: Some(AddressRange { diff --git a/riscv64/src/lib.rs b/riscv64/src/lib.rs index f8f3b0b051..8c5a34f510 100644 --- a/riscv64/src/lib.rs +++ b/riscv64/src/lib.rs @@ -24,6 +24,7 @@ use devices::serial_device::SerialParameters; use devices::Bus; use devices::BusDeviceObj; use devices::BusError; +use devices::BusType; use devices::IrqChipRiscv64; use devices::PciAddress; use devices::PciConfigMmio; @@ -202,10 +203,10 @@ impl arch::LinuxArch for Riscv64 { let mem = vm.get_memory().clone(); - let mmio_bus = Arc::new(Bus::new()); + let mmio_bus = Arc::new(Bus::new(BusType::Mmio)); // Riscv doesn't really use the io bus like x86, so just create an empty bus. - let io_bus = Arc::new(Bus::new()); + let io_bus = Arc::new(Bus::new(BusType::Io)); let com_evt_1_3 = Event::new().map_err(Error::CreateEvent)?; let com_evt_2_4 = Event::new().map_err(Error::CreateEvent)?; diff --git a/src/crosvm/sys/linux.rs b/src/crosvm/sys/linux.rs index e0857a4acb..9cc6022999 100644 --- a/src/crosvm/sys/linux.rs +++ b/src/crosvm/sys/linux.rs @@ -98,6 +98,7 @@ use devices::virtio::VirtioDeviceType; use devices::virtio::VirtioTransportType; use devices::Bus; use devices::BusDeviceObj; +use devices::BusType; use devices::CoIommuDev; #[cfg(feature = "usb")] use devices::DeviceProvider; @@ -3558,11 +3559,17 @@ fn run_control( // inside `linux`. If the checks below fail, then some other thread is probably still running // and needs to be explicitly stopped before dropping `linux` to ensure devices actually get // cleaned up. - match Arc::try_unwrap(std::mem::replace(&mut linux.mmio_bus, Arc::new(Bus::new()))) { + match Arc::try_unwrap(std::mem::replace( + &mut linux.mmio_bus, + Arc::new(Bus::new(BusType::Mmio)), + )) { Ok(_) => {} Err(_) => panic!("internal error: mmio_bus had more than one reference at shutdown"), } - match Arc::try_unwrap(std::mem::replace(&mut linux.io_bus, Arc::new(Bus::new()))) { + match Arc::try_unwrap(std::mem::replace( + &mut linux.io_bus, + Arc::new(Bus::new(BusType::Io)), + )) { Ok(_) => {} Err(_) => panic!("internal error: io_bus had more than one reference at shutdown"), } diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs index f9169d48d8..2bb03d0ebf 100644 --- a/x86_64/src/lib.rs +++ b/x86_64/src/lib.rs @@ -69,9 +69,11 @@ use base::TubeError; use chrono::Utc; pub use cpuid::adjust_cpuid; pub use cpuid::CpuIdContext; +use devices::Bus; use devices::BusDevice; use devices::BusDeviceObj; use devices::BusResumeDevice; +use devices::BusType; use devices::Debugcon; use devices::FwCfgParameters; use devices::IrqChip; @@ -727,8 +729,8 @@ impl arch::LinuxArch for X8664arch { } let pcie_vcfg_range = Self::get_pcie_vcfg_mmio_range(&mem, &pcie_cfg_mmio_range); - let mmio_bus = Arc::new(devices::Bus::new()); - let io_bus = Arc::new(devices::Bus::new()); + let mmio_bus = Arc::new(Bus::new(BusType::Mmio)); + let io_bus = Arc::new(Bus::new(BusType::Io)); let (pci_devices, devs): (Vec<_>, Vec<_>) = devs .into_iter() @@ -1519,7 +1521,7 @@ impl X8664arch { pflash_image: File, block_size: u32, bios_size: u64, - mmio_bus: &devices::Bus, + mmio_bus: &Bus, jail: Option, #[cfg(feature = "swap")] swap_controller: &mut Option, ) -> Result<()> { @@ -1705,7 +1707,7 @@ impl X8664arch { /// * - `fw_cfg_parameters` - command-line specified data to add to device. May contain /// all None fields if user did not specify data to add to the device fn setup_fw_cfg_device( - io_bus: &devices::Bus, + io_bus: &Bus, fw_cfg_parameters: Vec, bootorder_fw_cfg_blob: Vec, fw_cfg_jail: Option, @@ -1775,7 +1777,7 @@ impl X8664arch { /// * - `pit_uses_speaker_port` - does the PIT use port 0x61 for the PC speaker /// * - `vm_evt_wrtube` - the event object which should receive exit events pub fn setup_legacy_i8042_device( - io_bus: &devices::Bus, + io_bus: &Bus, pit_uses_speaker_port: bool, vm_evt_wrtube: SendTube, ) -> Result<()> { @@ -1798,7 +1800,7 @@ impl X8664arch { /// * - `io_bus` - the IO bus object /// * - `mem_size` - the size in bytes of physical ram for the guest pub fn setup_legacy_cmos_device( - io_bus: &devices::Bus, + io_bus: &Bus, irq_chip: &mut dyn IrqChipX86_64, vm_control: Tube, mem_size: u64, @@ -1860,7 +1862,7 @@ impl X8664arch { pub fn setup_acpi_devices( pci_root: Arc>, mem: &GuestMemory, - io_bus: &devices::Bus, + io_bus: &Bus, resources: &mut SystemAllocator, suspend_evt: Event, vm_evt_wrtube: SendTube, @@ -1868,7 +1870,7 @@ impl X8664arch { irq_chip: &mut dyn IrqChip, sci_irq: u32, battery: (Option, Option), - #[cfg_attr(windows, allow(unused_variables))] mmio_bus: &devices::Bus, + #[cfg_attr(windows, allow(unused_variables))] mmio_bus: &Bus, max_bus: u8, resume_notify_devices: &mut Vec>>, #[cfg(feature = "swap")] swap_controller: &mut Option, @@ -2128,7 +2130,7 @@ impl X8664arch { pub fn setup_serial_devices( protection_type: ProtectionType, irq_chip: &mut dyn IrqChip, - io_bus: &devices::Bus, + io_bus: &Bus, serial_parameters: &BTreeMap<(SerialHardware, u8), SerialParameters>, serial_jail: Option, #[cfg(feature = "swap")] swap_controller: &mut Option, @@ -2165,7 +2167,7 @@ impl X8664arch { fn setup_debugcon_devices( protection_type: ProtectionType, - io_bus: &devices::Bus, + io_bus: &Bus, serial_parameters: &BTreeMap<(SerialHardware, u8), SerialParameters>, debugcon_jail: Option, #[cfg(feature = "swap")] swap_controller: &mut Option, diff --git a/x86_64/tests/integration/main.rs b/x86_64/tests/integration/main.rs index 3bf4194b16..9c3625e9fc 100644 --- a/x86_64/tests/integration/main.rs +++ b/x86_64/tests/integration/main.rs @@ -16,6 +16,8 @@ use arch::LinuxArch; use arch::SmbiosOptions; use base::Event; use base::Tube; +use devices::Bus; +use devices::BusType; use devices::IrqChipX86_64; use devices::PciConfigIo; use hypervisor::CpuConfigX86_64; @@ -107,8 +109,8 @@ where let mut irq_chip = create_irq_chip(vm.try_clone().expect("failed to clone vm"), 1, device_tube); - let mmio_bus = Arc::new(devices::Bus::new()); - let io_bus = Arc::new(devices::Bus::new()); + let mmio_bus = Arc::new(Bus::new(BusType::Mmio)); + let io_bus = Arc::new(Bus::new(BusType::Io)); let (exit_evt_wrtube, _) = Tube::directional_pair().unwrap(); let mut control_tubes = vec![TaggedControlTube::VmIrq(irqchip_tube)];