interrupt: Initial interrupt manager traits

[sameo]

Signed-off-by: Andreea Florescu fandree@amazon.com
Signed-off-by: Samuel Ortiz sameo@linux.intel.com

[bonzini]

Not making it a reject, but I strongly suggest removing all mentions of interrupt types from this pull request.

[bonzini]

I think this should not be in the InterruptManager trait. In fact I don't think InterruptSourceType should be in this trait at all. Rather, there should be multiple implementations of the InterruptManager, for example one that returns an InterruptSourceGroup for legacy interrupts and one that returns an InterruptSourceGroup for MSIs. I would make this something like

trait InterruptManager {
    type Config;
    // use interior mutability, more than one device will own the InterruptManager
    fn create_source(&self, c: Config) -> Result<Box<dyn InterruptSourceGroup>>;
}

A PCI host bridge will have two interrupt managers, one for legacy interrupts and one for MSIs. In turn the one for legacy interrupts could delegate to the GSI (8259/IOAPIC) interrupt manager.

[jiangliu]

This proposal has two drawbacks:

1. carrying on multiple interrupt managers
2. multiple interrupt managers need cooperation on the global KVM routing table.
   We still need another level of wrapper manager to dispatch different type of requests to different InterruptManager.

With the "interrupt_type: InterruptSourceType" parameter, we may build a interrupt manager to handle both legacy and msi interrupt. Or we could be a hierarchy as
struct InterruptManagerWrapper {
struct InterruptManagerLegacy,
struct InterruptManagerMsi,
}.
By this way, the client side is simpler.

[sameo]

Not making it a reject, but I strongly suggest removing all mentions of interrupt types from this pull request.

Thanks for the suggestion @bonzini. What are the main reasons you have in mind for strongly suggesting not carrying an interrupt type definition in this trait? Generic design best practice (I agree it'd be better not to have the create_group method be interrupt type agnostic) or do you have some specific concerns in mind? Your proposal would mean potentially carrying several interrupt managers down to devices (for those that can support several kind of interrupts), which makes the internal API a little more complex.
That being said, I'm going to play with that proposal on Cloud Hypervisor and see how it'd look like.

[sameo]

This works pretty nicely, see cloud-hypervisor/cloud-hypervisor#704

I will update this proposal accordingly, looking for your input @andreeaflorescu and @jiangliu.

[bonzini]

Your proposal would mean potentially carrying several interrupt managers down to devices (for those that can support several kind of interrupts), which makes the internal API a little more complex.

Yes, and my hunch was that it would actually be nicer that way. :) In general, the legacy/MSI distinction is very x86 specific, every board and processor will probably do something different. Also, it would make it possible to reuse interrupts as a generic concepts for buses that aren't even wired to the CPU.

[bonzini]

Does it make sense to return an Arc<Box<>>? With so many methods on InterruptSourceGroup being on &mut self there isn't much that you could do. Would Box<> be too limiting? If so, I would rather switch enable/disable to &self.

But is enable/disable really needed? It should be controlled by the "parent" (which would just eat or latch interrupts) or handled by the device (likewise), depending on the control register that causes enabling or disabling. Otherwise you get a mess of enable/disable counters, unless I am misunderstanding the purpose of those methods.

[jiangliu]

We should remove "type GroupConfig" and reverts to "base: InterruptIndex, count: InterruptIndex".

Introduction of "type GroupConfig" causes troubles, it limits the InterruptManager could only create one type of InterruptSourceGroup, either LegacyIrq or MsiIrq. It works with Cloud Hypervisor because CLH handles LegacyIrq by userspace IOAPIC and handles MSI in kernel emulated LAPIC.

We use kvm emulated ioapic/lapic to support LegacyIrq, MsiIrq and VfioIrq.
So the GroupConfig breaks our design.

One the hand, revertint to base, count should work with CLH too.

[andreeaflorescu]

base and count don't really make sense for all interrupts out there. In Firecracker we don't really need this, and our group will always have one interrupt, which means that our GroupConfig will probably be empty. I think it makes more sense to enable all usecases by using GroupConfig.

[jiangliu]

base is always needed, the count is always 1 for firecracker so could be skipped:)

[jiangliu]

Suggest to revert enable() to:
fn enable(&self, configs: &[InterruptSourceConfig]) -> Result<()>;

The proposed work flow for virtio devices is:

1. guest kernel writes MSI configuration registers, the vmm caches those configuration information.
2. guest kernel writes virtio status register to start virtio device
   2.1) the vmm applies cached msi configuration as a batch and commit them to kvm routing table.
   2.2) invoke virtio device's activate()
3. guest kernel writes MSI registers to update MSI configuration. Because the virtio device has been activated, the vmm calls InterruptSourceGroup::update() to directly update kvm routing table.

So we need to pass the cached MSI configuration information when calling InterruptSourceGroup::enable().

And as Paolo suggested, it would be better for enable()/disable() to take "&self" instead of "&mut self". BTW, what's the scenario needing a "&mut self" for enable()/disable()?

[jiangliu]

This works pretty nicely, see cloud-hypervisor/cloud-hypervisor#704

I will update this proposal accordingly, looking for your input @andreeaflorescu and @jiangliu.

It works because CLH uses userspace IOAPIC. It gets complex when using in-kernel IOAPIC>

[jiangliu]

@sameo After checking cloud hypervisor and dragonball implementation, we can change to support following interfaces:
pub trait InterruptManager {
fn create_group(
&self,
type_: InterruptSourceType,
base: InterruptIndex,
count: InterruptIndex,
) -> Result<Arc<Box>>;
fn destroy_group(&self, group: Arc<Box>) -> Result<()>;
}

pub trait InterruptSourceGroup: Send + Sync {
fn enable(&self, configs: &[InterruptSourceConfig]) -> Result<()>;
fn disable(&self) -> Result<()>;
fn update(&self, index: InterruptIndex, config: &InterruptSourceConfig) -> Result<()>;
fn notifier(&self, _index: InterruptIndex) -> Option<&EventFd>;
fn trigger(&self, index: InterruptIndex) -> Result<()>;
fn mask(&self, _index: InterruptIndex) -> Result<()>;
fn unmask(&self, _index: InterruptIndex) -> Result<()>;
}

The are two main changes from your proposal:

1. use InterruptSourceType instead of GroupConfig
2. pass parameter "configs: &[InterruptSourceConfig]" in InterruptSourceGroup::enable().
   Any suggestions?

[bonzini]

I am strongly against including the interrupt type in the create_group method. Interrupts of different logical kinds should absolutely have separate interfaces even if the backend is shared in some implementations.

[jiangliu]

I am strongly against including the interrupt type in the create_group method. Interrupts of different logical kinds should absolutely have separate interfaces even if the backend is shared in some implementations.

Let's move one step back:)
InterruptManager is very VMM specific, InterruptSourceGroup is generic for all device drivers.
So how about focusing on InterruptSourceGroup and leaving InterruptManager for each VMM implementation?
Let's only include definition of InterruptSourceGroup in vm-device, and remove InterruptManager and concrete implementation.

[bonzini]

InterruptSourceGroup is generic for all device drivers.

Yes, but not for all interrupt kinds. In fact even MSIs will be represented by base/count in a PCI device and by value/address in the interface to the hypervisor---but perhaps both layers could use the InterruptSourceGroup trait? This is why config is best in my opinion. We can always standardized config types later.