RFC: refactor VirtIO devices, preparing for a PCI transport layer #5160
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/pcie #5160 +/- ##
================================================
- Coverage 83.06% 83.01% -0.06%
================================================
Files 250 251 +1
Lines 26897 27063 +166
================================================
+ Hits 22342 22466 +124
- Misses 4555 4597 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bchalios
force-pushed
the
feature/pcie
branch
7 times, most recently
from
7ab92d6 to
5dd5953
Compare
src/vmm/src/device_manager/mmio.rs
Outdated
| @@ -548,7 +552,8 @@ mod tests { | |||
| cmdline: &mut kernel_cmdline::Cmdline, | |||
| dev_id: &str, | |||
| ) -> Result<u64, MmioError> { | |||
| let mmio_device = MmioTransport::new(guest_mem, device, false); | |||
| let interrupt = Arc::new(IrqTrigger::new().unwrap()); | |||
There was a problem hiding this comment.
I was wondering if it made sense to create this directly in the MmioTransport
There was a problem hiding this comment.
I tried that. But restoring from a snapshot was a nightmare, because we first restore the device state and then we restore the transport. So, I needed the IrqTrigger before restoring the device itself.
bchalios
force-pushed
the
feature/pcie
branch
2 times, most recently
from
040a2e2 to
9880eac
Compare
src/vmm/src/devices/virtio/device.rs
Outdated
| use crate::vstate::memory::GuestMemoryMmap; | ||
|
|
||
| /// Enum that indicates if a VirtioDevice is inactive or has been activated | ||
| /// and memory attached to it. | ||
| #[derive(Debug)] | ||
| pub enum DeviceState { | ||
| Inactive, | ||
| Activated(GuestMemoryMmap), | ||
| Activated((GuestMemoryMmap, Arc<dyn VirtioInterrupt>)), |
There was a problem hiding this comment.
| Activated((GuestMemoryMmap, Arc<dyn VirtioInterrupt>)), | |
| Activated(GuestMemoryMmap, Arc<dyn VirtioInterrupt>), |
at the very least :)
There was a problem hiding this comment.
Added a struct type that includes them
src/vmm/src/devices/virtio/device.rs
Outdated
| @@ -36,7 +36,7 @@ impl DeviceState { | |||
| } | |||
|
|
|||
| /// Gets the memory and interrupt attached to the device if it is activated. | |||
| pub fn active_state(&self) -> Option<(&GuestMemoryMmap, Arc<IrqTrigger>)> { | |||
| pub fn active_state(&self) -> Option<(&GuestMemoryMmap, Arc<dyn VirtioInterrupt>)> { | |||
There was a problem hiding this comment.
I don't think we necessarily can, because then we'd have to decide at compile time whether we use MMIO or PCIe, right?
| &balloon_state.device_info, | ||
| constructor_args.event_manager, | ||
| )?; | ||
| } | ||
|
|
||
| for block_state in &state.block_devices { | ||
| let interrupt = | ||
| Arc::new(IrqTrigger::new().expect("Could not create interrupt for MMIO device")); |
There was a problem hiding this comment.
We unwrap on IrqTrigger::new() literally everywhere. Can we instead just unwrap() inside new() and have it not return Result?
| #[cfg(test)] | ||
| fn has_pending_interrupt(&self, interrupt_type: VirtioInterruptType) -> bool { | ||
| if let Ok(num_irqs) = self.irq_evt.read() { | ||
| if num_irqs == 0 { | ||
| return false; | ||
| } | ||
|
|
||
| let irq_status = self.irq_status.load(Ordering::SeqCst); | ||
| return matches!( | ||
| (irq_status, interrupt_type.into()), | ||
| (VIRTIO_MMIO_INT_CONFIG, IrqType::Config) | (VIRTIO_MMIO_INT_VRING, IrqType::Vring) | ||
| ); | ||
| } | ||
|
|
||
| false | ||
| } |
There was a problem hiding this comment.
Can this just be a free-standing function has_pending_interrupt<T: VirtioInterrupt>(&T, VirtioInterruptType) -> bool somewhere inside a test module, so we avoid the #[cfg(test)]?
There was a problem hiding this comment.
Let me see if I can somehow make this easily, but I think it should be doable.
There was a problem hiding this comment.
hmmm. The problem is that for as long as we support both MMIO and PCIe we will not have an easy way to create an type independent implementation. WDYT?
| } | ||
|
|
||
| /// API of interrupt types used by VirtIO devices | ||
| pub trait VirtioInterrupt: std::fmt::Debug + Send + Sync { |
There was a problem hiding this comment.
Mh, what is the advantage of this being a trait over it being an enum?
There was a problem hiding this comment.
Don't necessarily see an advantage. That's how it's defined on the CH side, so got it as is.
I do have a sort of personal preference to dynamic dispatch when it's not on a performance critical path, because I (visually) hate this:
impl VirtioInterrupt {
pub fn foo(&self) -> Bar {
match self {
Irq(irq) => irq.foo()
Msi(msi) => msi.foo()
}
}
}There was a problem hiding this comment.
Ah fair, I personally actually prefer the enum version, but that's indeed personal preference. I mainly left the comment because I found the explicit Debug bound on a trait that functionally has nothing to do with debugging a bit ugly, but can also stay like this if you prefer it
Cargo.lock
Outdated
| [[package]] | ||
| name = "vm-device" | ||
| version = "0.1.0" | ||
| dependencies = [ | ||
| "anyhow", | ||
| "serde", | ||
| "thiserror 2.0.12", | ||
| "vmm-sys-util", | ||
| ] |
There was a problem hiding this comment.
Leftovers from earlier experimentation? :o
There was a problem hiding this comment.
hmmmm. Rebuilding seems to bring this in. I've no idea who's pulling it in.
| @@ -311,7 +311,7 @@ impl<T: VhostUserHandleBackend + Send + 'static> VirtioDevice for VhostUserBlock | |||
| &self.queue_evts | |||
| } | |||
|
|
|||
| fn interrupt_trigger(&self) -> Arc<IrqTrigger> { | |||
| fn interrupt_trigger(&self) -> Arc<dyn VirtioInterrupt> { | |||
There was a problem hiding this comment.
I suppose all of these could just return &dyn VirtioInterrupt, as none of the callers make use of the Arcs as far as I can see. But also out of scope here.
There was a problem hiding this comment.
Yup, shall we do this when we bring the MSI code in?
| } | ||
| }; | ||
| if let Some(qidx) = raise_irq { | ||
| self.signal_used_queue(qidx as usize).unwrap_or_default(); |
There was a problem hiding this comment.
I don't think this call to unwrap_or_default() is needed
There was a problem hiding this comment.
the _or_default() variant was used already, I assume, to ignore errors. I am happy to change this to something like expect(). Is that what you mean?
There was a problem hiding this comment.
Ah, no I was thinking of just doing
| self.signal_used_queue(qidx as usize).unwrap_or_default(); | |
| self.signal_used_queue(qidx as usize); |
but you're right, clippy would complain about the unused results, so it'd have to be
| self.signal_used_queue(qidx as usize).unwrap_or_default(); | |
| _ = self.signal_used_queue(qidx as usize); |
but iirc we do unwrap() (without _or_default) on these in other devices, so that should be fine too?
There was a problem hiding this comment.
Changed it to expect().
bchalios
force-pushed
the
feature/pcie
branch
4 times, most recently
from
68d197d to
c59d619
Compare
This is just code organization changes. Create a new module under `virtio`, called `transport`. For the time being the only transport supported is `mmio`. Also, move `IrqInterrupt` type within the MMIO transport code, as it is MMIO specific. Signed-off-by: Babis Chalios <[email protected]>
The MMIO transport for VirtIO devices uses an `IrqTrigger` object as the object that models the logic for sending interrupts from the device to the guest. We create one such object for every VirtIO device when creating it. The MMIO device manager associates this object with an IRQ number and registers it with KVM. This commit changes the timing of association of an `IrqTrigger` with a VirtIO-mmio device. It only assigns such an object to the device during its activation. We do this to prepare for supporting a PCI transport for VirtIO devices. The cloud hypervisor implementation for these passes the interrupt objects used by the device during activation, so we make this change to have a uniform way to handle interrupts for both transports. Functionally, nothing changes for MMIO devices, as before activation we don't trigger any interrupts. Signed-off-by: Babis Chalios <[email protected]>
Describing the APIs that need to implement types that are used as interrupts for VirtIO devices. Currently, we only use `IrqInterrupt` interrupts, but this will change once we have MSI-X with PCIe devices. Signed-off-by: Babis Chalios <[email protected]>
VirtIO devices assume they're operating under an MMIO transport and as a consequence they use IrqTrigger as interrupts. Switch that to using VirtioInterrupt for all VirtIO device objects. Only assume a VirtioInterrupt is an IrqTrigger in MMIO specific code. Signed-off-by: Babis Chalios <[email protected]>
`IrqTrigger::new()` returns a `Result` because creating an `EventFd` might fail with an `std::io::Error` error. All users of `IrqTrigger` create the object and directly unwrap the error. To avoid unwraps all over the place, change `IrqTrigger::new()` to unwrap a potential error while creating the EventFd internally and just return `Self`. Signed-off-by: Babis Chalios <[email protected]>
Changes
This does a few changes around interrupts for VirtIO devices and code organization for being able to handle multiple transports for VirtIO devices:
transportmodule withinvmm::devices::virtioand movesmmiomodule inside itVirtioInterrupttrait that abstracts away the functionality of interrupts used by VirtIO devices. At the moment, onlyIrqTriggerimplementsVirtioInterrupt.IrqTriggerobject is assigned to a VirtIO device. Up until now we created this object when creating the device itself. This PR changes this logic so that it is the transport layer that creates this interrupt and passes it over to the device during activation time. This is how things are done with PCIe in the Cloud Hypervisor implementation. It also, doesn't change anything for us, semantically; devices only trigger interrupts after having been activated.Reason
Preparing for adding PCIe support
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.