kvm: direct msix injection
v3 cover kvm: direct msix injection
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).
Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying with kvm_set_irq: passing flags to it looks too ugly. Added a comment.
Jan, you said you can test this?
Michael S. Tsirkin (2):
kvm: implement kvm_set_msi_inatomic
kvm: deliver msix interrupts from irq handler
include/linux/kvm_host.h | 3 ++ virt/kvm/assigned-dev.c | 31 ++++++++++++++++++-- virt/kvm/irq_comm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 7 deletions(-)
Signature · 1 line
MST
RFC PATCH v3 1/2 kvm: implement kvm_set_msi_inatomic
We do not want a potential broadcast to all VCPUs to run in a host IRQ handler. Implement an API that sends an MSI interrupt but only if it's safe from interrupt context, that is if it is a unicast.
--- include/linux/kvm_host.h | 2 ++ virt/kvm/irq_comm.c | 37 ++++++++++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-)
include/linux/kvm_host.h +2 -0
@@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,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); +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *irq_entry, +struct kvm *kvm);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);
virt/kvm/irq_comm.c +32 -5
@@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,return r; } -int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, -struct kvm *kvm,int irq_source_id,int level) +static bool kvm_msi_is_multicast (unsigned dest,int dest_mode) { -struct kvm_lapic_irq irq; +if (dest_mode ==0 ) +/* Physical mode. */ +return dest ==0xff ; +else +/* Logical mode. */ +return dest & (dest -1 ); +} -if (!level) -return -1 ; +static int __kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,bool noblock) +{ +struct kvm_lapic_irq irq;trace_kvm_msi_set_irq (e->msi.address_lo, e->msi.data);
@@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, irq.level =1 ; irq.shorthand =0 ; +/* Multicast MSI doesn't really block but might take a long time. */ +if (unlikely (noblock &&kvm_msi_is_multicast (irq.dest_id, + irq.delivery_mode))) +return -EWOULDBLOCK ; +/* TODO Deal with RH bit of MSI message address */ return kvm_irq_delivery_to_apic (kvm,NULL , &irq); } +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm) +{ +return __kvm_set_msi (e, kvm, true); +} + +int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,int irq_source_id,int level) +{ +if (unlikely (!level)) +return -1 ; + +return __kvm_set_msi (e, kvm, false); +} +int kvm_send_userspace_msi (struct kvm *kvm,struct kvm_msi *msi) {struct kvm_kernel_irq_routing_entry route;
Signature · 1 line
MST
Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic
Quoted 5 lines · Michael S. Tsirkin
We do not want a potential broadcast to all VCPUs to run in a host IRQ handler. Implement an API that sends an MSI interrupt but only if it's safe from interrupt context, that is if it is a unicast.
We will still iterate over all vcpus, we may as well deliver to them. Or you want to avoid a lot of vcpu kicks?
Quoted 50 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- include/linux/kvm_host.h | 2 ++ virt/kvm/irq_comm.c | 37 ++++++++++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-)include/linux/kvm_host.h +2
-0@@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,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); +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *irq_entry, +struct kvm *kvm);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);virt/kvm/irq_comm.c +12
-5@@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,return r; } -int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, -struct kvm *kvm,int irq_source_id,int level) +static bool kvm_msi_is_multicast (unsigned dest,int dest_mode) { -struct kvm_lapic_irq irq; +if (dest_mode ==0 ) +/* Physical mode. */ +return dest ==0xff ; +else +/* Logical mode. */ +return dest & (dest -1 ); +} -if (!level) -return -1 ; +static int __kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,bool noblock) +{ +struct kvm_lapic_irq irq;trace_kvm_msi_set_irq (e->msi.address_lo, e->msi.data);
virt/kvm/irq_comm.c int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, · line 146
Quoted 6 lines
virt/kvm/irq_comm.c +3
-0@@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, irq.level =1 ; irq.shorthand =0 ; +/* Multicast MSI doesn't really block but might take a long time. */ +if (unlikely (noblock &&kvm_msi_is_multicast (irq.dest_id, + irq.delivery_mode)))
delivery_mode? Should be dest_mode. But you probably need to check that delivery_mode is not ExtINT either.
Quoted 31 lines
virt/kvm/irq_comm.c +17
-0+return -EWOULDBLOCK ; +/* TODO Deal with RH bit of MSI message address */ return kvm_irq_delivery_to_apic (kvm,NULL , &irq); } +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm) +{ +return __kvm_set_msi (e, kvm, true); +} + +int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,int irq_source_id,int level) +{ +if (unlikely (!level)) +return -1 ; + +return __kvm_set_msi (e, kvm, false); +} +int kvm_send_userspace_msi (struct kvm *kvm,struct kvm_msi *msi) {struct kvm_kernel_irq_routing_entry route;Signature · 6 lines
MST -- 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
--
Gleb.
Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic
Quoted 8 lines · Gleb Natapov
Quoted 5 lines · Michael S. Tsirkin
We do not want a potential broadcast to all VCPUs to run in a host IRQ handler. Implement an API that sends an MSI interrupt but only if it's safe from interrupt context, that is if it is a unicast.
We will still iterate over all vcpus, we may as well deliver to them. Or you want to avoid a lot of vcpu kicks?
This is an intermediate stage (thus RFC) for adding a cache:
we can't cache if it's a broadcast.
Quoted 93 lines
Quoted 50 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- include/linux/kvm_host.h | 2 ++ virt/kvm/irq_comm.c | 37 ++++++++++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-)include/linux/kvm_host.h +2
-0@@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,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); +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *irq_entry, +struct kvm *kvm);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);virt/kvm/irq_comm.c +12
-5@@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,return r; } -int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, -struct kvm *kvm,int irq_source_id,int level) +static bool kvm_msi_is_multicast (unsigned dest,int dest_mode) { -struct kvm_lapic_irq irq; +if (dest_mode ==0 ) +/* Physical mode. */ +return dest ==0xff ; +else +/* Logical mode. */ +return dest & (dest -1 ); +} -if (!level) -return -1 ; +static int __kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,bool noblock) +{ +struct kvm_lapic_irq irq;trace_kvm_msi_set_irq (e->msi.address_lo, e->msi.data);virt/kvm/irq_comm.c int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, · line 146
Quoted 6 lines
virt/kvm/irq_comm.c +3
-0@@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, irq.level =1 ; irq.shorthand =0 ; +/* Multicast MSI doesn't really block but might take a long time. */ +if (unlikely (noblock &&kvm_msi_is_multicast (irq.dest_id, + irq.delivery_mode)))
delivery_mode? Should bedest_mode. But you probably need to check thatdelivery_modeis not ExtINT either.Quoted 31 lines
virt/kvm/irq_comm.c +17
-0+return -EWOULDBLOCK ; +/* TODO Deal with RH bit of MSI message address */ return kvm_irq_delivery_to_apic (kvm,NULL , &irq); } +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm) +{ +return __kvm_set_msi (e, kvm, true); +} + +int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,int irq_source_id,int level) +{ +if (unlikely (!level)) +return -1 ; + +return __kvm_set_msi (e, kvm, false); +} +int kvm_send_userspace_msi (struct kvm *kvm,struct kvm_msi *msi) {struct kvm_kernel_irq_routing_entry route;Signature · 6 lines
MST -- 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--
Gleb.
Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Gleb Natapov
Quoted 5 lines · Michael S. Tsirkin
We do not want a potential broadcast to all VCPUs to run in a host IRQ handler. Implement an API that sends an MSI interrupt but only if it's safe from interrupt context, that is if it is a unicast.
Quoted 6 lines
Quoted 2 lines
We will still iterate over all vcpus, we may as well deliver to them. Or you want to avoid a lot of vcpu kicks?
This is an intermediate stage (thus RFC) for adding a cache:
we can't cache if it's a broadcast.
You skipped my comment about the code bellow :)
Quoted 86 lines
Quoted 86 lines
Quoted 50 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- include/linux/kvm_host.h | 2 ++ virt/kvm/irq_comm.c | 37 ++++++++++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-)include/linux/kvm_host.h +2
-0@@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,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); +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *irq_entry, +struct kvm *kvm);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);virt/kvm/irq_comm.c +12
-5@@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,return r; } -int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, -struct kvm *kvm,int irq_source_id,int level) +static bool kvm_msi_is_multicast (unsigned dest,int dest_mode) { -struct kvm_lapic_irq irq; +if (dest_mode ==0 ) +/* Physical mode. */ +return dest ==0xff ; +else +/* Logical mode. */ +return dest & (dest -1 ); +} -if (!level) -return -1 ; +static int __kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,bool noblock) +{ +struct kvm_lapic_irq irq;trace_kvm_msi_set_irq (e->msi.address_lo, e->msi.data);virt/kvm/irq_comm.c int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, · line 146
Quoted 6 lines
virt/kvm/irq_comm.c +3
-0@@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, irq.level =1 ; irq.shorthand =0 ; +/* Multicast MSI doesn't really block but might take a long time. */ +if (unlikely (noblock &&kvm_msi_is_multicast (irq.dest_id, + irq.delivery_mode)))
delivery_mode? Should bedest_mode. But you probably need to check thatdelivery_modeis not ExtINT either.Quoted 27 lines
virt/kvm/irq_comm.c +17
-0+return -EWOULDBLOCK ; +/* TODO Deal with RH bit of MSI message address */ return kvm_irq_delivery_to_apic (kvm,NULL , &irq); } +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm) +{ +return __kvm_set_msi (e, kvm, true); +} + +int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,int irq_source_id,int level) +{ +if (unlikely (!level)) +return -1 ; + +return __kvm_set_msi (e, kvm, false); +} +int kvm_send_userspace_msi (struct kvm *kvm,struct kvm_msi *msi) {struct kvm_kernel_irq_routing_entry route;Signature · 1 line
MST
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
--
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--
Gleb.
--
Gleb.
Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic
Quoted 8 lines · Gleb Natapov
Quoted 7 lines · Michael S. Tsirkin
Quoted 6 lines · Gleb Natapov
Quoted 5 lines · Michael S. Tsirkin
We do not want a potential broadcast to all VCPUs to run in a host IRQ handler. Implement an API that sends an MSI interrupt but only if it's safe from interrupt context, that is if it is a unicast.
Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
We will still iterate over all vcpus, we may as well deliver to them. Or you want to avoid a lot of vcpu kicks?
This is an intermediate stage (thus RFC) for adding a cache:
we can't cache if it's a broadcast.You skipped my comment about the code bellow :)
Ouch. I missed it.
Thanks for pointing out and sorry.
Quoted 49 lines
Quoted 49 lines
Quoted 49 lines
Quoted 49 lines
Signed-off-by Michael S. Tsirkin <[email protected]>--- include/linux/kvm_host.h | 2 ++ virt/kvm/irq_comm.c | 37 ++++++++++++++++++++++++++++++++----- 2 files changed, 34 insertions(+), 5 deletions(-)include/linux/kvm_host.h +2
-0@@ -618,6 +618,8 @@ void kvm_get_intr_delivery_bitmask(struct kvm_ioapic *ioapic,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); +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *irq_entry, +struct kvm *kvm);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);virt/kvm/irq_comm.c +12
-5@@ -114,13 +114,20 @@ int kvm_irq_delivery_to_apic(struct kvm *kvm, struct kvm_lapic *src,return r; } -int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, -struct kvm *kvm,int irq_source_id,int level) +static bool kvm_msi_is_multicast (unsigned dest,int dest_mode) { -struct kvm_lapic_irq irq; +if (dest_mode ==0 ) +/* Physical mode. */ +return dest ==0xff ; +else +/* Logical mode. */ +return dest & (dest -1 ); +} -if (!level) -return -1 ; +static int __kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,bool noblock) +{ +struct kvm_lapic_irq irq;trace_kvm_msi_set_irq (e->msi.address_lo, e->msi.data);
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
virt/kvm/irq_comm.c int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, · line 146
Quoted 7 lines
virt/kvm/irq_comm.c +3
-0@@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, irq.level =1 ; irq.shorthand =0 ; +/* Multicast MSI doesn't really block but might take a long time. */ +if (unlikely (noblock &&kvm_msi_is_multicast (irq.dest_id, + irq.delivery_mode)))
delivery_mode? Should bedest_mode.
Yes. Good catch, thanks.
Quoted 2 lines
But you probably need to check that
Quoted 1 line
Quoted 1 line
delivery_modeis not ExtINT either.
It does not look like anything happens with ExtInt if you try to trigger it from MSI.
Quoted 38 lines
Quoted 28 lines
Quoted 28 lines
Quoted 28 lines
virt/kvm/irq_comm.c +17
-0+return -EWOULDBLOCK ; +/* TODO Deal with RH bit of MSI message address */ return kvm_irq_delivery_to_apic (kvm,NULL , &irq); } +int kvm_set_msi_inatomic (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm) +{ +return __kvm_set_msi (e, kvm, true); +} + +int kvm_set_msi (struct kvm_kernel_irq_routing_entry *e, +struct kvm *kvm,int irq_source_id,int level) +{ +if (unlikely (!level)) +return -1 ; + +return __kvm_set_msi (e, kvm, false); +} +int kvm_send_userspace_msi (struct kvm *kvm,struct kvm_msi *msi) {struct kvm_kernel_irq_routing_entry route;Signature · 1 line
MSTQuoted 7 lines
Quoted 7 lines
Quoted 4 lines
--
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--
Gleb.--
Gleb.
Re: [PATCHv3 RFC 1/2] kvm: implement kvm_set_msi_inatomic
Quoted 9 lines · Michael S. Tsirkin
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
virt/kvm/irq_comm.c int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, · line 146
Quoted 7 lines
virt/kvm/irq_comm.c +3
-0@@ -134,10 +141,30 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, irq.level =1 ; irq.shorthand =0 ; +/* Multicast MSI doesn't really block but might take a long time. */ +if (unlikely (noblock &&kvm_msi_is_multicast (irq.dest_id, + irq.delivery_mode)))
delivery_mode? Should bedest_mode.
Quoted 8 lines
Yes. Good catch, thanks.
Quoted 2 lines
But you probably need to check that
Quoted 1 line
Quoted 1 line
delivery_modeis not ExtINT either.It does not look like anything happens with ExtInt if you try to trigger it from MSI.
Currently no, but it should appear as if interrupt comes from PIC. I wouldn't allow anything but fixed mode here just to be on a safe side. Lowest prio will have to loop even after introducing irq cache.
--
Gleb.
RFC PATCH v3 2/2 kvm: deliver msix interrupts from irq handler
In reply to Michael S. TsirkinWe 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.
Changes from v1:
Tried to address comments from v1.
Not all suggestions from v1 were addressed yet, but run out of time and weekend is starting.
Posting so Jan can test if he likes.
--- include/linux/kvm_host.h | 1 + virt/kvm/assigned-dev.c | 31 +++++++++++++++++++++++++++++-- virt/kvm/irq_comm.c | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-)
include/linux/kvm_host.h +1 -0
@@ -616,6 +616,7 @@ 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_irq_inatomic (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); int kvm_set_msi_inatomic(struct kvm_kernel_irq_routing_entry *irq_entry,
virt/kvm/assigned-dev.c +29 -2
@@ -117,6 +117,23 @@ static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)#endif #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 ); + } + +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;
@@ -334,6 +351,15 @@ static int assigned_device_enable_host_intx(struct kvm *kvm, }#ifdef __KVM_HAVE_MSI +static irqreturn_t kvm_assigned_dev_msi (int irq,void *dev_id) +{ +struct kvm_assigned_dev_kernel *assigned_dev = dev_id; +int ret =kvm_set_irq_inatomic (assigned_dev->kvm, + assigned_dev->irq_source_id, + assigned_dev->guest_irq,1 ); +return unlikely (ret == -EWOULDBLOCK ) ?IRQ_WAKE_THREAD :IRQ_HANDLED ; +} +static int assigned_device_enable_host_msi (struct kvm *kvm,struct kvm_assigned_dev_kernel *dev) {
@@ -346,7 +372,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm, } dev->host_irq = dev->dev->irq; -if (request_threaded_irq (dev->host_irq,NULL , +if (request_threaded_irq (dev->host_irq, kvm_assigned_dev_msi, kvm_assigned_dev_thread_msi,0 , dev->irq_name, dev)) {pci_disable_msi (dev->dev);
@@ -374,7 +400,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 +38 -0
@@ -217,6 +217,44 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)return ret; } +/* +* 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: retry in a process context. +* Other values - No need to retry. +*/ +int kvm_set_irq_inatomic (struct kvm *kvm,int irq_source_id,u32 irq,int level) +{ +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. +* +* This shares some code with kvm_set_irq: this +* version is optimized for a single entry MSI only case. +*/ +rcu_read_lock (); + 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 (likely (e->type ==KVM_IRQ_ROUTING_MSI )) + ret =kvm_set_msi_inatomic (e, kvm); +else + ret = -EWOULDBLOCK ; +break ; + } +rcu_read_unlock (); +return ret; +} +void kvm_notify_acked_irq (struct kvm *kvm,unsigned irqchip,unsigned pin) {struct kvm_irq_ack_notifier *kian;
Signature · 1 line
MST
Quoted 4 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).
Quoted 8 lines
Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.Jan, you said you can test this?
Looks fine to me (except the unlikely). Avi/Gleb?
Quoted 12 lines · Marcelo Tosatti
Quoted 11 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.
Quoted 3 lines
Quoted 1 line
Jan, you said you can test this?
Looks fine to me (except the unlikely). Avi/Gleb?
Which of the unlikely do you mean?
Most of them cover the case where msix is a broadcast or where level is 0 for msi. This never triggered in my testing. So the unlikely seems justified.
Signature · 1 line
MST
Quoted 12 lines · Marcelo Tosatti
Quoted 11 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.
Quoted 3 lines
Quoted 1 line
Jan, you said you can test this?
Looks fine to me (except the unlikely). Avi/Gleb?
I dislike the vcpu loop in atomic context. Some crazies keep increasing the maximum vcpu count.
Signature · 1 line
error compiling committee.c: too many arguments to function
Quoted 14 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.Jan, you said you can test this?
Quoted 9 lines
Michael S. Tsirkin (2):
kvm: implementkvm_set_msi_inatomic
kvm: deliver msix interrupts from irq handlerinclude/linux/kvm_host.h | 3 ++ virt/kvm/assigned-dev.c | 31 ++++++++++++++++++-- virt/kvm/irq_comm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 7 deletions(-)
Thanks,
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Quoted 19 lines · Jan Kiszka
Quoted 18 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.Jan, you said you can test this?
Michael S. Tsirkin (2):
kvm: implementkvm_set_msi_inatomic
kvm: deliver msix interrupts from irq handler
Quoted 7 lines
Quoted 5 lines
include/linux/kvm_host.h | 3 ++ virt/kvm/assigned-dev.c | 31 ++++++++++++++++++-- virt/kvm/irq_comm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 7 deletions(-)Finally-tested-by Jan Kiszka <[email protected]>
Michael, we need either this or the simple oneshot patch to get device assignment working again for 3.5. Are you planning to push this for 3.5? Thanks,
Alex
Hi, Michael/Alex, do you have progress for device assignment issue fixing?
https://bugzilla.kernel.org/show_bug.cgi?id=43328
Thanks,
-Xudong
Quoted 49 lines
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Alex Williamson
Sent: Tuesday, July 03, 2012 1:08 AM
To: Jan Kiszka
Cc: Michael S. Tsirkin; [email protected]
Subject: Re: [PATCHv3 RFC 0/2] kvm: direct msix injectionQuoted 15 lines · Jan Kiszka
Quoted 14 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.Jan, you said you can test this?
Quoted 7 lines
Quoted 7 lines
Michael S. Tsirkin (2):
kvm: implementkvm_set_msi_inatomic
kvm: deliver msix interrupts from irq handlerinclude/linux/kvm_host.h | 3 ++ virt/kvm/assigned-dev.c | 31 ++++++++++++++++++-- virt/kvm/irq_comm.c | 75++++++++++++++++++++++++++++++++++++++++++++----
Quoted 4 lines
Quoted 2 lines
3 files changed, 102 insertions(+), 7 deletions(-)Finally-tested-by Jan Kiszka <[email protected]>Michael, we need either this or the simple oneshot patch to get device assignment working again for 3.5. Are you planning to push this for 3.5? Thanks,
Alex
--
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
Quoted 26 lines · Alex Williamson
Quoted 25 lines · Jan Kiszka
Quoted 23 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.Jan, you said you can test this?
Michael S. Tsirkin (2):
kvm: implementkvm_set_msi_inatomic
kvm: deliver msix interrupts from irq handlerinclude/linux/kvm_host.h | 3 ++ virt/kvm/assigned-dev.c | 31 ++++++++++++++++++-- virt/kvm/irq_comm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 7 deletions(-)
Quoted 8 lines
Quoted 1 line
Finally-tested-by Jan Kiszka <[email protected]>Michael, we need either this or the simple oneshot patch to get device assignment working again for 3.5. Are you planning to push this for 3.5? Thanks,
Alex
Avi wants this reworked using an injection cache. I agree with Jan
though: oneshot looks too ugly. Just so you can make progress, can't we add a stub handler returning IRQ_WAKE_THREAD unconditionally for now? I.e. like the below (warning: completely untested).
--
virt/kvm/assigned-dev.c +13 -2
@@ -334,6 +334,11 @@ static int assigned_device_enable_host_intx(struct kvm *kvm, }#ifdef __KVM_HAVE_MSI +static irqreturn_t kvm_assigned_dev_msi (int irq,void *dev_id) +{ +return IRQ_WAKE_THREAD ; +} +static int assigned_device_enable_host_msi (struct kvm *kvm,struct kvm_assigned_dev_kernel *dev) {
@@ -346,7 +351,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm, } dev->host_irq = dev->dev->irq; -if (request_threaded_irq (dev->host_irq,NULL , +if (request_threaded_irq (dev->host_irq, kvm_assigned_dev_msi, kvm_assigned_dev_thread_msi,0 , dev->irq_name, dev)) {pci_disable_msi (dev->dev);
@@ -358,6 +363,11 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,#endif #ifdef __KVM_HAVE_MSIX +static irqreturn_t kvm_assigned_dev_msix (int irq,void *dev_id) +{ +return IRQ_WAKE_THREAD ; +} +static int assigned_device_enable_host_msix (struct kvm *kvm,struct kvm_assigned_dev_kernel *dev) {
@@ -374,7 +384,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;
Signature · 1 line
MST
Quoted 84 lines · Michael S. Tsirkin
Quoted 26 lines · Alex Williamson
Quoted 25 lines · Jan Kiszka
Quoted 23 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.Jan, you said you can test this?
Michael S. Tsirkin (2):
kvm: implementkvm_set_msi_inatomic
kvm: deliver msix interrupts from irq handlerinclude/linux/kvm_host.h | 3 ++ virt/kvm/assigned-dev.c | 31 ++++++++++++++++++-- virt/kvm/irq_comm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 7 deletions(-)Quoted 8 lines
Quoted 1 line
Finally-tested-by Jan Kiszka <[email protected]>Michael, we need either this or the simple oneshot patch to get device assignment working again for 3.5. Are you planning to push this for 3.5? Thanks,
Alex
Avi wants this reworked using an injection cache. I agree with Jan
though: oneshot looks too ugly. Just so you can make progress, can't we add a stub handler returningIRQ_WAKE_THREADunconditionally for now? I.e. like the below (warning: completely untested).Signed-off-by Michael S. Tsirkin <[email protected]>--
virt/kvm/assigned-dev.c +11
-1@@ -334,6 +334,11 @@ static int assigned_device_enable_host_intx(struct kvm *kvm, }#ifdef __KVM_HAVE_MSI +static irqreturn_t kvm_assigned_dev_msi (int irq,void *dev_id) +{ +return IRQ_WAKE_THREAD ; +} +static int assigned_device_enable_host_msi (struct kvm *kvm,struct kvm_assigned_dev_kernel *dev) {@@ -346,7 +351,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm, } dev->host_irq = dev->dev->irq; -if (request_threaded_irq (dev->host_irq,NULL , +if (request_threaded_irq (dev->host_irq, kvm_assigned_dev_msi, kvm_assigned_dev_thread_msi,0 , dev->irq_name, dev)) {pci_disable_msi (dev->dev);@@ -358,6 +363,11 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,#endif #ifdef __KVM_HAVE_MSIX +static irqreturn_t kvm_assigned_dev_msix (int irq,void *dev_id) +{ +return IRQ_WAKE_THREAD ; +} +static int assigned_device_enable_host_msix (struct kvm *kvm,struct kvm_assigned_dev_kernel *dev) {
virt/kvm/assigned-dev.c static int assigned_device_enable_host_msix(struct kvm *kvm, · line 391
Quoted 9 lines
virt/kvm/assigned-dev.c +2
-1@@ -374,7 +384,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;
Should restore the status quo before 3.5's IRQ layer changes. Looks ok to me.
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Quoted 85 lines · Jan Kiszka
Quoted 84 lines · Michael S. Tsirkin
Quoted 26 lines · Alex Williamson
Quoted 25 lines · Jan Kiszka
Quoted 23 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSIX, from atomic context.
Here's an untested patch to do this (compiled only).Changes from v2:
Don't inject broadcast interrupts directly Changes from v1:
Tried to address comments from v1, except unifying withkvm_set_irq: passing flags to it looks too ugly. Added a comment.Jan, you said you can test this?
Michael S. Tsirkin (2):
kvm: implementkvm_set_msi_inatomic
kvm: deliver msix interrupts from irq handlerinclude/linux/kvm_host.h | 3 ++ virt/kvm/assigned-dev.c | 31 ++++++++++++++++++-- virt/kvm/irq_comm.c | 75 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 102 insertions(+), 7 deletions(-)Quoted 8 lines
Quoted 1 line
Finally-tested-by Jan Kiszka <[email protected]>Michael, we need either this or the simple oneshot patch to get device assignment working again for 3.5. Are you planning to push this for 3.5? Thanks,
Alex
Avi wants this reworked using an injection cache. I agree with Jan
though: oneshot looks too ugly. Just so you can make progress, can't we add a stub handler returningIRQ_WAKE_THREADunconditionally for now? I.e. like the below (warning: completely untested).Signed-off-by Michael S. Tsirkin <[email protected]>--
virt/kvm/assigned-dev.c +11
-1@@ -334,6 +334,11 @@ static int assigned_device_enable_host_intx(struct kvm *kvm, }#ifdef __KVM_HAVE_MSI +static irqreturn_t kvm_assigned_dev_msi (int irq,void *dev_id) +{ +return IRQ_WAKE_THREAD ; +} +static int assigned_device_enable_host_msi (struct kvm *kvm,struct kvm_assigned_dev_kernel *dev) {@@ -346,7 +351,7 @@ static int assigned_device_enable_host_msi(struct kvm *kvm, } dev->host_irq = dev->dev->irq; -if (request_threaded_irq (dev->host_irq,NULL , +if (request_threaded_irq (dev->host_irq, kvm_assigned_dev_msi, kvm_assigned_dev_thread_msi,0 , dev->irq_name, dev)) {pci_disable_msi (dev->dev);@@ -358,6 +363,11 @@ static int assigned_device_enable_host_msi(struct kvm *kvm,#endif #ifdef __KVM_HAVE_MSIX +static irqreturn_t kvm_assigned_dev_msix (int irq,void *dev_id) +{ +return IRQ_WAKE_THREAD ; +} +static int assigned_device_enable_host_msix (struct kvm *kvm,struct kvm_assigned_dev_kernel *dev) {@@ -374,7 +384,8 @@ static int assigned_device_enable_host_msix(struct kvm *kvm,
Quoted 12 lines
virt/kvm/assigned-dev.c static int assigned_device_enable_host_msix(struct kvm *kvm, · line 391
Quoted 9 lines
virt/kvm/assigned-dev.c +2
-1for (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;Should restore the status quo before 3.5's IRQ layer changes. Looks ok to me.
I'll test and post this, Michael is out this week. Thanks,
Alex