From ed87e42e6f79f8ca7f8ac25c79f5f121b6fa3182 Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Mon, 11 Apr 2022 16:21:20 +0100 Subject: [PATCH] vm-device, pci, devices: Remove InterruptSourceGroup::{un}mask The calls to these functions are always preceded by a call to InterruptSourceGroup::update(). By adding a masked boolean to that function call it possible to remove 50% of the calls to the KVM_SET_GSI_ROUTING ioctl as the the update will correctly handle the masked or unmasked case. This causes the ioctl to disappear from the perf report for a boot of the VM. Signed-off-by: Rob Bradford --- devices/src/gic.rs | 1 + devices/src/ioapic.rs | 16 +++----- devices/src/legacy/gpio_pl061.rs | 1 + devices/src/legacy/rtc_pl031.rs | 1 + devices/src/legacy/serial.rs | 1 + devices/src/legacy/uart_pl011.rs | 1 + pci/src/msi.rs | 17 +++------ pci/src/msix.rs | 33 ++++++---------- vm-device/src/interrupt/mod.rs | 22 ++++------- vmm/src/interrupt.rs | 64 ++++++++++---------------------- 10 files changed, 52 insertions(+), 105 deletions(-) diff --git a/devices/src/gic.rs b/devices/src/gic.rs index f3d5bd5f6..4613ee0fa 100644 --- a/devices/src/gic.rs +++ b/devices/src/gic.rs @@ -76,6 +76,7 @@ impl InterruptController for Gic { .update( i as InterruptIndex, InterruptSourceConfig::LegacyIrq(config), + false, ) .map_err(Error::EnableInterrupt)?; } diff --git a/devices/src/ioapic.rs b/devices/src/ioapic.rs index 08b2355e9..8b072946c 100644 --- a/devices/src/ioapic.rs +++ b/devices/src/ioapic.rs @@ -367,19 +367,13 @@ impl Ioapic { }; self.interrupt_source_group - .update(irq as InterruptIndex, InterruptSourceConfig::MsiIrq(config)) + .update( + irq as InterruptIndex, + InterruptSourceConfig::MsiIrq(config), + interrupt_mask(entry) == 1, + ) .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(()) } } diff --git a/devices/src/legacy/gpio_pl061.rs b/devices/src/legacy/gpio_pl061.rs index 90a11323d..ef2bbf425 100644 --- a/devices/src/legacy/gpio_pl061.rs +++ b/devices/src/legacy/gpio_pl061.rs @@ -356,6 +356,7 @@ mod tests { &self, _index: InterruptIndex, _config: InterruptSourceConfig, + _masked: bool, ) -> result::Result<(), std::io::Error> { Ok(()) } diff --git a/devices/src/legacy/rtc_pl031.rs b/devices/src/legacy/rtc_pl031.rs index ca4fe6398..b07b009e1 100644 --- a/devices/src/legacy/rtc_pl031.rs +++ b/devices/src/legacy/rtc_pl031.rs @@ -430,6 +430,7 @@ mod tests { &self, _index: InterruptIndex, _config: InterruptSourceConfig, + _masked: bool, ) -> result::Result<(), std::io::Error> { Ok(()) } diff --git a/devices/src/legacy/serial.rs b/devices/src/legacy/serial.rs index ff4074a27..2fb528159 100644 --- a/devices/src/legacy/serial.rs +++ b/devices/src/legacy/serial.rs @@ -339,6 +339,7 @@ mod tests { &self, _index: InterruptIndex, _config: InterruptSourceConfig, + _masked: bool, ) -> result::Result<(), std::io::Error> { Ok(()) } diff --git a/devices/src/legacy/uart_pl011.rs b/devices/src/legacy/uart_pl011.rs index 04def48c3..1db16ecf5 100644 --- a/devices/src/legacy/uart_pl011.rs +++ b/devices/src/legacy/uart_pl011.rs @@ -450,6 +450,7 @@ mod tests { &self, _index: InterruptIndex, _config: InterruptSourceConfig, + _masked: bool, ) -> result::Result<(), std::io::Error> { Ok(()) } diff --git a/pci/src/msi.rs b/pci/src/msi.rs index bd2ec355f..fe94d691c 100644 --- a/pci/src/msi.rs +++ b/pci/src/msi.rs @@ -201,20 +201,13 @@ impl MsiConfig { devid: 0, }; - if let Err(e) = self - .interrupt_source_group - .update(idx as InterruptIndex, InterruptSourceConfig::MsiIrq(config)) - { + if let Err(e) = self.interrupt_source_group.update( + idx as InterruptIndex, + InterruptSourceConfig::MsiIrq(config), + self.cap.vector_masked(idx), + ) { error!("Failed updating vector: {:?}", e); } - - if self.cap.vector_masked(idx) { - if let Err(e) = self.interrupt_source_group.mask(idx as InterruptIndex) { - error!("Failed masking vector: {:?}", e); - } - } else if let Err(e) = self.interrupt_source_group.unmask(idx as InterruptIndex) { - error!("Failed unmasking vector: {:?}", e); - } } if !old_enabled { diff --git a/pci/src/msix.rs b/pci/src/msix.rs index 73a1fef0d..c9d72c95e 100644 --- a/pci/src/msix.rs +++ b/pci/src/msix.rs @@ -132,7 +132,11 @@ impl MsixConfig { }; self.interrupt_source_group - .update(idx as InterruptIndex, InterruptSourceConfig::MsiIrq(config)) + .update( + idx as InterruptIndex, + InterruptSourceConfig::MsiIrq(config), + self.masked, + ) .map_err(Error::UpdateInterruptRoute)?; self.interrupt_source_group @@ -171,21 +175,13 @@ impl MsixConfig { devid: self.devid, }; - if let Err(e) = self - .interrupt_source_group - .update(idx as InterruptIndex, InterruptSourceConfig::MsiIrq(config)) - { + if let Err(e) = self.interrupt_source_group.update( + idx as InterruptIndex, + InterruptSourceConfig::MsiIrq(config), + table_entry.masked(), + ) { error!("Failed updating vector: {:?}", e); } - - if table_entry.masked() { - if let Err(e) = self.interrupt_source_group.mask(idx as InterruptIndex) { - error!("Failed masking vector: {:?}", e); - } - } else if let Err(e) = self.interrupt_source_group.unmask(idx as InterruptIndex) - { - error!("Failed unmasking vector: {:?}", e); - } } } else if old_enabled || !old_masked { debug!("MSI-X disabled for device 0x{:x}", self.devid); @@ -314,17 +310,10 @@ impl MsixConfig { if let Err(e) = self.interrupt_source_group.update( index as InterruptIndex, InterruptSourceConfig::MsiIrq(config), + table_entry.masked(), ) { error!("Failed updating vector: {:?}", e); } - - if table_entry.masked() { - if let Err(e) = self.interrupt_source_group.mask(index as InterruptIndex) { - error!("Failed masking vector: {:?}", e); - } - } else if let Err(e) = self.interrupt_source_group.unmask(index as InterruptIndex) { - error!("Failed unmasking vector: {:?}", e); - } } // After the MSI-X table entry has been updated, it is necessary to diff --git a/vm-device/src/interrupt/mod.rs b/vm-device/src/interrupt/mod.rs index b73821e31..7c44404de 100644 --- a/vm-device/src/interrupt/mod.rs +++ b/vm-device/src/interrupt/mod.rs @@ -178,19 +178,11 @@ pub trait InterruptSourceGroup: Send + Sync { /// # Arguments /// * index: sub-index into the group. /// * config: configuration data for the interrupt source. - fn update(&self, index: InterruptIndex, config: InterruptSourceConfig) -> Result<()>; - - /// Mask an interrupt from this interrupt source. - fn mask(&self, _index: InterruptIndex) -> Result<()> { - // Not all interrupt sources can be disabled. - // To accommodate this, we can have a no-op here. - Ok(()) - } - - /// Unmask an interrupt from this interrupt source. - fn unmask(&self, _index: InterruptIndex) -> Result<()> { - // Not all interrupt sources can be disabled. - // To accommodate this, we can have a no-op here. - Ok(()) - } + /// * masked: if the interrupt is masked + fn update( + &self, + index: InterruptIndex, + config: InterruptSourceConfig, + masked: bool, + ) -> Result<()>; } diff --git a/vmm/src/interrupt.rs b/vmm/src/interrupt.rs index 14b1d590f..a775fedf6 100644 --- a/vmm/src/interrupt.rs +++ b/vmm/src/interrupt.rs @@ -165,9 +165,20 @@ impl InterruptSourceGroup for MsiInterruptGroup { None } - fn update(&self, index: InterruptIndex, config: InterruptSourceConfig) -> Result<()> { + fn update( + &self, + index: InterruptIndex, + config: InterruptSourceConfig, + masked: bool, + ) -> Result<()> { if let Some(route) = self.irq_routes.get(&index) { - let entry = RoutingEntry::<_>::make_entry(&self.vm, route.gsi, &config)?; + let mut entry = RoutingEntry::<_>::make_entry(&self.vm, route.gsi, &config)?; + entry.masked = masked; + if masked { + route.disable(&self.vm)?; + } else { + route.enable(&self.vm)?; + } let mut routes = self.gsi_msi_routes.lock().unwrap(); routes.insert(route.gsi, *entry); return self.set_gsi_routes(&routes); @@ -178,48 +189,6 @@ impl InterruptSourceGroup for MsiInterruptGroup { format!("update: Invalid interrupt index {}", index), )) } - - fn mask(&self, index: InterruptIndex) -> Result<()> { - if let Some(route) = self.irq_routes.get(&index) { - let mut routes = self.gsi_msi_routes.lock().unwrap(); - if let Some(entry) = routes.get_mut(&route.gsi) { - entry.masked = true; - } else { - return Err(io::Error::new( - io::ErrorKind::Other, - format!("mask: No existing route for interrupt index {}", index), - )); - } - route.disable(&self.vm)?; - return self.set_gsi_routes(&routes); - } - - Err(io::Error::new( - io::ErrorKind::Other, - format!("mask: Invalid interrupt index {}", index), - )) - } - - fn unmask(&self, index: InterruptIndex) -> Result<()> { - if let Some(route) = self.irq_routes.get(&index) { - let mut routes = self.gsi_msi_routes.lock().unwrap(); - if let Some(entry) = routes.get_mut(&route.gsi) { - entry.masked = false; - } else { - return Err(io::Error::new( - io::ErrorKind::Other, - format!("mask: No existing route for interrupt index {}", index), - )); - } - route.enable(&self.vm)?; - return self.set_gsi_routes(&routes); - } - - Err(io::Error::new( - io::ErrorKind::Other, - format!("unmask: Invalid interrupt index {}", index), - )) - } } pub struct LegacyUserspaceInterruptGroup { @@ -247,7 +216,12 @@ impl InterruptSourceGroup for LegacyUserspaceInterruptGroup { }) } - fn update(&self, _index: InterruptIndex, _config: InterruptSourceConfig) -> Result<()> { + fn update( + &self, + _index: InterruptIndex, + _config: InterruptSourceConfig, + _masked: bool, + ) -> Result<()> { Ok(()) }