kvm: add support for irqfd

(Applies to kvm.git/queue:fd2e987d)

KVM provides a complete virtual system environment for guests, including support for injecting interrupts modeled after the real exception/interrupt facilities present on the native platform (such as the IDT on x86). Virtual interrupts can come from a variety of sources (emulated devices, pass-through devices, etc) but all must be injected to the guest via the KVM infrastructure. This patch adds a new mechanism to inject a specific interrupt to a guest using a decoupled eventfd mechnanism: Any legal signal on the irqfd (using eventfd semantics from either userspace or kernel) will translate into an injected interrupt in the guest at the next available interrupt window.

(quoted diff) +4 -0

+static int
+irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
+{
+	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);

Quoted 10 lines · Gregory Haskins

(quoted diff) +10 -0

+static int
+irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
+{
+	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+
+	/*
+	 * The wake_up is called with interrupts disabled.  Therefore we need
+	 * to defer the IRQ injection until later since we need to acquire the
+	 * kvm->lock to do so.
+	 */

fs/eventfd.c int eventfd_signal(struct file *file, int n) · line 55

Quoted 5 lines

fs/eventfd.c +4 -0

+	schedule_work(&irqfd->work);
+
+	return 0;
+}

This schedule_work is there just to work around the spinlock in eventfd_signal, which we don't really need. Isn't this right?

And this is on each interrupt. Seems like a pity.

How about a flag in eventfd that would
convert it from waking up someone to a plain function call?

Davide, could we add something like

fs/eventfd.c +1 -0

index 2a701d5..8bfa308 100644
@@ -29,6 +29,7 @@ struct eventfd_ctx {
 	 */
 	__u64 count;
 	unsigned int flags;
+	int nolock;
 };
 
 /*

@@ -46,6 +47,12 @@ int eventfd_signal(struct file *file, int n)
 
 	if (n < 0)

Quoted 4 lines · Gregory Haskins

(quoted diff) +4 -0

+static int
+irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
+{
+	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+	/*
+	 * The wake_up is called with interrupts disabled.  Therefore we need
+	 * to defer the IRQ injection until later since we need to acquire the
+	 * kvm->lock to do so.
+	 */
+	schedule_work(&irqfd->work);
+
+	return 0;
+}

This schedule_work is there just to work around the spinlock in eventfd_signal, which we don't really need. Isn't this right? And this is on each interrupt. Seems like a pity.
How about a flag in eventfd that would
convert it from waking up someone to a plain function call?

Davide, could we add something like

fs/eventfd.c +1 -0

index 2a701d5..8bfa308 100644
@@ -29,6 +29,7 @@ struct eventfd_ctx {
 	 */
 	__u64 count;
 	unsigned int flags;
+	int nolock;
 };
 
 /*

@@ -46,6 +47,12 @@ int eventfd_signal(struct file *file, int n)
 
 	if (n < 0)

virt/kvm/eventfd.c new file +14 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);

Quoted 21 lines · Gregory Haskins

virt/kvm/eventfd.c new file +21 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {

virt/kvm/eventfd.c line 109

Quoted 4 lines

virt/kvm/eventfd.c new file +3 -0

+		ret = PTR_ERR(file);
+		goto fail;
+	}

So we get a reference to a file, and unless the user is nice to us, it will only be dropped when kvm char device file is closed?
I think this will deadlock if the fd in question is the open kvm char device.

Michael Tsirkin pointed out an issue in kvm.git w.r.t. malicious userspace configuring irqfd objects:

http://lkml.org/lkml/2009/5/27/341

This series applies to kvm.git/master:1a6a35a1 to attempt to close the vulnerability.

---

Gregory Haskins (2):
kvm: validate irqfd type
eventfd: export eventfd interfaces for module use

 fs/eventfd.c       |    3 +++
 virt/kvm/eventfd.c |    3 ++-
 2 files changed, 5 insertions(+), 1 deletions(-)

Michael Tsirkin pointed out an issue in kvm.git w.r.t. malicious userspace configuring irqfd objects:

http://lkml.org/lkml/2009/5/27/341

This series applies to kvm.git/master:1a6a35a1 to attempt to close the vulnerability.

Quoted 23 lines · Michael S. Tsirkin

Quoted 21 lines · Gregory Haskins

virt/kvm/eventfd.c new file +21 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {

Quoted 11 lines

virt/kvm/eventfd.c line 109

Quoted 4 lines

virt/kvm/eventfd.c new file +3 -0

+		ret = PTR_ERR(file);
+		goto fail;
+	}

So we get a reference to a file, and unless the user is nice to us, it will only be dropped when kvm char device file is closed?
I think this will deadlock if the fd in question is the open kvm char device.

Hmm...I hadn't considered this possibility, though I am not sure if it would cause a deadlock in the pattern you suggest. It seems more like it would result in, at worst, an extra reference to itself (and thus a leak) rather than a deadlock...

I digress. In either case, perhaps I should s/fget/eventfd_fget to at least limit the type of fd to eventfd. I was trying to be "slick" by not needing the eventfd_fget() exported, but I am going to need to export it later anyway for iosignalfd, so its probably a moot point.

Thanks Michael,
-Greg

Quoted 40 lines · Gregory Haskins

Quoted 23 lines · Michael S. Tsirkin

Quoted 21 lines · Gregory Haskins

(quoted diff) +21 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {

Quoted 11 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

+		ret = PTR_ERR(file);
+		goto fail;
+	}

So we get a reference to a file, and unless the user is nice to us, it will only be dropped when kvm char device file is closed?
I think this will deadlock if the fd in question is the open kvm char device.

Hmm...I hadn't considered this possibility, though I am not sure if it would cause a deadlock in the pattern you suggest. It seems more like it would result in, at worst, an extra reference to itself (and thus a leak) rather than a deadlock...

Quoted 8 lines

I digress. In either case, perhaps I should s/fget/eventfd_fget to at least limit the type of fd to eventfd. I was trying to be "slick" by not needing the eventfd_fget() exported, but I am going to need to export it later anyway for iosignalfd, so its probably a moot point.

Thanks Michael,
-Greg

This only works as long as eventfd does not do fget on some fd as well. Which it does not do now, and may never do - but we create a fragile system this way.

I think it's really wrong, fundamentally, to keep a reference to a file until another file is closed, unless you are code under fs/. We will get nasty circular references sooner or later.

Isn't the real reason we use fd to be able to support the same interface on top of both kvm and lguest?
And if so, wouldn't some kind of bus be a better solution?

Quoted 43 lines · Gregory Haskins

Quoted 24 lines · Michael S. Tsirkin

Quoted 21 lines · Gregory Haskins

virt/kvm/eventfd.c new file +21 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {

Quoted 12 lines

virt/kvm/eventfd.c line 109

Quoted 5 lines

virt/kvm/eventfd.c new file +3 -0

+		ret = PTR_ERR(file);
+		goto fail;
+	}

So we get a reference to a file, and unless the user is nice to us, it will only be dropped when kvm char device file is closed?
I think this will deadlock if the fd in question is the open kvm char device.

Hmm...I hadn't considered this possibility, though I am not sure if it would cause a deadlock in the pattern you suggest. It seems more like it would result in, at worst, an extra reference to itself (and thus a leak) rather than a deadlock...

Quoted 9 lines

I digress. In either case, perhaps I should s/fget/eventfd_fget to at least limit the type of fd to eventfd. I was trying to be "slick" by not needing the eventfd_fget() exported, but I am going to need to export it later anyway for iosignalfd, so its probably a moot point.

Thanks Michael,
-Greg

This only works as long as eventfd does not do fget on some fd as well. Which it does not do now, and may never do - but we create a fragile system this way.

I think it's really wrong, fundamentally, to keep a reference to a file until another file is closed, unless you are code under fs/. We will get nasty circular references sooner or later.

Isn't the real reason we use fd to be able to support the same interface on top of both kvm and lguest?

And if so, wouldn't some kind of bus be a better solution?

Quoted 55 lines · Michael S. Tsirkin

Quoted 43 lines · Gregory Haskins

Quoted 24 lines · Michael S. Tsirkin

Quoted 21 lines · Gregory Haskins

virt/kvm/eventfd.c new file +21 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {

Quoted 12 lines

virt/kvm/eventfd.c line 109

Quoted 5 lines

virt/kvm/eventfd.c new file +3 -0

+		ret = PTR_ERR(file);
+		goto fail;
+	}

So we get a reference to a file, and unless the user is nice to us, it will only be dropped when kvm char device file is closed?
I think this will deadlock if the fd in question is the open kvm char device.

Hmm...I hadn't considered this possibility, though I am not sure if it would cause a deadlock in the pattern you suggest. It seems more like it would result in, at worst, an extra reference to itself (and thus a leak) rather than a deadlock...

Quoted 9 lines

I digress. In either case, perhaps I should s/fget/eventfd_fget to at least limit the type of fd to eventfd. I was trying to be "slick" by not needing the eventfd_fget() exported, but I am going to need to export it later anyway for iosignalfd, so its probably a moot point.

Thanks Michael,
-Greg

Quoted 8 lines

This only works as long as eventfd does not do fget on some fd as well. Which it does not do now, and may never do - but we create a fragile system this way.

I think it's really wrong, fundamentally, to keep a reference to a file until another file is closed, unless you are code under fs/. We will get nasty circular references sooner or later.

Hmm.. I understand your concern, but I respectfully disagree.

One object referencing another is a natural expression, regardless of what type they may be. The fact is that introducing the concept of irqfd creates a relationship between an eventfd instance and a kvm instance whether we like it or not, and this relationship needs to be managed. It is therefore IMO perfectly natural to express that relationship with a reference count, and I do not currently see anything wrong or even particularly fragile about how I've currently done this. I'm sure there are other ways, however. Do you have a particular suggestion in mind?

Quoted 3 lines

Isn't the real reason we use fd to be able to support the same interface on top of both kvm and lguest?

Actually, the reason why we use an fd is to decouple the interrupt-producing end-point from the KVM core. Ignoring eventfd in specific for a moment, one convenient way to do that is with an fd because it provides a nice, already written/tested handle-to-pointer translation, and a polymorphic interface (e.g. f_ops). Choosing to use eventfd flavored fd's buys us additional advantages in terms of leveraging already tested f_ops code, and compatibility with an interface that is designed-for/used-by other established subsystems for signaling.

Quoted 2 lines

And if so, wouldn't some kind of bus be a better solution?

Ultimately I aim to implement a bus (vbus, specifically) in terms of irqfd (and iosignalfd, for that matter). However, the eventfd interfaces are general purpose and can be used in other areas as well (for instance, virtio-pci, or the shared-mem driver recently discussed). I realize this is probably not the point you were making here, but fyi.

Regards,
-Greg

Quoted 2 lines · Gregory Haskins

Do you have a particular suggestion in mind?

Yes. I'll try to post a patch soon.

virt/kvm/eventfd.c new file +122 -0

new file mode 100644
index 0000000..72a282e
@@ -0,0 +1,228 @@
+/*
+ * kvm eventfd support - use eventfd objects to signal various KVM events
+ *
+ * Copyright 2009 Novell.  All Rights Reserved.
+ *
+ * Author:
+ *	Gregory Haskins <[email protected]>
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.
+ */
+
+#include <linux/kvm_host.h>
+#include <linux/workqueue.h>
+#include <linux/syscalls.h>
+#include <linux/wait.h>
+#include <linux/poll.h>
+#include <linux/file.h>
+#include <linux/list.h>
+
+/*
+ * --------------------------------------------------------------------
+ * irqfd: Allows an fd to be used to inject an interrupt to the guest
+ *
+ * Credit goes to Avi Kivity for the original idea.
+ * --------------------------------------------------------------------
+ */
+struct _irqfd {
+	struct kvm               *kvm;
+	int                       gsi;
+	struct file              *file;
+	struct list_head          list;
+	poll_table                pt;
+	wait_queue_head_t        *wqh;
+	wait_queue_t              wait;
+	struct work_struct        work;
+};
+
+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);
+	kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+	mutex_unlock(&kvm->lock);
+}
+
+static int
+irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
+{
+	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
+
+	/*
+	 * The wake_up is called with interrupts disabled.  Therefore we need
+	 * to defer the IRQ injection until later since we need to acquire the
+	 * kvm->lock to do so.
+	 */
+	schedule_work(&irqfd->work);
+
+	return 0;
+}
+
+static void
+irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
+			poll_table *pt)
+{
+	struct _irqfd *irqfd = container_of(pt, struct _irqfd, pt);
+
+	irqfd->wqh = wqh;
+	add_wait_queue(wqh, &irqfd->wait);
+}
+
+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	/*
+	 * Install our own custom wake-up handling so we are notified via
+	 * a callback whenever someone signals the underlying eventfd
+	 */
+	init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+	init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
+
+	ret = file->f_op->poll(file, &irqfd->pt);
+	if (ret < 0)
+		goto fail;
+
+	irqfd->file = file;

virt/kvm/eventfd.c new file +96 -0

+
+	if (file && !IS_ERR(file))
+		fput(file);
+
+	kfree(irqfd);
+	return ret;
+}
+
+static void
+irqfd_release(struct _irqfd *irqfd)
+{
+	/*
+	 * The ordering is important.  We must remove ourselves from the wqh
+	 * first to ensure no more event callbacks are issued, and then flush
+	 * any previously scheduled work prior to freeing the memory
+	 */
+	remove_wait_queue(irqfd->wqh, &irqfd->wait);
+
+	flush_work(&irqfd->work);
+
+	fput(irqfd->file);
+	kfree(irqfd);
+}
+
+static struct _irqfd *
+irqfd_remove(struct kvm *kvm, struct file *file, int gsi)
+{
+	struct _irqfd *irqfd;
+
+	mutex_lock(&kvm->lock);
+
+	/*
+	 * linear search isn't brilliant, but this should be an infrequent
+	 * slow-path operation, and the list should not grow very large
+	 */
+	list_for_each_entry(irqfd, &kvm->irqfds, list) {
+		if (irqfd->file != file || irqfd->gsi != gsi)
+			continue;
+
+		list_del(&irqfd->list);
+		mutex_unlock(&kvm->lock);
+
+		return irqfd;
+	}
+
+	mutex_unlock(&kvm->lock);
+
+	return NULL;
+}
+
+static int
+kvm_deassign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file;
+	int count = 0;
+
+	file = fget(fd);
+	if (IS_ERR(file))
+		return PTR_ERR(file);
+
+	while ((irqfd = irqfd_remove(kvm, file, gsi))) {
+		/*
+		 * We remove the item from the list under the lock, but we
+		 * free it outside the lock to avoid deadlocking with the
+		 * flush_work and the work_item taking the lock
+		 */
+		irqfd_release(irqfd);
+		count++;
+	}
+
+	fput(file);
+
+	return count ? count : -ENOENT;
+}
+
+int
+kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
+{
+	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
+		return kvm_deassign_irqfd(kvm, fd, gsi);
+
+	return kvm_assign_irqfd(kvm, fd, gsi);
+}
+
+void
+kvm_irqfd_release(struct kvm *kvm)
+{
+	struct _irqfd *irqfd, *tmp;
+
+	/* don't bother with the lock..we are shutting down */
+	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
+		list_del(&irqfd->list);
+		irqfd_release(irqfd);
+	}
+}

virt/kvm/eventfd.c line 120

Quoted 4 lines

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

+
+	ret = file->f_op->poll(file, &irqfd->pt);
+	if (ret < 0)
+		goto fail;

virt/kvm/eventfd.c line 132

Quoted 12 lines

virt/kvm/eventfd.c new file +12 -0

+
+	irqfd->file = file;
+
+	mutex_lock(&kvm->lock);
+	list_add_tail(&irqfd->list, &kvm->irqfds);
+	mutex_unlock(&kvm->lock);
+
+	return 0;
+
+fail:
+	if (irqfd->wqh)
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);

Why are these 2 lines here? Either we might get a callback even though poll failed - and then this test without lock is probably racy - or we can't, and then we can replace the above with BUG_ON(irqfd->wqh).

Which is it? I think the later ...

Quoted 8 lines

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

+
+	if (file && !IS_ERR(file))
+		fput(file);
+
+	kfree(irqfd);
+	return ret;
+}
+

Quoted 5 lines · Michael S. Tsirkin

virt/kvm/eventfd.c line 120

Quoted 5 lines

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

+
+	ret = file->f_op->poll(file, &irqfd->pt);
+	if (ret < 0)
+		goto fail;

Looking at it some more, we have:
struct file_operations {
....

	unsigned int (*poll) (struct file *, struct poll_table_struct *);

So the comparison above does not seem to make sense:
it seems that the return value from poll can not be negative.

Will the callback be executed if someone did a write to eventfd before we attached it? If no, maybe we should call it here if ret != 0.

Quoted 32 lines

Quoted 5 lines

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

+
+	irqfd->file = file;

virt/kvm/eventfd.c line 132

Quoted 10 lines

virt/kvm/eventfd.c new file +10 -0

+
+	mutex_lock(&kvm->lock);
+	list_add_tail(&irqfd->list, &kvm->irqfds);
+	mutex_unlock(&kvm->lock);
+
+	return 0;
+
+fail:
+	if (irqfd->wqh)
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);

Why are these 2 lines here? Either we might get a callback even though poll failed - and then this test without lock is probably racy - or we can't, and then we can replace the above with BUG_ON(irqfd->wqh).

Which is it? I think the later ...

Quoted 9 lines

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

+
+	if (file && !IS_ERR(file))
+		fput(file);
+
+	kfree(irqfd);
+	return ret;
+}
+

Quoted 11 lines · Michael S. Tsirkin

Quoted 5 lines · Michael S. Tsirkin

virt/kvm/eventfd.c line 120

Quoted 5 lines

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

+
+	ret = file->f_op->poll(file, &irqfd->pt);
+	if (ret < 0)
+		goto fail;

Looking at it some more, we have:
struct file_operations {
....

Quoted 5 lines

	unsigned int (*poll) (struct file *, struct poll_table_struct *);

So the comparison above does not seem to make sense:
it seems that the return value from poll can not be negative.

Indeed. Will fix.

Quoted 4 lines

Will the callback be executed if someone did a write to eventfd before we attached it? If no, maybe we should call it here if ret != 0.

I do the cleanup in case the callback has been called, but poll() fails somewhere internally afterwards. Perhaps this is not a realistic scenario, but it was my motivation for adding the wqh cleanup.

Quoted 31 lines

Quoted 31 lines

Quoted 4 lines

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

+
+	irqfd->file = file;

virt/kvm/eventfd.c line 132

Quoted 10 lines

virt/kvm/eventfd.c new file +10 -0

+
+	mutex_lock(&kvm->lock);
+	list_add_tail(&irqfd->list, &kvm->irqfds);
+	mutex_unlock(&kvm->lock);
+
+	return 0;
+
+fail:
+	if (irqfd->wqh)
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);

Why are these 2 lines here? Either we might get a callback even though poll failed - and then this test without lock is probably racy - or we can't, and then we can replace the above with BUG_ON(irqfd->wqh).

Which is it? I think the later ...

Quoted 9 lines

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

+
+	if (file && !IS_ERR(file))
+		fput(file);
+
+	kfree(irqfd);
+	return ret;
+}
+

virt/kvm/eventfd.c +44 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	/*
+	 * Install our own custom wake-up handling so we are notified via
+	 * a callback whenever someone signals the underlying eventfd
+	 */
+	init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+	init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
+
+	ret = file->f_op->poll(file, &irqfd->pt);
+	if (ret < 0)
+		goto fail;
+
+	irqfd->file = file;
+
+	mutex_lock(&kvm->lock);
+	list_add_tail(&irqfd->list, &kvm->irqfds);
+	mutex_unlock(&kvm->lock);
+
+	return 0;
+

On Wed, May 20, 2009 at 10:30:49AM -0400, Gregory Haskins wrote:

...

Quoted 46 lines

virt/kvm/eventfd.c +44 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	/*
+	 * Install our own custom wake-up handling so we are notified via
+	 * a callback whenever someone signals the underlying eventfd
+	 */
+	init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+	init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
+
+	ret = file->f_op->poll(file, &irqfd->pt);
+	if (ret < 0)
+		goto fail;
+
+	irqfd->file = file;
+
+	mutex_lock(&kvm->lock);
+	list_add_tail(&irqfd->list, &kvm->irqfds);
+	mutex_unlock(&kvm->lock);
+
+	return 0;
+

virt/kvm/eventfd.c line 139

Quoted 10 lines

virt/kvm/eventfd.c +10 -0

+fail:
+	if (irqfd->wqh)
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);
+
+	if (file && !IS_ERR(file))
+		fput(file);
+
+	kfree(irqfd);
+	return ret;
+}

It seems that this lets the guest assign an unlimited number of fds to the same gsi, potentially using up all of kernel memory.

Since we don't need multiple fds assigned to the same gsi (instead, multiple processes can write to the same eventfd to trigger an interrupt) let's simply check that no fd is yet assigned to this gsi.

Quoted 65 lines · Michael S. Tsirkin

On Wed, May 20, 2009 at 10:30:49AM -0400, Gregory Haskins wrote:

...

Quoted 46 lines

virt/kvm/eventfd.c +44 -0

+static int
+kvm_assign_irqfd(struct kvm *kvm, int fd, int gsi)
+{
+	struct _irqfd *irqfd;
+	struct file *file = NULL;
+	int ret;
+
+	irqfd = kzalloc(sizeof(*irqfd), GFP_KERNEL);
+	if (!irqfd)
+		return -ENOMEM;
+
+	irqfd->kvm = kvm;
+	irqfd->gsi = gsi;
+	INIT_LIST_HEAD(&irqfd->list);
+	INIT_WORK(&irqfd->work, irqfd_inject);
+
+	/*
+	 * Embed the file* lifetime in the irqfd.
+	 */
+	file = fget(fd);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto fail;
+	}
+
+	/*
+	 * Install our own custom wake-up handling so we are notified via
+	 * a callback whenever someone signals the underlying eventfd
+	 */
+	init_waitqueue_func_entry(&irqfd->wait, irqfd_wakeup);
+	init_poll_funcptr(&irqfd->pt, irqfd_ptable_queue_proc);
+
+	ret = file->f_op->poll(file, &irqfd->pt);
+	if (ret < 0)
+		goto fail;
+
+	irqfd->file = file;
+
+	mutex_lock(&kvm->lock);
+	list_add_tail(&irqfd->list, &kvm->irqfds);
+	mutex_unlock(&kvm->lock);
+
+	return 0;
+

virt/kvm/eventfd.c line 139

Quoted 10 lines

virt/kvm/eventfd.c +10 -0

+fail:
+	if (irqfd->wqh)
+		remove_wait_queue(irqfd->wqh, &irqfd->wait);
+
+	if (file && !IS_ERR(file))
+		fput(file);
+
+	kfree(irqfd);
+	return ret;
+}

It seems that this lets the guest assign an unlimited number of fds to the same gsi, potentially using up all of kernel memory.

Quoted 4 lines

Since we don't need multiple fds assigned to the same gsi (instead, multiple processes can write to the same eventfd to trigger an interrupt) let's simply check that no fd is yet assigned to this gsi.

I think Avi asked for this specific feature during review which is the reason why its there today. However, I agree that it would probably be a good idea to put an upper limit on the number of supported aliases that can be registered. Will fix.

Thanks Michael,

-Greg

Quoted 11 lines

I think Avi asked for this specific feature during review which is the reason why its there today. However, I agree that it would probably be a good idea to put an upper limit on the number of supported aliases that can be registered. Will fix.

Thanks Michael,

-Greg

Avi, can you elaborate on why do we want to map multiple fds to the same gsi? I think it's better to allow a 1:1 mapping
only: if many processes want to trigger interrupts they can all write to the same fd.

Quoted 13 lines · Michael S. Tsirkin

Quoted 11 lines

I think Avi asked for this specific feature during review which is the reason why its there today. However, I agree that it would probably be a good idea to put an upper limit on the number of supported aliases that can be registered. Will fix.

Thanks Michael,

-Greg

Quoted 5 lines

Avi, can you elaborate on why do we want to map multiple fds to the same gsi? I think it's better to allow a 1:1 mapping
only: if many processes want to trigger interrupts they can all write to the same fd.

I don't want to assume that the eventfds all come from the same source.

That said, we have a workaround, allocate a new gsi with the same routes and attach the excess eventfds there.

Quoted 4 lines

I don't want to assume that the eventfds all come from the same source.

That said, we have a workaround, allocate a new gsi with the same routes and attach the excess eventfds there.

Right. So you are ok with 1:1 irqfd:gsi requirement for now? This seems nicer than N:1 with an arbitrary N

Quoted 2 lines · Michael S. Tsirkin

Quoted 2 lines

I don't want to assume that the eventfds all come from the same source.

Quoted 6 lines

Quoted 3 lines

That said, we have a workaround, allocate a new gsi with the same routes and attach the excess eventfds there.

Right. So you are ok with 1:1 irqfd:gsi requirement for now? This seems nicer than N:1 with an arbitrary N

Not too happy, but okay.