Add interrupt traits and a KVM based implementation #11

jiangliu · 2019-10-26T16:57:18Z

This PR add consts/structs/traits to manage interrupt sources for backend devices.
It also provides a KVM hypervisor based implementation for x86 legacy interrupts, PCI MSI/MSI-x interrupts.

jiangliu · 2019-10-26T17:52:30Z

Please refer to #8, #6 for related discussions.
Needs help from ARM experts to make it work on ARM/ARM64 platforms.

src/interrupt/kvm_irq/pci_msi_irq.rs

sameo · 2019-10-28T07:44:30Z

@jiangliu Does that PR replace #8 ?

jiangliu · 2019-10-28T07:58:11Z

@jiangliu Does that PR replace #8 ?

Yes, #11 and #9 replaces #8 .

src/interrupt/mod.rs

src/interrupt/kvm_irq/legacy_irq.rs

src/interrupt/mod.rs

src/interrupt/kvm_irq/legacy_irq.rs

Switch to rust 2018 edition and turn on deny(missing_docs). Signed-off-by: Liu Jiang <[email protected]>

Introduce traits InterruptManager and InterruptSourceGroup to manage interrupt sources for virtual devices. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

liujing2 · 2019-11-08T02:34:26Z

src/interrupt/mod.rs

+//! * the VMM creates a device manager, passing on an reference to the interrupt manager
+//! * the device manager passes on an reference to the interrupt manager to all registered devices


After we deciding whether device manager is responsible for passing the reference of interrupt manager to each registered devices, we can update the comments or the PR #12.

sboeuf

A few comments, but nice work :)

src/interrupt/mod.rs

src/interrupt/kvm/legacy_irq.rs

src/interrupt/kvm/pci_msi_irq.rs

src/interrupt/kvm/mod.rs

src/interrupt/kvm/pci_msi_irq.rs

Implement infrastructure to manage interrupt sources based on Linux KVM kernel module. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

andreeaflorescu

I have a few general concerns here:

vm-device has too many dependencies
it looks very x86_64 and linux specific. @petrutlucian94 can you offer some insight into interrupts on Windows? Is the trait generic enough to work for your usecase as well?
At the design level it looks like the current trait doesn't follow the open-closed design principle. Specifically, if you need to add a new interrupt type, you need to change the following things:

define a new InterruptSourceType with the name of your interrupt
define a new InterruptSourceConfig

I haven't have the time to dive deep into how interrupts work, but before merging this I would like to take some more time and discuss to see if we can get to a simpler Interrupt interface that can be useful for Firecracker as well. I find this of particular importance since we want the Virtio implementation to depend on the Interrupt trait.

andreeaflorescu · 2019-11-12T17:32:55Z

Cargo.toml

+kvm-bindings = { version = ">=0.1.1, <1.0", optional = true }
+kvm-ioctls = { git = "https://github.com/rust-vmm/kvm-ioctls.git", branch = "master", optional = true }


I am not particularly happy with adding dependencies on kvm related wrappers here. Even though you specify them as optional, their meta-data would still be pulled even if you don't use them. Furthermore, it makes the vm-device a very coupled crate. If there is a way to decouple this, I would prefer to not have these dependencies.

It depends on the granularity for rust-vmm crates. We may have separate interface definition and implementation crates.
My suggestion is that:

integrate interface and implementation within the same crate if there's a majority implementation existing. Vm-memory and interrupt falls into this case.

separate interface definition and implementations if there are multiple possible implementations. VmDevice(DeviceIo) falls into this case.

Can we split this PR in two? One PR that adds the interface so people can experiment with it and one PR for actual implementation?

src/interrupt/kvm/legacy_irq.rs

andreeaflorescu · 2019-11-12T17:45:31Z

src/interrupt/mod.rs

+#[allow(clippy::trivially_copy_pass_by_ref)]
+pub trait InterruptSourceGroup: Send + Sync {
+    /// Get type of interrupt sources managed by the group.
+    fn interrupt_type(&self) -> InterruptSourceType;


I believe that having the interrupt_type accessible from outside defeats the purpose of having a generic trait for working with interrupts. Unfortunately this means that in the crates depending on InterruptSourceGroup instead of counting only on the trait interface, you will use the type to alter the parameters of trait functions. This already happens with the current proposal of Virtio: https://github.com/rust-vmm/vm-virtio/blob/5b37629a8edae71004d11f17da5b0b0856531bb3/src/device.rs#L59

We may have some other technical ways to solve this issue, such as change

fn trigger(&self, index: InterruptIndex, flags: u32)

as

fn trigger(&self, index: InterruptIndex, subindex: u32)

The Intel interrupt remapping tech has used this style terms.
But the device backend drivers need to know about interrupt working mode, it's not easy to hide all interrupt details from device backend drivers.

src/interrupt/mod.rs

andreeaflorescu · 2019-11-12T17:48:42Z

src/interrupt/mod.rs

+        ty: InterruptSourceType,
+        base: InterruptIndex,
+        count: InterruptIndex,
+    ) -> Result<Arc<Box<dyn InterruptSourceGroup>>>;


Shouldn't the caller of create_group make sure to add the InterruptSourceGroup in an Arc?

The InterruptManager implementation needs to hold a reference to Arc<Box> for host keeping.

Hmmm, I still don't understand why create_group can't return InterruptSourceGroup and the caller of create_group can wrap the InterruptSourceGroup in an Arc.

When creating a new interrupt group, the new interrupt group object will be inserted into a hash map for house-keeping. So we need to maintain the ownership by either:

use Arc::clone()

use reference.
And it's a little complex to handle lifetime parameter when using reference. So an <Box> object is returned.

fn create_group( &self, type_: InterruptSourceType, base: InterruptIndex, count: InterruptIndex, ) -> Result<Arc<Box<dyn InterruptSourceGroup>>>; struct KvmIrqManagerObj { vmfd: Arc<VmFd>, routes: Arc<KvmIrqRouting>, groups: HashMap<InterruptIndex, Arc<Box<dyn InterruptSourceGroup>>>, max_msi_irqs: InterruptIndex, }

andreeaflorescu · 2019-11-12T17:49:57Z

src/interrupt/mod.rs

+/// interrupt source as a distinct InterruptSource.
+#[allow(clippy::len_without_is_empty)]
+#[allow(clippy::trivially_copy_pass_by_ref)]
+pub trait InterruptSourceGroup: Send + Sync {


I am not super sure how this would work with aarch64 and I would need more time to test it out. Did you run any experiments on aarch64 as well? From the naming it looks coupled to the way x86_64 works.

Not yet. I have had a quick glance at GICv3 spec, which look similar to x86 interrupt architecture.

I have refined the implementation to better support arm/arm64, could you please help to take a look? @andreeaflorescu

Can you point me to the changes? GitHub is not very good at showing diffs between versions.

Basically I have moved legacy irq related code from src/interrupt/kvm/mod.rs into src/interrupt/kvm/legacy_irq.rs with better support of ARM/ARM64. For ARM/ARM64, the MSI part should be the same, and hopefully we only need to implement initialize_legacy().

} #[cfg(any(target_arch = "aarch", target_arch = "aarch64"))] pub(super) fn initialize_legacy( _routes: &mut HashMap<u64, kvm_irq_routing_entry>, ) -> Result<()> { //TODO Ok(()) }

Seems we are reaching the real interesting parts now:)
Essentially the PR defines two sets of features:

feature bit to control supported interrupt source types:
[features]
legacy_irq = []
msi_irq = []
pci_msi_irq = ["msi_irq"]
And there's one more coming feature bit of this class:
generic_msi_irq = ["msi_irq"]
So we could support three types of interrupt sources: legacy, pci msi and generic msi.

feature bit to control the implementations of InterruptManager/InterruptSourceGroup.
kvm_irq = ["kvm-ioctls", "kvm-bindings"]
The 'kvm_irq' enables a default implementation of InterruptManager/InterruptSourceGroup based KVM kernel module. If the vmm has special requirements, it may provide its own implementation by disabling the kvm_irq feature.

For the vm-virtio, there are three possible combinations:

use legacy irq only. The crosvm/firecracker project uses this mode.

use msi irq only. The cloud hypervisor works in this mode.

use both legacy irq and msi irq. Our dragonball vmm works in this mode, and we plan to switch to msi irq only mode.

Please also refer to this kvm forum presentation for MMIO MSI support, https://kvmforum2019.sched.com/event/Tmw2/a-lightweight-virtual-interrupt-controller-for-containerserverless-jing-liu-chao-peng-intel?iframe=no&w=100%&sidebar=yes&bg=no

Since this is a complex problem, I would really like to insist on splitting this PR. It looks like we have a few loose ends that we need to decide on. Let's first decide on the interface and have a separate PR only with that. Can you please add a PR only with the interface definition?

I have different opinion here.
A PR with both interface definition and concrete implementation may give more information about the design, and it's easier for community to do experiments with the design.
If some design flaw has been discovered during experiments, we could easily change the interface design and implementation, or even revert the PR.
The vm-device is still in early stage, it would be better to share design and implementation sooner, otherwise we could not speed up the progress.

I am not against prototyping. This is something that we discussed about a few times already.

The proposal that everyone seemed to be on board with was to have a PR that defines the interfaces and nothing more. Functionality was to be added in subsequent PRs.

The initial PR can include external links to prototypes with the interface being used in practice. Having such a large PR introduces a few problems:

Delays in reviewing and merging the code.

Key design points of the interface can be missed because we are not insisting on the interface.

Large PRs such as this one tend to reach around 100 comments. After a few iterations the comments get outdated, you cannot possibly go through all of them to see if they were fixed.

andreeaflorescu · 2019-11-12T17:58:48Z

src/interrupt/mod.rs

+    ///
+    /// If the interrupt has an associated `interrupt_status` register, all bits set in `flag`
+    /// will be atomically ORed into the `interrupt_status` register.
+    fn trigger(&self, index: InterruptIndex, flags: u32) -> Result<()>;


I find it a bit weird that depending on the InterruptSourceType either index or flags is used. It looks to me that trigger cannot be abstract it for all interrupt types.

Yes, it's a tradeoff. For pin-based IRQ, it almost have an associated status register. So the extra parameter flag is used to share the common code to manipulate the status register. If we skip the parameter flags, each driver will need to implement the code to manipulate status register repeatedly.

The code to manipulate the status register is usually one line. So I think that the benefit of having the flags parameter removed is higher than the trouble of handling this outside.

The idea is to hide interrupt delivery details from device drivers.
Take virtio-net as an example, the virtio-net driver only cares to trigger an interrupt to notify the guest, and the virtio transport layer decides the way to deliver the interrupt. It doesn't make sense for the virtio-net driver to maintain and manipulate the interrupt status bit flags.
Or it's not only about lines of code, but also information encapsulation.

I agree with encapsulating the information, but I don't think this should be encapsulated at the vm-device level because not all devices need flags. Virtio devices need flags and interrupt_status, so instead of forcing this interface for interrupts on all devices, we can define an interrupt wrapper in vm-virtio. Looking at the implementation of legacy devices for example, it seems like flags is never going to be used. It also looks like the only thing that legacy interrupts and MSI have in common is trigger. I've been playing a bit with separating this interface in multiple specialized interfaces. I hope I'll have a prototype by the end of this week.

jiangliu · 2019-11-13T05:32:26Z

I have a few general concerns here:

vm-device has too many dependencies
It's a valid state. Currently we have dependency tree as:
vm-device v0.1.0 (/ws/src/vmm/rust-vmm/vm-device.git)
├── kvm-bindings v0.2.0
├── kvm-ioctls v0.3.0 (https://github.com/rust-vmm/kvm-ioctls.git#681745a7)
│ ├── kvm-bindings v0.2.0 ()
│ ├── libc v0.2.62
│ └── vmm-sys-util v0.2.0
│ └── libc v0.2.62 ()
├── libc v0.2.62 ()
├── vm-memory v0.1.0 (https://github.com/rust-vmm/vm-memory#8669369d)
│ ├── cast v0.2.2
│ └── libc v0.2.62 ()
└── vmm-sys-util v0.2.0 (*)
Among those, the vm-memory could be removed by using u64 instead of GuestMemoryAddress.
kvm-bindings/kvm-ioctls are gated by features.

it looks very x86_64 and linux specific. @petrutlucian94 can you offer some insight into interrupts on Windows? Is the trait generic enough to work for your usecase as well?
Any plan to support HyperV in addition to KVM?

At the design level it looks like the current trait doesn't follow the open-closed design principle. Specifically, if you need to add a new interrupt type, you need to change the following things:

define a new InterruptSourceType with the name of your interrupt

define a new InterruptSourceConfig
The interrupt is a relative stable subsystem, it doesn't evolve as quick as the VirtIo subsystem.
So I choose to keep things straightforward.

I haven't have the time to dive deep into how interrupts work, but before merging this I would like to take some more time and discuss to see if we can get to a simpler Interrupt interface that can be useful for Firecracker as well. I find this of particular importance since we want the Virtio implementation to depend on the Interrupt trait.

michael2012z

Some comments mainly from the point of view of ARM.

src/interrupt/kvm/mod.rs

src/interrupt/kvm/msi_irq.rs

Implement InterruptSourceGroup trait to manage x86 legacy interruts. On x86 platforms, pin-based device interrupts connecting to the master PIC, the slave PIC and IOAPICs are named as legacy interrupts. For legacy interrupts, the interrupt routing logic are manged by the PICs/IOAPICs and the interrupt group logic only takes responsibility to enable/disable the interrupts. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

With some kvm version, setting irq_routing for non-existing legaccy IRQs may cause system crash. So limit the number to available legacy interrupts. Signed-off-by: 守情 <[email protected]>

Introduce generic mechanism to support message signalled interrupts based on KVM hypervisor. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

Implement interrupt source driver to manage PCI MSI/MSI-x interrupts. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

Signed-off-by: 守情 <[email protected]>

liujing2

One concern for me is, we can see that every device that uses MSI would have a irq_routing: Arc<KvmIrqRouting> which shows the whole routing information of system.
Is it available to keep such info in KvmIrqManager and when a device wants to add new routing entries,
call function to do that?

jiangliu · 2019-11-25T05:55:58Z

One concern for me is, we can see that every device that uses MSI would have a irq_routing: Arc<KvmIrqRouting> which shows the whole routing information of system.
Is it available to keep such info in KvmIrqManager and when a device wants to add new routing entries,
call function to do that?

It's an implementation detail, both PciMsiIrq and KvmIrqRouting are hidden from device backend drivers.

jiangliu · 2020-01-31T18:04:06Z

As we have discussed, close this one in perfer of PR #21

jiangliu force-pushed the irq_v2 branch 4 times, most recently from 34393fe to a9c3d46 Compare October 26, 2019 17:44

jiangliu requested review from acatangiu, andreeaflorescu, bonzini, chao-p, liujing2, rbradford, sboeuf and zachreizner October 26, 2019 17:50

liujing2 reviewed Oct 28, 2019

View reviewed changes

src/interrupt/kvm_irq/pci_msi_irq.rs Outdated Show resolved Hide resolved

sameo mentioned this pull request Oct 29, 2019

Add interrupt subsystem to vm-device #8

Closed

sameo reviewed Oct 29, 2019

View reviewed changes

jiangliu force-pushed the irq_v2 branch from a9c3d46 to 67f3166 Compare October 31, 2019 05:53

jiangliu mentioned this pull request Oct 31, 2019

Refine the VirtioDevice trait rust-vmm/vm-virtio#10

Closed

jiangliu requested review from sameo and liujing2 October 31, 2019 11:22

liujing2 mentioned this pull request Oct 31, 2019

Add IO manager support #12

Merged

jiangliu added 2 commits November 7, 2019 16:40

Switch to rust 2018 edition

902b9ae

Switch to rust 2018 edition and turn on deny(missing_docs). Signed-off-by: Liu Jiang <[email protected]>

interrupt: introduce traits to manage interrupt sources

43b8c0b

Introduce traits InterruptManager and InterruptSourceGroup to manage interrupt sources for virtual devices. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

jiangliu force-pushed the irq_v2 branch 2 times, most recently from ef4fa92 to 355e1a8 Compare November 7, 2019 09:20

liujing2 reviewed Nov 8, 2019

View reviewed changes

sboeuf reviewed Nov 11, 2019

View reviewed changes

Implement infrastructure to manage interrupts by KVM

4e80190

Implement infrastructure to manage interrupt sources based on Linux KVM kernel module. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

andreeaflorescu requested changes Nov 12, 2019

View reviewed changes

jiangliu force-pushed the irq_v2 branch from 355e1a8 to eefb5ff Compare November 13, 2019 06:01

michael2012z reviewed Nov 19, 2019

View reviewed changes

src/interrupt/kvm/mod.rs Outdated Show resolved Hide resolved

src/interrupt/kvm/mod.rs Show resolved Hide resolved

src/interrupt/kvm/mod.rs Outdated Show resolved Hide resolved

src/interrupt/kvm/mod.rs Outdated Show resolved Hide resolved

src/interrupt/kvm/msi_irq.rs Show resolved Hide resolved

jiangliu force-pushed the irq_v2 branch 2 times, most recently from 613c326 to 4c57c28 Compare November 21, 2019 02:30

juliusxlh and others added 4 commits November 21, 2019 10:46

Limit number of legacy irqs

8854f47

With some kvm version, setting irq_routing for non-existing legaccy IRQs may cause system crash. So limit the number to available legacy interrupts. Signed-off-by: 守情 <[email protected]>

Add generic heplers to manage MSI interrupts

288f947

Introduce generic mechanism to support message signalled interrupts based on KVM hypervisor. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

Manage PCI MSI/PCI MSI-x interrupts

ed65d8a

Implement interrupt source driver to manage PCI MSI/MSI-x interrupts. Signed-off-by: Liu Jiang <[email protected]> Signed-off-by: Bin Zha <[email protected]>

Add unit tests for interrupt manager

f3bd7b1

Signed-off-by: 守情 <[email protected]>

jiangliu force-pushed the irq_v2 branch 2 times, most recently from f915a2e to f3bd7b1 Compare November 21, 2019 03:30

jiangliu requested review from liujing2, sboeuf and andreeaflorescu November 21, 2019 06:19

sboeuf approved these changes Nov 21, 2019

View reviewed changes

liujing2 reviewed Nov 22, 2019

View reviewed changes

andreeaflorescu mentioned this pull request Dec 18, 2019

TODO list rust-vmm/vm-virtio#11

Open

rbradford removed their request for review January 13, 2020 14:42

This was referenced Jan 29, 2020

interrupt: Initial interrupt manager traits sameo/vm-device#1

Closed

Interrupt v3: Add interrupt traits and a KVM based implementation #21

Open

jiangliu closed this Jan 31, 2020

		//! * the VMM creates a device manager, passing on an reference to the interrupt manager
		//! * the device manager passes on an reference to the interrupt manager to all registered devices

		kvm-bindings = { version = ">=0.1.1, <1.0", optional = true }
		kvm-ioctls = { git = "https://github.com/rust-vmm/kvm-ioctls.git", branch = "master", optional = true }

Add interrupt traits and a KVM based implementation #11

Add interrupt traits and a KVM based implementation #11

Conversation

jiangliu commented Oct 26, 2019

jiangliu commented Oct 26, 2019

sameo commented Oct 28, 2019

jiangliu commented Oct 28, 2019

Choose a reason for hiding this comment

sboeuf left a comment

Choose a reason for hiding this comment

andreeaflorescu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangliu commented Nov 13, 2019

michael2012z left a comment

Choose a reason for hiding this comment

liujing2 left a comment • edited Loading

Choose a reason for hiding this comment

jiangliu commented Nov 25, 2019

jiangliu commented Jan 31, 2020

jiangliu Nov 27, 2019 •

edited

Loading

liujing2 left a comment •

edited

Loading