kvm: fast-path msi injection with irqfd

Store irq routing table pointer in the irqfd object, and use that to inject MSI directly without bouncing out to a kernel thread.

While we touch this structure, rearrange irqfd fields to make fastpath better packed for better cache utilization.

Some notes on the design:
- Use pointer into the rt instead of copying an entry, to make it possible to use rcu, thus side-stepping locking complexities. We also save some memory this way.

- Old workqueue code is still used for level irqs.
I don't think we DTRT with level anyway, however,
it seems easier to keep the code around as
it has been thought through and debugged, and fix level later than rip out and re-instate it later.

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

---

The below is compile tested only. Sending out for early flames/feedback. Please review!

 include/linux/kvm_host.h |    4 ++
 virt/kvm/eventfd.c       |   81 +++++++++++++++++++++++++++++++++++++++------
 virt/kvm/irq_comm.c      |    6 ++-
 3 files changed, 78 insertions(+), 13 deletions(-)

include/linux/kvm_host.h +4 -0

index a055742..b6f7047 100644
@@ -462,6 +462,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   unsigned long *deliver_bitmask);
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
+		int irq_source_id, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);

@@ -603,6 +605,7 @@ static inline void kvm_free_irq_routing(struct kvm *kvm) {}
 void kvm_eventfd_init(struct kvm *kvm);
 int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags);
 void kvm_irqfd_release(struct kvm *kvm);
+void kvm_irqfd_update(struct kvm *kvm, struct kvm_irq_routing_table *irq_rt);
 int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args);
 
 #else

@@ -614,6 +617,7 @@ static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 }
 
 static inline void kvm_irqfd_release(struct kvm *kvm) {}
+static inline void kvm_irqfd_update(struct kvm *kvm) {}
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

virt/kvm/eventfd.c +12 -8

index c1f1e3c..49c1864 100644
@@ -44,14 +44,18 @@
  */
 
 struct _irqfd {
-	struct kvm               *kvm;
-	struct eventfd_ctx       *eventfd;
-	int                       gsi;
-	struct list_head          list;
-	poll_table                pt;
-	wait_queue_t              wait;
-	struct work_struct        inject;
-	struct work_struct        shutdown;
+	/* Used for MSI fast-path */
+	struct kvm *kvm;
+	wait_queue_t wait;
+	struct kvm_kernel_irq_routing_entry __rcu *irq_entry;
+	/* Used for level IRQ fast-path */
+	int gsi;
+	struct work_struct inject;
+	/* Used for setup/shutdown */
+	struct eventfd_ctx *eventfd;
+	struct list_head list;
+	poll_table pt;
+	struct work_struct shutdown;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

virt/kvm/eventfd.c +28 -2

 		/* An event has been signaled, inject an interrupt */
-		schedule_work(&irqfd->inject);
+		if (irq)
+			kvm_set_msi(irq, irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+		else
+			schedule_work(&irqfd->inject);
+		rcu_read_unlock();
+	}
 
 	if (flags & POLLHUP) {
 		/* The eventfd is closing, detach from KVM */

@@ -166,6 +178,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)
 {
+	struct kvm_irq_routing_table *irq_rt;
 	struct _irqfd *irqfd, *tmp;
 	struct file *file = NULL;
 	struct eventfd_ctx *eventfd = NULL;

@@ -215,6 +228,10 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 		goto fail;
 	}
 
+	rcu_read_lock();
+	irqfd_update(kvm, irqfd, rcu_dereference(kvm->irq_routing));
+	rcu_read_unlock();
+
 	events = file->f_op->poll(file, &irqfd->pt);
 
 	list_add_tail(&irqfd->list, &kvm->irqfds.items);

@@ -271,8 +288,15 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 	spin_lock_irq(&kvm->irqfds.lock);
 
 	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds.items, list) {
-		if (irqfd->eventfd == eventfd && irqfd->gsi == gsi)
+		if (irqfd->eventfd == eventfd && irqfd->gsi == gsi) {
+			/* This rcu_assign_pointer is needed for when
+			 * another thread calls kvm_irqfd_update before
+			 * we flush workqueue below.
+			 * It is paired with synchronize_rcu done by caller
+			 * of that function. */
+			rcu_assign_pointer(irqfd->irq_entry, NULL);
 			irqfd_deactivate(irqfd);
+		}
 	}
 
 	spin_unlock_irq(&kvm->irqfds.lock);

@@ -321,6 +345,41 @@ kvm_irqfd_release(struct kvm *kvm)
 
 }
 
+/* Must be called under irqfds.lock */
+static void irqfd_update(struct kvm *kvm, struct _irqfd *irqfd,
+			 struct kvm_irq_routing_table *irq_rt)
+{
+	struct kvm_kernel_irq_routing_entry *e;
+	struct hlist_node *n;
+
+	if (irqfd->gsi >= irq_rt->nr_rt_entries) {
+		rcu_assign_pointer(irqfd->irq_entry, NULL);

virt/kvm/eventfd.c +16 -0

+}
+
+/* Update irqfd after a routing change.  Caller must invoke synchronize_rcu
+ * afterwards. */
+void kvm_irqfd_update(struct kvm *kvm, struct kvm_irq_routing_table *irq_rt)
+{
+	struct _irqfd *irqfd;
+
+	spin_lock_irq(&kvm->irqfds.lock);
+
+	list_for_each_entry(irqfd, &kvm->irqfds.items, list)
+		irqfd_update(kvm, irqfd, irq_rt);
+
+	spin_unlock_irq(&kvm->irqfds.lock);
+}
+
 /*
  * create a host-wide workqueue for issuing deferred shutdown requests
  * aggregated from all vm* instances. We need our own isolated single-thread

virt/kvm/irq_comm.c +4 -2

index 8edca91..265ab72 100644
@@ -114,8 +114,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	return r;
 }
 
-static int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
-		       struct kvm *kvm, int irq_source_id, int level)
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
+		struct kvm *kvm, int irq_source_id, int level)
 {
 	struct kvm_lapic_irq irq;
 

@@ -410,7 +410,9 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	mutex_lock(&kvm->irq_lock);
 	old = kvm->irq_routing;
 	rcu_assign_pointer(kvm->irq_routing, new);
+	kvm_irqfd_update(kvm, new);
 	mutex_unlock(&kvm->irq_lock);
+
 	synchronize_rcu();
 
 	new = old;

Signature · 1 line

1.7.3.2.91.g446ac

Quoted 4 lines · Michael S. Tsirkin

Store irq routing table pointer in the irqfd object, and use that to inject MSI directly without bouncing out to a kernel thread.

Quoted 7 lines

While we touch this structure, rearrange irqfd fields to make fastpath better packed for better cache utilization.

Some notes on the design:
- Use pointer into the rt instead of copying an entry, to make it possible to use rcu, thus side-stepping locking complexities. We also save some memory this way.

What locking complexity is there with copying entry approach?

Quoted 78 lines

- Old workqueue code is still used for level irqs.
I don't think we DTRT with level anyway, however,
it seems easier to keep the code around as
it has been thought through and debugged, and fix level later than rip out and re-instate it later.

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

---

The below is compile tested only. Sending out for early flames/feedback. Please review!

 include/linux/kvm_host.h |    4 ++
 virt/kvm/eventfd.c       |   81 +++++++++++++++++++++++++++++++++++++++------
 virt/kvm/irq_comm.c      |    6 ++-
 3 files changed, 78 insertions(+), 13 deletions(-)

include/linux/kvm_host.h +4 -0

index a055742..b6f7047 100644
@@ -462,6 +462,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   unsigned long *deliver_bitmask);
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
+		int irq_source_id, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);

@@ -603,6 +605,7 @@ static inline void kvm_free_irq_routing(struct kvm *kvm) {}
 void kvm_eventfd_init(struct kvm *kvm);
 int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags);
 void kvm_irqfd_release(struct kvm *kvm);
+void kvm_irqfd_update(struct kvm *kvm, struct kvm_irq_routing_table *irq_rt);
 int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args);
 
 #else

@@ -614,6 +617,7 @@ static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 }
 
 static inline void kvm_irqfd_release(struct kvm *kvm) {}
+static inline void kvm_irqfd_update(struct kvm *kvm) {}
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

virt/kvm/eventfd.c +12 -8

index c1f1e3c..49c1864 100644
@@ -44,14 +44,18 @@
  */
 
 struct _irqfd {
-	struct kvm               *kvm;
-	struct eventfd_ctx       *eventfd;
-	int                       gsi;
-	struct list_head          list;
-	poll_table                pt;
-	wait_queue_t              wait;
-	struct work_struct        inject;
-	struct work_struct        shutdown;
+	/* Used for MSI fast-path */
+	struct kvm *kvm;
+	wait_queue_t wait;
+	struct kvm_kernel_irq_routing_entry __rcu *irq_entry;
+	/* Used for level IRQ fast-path */
+	int gsi;
+	struct work_struct inject;
+	/* Used for setup/shutdown */
+	struct eventfd_ctx *eventfd;
+	struct list_head list;
+	poll_table pt;
+	struct work_struct shutdown;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 136

Quoted 9 lines

virt/kvm/eventfd.c +4 -1

@@ -125,10 +129,18 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 {
 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
 	unsigned long flags = (unsigned long)key;
+	struct kvm_kernel_irq_routing_entry *irq;
 
-	if (flags & POLLIN)
+	if (flags & POLLIN) {
+		rcu_read_lock();
+		irq = irqfd->irq_entry;

Why not rcu_dereference()? And why it can't be zero here?

Quoted 60 lines

virt/kvm/eventfd.c +28 -2

 		/* An event has been signaled, inject an interrupt */
-		schedule_work(&irqfd->inject);
+		if (irq)
+			kvm_set_msi(irq, irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+		else
+			schedule_work(&irqfd->inject);
+		rcu_read_unlock();
+	}
 
 	if (flags & POLLHUP) {
 		/* The eventfd is closing, detach from KVM */

@@ -166,6 +178,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)
 {
+	struct kvm_irq_routing_table *irq_rt;
 	struct _irqfd *irqfd, *tmp;
 	struct file *file = NULL;
 	struct eventfd_ctx *eventfd = NULL;

@@ -215,6 +228,10 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 		goto fail;
 	}
 
+	rcu_read_lock();
+	irqfd_update(kvm, irqfd, rcu_dereference(kvm->irq_routing));
+	rcu_read_unlock();
+
 	events = file->f_op->poll(file, &irqfd->pt);
 
 	list_add_tail(&irqfd->list, &kvm->irqfds.items);

@@ -271,8 +288,15 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 	spin_lock_irq(&kvm->irqfds.lock);
 
 	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds.items, list) {
-		if (irqfd->eventfd == eventfd && irqfd->gsi == gsi)
+		if (irqfd->eventfd == eventfd && irqfd->gsi == gsi) {
+			/* This rcu_assign_pointer is needed for when
+			 * another thread calls kvm_irqfd_update before
+			 * we flush workqueue below.
+			 * It is paired with synchronize_rcu done by caller
+			 * of that function. */
+			rcu_assign_pointer(irqfd->irq_entry, NULL);
 			irqfd_deactivate(irqfd);
+		}
 	}
 
 	spin_unlock_irq(&kvm->irqfds.lock);

@@ -321,6 +345,41 @@ kvm_irqfd_release(struct kvm *kvm)
 
 }
 
+/* Must be called under irqfds.lock */
+static void irqfd_update(struct kvm *kvm, struct _irqfd *irqfd,
+			 struct kvm_irq_routing_table *irq_rt)
+{
+	struct kvm_kernel_irq_routing_entry *e;
+	struct hlist_node *n;
+
+	if (irqfd->gsi >= irq_rt->nr_rt_entries) {
+		rcu_assign_pointer(irqfd->irq_entry, NULL);

virt/kvm/eventfd.c kvm_irqfd_release(struct kvm *kvm) · line 366

Quoted 10 lines

virt/kvm/eventfd.c +10 -0

+		return;
+	}
+
+	hlist_for_each_entry(e, n, &irq_rt->map[irqfd->gsi], link) {
+		/* Only fast-path MSI. */
+		if (e->type == KVM_IRQ_ROUTING_MSI)
+			rcu_assign_pointer(irqfd->irq_entry, e);
+		else
+			rcu_assign_pointer(irqfd->irq_entry, NULL);
+	}

Shouldn't we flush workqueue if routing entry is gone?

Quoted 46 lines

virt/kvm/eventfd.c +16 -0

+}
+
+/* Update irqfd after a routing change.  Caller must invoke synchronize_rcu
+ * afterwards. */
+void kvm_irqfd_update(struct kvm *kvm, struct kvm_irq_routing_table *irq_rt)
+{
+	struct _irqfd *irqfd;
+
+	spin_lock_irq(&kvm->irqfds.lock);
+
+	list_for_each_entry(irqfd, &kvm->irqfds.items, list)
+		irqfd_update(kvm, irqfd, irq_rt);
+
+	spin_unlock_irq(&kvm->irqfds.lock);
+}
+
 /*
  * create a host-wide workqueue for issuing deferred shutdown requests
  * aggregated from all vm* instances. We need our own isolated single-thread

virt/kvm/irq_comm.c +4 -2

index 8edca91..265ab72 100644
@@ -114,8 +114,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	return r;
 }
 
-static int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
-		       struct kvm *kvm, int irq_source_id, int level)
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
+		struct kvm *kvm, int irq_source_id, int level)
 {
 	struct kvm_lapic_irq irq;
 

@@ -410,7 +410,9 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	mutex_lock(&kvm->irq_lock);
 	old = kvm->irq_routing;
 	rcu_assign_pointer(kvm->irq_routing, new);
+	kvm_irqfd_update(kvm, new);
 	mutex_unlock(&kvm->irq_lock);
+
 	synchronize_rcu();
 
 	new = old;

Signature · 1 line

1.7.3.2.91.g446ac

--
Gleb.

Quoted 5 lines · Gleb Natapov

Quoted 4 lines · Michael S. Tsirkin

Store irq routing table pointer in the irqfd object, and use that to inject MSI directly without bouncing out to a kernel thread.

Quoted 8 lines

Quoted 7 lines

While we touch this structure, rearrange irqfd fields to make fastpath better packed for better cache utilization.

Some notes on the design:
- Use pointer into the rt instead of copying an entry, to make it possible to use rcu, thus side-stepping locking complexities. We also save some memory this way.

What locking complexity is there with copying entry approach?

Without RCU, we need two locks:
- irqfd lock to scan the list of irqfds
- eventfd wqh lock in the irqfd to update the entry
To update all irqfds on list, wqh lock would be nested within irqfd lock.

	lock(kvm->irqfds.lock)
	list_for_each(irqfd, kvm->irqfds.list)
		lock(irqfd->wqh)
		update(irqfd)
		unlock(irqfd->wqh)
	unlock(kvm->irqfds.lock)

Problem is, irqfd is nested within wqh for cleanup (POLLHUP) path.

With RCU we do assign and let sync take care of flushing old entries out.

Quoted 204 lines

Quoted 78 lines

- Old workqueue code is still used for level irqs.
I don't think we DTRT with level anyway, however,
it seems easier to keep the code around as
it has been thought through and debugged, and fix level later than rip out and re-instate it later.

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

---

The below is compile tested only. Sending out for early flames/feedback. Please review!

 include/linux/kvm_host.h |    4 ++
 virt/kvm/eventfd.c       |   81 +++++++++++++++++++++++++++++++++++++++------
 virt/kvm/irq_comm.c      |    6 ++-
 3 files changed, 78 insertions(+), 13 deletions(-)

include/linux/kvm_host.h +4 -0

index a055742..b6f7047 100644
@@ -462,6 +462,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   unsigned long *deliver_bitmask);
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
+		int irq_source_id, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);

@@ -603,6 +605,7 @@ static inline void kvm_free_irq_routing(struct kvm *kvm) {}
 void kvm_eventfd_init(struct kvm *kvm);
 int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags);
 void kvm_irqfd_release(struct kvm *kvm);
+void kvm_irqfd_update(struct kvm *kvm, struct kvm_irq_routing_table *irq_rt);
 int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args);
 
 #else

@@ -614,6 +617,7 @@ static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 }
 
 static inline void kvm_irqfd_release(struct kvm *kvm) {}
+static inline void kvm_irqfd_update(struct kvm *kvm) {}
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

virt/kvm/eventfd.c +12 -8

index c1f1e3c..49c1864 100644
@@ -44,14 +44,18 @@
  */
 
 struct _irqfd {
-	struct kvm               *kvm;
-	struct eventfd_ctx       *eventfd;
-	int                       gsi;
-	struct list_head          list;
-	poll_table                pt;
-	wait_queue_t              wait;
-	struct work_struct        inject;
-	struct work_struct        shutdown;
+	/* Used for MSI fast-path */
+	struct kvm *kvm;
+	wait_queue_t wait;
+	struct kvm_kernel_irq_routing_entry __rcu *irq_entry;
+	/* Used for level IRQ fast-path */
+	int gsi;
+	struct work_struct inject;
+	/* Used for setup/shutdown */
+	struct eventfd_ctx *eventfd;
+	struct list_head list;
+	poll_table pt;
+	struct work_struct shutdown;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 136

Quoted 9 lines

virt/kvm/eventfd.c +4 -1

@@ -125,10 +129,18 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 {
 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
 	unsigned long flags = (unsigned long)key;
+	struct kvm_kernel_irq_routing_entry *irq;
 
-	if (flags & POLLIN)
+	if (flags & POLLIN) {
+		rcu_read_lock();
+		irq = irqfd->irq_entry;

Why not rcu_dereference()? And why it can't be zero here?

Quoted 60 lines

virt/kvm/eventfd.c +28 -2

 		/* An event has been signaled, inject an interrupt */
-		schedule_work(&irqfd->inject);
+		if (irq)
+			kvm_set_msi(irq, irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+		else
+			schedule_work(&irqfd->inject);
+		rcu_read_unlock();
+	}
 
 	if (flags & POLLHUP) {
 		/* The eventfd is closing, detach from KVM */

@@ -166,6 +178,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)
 {
+	struct kvm_irq_routing_table *irq_rt;
 	struct _irqfd *irqfd, *tmp;
 	struct file *file = NULL;
 	struct eventfd_ctx *eventfd = NULL;

@@ -215,6 +228,10 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 		goto fail;
 	}
 
+	rcu_read_lock();
+	irqfd_update(kvm, irqfd, rcu_dereference(kvm->irq_routing));
+	rcu_read_unlock();
+
 	events = file->f_op->poll(file, &irqfd->pt);
 
 	list_add_tail(&irqfd->list, &kvm->irqfds.items);

@@ -271,8 +288,15 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 	spin_lock_irq(&kvm->irqfds.lock);
 
 	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds.items, list) {
-		if (irqfd->eventfd == eventfd && irqfd->gsi == gsi)
+		if (irqfd->eventfd == eventfd && irqfd->gsi == gsi) {
+			/* This rcu_assign_pointer is needed for when
+			 * another thread calls kvm_irqfd_update before
+			 * we flush workqueue below.
+			 * It is paired with synchronize_rcu done by caller
+			 * of that function. */
+			rcu_assign_pointer(irqfd->irq_entry, NULL);
 			irqfd_deactivate(irqfd);
+		}
 	}
 
 	spin_unlock_irq(&kvm->irqfds.lock);

@@ -321,6 +345,41 @@ kvm_irqfd_release(struct kvm *kvm)
 
 }
 
+/* Must be called under irqfds.lock */
+static void irqfd_update(struct kvm *kvm, struct _irqfd *irqfd,
+			 struct kvm_irq_routing_table *irq_rt)
+{
+	struct kvm_kernel_irq_routing_entry *e;
+	struct hlist_node *n;
+
+	if (irqfd->gsi >= irq_rt->nr_rt_entries) {
+		rcu_assign_pointer(irqfd->irq_entry, NULL);

virt/kvm/eventfd.c kvm_irqfd_release(struct kvm *kvm) · line 366

Quoted 10 lines

virt/kvm/eventfd.c +10 -0

+		return;
+	}
+
+	hlist_for_each_entry(e, n, &irq_rt->map[irqfd->gsi], link) {
+		/* Only fast-path MSI. */
+		if (e->type == KVM_IRQ_ROUTING_MSI)
+			rcu_assign_pointer(irqfd->irq_entry, e);
+		else
+			rcu_assign_pointer(irqfd->irq_entry, NULL);
+	}

Shouldn't we flush workqueue if routing entry is gone?

Quoted 43 lines

virt/kvm/eventfd.c +16 -0

+}
+
+/* Update irqfd after a routing change.  Caller must invoke synchronize_rcu
+ * afterwards. */
+void kvm_irqfd_update(struct kvm *kvm, struct kvm_irq_routing_table *irq_rt)
+{
+	struct _irqfd *irqfd;
+
+	spin_lock_irq(&kvm->irqfds.lock);
+
+	list_for_each_entry(irqfd, &kvm->irqfds.items, list)
+		irqfd_update(kvm, irqfd, irq_rt);
+
+	spin_unlock_irq(&kvm->irqfds.lock);
+}
+
 /*
  * create a host-wide workqueue for issuing deferred shutdown requests
  * aggregated from all vm* instances. We need our own isolated single-thread

virt/kvm/irq_comm.c +4 -2

index 8edca91..265ab72 100644
@@ -114,8 +114,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	return r;
 }
 
-static int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
-		       struct kvm *kvm, int irq_source_id, int level)
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
+		struct kvm *kvm, int irq_source_id, int level)
 {
 	struct kvm_lapic_irq irq;
 

@@ -410,7 +410,9 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	mutex_lock(&kvm->irq_lock);
 	old = kvm->irq_routing;
 	rcu_assign_pointer(kvm->irq_routing, new);
+	kvm_irqfd_update(kvm, new);
 	mutex_unlock(&kvm->irq_lock);
+
 	synchronize_rcu();
 

Quoted 6 lines

Quoted 3 lines

virt/kvm/irq_comm.c +0 -0

 	new = old;

Signature · 1 line

1.7.3.2.91.g446ac

--
Gleb.

Quoted 11 lines · Michael S. Tsirkin

Store irq routing table pointer in the irqfd object, and use that to inject MSI directly without bouncing out to a kernel thread.

While we touch this structure, rearrange irqfd fields to make fastpath better packed for better cache utilization.

Some notes on the design:
- Use pointer into the rt instead of copying an entry, to make it possible to use rcu, thus side-stepping locking complexities. We also save some memory this way.

What locking complexity is there with copying entry approach?

Quoted 83 lines

- Old workqueue code is still used for level irqs.
I don't think we DTRT with level anyway, however,
it seems easier to keep the code around as
it has been thought through and debugged, and fix level later than rip out and re-instate it later.

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

---

The below is compile tested only. Sending out for early flames/feedback. Please review!

 include/linux/kvm_host.h |    4 ++
 virt/kvm/eventfd.c       |   81 +++++++++++++++++++++++++++++++++++++++------
 virt/kvm/irq_comm.c      |    6 ++-
 3 files changed, 78 insertions(+), 13 deletions(-)

include/linux/kvm_host.h +4 -0

index a055742..b6f7047 100644
@@ -462,6 +462,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   unsigned long *deliver_bitmask);
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
+		int irq_source_id, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);

@@ -603,6 +605,7 @@ static inline void kvm_free_irq_routing(struct kvm *kvm) {}
 void kvm_eventfd_init(struct kvm *kvm);
 int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags);
 void kvm_irqfd_release(struct kvm *kvm);
+void kvm_irqfd_update(struct kvm *kvm, struct kvm_irq_routing_table *irq_rt);
 int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args);
 
 #else

@@ -614,6 +617,7 @@ static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 }
 
 static inline void kvm_irqfd_release(struct kvm *kvm) {}
+static inline void kvm_irqfd_update(struct kvm *kvm) {}
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

virt/kvm/eventfd.c +13 -8

index c1f1e3c..49c1864 100644
@@ -44,14 +44,18 @@
  */
 
 struct _irqfd {
-	struct kvm               *kvm;
-	struct eventfd_ctx       *eventfd;
-	int                       gsi;
-	struct list_head          list;
-	poll_table                pt;
-	wait_queue_t              wait;
-	struct work_struct        inject;
-	struct work_struct        shutdown;
+	/* Used for MSI fast-path */
+	struct kvm *kvm;
+	wait_queue_t wait;
+	struct kvm_kernel_irq_routing_entry __rcu *irq_entry;
+	/* Used for level IRQ fast-path */
+	int gsi;
+	struct work_struct inject;
+	/* Used for setup/shutdown */
+	struct eventfd_ctx *eventfd;
+	struct list_head list;
+	poll_table pt;
+	struct work_struct shutdown;
 };
 
 static struct workqueue_struct *irqfd_cleanup_wq;

@@ -125,10 +129,18 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 {
 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
 	unsigned long flags = (unsigned long)key;
+	struct kvm_kernel_irq_routing_entry *irq;
 

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 136

Quoted 4 lines

virt/kvm/eventfd.c +3 -1

-	if (flags & POLLIN)
+	if (flags & POLLIN) {
+		rcu_read_lock();
+		irq = irqfd->irq_entry;

Why not rcu_dereference()?

And why it can't be zero here?

Quoted 63 lines

virt/kvm/eventfd.c +31 -2

 		/* An event has been signaled, inject an interrupt */
-		schedule_work(&irqfd->inject);
+		if (irq)
+			kvm_set_msi(irq, irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+		else
+			schedule_work(&irqfd->inject);
+		rcu_read_unlock();
+	}
 
 	if (flags & POLLHUP) {
 		/* The eventfd is closing, detach from KVM */

@@ -166,6 +178,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)
 {
+	struct kvm_irq_routing_table *irq_rt;
 	struct _irqfd *irqfd, *tmp;
 	struct file *file = NULL;
 	struct eventfd_ctx *eventfd = NULL;

@@ -215,6 +228,10 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
 		goto fail;
 	}
 
+	rcu_read_lock();
+	irqfd_update(kvm, irqfd, rcu_dereference(kvm->irq_routing));
+	rcu_read_unlock();
+
 	events = file->f_op->poll(file, &irqfd->pt);
 
 	list_add_tail(&irqfd->list, &kvm->irqfds.items);

@@ -271,8 +288,15 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
 	spin_lock_irq(&kvm->irqfds.lock);
 
 	list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds.items, list) {
-		if (irqfd->eventfd == eventfd && irqfd->gsi == gsi)
+		if (irqfd->eventfd == eventfd && irqfd->gsi == gsi) {
+			/* This rcu_assign_pointer is needed for when
+			 * another thread calls kvm_irqfd_update before
+			 * we flush workqueue below.
+			 * It is paired with synchronize_rcu done by caller
+			 * of that function. */
+			rcu_assign_pointer(irqfd->irq_entry, NULL);
 			irqfd_deactivate(irqfd);
+		}
 	}
 
 	spin_unlock_irq(&kvm->irqfds.lock);

@@ -321,6 +345,41 @@ kvm_irqfd_release(struct kvm *kvm)
 
 }
 
+/* Must be called under irqfds.lock */
+static void irqfd_update(struct kvm *kvm, struct _irqfd *irqfd,
+			 struct kvm_irq_routing_table *irq_rt)
+{
+	struct kvm_kernel_irq_routing_entry *e;
+	struct hlist_node *n;
+
+	if (irqfd->gsi >= irq_rt->nr_rt_entries) {
+		rcu_assign_pointer(irqfd->irq_entry, NULL);
+		return;
+	}
+

virt/kvm/eventfd.c kvm_irqfd_release(struct kvm *kvm) · line 366

Quoted 7 lines

virt/kvm/eventfd.c +7 -0

+	hlist_for_each_entry(e, n, &irq_rt->map[irqfd->gsi], link) {
+		/* Only fast-path MSI. */
+		if (e->type == KVM_IRQ_ROUTING_MSI)
+			rcu_assign_pointer(irqfd->irq_entry, e);
+		else
+			rcu_assign_pointer(irqfd->irq_entry, NULL);
+	}

Shouldn't we flush workqueue if routing entry is gone?

Quoted 46 lines

virt/kvm/eventfd.c +16 -0

+}
+
+/* Update irqfd after a routing change.  Caller must invoke synchronize_rcu
+ * afterwards. */
+void kvm_irqfd_update(struct kvm *kvm, struct kvm_irq_routing_table *irq_rt)
+{
+	struct _irqfd *irqfd;
+
+	spin_lock_irq(&kvm->irqfds.lock);
+
+	list_for_each_entry(irqfd, &kvm->irqfds.items, list)
+		irqfd_update(kvm, irqfd, irq_rt);
+
+	spin_unlock_irq(&kvm->irqfds.lock);
+}
+
 /*
  * create a host-wide workqueue for issuing deferred shutdown requests
  * aggregated from all vm* instances. We need our own isolated single-thread

virt/kvm/irq_comm.c +4 -2

index 8edca91..265ab72 100644
@@ -114,8 +114,8 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,
 	return r;
 }
 
-static int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
-		       struct kvm *kvm, int irq_source_id, int level)
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
+		struct kvm *kvm, int irq_source_id, int level)
 {
 	struct kvm_lapic_irq irq;
 

@@ -410,7 +410,9 @@ int kvm_set_irq_routing(struct kvm *kvm,
 	mutex_lock(&kvm->irq_lock);
 	old = kvm->irq_routing;
 	rcu_assign_pointer(kvm->irq_routing, new);
+	kvm_irqfd_update(kvm, new);
 	mutex_unlock(&kvm->irq_lock);
+
 	synchronize_rcu();
 
 	new = old;

Signature · 1 line

1.7.3.2.91.g446ac

--
Gleb.

Store irq routing table pointer in the irqfd object, and use that to inject MSI directly without bouncing out to a kernel thread.

While we touch this structure, rearrange irqfd fields to make fastpath better packed for better cache utilization.

Some notes on the design:
- Use pointer into the rt instead of copying an entry,
to make it possible to use rcu, thus side-stepping locking complexities. We also save some memory this way.
- Old workqueue code is still used for level irqs.
I don't think we DTRT with level anyway, however,
it seems easier to keep the code around as
it has been thought through and debugged, and fix level later than rip out and re-instate it later.

virt/kvm/eventfd.c +1 -0

@@ -166,6 +178,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)
  {
+	struct kvm_irq_routing_table *irq_rt;
  	struct _irqfd *irqfd, *tmp;
  	struct file *file = NULL;
  	struct eventfd_ctx *eventfd = NULL;

Quoted 11 lines

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 136

Quoted 10 lines

virt/kvm/eventfd.c +4 -1

@@ -125,10 +129,18 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
 {
 	struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
 	unsigned long flags = (unsigned long)key;
+	struct kvm_kernel_irq_routing_entry *irq;
 
-	if (flags & POLLIN)
+	if (flags & POLLIN) {
+		rcu_read_lock();
+		irq = irqfd->irq_entry;

Why not rcu_dereference()?

Of course. Good catch, thanks.

Quoted 1 line

And why it can't be zero here?

It can, I check below.

Quoted 9 lines

virt/kvm/eventfd.c irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key) · line 144

Quoted 9 lines

virt/kvm/eventfd.c +6 -1

 		/* An event has been signaled, inject an interrupt */
-		schedule_work(&irqfd->inject);
+		if (irq)
+			kvm_set_msi(irq, irqfd->kvm, KVM_USERSPACE_IRQ_SOURCE_ID, 1);
+		else
+			schedule_work(&irqfd->inject);
+		rcu_read_unlock();
+	}
 

So the following on top will fix it all.
Any more comments befpre I bundle it up,
test and report?

Quoted 4 lines · Michael S. Tsirkin

So the following on top will fix it all.
Any more comments befpre I bundle it up,
test and report?

Nope (not that I can comment on an incremental).

I guess I should create an empty Documentation/kvm/locking.txt and force everyone else to update it.

Quoted 2 lines

I guess I should create an empty Documentation/kvm/locking.txt and force everyone else to update it.

Comments near the relevant fields not better?

include/linux/kvm_host.h +1 -0

index a055742..d13ced3 100644
@@ -16,6 +16,7 @@
  #include<linux/mm.h>
  #include<linux/preempt.h>
  #include<linux/msi.h>
+#include<linux/rcupdate.h>
  #include<asm/signal.h>
 
  #include<linux/kvm.h>

Quoted 5 lines · Michael S. Tsirkin

Quoted 2 lines

I guess I should create an empty Documentation/kvm/locking.txt and force everyone else to update it.

Comments near the relevant fields not better?

Not an either/or. You can't understand the system from random source comments.

Quoted 13 lines

include/linux/kvm_host.h +1 -0

index a055742..d13ced3 100644
@@ -16,6 +16,7 @@
 #include<linux/mm.h>
 #include<linux/preempt.h>
 #include<linux/msi.h>
+#include<linux/rcupdate.h>
 #include<asm/signal.h>
 
 #include<linux/kvm.h>

include/linux/kvm_host.h struct kvm { · line 211

Quoted 5 lines

include/linux/kvm_host.h +2 -0

@@ -206,6 +207,8 @@ struct kvm {
 
 	struct mutex irq_lock;
 #ifdef CONFIG_HAVE_KVM_IRQCHIP
+	/* Update side is protected by irq_lock and,
+	 * if configured, irqfds.lock. */

/*
* kernel style comment
* here and elsewhere
*/

include/linux/kvm_host.h void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic, · line 469

Quoted 9 lines

include/linux/kvm_host.h +2 -0

 	struct kvm_irq_routing_table __rcu *irq_routing;
 	struct hlist_head mask_notifier_list;
 	struct hlist_head irq_ack_notifier_list;

@@ -462,6 +465,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,
 				   unsigned long *deliver_bitmask);
 #endif
 int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);
+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
+		int irq_source_id, int level);

No point in the level argument for an msi specific function.

include/linux/kvm_host.h static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 622

Quoted 6 lines

include/linux/kvm_host.h +0 -0

 #else

@@ -614,6 +620,12 @@ static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
 }
 
 static inline void kvm_irqfd_release(struct kvm *kvm) {}

blank line

include/linux/kvm_host.h static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 631

Quoted 9 lines

include/linux/kvm_host.h +6 -0

+static inline void kvm_irq_routing_update(struct kvm *kvm,
+					  struct kvm_irq_routing_table *irq_rt)
+{
+	rcu_assign_pointer(kvm->irq_routing, irq_rt);
+}
+
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

Apart from these minor issues, looks good.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 5 lines

include/linux/kvm_host.h void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic, · line 469

Quoted 3 lines

int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level);

include/linux/kvm_host.h +2 -0

+int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
+		int irq_source_id, int level);

No point in the level argument for an msi specific function.

This is an existing function I made non-static.
We have per-gsi callbacks so level is required there to match.

I could add a wrapper I guess:

int kvm_set_msi(struct kvm_kernel_irq_routing_entry *irq_entry, struct kvm *kvm,
int irq_source_id, int level)

{
	if (!level)
		return -1;
	return kvm_send_msi(irq_entry, kvm, irq_source_id);
}

This results in less code for irqfd but more code for ioctl injection ... is it worth it?

Quoted 2 lines

Apart from these minor issues, looks good.

Something we should consider improving is the loop over all VCPUs that kvm_irq_delivery_to_apic invokes. I think that (for non-broadcast interrupts) it should be possible to precompute an store the CPU in question as part of the routing entry.

Something for a separate patch ... comments?

Quoted 11 lines

include/linux/kvm_host.h static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 631

Quoted 9 lines

include/linux/kvm_host.h +6 -0

+static inline void kvm_irq_routing_update(struct kvm *kvm,
+					  struct kvm_irq_routing_table *irq_rt)
+{
+	rcu_assign_pointer(kvm->irq_routing, irq_rt);
+}
+
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

Apart from these minor issues, looks good.

Something we should consider improving is the loop over all VCPUs that kvm_irq_delivery_to_apic invokes. I think that (for non-broadcast interrupts) it should be possible to precompute an store the CPU in question as part of the routing entry.

Something for a separate patch ... comments?

Quoted 18 lines · Michael S. Tsirkin

Quoted 11 lines

include/linux/kvm_host.h static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 631

Quoted 9 lines

include/linux/kvm_host.h +6 -0

+static inline void kvm_irq_routing_update(struct kvm *kvm,
+					  struct kvm_irq_routing_table *irq_rt)
+{
+	rcu_assign_pointer(kvm->irq_routing, irq_rt);
+}
+
  static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
  {
  	return -ENOSYS;

Apart from these minor issues, looks good.

Something we should consider improving is the loop over all VCPUs that kvm_irq_delivery_to_apic invokes. I think that (for non-broadcast interrupts) it should be possible to precompute an store the CPU in question as part of the routing entry.

Quoted 2 lines

Something for a separate patch ... comments?

I do not think this info should be part of routing entry. Routing entry is more about describing wires on the board. Other then that this is a good idea that, IIRC, we already discussed once.

Quoted 18 lines · Michael S. Tsirkin

Quoted 11 lines

include/linux/kvm_host.h static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 631

Quoted 9 lines

include/linux/kvm_host.h +6 -0

+static inline void kvm_irq_routing_update(struct kvm *kvm,
+					  struct kvm_irq_routing_table *irq_rt)
+{
+	rcu_assign_pointer(kvm->irq_routing, irq_rt);
+}
+
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

Apart from these minor issues, looks good.

Something we should consider improving is the loop over all VCPUs that kvm_irq_delivery_to_apic invokes. I think that (for non-broadcast interrupts) it should be possible to precompute an store the CPU in question as part of the routing entry.

Quoted 2 lines

Something for a separate patch ... comments?

I do not think this info should be part of routing entry. Routing entry is more about describing wires on the board.

Other then that
this is a good idea that, IIRC, we already discussed once.

--
Gleb.

Quoted 19 lines · Gleb Natapov

Quoted 18 lines · Michael S. Tsirkin

Quoted 11 lines

include/linux/kvm_host.h static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 631

Quoted 9 lines

include/linux/kvm_host.h +6 -0

+static inline void kvm_irq_routing_update(struct kvm *kvm,
+					  struct kvm_irq_routing_table *irq_rt)
+{
+	rcu_assign_pointer(kvm->irq_routing, irq_rt);
+}
+
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

Apart from these minor issues, looks good.

Something we should consider improving is the loop over all VCPUs that kvm_irq_delivery_to_apic invokes. I think that (for non-broadcast interrupts) it should be possible to precompute an store the CPU in question as part of the routing entry.

Quoted 4 lines

Quoted 2 lines

Something for a separate patch ... comments?

I do not think this info should be part of routing entry. Routing entry is more about describing wires on the board.

Not for msi. kvm_kernel_irq_routing_entry seems to just keep an address/data pair in that case. So

	union {
		struct {
			unsigned irqchip;
			unsigned pin;
		} irqchip;
		struct msi_msg msi;
	};

would become

	union {
		struct {
			unsigned irqchip;
			unsigned pin;
		} irqchip;
		struct {
			struct msi_msg msi;
			struct kvm_vpcu *dest;
		} msi;
	};

or something like this.

Quoted 6 lines

Other then that
this is a good idea that, IIRC, we already discussed once.

--
Gleb.

Quoted 2 lines

or something like this.

Ah so you want to do it only for MSI? For MSI it makes sense. Remember though that sometimes destination depend on message itself (specifically on delivery mode).

Quoted 20 lines · Michael S. Tsirkin

Quoted 19 lines · Gleb Natapov

Quoted 18 lines · Michael S. Tsirkin

Quoted 11 lines

include/linux/kvm_host.h static inline int kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags) · line 631

Quoted 9 lines

include/linux/kvm_host.h +6 -0

+static inline void kvm_irq_routing_update(struct kvm *kvm,
+					  struct kvm_irq_routing_table *irq_rt)
+{
+	rcu_assign_pointer(kvm->irq_routing, irq_rt);
+}
+
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

Apart from these minor issues, looks good.

Something we should consider improving is the loop over all VCPUs that kvm_irq_delivery_to_apic invokes. I think that (for non-broadcast interrupts) it should be possible to precompute an store the CPU in question as part of the routing entry.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Something for a separate patch ... comments?

I do not think this info should be part of routing entry. Routing entry is more about describing wires on the board.

Not for msi. kvm_kernel_irq_routing_entry seems to just keep an address/data pair in that case. So

Yeah. Using routing_entry for MSI was miss design. We discussed that too :)

Quoted 21 lines

	union {
		struct {
			unsigned irqchip;
			unsigned pin;
		} irqchip;
		struct msi_msg msi;
	};

would become

	union {
		struct {
			unsigned irqchip;
			unsigned pin;
		} irqchip;
		struct {
			struct msi_msg msi;
			struct kvm_vpcu *dest;
		} msi;
	};

Quoted 1 line

or something like this.

Ah so you want to do it only for MSI? For MSI it makes sense. Remember though that sometimes destination depend on message itself (specifically on delivery mode).

Quoted 6 lines

Quoted 6 lines

Other then that
this is a good idea that, IIRC, we already discussed once.

--
Gleb.

--
Gleb.

Quoted 5 lines · Gleb Natapov

Quoted 2 lines

or something like this.

Ah so you want to do it only for MSI? For MSI it makes sense. Remember though that sometimes destination depend on message itself (specifically on delivery mode).

Yes, broadcast or multicast or lowest priority wouldn't get this treatment.

Signature · 1 line

error compiling committee.c: too many arguments to function

Quoted 53 lines · Gleb Natapov

Quoted 20 lines · Michael S. Tsirkin

Quoted 19 lines · Gleb Natapov

Quoted 18 lines · Michael S. Tsirkin

Quoted 11 lines

Review

Quoted 9 lines

(quoted diff) +6 -0

+static inline void kvm_irq_routing_update(struct kvm *kvm,
+					  struct kvm_irq_routing_table *irq_rt)
+{
+	rcu_assign_pointer(kvm->irq_routing, irq_rt);
+}
+
 static inline int kvm_ioeventfd(struct kvm *kvm, struct kvm_ioeventfd *args)
 {
 	return -ENOSYS;

Apart from these minor issues, looks good.

Something we should consider improving is the loop over all VCPUs that kvm_irq_delivery_to_apic invokes. I think that (for non-broadcast interrupts) it should be possible to precompute an store the CPU in question as part of the routing entry.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Something for a separate patch ... comments?

I do not think this info should be part of routing entry. Routing entry is more about describing wires on the board.

Not for msi. kvm_kernel_irq_routing_entry seems to just keep an address/data pair in that case. So

Yeah. Using routing_entry for MSI was miss design. We discussed that too :)

Quoted 22 lines

	union {
		struct {
			unsigned irqchip;
			unsigned pin;
		} irqchip;
		struct msi_msg msi;
	};

would become

	union {
		struct {
			unsigned irqchip;
			unsigned pin;
		} irqchip;
		struct {
			struct msi_msg msi;
			struct kvm_vpcu *dest;
		} msi;
	};

or something like this.

Quoted 3 lines

Ah so you want to do it only for MSI? For MSI it makes sense. Remember though that sometimes destination depend on message itself (specifically on delivery mode).

Of course. We'll take message/data and precompute destination. Set to NULL for e.g. broadcast and recompute at injection time in that case. BTW SELF doesn't work for MSI at the moment, not sure whether it's relevant or when is it used.