kvm: direct msix injection

PATCH Applied Michael S. Tsirkin 3 participants 16 messages Oct 17, 2012 – Dec 5, 2012 KVM

Share & tools

Michael S. Tsirkin

author

2012-10-17 15:03 UTC #0 lore raw

v4 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: add kvm_set_irq_inatomic
kvm: deliver msi interrupts from irq handler

 include/linux/kvm_host.h |  1 +
 virt/kvm/assigned-dev.c  | 36 +++++++++++++++------
 virt/kvm/irq_comm.c      | 83 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 98 insertions(+), 22 deletions(-)

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-10-17 15:04 UTC #1 lore raw

PATCH v4 1/2 kvm: add kvm_set_irq_inatomic

Add an API to inject IRQ from atomic context. Return EWOULDBLOCK if impossible (e.g. for multicast). Only MSI is supported ATM.

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

---
 include/linux/kvm_host.h |  1 +
 virt/kvm/irq_comm.c      | 83 +++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 72 insertions(+), 12 deletions(-)

include/linux/kvm_host.h +1 -0

index 93bfc9f..e165c09 100644
@@ -677,6 +677,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);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin);

virt/kvm/irq_comm.c +71 ~~-12~~

105 lines not on this page · raw message

Signature · 1 line

MST

Michael S. Tsirkin

author

2012-10-17 15:04 UTC #2 lore raw

PATCH v4 2/2 kvm: deliver msi interrupts from irq handler

In reply to Michael S. Tsirkin

We can deliver certain interrupts, notably MSI, from atomic context. Use kvm_set_irq_inatomic, 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]>

---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

virt/kvm/assigned-dev.c +26 ~~-10~~

index 23a41a9..3642239 100644
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
 #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 irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Signature · 1 line

MST

Gleb Natapov

2012-11-28 11:43 UTC #3 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 17 lines · Michael S. Tsirkin

We can deliver certain interrupts, notably MSI,
from atomic context. Use kvm_set_irq_inatomic,
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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644

virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113

Quoted 9 lines

virt/kvm/assigned-dev.c +6 -0

@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
 #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);

Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?

Quoted 60 lines

virt/kvm/assigned-dev.c +20 ~~-10~~

+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Signature · 5 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.

Michael S. Tsirkin

author

2012-11-28 11:53 UTC #4 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 20 lines · Gleb Natapov

Quoted 19 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSI,
from atomic context. Use kvm_set_irq_inatomic,
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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 

Quoted 9 lines

virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113
Quoted 7 lines
virt/kvm/assigned-dev.c +6 -0
 #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?

kvm_set_msi_inatomic needs a routing entry, and we don't have the routing entry at this level.

Further, guest irq might not be an MSI: host MSI can cause guest intx injection I think, we need to bounce it to thread as we did earlier.

Quoted 63 lines

Quoted 60 lines

virt/kvm/assigned-dev.c +20 ~~-10~~

+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Signature · 5 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.

Gleb Natapov

2012-11-28 12:13 UTC #5 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 31 lines · Michael S. Tsirkin

Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSI,
from atomic context. Use kvm_set_irq_inatomic,
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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
Quoted 9 lines
virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113
Quoted 7 lines
virt/kvm/assigned-dev.c +6 -0
 #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?

Quoted 3 lines

kvm_set_msi_inatomic needs a routing entry, and we don't have the routing entry at this level.

Yes, right. BTW is this interface will be used only for legacy assigned device or there will be other users too?

Quoted 3 lines

Further, guest irq might not be an MSI: host MSI can cause guest intx injection I think, we need to bounce it to thread as we did earlier.

Ah, so msi in kvm_assigned_dev_msi() is about host msi? Can host be intx but guest msi? You seems to not handle this case. Also injection of intx via ioapic is the same as injecting MSI. The format and the capability of irq message are essentially the same.

Quoted 56 lines

virt/kvm/assigned-dev.c +20 ~~-10~~

+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Quoted 8 lines

Quoted 8 lines
Quoted 5 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.

--
Gleb.

Michael S. Tsirkin

author

2012-11-28 12:19 UTC #6 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 32 lines · Gleb Natapov

Quoted 31 lines · Michael S. Tsirkin
Quoted 29 lines · Gleb Natapov
Quoted 17 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSI,
from atomic context. Use kvm_set_irq_inatomic,
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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644
virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113
Quoted 9 lines
virt/kvm/assigned-dev.c +6 -0
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
 #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?

Quoted 5 lines

Quoted 3 lines
kvm_set_msi_inatomic needs a routing entry, and we don't have the routing entry at this level.
Yes, right. BTW is this interface will be used only for legacy assigned device or there will be other users too?

I think long term we should convert irqfd to this too.

Quoted 4 lines

Quoted 3 lines
Further, guest irq might not be an MSI: host MSI can cause guest intx injection I think, we need to bounce it to thread as we did earlier.
Ah, so msi in kvm_assigned_dev_msi() is about host msi?

Yes.

Quoted 2 lines

Can host be intx
but guest msi?

No.

Quoted 3 lines

You seems to not handle this case. Also injection of intx via ioapic is the same as injecting MSI. The format and the capability of irq message are essentially the same.

Absolutely. So we will be able to extend this to intx long term. The difference is in the fact that unlike msi, intx can (and does) have multiple entries per GSI.
I have not yet figured out how to report and handle failure in case one of these can be injected in atomic context, another can't. There's likely an easy way but can
be a follow up patch I think.

Quoted 68 lines

Quoted 57 lines

virt/kvm/assigned-dev.c +20 ~~-10~~

+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Quoted 8 lines

Quoted 8 lines
Quoted 5 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.

--
Gleb.

Gleb Natapov

2012-11-28 12:45 UTC #7 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 33 lines · Michael S. Tsirkin

Quoted 32 lines · Gleb Natapov
Quoted 31 lines · Michael S. Tsirkin
Quoted 29 lines · Gleb Natapov
Quoted 17 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSI,
from atomic context. Use kvm_set_irq_inatomic,
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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644
virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113
Quoted 9 lines
virt/kvm/assigned-dev.c +6 -0
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
 #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
kvm_set_msi_inatomic needs a routing entry, and we don't have the routing entry at this level.
Yes, right. BTW is this interface will be used only for legacy assigned device or there will be other users too?
I think long term we should convert irqfd to this too.

VIFO uses irqfd, no? So, why legacy device assignment needs that code to achieve parity with VFIO? Also why long term? What are the complications?

Quoted 16 lines

Quoted 4 lines
Quoted 3 lines
Further, guest irq might not be an MSI: host MSI can cause guest intx injection I think, we need to bounce it to thread as we did earlier.
Ah, so msi in kvm_assigned_dev_msi() is about host msi?
Yes.
Quoted 2 lines
Can host be intx
but guest msi?
No.
Quoted 3 lines
You seems to not handle this case. Also injection of intx via ioapic is the same as injecting MSI. The format and the capability of irq message are essentially the same.

Quoted 7 lines

Absolutely. So we will be able to extend this to intx long term. The difference is in the fact that unlike msi, intx can (and does) have multiple entries per GSI.
I have not yet figured out how to report and handle failure in case one of these can be injected in atomic context, another can't. There's likely an easy way but can
be a follow up patch I think.

I prefer to figure that out before introducing the interface. Hmm, we can get rid of vcpu loop in pic (should be very easily done by checking for kvm_apic_accept_pic_intr() during apic configuration and keeping global extint vcpu) and then sorting irq routing entries so that ioapic entry is first since only ioapic injection can fail.

Quoted 67 lines

Quoted 66 lines

Quoted 63 lines

virt/kvm/assigned-dev.c +20 ~~-10~~

+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Signature · 5 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.

Quoted 2 lines

Quoted 2 lines
--
Gleb.

--
Gleb.

Michael S. Tsirkin

author

2012-11-28 13:22 UTC #8 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 37 lines · Gleb Natapov

Quoted 36 lines · Michael S. Tsirkin
Quoted 35 lines · Gleb Natapov
Quoted 34 lines · Michael S. Tsirkin
Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSI,
from atomic context. Use kvm_set_irq_inatomic,
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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
Quoted 9 lines
virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113
Quoted 7 lines
virt/kvm/assigned-dev.c +6 -0
 #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?
kvm_set_msi_inatomic needs a routing entry, and we don't have the routing entry at this level.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Yes, right. BTW is this interface will be used only for legacy assigned device or there will be other users too?
I think long term we should convert irqfd to this too.
VIFO uses irqfd, no? So, why legacy device assignment needs that code to achieve parity with VFIO?

Clarification: there are two issues:

1. legacy assignment has bad worst case latency
this is because we bounce all ainterrupts through threads this patch fixes this
2. irqfd injects all MSIs from an atomic context
this patch does not fix this, but it can
be fixed on top of this patch

Quoted 1 line

Also why long term? What are the complications?

Nothing special. Just need to be careful with all the rcu trickery that irqfd uses.

Quoted 24 lines

Quoted 16 lines
Quoted 4 lines
Quoted 3 lines
Further, guest irq might not be an MSI: host MSI can cause guest intx injection I think, we need to bounce it to thread as we did earlier.
Ah, so msi in kvm_assigned_dev_msi() is about host msi?
Yes.
Quoted 2 lines
Can host be intx
but guest msi?
No.
Quoted 3 lines
You seems to not handle this case. Also injection of intx via ioapic is the same as injecting MSI. The format and the capability of irq message are essentially the same.
Quoted 7 lines
Absolutely. So we will be able to extend this to intx long term. The difference is in the fact that unlike msi, intx can (and does) have multiple entries per GSI.
I have not yet figured out how to report and handle failure in case one of these can be injected in atomic context, another can't. There's likely an easy way but can
be a follow up patch I think.

Quoted 1 line

I prefer to figure that out before introducing the interface.

Ow come on, it's just an internal interface, not even exported to modules. Changing it would be trivial and the implementation is very small too.

Quoted 5 lines

Hmm, we
can get rid of vcpu loop in pic (should be very easily done by checking for kvm_apic_accept_pic_intr() during apic configuration and keeping global extint vcpu) and then sorting irq routing entries so that ioapic entry is first since only ioapic injection can fail.

Yes, I think it's a good idea to remove as many vcpu loops as possible:
for example, this vcpu loop is currently hit from atomic context anyway, isn't it?

Quoted 72 lines

Quoted 67 lines

Quoted 66 lines

Quoted 63 lines

virt/kvm/assigned-dev.c +20 ~~-10~~

+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Signature · 5 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.

Quoted 2 lines

Quoted 2 lines
--
Gleb.

--
Gleb.

Gleb Natapov

2012-11-28 13:38 UTC #9 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 46 lines · Michael S. Tsirkin

Quoted 37 lines · Gleb Natapov
Quoted 36 lines · Michael S. Tsirkin
Quoted 35 lines · Gleb Natapov
Quoted 34 lines · Michael S. Tsirkin
Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSI,
from atomic context. Use kvm_set_irq_inatomic,
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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
Quoted 9 lines
virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113
Quoted 7 lines
virt/kvm/assigned-dev.c +6 -0
 #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?
kvm_set_msi_inatomic needs a routing entry, and we don't have the routing entry at this level.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Yes, right. BTW is this interface will be used only for legacy assigned device or there will be other users too?
I think long term we should convert irqfd to this too.
VIFO uses irqfd, no? So, why legacy device assignment needs that code to achieve parity with VFIO?

Quoted 9 lines

Clarification: there are two issues:
1. legacy assignment has bad worst case latency
this is because we bounce all ainterrupts through threads this patch fixes this
2. irqfd injects all MSIs from an atomic context
this patch does not fix this, but it can
be fixed on top of this patch

Thanks for clarification.

Quoted 29 lines

Quoted 1 line
Also why long term? What are the complications?
Nothing special. Just need to be careful with all the rcu trickery that irqfd uses.
Quoted 24 lines
Quoted 23 lines
Quoted 4 lines
Quoted 3 lines
Further, guest irq might not be an MSI: host MSI can cause guest intx injection I think, we need to bounce it to thread as we did earlier.
Ah, so msi in kvm_assigned_dev_msi() is about host msi?
Yes.
Quoted 2 lines
Can host be intx
but guest msi?
No.
Quoted 3 lines
You seems to not handle this case. Also injection of intx via ioapic is the same as injecting MSI. The format and the capability of irq message are essentially the same.
Absolutely. So we will be able to extend this to intx long term. The difference is in the fact that unlike msi, intx can (and does) have multiple entries per GSI.
I have not yet figured out how to report and handle failure in case one of these can be injected in atomic context, another can't. There's likely an easy way but can
be a follow up patch I think.

Quoted 6 lines

Quoted 1 line
I prefer to figure that out before introducing the interface.
Ow come on, it's just an internal interface, not even exported to modules. Changing it would be trivial and the implementation is very small too.

The question is if it can be done at all or not. If it cannot then it does not matter that interface is internal, but fortunately looks like it is possible, so I am fine with proposed implementation for now.

Quoted 9 lines

Quoted 5 lines
Hmm, we
can get rid of vcpu loop in pic (should be very easily done by checking for kvm_apic_accept_pic_intr() during apic configuration and keeping global extint vcpu) and then sorting irq routing entries so that ioapic entry is first since only ioapic injection can fail.
Yes, I think it's a good idea to remove as many vcpu loops as possible:
for example, this vcpu loop is currently hit from atomic context anyway, isn't it?

Actually it is not. The lock is dropped just before the loop, so this loop shouldn't be the roadblock at all.

Quoted 68 lines

Quoted 67 lines

Quoted 64 lines

virt/kvm/assigned-dev.c +20 ~~-10~~

+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Signature · 5 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.

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
--
Gleb.
--
Gleb.

--
Gleb.

Michael S. Tsirkin

author

2012-11-28 15:22 UTC #10 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 103 lines · Gleb Natapov

Quoted 46 lines · Michael S. Tsirkin
Quoted 37 lines · Gleb Natapov
Quoted 36 lines · Michael S. Tsirkin
Quoted 35 lines · Gleb Natapov
Quoted 34 lines · Michael S. Tsirkin
Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSI,
from atomic context. Use kvm_set_irq_inatomic,
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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
Quoted 9 lines
virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113
Quoted 7 lines
virt/kvm/assigned-dev.c +6 -0
 #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?
kvm_set_msi_inatomic needs a routing entry, and we don't have the routing entry at this level.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Yes, right. BTW is this interface will be used only for legacy assigned device or there will be other users too?
I think long term we should convert irqfd to this too.
VIFO uses irqfd, no? So, why legacy device assignment needs that code to achieve parity with VFIO?
Quoted 9 lines
Clarification: there are two issues:
1. legacy assignment has bad worst case latency
this is because we bounce all ainterrupts through threads this patch fixes this
2. irqfd injects all MSIs from an atomic context
this patch does not fix this, but it can
be fixed on top of this patch
Thanks for clarification.
Quoted 29 lines
Quoted 1 line
Also why long term? What are the complications?
Nothing special. Just need to be careful with all the rcu trickery that irqfd uses.
Quoted 24 lines
Quoted 23 lines
Quoted 4 lines
Quoted 3 lines
Further, guest irq might not be an MSI: host MSI can cause guest intx injection I think, we need to bounce it to thread as we did earlier.
Ah, so msi in kvm_assigned_dev_msi() is about host msi?
Yes.
Quoted 2 lines
Can host be intx
but guest msi?
No.
Quoted 3 lines
You seems to not handle this case. Also injection of intx via ioapic is the same as injecting MSI. The format and the capability of irq message are essentially the same.
Absolutely. So we will be able to extend this to intx long term. The difference is in the fact that unlike msi, intx can (and does) have multiple entries per GSI.
I have not yet figured out how to report and handle failure in case one of these can be injected in atomic context, another can't. There's likely an easy way but can
be a follow up patch I think.
Quoted 6 lines
Quoted 1 line
I prefer to figure that out before introducing the interface.
Ow come on, it's just an internal interface, not even exported to modules. Changing it would be trivial and the implementation is very small too.
The question is if it can be done at all or not. If it cannot then it does not matter that interface is internal, but fortunately looks like it is possible, so I am fine with proposed implementation for now.
Quoted 6 lines
Quoted 5 lines
Hmm, we
can get rid of vcpu loop in pic (should be very easily done by checking for kvm_apic_accept_pic_intr() during apic configuration and keeping global extint vcpu) and then sorting irq routing entries so that ioapic entry is first since only ioapic injection can fail.

Quoted 5 lines

Quoted 3 lines
Yes, I think it's a good idea to remove as many vcpu loops as possible:
for example, this vcpu loop is currently hit from atomic context anyway, isn't it?
Actually it is not. The lock is dropped just before the loop, so this loop shouldn't be the roadblock at all.

Hmm you are saying PIC injections in atomic context always succeeds?

Quoted 76 lines

Quoted 68 lines

Quoted 67 lines

Quoted 64 lines

virt/kvm/assigned-dev.c +20 ~~-10~~

+	return unlikely(ret == -EWOULDBLOCK) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
+}
+
 static irqreturn_t kvm_assigned_dev_thread_msi(int irq, void *dev_id)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev = dev_id;

@@ -117,6 +126,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,11 +360,6 @@ 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)
 {

@@ -363,11 +384,6 @@ 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)
 {

Signature · 5 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.

Quoted 5 lines

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
--
Gleb.
--
Gleb.

--
Gleb.

Gleb Natapov

2012-11-28 15:25 UTC #11 lore raw

Re: [PATCHv4 2/2] kvm: deliver msi interrupts from irq handler

Quoted 104 lines · Michael S. Tsirkin

Quoted 103 lines · Gleb Natapov
Quoted 46 lines · Michael S. Tsirkin
Quoted 37 lines · Gleb Natapov
Quoted 36 lines · Michael S. Tsirkin
Quoted 35 lines · Gleb Natapov
Quoted 34 lines · Michael S. Tsirkin
Quoted 20 lines · Gleb Natapov
Quoted 19 lines · Michael S. Tsirkin
We can deliver certain interrupts, notably MSI, from atomic context. Use kvm_set_irq_inatomic, 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]>
---
 virt/kvm/assigned-dev.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)
virt/kvm/assigned-dev.c +0 -0
index 23a41a9..3642239 100644
@@ -105,6 +105,15 @@ static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id)
 }
 
Quoted 9 lines
virt/kvm/assigned-dev.c static irqreturn_t kvm_assigned_dev_thread_intx(int irq, void *dev_id) · line 113
Quoted 7 lines
virt/kvm/assigned-dev.c +6 -0
 #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);
Why not use kvm_set_msi_inatomic() and drop kvm_set_irq_inatomic() from previous patch?
kvm_set_msi_inatomic needs a routing entry, and we don't have the routing entry at this level.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Yes, right. BTW is this interface will be used only for legacy assigned device or there will be other users too?
I think long term we should convert irqfd to this too.
VIFO uses irqfd, no? So, why legacy device assignment needs that code to achieve parity with VFIO?
Quoted 9 lines
Clarification: there are two issues:
1. legacy assignment has bad worst case latency
this is because we bounce all ainterrupts through threads this patch fixes this
2. irqfd injects all MSIs from an atomic context
this patch does not fix this, but it can
be fixed on top of this patch
Thanks for clarification.
Quoted 29 lines
Quoted 1 line
Also why long term? What are the complications?
Nothing special. Just need to be careful with all the rcu trickery that irqfd uses.
Quoted 24 lines
Quoted 23 lines
Quoted 4 lines
Quoted 3 lines
Further, guest irq might not be an MSI: host MSI can cause guest intx injection I think, we need to bounce it to thread as we did earlier.
Ah, so msi in kvm_assigned_dev_msi() is about host msi?
Yes.
Quoted 2 lines
Can host be intx
but guest msi?
No.
Quoted 3 lines
You seems to not handle this case. Also injection of intx via ioapic is the same as injecting MSI. The format and the capability of irq message are essentially the same.
Absolutely. So we will be able to extend this to intx long term. The difference is in the fact that unlike msi, intx can (and does) have multiple entries per GSI.
I have not yet figured out how to report and handle failure in case one of these can be injected in atomic context, another can't. There's likely an easy way but can
be a follow up patch I think.
Quoted 6 lines
Quoted 1 line
I prefer to figure that out before introducing the interface.
Ow come on, it's just an internal interface, not even exported to modules. Changing it would be trivial and the implementation is very small too.
The question is if it can be done at all or not. If it cannot then it does not matter that interface is internal, but fortunately looks like it is possible, so I am fine with proposed implementation for now.
Quoted 6 lines
Quoted 5 lines
Hmm, we
can get rid of vcpu loop in pic (should be very easily done by checking for kvm_apic_accept_pic_intr() during apic configuration and keeping global extint vcpu) and then sorting irq routing entries so that ioapic entry is first since only ioapic injection can fail.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Yes, I think it's a good idea to remove as many vcpu loops as possible:
for example, this vcpu loop is currently hit from atomic context anyway, isn't it?
Actually it is not. The lock is dropped just before the loop, so this loop shouldn't be the roadblock at all.
Hmm you are saying PIC injections in atomic context always succeeds?

No, I am saying vcpu loop is not hit from atomic context currently.

--
Gleb.

Michael S. Tsirkin

author

2012-11-21 19:23 UTC #12 lore raw

In reply to Michael S. Tsirkin

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 with kvm_set_irq: passing flags to it looks too ugly. Added a comment.
Jan, you said you can test this?

I have tested this with some networking workloads and this patchset seems to work fine.
My setup isn't a good fit for benchmarking device assignment though.
Alex, could you pls verifyu that this solves the latency issue that you sometimes observe? With this patchset device assignment latency should be as fast as vfio.

Quoted 12 lines

Michael S. Tsirkin (2):
kvm: add kvm_set_irq_inatomic
kvm: deliver msi interrupts from irq handler
 include/linux/kvm_host.h |  1 +
 virt/kvm/assigned-dev.c  | 36 +++++++++++++++------
 virt/kvm/irq_comm.c      | 83 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 98 insertions(+), 22 deletions(-)
Signature · 1 line
MST

Alex Williamson

2012-11-28 04:34 UTC #13 lore raw

Quoted 14 lines · Michael S. Tsirkin

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 with kvm_set_irq: passing flags to it looks too ugly. Added a comment.
Jan, you said you can test this?

Quoted 8 lines

I have tested this with some networking workloads and this patchset seems to work fine.
My setup isn't a good fit for benchmarking device assignment though.
Alex, could you pls verifyu that this solves the latency issue that you sometimes observe? With this patchset device assignment latency should be as fast as vfio.

Yep, that seems to cover the gap. My environment is too noisy to declare an absolute winner, but pci-assign and vfio-pci are now very, very similar under netperf TCP_RR with these patches. Thanks,

Alex

Michael S. Tsirkin

author Tested-by Alex Williamson

2012-11-28 11:17 UTC #14 lore raw

Quoted 24 lines · Alex Williamson

Quoted 14 lines · Michael S. Tsirkin
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 with kvm_set_irq: passing flags to it looks too ugly. Added a comment.
Jan, you said you can test this?
Quoted 8 lines
I have tested this with some networking workloads and this patchset seems to work fine.
My setup isn't a good fit for benchmarking device assignment though.
Alex, could you pls verifyu that this solves the latency issue that you sometimes observe? With this patchset device assignment latency should be as fast as vfio.

Quoted 5 lines

Yep, that seems to cover the gap. My environment is too noisy to declare an absolute winner, but pci-assign and vfio-pci are now very, very similar under netperf TCP_RR with these patches. Thanks,
Alex

Thanks very much for the report.
Gleb, Marcelo, ACK?
Also please add

Tested-by Alex Williamson <[email protected]>

Signature · 1 line

MST

Gleb Natapov

Applied

2012-12-05 13:12 UTC #15 lore raw

In reply to Michael S. Tsirkin

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 10 lines

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?

Applied to queue. Thanks.

Quoted 4 lines

Michael S. Tsirkin (2):
kvm: add kvm_set_irq_inatomic
kvm: deliver msi interrupts from irq handler

Quoted 11 lines

 include/linux/kvm_host.h |  1 +
 virt/kvm/assigned-dev.c  | 36 +++++++++++++++------
 virt/kvm/irq_comm.c      | 83 +++++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 98 insertions(+), 22 deletions(-)

Signature · 5 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.