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

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

[Sheng Yang]

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   68 ++++++++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3ecc5a8..ac1d652 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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:
-- 
1.5.4.5

[PATCH 0/10][v4] Enable MSI for KVM assigned device kernel part

[Sheng Yang]

Hi, Avi

Here is v4 for MSI on assigned devices.

v3->v4

1. After discuss with Xiantao, we decide to delay MSI enabling for IA64, so
I've temporary wrapped MSI critical part with #ifdef CONFIG_X86. These are
expected to be removed soon before 2.6.28 release.

2. Fix two wrong usage of macro.

The userspace patch would follow soon.

Another significant change from v2 is, this patchset not only enable MSI for
MSI capable guests(patch 1-9), also enable MSI by default for all guests(patch
10). I implement a MSI to INTx convert in patch 10, which is a little tricky,
but don't have much trouble in theory (the only thing we are worried is about
device would use different "register" to indicate interrupt reason for INTx
mode and MSI mode, but didn't find this kind of device yet). We may need a
black/white list for device which can work in this way. This also avoid host
sharing interrupt trouble, and provide better scalability for the guest which
lacks of MSI capablility. The patch also provide non-sharing host INTx support
for legacy devices. So we suggest this can be a default solution for current
KVM device assignment support.

Comments are welcome!

--
regards
Yang, Sheng

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

[Sheng Yang]

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4f43abe..4647a7b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -190,16 +190,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;
@@ -212,13 +227,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
-- 
1.5.4.5

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

[Sheng Yang]

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   68 ++++++++++++++++++++++++++++----------------------
 1 files changed, 38 insertions(+), 30 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4647a7b..7d261b1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -174,6 +174,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)
@@ -208,39 +243,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:
-- 
1.5.4.5

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

[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 <sheng@linux.intel.com>
---
 include/linux/kvm_host.h |    4 +++-
 virt/kvm/kvm_main.c      |    9 +++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 3a0fb77..c3d4b96 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 7d261b1..ed1b243 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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);
@@ -178,7 +178,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;
@@ -205,7 +205,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;
 }
 
@@ -225,7 +226,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)) {
-- 
1.5.4.5

[PATCH 04/10] KVM: Clean up assigned_device_update_irq

[Sheng Yang]

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ed1b243..c69c108 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -178,11 +178,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))
@@ -192,8 +192,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
-- 
1.5.4.5

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

[Sheng Yang]

Prepared for kvm_arch_assigned_device_msi_dispatch().

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm.h      |    6 +++++-
 include/linux/kvm_host.h |    4 ++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 44fd7fa..8ec74b8 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -506,11 +506,15 @@ struct kvm_assigned_irq {
 	__u32 host_irq;
 	__u32 guest_irq;
 	__u32 flags;
+	__u32 guest_msi_data;
+	__u32 guest_msi_addr;
 	union {
-		__u32 reserved[12];
+		__u32 reserved[10];
 	};
 };
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 
+#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
+
 #endif
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index c3d4b96..2e9a9a8 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -307,8 +307,12 @@ struct kvm_assigned_dev_kernel {
 	int host_devfn;
 	int host_irq;
 	int guest_irq;
+	u32 guest_msi_addr;
+	u32 guest_msi_data;
 #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;
-- 
1.5.4.5

[PATCH 06/10] KVM: Export ioapic_get_delivery_bitmask

[Sheng Yang]

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

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/ioapic.c |    4 ++--
 virt/kvm/ioapic.h |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c
index c8f939c..1e0ff8c 100644
--- a/virt/kvm/ioapic.c
+++ b/virt/kvm/ioapic.c
@@ -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 ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
+				u8 dest_mode)
 {
 	u32 mask = 0;
 	int i;
diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h
index cd7ae76..42972f5 100644
--- a/virt/kvm/ioapic.h
+++ b/virt/kvm/ioapic.h
@@ -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 ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
+				u8 dest_mode);
 
 #endif
-- 
1.5.4.5

[PATCH 07/10] x86: Add MSI delivery mode value

[Sheng Yang]

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/include/asm/msidef.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/msidef.h b/arch/x86/include/asm/msidef.h
index 6706b30..32edc7f 100644
--- a/arch/x86/include/asm/msidef.h
+++ b/arch/x86/include/asm/msidef.h
@@ -17,6 +17,8 @@
 #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_VAL	0
+#define  MSI_DATA_DELIVERY_LOWPRI_VAL	1
 
 #define MSI_DATA_LEVEL_SHIFT		14
 #define	 MSI_DATA_LEVEL_DEASSERT	(0 << MSI_DATA_LEVEL_SHIFT)
-- 
1.5.4.5

[PATCH 08/10] KVM: Add assigned_device_msi_dispatch()

[Sheng Yang]

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

(Any way to replace macro GET_VAL_FROM_SHIFT()?)

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c69c108..d23f555 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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,57 @@ 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)
+{
+#define GET_VAL_FROM_SHIFT(v, s) (((v) & (1 << s)) >> s)
+	int vcpu_id;
+	struct kvm_vcpu *vcpu;
+	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
+	u8 dest_id = (dev->guest_msi_addr & MSI_ADDR_DEST_ID_MASK)
+			>> MSI_ADDR_DEST_ID_SHIFT;
+	u8 vector = (dev->guest_msi_data & MSI_DATA_VECTOR_MASK)
+			>> MSI_DATA_VECTOR_SHIFT;
+	u8 dest_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_addr,
+					  MSI_ADDR_DEST_MODE_SHIFT);
+	u8 trig_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_data,
+					  MSI_DATA_TRIGGER_SHIFT);
+	u8 delivery_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_data,
+					  MSI_DATA_DELIVERY_MODE_SHIFT);
+	u32 deliver_bitmask;
+
+	BUG_ON(!ioapic);
+
+	deliver_bitmask = ioapic_get_delivery_bitmask(ioapic,
+						      dest_id, dest_mode);
+	switch (delivery_mode) {
+	case MSI_DATA_DELIVERY_LOWPRI_VAL:
+		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_VAL:
+		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)
 {
-- 
1.5.4.5

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

[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.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 74 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d23f555..3ae8653 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -159,9 +159,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);
+	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);
+	}
 	mutex_unlock(&assigned_dev->kvm->lock);
 	kvm_put_kvm(assigned_dev->kvm);
 }
@@ -197,6 +203,8 @@ static void kvm_free_assigned_device(struct kvm *kvm,
 {
 	if (irqchip_in_kernel(kvm) && assigned_dev->irq_requested_type)
 		free_irq(assigned_dev->host_irq, (void *)assigned_dev);
+	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)
+		pci_disable_msi(assigned_dev->dev);
 
 	kvm_unregister_irq_ack_notifier(&assigned_dev->ack_notifier);
 	kvm_free_irq_source_id(kvm, assigned_dev->irq_source_id);
@@ -240,6 +248,11 @@ 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) {
+			free_irq(adev->host_irq, (void *)kvm);
+			pci_disable_msi(adev->dev);
+		}
+
 		if (!capable(CAP_SYS_RAWIO))
 			return -EPERM;
 
@@ -263,6 +276,40 @@ static int assigned_device_update_intx(struct kvm *kvm,
 	return 0;
 }
 
+#ifdef CONFIG_X86
+static int assigned_device_update_msi(struct kvm *kvm,
+			struct kvm_assigned_dev_kernel *adev,
+			struct kvm_assigned_irq *airq)
+{
+	int r;
+
+	adev->guest_msi_addr = airq->guest_msi_addr;
+	adev->guest_msi_data = airq->guest_msi_data;
+	adev->ack_notifier.gsi = -1;
+
+	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;
+
+		adev->host_irq = adev->dev->irq;
+		if (request_irq(adev->host_irq, kvm_assigned_dev_intr, 0,
+				"kvm_assigned_msi_device", (void *)adev))
+			return -EIO;
+	}
+
+	adev->irq_requested_type = KVM_ASSIGNED_DEV_GUEST_MSI |
+				   KVM_ASSIGNED_DEV_HOST_MSI;
+	return 0;
+}
+#endif
+
 static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 				   struct kvm_assigned_irq
 				   *assigned_irq)
@@ -299,9 +346,30 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 		}
 	}
 
-	r = assigned_device_update_intx(kvm, match, assigned_irq);
-	if (r)
-		goto out_release;
+	if (assigned_irq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSI) {
+#ifdef CONFIG_X86
+		r = assigned_device_update_msi(kvm, match, assigned_irq);
+		if (r) {
+			printk(KERN_WARNING "kvm: failed to enable "
+					"MSI device!\n");
+			goto out_release;
+		}
+#else
+		r = -ENOTTY;
+#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;
+	} else {
+		/* Non-sharing INTx mode */
+		r = assigned_device_update_intx(kvm, match, assigned_irq);
+		if (r) {
+			printk(KERN_WARNING "kvm: failed to enable "
+					"INTx device!\n");
+			goto out_release;
+		}
+	}
 
 	mutex_unlock(&kvm->lock);
 	return r;
-- 
1.5.4.5

[PATCH 10/10] KVM: MSI to INTx translate

[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.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 virt/kvm/kvm_main.c |   52 ++++++++++++++++++++++++++++----------------------
 1 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3ae8653..5aa975e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -241,6 +241,8 @@ static int assigned_device_update_intx(struct kvm *kvm,
 			struct kvm_assigned_dev_kernel *adev,
 			struct kvm_assigned_irq *airq)
 {
+	BUG_ON(!!adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI);
+
 	adev->guest_irq = airq->guest_irq;
 	adev->ack_notifier.gsi = airq->guest_irq;
 
@@ -248,11 +250,6 @@ 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) {
-			free_irq(adev->host_irq, (void *)kvm);
-			pci_disable_msi(adev->dev);
-		}
-
 		if (!capable(CAP_SYS_RAWIO))
 			return -EPERM;
 
@@ -281,31 +278,32 @@ static int assigned_device_update_msi(struct kvm *kvm,
 			struct kvm_assigned_dev_kernel *adev,
 			struct kvm_assigned_irq *airq)
 {
-	int r;
+	BUG_ON(!!adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_INTX);
 
-	adev->guest_msi_addr = airq->guest_msi_addr;
-	adev->guest_msi_data = airq->guest_msi_data;
-	adev->ack_notifier.gsi = -1;
+	if (airq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSI) {
+		adev->irq_requested_type |= KVM_ASSIGNED_DEV_GUEST_MSI;
+		adev->irq_requested_type &= ~KVM_ASSIGNED_DEV_GUEST_INTX;
+		adev->guest_msi_addr = airq->guest_msi_addr;
+		adev->guest_msi_data = airq->guest_msi_data;
+		adev->ack_notifier.gsi = -1;
+	} else {
+		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;
-
 		adev->host_irq = adev->dev->irq;
 		if (request_irq(adev->host_irq, kvm_assigned_dev_intr, 0,
 				"kvm_assigned_msi_device", (void *)adev))
 			return -EIO;
 	}
 
-	adev->irq_requested_type = KVM_ASSIGNED_DEV_GUEST_MSI |
-				   KVM_ASSIGNED_DEV_HOST_MSI;
+	adev->irq_requested_type |= KVM_ASSIGNED_DEV_HOST_MSI;
 	return 0;
 }
 #endif
@@ -343,11 +341,18 @@ 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 */
+			pci_enable_msi(match->dev);
+#endif
 		}
 	}
 
-	if (assigned_irq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSI) {
+	if (match->dev->msi_enabled) {
 #ifdef CONFIG_X86
+		/* Device support MSI */
 		r = assigned_device_update_msi(kvm, match, assigned_irq);
 		if (r) {
 			printk(KERN_WARNING "kvm: failed to enable "
@@ -358,9 +363,10 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 		r = -ENOTTY;
 #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;
+		/* Device only support MSI but can't enable */
+		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);
-- 
1.5.4.5

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

[Avi Kivity]

Sheng Yang wrote:
> Prepared for kvm_arch_assigned_device_msi_dispatch().
>
>  	__u32 host_irq;
>  	__u32 guest_irq;
>  	__u32 flags;
> +	__u32 guest_msi_data;
> +	__u32 guest_msi_addr;
>  	union {
> -		__u32 reserved[12];
> +		__u32 reserved[10];
>  	};
>  };
>   

_guest_msi_* should be in the union, so they don't consume space for 
non-msi payloads.


-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 06/10] KVM: Export ioapic_get_delivery_bitmask

[Avi Kivity]

Sheng Yang wrote:
> It would be used for MSI in device assignment, for MSI dispatch.
>  
> -static u32 ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
> -				       u8 dest_mode)
> +u32 ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
> +				u8 dest_mode)
>   

Need to be prefixed with kvm_, otherwise it might be confused with the 
host ioapic.

I'm a little worried about locking; we'll want to have a separate lock 
for the ioapic, and callers of this function will need to know about 
it.  But we can handle that later.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 07/10] x86: Add MSI delivery mode value

[Avi Kivity]

Sheng Yang wrote:
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  arch/x86/include/asm/msidef.h |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/include/asm/msidef.h b/arch/x86/include/asm/msidef.h
> index 6706b30..32edc7f 100644
> --- a/arch/x86/include/asm/msidef.h
> +++ b/arch/x86/include/asm/msidef.h
> @@ -17,6 +17,8 @@
>  #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_VAL	0
> +#define  MSI_DATA_DELIVERY_LOWPRI_VAL	1
>   

Shouldn't _FIXED be defined in terms of _FIXED_VAL then?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 06/10] KVM: Export ioapic_get_delivery_bitmask

[Sheng Yang]

On Tuesday 04 November 2008 19:07:19 Avi Kivity wrote:
> Sheng Yang wrote:
> > It would be used for MSI in device assignment, for MSI dispatch.
> >
> > -static u32 ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8
> > dest, -				       u8 dest_mode)
> > +u32 ioapic_get_delivery_bitmask(struct kvm_ioapic *ioapic, u8 dest,
> > +				u8 dest_mode)
>
> Need to be prefixed with kvm_, otherwise it might be confused with the
> host ioapic.
>
> I'm a little worried about locking; we'll want to have a separate lock
> for the ioapic, and callers of this function will need to know about
> it.  But we can handle that later.

Yeah, agree. And after kvm_apic_local_deliver() was exported, I also a little 
worry about locking on apic side. I would like take a look at them later.

-- 
regards
Yang, Sheng

Re: [PATCH 08/10] KVM: Add assigned_device_msi_dispatch()

[Avi Kivity]

Sheng Yang wrote:
> The function is used to dispatch MSI to lapic according to MSI message
> address and message data.
>
> (Any way to replace macro GET_VAL_FROM_SHIFT()?)
>   

> +#ifdef CONFIG_X86
> +#include <asm/msidef.h>
> +#endif
>   

The include can be unconditional.

> +
> +#ifdef CONFIG_X86
> +static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
> +{
> +#define GET_VAL_FROM_SHIFT(v, s) (((v) & (1 << s)) >> s)
>   

test_bit() ?

> +	int vcpu_id;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
> +	u8 dest_id = (dev->guest_msi_addr & MSI_ADDR_DEST_ID_MASK)
> +			>> MSI_ADDR_DEST_ID_SHIFT;
> +	u8 vector = (dev->guest_msi_data & MSI_DATA_VECTOR_MASK)
> +			>> MSI_DATA_VECTOR_SHIFT;
> +	u8 dest_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_addr,
> +					  MSI_ADDR_DEST_MODE_SHIFT);
> +	u8 trig_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_data,
> +					  MSI_DATA_TRIGGER_SHIFT);
> +	u8 delivery_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_data,
> +					  MSI_DATA_DELIVERY_MODE_SHIFT);
>   

int is usually better than u8.

> +	u32 deliver_bitmask;
> +
> +	case MSI_DATA_DELIVERY_FIXED_VAL:
> +		for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) {
>   

__ffs() and __clear_bit() can make this a bit faster.  Can be done later 
though.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 08/10] KVM: Add assigned_device_msi_dispatch()

[Sheng Yang]

On Tuesday 04 November 2008 19:17:03 Avi Kivity wrote:
> Sheng Yang wrote:
> > The function is used to dispatch MSI to lapic according to MSI message
> > address and message data.
> >
> > (Any way to replace macro GET_VAL_FROM_SHIFT()?)
> >
> >
> > +#ifdef CONFIG_X86
> > +#include <asm/msidef.h>
> > +#endif
>
> The include can be unconditional.

it's pity that IA64 don't have this head file... I will separate the msi.c and 
add this later.

>
> > +
> > +#ifdef CONFIG_X86
> > +static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel
> > *dev) +{
> > +#define GET_VAL_FROM_SHIFT(v, s) (((v) & (1 << s)) >> s)
>
> test_bit() ?

Oh, indeed here. :)
>
> > +	int vcpu_id;
> > +	struct kvm_vcpu *vcpu;
> > +	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
> > +	u8 dest_id = (dev->guest_msi_addr & MSI_ADDR_DEST_ID_MASK)
> > +			>> MSI_ADDR_DEST_ID_SHIFT;
> > +	u8 vector = (dev->guest_msi_data & MSI_DATA_VECTOR_MASK)
> > +			>> MSI_DATA_VECTOR_SHIFT;
> > +	u8 dest_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_addr,
> > +					  MSI_ADDR_DEST_MODE_SHIFT);
> > +	u8 trig_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_data,
> > +					  MSI_DATA_TRIGGER_SHIFT);
> > +	u8 delivery_mode = GET_VAL_FROM_SHIFT(dev->guest_msi_data,
> > +					  MSI_DATA_DELIVERY_MODE_SHIFT);
>
> int is usually better than u8.

OK.
>
> > +	u32 deliver_bitmask;
> > +
> > +	case MSI_DATA_DELIVERY_FIXED_VAL:
> > +		for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) {
>
> __ffs() and __clear_bit() can make this a bit faster.  Can be done later
> though.

OK, also would optimize IOAPIC side later.

-- 
regards
Yang, Sheng

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

[Avi Kivity]

Sheng Yang wrote:
> 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.
>   

Need a KVM_CAP_MSI so that userspace can tell before trying.

> +	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);
> +	}
>   

Hmm.  This means the only path that can trigger msi is device 
assignment.  But we also want virtio-pci to use msi.

How about modifying kvm_set_irq() to dispatch either msi or normal 
irqs?  We'd need a 'struct kvm_guest_irq' if we can't define the msi 
using the bits available in gsi (and also define a new KVM_IRQ_LINE... I 
really hope we can fit the msi info in gsi).

The device-assignment-independent parts should be in a separate patch.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 10/10] KVM: MSI to INTx translate

[Avi Kivity]

Sheng Yang wrote:
> 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.
>
>   

Won't we run into trouble with edge-triggered vs level-triggered.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 07/10] x86: Add MSI delivery mode value

[Sheng Yang]

On Tuesday 04 November 2008 19:09:15 Avi Kivity wrote:
> Sheng Yang wrote:
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  arch/x86/include/asm/msidef.h |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/msidef.h
> > b/arch/x86/include/asm/msidef.h index 6706b30..32edc7f 100644
> > --- a/arch/x86/include/asm/msidef.h
> > +++ b/arch/x86/include/asm/msidef.h
> > @@ -17,6 +17,8 @@
> >  #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_VAL	0
> > +#define  MSI_DATA_DELIVERY_LOWPRI_VAL	1
>
> Shouldn't _FIXED be defined in terms of _FIXED_VAL then?

Yes... It's a compromise... Well, if you think it's OK, I would make it more 
elegant (rename macro on host side, I would do the same thing to IA64 sooner 
or later).

-- 
regards
Yang, Sheng

Re: [PATCH 10/10] KVM: MSI to INTx translate

[Sheng Yang]

On Tuesday 04 November 2008 19:25:34 Avi Kivity wrote:
> Sheng Yang wrote:
> > 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.
>
> Won't we run into trouble with edge-triggered vs level-triggered.

Yes, in theory, we got a little trouble, but not that much.

The mechanism based on a assumption: every time guest got a interrupt, it 
would deassert the interrupt source sooner or later. So we take the Ack action 
as the same as deassert the IRQ line here (unmasked the IRQ).

So the biggest known issue is, after guest got the interrupt, rather than 
deassert it, guest driver took the advantage of level triggered interrupt, and 
just want to deassert after it receive it for e.g. 10 times. Then we would got 
the trouble. But it's so tricky and we don't know any device by now take this 
way yet. Of course they can, so we also suggest to use a whitelist/blacklist 
for this approach. But this should work in most condition.

Yeah, it's aggressive, but we think it's reasonable, and tested OK (in my 
limited environment). Also I heard that Hyper-V or Vmware did this.

Any more concerns? Hope we didn't neglect something.

-- 
regards
Yang, Sheng

Re: [PATCH 10/10] KVM: MSI to INTx translate

[Avi Kivity]

Sheng Yang wrote:
> Yes, in theory, we got a little trouble, but not that much.
>
> The mechanism based on a assumption: every time guest got a interrupt, it 
> would deassert the interrupt source sooner or later. So we take the Ack action 
> as the same as deassert the IRQ line here (unmasked the IRQ).
>
> So the biggest known issue is, after guest got the interrupt, rather than 
> deassert it, guest driver took the advantage of level triggered interrupt, and 
> just want to deassert after it receive it for e.g. 10 times. Then we would got 
> the trouble. But it's so tricky and we don't know any device by now take this 
> way yet. Of course they can, so we also suggest to use a whitelist/blacklist 
> for this approach. But this should work in most condition.
>
> Yeah, it's aggressive, but we think it's reasonable, and tested OK (in my 
> limited environment). Also I heard that Hyper-V or Vmware did this.
>
> Any more concerns? Hope we didn't neglect something.
>   

Please add a module parameter to disable msi->pciirq conversion in that 
case.  So if someone runs into trouble, we can try out the alternative.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 10/10] KVM: MSI to INTx translate

[Sheng Yang]

On Tue, Nov 04, 2008 at 01:57:56PM +0200, Avi Kivity wrote:
> Sheng Yang wrote:
>> Yes, in theory, we got a little trouble, but not that much.
>>
>> The mechanism based on a assumption: every time guest got a interrupt, 
>> it would deassert the interrupt source sooner or later. So we take the 
>> Ack action as the same as deassert the IRQ line here (unmasked the 
>> IRQ).
>>
>> So the biggest known issue is, after guest got the interrupt, rather 
>> than deassert it, guest driver took the advantage of level triggered 
>> interrupt, and just want to deassert after it receive it for e.g. 10 
>> times. Then we would got the trouble. But it's so tricky and we don't 
>> know any device by now take this way yet. Of course they can, so we 
>> also suggest to use a whitelist/blacklist for this approach. But this 
>> should work in most condition.
>>
>> Yeah, it's aggressive, but we think it's reasonable, and tested OK (in 
>> my limited environment). Also I heard that Hyper-V or Vmware did this.
>>
>> Any more concerns? Hope we didn't neglect something.
>>   
>
> Please add a module parameter to disable msi->pciirq conversion in that  
> case.  So if someone runs into trouble, we can try out the alternative.

Yes of course. :)

--
regards
Yang, Sheng

> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sheng Yang]

On Tue, Nov 04, 2008 at 01:23:04PM +0200, Avi Kivity wrote:
> Sheng Yang wrote:
>> 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.
>>   
>
> Need a KVM_CAP_MSI so that userspace can tell before trying.
>
>> +	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);
>> +	}
>>   
>
> Hmm.  This means the only path that can trigger msi is device  
> assignment.  But we also want virtio-pci to use msi.
>
> How about modifying kvm_set_irq() to dispatch either msi or normal irqs?  
> We'd need a 'struct kvm_guest_irq' if we can't define the msi using the 
> bits available in gsi (and also define a new KVM_IRQ_LINE... I really 
> hope we can fit the msi info in gsi).
>
> The device-assignment-independent parts should be in a separate patch.

One question here: did the architectures other than X86 and IA64 need
dispatch MSI support?  If so, I am afraid reuse assigned_device_msi_dispatch
(or similar solution) won't be elegant (though I already lean toward a arch
specific dispatch function).

I am not familiar with virtio-pci and how it works, it would be good if
it can be illustrated a little more. But I guess what you means is only set
gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
list, for gsi >= IOAPIC_PINS, kvm_set_irq() checked the list and deliver the
interrupt. Of course, some lock should be taken to maintain the list, maybe
kvm->lock in first step as we used for kvm_set_irq (kvm->lock become more
and more bigger...).

--
regards
Yang, Sheng
>
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Avi Kivity]

Sheng Yang wrote:
> One question here: did the architectures other than X86 and IA64 need
> dispatch MSI support?  

s390 doesn't.  ppc may, one day in the future.  Not now I think.

> If so, I am afraid reuse assigned_device_msi_dispatch
> (or similar solution) won't be elegant (though I already lean toward a arch
> specific dispatch function).
>
> I am not familiar with virtio-pci and how it works, it would be good if
> it can be illustrated a little more. 

For this context, it's just a pci device.  qemu calls KVM_IRQ_LINE to 
inject an irq, and the pci layer in the guest delivers the irq to the 
virtio-pci driver.

In fact, it's not just for pci.  We could msi-enable e1000 and get 
improved performance there as well.

> But I guess what you means is only set
> gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
> be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
> list, for gsi >= IOAPIC_PINS,
Let's say, (gsi >> 24) == 1.  That gives us 16M potential MSIs.

>  kvm_set_irq() checked the list and deliver the
> interrupt. Of course, some lock should be taken to maintain the list, maybe
> kvm->lock in first step as we used for kvm_set_irq (kvm->lock become more
> and more bigger...).
>   

I think kvm_set_irq() already takes kvm->lock.

-- 
error compiling committee.c: too many arguments to function

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

[Sheng Yang]

On Tue, Nov 04, 2008 at 03:32:49PM +0200, Avi Kivity wrote:
> Sheng Yang wrote:
>> One question here: did the architectures other than X86 and IA64 need
>> dispatch MSI support?  
>
> s390 doesn't.  ppc may, one day in the future.  Not now I think.

OK, so reuse is enough for today.
>
>> If so, I am afraid reuse assigned_device_msi_dispatch
>> (or similar solution) won't be elegant (though I already lean toward a arch
>> specific dispatch function).
>>
>> I am not familiar with virtio-pci and how it works, it would be good if
>> it can be illustrated a little more. 
>
> For this context, it's just a pci device.  qemu calls KVM_IRQ_LINE to  
> inject an irq, and the pci layer in the guest delivers the irq to the  
> virtio-pci driver.

Got it.
>
> In fact, it's not just for pci.  We could msi-enable e1000 and get  
> improved performance there as well.

E1000? Don't understand... Sounds like INTx->MSI...
>
>> But I guess what you means is only set
>> gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
>> be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
>> list, for gsi >= IOAPIC_PINS,
> Let's say, (gsi >> 24) == 1.  That gives us 16M potential MSIs.

Confused... GSI become a bitmap?
>
>>  kvm_set_irq() checked the list and deliver the
>> interrupt. Of course, some lock should be taken to maintain the list, maybe
>> kvm->lock in first step as we used for kvm_set_irq (kvm->lock become more
>> and more bigger...).
>>   
>
> I think kvm_set_irq() already takes kvm->lock.

Yes. So I means maybe we can improve the granularity of the lock in interrupt
handling part in the future.

--
regards
Yang, Sheng
>
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Avi Kivity]

Sheng Yang wrote:
>> In fact, it's not just for pci.  We could msi-enable e1000 and get  
>> improved performance there as well.
>>     
>
> E1000? Don't understand... Sounds like INTx->MSI...
>   

There are msi-capable e1000 cards.  If we upgrade qemu's e1000 to 
support msi, and if the guest supports msi, it will be msi all the way.

>>> But I guess what you means is only set
>>> gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
>>> be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
>>> list, for gsi >= IOAPIC_PINS,
>>>       
>> Let's say, (gsi >> 24) == 1.  That gives us 16M potential MSIs.
>>     
>
> Confused... GSI become a bitmap?
>   

A structure...

struct gsi {
    u32 n : 24;
    u32 type : 8; // 0 -> classic gsi, 1 -> msi
};


-- 
error compiling committee.c: too many arguments to function

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

[Sheng Yang]

On Tue, Nov 04, 2008 at 04:35:19PM +0200, Avi Kivity wrote:
> Sheng Yang wrote:
>>> In fact, it's not just for pci.  We could msi-enable e1000 and get   
>>> improved performance there as well.
>>>     
>>
>> E1000? Don't understand... Sounds like INTx->MSI...
>>   
>
> There are msi-capable e1000 cards.  If we upgrade qemu's e1000 to  
> support msi, and if the guest supports msi, it will be msi all the way.

Oh, forgot we got e1000 emulation in qemu...
>
>>>> But I guess what you means is only set
>>>> gsi can result in kvm_set_irq() deliver the MSI correctly. I think this can
>>>> be done. Associating gsi with guest_msi_addr and guest_msi_data in a linked
>>>> list, for gsi >= IOAPIC_PINS,
>>>>       
>>> Let's say, (gsi >> 24) == 1.  That gives us 16M potential MSIs.
>>>     
>>
>> Confused... GSI become a bitmap?
>>   
>
> A structure...
>
> struct gsi {
>    u32 n : 24;
>    u32 type : 8; // 0 -> classic gsi, 1 -> msi
> };

I just don't understand why we need 24 bits for n? GSI can be that wide?

--
regards
Yang, Sheng
>
>
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Avi Kivity]

Sheng Yang wrote:
>> struct gsi {
>>    u32 n : 24;
>>    u32 type : 8; // 0 -> classic gsi, 1 -> msi
>> };
>>     
>
> I just don't understand why we need 24 bits for n? GSI can be that wide?
>   

I don't think so, but who knows what we'll need for type == 2?

-- 
error compiling committee.c: too many arguments to function

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

[Sheng Yang]

On Tue, Nov 04, 2008 at 05:01:04PM +0200, Avi Kivity wrote:
> Sheng Yang wrote:
>>> struct gsi {
>>>    u32 n : 24;
>>>    u32 type : 8; // 0 -> classic gsi, 1 -> msi
>>> };
>>>     
>>
>> I just don't understand why we need 24 bits for n? GSI can be that wide?
>>   
>
> I don't think so, but who knows what we'll need for type == 2?

Oh, OK, don't take it too seriously. I just took it too seriously to think
that you got some certain defines to support magic number 24. :)

--
regards
Yang, Sheng
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html