Skip to contentSkip to conversation

Enable MSI for KVM

PATCH Superseded Sheng Yang 3 participants 25 messages Nov 19, 2008 – Nov 25, 2008

Subsystems (2)
x86 KVM

v5 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Sheng Yang

author
#0 lore raw

v5 cover Enable MSI for KVM

Hi

Here is v5 for MSI on assigned devices, sorry for the delay. Due to the bad weather, I got a badly cold recently, indeed feeling bad... :(

v4->v5

Addressed all the comments from Avi. I would work on the generic MSI solution later as well. The most important one is a new modules paramter "msi2intx" can disable MSI2INTx convert if necessary, which is prepare for someone ran into trouble because of this.

The userspace patch would follow soon, I still need sometime to clean up current device assignment userspace.

Comments are welcome!

Thanks!
--
regards
Yang, Sheng

author
#1 lore raw

PATCH v5 1/11 KVM: Move ack notifier register and IRQ sourcd ID request

Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)
virt/kvm/kvm_main.c +19 -11
index 4727c08..8966fd1 100644
@@ -192,16 +192,31 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 		return -EINVAL;
 	}
 
-	if (match->irq_requested) {
+	if (!match->irq_requested) {
+		INIT_WORK(&match->interrupt_work,
+				kvm_assigned_dev_interrupt_work_handler);
+		if (irqchip_in_kernel(kvm)) {
+			/* Register ack nofitier */
+			match->ack_notifier.gsi = -1;
+			match->ack_notifier.irq_acked =
+					kvm_assigned_dev_ack_irq;
+			kvm_register_irq_ack_notifier(kvm,
+					&match->ack_notifier);
+
+			/* Request IRQ source ID */
+			r = kvm_request_irq_source_id(kvm);
+			if (r < 0)
+				goto out_release;
+			else
+				match->irq_source_id = r;
+		}
+	} else {
 		match->guest_irq = assigned_irq->guest_irq;
 		match->ack_notifier.gsi = assigned_irq->guest_irq;
 		mutex_unlock(&kvm->lock);
 		return 0;
 	}
 
-	INIT_WORK(&match->interrupt_work,
-		  kvm_assigned_dev_interrupt_work_handler);
-
 	if (irqchip_in_kernel(kvm)) {
 		if (!capable(CAP_SYS_RAWIO)) {
 			r = -EPERM;

@@ -214,13 +229,6 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 			match->host_irq = match->dev->irq;
 		match->guest_irq = assigned_irq->guest_irq;
 		match->ack_notifier.gsi = assigned_irq->guest_irq;
-		match->ack_notifier.irq_acked = kvm_assigned_dev_ack_irq;
-		kvm_register_irq_ack_notifier(kvm, &match->ack_notifier);
-		r = kvm_request_irq_source_id(kvm);
-		if (r < 0)
-			goto out_release;
-		else
-			match->irq_source_id = r;
 
 		/* Even though this is PCI, we don't want to use shared
 		 * interrupts. Sharing host devices with guest-assigned devices
Signature · 1 line
1.5.4.5
#2 lore raw

Re: [PATCH 01/11] KVM: Move ack notifier register and IRQ sourcd ID request

Quoted 5 lines · Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)

Why is this needed? comment for the changelog.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#3 lore raw

PATCH v5 2/11 KVM: Separate update irq to a single function

In reply to Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |   68 ++++++++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 30 deletions(-)
virt/kvm/kvm_main.c +38 -30
index 8966fd1..ef2f03c 100644
@@ -176,6 +176,41 @@ void kvm_free_all_assigned_devices(struct kvm *kvm)
 	}
 }
 
+static int assigned_device_update_intx(struct kvm *kvm,
+			struct kvm_assigned_dev_kernel *adev,
+			struct kvm_assigned_irq *airq)
+{
+	if (adev->irq_requested) {
+		adev->guest_irq = airq->guest_irq;
+		adev->ack_notifier.gsi = airq->guest_irq;
+		return 0;
+	}
+
+	if (irqchip_in_kernel(kvm)) {
+		if (!capable(CAP_SYS_RAWIO))
+			return -EPERM;
+
+		if (airq->host_irq)
+			adev->host_irq = airq->host_irq;
+		else
+			adev->host_irq = adev->dev->irq;
+		adev->guest_irq = airq->guest_irq;
+		adev->ack_notifier.gsi = airq->guest_irq;
+
+		/* Even though this is PCI, we don't want to use shared
+		 * interrupts. Sharing host devices with guest-assigned devices
+		 * on the same interrupt line is not a happy situation: there
+		 * are going to be long delays in accepting, acking, etc.
+		 */
+		if (request_irq(adev->host_irq, kvm_assigned_dev_intr,
+				0, "kvm_assigned_intx_device", (void *)adev))
+			return -EIO;
+	}
+
+	adev->irq_requested = true;
+	return 0;
+}
+
 static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 				   struct kvm_assigned_irq
 				   *assigned_irq)

@@ -210,39 +245,12 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 			else
 				match->irq_source_id = r;
 		}
-	} else {
-		match->guest_irq = assigned_irq->guest_irq;
-		match->ack_notifier.gsi = assigned_irq->guest_irq;
-		mutex_unlock(&kvm->lock);
-		return 0;
 	}
 
-	if (irqchip_in_kernel(kvm)) {
-		if (!capable(CAP_SYS_RAWIO)) {
-			r = -EPERM;
-			goto out_release;
-		}
-
-		if (assigned_irq->host_irq)
-			match->host_irq = assigned_irq->host_irq;
-		else
-			match->host_irq = match->dev->irq;
-		match->guest_irq = assigned_irq->guest_irq;
-		match->ack_notifier.gsi = assigned_irq->guest_irq;
-
-		/* Even though this is PCI, we don't want to use shared
-		 * interrupts. Sharing host devices with guest-assigned devices
-		 * on the same interrupt line is not a happy situation: there
-		 * are going to be long delays in accepting, acking, etc.
-		 */
-		if (request_irq(match->host_irq, kvm_assigned_dev_intr, 0,
-				"kvm_assigned_device", (void *)match)) {
-			r = -EIO;
-			goto out_release;
-		}
-	}
+	r = assigned_device_update_intx(kvm, match, assigned_irq);
+	if (r)
+		goto out_release;
 
-	match->irq_requested = true;
 	mutex_unlock(&kvm->lock);
 	return r;
 out_release:
Signature · 1 line
1.5.4.5
#4 lore raw

Re: [PATCH 02/11] KVM: Separate update irq to a single function

Quoted 2 lines · Sheng Yang
Signed-off-by Sheng Yang <[email protected]>

Again, missing explanation in changelog comment.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#5 lore raw

PATCH v5 3/11 KVM: Replace irq_requested with more generic irq_requested_type

In reply to Sheng Yang

Separate guest irq type and host irq type, for we can support guest using INTx with host using MSI (but not opposite combination).

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm_host.h |    4 +++-
 virt/kvm/kvm_main.c      |    9 +++++----
 2 files changed, 8 insertions(+), 5 deletions(-)
include/linux/kvm_host.h +3 -1
index 3a0fb77..c3d4b96 100644
@@ -307,7 +307,9 @@ struct kvm_assigned_dev_kernel {
 	int host_devfn;
 	int host_irq;
 	int guest_irq;
-	int irq_requested;
+#define KVM_ASSIGNED_DEV_GUEST_INTX	(1 << 0)
+#define KVM_ASSIGNED_DEV_HOST_INTX	(1 << 8)
+	unsigned long irq_requested_type;
 	int irq_source_id;
 	struct pci_dev *dev;
 	struct kvm *kvm;
virt/kvm/kvm_main.c +5 -4
index ef2f03c..638de47 100644
@@ -140,7 +140,7 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 				     struct kvm_assigned_dev_kernel
 				     *assigned_dev)
 {
-	if (irqchip_in_kernel(kvm) && assigned_dev->irq_requested)
+	if (irqchip_in_kernel(kvm) && assigned_dev->irq_requested_type)
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
 
 	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);

@@ -180,7 +180,7 @@ static int assigned_device_update_intx(struct kvm *kvm,
 			struct kvm_assigned_dev_kernel *adev,
 			struct kvm_assigned_irq *airq)
 {
-	if (adev->irq_requested) {
+	if (adev->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_INTX) {
 		adev->guest_irq = airq->guest_irq;
 		adev->ack_notifier.gsi = airq->guest_irq;
 		return 0;

@@ -207,7 +207,8 @@ static int assigned_device_update_intx(struct kvm *kvm,
 			return -EIO;
 	}
 
-	adev->irq_requested = true;
+	adev->irq_requested_type = KVM_ASSIGNED_DEV_GUEST_INTX |
+				   KVM_ASSIGNED_DEV_HOST_INTX;
 	return 0;
 }
 

@@ -227,7 +228,7 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 		return -EINVAL;
 	}
 
-	if (!match->irq_requested) {
+	if (!match->irq_requested_type) {
 		INIT_WORK(&match->interrupt_work,
 				kvm_assigned_dev_interrupt_work_handler);
 		if (irqchip_in_kernel(kvm)) {
Signature · 1 line
1.5.4.5
#6 lore raw

Re: [PATCH 03/11] KVM: Replace irq_requested with more generic irq_requested_type

Quoted 14 lines · Sheng Yang

Separate guest irq type and host irq type, for we can support guest using INTx with host using MSI (but not opposite combination).

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm_host.h |    4 +++-
 virt/kvm/kvm_main.c      |    9 +++++----
 2 files changed, 8 insertions(+), 5 deletions(-)
include/linux/kvm_host.h +0 -0
index 3a0fb77..c3d4b96 100644

include/linux/kvm_host.h struct kvm_assigned_dev_kernel { · line 314

Quoted 9 lines
include/linux/kvm_host.h +3 -1
@@ -307,7 +307,9 @@ struct kvm_assigned_dev_kernel {
 	int host_devfn;
 	int host_irq;
 	int guest_irq;
-	int irq_requested;
+#define KVM_ASSIGNED_DEV_GUEST_INTX	(1 << 0)
+#define KVM_ASSIGNED_DEV_HOST_INTX	(1 << 8)
+	unsigned long irq_requested_type;
 	int irq_source_id;
 	struct pci_dev *dev

Any particular reason the bits were not assigned sequentially?

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#7 lore raw

PATCH v5 4/11 KVM: Clean up assigned_device_update_irq

In reply to Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)
virt/kvm/kvm_main.c +4 -6
index 638de47..2089f8b 100644
@@ -180,11 +180,11 @@ static int assigned_device_update_intx(struct kvm *kvm,
 			struct kvm_assigned_dev_kernel *adev,
 			struct kvm_assigned_irq *airq)
 {
-	if (adev->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_INTX) {
-		adev->guest_irq = airq->guest_irq;
-		adev->ack_notifier.gsi = airq->guest_irq;
+	adev->guest_irq = airq->guest_irq;
+	adev->ack_notifier.gsi = airq->guest_irq;
+
+	if (adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_INTX)
 		return 0;
-	}
 
 	if (irqchip_in_kernel(kvm)) {
 		if (!capable(CAP_SYS_RAWIO))

@@ -194,8 +194,6 @@ static int assigned_device_update_intx(struct kvm *kvm,
 			adev->host_irq = airq->host_irq;
 		else
 			adev->host_irq = adev->dev->irq;
-		adev->guest_irq = airq->guest_irq;
-		adev->ack_notifier.gsi = airq->guest_irq;
 
 		/* Even though this is PCI, we don't want to use shared
 		 * interrupts. Sharing host devices with guest-assigned devices
Signature · 1 line
1.5.4.5
author
#8 lore raw

PATCH v5 5/11 KVM: Add fields for MSI device assignment

In reply to Sheng Yang

Prepared for kvm_arch_assigned_device_msi_dispatch().

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm.h      |    7 +++++++
 include/linux/kvm_host.h |    4 ++++
 2 files changed, 11 insertions(+), 0 deletions(-)
include/linux/kvm.h +7 -0
index 44fd7fa..bb283c3 100644
@@ -507,10 +507,17 @@ struct kvm_assigned_irq {
 	__u32 guest_irq;
 	__u32 flags;
 	union {
+		struct {
+			__u32 addr_lo;
+			__u32 addr_hi;
+			__u32 data;
+		} guest_msi;
 		__u32 reserved[12];
 	};
 };
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 
+#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
+
 #endif
include/linux/kvm_host.h +4 -0
index c3d4b96..8091a4d 100644
@@ -16,6 +16,7 @@
 #include <linux/mm.h>
 #include <linux/preempt.h>
 #include <linux/marker.h>
+#include <linux/msi.h>
 #include <asm/signal.h>
 
 #include <linux/kvm.h>

@@ -307,8 +308,11 @@ struct kvm_assigned_dev_kernel {
 	int host_devfn;
 	int host_irq;
 	int guest_irq;
+	struct msi_msg guest_msi;
 #define KVM_ASSIGNED_DEV_GUEST_INTX	(1 << 0)
+#define KVM_ASSIGNED_DEV_GUEST_MSI	(1 << 1)
 #define KVM_ASSIGNED_DEV_HOST_INTX	(1 << 8)
+#define KVM_ASSIGNED_DEV_HOST_MSI	(1 << 9)
 	unsigned long irq_requested_type;
 	int irq_source_id;
 	struct pci_dev *dev;
Signature · 1 line
1.5.4.5
#9 lore raw

Re: [PATCH 05/11] KVM: Add fields for MSI device assignment

include/linux/kvm.h struct kvm_assigned_irq { · line 513

Quoted 10 lines · Sheng Yang

Prepared for kvm_arch_assigned_device_msi_dispatch().

include/linux/kvm.h +3 -0
@@ -507,10 +507,17 @@ struct kvm_assigned_irq {
 	__u32 guest_irq;
 	__u32 flags;
 	union {
+		struct {
+			__u32 addr_lo;
+			__u32 addr_hi;

__u64 addr;

?

Quoted 5 lines
include/linux/kvm.h +1 -0
+			__u32 data;

include/linux/kvm.h struct kvm_assigned_dev_kernel { · line 316

Quoted 9 lines
include/linux/kvm.h +3 -0
@@ -307,8 +308,11 @@ struct kvm_assigned_dev_kernel {
 	int host_devfn;
 	int host_irq;
 	int guest_irq;
+	struct msi_msg guest_msi;
 #define KVM_ASSIGNED_DEV_GUEST_INTX	(1 << 0)
+#define KVM_ASSIGNED_DEV_GUEST_MSI	(1 << 1)
 #define KVM_ASSIGNED_DEV_HOST_INTX	(1 << 8)
+#define KVM_ASSIGNED_DEV_HOST_MSI	(1 << 9)

Okay, I see the reason for non sequential assignment.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#10 lore raw

Re: [PATCH 05/11] KVM: Add fields for MSI device assignment

Quoted 13 lines · On Sunday 23 November 2008 18:10:47 Avi Kivity wrote:

include/linux/kvm.h struct kvm_assigned_irq { · line 512

Quoted 9 lines · Sheng Yang

Prepared for kvm_arch_assigned_device_msi_dispatch().

include/linux/kvm.h +3 -0
@@ -507,10 +507,17 @@ struct kvm_assigned_irq {
 	__u32 guest_irq;
 	__u32 flags;
 	union {
+		struct {
+			__u32 addr_lo;
+			__u32 addr_hi;

__u64 addr;

Quoted 1 line

?

Here I followed the spec that distinguish the "Message Address" and "Message Upper address". And the native Linux structure:

struct msi_msg {
u32 address_lo; /* low 32 bits of msi message address */
u32 address_hi; /* high 32 bits of msi message address */
u32 data; /* 16 bits of msi message data */
};

For now, we needn't care about address_hi. I can only see address_hi used in hypertransport part... So I think keep it independence here is OK.

(In fact, PCI spec defined message data length is u64, but as you see, now msi_msg for whole Linux only have u32...)

Signature · 2 lines
regards
Yang, Sheng
Quoted 16 lines
Quoted 6 lines
include/linux/kvm.h +1 -0
+			__u32 data;

include/linux/kvm.h struct kvm_assigned_dev_kernel { · line 315

Quoted 8 lines
include/linux/kvm.h +3 -0
@@ -307,8 +308,11 @@ struct kvm_assigned_dev_kernel {
 	int host_devfn;
 	int host_irq;
 	int guest_irq;
+	struct msi_msg guest_msi;
 #define KVM_ASSIGNED_DEV_GUEST_INTX	(1 << 0)
+#define KVM_ASSIGNED_DEV_GUEST_MSI	(1 << 1)
 #define KVM_ASSIGNED_DEV_HOST_INTX	(1 << 8)
+#define KVM_ASSIGNED_DEV_HOST_MSI	(1 << 9)

Okay, I see the reason for non sequential assignment.

#11 lore raw

Re: [PATCH 05/11] KVM: Add fields for MSI device assignment

Quoted 12 lines · Sheng Yang
Quoted 9 lines

include/linux/kvm.h struct kvm_assigned_irq { · line 512

Quoted 5 lines
include/linux/kvm.h +3 -0
 	union {
+		struct {
+			__u32 addr_lo;
+			__u32 addr_hi;

__u64 addr;

?

Here I followed the spec that distinguish the "Message Address" and "Message Upper address". And the native Linux structure:

Quoted 9 lines

struct msi_msg {
u32 address_lo; /* low 32 bits of msi message address */
u32 address_hi; /* high 32 bits of msi message address */
u32 data; /* 16 bits of msi message data */
};

For now, we needn't care about address_hi. I can only see address_hi used in hypertransport part... So I think keep it independence here is OK.

Fair enough, documentation wins.

Quoted 2 lines

(In fact, PCI spec defined message data length is u64, but as you see, now msi_msg for whole Linux only have u32...)

Well in that case please define data as __u64, so we don't have surprises later on.

Signature · 1 line
error compiling committee.c: too many arguments to function
#12 lore raw

Re: [PATCH 05/11] KVM: Add fields for MSI device assignment

Quoted 5 lines · Avi Kivity
Quoted 2 lines

(In fact, PCI spec defined message data length is u64, but as you see, now msi_msg for whole Linux only have u32...)

Well in that case please define data as __u64, so we don't have surprises later on.

btw, if you agree with this, don't resend; I'll edit the patch.

Signature · 1 line
error compiling committee.c: too many arguments to function
#13 lore raw

Re: [PATCH 05/11] KVM: Add fields for MSI device assignment

Quoted 10 lines · Avi Kivity
Quoted 7 lines · Avi Kivity
Quoted 3 lines

(In fact, PCI spec defined message data length is u64, but as you see, now msi_msg for whole Linux only have u32...)

Well in that case please define data as __u64, so we don't have surprises later on.

btw, if you agree with this, don't resend; I'll edit the patch.

Sorry, I found I made a mistake. Intel SDM 3A misleaded me... The PCI spec defined message data as u16(!!), but Intel SDM 3A got a figure(Figure 8-25) that reserved all bits up to bit 63, so I thought message data got u64 long...(At last moment, I remembered the figure on PCI spec and felt something strange...)

Now I think the latest patchset(v6) is OK to apply. --
regards
Yang, Sheng

Quoted 8 lines
Signature · 7 lines
-- 
error compiling committee.c: too many arguments to function

--
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
author
#14 lore raw

PATCH v5 6/11 KVM: Export ioapic_get_delivery_bitmask

In reply to Sheng Yang

It would be used for MSI in device assignment, for MSI dispatch.

Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/ioapic.c |    7 ++++---
 virt/kvm/ioapic.h |    2 ++
 2 files changed, 6 insertions(+), 3 deletions(-)
virt/kvm/ioapic.c +4 -3
index c8f939c..23b81cf 100644
@@ -153,8 +153,8 @@ static void ioapic_inj_nmi(struct kvm_vcpu *vcpu)
 	kvm_vcpu_kick(vcpu);
 }
 
-static u32 ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
-				       u8 dest_mode)
+u32 kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
+				    u8 dest_mode)
 {
 	u32 mask = 0;
 	int i;

@@ -208,7 +208,8 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, int irq)
 		     "vector=%x trig_mode=%x\n",
 		     dest, dest_mode, delivery_mode, vector, trig_mode);
 
-	deliver_bitmask = ioapic_get_delivery_bitmask(ioapic, dest, dest_mode);
+	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic, dest,
+							  dest_mode);
 	if (!deliver_bitmask) {
 		ioapic_debug("no target on destination\n");
 		return 0;
virt/kvm/ioapic.h +2 -0
index cd7ae76..49c9581 100644
@@ -85,5 +85,7 @@ void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode);
 int kvm_ioapic_init(struct kvm *kvm);
 void kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level);
 void kvm_ioapic_reset(struct kvm_ioapic *ioapic);
+u32 kvm_ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
+				u8 dest_mode);
 
 #endif
Signature · 1 line
1.5.4.5
author
#15 lore raw

PATCH v5 7/11 x86: Rename MSI macro name

In reply to Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/include/asm/msidef.h |    4 ++--
 arch/x86/kernel/io_apic.c     |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
arch/x86/include/asm/msidef.h +2 -2
index 6706b30..988cb27 100644
@@ -15,8 +15,8 @@
 					 MSI_DATA_VECTOR_MASK)
 
 #define MSI_DATA_DELIVERY_MODE_SHIFT	8
-#define  MSI_DATA_DELIVERY_FIXED	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
-#define  MSI_DATA_DELIVERY_LOWPRI	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)
+#define  MSI_DATA_DELIVERY_FIXED_BIT	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
+#define  MSI_DATA_DELIVERY_LOWPRI_BIT	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)
 
 #define MSI_DATA_LEVEL_SHIFT		14
 #define	 MSI_DATA_LEVEL_DEASSERT	(0 << MSI_DATA_LEVEL_SHIFT)
arch/x86/kernel/io_apic.c +2 -2
index 7a3f202..9cf604d 100644
@@ -3005,8 +3005,8 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq, struct msi_ms
 			MSI_DATA_TRIGGER_EDGE |
 			MSI_DATA_LEVEL_ASSERT |
 			((INT_DELIVERY_MODE != dest_LowestPrio) ?
-				MSI_DATA_DELIVERY_FIXED:
-				MSI_DATA_DELIVERY_LOWPRI) |
+				MSI_DATA_DELIVERY_FIXED_BIT:
+				MSI_DATA_DELIVERY_LOWPRI_BIT) |
 			MSI_DATA_VECTOR(cfg->vector);
 	}
 	return err;
Signature · 1 line
1.5.4.5
#16 lore raw

Re: [PATCH 07/11] x86: Rename MSI macro name

Quoted 11 lines · Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/include/asm/msidef.h |    4 ++--
 arch/x86/kernel/io_apic.c     |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
arch/x86/include/asm/msidef.h +0 -0
index 6706b30..988cb27 100644

arch/x86/include/asm/msidef.h line 20

Quoted 8 lines
arch/x86/include/asm/msidef.h +2 -2
@@ -15,8 +15,8 @@
 					 MSI_DATA_VECTOR_MASK)
 
 #define MSI_DATA_DELIVERY_MODE_SHIFT	8
-#define  MSI_DATA_DELIVERY_FIXED	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
-#define  MSI_DATA_DELIVERY_LOWPRI	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)
+#define  MSI_DATA_DELIVERY_FIXED_BIT	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
+#define  MSI_DATA_DELIVERY_LOWPRI_BIT	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)

These are usually named _MASK, not _BIT. But I recommend dropping this patch, it can only cause conflicts with other development.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#17 lore raw

Re: [PATCH 07/11] x86: Rename MSI macro name

Quoted 14 lines · On Sunday 23 November 2008 18:12:54 Avi Kivity wrote:
Quoted 13 lines · Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/include/asm/msidef.h |    4 ++--
 arch/x86/kernel/io_apic.c     |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
(quoted diff) +0 -0

b/arch/x86/include/asm/msidef.h index 6706b30..988cb27 100644

arch/x86/include/asm/msidef.h +0 -0
@@ -15,8 +15,8 @@
 					 MSI_DATA_VECTOR_MASK)
Quoted 9 lines

arch/x86/include/asm/msidef.h

Quoted 6 lines
arch/x86/include/asm/msidef.h +2 -2
 #define MSI_DATA_DELIVERY_MODE_SHIFT	8
-#define  MSI_DATA_DELIVERY_FIXED	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
-#define  MSI_DATA_DELIVERY_LOWPRI	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)
+#define  MSI_DATA_DELIVERY_FIXED_BIT	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
+#define  MSI_DATA_DELIVERY_LOWPRI_BIT	(1 <<
 MSI_DATA_DELIVERY_MODE_SHIFT)

These are usually named _MASK, not _BIT. But I recommend dropping this patch, it can only cause conflicts with other development.

Yeah, I also don't like this. It's easily cause chaos... I will compromise on it, use IOAPIC_LOWEST_PRIORITY and IOAPIC_FIXED for now, and drop the related two patches.

Signature · 2 lines
regards
Yang, Sheng
author
#18 lore raw

PATCH v5 8/11 x86: Add MSI delivery mode value

In reply to Sheng Yang
Signed-off-by Sheng Yang <[email protected]>
---
 arch/x86/include/asm/msidef.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
arch/x86/include/asm/msidef.h +2 -0
index 988cb27..412f94e 100644
@@ -17,6 +17,8 @@
 #define MSI_DATA_DELIVERY_MODE_SHIFT	8
 #define  MSI_DATA_DELIVERY_FIXED_BIT	(0 << MSI_DATA_DELIVERY_MODE_SHIFT)
 #define  MSI_DATA_DELIVERY_LOWPRI_BIT	(1 << MSI_DATA_DELIVERY_MODE_SHIFT)
+#define  MSI_DATA_DELIVERY_FIXED	0
+#define  MSI_DATA_DELIVERY_LOWPRI	1
 
 #define MSI_DATA_LEVEL_SHIFT		14
 #define	 MSI_DATA_LEVEL_DEASSERT	(0 << MSI_DATA_LEVEL_SHIFT)
Signature · 1 line
1.5.4.5
author
#19 lore raw

PATCH v5 9/11 KVM: Add assigned_device_msi_dispatch()

In reply to Sheng Yang

The function is used to dispatch MSI to lapic according to MSI message address and message data.

Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |   54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)
virt/kvm/kvm_main.c +54 -0
index 2089f8b..c41488f 100644
@@ -47,6 +47,10 @@
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
 
+#ifdef CONFIG_X86
+#include <asm/msidef.h>
+#endif
+
 #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
 #include "coalesced_mmio.h"
 #endif

@@ -78,6 +82,56 @@ static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl,
 bool kvm_rebooting;
 
 #ifdef KVM_CAP_DEVICE_ASSIGNMENT
+
+#ifdef CONFIG_X86
+static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
+{
+	int vcpu_id;
+	struct kvm_vcpu *vcpu;
+	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
+	int dest_id = (dev->guest_msi.address_lo & MSI_ADDR_DEST_ID_MASK)
+			>> MSI_ADDR_DEST_ID_SHIFT;
+	int vector = (dev->guest_msi.data & MSI_DATA_VECTOR_MASK)
+			>> MSI_DATA_VECTOR_SHIFT;
+	int dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT,
+				(unsigned long *)&dev->guest_msi.address_lo);
+	int trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT,
+				(unsigned long *)&dev->guest_msi.data);
+	int delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
+				(unsigned long *)&dev->guest_msi.data);
+	u32 deliver_bitmask;
+
+	BUG_ON(!ioapic);
+
+	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
+				dest_id, dest_mode);
+	switch (delivery_mode) {
+	case MSI_DATA_DELIVERY_LOWPRI:
+		vcpu = kvm_get_lowest_prio_vcpu(ioapic->kvm, vector,
+				deliver_bitmask);
+		if (vcpu != NULL)
+			kvm_apic_set_irq(vcpu, vector, trig_mode);
+		else
+			printk(KERN_INFO "kvm: null lowest priority vcpu!\n");
+		break;
+	case MSI_DATA_DELIVERY_FIXED:
+		for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) {
+			if (!(deliver_bitmask & (1 << vcpu_id)))
+				continue;
+			deliver_bitmask &= ~(1 << vcpu_id);
+			vcpu = ioapic->kvm->vcpus[vcpu_id];
+			if (vcpu)
+				kvm_apic_set_irq(vcpu, vector, trig_mode);
+		}
+		break;
+	default:
+		printk(KERN_INFO "kvm: unsupported MSI delivery mode\n");
+	}
+}
+#else
+static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev) {}
+#endif
+
 static struct kvm_assigned_dev_kernel *kvm_find_assigned_dev(struct list_head *head,
 						      int assigned_dev_id)
 {
Signature · 1 line
1.5.4.5
#20 lore raw

Re: [PATCH 09/11] KVM: Add assigned_device_msi_dispatch()

Quoted 17 lines · Sheng Yang

The function is used to dispatch MSI to lapic according to MSI message address and message data.

virt/kvm/kvm_main.c +13 -0
+
+	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
+				dest_id, dest_mode);
+	switch (delivery_mode) {
+	case MSI_DATA_DELIVERY_LOWPRI:
+		vcpu = kvm_get_lowest_prio_vcpu(ioapic->kvm, vector,
+				deliver_bitmask);
+		if (vcpu != NULL)
+			kvm_apic_set_irq(vcpu, vector, trig_mode);
+		else
+			printk(KERN_INFO "kvm: null lowest priority vcpu!\n");
+		break;
+	case MSI_DATA_DELIVERY_FIXED:

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl, · line 127

Quoted 10 lines
virt/kvm/kvm_main.c +10 -0
+		for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) {
+			if (!(deliver_bitmask & (1 << vcpu_id)))
+				continue;
+			deliver_bitmask &= ~(1 << vcpu_id);
+			vcpu = ioapic->kvm->vcpus[vcpu_id];
+			if (vcpu)
+				kvm_apic_set_irq(vcpu, vector, trig_mode);
+		}
+		break;
+	default:

This duplicates the ioapic code. That's fine for now, but eventually we'll want to refactor this.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#21 lore raw

Re: [PATCH 09/11] KVM: Add assigned_device_msi_dispatch()

Quoted 22 lines · On Sunday 23 November 2008 18:22:47 Avi Kivity wrote:
Quoted 21 lines · Sheng Yang

The function is used to dispatch MSI to lapic according to MSI message address and message data.

virt/kvm/kvm_main.c +18 -0
+
+	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
+				dest_id, dest_mode);
+	switch (delivery_mode) {
+	case MSI_DATA_DELIVERY_LOWPRI:
+		vcpu = kvm_get_lowest_prio_vcpu(ioapic->kvm, vector,
+				deliver_bitmask);
+		if (vcpu != NULL)
+			kvm_apic_set_irq(vcpu, vector, trig_mode);
+		else
+			printk(KERN_INFO "kvm: null lowest priority vcpu!\n");
+		break;
+	case MSI_DATA_DELIVERY_FIXED:
+		for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) {
+			if (!(deliver_bitmask & (1 << vcpu_id)))
+				continue;
+			deliver_bitmask &= ~(1 << vcpu_id);
+			vcpu = ioapic->kvm->vcpus[vcpu_id];
Quoted 8 lines

virt/kvm/kvm_main.c static long kvm_vcpu_ioctl(struct file *file, unsigned int ioctl, · line 127

Quoted 5 lines
virt/kvm/kvm_main.c +5 -0
+			if (vcpu)
+				kvm_apic_set_irq(vcpu, vector, trig_mode);
+		}
+		break;
+	default:

This duplicates the ioapic code. That's fine for now, but eventually we'll want to refactor this.

Sure. Put on my todo list...

Signature · 2 lines
regards
Yang, Sheng
author
#22 lore raw

PATCH v5 10/11 KVM: Enable MSI for device assignment

In reply to Sheng Yang

We enable guest MSI and host MSI support in this patch. The userspace want to enable MSI should set KVM_DEV_IRQ_ASSIGN_ENABLE_MSI in the assigned_irq's flag. Function would return -ENOTTY if can't enable MSI, userspace shouldn't set MSI Enable bit when KVM_ASSIGN_IRQ return -ENOTTY with KVM_DEV_IRQ_ASSIGN_ENABLE_MSI.

Userspace can tell the support of MSI device from #ifdef KVM_CAP_DEVICE_MSI.

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm.h |    3 ++
 virt/kvm/kvm_main.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 78 insertions(+), 6 deletions(-)
include/linux/kvm.h +3 -0
index bb283c3..e7dae05 100644
@@ -392,6 +392,9 @@ struct kvm_trace_rec {
 #endif
 #define KVM_CAP_IOMMU 18
 #define KVM_CAP_NMI 19
+#if defined(CONFIG_X86)||defined(CONFIG_IA64)
+#define KVM_CAP_DEVICE_MSI 20
+#endif
 
 /*
  * ioctls for VM fds
virt/kvm/kvm_main.c +75 -6

111 lines not on this page · raw message

Signature · 1 line
1.5.4.5
#23 lore raw

Re: [PATCH 10/11] KVM: Enable MSI for device assignment

Quoted 19 lines · Sheng Yang

We enable guest MSI and host MSI support in this patch. The userspace want to enable MSI should set KVM_DEV_IRQ_ASSIGN_ENABLE_MSI in the assigned_irq's flag. Function would return -ENOTTY if can't enable MSI, userspace shouldn't set MSI Enable bit when KVM_ASSIGN_IRQ return -ENOTTY with KVM_DEV_IRQ_ASSIGN_ENABLE_MSI.

Userspace can tell the support of MSI device from #ifdef KVM_CAP_DEVICE_MSI.

Signed-off-by Sheng Yang <[email protected]>
---
 include/linux/kvm.h |    3 ++
 virt/kvm/kvm_main.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 78 insertions(+), 6 deletions(-)
include/linux/kvm.h +0 -0
index bb283c3..e7dae05 100644

include/linux/kvm.h struct kvm_trace_rec { · line 398

Quoted 7 lines
include/linux/kvm.h +3 -0
@@ -392,6 +392,9 @@ struct kvm_trace_rec {
 #endif
 #define KVM_CAP_IOMMU 18
 #define KVM_CAP_NMI 19
+#if defined(CONFIG_X86)||defined(CONFIG_IA64)
+#define KVM_CAP_DEVICE_MSI 20
+#endif

Since the code only enables x86 for now, please drop ia64.

Quoted 11 lines
virt/kvm/kvm_main.c +0 -3
index c41488f..8e0b599 100644
@@ -158,9 +158,15 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 	 * finer-grained lock, update this
 	 */
 	mutex_lock(&assigned_dev->kvm->lock);
-	kvm_set_irq(assigned_dev->kvm,
-		    assigned_dev->irq_source_id,
-		    assigned_dev->guest_irq, 1);

virt/kvm/kvm_main.c static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work) · line 170

Quoted 10 lines
virt/kvm/kvm_main.c +9 -0
+	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_INTX)
+		kvm_set_irq(assigned_dev->kvm,
+			    assigned_dev->irq_source_id,
+			    assigned_dev->guest_irq, 1);
+	else if (assigned_dev->irq_requested_type &
+				KVM_ASSIGNED_DEV_GUEST_MSI) {
+		assigned_device_msi_dispatch(assigned_dev);
+		enable_irq(assigned_dev->host_irq);
+	}

Please move this logic to kvm_set_irq(). Hmm, that's not possible right now, so we can leave it as is for now.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#24 lore raw

PATCH v5 11/11 KVM: MSI to INTx translate

In reply to Sheng Yang

Now we use MSI as default one, and translate MSI to INTx when guest need INTx rather than MSI. For legacy device, we provide support for non-sharing host IRQ.

Provide a parameter msi2intx for this method. The value is true by default.

Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |   67 ++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 51 insertions(+), 16 deletions(-)
virt/kvm/kvm_main.c +51 -16
index 8e0b599..98f145d 100644
@@ -64,6 +64,9 @@
 MODULE_AUTHOR("Qumranet");
 MODULE_LICENSE("GPL");
 
+static int msi2intx = 1;
+module_param(msi2intx, bool, 0);
+
 DEFINE_SPINLOCK(kvm_lock);
 LIST_HEAD(vm_list);
 

@@ -249,7 +252,8 @@ static int assigned_device_update_intx(struct kvm *kvm,
 		return 0;
 
 	if (irqchip_in_kernel(kvm)) {
-		if (adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) {
+		if (!msi2intx &&
+		    adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) {
 			free_irq(adev->host_irq, (void *)kvm);
 			pci_disable_msi(adev->dev);
 		}

@@ -284,21 +288,33 @@ static int assigned_device_update_msi(struct kvm *kvm,
 {
 	int r;
 
-	/* x86 don't care upper address of guest msi message addr */
-	adev->guest_msi.address_lo = airq->guest_msi.addr_lo;
-	adev->guest_msi.data = airq->guest_msi.data;
-	adev->ack_notifier.gsi = -1;
+	if (airq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSI) {
+		/* x86 don't care upper address of guest msi message addr */
+		adev->irq_requested_type |= KVM_ASSIGNED_DEV_GUEST_MSI;
+		adev->irq_requested_type &= ~KVM_ASSIGNED_DEV_GUEST_INTX;
+		adev->guest_msi.address_lo = airq->guest_msi.addr_lo;
+		adev->guest_msi.data = airq->guest_msi.data;
+		adev->ack_notifier.gsi = -1;
+	} else if (msi2intx) {
+		adev->irq_requested_type |= KVM_ASSIGNED_DEV_GUEST_INTX;
+		adev->irq_requested_type &= ~KVM_ASSIGNED_DEV_GUEST_MSI;
+		adev->guest_irq = airq->guest_irq;
+		adev->ack_notifier.gsi = airq->guest_irq;
+	}
 
 	if (adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)
 		return 0;
 
 	if (irqchip_in_kernel(kvm)) {
-		if (adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_INTX)
-			free_irq(adev->host_irq, (void *)adev);
-
-		r = pci_enable_msi(adev->dev);
-		if (r)
-			return r;
+		if (!msi2intx) {
+			if (adev->irq_requested_type &
+					KVM_ASSIGNED_DEV_HOST_INTX)
+				free_irq(adev->host_irq, (void *)adev);
+
+			r = pci_enable_msi(adev->dev);
+			if (r)
+				return r;
+		}
 
 		adev->host_irq = adev->dev->irq;
 		if (request_irq(adev->host_irq, kvm_assigned_dev_intr, 0,

@@ -306,8 +322,10 @@ static int assigned_device_update_msi(struct kvm *kvm,
 			return -EIO;
 	}
 
-	adev->irq_requested_type = KVM_ASSIGNED_DEV_GUEST_MSI |
-				   KVM_ASSIGNED_DEV_HOST_MSI;
+	if (!msi2intx)
+		adev->irq_requested_type = KVM_ASSIGNED_DEV_GUEST_MSI;
+
+	adev->irq_requested_type |= KVM_ASSIGNED_DEV_HOST_MSI;
 	return 0;
 }
 #endif

@@ -345,10 +363,19 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 				goto out_release;
 			else
 				match->irq_source_id = r;
+
+#ifdef CONFIG_X86
+			/* Determine host device irq type, we can know the
+			 * result from dev->msi_enabled */
+			if (msi2intx)
+				pci_enable_msi(match->dev);
+#endif
 		}
 	}
 
-	if (assigned_irq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSI) {
+	if ((!msi2intx &&
+	     (assigned_irq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSI)) ||
+	    (msi2intx && match->dev->msi_enabled)) {
 #ifdef CONFIG_X86
 		r = assigned_device_update_msi(kvm, match, assigned_irq);
 		if (r) {

@@ -361,8 +388,16 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 #endif
 	} else if (assigned_irq->host_irq == 0 && match->dev->irq == 0) {
 		/* Host device IRQ 0 means don't support INTx */
-		printk(KERN_WARNING "kvm: wait device to enable MSI!\n");
-		r = 0;
+		if (!msi2intx) {
+			printk(KERN_WARNING
+			       "kvm: wait device to enable MSI!\n");
+			r = 0;
+		} else {
+			printk(KERN_WARNING
+			       "kvm: failed to enable MSI device!\n");
+			r = -ENOTTY;
+			goto out_release;
+		}
 	} else {
 		/* Non-sharing INTx mode */
 		r = assigned_device_update_intx(kvm, match, assigned_irq);
Signature · 1 line
1.5.4.5