irqfd

This allows an eventfd to be registered as an irq source with a guest. Any signaling operation on the eventfd (via userspace or kernel) will inject the registered GSI at the next available window.

Signed-off-by Gregory Haskins <[email protected]>

(quoted diff) +2 -0

+}
+

Quoted 4 lines · Gregory Haskins

This allows an eventfd to be registered as an irq source with a guest. Any signaling operation on the eventfd (via userspace or kernel) will inject the registered GSI at the next available window.

Quoted 2 lines

Signed-off-by Gregory Haskins <[email protected]>

If we ever want to use this with e.g. MSI-X emulation in guest, and want to be stricly compliant to MSI-X, we'll need a way for guest to mask interrupts, and for host to report that a masked interrupt is pending. Ideally, all this will be doable with a couple of mmapped pages to avoid vmexits/system calls.

Quoted 8 lines

virt/kvm/irqfd.c new file +8 -0

+static void
+irqfd_inject(struct work_struct *work)
+{
+	struct _irqfd *irqfd = container_of(work, struct _irqfd, work);
+	struct kvm *kvm = irqfd->kvm;
+
+	mutex_lock(&kvm->lock);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);

virt/kvm/irqfd.c line 54

Quoted 3 lines

virt/kvm/irqfd.c new file +2 -0

+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	mutex_unlock(&kvm->lock);

This will do weird stuff (deliver the irq twice) if the irq is MSI/MSI-X. I know this was discussed already and is a temporary shortcut, but maybe add a comment that we really want kvm_toggle_irq, so that we won't forget?

Quoted 6 lines · Michael S. Tsirkin

Quoted 4 lines · Gregory Haskins

This allows an eventfd to be registered as an irq source with a guest. Any signaling operation on the eventfd (via userspace or kernel) will inject the registered GSI at the next available window.

Quoted 10 lines

Quoted 2 lines

Signed-off-by Gregory Haskins <[email protected]>

If we ever want to use this with e.g. MSI-X emulation in guest, and want to be stricly compliant to MSI-X, we'll need a way for guest to mask interrupts, and for host to report that a masked interrupt is pending. Ideally, all this will be doable with a couple of mmapped pages to avoid vmexits/system calls.

We could do this in two ways:

- move msix entry emulation into the kernel

- require the device to support replacing its irqfd, and juggle it like so:
- guest disables msi
- replace device model fd with eventfd belonging to us
- when the device fires its eventfd, set the irq pending bit
- guest enables msi
- if the pending bit is set, fire the interrupt?
- replace device model fd with the real irqfd

I'm leaning towards the latter, though it's not an easy call.

Quoted 8 lines

Quoted 8 lines

virt/kvm/irqfd.c new file +8 -0

+static void
+irqfd_inject(struct work_struct *work)
+{
+	struct _irqfd *irqfd = container_of(work, struct _irqfd, work);
+	struct kvm *kvm = irqfd->kvm;
+
+	mutex_lock(&kvm->lock);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);

Quoted 9 lines

virt/kvm/irqfd.c line 54

Quoted 3 lines

virt/kvm/irqfd.c new file +2 -0

+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	mutex_unlock(&kvm->lock);

This will do weird stuff (deliver the irq twice) if the irq is MSI/MSI-X. I know this was discussed already and is a temporary shortcut, but maybe add a comment that we really want kvm_toggle_irq, so that we won't forget?

If so, that's a bug. MSI should ignore kvm_set_irq(..., 0).

Quoted 20 lines · Avi Kivity

Quoted 18 lines · Michael S. Tsirkin

Quoted 7 lines · Gregory Haskins

This allows an eventfd to be registered as an irq source with a guest. Any signaling operation on the eventfd (via userspace or kernel) will inject the registered GSI at the next available window.

Signed-off-by Gregory Haskins <[email protected]>

If we ever want to use this with e.g. MSI-X emulation in guest, and want to be stricly compliant to MSI-X, we'll need a way for guest to mask interrupts, and for host to report that a masked interrupt is pending. Ideally, all this will be doable with a couple of mmapped pages to avoid vmexits/system calls.

Quoted 4 lines

We could do this in two ways:

- move msix entry emulation into the kernel

It's not too bad IMO: MSIX is just a table with a list of vectors, you check the mask bit on each interrupt, if masked mark vector pending and poll until unmasked.

Quoted 9 lines

- require the device to support replacing its irqfd, and juggle it like so:
- guest disables msi
- replace device model fd with eventfd belonging to us
- when the device fires its eventfd, set the irq pending bit
- guest enables msi
- if the pending bit is set, fire the interrupt?
- replace device model fd with the real irqfd

Looks like a lot of code. No?

Quoted 2 lines

I'm leaning towards the latter, though it's not an easy call.

Actually there's a third option: add KVM_MASK_IRQ, KVM_UNMASK_IRQ ioctls which will block/unblock guest from getting interrupt on this irq, whatever the source. Interrupts are queued in kernel while masked. A third ioctl KVM_PENDING_IRQS will return the status for a set if IRQs. qemu would call these ioctls when guest edits the MSIX vector control or reads the pending bit array.

Quoted 7 lines

Actually there's a third option: add KVM_MASK_IRQ, KVM_UNMASK_IRQ ioctls which will block/unblock guest from getting interrupt on this irq, whatever the source. Interrupts are queued in kernel while masked. A third ioctl KVM_PENDING_IRQS will return the status for a set if IRQs. qemu would call these ioctls when guest edits the MSIX vector control or reads the pending bit array.

I think this is the best option.

virt/kvm/irqfd.c new file +5 -0

+	/* We re-use eventfd for irqfd */
+	fd = sys_eventfd2(0, 0);
+	if (fd < 0) {
+		ret = fd;
+		goto fail;

Quoted 11 lines · Gregory Haskins

virt/kvm/irqfd.c new file +11 -0

+	/* We re-use eventfd for irqfd */
+	fd = sys_eventfd2(0, 0);
+	if (fd < 0) {
+		ret = fd;
+		goto fail;
+	}
+
+	/* We maintain a reference to eventfd for the irqfd lifetime */
+	file = eventfd_fget(fd);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);

virt/kvm/irqfd.c line 114

Quoted 4 lines

virt/kvm/irqfd.c new file +4 -0

+		goto fail;
+	}
+
+	irqfd->file = file;

This is just plain wrong. You have no promise whatsoever that caller of that sucker won't race with e.g. dup2(). IOW, you can't assume that file will be of the expected kind.

Quoted 12 lines · Al Viro

Quoted 11 lines · Gregory Haskins

virt/kvm/irqfd.c new file +11 -0

+	/* We re-use eventfd for irqfd */
+	fd = sys_eventfd2(0, 0);
+	if (fd < 0) {
+		ret = fd;
+		goto fail;
+	}
+
+	/* We maintain a reference to eventfd for the irqfd lifetime */
+	file = eventfd_fget(fd);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);

Quoted 8 lines

virt/kvm/irqfd.c line 114

Quoted 4 lines

virt/kvm/irqfd.c new file +4 -0

+		goto fail;
+	}
+
+	irqfd->file = file;

This is just plain wrong. You have no promise whatsoever that caller of that sucker won't race with e.g. dup2(). IOW, you can't assume that file will be of the expected kind.

The eventfd_fget() checks for the file_operations pointer, before returning the file*, and fails if the fd in not an eventfd. Or you have other concerns?

General rules:
* descriptor you've generated is fit only for return to userland;
* descriptor you've got from userland is fit only for *single*
fget() or equivalent, unless you are one of the core syscalls manipulating the descriptor table itself (dup2, etc.)
* once file is installed in descriptor table, you'd better be past
the last failure exit; sys_close() on cleanup path is not acceptable. That's what reserving descriptors is for.

IOW, the sane solution would be to export something that returns your struct file *. And stop playing with fd completely.

IOW, the sane solution would be to export something that returns your struct file *. And stop playing with fd completely.

Quoted 2 lines · Al Viro

IOW, the sane solution would be to export something that returns your struct file *. And stop playing with fd completely.

This builds but it's not tested at all.

- Make all the work of the old anon_inode_getfd(), done by a new anon_inode_getfile(), with anon_inode_getfd() using its services

- Make all the work done by sys_eventfd(), done by a new eventfd_file_create() (which in turn uses anon_inode_getfile()), with sys_eventfd() using its services

IRQfd can use eventfd_file_create(), fget(), get_unused_fd_flags() and fd_install() just before returning.
Is that what you had in mind?

(Applies to kvm.git 41b76d8d0487c26d6d4d3fe53c1ff59b3236f096)

This series implements a mechanism called "irqfd". It lets you create an eventfd based file-desriptor to inject interrupts to a kvm guest. We associate one gsi per fd for fine-grained routing.

We do not have a user of this interface in this series, though note future version of virtual-bus (v4 and above) will be based on this.

The first patch will require mainline buy-in, particularly from Davide (cc'd). The last patch is kvm specific.

Quoted 2 lines · On Mon, 27 Apr 2009 14:33:24 -0400 Gregory Haskins <[email protected]> wrote:

(Applies to kvm.git 41b76d8d0487c26d6d4d3fe53c1ff59b3236f096)

Quoted 4 lines

This series implements a mechanism called "irqfd". It lets you create an eventfd based file-desriptor to inject interrupts to a kvm guest. We associate one gsi per fd for fine-grained routing.

It'd be nice if the KVM weenies amongst us were to be told why one would want to inject interrupts into a KVM guest. Monosyllabic words would be preferred ;)

Quoted 3 lines

We do not have a user of this interface in this series, though note future version of virtual-bus (v4 and above) will be based on this.

So I assume that this patchset will be merged if/when a user of it is merged?

Quoted 3 lines

The first patch will require mainline buy-in, particularly from Davide (cc'd). The last patch is kvm specific.

Three EXPORT_SYMBOL_GPL()s. Once the shouting has subsided I'd suggest that this be merged via the KVM tree.