From 0042f1de7579aa12e94961f3a39fa77d9de82f1f Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Wed, 22 Jan 2020 23:45:27 +0100 Subject: [PATCH] ioapic: Rely fully on the InterruptSourceGroup to manage interrupts This commit relies on the interrupt manager and the resulting interrupt source group to abstract the knowledge about KVM and how interrupts are updated and delivered. This allows the entire "devices" crate to be freed from kvm_ioctls and kvm_bindings dependencies. Signed-off-by: Sebastien Boeuf --- Cargo.lock | 2 - devices/Cargo.toml | 2 - devices/src/ioapic.rs | 111 +++++++++++++++++++++++--------------- devices/src/lib.rs | 2 - vmm/src/device_manager.rs | 6 +-- 5 files changed, 69 insertions(+), 54 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a952471e5..7903c07de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -226,8 +226,6 @@ dependencies = [ "bitflags 1.2.1 (registry+https://github.com/rust-lang/crates.io-index)", "byteorder 1.3.2 (registry+https://github.com/rust-lang/crates.io-index)", "epoll 4.1.0 (registry+https://github.com/rust-lang/crates.io-index)", - "kvm-bindings 0.2.0 (registry+https://github.com/rust-lang/crates.io-index)", - "kvm-ioctls 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)", "libc 0.2.66 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.10 (registry+https://github.com/rust-lang/crates.io-index)", "tempfile 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)", diff --git a/devices/Cargo.toml b/devices/Cargo.toml index 05ec20e84..e0c53e2b2 100644 --- a/devices/Cargo.toml +++ b/devices/Cargo.toml @@ -7,8 +7,6 @@ authors = ["The Chromium OS Authors"] bitflags = ">=1.2.1" byteorder = "1.3.2" epoll = ">=4.0.1" -kvm-bindings = "0.2.0" -kvm-ioctls = "0.4.0" libc = "0.2.60" log = "0.4.8" vm-device = { path = "../vm-device" } diff --git a/devices/src/ioapic.rs b/devices/src/ioapic.rs index 97a8c87ea..f84c46446 100644 --- a/devices/src/ioapic.rs +++ b/devices/src/ioapic.rs @@ -11,18 +11,17 @@ use crate::BusDevice; use byteorder::{ByteOrder, LittleEndian}; -use kvm_bindings::kvm_msi; -use kvm_ioctls::VmFd; use std::io; use std::result; use std::sync::Arc; -use vm_device::interrupt::{InterruptIndex, InterruptManager, InterruptSourceGroup, PCI_MSI_IRQ}; +use vm_device::interrupt::{ + InterruptIndex, InterruptManager, InterruptSourceConfig, InterruptSourceGroup, + MsiIrqSourceConfig, PCI_MSI_IRQ, +}; use vm_memory::GuestAddress; #[derive(Debug)] pub enum Error { - /// Failed to send an interrupt. - InterruptFailed(kvm_ioctls::Error), /// Invalid destination mode. InvalidDestinationMode, /// Invalid trigger mode. @@ -31,6 +30,16 @@ pub enum Error { InvalidDeliveryMode, /// Failed creating the interrupt source group. CreateInterruptSourceGroup(io::Error), + /// Failed triggering the interrupt. + TriggerInterrupt(io::Error), + /// Failed masking the interrupt. + MaskInterrupt(io::Error), + /// Failed unmasking the interrupt. + UnmaskInterrupt(io::Error), + /// Failed updating the interrupt. + UpdateInterrupt(io::Error), + /// Failed enabling the interrupt. + EnableInterrupt(io::Error), } type Result = result::Result; @@ -160,9 +169,8 @@ pub struct Ioapic { id: u32, reg_sel: u32, reg_entries: [RedirectionTableEntry; NUM_IOAPIC_PINS], - vm_fd: Arc, apic_address: GuestAddress, - _interrupt_source_group: Arc>, + interrupt_source_group: Arc>, } impl BusDevice for Ioapic { @@ -202,7 +210,6 @@ impl BusDevice for Ioapic { impl Ioapic { pub fn new( - vm_fd: Arc, apic_address: GuestAddress, interrupt_manager: Arc, ) -> Result { @@ -214,13 +221,16 @@ impl Ioapic { ) .map_err(Error::CreateInterruptSourceGroup)?; + interrupt_source_group + .enable() + .map_err(Error::EnableInterrupt)?; + Ok(Ioapic { id: 0, reg_sel: 0, reg_entries: [0; NUM_IOAPIC_PINS], - vm_fd, apic_address, - _interrupt_source_group: interrupt_source_group, + interrupt_source_group, }) } @@ -241,16 +251,30 @@ impl Ioapic { pub fn service_irq(&mut self, irq: usize) -> Result<()> { let entry = &mut self.reg_entries[irq]; - // Don't inject the interrupt if the IRQ is masked - if interrupt_mask(*entry) == 1 { - return Ok(()); + self.interrupt_source_group + .trigger(irq as InterruptIndex) + .map_err(Error::TriggerInterrupt)?; + debug!("Interrupt successfully delivered"); + + // If trigger mode is level sensitive, set the Remote IRR bit. + // It will be cleared when the EOI is received. + if trigger_mode(*entry) == 1 { + set_remote_irr(entry, 1); } + // Clear the Delivery Status bit + set_delivery_status(entry, 0); + + Ok(()) + } + + fn update_entry(&self, irq: usize) -> Result<()> { + let entry = self.reg_entries[irq]; // Validate Destination Mode value, and retrieve Destination ID - let destination_mode = destination_mode(*entry); + let destination_mode = destination_mode(entry); let destination_id: u8 = match destination_mode { - x if x == DestinationMode::Physical as u8 => destination_field_physical(*entry), - x if x == DestinationMode::Logical as u8 => destination_field_logical(*entry), + x if x == DestinationMode::Physical as u8 => destination_field_physical(entry), + x if x == DestinationMode::Logical as u8 => destination_field_logical(entry), _ => return Err(Error::InvalidDestinationMode), }; @@ -260,20 +284,20 @@ impl Ioapic { let redirection_hint: u8 = 1; // Generate MSI message address - let address_lo: u32 = self.apic_address.0 as u32 + let low_addr: u32 = self.apic_address.0 as u32 | u32::from(destination_id) << 12 | u32::from(redirection_hint) << 3 | u32::from(destination_mode) << 2; // Validate Trigger Mode value - let trigger_mode = trigger_mode(*entry); + let trigger_mode = trigger_mode(entry); match trigger_mode { x if (x == TriggerMode::Edge as u8) || (x == TriggerMode::Level as u8) => {} _ => return Err(Error::InvalidTriggerMode), } // Validate Delivery Mode value - let delivery_mode = delivery_mode(*entry); + let delivery_mode = delivery_mode(entry); match delivery_mode { x if (x == DeliveryMode::Fixed as u8) || (x == DeliveryMode::Lowest as u8) @@ -288,37 +312,31 @@ impl Ioapic { // Generate MSI message data let data: u32 = u32::from(trigger_mode) << 15 - | u32::from(remote_irr(*entry)) << 14 + | u32::from(remote_irr(entry)) << 14 | u32::from(delivery_mode) << 8 - | u32::from(vector(*entry)); + | u32::from(vector(entry)); - let msi = kvm_msi { - address_lo, - address_hi: 0x0, + let config = MsiIrqSourceConfig { + high_addr: 0x0, + low_addr, data, - flags: 0u32, - devid: 0u32, - pad: [0u8; 12], }; - match self.vm_fd.signal_msi(msi) { - Ok(ret) => { - if ret > 0 { - debug!("MSI message successfully delivered"); - // If trigger mode is level sensitive, set the Remote IRR bit. - // It will be cleared when the EOI is received. - if trigger_mode == 1 { - set_remote_irr(entry, 1); - } - // Clear the Delivery Status bit - set_delivery_status(entry, 0); - } else { - warn!("failed to deliver MSI message, blocked by guest"); - } - Ok(()) - } - Err(e) => Err(Error::InterruptFailed(e)), + self.interrupt_source_group + .update(irq as InterruptIndex, InterruptSourceConfig::MsiIrq(config)) + .map_err(Error::UpdateInterrupt)?; + + if interrupt_mask(entry) == 1 { + self.interrupt_source_group + .mask(irq as InterruptIndex) + .map_err(Error::MaskInterrupt)?; + } else { + self.interrupt_source_group + .unmask(irq as InterruptIndex) + .map_err(Error::UnmaskInterrupt)?; } + + Ok(()) } fn ioapic_write(&mut self, val: u32) { @@ -338,6 +356,11 @@ impl Ioapic { self.reg_entries[index] &= 0xffff_ffff_0000_5000; self.reg_entries[index] |= u64::from(val) & 0xffff_afff; } + // The entry must be updated through the interrupt source + // group. + if let Err(e) = self.update_entry(index) { + error!("Failed updating IOAPIC entry: {:?}", e); + } } _ => error!("IOAPIC: invalid write to register offset"), } diff --git a/devices/src/lib.rs b/devices/src/lib.rs index 41b9800bd..9d6f5a17c 100644 --- a/devices/src/lib.rs +++ b/devices/src/lib.rs @@ -10,8 +10,6 @@ extern crate bitflags; extern crate byteorder; extern crate epoll; -extern crate kvm_bindings; -extern crate kvm_ioctls; extern crate libc; #[macro_use] extern crate log; diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 6ccfec812..08966eb7f 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -451,8 +451,7 @@ impl DeviceManager { None, )); - let ioapic = - DeviceManager::add_ioapic(vm_info, &address_manager, ioapic_interrupt_manager)?; + let ioapic = DeviceManager::add_ioapic(&address_manager, ioapic_interrupt_manager)?; // Creation of the global interrupt manager, which can take a hold onto // the brand new Ioapic. @@ -698,13 +697,12 @@ impl DeviceManager { } fn add_ioapic( - vm_info: &VmInfo, address_manager: &Arc, interrupt_manager: Arc, ) -> DeviceManagerResult>> { // Create IOAPIC let ioapic = Arc::new(Mutex::new( - ioapic::Ioapic::new(vm_info.vm_fd.clone(), APIC_START, interrupt_manager) + ioapic::Ioapic::new(APIC_START, interrupt_manager) .map_err(DeviceManagerError::CreateIoapic)?, ));