KVM&genirq: Enable adaptive IRQ sharing for passed-through devices

From: Jan Kiszka<[email protected]>

PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices on the host side when passing them to a guest.

However, IRQ disabling via the PCI config space is more costly than masking the line via disable_irq. Therefore we register an IRQ sharing notifier and switch between line and device level disabling on demand.

This feature is optional, user space has to request it explicitly as it also has to inform us about its view of PCI_COMMAND_INTX_DISABLE. That way, we can avoid unmasking the interrupt and signaling it if the guest masked it via the PCI config space.

Documentation/kvm/api.txt +10 -0

index e1a9297..5e164db 100644
@@ -1112,6 +1112,14 @@ following flags are specified:
 
  /* Depends on KVM_CAP_IOMMU */
  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1<<  0)
+/* The following two depend on KVM_CAP_PCI_2_3 */
+#define KVM_DEV_ASSIGN_PCI_2_3		(1<<  1)
+#define KVM_DEV_ASSIGN_MASK_INTX	(1<<  2)
+
+If KVM_DEV_ASSIGN_PCI_2_3 is set, the kernel will manage legacy INTx interrupts
+via the PCI-2.3-compliant device-level mask, but only if IRQ sharing with other
+assigned or host devices requires it. KVM_DEV_ASSIGN_MASK_INTX specifies the
+guest's view on the INTx mask, see KVM_ASSIGN_SET_INTX_MASK for details.
 
  4.48 KVM_DEASSIGN_PCI_DEVICE
 

@@ -1263,6 +1271,23 @@ struct kvm_assigned_msix_entry {
  	__u16 padding[3];
  };
 
+4.54 KVM_ASSIGN_SET_INTX_MASK
+

Documentation/kvm/api.txt +5 -0

+
+See KVM_ASSIGN_DEV_IRQ for the data structure. The target device is specified
+by assigned_dev_id. In the flags field, only KVM_DEV_ASSIGN_MASK_INTX is
+evaluated.
+

Quoted 39 lines · Jan Kiszka

From: Jan Kiszka<[email protected]>

PCI 2.3 allows to generically disable IRQ sources at device level. This enables us to share IRQs of such devices on the host side when passing them to a guest.

However, IRQ disabling via the PCI config space is more costly than masking the line via disable_irq. Therefore we register an IRQ sharing notifier and switch between line and device level disabling on demand.

This feature is optional, user space has to request it explicitly as it also has to inform us about its view of PCI_COMMAND_INTX_DISABLE. That way, we can avoid unmasking the interrupt and signaling it if the guest masked it via the PCI config space.

Documentation/kvm/api.txt +8 -0

index e1a9297..5e164db 100644
@@ -1112,6 +1112,14 @@ following flags are specified:
 
  /* Depends on KVM_CAP_IOMMU */
  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1<<  0)
+/* The following two depend on KVM_CAP_PCI_2_3 */
+#define KVM_DEV_ASSIGN_PCI_2_3		(1<<  1)
+#define KVM_DEV_ASSIGN_MASK_INTX	(1<<  2)
+
+If KVM_DEV_ASSIGN_PCI_2_3 is set, the kernel will manage legacy INTx interrupts
+via the PCI-2.3-compliant device-level mask, but only if IRQ sharing with other
+assigned or host devices requires it. KVM_DEV_ASSIGN_MASK_INTX specifies the
+guest's view on the INTx mask, see KVM_ASSIGN_SET_INTX_MASK for details.
 
  4.48 KVM_DEASSIGN_PCI_DEVICE
 

@@ -1263,6 +1271,23 @@ struct kvm_assigned_msix_entry {
  	__u16 padding[3];
  };
 

Documentation/kvm/api.txt struct kvm_assigned_msix_entry { · line 1285

Quoted 12 lines

Documentation/kvm/api.txt +12 -0

+4.54 KVM_ASSIGN_SET_INTX_MASK
+
+Capability: KVM_CAP_PCI_2_3
+Architectures: x86
+Type: vm ioctl
+Parameters: struct kvm_assigned_pci_dev (in)
+Returns: 0 on success, -1 on error
+
+Informs the kernel about the guest's view on the INTx mask. As long as the
+guest masks the legacy INTx, the kernel will refrain from unmasking it at
+hardware level and will not assert the guest's IRQ line. User space is still
+responsible for applying this state to the assigned device's real config space.

What's the protocol for doing this? I suppose userspace has to disable interrupts, ioctl(SET_INTX_MASK, masked), ..., ioctl(SET_INTX_MASK, unmasked), enable interrupts?

Isn't there a race window between the two operations?

Maybe we should give the kernel full ownership of that bit.

Quoted 4 lines

What's the protocol for doing this? I suppose userspace has to disable interrupts, ioctl(SET_INTX_MASK, masked), ..., ioctl(SET_INTX_MASK, unmasked), enable interrupts?

Userspace just has to synchronize against itself - what it already does:
qemu_mutex, and masking/unmasking is synchronous /wrt the the executing VCPU. Otherwise, masking/unmasking is naturally racy, also in Real Life. The guest resolves the remaining races.

Quoted 4 lines

Isn't there a race window between the two operations?

Maybe we should give the kernel full ownership of that bit.

I think this is what VFIO does and is surely cleaner than this approach. But it's not possible with the existing interface (sysfs + KVM ioctls) - or can you restrict the sysfs access to the config space in such details?

Quoted 5 lines · Jan Kiszka

Quoted 4 lines

What's the protocol for doing this? I suppose userspace has to disable interrupts, ioctl(SET_INTX_MASK, masked), ..., ioctl(SET_INTX_MASK, unmasked), enable interrupts?

Quoted 4 lines

Userspace just has to synchronize against itself - what it already does:
qemu_mutex, and masking/unmasking is synchronous /wrt the the executing VCPU. Otherwise, masking/unmasking is naturally racy, also in Real Life. The guest resolves the remaining races.

I meant when qemu sets INTX_MASK and the kernel clears it immediately afterwards because the two are not synchronized. I guess that won't happen in practice because playing with INTX_MASK is very rare.

Quoted 5 lines

Quoted 5 lines

Isn't there a race window between the two operations?

Quoted 5 lines

Quoted 1 line

Maybe we should give the kernel full ownership of that bit.

I think this is what VFIO does and is surely cleaner than this approach. But it's not possible with the existing interface (sysfs + KVM ioctls) - or can you restrict the sysfs access to the config space in such details?

I'm sure you can, not sure it's worth it. Can the situation be exploited? what if userspace lies?

Quoted 6 lines · Avi Kivity

Quoted 5 lines · Jan Kiszka

Quoted 4 lines

What's the protocol for doing this? I suppose userspace has to disable interrupts, ioctl(SET_INTX_MASK, masked), ..., ioctl(SET_INTX_MASK, unmasked), enable interrupts?

Quoted 8 lines

Quoted 4 lines

Userspace just has to synchronize against itself - what it already does:
qemu_mutex, and masking/unmasking is synchronous /wrt the the executing VCPU. Otherwise, masking/unmasking is naturally racy, also in Real Life. The guest resolves the remaining races.

I meant when qemu sets INTX_MASK and the kernel clears it immediately afterwards because the two are not synchronized. I guess that won't happen in practice because playing with INTX_MASK is very rare.

Ah, there is indeed a race, and the qemu-kvm patches I did not post yet (to wait for the kernel interface to settle) actually suffer from it:
userspace needs to set the kernel mask before writing the config space (it's the other way around ATM). This avoids that the kernel overwrites what userspace just wrote out. We always suffer from the race the other way around, see below.

Quoted 5 lines

Quoted 4 lines

I think this is what VFIO does and is surely cleaner than this approach. But it's not possible with the existing interface (sysfs + KVM ioctls) - or can you restrict the sysfs access to the config space in such details?

Quoted 2 lines

I'm sure you can, not sure it's worth it. Can the situation be exploited? what if userspace lies?

That's also the above scenario inverted: Userspace can mask or unmask at any time. If it unmasks a yet unhandled, thus raise interrupt, it will trigger another one. The kernel will catch it and mask it again. That can repeat forever with the frequency userspace is able to run its unmasking code. Not nice, but nothing to leverage for a DoS.

Quoted 16 lines · Jan Kiszka

Quoted 14 lines · Avi Kivity

Quoted 9 lines · Jan Kiszka

Quoted 4 lines

What's the protocol for doing this? I suppose userspace has to disable interrupts, ioctl(SET_INTX_MASK, masked), ..., ioctl(SET_INTX_MASK, unmasked), enable interrupts?

Userspace just has to synchronize against itself - what it already does:
qemu_mutex, and masking/unmasking is synchronous /wrt the the executing VCPU. Otherwise, masking/unmasking is naturally racy, also in Real Life. The guest resolves the remaining races.

I meant when qemu sets INTX_MASK and the kernel clears it immediately afterwards because the two are not synchronized. I guess that won't happen in practice because playing with INTX_MASK is very rare.

Quoted 6 lines

Ah, there is indeed a race, and the qemu-kvm patches I did not post yet (to wait for the kernel interface to settle) actually suffer from it:
userspace needs to set the kernel mask before writing the config space (it's the other way around ATM). This avoids that the kernel overwrites what userspace just wrote out. We always suffer from the race the other way around, see below.

Please document the protocol.

Is this always the right order?
Shouldn't it be reversed when unmasking? I admit I'm confused about this.

Besides 3 cleanup patches, this series consists of two major changes. The first introduces an interrupt sharing notifier to the genirq subsystem. It fires when an interrupt line is about to be use by more than one driver or the last but one user called free_irq.

The second major change makes use of this interface in KVM's PCI pass- through subsystem. KVM has to keep the interrupt source disabled while calling into the guest to handle the event. This can be done at device or line level. The former is required to share the interrupt line, the latter is an order of magnitude faster (see patch 3 for details).

Beside pass-through support of KVM, further users of the IRQ notifier could become VFIO (not yet mainline) and uio_pci_generic which have to resolve the same conflict.

Quoted 11 lines · Jan Kiszka

Besides 3 cleanup patches, this series consists of two major changes. The first introduces an interrupt sharing notifier to the genirq subsystem. It fires when an interrupt line is about to be use by more than one driver or the last but one user called free_irq.

The second major change makes use of this interface in KVM's PCI pass- through subsystem. KVM has to keep the interrupt source disabled while calling into the guest to handle the event. This can be done at device or line level. The former is required to share the interrupt line, the latter is an order of magnitude faster (see patch 3 for details).

Quoted 3 lines

Beside pass-through support of KVM, further users of the IRQ notifier could become VFIO (not yet mainline) and uio_pci_generic which have to resolve the same conflict.

Hmm. You basically want to have the following functionality:

If interrupt is shared, then you want to keep the current behaviour:

disable at line level (IRQF_ONESHOT)
run handler thread (PCI level masking) reenable at line level in irq_finalize_oneshot() reenable at PCI level when guest is done

If interrupt is not shared:

disable at line level (IRQF_ONESHOT)
run handler thread (no PCI level masking)
no reenable at line level
reenable at line level when guest is done

I think the whole notifier approach including the extra irq handlers plus the requirement to call disable_irq_nosync() from the non shared handler is overkill. We can be more clever.

The genirq code knows whether you have one or more handler registered. So we can add IRQF_ONESHOT_UNMASK_SHARED and add a status field to irq_data (which I was going to do anyway for other reasons). In that status field you get a bit which says IRQ_MASK_DEVICE.

So with IRQF_ONESHOT_UNMASK_SHARED == 0 we keep the current behaviour.

If IRQF_ONESHOT_UNMASK_SHARED== 1 and IRQ_MASK_DEVICE == 1 we keep the current behaviour.

If IRQF_ONESHOT_UNMASK_SHARED== 1 and IRQ_MASK_DEVICE == 0 then then irq_finalize_oneshot() simply marks the interrupt disabled (equivalent to disable_irq_nosync()) and returns.

Now in your primary irq handler you simply check the IRQ_MASK_DEVICE status flag and decide whether you need to mask at PCI level or not.

Your threaded handler gets the same information via IRQ_MASK_DEVICE so it can issue the appropriate user space notification depending on that flag.

This works fully transparent across adding and removing handlers. On request_irq/free_irq we update the IRQ_MASK_DEVICE flag with the following logic:

    nr_actions	IRQF_ONESHOT_UNMASK_SHARED	IRQ_MASK_DEVICE
    1		0				1
    1		1				0
    >1		don't care			1

If interrupts are in flight accross request/free then this change takes effect when the next interrupt comes in.

No notifiers, no disable_irq_nosync() magic, less and simpler code.

Thoughts ?

Quoted 7 lines · Thomas Gleixner

On Sat, 4 Dec 2010, Jan Kiszka wrote:
If interrupt is shared, then you want to keep the current behaviour:

disable at line level (IRQF_ONESHOT)
run handler thread (PCI level masking) reenable at line level in irq_finalize_oneshot() reenable at PCI level when guest is done

If the interrupt is shared, we must mask at PCI level inside the primary handler as we also have to support non-threaded users of the same line. So we always have a transition line-level -> device-level masking in a primary handler.

reduce the latency. So both threaded and non-threaded cases should be addressable by any approach.

Quoted 2 lines

If interrupts are in flight accross request/free then this change takes effect when the next interrupt comes in.

For registration, that might be too late. We need to synchronize on in-flight interrupts for that line and then ensure that it gets enabled independently of the registered user. That user may have applied outdated information, thus would block the line for too long if user space decides to do something else.

Pulling the effect of disable_irq_nosync into genirq also for threaded handling would remove the need for re-registering handlers. That's good. But we need to think about the hand-over scenarios again. Maybe solvable, but non-trivial.

Jan,

Quoted 9 lines · Jan Kiszka

Quoted 7 lines · Thomas Gleixner

On Sat, 4 Dec 2010, Jan Kiszka wrote:
If interrupt is shared, then you want to keep the current behaviour:

disable at line level (IRQF_ONESHOT)
run handler thread (PCI level masking) reenable at line level in irq_finalize_oneshot() reenable at PCI level when guest is done

Quoted 4 lines

If the interrupt is shared, we must mask at PCI level inside the primary handler as we also have to support non-threaded users of the same line. So we always have a transition line-level -> device-level masking in a primary handler.

Sorry that left out the hard irq part. Of course it needs to do the PCI level masking in the primary one.

Quoted 2 lines

reduce the latency. So both threaded and non-threaded cases should be addressable by any approach.

The oneshot magic should work on non threaded cases as well. Needs some modifications, but nothing complex.

Quoted 8 lines

Quoted 2 lines

If interrupts are in flight accross request/free then this change takes effect when the next interrupt comes in.

For registration, that might be too late. We need to synchronize on in-flight interrupts for that line and then ensure that it gets enabled independently of the registered user. That user may have applied outdated information, thus would block the line for too long if user space decides to do something else.

No, that's really just a corner case when going from one to two handlers and I don't think it matters much. If you setup a new driver it's not really important whether that first thing comes in a few ms later.

Also there is a pretty simple solution for this: The core code knows, that there is an ONESHOT interrupt in flight, so it simply can call

the primary handler of that device with the appropriate flag set (maybe an additional one to indicate the transition) and let that deal with it. Needs some thought vs. locking and races, but that shouldn't be hard.

Quoted 2 lines · Thomas Gleixner

Also there is a pretty simple solution for this: The core code knows, that there is an ONESHOT interrupt in flight, so it simply can call

It doesn't synchronize the tail part against the masking in the handler(s), that's driver business.

Quoted 4 lines

the primary handler of that device with the appropriate flag set (maybe an additional one to indicate the transition) and let that deal with it. Needs some thought vs. locking and races, but that shouldn't be hard.

Yes, I thought about this kind of transition (re-invoking the existing handler) already. We do need notification of the switch (at least for exclusive->shared) as only the driver can migrate the masking for in-flight interrupts.