kvm: deliver msix interrupts from irq handler
RFC PATCH 1/1 kvm: deliver msix interrupts from irq handler
We can deliver certain interrupts, notably MSIX, from atomic context. Add a new API kvm_set_irq_inatomic, that does exactly that, and use it to implement an irq handler for msi.
This reduces the pressure on scheduler in case where host and guest irq share a host cpu.
Untested.
Note: this is on top of my host irq patch. Probably needs to be rebased to be independent and split up to new API + usage.
--- include/linux/kvm_host.h | 2 + virt/kvm/assigned-dev.c | 31 +++++++++++++++++++++++++- virt/kvm/irq_comm.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-)
include/linux/kvm_host.h +2 -0
@@ -548,6 +548,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,#endif int kvm_set_irq (struct kvm *kvm,int irq_source_id,u32 irq,int level,int host_irq); +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level, +int host_irq);int kvm_set_msi (struct kvm_kernel_irq_routing_entry *irq_entry,struct kvm *kvm,int irq_source_id,int level,int host_irq);void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin);
virt/kvm/assigned-dev.c +29 -2
@@ -57,6 +57,14 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernelreturn index; } +static irqreturn_t kvm_assigned_dev_msi (int irq,void *dev_id) +{ +int ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + assigned_dev->guest_irq,1 , irq); +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +} +static irqreturn_t kvm_assigned_dev_thread (int irq,void *dev_id) {struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
@@ -75,6 +83,23 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id) }#ifdef __KVM_HAVE_MSIX +static irqreturn_t kvm_assigned_dev_msix (int irq,void *dev_id) +{ +struct kvm_assigned_dev_kernel *assigned_dev = dev_id; +int index =find_index_from_host_irq (assigned_dev, irq); +u32 vector; +int ret =0 ; + +if (index >=0 ) { + vector = assigned_dev->guest_msix_entries[index].vector; + ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + vector,1 , irq); + } + +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +} +static irqreturn_t kvm_assigned_dev_thread_msix (int irq,void *dev_id) {struct kvm_assigned_dev_kernel *assigned_dev = dev_id;
@@ -266,7 +291,8 @@ static int assigned_device_enable_host_msi(struct kvm *kvm, } dev->host_irq = dev->dev->irq; -if (request_threaded_irq (dev->host_irq,NULL , kvm_assigned_dev_thread, +if (request_threaded_irq (dev->host_irq, kvm_assigned_dev_msi, + kvm_assigned_dev_thread,0 , dev->irq_name, dev)) {pci_disable_msi (dev->dev);return -EIO ;
@@ -293,7 +319,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,for (i =0 ; i < dev->entries_nr; i++) { r =request_threaded_irq (dev->host_msix_entries[i].vector, -NULL , kvm_assigned_dev_thread_msix, + kvm_assigned_dev_msix, + kvm_assigned_dev_thread_msix,0 , dev->irq_name, dev);if (r) goto err;
virt/kvm/irq_comm.c +52 -0
58 lines not on this page · raw message
Signature · 1 line
1.7.8.2.325.g247f9
Quoted 107 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context. Add a new API
kvm_set_irq_inatomic, that does exactly that, and use it to implement an irq handler for msi.This reduces the pressure on scheduler in case
where host and guest irq share a host cpu.Signed-off-by Michael S. Tsirkin <[email protected]>Untested.
Note: this is on top of my host irq patch.
Probably needs to be rebased to be independent
and split up to new API + usage.--- include/linux/kvm_host.h | 2 + virt/kvm/assigned-dev.c | 31 +++++++++++++++++++++++++- virt/kvm/irq_comm.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-)include/linux/kvm_host.h +2
-0@@ -548,6 +548,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,#endif int kvm_set_irq (struct kvm *kvm,int irq_source_id,u32 irq,int level,int host_irq); +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level, +int host_irq);int kvm_set_msi (struct kvm_kernel_irq_routing_entry *irq_entry,struct kvm *kvm,int irq_source_id,int level,int host_irq);void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin);virt/kvm/assigned-dev.c +29
-2@@ -57,6 +57,14 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernelreturn index; } +static irqreturn_t kvm_assigned_dev_msi (int irq,void *dev_id) +{ +int ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + assigned_dev->guest_irq,1 , irq); +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +} +static irqreturn_t kvm_assigned_dev_thread (int irq,void *dev_id) {struct kvm_assigned_dev_kernel *assigned_dev = dev_id;@@ -75,6 +83,23 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id) }#ifdef __KVM_HAVE_MSIX +static irqreturn_t kvm_assigned_dev_msix (int irq,void *dev_id) +{ +struct kvm_assigned_dev_kernel *assigned_dev = dev_id; +int index =find_index_from_host_irq (assigned_dev, irq); +u32 vector; +int ret =0 ; + +if (index >=0 ) { + vector = assigned_dev->guest_msix_entries[index].vector; + ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + vector,1 , irq); + } + +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +} +static irqreturn_t kvm_assigned_dev_thread_msix (int irq,void *dev_id) {struct kvm_assigned_dev_kernel *assigned_dev = dev_id;@@ -266,7 +291,8 @@ static int assigned_device_enable_host_msi(struct kvm *kvm, } dev->host_irq = dev->dev->irq; -if (request_threaded_irq (dev->host_irq,NULL , kvm_assigned_dev_thread, +if (request_threaded_irq (dev->host_irq, kvm_assigned_dev_msi, + kvm_assigned_dev_thread,0 , dev->irq_name, dev)) {pci_disable_msi (dev->dev);return -EIO ;@@ -293,7 +319,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,for (i =0 ; i < dev->entries_nr; i++) { r =request_threaded_irq (dev->host_msix_entries[i].vector, -NULL , kvm_assigned_dev_thread_msix, + kvm_assigned_dev_msix, + kvm_assigned_dev_thread_msix,0 , dev->irq_name, dev);if (r) goto err;virt/kvm/irq_comm.c +2
-0@@ -201,6 +201,58 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,return ret; } +static inline struct kvm_kernel_irq_routing_entry * +kvm_get_entry (struct kvm *kvm,struct kvm_irq_routing_table *irq_rq,u32 irq)
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 215
Quoted 10 lines
virt/kvm/irq_comm.c +10
-0+{ +struct kvm_kernel_irq_routing_entry *e; +if (likely (irq < irq_rt->nr_rt_entries)) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) +if (e->type ==KVM_IRQ_ROUTING_MSI ) +return e; +else +return ERR_PTR (-EWOULDBLOCK ); +return ERR_PTR (-EINVAL ); +}
Unused?
Quoted 16 lines
virt/kvm/irq_comm.c +16
-0+ +/* +* Deliver an IRQ in an atomic context if we can, or return a failure, +* user can retry in a process context. +* Return value: +* -EWOULDBLOCK Can't deliver in atomic context +* < 0 Interrupt was ignored (masked or not delivered for other reasons) +* = 0 Interrupt was coalesced (previous irq is still pending) +* > 0 Number of CPUs interrupt was delivered to +*/ +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level, +int host_irq) +{ +struct kvm_kernel_irq_routing_entry *e; +int ret = -EINVAL ; +struct kvm_irq_routing_table *irq_rt;
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 241
Quoted 10 lines
virt/kvm/irq_comm.c +10
-0+struct hlist_node *n; + +trace_kvm_set_irq (irq, level, irq_source_id); + +/* +* We know MSI are safe in interrupt context. They are also +* easy as there's a single routing entry for these GSIs. +* So only handle MSI in an atomic context, for now. +*/ +rcu_read_lock_bh ();
_bh?
/**
* rcu_read_lock_bh() - mark the beginning of an RCU-bh critical section *
* This is equivalent of rcu_read_lock(), but to be used when updates
* are being done using call_rcu_bh() or synchronize_rcu_bh().
....
Since updates to irq routing table are not done using _bh variant I doubt rcu_read_lock_bh() is justified here.
Quoted 4 lines
virt/kvm/irq_comm.c +4
-0+ irq_rt =rcu_dereference (kvm->irq_routing); +if (irq < irq_rt->nr_rt_entries) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) { +if (ei->type ==KVM_IRQ_ROUTING_MSI )
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 255
Quoted 10 lines
virt/kvm/irq_comm.c +10
-0+ ret =kvm_set_msi (e, kvm, irq_source_id, level, + host_irq); + else + ret = -EWOULDBLOCK ; +break ; + } +rcu_read_unlock_bh (); +return ret; +} +
Share implementation with kvm_set_irq().
Quoted 10 lines
virt/kvm/irq_comm.c +0
-0void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin) {struct kvm_irq_ack_notifier *kian;Signature · 6 lines
1.7.8.2.325.g247f9 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
--
Gleb.
Quoted 39 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context. Add a new API
kvm_set_irq_inatomic, that does exactly that, and use it to implement an irq handler for msi.This reduces the pressure on scheduler in case where host and guest irq share a host cpu.
Signed-off-by Michael S. Tsirkin <[email protected]>Untested.
Note: this is on top of my host irq patch. Probably needs to be rebased to be independent and split up to new API + usage.--- include/linux/kvm_host.h | 2 + virt/kvm/assigned-dev.c | 31 +++++++++++++++++++++++++- virt/kvm/irq_comm.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-)include/linux/kvm_host.h +2
-0@@ -548,6 +548,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,#endif int kvm_set_irq (struct kvm *kvm,int irq_source_id,u32 irq,int level,int host_irq); +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level, +int host_irq);int kvm_set_msi (struct kvm_kernel_irq_routing_entry *irq_entry,struct kvm *kvm,int irq_source_id,int level,int host_irq);void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin);virt/kvm/assigned-dev.c +0
-0
virt/kvm/assigned-dev.c static int find_index_from_host_irq(struct kvm_assigned_dev_kernel · line 66
Quoted 10 lines
virt/kvm/assigned-dev.c +7
-0@@ -57,6 +57,14 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernelreturn index; } +static irqreturn_t kvm_assigned_dev_msi (int irq,void *dev_id) +{ +int ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + assigned_dev->guest_irq,1 , irq); +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +}
This function gets unused (in theory) if !__KVM_HAVE_MSIX && !__KVM_HAVE_MSI. Should be fixed for consistency reasons.
Quoted 67 lines
virt/kvm/assigned-dev.c +22
-2+static irqreturn_t kvm_assigned_dev_thread (int irq,void *dev_id) {struct kvm_assigned_dev_kernel *assigned_dev = dev_id;@@ -75,6 +83,23 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id) }#ifdef __KVM_HAVE_MSIX +static irqreturn_t kvm_assigned_dev_msix (int irq,void *dev_id) +{ +struct kvm_assigned_dev_kernel *assigned_dev = dev_id; +int index =find_index_from_host_irq (assigned_dev, irq); +u32 vector; +int ret =0 ; + +if (index >=0 ) { + vector = assigned_dev->guest_msix_entries[index].vector; + ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + vector,1 , irq); + } + +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +} +static irqreturn_t kvm_assigned_dev_thread_msix (int irq,void *dev_id) {struct kvm_assigned_dev_kernel *assigned_dev = dev_id;@@ -266,7 +291,8 @@ static int assigned_device_enable_host_msi(struct kvm *kvm, } dev->host_irq = dev->dev->irq; -if (request_threaded_irq (dev->host_irq,NULL , kvm_assigned_dev_thread, +if (request_threaded_irq (dev->host_irq, kvm_assigned_dev_msi, + kvm_assigned_dev_thread,0 , dev->irq_name, dev)) {pci_disable_msi (dev->dev);return -EIO ;@@ -293,7 +319,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,for (i =0 ; i < dev->entries_nr; i++) { r =request_threaded_irq (dev->host_msix_entries[i].vector, -NULL , kvm_assigned_dev_thread_msix, + kvm_assigned_dev_msix, + kvm_assigned_dev_thread_msix,0 , dev->irq_name, dev);if (r) goto err;virt/kvm/irq_comm.c +11
-0@@ -201,6 +201,58 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,return ret; } +static inline struct kvm_kernel_irq_routing_entry * +kvm_get_entry (struct kvm *kvm,struct kvm_irq_routing_table *irq_rq,u32 irq) +{ +struct kvm_kernel_irq_routing_entry *e; +if (likely (irq < irq_rt->nr_rt_entries)) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) +if (e->type ==KVM_IRQ_ROUTING_MSI ) +return e; +else +return ERR_PTR (-EWOULDBLOCK ); +return ERR_PTR (-EINVAL );
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 224
Quoted 10 lines
virt/kvm/irq_comm.c +10
-0+} + +/* +* Deliver an IRQ in an atomic context if we can, or return a failure, +* user can retry in a process context. +* Return value: +* -EWOULDBLOCK Can't deliver in atomic context +* < 0 Interrupt was ignored (masked or not delivered for other reasons) +* = 0 Interrupt was coalesced (previous irq is still pending) +* > 0 Number of CPUs interrupt was delivered to
Where do you make use of =0 vs. >0? Not in this patch at least. Do you consider using it for the APIC timer as well?
Quoted 24 lines
virt/kvm/irq_comm.c +24
-0+*/ +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level, +int host_irq) +{ +struct kvm_kernel_irq_routing_entry *e; +int ret = -EINVAL ; +struct kvm_irq_routing_table *irq_rt; +struct hlist_node *n; + +trace_kvm_set_irq (irq, level, irq_source_id); + +/* +* We know MSI are safe in interrupt context. They are also +* easy as there's a single routing entry for these GSIs. +* So only handle MSI in an atomic context, for now. +*/ +rcu_read_lock_bh (); + irq_rt =rcu_dereference (kvm->irq_routing); +if (irq < irq_rt->nr_rt_entries) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) { +if (ei->type ==KVM_IRQ_ROUTING_MSI ) + ret =kvm_set_msi (e, kvm, irq_source_id, level, + host_irq); +else
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 258
Quoted 10 lines
virt/kvm/irq_comm.c +7
-0+ ret = -EWOULDBLOCK ; +break ; + } +rcu_read_unlock_bh (); +return ret; +} +void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin) {struct kvm_irq_ack_notifier *kian;
Highly welcome feature!
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 8 lines · Gleb Natapov
Quoted 8 lines
virt/kvm/irq_comm.c +0
-0@@ -201,6 +201,58 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,return ret; }
Quoted 13 lines
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 215
Quoted 12 lines
virt/kvm/irq_comm.c +12
-0+static inline struct kvm_kernel_irq_routing_entry * +kvm_get_entry (struct kvm *kvm,struct kvm_irq_routing_table *irq_rq,u32 irq) +{ +struct kvm_kernel_irq_routing_entry *e; +if (likely (irq < irq_rt->nr_rt_entries)) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) +if (e->type ==KVM_IRQ_ROUTING_MSI ) +return e; +else +return ERR_PTR (-EWOULDBLOCK ); +return ERR_PTR (-EINVAL ); +}Unused?
Yes, leftovers from an attempt to reuse kvm_set_irq as you suggested below. It didn't work out - we get much more code this way, so this needs to be removed.
Quoted 35 lines
Quoted 16 lines
virt/kvm/irq_comm.c +16
-0+ +/* +* Deliver an IRQ in an atomic context if we can, or return a failure, +* user can retry in a process context. +* Return value: +* -EWOULDBLOCK Can't deliver in atomic context +* < 0 Interrupt was ignored (masked or not delivered for other reasons) +* = 0 Interrupt was coalesced (previous irq is still pending) +* > 0 Number of CPUs interrupt was delivered to +*/ +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level, +int host_irq) +{ +struct kvm_kernel_irq_routing_entry *e; +int ret = -EINVAL ; +struct kvm_irq_routing_table *irq_rt;virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 241
Quoted 10 lines
virt/kvm/irq_comm.c +10
-0+struct hlist_node *n; + +trace_kvm_set_irq (irq, level, irq_source_id); + +/* +* We know MSI are safe in interrupt context. They are also +* easy as there's a single routing entry for these GSIs. +* So only handle MSI in an atomic context, for now. +*/ +rcu_read_lock_bh ();
_bh?/**
*rcu_read_lock_bh()- mark the beginning of an RCU-bh critical section *
* This is equivalent ofrcu_read_lock(), but to be used when updates
* are being done usingcall_rcu_bh()orsynchronize_rcu_bh().
....
Quoted 2 lines
Since updates to irq routing table are not done using
_bhvariant I doubtrcu_read_lock_bh()is justified here.
Right. Thanks for ppointing this out, I was confused.
Quoted 7 lines
Quoted 7 lines
virt/kvm/irq_comm.c +7
-0+ irq_rt =rcu_dereference (kvm->irq_routing); +if (irq < irq_rt->nr_rt_entries) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) { +if (ei->type ==KVM_IRQ_ROUTING_MSI ) + ret =kvm_set_msi (e, kvm, irq_source_id, level, + host_irq); +else
Quoted 8 lines
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 255
Quoted 7 lines
virt/kvm/irq_comm.c +7
-0+ ret = -EWOULDBLOCK ; +break ; + } +rcu_read_unlock_bh (); +return ret; +} +Share implementation with
kvm_set_irq().
I considered this. There are several reasons not to do it:
- Amount of common code is very small
- As it's separate, it's more obvious that it can't block (kvm_set_irq can block) We can even tag kvm_set_irq with might_sleep.
- This is way simpler and faster as we can do operations directly, instead of copying the irq out, and as it's datapath
an optimization is I think justified.
Quoted 13 lines
Quoted 10 lines
virt/kvm/irq_comm.c +0
-0void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin) {struct kvm_irq_ack_notifier *kian;Signature · 6 lines
1.7.8.2.325.g247f9 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/--
Gleb.
Quoted 43 lines · Jan Kiszka
Quoted 42 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context. Add a new API
kvm_set_irq_inatomic, that does exactly that, and use it to implement an irq handler for msi.This reduces the pressure on scheduler in case where host and guest irq share a host cpu.
Signed-off-by Michael S. Tsirkin <[email protected]>Untested.
Note: this is on top of my host irq patch. Probably needs to be rebased to be independent and split up to new API + usage.--- include/linux/kvm_host.h | 2 + virt/kvm/assigned-dev.c | 31 +++++++++++++++++++++++++- virt/kvm/irq_comm.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 2 deletions(-)include/linux/kvm_host.h +2
-0@@ -548,6 +548,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,#endif int kvm_set_irq (struct kvm *kvm,int irq_source_id,u32 irq,int level,int host_irq); +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level, +int host_irq);int kvm_set_msi (struct kvm_kernel_irq_routing_entry *irq_entry,struct kvm *kvm,int irq_source_id,int level,int host_irq);void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin);virt/kvm/assigned-dev.c +0
-0@@ -57,6 +57,14 @@ static int find_index_from_host_irq(struct kvm_assigned_dev_kernelreturn index; }
Quoted 10 lines
virt/kvm/assigned-dev.c static int find_index_from_host_irq(struct kvm_assigned_dev_kernel · line 66
Quoted 7 lines
virt/kvm/assigned-dev.c +7
-0+static irqreturn_t kvm_assigned_dev_msi (int irq,void *dev_id) +{ +int ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + assigned_dev->guest_irq,1 , irq); +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +}This function gets unused (in theory) if !__KVM_HAVE_MSIX && !__KVM_HAVE_MSI. Should be fixed for consistency reasons.
Does kvm still build with this !__KVM_HAVE_MSI? It does not look like it would ...
Quoted 72 lines
Quoted 72 lines
virt/kvm/assigned-dev.c +22
-2+static irqreturn_t kvm_assigned_dev_thread (int irq,void *dev_id) {struct kvm_assigned_dev_kernel *assigned_dev = dev_id;@@ -75,6 +83,23 @@ static irqreturn_t kvm_assigned_dev_thread(int irq, void *dev_id) }#ifdef __KVM_HAVE_MSIX +static irqreturn_t kvm_assigned_dev_msix (int irq,void *dev_id) +{ +struct kvm_assigned_dev_kernel *assigned_dev = dev_id; +int index =find_index_from_host_irq (assigned_dev, irq); +u32 vector; +int ret =0 ; + +if (index >=0 ) { + vector = assigned_dev->guest_msix_entries[index].vector; + ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + vector,1 , irq); + } + +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +} +static irqreturn_t kvm_assigned_dev_thread_msix (int irq,void *dev_id) {struct kvm_assigned_dev_kernel *assigned_dev = dev_id;@@ -266,7 +291,8 @@ static int assigned_device_enable_host_msi(struct kvm *kvm, } dev->host_irq = dev->dev->irq; -if (request_threaded_irq (dev->host_irq,NULL , kvm_assigned_dev_thread, +if (request_threaded_irq (dev->host_irq, kvm_assigned_dev_msi, + kvm_assigned_dev_thread,0 , dev->irq_name, dev)) {pci_disable_msi (dev->dev);return -EIO ;@@ -293,7 +319,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,for (i =0 ; i < dev->entries_nr; i++) { r =request_threaded_irq (dev->host_msix_entries[i].vector, -NULL , kvm_assigned_dev_thread_msix, + kvm_assigned_dev_msix, + kvm_assigned_dev_thread_msix,0 , dev->irq_name, dev);if (r) goto err;virt/kvm/irq_comm.c +16
-0@@ -201,6 +201,58 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level,return ret; } +static inline struct kvm_kernel_irq_routing_entry * +kvm_get_entry (struct kvm *kvm,struct kvm_irq_routing_table *irq_rq,u32 irq) +{ +struct kvm_kernel_irq_routing_entry *e; +if (likely (irq < irq_rt->nr_rt_entries)) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) +if (e->type ==KVM_IRQ_ROUTING_MSI ) +return e; +else +return ERR_PTR (-EWOULDBLOCK ); +return ERR_PTR (-EINVAL ); +} + +/* +* Deliver an IRQ in an atomic context if we can, or return a failure, +* user can retry in a process context.
Quoted 8 lines
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 224
Quoted 5 lines
virt/kvm/irq_comm.c +5
-0+* Return value: +* -EWOULDBLOCK Can't deliver in atomic context +* < 0 Interrupt was ignored (masked or not delivered for other reasons) +* = 0 Interrupt was coalesced (previous irq is still pending) +* > 0 Number of CPUs interrupt was delivered to Where do you make use of =0 vs. >0? Not in this patch at least. Do you consider using it for the APIC timer as well?
I think it might be handy in the future. It's an internal function (not exported) so I just return what I get without worrying about it too much: I didn't need to add code to get this :)
Quoted 37 lines
Quoted 24 lines
virt/kvm/irq_comm.c +24
-0+*/ +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level, +int host_irq) +{ +struct kvm_kernel_irq_routing_entry *e; +int ret = -EINVAL ; +struct kvm_irq_routing_table *irq_rt; +struct hlist_node *n; + +trace_kvm_set_irq (irq, level, irq_source_id); + +/* +* We know MSI are safe in interrupt context. They are also +* easy as there's a single routing entry for these GSIs. +* So only handle MSI in an atomic context, for now. +*/ +rcu_read_lock_bh (); + irq_rt =rcu_dereference (kvm->irq_routing); +if (irq < irq_rt->nr_rt_entries) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) { +if (ei->type ==KVM_IRQ_ROUTING_MSI ) + ret =kvm_set_msi (e, kvm, irq_source_id, level, + host_irq); +else virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 258
Quoted 10 lines
virt/kvm/irq_comm.c +7
-0+ ret = -EWOULDBLOCK ; +break ; + } +rcu_read_unlock_bh (); +return ret; +} +void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin) {struct kvm_irq_ack_notifier *kian;Highly welcome feature!
Quoted 1 line
Jan
I think we need to fix MSIX masking now, otherwise delivering interrupt faster might get us a storm of interrupts.
Quoted 3 lines
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 16 lines · Michael S. Tsirkin
Quoted 7 lines
Quoted 7 lines
virt/kvm/irq_comm.c +7
-0+ irq_rt =rcu_dereference (kvm->irq_routing); +if (irq < irq_rt->nr_rt_entries) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) { +if (ei->type ==KVM_IRQ_ROUTING_MSI ) + ret =kvm_set_msi (e, kvm, irq_source_id, level, + host_irq); +else Quoted 8 lines
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 255
Quoted 7 lines
virt/kvm/irq_comm.c +7
-0+ ret = -EWOULDBLOCK ; +break ; + } +rcu_read_unlock_bh (); +return ret; +} +Share implementation with
kvm_set_irq().
Quoted 2 lines
I considered this. There are several reasons not to do it:
- Amount of common code is very small
Why? Just pass msi_only flag to kvm_set_irq() and skip an entry if flag is set and entry type is not msi.
Quoted 2 lines
- As it's separate, it's more obvious that it can't block (
kvm_set_irqcan block) We can even tagkvm_set_irqwithmight_sleep.
They can still be two separate function calling common one.
Quoted 3 lines
- This is way simpler and faster as we can do operations directly, instead of copying the irq out, and as it's datapath an optimization is I think justified.
I really do not think the copy of one small data structure will be measurable. If it is (has to be proven) we can optimize that two in the common code.
--
Gleb.
Quoted 17 lines · Gleb Natapov
Quoted 16 lines · Michael S. Tsirkin
Quoted 15 lines
Quoted 4 lines
virt/kvm/irq_comm.c +4
-0+ irq_rt =rcu_dereference (kvm->irq_routing); +if (irq < irq_rt->nr_rt_entries) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) { +if (ei->type ==KVM_IRQ_ROUTING_MSI )virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 255
Quoted 10 lines
virt/kvm/irq_comm.c +10
-0+ ret =kvm_set_msi (e, kvm, irq_source_id, level, + host_irq); + else + ret = -EWOULDBLOCK ; +break ; + } +rcu_read_unlock_bh (); +return ret; +} +Share implementation with
kvm_set_irq().
Quoted 8 lines
Quoted 2 lines
I considered this. There are several reasons not to do it:
- Amount of common code is very smallWhy? Just pass
msi_onlyflag tokvm_set_irq()and skip an entry if flag is set and entry type is not msi.Quoted 2 lines
- As it's separate, it's more obvious that it can't block (
kvm_set_irqcan block) We can even tagkvm_set_irqwithmight_sleep.They can still be two separate function calling common one.
No, the common code is the surrounding foreach loop, the internal if branch is different.
Quoted 9 lines
Quoted 3 lines
- This is way simpler and faster as we can do operations directly, instead of copying the irq out, and as it's datapath an optimization is I think justified.
I really do not think the copy of one small data structure will be measurable. If it is (has to be proven) we can optimize that two in the common code.
--
Gleb.
Quoted 23 lines · Michael S. Tsirkin
Quoted 22 lines · Gleb Natapov
Quoted 16 lines · Michael S. Tsirkin
Quoted 7 lines
Quoted 7 lines
virt/kvm/irq_comm.c +7
-0+ irq_rt =rcu_dereference (kvm->irq_routing); +if (irq < irq_rt->nr_rt_entries) +hlist_for_each_entry (e, n, &irq_rt->map[irq], link) { +if (ei->type ==KVM_IRQ_ROUTING_MSI ) + ret =kvm_set_msi (e, kvm, irq_source_id, level, + host_irq); +else Quoted 8 lines
virt/kvm/irq_comm.c int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level, · line 255
Quoted 7 lines
virt/kvm/irq_comm.c +7
-0+ ret = -EWOULDBLOCK ; +break ; + } +rcu_read_unlock_bh (); +return ret; +} +Share implementation with
kvm_set_irq().Quoted 2 lines
I considered this. There are several reasons not to do it:
- Amount of common code is very smallWhy? Just pass
msi_onlyflag tokvm_set_irq()and skip an entry if flag is set and entry type is not msi.
Quoted 7 lines
Quoted 3 lines
Quoted 2 lines
- As it's separate, it's more obvious that it can't block (
kvm_set_irqcan block) We can even tagkvm_set_irqwithmight_sleep.They can still be two separate function calling common one.
No, the common code is the surrounding foreach loop, the internal if branch is different.
I do not see any complication whatsoever. The reuse it trivial.
--
Gleb.