kvm: irqfd support for level interrupts

RFC PATCH In review Michael S. Tsirkin 4 participants 13 messages Jul 26, 2009 – Aug 19, 2009 KVM

Share & tools

Michael S. Tsirkin

author

2009-07-26 16:23 UTC #0 lore raw

RFC PATCH 1/1 kvm: irqfd support for level interrupts

Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.

Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?

Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +23 -3

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))
+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);
 
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }
 
 /*

Gleb Natapov

2009-07-27 05:33 UTC #1 lore raw

Quoted 5 lines · Michael S. Tsirkin

Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.

Do we want level triggered irqfd to be used only for device assignment? Because if not it is not appropriate to zero irq level on ack. And you have no other way to zero irq level?! Another note is that level triggered irqfd shouldn't use KVM_USERSPACE_IRQ_SOURCE_ID since it may be shared.

Quoted 103 lines

Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?

Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +20 -2

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))

virt/kvm/eventfd.c kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi) · line 310

Quoted 10 lines

virt/kvm/eventfd.c +3 -1

+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);
 
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }
 
 /*

--
Gleb.

Michael S. Tsirkin

author

2009-07-27 06:29 UTC #2 lore raw

Quoted 7 lines · Gleb Natapov

Quoted 5 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.
Do we want level triggered irqfd to be used only for device assignment?

generally level triggered-not only.
But irqfd is promarily for device assignment and emulated devices.

Quoted 1 line

Because if not it is not appropriate to zero irq level on ack.

The bit that is missing in this patch, is that we'll signal eventfd after we zero irq. Userspace polls that and re-asserts irq.

Quoted 2 lines

And you
have no other way to zero irq level?!

The reason is interrupt sharing: device can assert interrupt but can not clear it by itself.
qemu can still use the SET_IRQ ioctl if it wants full control.

Quoted 3 lines

Another note is that level
triggered irqfd shouldn't use KVM_USERSPACE_IRQ_SOURCE_ID since it may be shared.

Since we zero on ack, I think this is not a problem.

Quoted 116 lines

Quoted 103 lines

Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?

Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +20 -2

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))

virt/kvm/eventfd.c kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi) · line 310

Quoted 10 lines

virt/kvm/eventfd.c +3 -1

+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);
 
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }
 
 /*

--
Gleb.

Gleb Natapov

2009-07-27 07:31 UTC #3 lore raw

Quoted 7 lines · Michael S. Tsirkin

Quoted 6 lines · Gleb Natapov
Quoted 5 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.

Quoted 5 lines

Quoted 1 line
Do we want level triggered irqfd to be used only for device assignment?
generally level triggered-not only.
But irqfd is promarily for device assignment and emulated devices.

If it is for emulated device zeroing irq level on ack is not acceptable.

Quoted 4 lines

Quoted 1 line
Because if not it is not appropriate to zero irq level on ack.
The bit that is missing in this patch, is that we'll signal eventfd after we zero irq. Userspace polls that and re-asserts irq.

That is not needed for device emulation. Emulated device should lower irq line by itself. It know exactly when to do it. Please don't push broken device assignment model to emulated devices.

Quoted 6 lines

Quoted 3 lines
And you
have no other way to zero irq level?!
The reason is interrupt sharing: device can assert interrupt but can not clear it by itself.

For interrupt sharing to work you need to allocate source id for each irqfd.

Quoted 9 lines

qemu can still use the SET_IRQ ioctl if it wants full control.
Quoted 3 lines
Another note is that level
triggered irqfd shouldn't use KVM_USERSPACE_IRQ_SOURCE_ID since it may be shared.
Since we zero on ack, I think this is not a problem.

We will not zero on ack for emulated devices. Please don't abuse ack notifiers. Even if you zero on ack this is still the problem BTW since other device may lower your line level.

Quoted 108 lines

Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?

Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +22 -2

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))
+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);

Quoted 8 lines

Quoted 8 lines
virt/kvm/eventfd.c kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi) · line 310
Quoted 5 lines
virt/kvm/eventfd.c +1 -1
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }
 
 /*
--
Gleb.

--
Gleb.

Michael S. Tsirkin

author

2009-07-27 08:03 UTC #4 lore raw

Quoted 8 lines · Gleb Natapov

Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Gleb Natapov
Quoted 5 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.

Quoted 6 lines

Quoted 5 lines
Quoted 1 line
Do we want level triggered irqfd to be used only for device assignment?
generally level triggered-not only.
But irqfd is promarily for device assignment and emulated devices.
If it is for emulated device zeroing irq level on ack is not acceptable.

OK, forget about emulated devices for now. Let's focus on assigned devices.

Quoted 9 lines

Quoted 4 lines
Quoted 1 line
Because if not it is not appropriate to zero irq level on ack.
The bit that is missing in this patch, is that we'll signal eventfd after we zero irq. Userspace polls that and re-asserts irq.
That is not needed for device emulation. Emulated device should lower irq line by itself. It know exactly when to do it. Please don't push broken device assignment model to emulated devices.
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
And you
have no other way to zero irq level?!
The reason is interrupt sharing: device can assert interrupt but can not clear it by itself.
For interrupt sharing to work you need to allocate source id for each irqfd.

BTW, current SET_IRQ_LINE interface uses a common source id for all irqs. Does this mean that interrupt sharing in guest is broken now?

Quoted 132 lines

Quoted 9 lines

qemu can still use the SET_IRQ ioctl if it wants full control.
Quoted 3 lines
Another note is that level
triggered irqfd shouldn't use KVM_USERSPACE_IRQ_SOURCE_ID since it may be shared.
Since we zero on ack, I think this is not a problem.

We will not zero on ack for emulated devices. Please don't abuse ack notifiers. Even if you zero on ack this is still the problem BTW since other device may lower your line level.

Quoted 108 lines

Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?

Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +22 -2

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))
+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);

Quoted 8 lines

Quoted 8 lines
virt/kvm/eventfd.c kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi) · line 310
Quoted 5 lines
virt/kvm/eventfd.c +1 -1
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }
 
 /*
--
Gleb.

--
Gleb.

Gleb Natapov

2009-07-27 08:13 UTC #5 lore raw

Quoted 11 lines · Michael S. Tsirkin

Quoted 10 lines · Gleb Natapov
Quoted 9 lines · Michael S. Tsirkin
Quoted 7 lines · Gleb Natapov
Quoted 5 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.
Do we want level triggered irqfd to be used only for device assignment?

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
generally level triggered-not only.
But irqfd is promarily for device assignment and emulated devices.
If it is for emulated device zeroing irq level on ack is not acceptable.
OK, forget about emulated devices for now. Let's focus on assigned devices.

Can't. We should design interface that works for everything.

Quoted 12 lines

Quoted 12 lines
Quoted 4 lines
Quoted 1 line
Because if not it is not appropriate to zero irq level on ack.
The bit that is missing in this patch, is that we'll signal eventfd after we zero irq. Userspace polls that and re-asserts irq.
That is not needed for device emulation. Emulated device should lower irq line by itself. It know exactly when to do it. Please don't push broken device assignment model to emulated devices.
Quoted 4 lines
Quoted 3 lines
And you
have no other way to zero irq level?!

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
The reason is interrupt sharing: device can assert interrupt but can not clear it by itself.
For interrupt sharing to work you need to allocate source id for each irqfd.
BTW, current SET_IRQ_LINE interface uses a common source id for all irqs. Does this mean that interrupt sharing in guest is broken now?

No. It means that sharing in handled in userspace by chipset emulation.

Quoted 126 lines

Quoted 10 lines

qemu can still use the SET_IRQ ioctl if it wants full control.
Quoted 3 lines
Another note is that level
triggered irqfd shouldn't use KVM_USERSPACE_IRQ_SOURCE_ID since it may be shared.
Since we zero on ack, I think this is not a problem.

We will not zero on ack for emulated devices. Please don't abuse ack notifiers. Even if you zero on ack this is still the problem BTW since other device may lower your line level.

Quoted 112 lines

Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?

Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +23 -3

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))
+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);
 
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
virt/kvm/eventfd.c kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi) · line 310
Quoted 1 line
virt/kvm/eventfd.c +0 -0
 /*
--
Gleb.
--
Gleb.

--
Gleb.

Michael S. Tsirkin

author

2009-07-27 08:22 UTC #6 lore raw

Quoted 15 lines · Gleb Natapov

Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
Quoted 7 lines · Gleb Natapov
Quoted 5 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.
Do we want level triggered irqfd to be used only for device assignment?
generally level triggered-not only.
But irqfd is promarily for device assignment and emulated devices.

Quoted 6 lines

Quoted 5 lines
Quoted 1 line
If it is for emulated device zeroing irq level on ack is not acceptable.
OK, forget about emulated devices for now. Let's focus on assigned devices.
Can't. We should design interface that works for everything.

We can always add the KVM_MAKE_COFFEE ioctl later.

Quoted 12 lines

Quoted 12 lines
Quoted 12 lines
Quoted 4 lines
Quoted 1 line
Because if not it is not appropriate to zero irq level on ack.
The bit that is missing in this patch, is that we'll signal eventfd after we zero irq. Userspace polls that and re-asserts irq.
That is not needed for device emulation. Emulated device should lower irq line by itself. It know exactly when to do it. Please don't push broken device assignment model to emulated devices.
Quoted 4 lines
Quoted 3 lines
And you
have no other way to zero irq level?!

Quoted 8 lines

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
The reason is interrupt sharing: device can assert interrupt but can not clear it by itself.
For interrupt sharing to work you need to allocate source id for each irqfd.
BTW, current SET_IRQ_LINE interface uses a common source id for all irqs. Does this mean that interrupt sharing in guest is broken now?
No. It means that sharing in handled in userspace by chipset emulation.

Aha. Where's that code in qemu-kvm? I only see it forwarding to the ioctl call directly. And what about assigned devices? These bypass qemu.

Quoted 136 lines

Quoted 126 lines

Quoted 10 lines

qemu can still use the SET_IRQ ioctl if it wants full control.
Quoted 3 lines
Another note is that level
triggered irqfd shouldn't use KVM_USERSPACE_IRQ_SOURCE_ID since it may be shared.
Since we zero on ack, I think this is not a problem.

We will not zero on ack for emulated devices. Please don't abuse ack notifiers. Even if you zero on ack this is still the problem BTW since other device may lower your line level.

Quoted 112 lines

Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?

Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +23 -3

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))
+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);
 
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }

Quoted 7 lines

Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
virt/kvm/eventfd.c kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi) · line 310
Quoted 1 line
virt/kvm/eventfd.c +0 -0
 /*
--
Gleb.
--
Gleb.

--
Gleb.

Gleb Natapov

2009-07-27 08:31 UTC #7 lore raw

Quoted 16 lines · Michael S. Tsirkin

Quoted 15 lines · Gleb Natapov
Quoted 14 lines · Michael S. Tsirkin
Quoted 13 lines · Gleb Natapov
Quoted 12 lines · Michael S. Tsirkin
Quoted 7 lines · Gleb Natapov
Quoted 5 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.
Do we want level triggered irqfd to be used only for device assignment?
generally level triggered-not only.
But irqfd is promarily for device assignment and emulated devices.

Quoted 9 lines

Quoted 6 lines
Quoted 5 lines
Quoted 1 line
If it is for emulated device zeroing irq level on ack is not acceptable.
OK, forget about emulated devices for now. Let's focus on assigned devices.
Can't. We should design interface that works for everything.
We can always add the KVM_MAKE_COFFEE ioctl later.

Assigned devices are non interesting case, so we can hack something afterwards to make it work. Emulated devices is a normal case that should work when interface is introduced. Why assigning devices through the kernel is not good enough? What is the reason to push it to userspace?

Quoted 14 lines

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
Quoted 4 lines
Quoted 1 line
Because if not it is not appropriate to zero irq level on ack.
The bit that is missing in this patch, is that we'll signal eventfd after we zero irq. Userspace polls that and re-asserts irq.
That is not needed for device emulation. Emulated device should lower irq line by itself. It know exactly when to do it. Please don't push broken device assignment model to emulated devices.
Quoted 6 lines
Quoted 3 lines
And you
have no other way to zero irq level?!
The reason is interrupt sharing: device can assert interrupt but can not clear it by itself.

Quoted 8 lines

Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
For interrupt sharing to work you need to allocate source id for each irqfd.
BTW, current SET_IRQ_LINE interface uses a common source id for all irqs. Does this mean that interrupt sharing in guest is broken now?
No. It means that sharing in handled in userspace by chipset emulation.
Aha. Where's that code in qemu-kvm? I only see it forwarding to the

AFAIR in hw/pci.c and/or hw/piix_pci.c.

Quoted 2 lines

ioctl call directly. And what about assigned devices? These bypass qemu.

Each assigned device allocates its own source id. This is the reason source id exists at all.

--
Gleb.

Avi Kivity

2009-08-19 12:17 UTC #8 lore raw

In reply to Gleb Natapov

Quoted 10 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.
Do we want level triggered irqfd to be used only for device assignment?

If it works, it should work everywhere.

Signature · 1 line

error compiling committee.c: too many arguments to function

Gregory Haskins

Acked-by on v0 1/1

2009-07-29 16:01 UTC #9 lore raw

In reply to Michael S. Tsirkin

Quoted 5 lines · Michael S. Tsirkin

Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.

Quoted 7 lines

Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?
Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.
Signed-off-by Michael S. Tsirkin <[email protected]>

Patch seems reasonable to me.

Acked-by Gregory Haskins <[email protected]>

Quoted 110 lines

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +23 -4

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))
+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);
 
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }
 
 /*
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Gregory Haskins

2009-07-29 16:03 UTC #10 lore raw

Quoted 9 lines · Gregory Haskins

Quoted 8 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.
Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?

Quoted 8 lines

Quoted 4 lines
Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.
Signed-off-by Michael S. Tsirkin <[email protected]>
Patch seems reasonable to me.
Acked-by Gregory Haskins <[email protected]>

Actually, one thing I didn't think of earlier is whether a CAP_IRQFD_LEVEL kind of feature is required. I would think not, since the baseline IRQFD hasnt been officially published yet.

But its something to consider.

-Greg

Quoted 112 lines

Quoted 110 lines

---

include/linux/kvm.h +1 -0

index 230a91a..8bf16af 100644
@@ -488,6 +488,7 @@ struct kvm_x86_mce {
 #endif
 
 #define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+#define KVM_IRQFD_FLAG_LEVEL (1 << 1)
 
 struct kvm_irqfd {
 	__u32 fd;

virt/kvm/eventfd.c +23 -4

index 99017e8..fcbf5b5 100644
@@ -45,12 +45,14 @@ struct _irqfd {
 	struct kvm               *kvm;
 	struct eventfd_ctx       *eventfd;
 	int                       gsi;
+	int                       is_level;
 	struct list_head          list;
 	poll_table                pt;
 	wait_queue_head_t        *wqh;
 	wait_queue_t              wait;
 	struct work_struct        inject;
 	struct work_struct        shutdown;
+	struct kvm_irq_ack_notifier kian;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
 	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 	mutex_unlock(&kvm->irq_lock);
 }
 
+static void irqfd_irq_acked(struct kvm_irq_ack_notifier *kian)
+{
+	kvm_set_irq(kian->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, kian->gsi, 0);
+}
 /*
  * Race-free decouple logic (ordering is critical)
  */

@@ -87,6 +94,9 @@ irqfd_shutdown(struct work_struct *work)
 	 */
 	flush_work(&irqfd->inject);
 
+	if (irqfd->is_level)
+		kvm_unregister_irq_ack_notifier(&irqfd->kian);
+
 	/*
 	 * It is now safe to release the object's resources
 	 */

@@ -166,7 +176,7 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
 }
 
 static int
-kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi, int is_level)
 {
 	struct _irqfd *irqfd;
 	struct file *file = NULL;

@@ -180,6 +190,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->kvm = kvm;
 	irqfd->gsi = gsi;
+	irqfd->is_level = is_level;
 	INIT_LIST_HEAD(&irqfd->list);
 	INIT_WORK(&irqfd->inject, irqfd_inject);
 	INIT_WORK(&irqfd->shutdown, irqfd_shutdown);

@@ -198,6 +209,12 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 
 	irqfd->eventfd = eventfd;
 
+	if (is_level) {
+		irqfd->kian.gsi = gsi;
+		irqfd->kian.irq_acked = irqfd_irq_acked;
+		kvm_register_irq_ack_notifier(&irqfd->kian);
+	}
+
 	/*
 	 * Install our own custom wake-up handling so we are notified via
 	 * a callback whenever someone signals the underlying eventfd

@@ -281,10 +298,13 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 int
 kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 {
+	if (flags & ~(KVM_IRQFD_FLAG_DEASSIGN | KVM_IRQFD_FLAG_LEVEL))
+		return -EINVAL;
+
 	if (flags & KVM_IRQFD_FLAG_DEASSIGN)
 		return kvm_irqfd_deassign(kvm, fd, gsi);
 
-	return kvm_irqfd_assign(kvm, fd, gsi);
+	return kvm_irqfd_assign(kvm, fd, gsi, !!(flags & KVM_IRQFD_FLAG_LEVEL));
 }
 
 /*
--

To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Avi Kivity

2009-08-19 12:20 UTC #11 lore raw

In reply to Michael S. Tsirkin

Quoted 11 lines · Michael S. Tsirkin

Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.
Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?
Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.

What about the state-capable eventfd? Seems much more natural for this.

virt/kvm/eventfd.c irqfd_inject(struct work_struct *work) · line 70

Quoted 10 lines

static struct workqueue_struct *irqfd_cleanup_wq;

virt/kvm/eventfd.c +2 -1

@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
  	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);

e.g. inject the eventfd->state as the irq level.

Signature · 1 line

error compiling committee.c: too many arguments to function

Michael S. Tsirkin

author

2009-08-19 13:10 UTC #12 lore raw

Quoted 6 lines · Avi Kivity

Quoted 5 lines · Michael S. Tsirkin
Here's an untested patch with partial support for level triggered interrupts in irqfd. What this patch has: support for clearing interrupt on ack. What this patch does not have: support signalling eventfd on ack so that userspace can take action and e.g. reenable interrupt.

Quoted 8 lines

Quoted 6 lines
Gleb, Marcelo, I'd like your input on the approach taken wrt locking. Does it look sane?
Avi, how's the interface? I intend to also add an eventfd probably in the padding in the irqfd struct.
What about the state-capable eventfd?

This patch precedes that effort.

Quoted 1 line

Seems much more natural for this.

Good. Could you please review and ack the state-capable eventfd patch then, so I can try pushing it upstream?

Quoted 15 lines

virt/kvm/eventfd.c irqfd_inject(struct work_struct *work) · line 70
Quoted 10 lines
static struct workqueue_struct *irqfd_cleanup_wq;
virt/kvm/eventfd.c +2 -1
@@ -63,10 +65,15 @@ irqfd_inject(struct work_struct *work)
 
  	mutex_lock(&kvm->irq_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);
+	if (!irqfd->is_level)
+		kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
 
e.g. inject the eventfd->state as the irq level.
Signature · 1 line
error compiling committee.c: too many arguments to function