Interrupt v3: Add interrupt traits and a KVM based implementation

[jiangliu]

This PR is the successor of PR #11. The #11 has too many discussions so we reopen a new PR for clear review history, it also rebases to the latest vm-device code base.

The adds 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.

There are also two planned PR based on this one:

1. Refine the InterruptSourceGroup trait based on Cloud Hypervisor's implementation.
2. Add support for VFIO devices, which has dependency on the vfio-ioctls crate(which is still under review), so not included in this PR.

[michael2012z]

It should be "arm" for the architecture before aarch64.

[bonzini]

My remarks from sameo#3 still apply; I don't think that having the interrupt type as an enum is future proof, since it prevents you from using the interrupt traits internally (for example for buses that are internal to device emulation). I'm not a contributor to vm-device so I'm not making this a reject, but anyway that's my opinion.

[bonzini]

Asking for a rework of the features, leaving them to later commits that implement the KVM integrations.

[bonzini]

Sorry, now including the comments. I just cannot get this GitHub thing. :D

[bonzini]

I would go a step further if possible:

• make this enum an associated type of InterruptSourceGroup

• move InterruptSourceConfig to the implementation of the KVM manager, removing the two features.

[jiangliu]

Hi, @bonzini @sameo @sboeuf @alexandruag
I have updated the PR to only include the interface definition of InterruptSourceGroup, and leave InterruptManager and InterruptSourceGroup concrete implementations to VMMs.

I have merged most changed from Cloud Hypervisor into the new InterruptSourceGroup trait, with two main changes:

1. enable() takes a parameter configs and enforce that enable should be called before calling other methods of InterruptSourceGroup.
2. add get_pending_state() method, to company mask/unmask.

[andreeaflorescu]

Asking for a rework of the features, leaving them to later commits that implement the KVM integrations.

I would actually prefer the KVM implementations to live in a separate crate or in the VMM specific code. In the future, I would love to see vm-device independent of KVM and extend this code to support other hypervisors as well. Right now it feels a bit targeted at x86_64 and KVM.

[bonzini]

To me it's okay as long as it's hidden behind a feature, just like we have an mmap backend in vm-memory that isn't strictly speaking needed for all VMMs. Making a separate crate for KVM things makes sense too, we could have also an integration with GuestMemory in the same crate, or a dispatcher for MMIO vmexits. But the most important part is that it should certainly be customizable (it's also easier to move stuff in the future out if it's a separate feature).

[jiangliu]

To me it's okay as long as it's hidden behind a feature, just like we have an mmap backend in vm-memory that isn't strictly speaking needed for all VMMs. Making a separate crate for KVM things makes sense too, we could have also an integration with GuestMemory in the same crate, or a dispatcher for MMIO vmexits. But the most important part is that it should certainly be customizable (it's also easier to move stuff in the future out if it's a separate feature).

I agree it would be better to make the interface trait as common as possible, and use feature to gate platform/hypervisor specific implementation. I also prefer including such a reference implementation in the same crate, it helps to verify that the interfaces really work.

It's challenge to design interface by rust without a reference implementation:(

[jiangliu]

How does the caller know the number of interrupts? Should the trait include a len() method and a constructor that takes size: u32?

Sorry, interrupt_type() getter should be removed, but the len() method should be kept. Will recover it .

[jiangliu]

This only makes sense for level-triggered interrupts. Should this return a Result, so that it can fail in the edge-triggered case?

pardon, which method?

[bonzini]

pardon, which method?

get_pending_state. See b33c69e, I have no idea why the relevant contenxt is not showing up in the pull request web page.

[jiangliu]

pardon, which method?

get_pending_state. See b33c69e, I have no idea why the relevant contenxt is not showing up in the pull request web page.

The main use case of mask/unmask/get_pending_state() is to support PCI MSI/MSIx. PCI MSI/MSIx is edge triggered and supports pending state bit. So used bool instead of Result<bool>. Any requirement to distinguish between "No pending interrupt" and "no support of querying pending state"?

It's really challenge to support PCI bus, much more complex than MMIO. We have used MSI with MMIO devices without support of mask/unmask for a long time.

[bonzini]

Looking more at the cloud-hypervisor code, a lot of the MsiInterruptGroup group seems to be a very boring wrapper around Vec. I wonder if the best solution is to have InterruptSourceGroup be a struct that implements Index, and instead making VMMs implement an Interrupt trait. So you would write:

grp[i].trigger();

Something like this:

pub trait Interrupt {
    type Config;
    fn enable(&self) -> Result<()>;
    fn disable(&self) -> Result<()>;
    fn notifier(&self) -> Option<&EventFd> { None }
    fn trigger(&self) -> Result<()> { self.notifier().write(1) }
}
pub trait MSIInterrupt: Interrupt {
    fn mask(&self) -> Result<()>;
    fn unmask(&self) -> Result<()>;
    fn get_state(&self) -> bool;
}

pub struct InterruptSourceGroup<'a, I: Interrupt> {
    vec: Vec<I>
}

impl InterruptSourceGroup {
    fn enable(&self) -> Result<()> {
        for (_, route) in self.vec.iter() {
            route.enable()?;
        }

        Ok(())
    }
    // etc. possibly implement a get() method that returns an Option<I>
    // to not panic on out-of-bounds accesses
}

impl<I: Interrupt> Index<u32> for InterruptSourceGroup<'_, I> {
    type Output = I;
    fn index(&self, index: u32) -> &Self::Output { self.vec[index] }
}

create_group then can just return an Arc<InterruptSourceGroup<Self::Interrupt>> where Interrupt is an associated type of InterruptManager (and the config type then is Self::Interrupt::Config>).

[bonzini]

See bonzini@388a575 for an example (also including unit tests).