[PATCH 0/3 v4][Resend] MSI-X enabling

[PATCH 0/3 v4][Resend] MSI-X enabling

[Sheng Yang]

Drop the first patch which corrected ABI error.

[PATCH 1/3] KVM: Ioctls for init MSI-X entry

[Sheng Yang]

Introduce KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY two ioctls.

This two ioctls are used by userspace to specific guest device MSI-X entry
number and correlate MSI-X entry with GSI during the initialization stage.

MSI-X should be well initialzed before enabling.

Don't support change MSI-X entry number for now.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm.h      |   18 ++++++++
 include/linux/kvm_host.h |   10 ++++
 virt/kvm/kvm_main.c      |  104 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+), 0 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 0d94b27..c210856 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -485,6 +485,10 @@ struct kvm_irq_routing {
 #define KVM_REINJECT_CONTROL      _IO(KVMIO, 0x71)
 #define KVM_DEASSIGN_PCI_DEVICE _IOR(KVMIO, 0x72, \
 				     struct kvm_assigned_pci_dev)
+#define KVM_ASSIGN_SET_MSIX_NR \
+			_IOW(KVMIO, 0x73, struct kvm_assigned_msix_nr)
+#define KVM_ASSIGN_SET_MSIX_ENTRY \
+			_IOW(KVMIO, 0x74, struct kvm_assigned_msix_entry)
 
 /*
  * ioctls for vcpu fds
@@ -605,4 +609,18 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
 
+struct kvm_assigned_msix_nr {
+	__u32 assigned_dev_id;
+	__u16 entry_nr;
+	__u16 padding;
+};
+
+#define KVM_MAX_MSIX_PER_DEV		512
+struct kvm_assigned_msix_entry {
+	__u32 assigned_dev_id;
+	__u32 gsi;
+	__u16 entry; /* The index of entry in the MSI-X table */
+	__u16 padding[3];
+};
+
 #endif
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 1a2f98f..432edc2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -319,6 +319,12 @@ struct kvm_irq_ack_notifier {
 	void (*irq_acked)(struct kvm_irq_ack_notifier *kian);
 };
 
+struct kvm_guest_msix_entry {
+	u32 vector;
+	u16 entry;
+	u16 flags;
+};
+
 struct kvm_assigned_dev_kernel {
 	struct kvm_irq_ack_notifier ack_notifier;
 	struct work_struct interrupt_work;
@@ -326,13 +332,17 @@ struct kvm_assigned_dev_kernel {
 	int assigned_dev_id;
 	int host_busnr;
 	int host_devfn;
+	unsigned int entries_nr;
 	int host_irq;
 	bool host_irq_disabled;
+	struct msix_entry *host_msix_entries;
 	int guest_irq;
+	struct kvm_guest_msix_entry *guest_msix_entries;
 #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)
+#define KVM_ASSIGNED_DEV_MSIX		((1 << 2) | (1 << 10))
 	unsigned long irq_requested_type;
 	int irq_source_id;
 	int flags;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index c427897..ac1a0e8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1595,6 +1595,88 @@ static int kvm_vcpu_ioctl_set_sigmask(struct kvm_vcpu *vcpu, sigset_t *sigset)
 	return 0;
 }
 
+#ifdef __KVM_HAVE_MSIX
+static int kvm_vm_ioctl_set_msix_nr(struct kvm *kvm,
+				    struct kvm_assigned_msix_nr *entry_nr)
+{
+	int r = 0;
+	struct kvm_assigned_dev_kernel *adev;
+
+	mutex_lock(&kvm->lock);
+
+	adev = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
+				      entry_nr->assigned_dev_id);
+	if (!adev) {
+		r = -EINVAL;
+		goto msix_nr_out;
+	}
+
+	if (adev->entries_nr == 0) {
+		adev->entries_nr = entry_nr->entry_nr;
+		if (adev->entries_nr == 0 ||
+		    adev->entries_nr >= KVM_MAX_MSIX_PER_DEV) {
+			r = -EINVAL;
+			goto msix_nr_out;
+		}
+
+		adev->host_msix_entries = kzalloc(sizeof(struct msix_entry) *
+						entry_nr->entry_nr,
+						GFP_KERNEL);
+		if (!adev->host_msix_entries) {
+			r = -ENOMEM;
+			goto msix_nr_out;
+		}
+		adev->guest_msix_entries = kzalloc(
+				sizeof(struct kvm_guest_msix_entry) *
+				entry_nr->entry_nr, GFP_KERNEL);
+		if (!adev->guest_msix_entries) {
+			kfree(adev->host_msix_entries);
+			r = -ENOMEM;
+			goto msix_nr_out;
+		}
+	} else /* Not allowed set MSI-X number twice */
+		r = -EINVAL;
+msix_nr_out:
+	mutex_unlock(&kvm->lock);
+	return r;
+}
+
+static int kvm_vm_ioctl_set_msix_entry(struct kvm *kvm,
+				       struct kvm_assigned_msix_entry *entry)
+{
+	int r = 0, i;
+	struct kvm_assigned_dev_kernel *adev;
+
+	mutex_lock(&kvm->lock);
+
+	adev = kvm_find_assigned_dev(&kvm->arch.assigned_dev_head,
+				      entry->assigned_dev_id);
+
+	if (!adev) {
+		r = -EINVAL;
+		goto msix_entry_out;
+	}
+
+	for (i = 0; i < adev->entries_nr; i++)
+		if (adev->guest_msix_entries[i].vector == 0 ||
+		    adev->guest_msix_entries[i].entry == entry->entry) {
+			adev->guest_msix_entries[i].entry = entry->entry;
+			adev->guest_msix_entries[i].vector = entry->gsi;
+			adev->host_msix_entries[i].entry = entry->entry;
+			break;
+		}
+	if (i == adev->entries_nr) {
+		r = -ENOSPC;
+		goto msix_entry_out;
+	}
+
+msix_entry_out:
+	mutex_unlock(&kvm->lock);
+
+	return r;
+}
+#endif
+
 static long kvm_vcpu_ioctl(struct file *filp,
 			   unsigned int ioctl, unsigned long arg)
 {
@@ -1919,7 +2001,29 @@ static long kvm_vm_ioctl(struct file *filp,
 		vfree(entries);
 		break;
 	}
+#ifdef __KVM_HAVE_MSIX
+	case KVM_ASSIGN_SET_MSIX_NR: {
+		struct kvm_assigned_msix_nr entry_nr;
+		r = -EFAULT;
+		if (copy_from_user(&entry_nr, argp, sizeof entry_nr))
+			goto out;
+		r = kvm_vm_ioctl_set_msix_nr(kvm, &entry_nr);
+		if (r)
+			goto out;
+		break;
+	}
+	case KVM_ASSIGN_SET_MSIX_ENTRY: {
+		struct kvm_assigned_msix_entry entry;
+		r = -EFAULT;
+		if (copy_from_user(&entry, argp, sizeof entry))
+			goto out;
+		r = kvm_vm_ioctl_set_msix_entry(kvm, &entry);
+		if (r)
+			goto out;
+		break;
+	}
 #endif
+#endif /* KVM_CAP_IRQ_ROUTING */
 	default:
 		r = kvm_arch_vm_ioctl(filp, ioctl, arg);
 	}
-- 
1.5.4.5

[PATCH 2/3] KVM: Add MSI-X interrupt injection logic

[Sheng Yang]

We have to handle more than one interrupt with one handler for MSI-X. Avi
suggested to use a flag to indicate the pending. So here is it.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm_host.h |    1 +
 virt/kvm/kvm_main.c      |   66 +++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 432edc2..3832243 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -319,6 +319,7 @@ struct kvm_irq_ack_notifier {
 	void (*irq_acked)(struct kvm_irq_ack_notifier *kian);
 };
 
+#define KVM_ASSIGNED_MSIX_PENDING		0x1
 struct kvm_guest_msix_entry {
 	u32 vector;
 	u16 entry;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ac1a0e8..e75855a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -95,25 +95,69 @@ static struct kvm_assigned_dev_kernel *kvm_find_assigned_dev(struct list_head *h
 	return NULL;
 }
 
+static int find_index_from_host_irq(struct kvm_assigned_dev_kernel
+				    *assigned_dev, int irq)
+{
+	int i, index;
+	struct msix_entry *host_msix_entries;
+
+	host_msix_entries = assigned_dev->host_msix_entries;
+
+	index = -1;
+	for (i = 0; i < assigned_dev->entries_nr; i++)
+		if (irq == host_msix_entries[i].vector) {
+			index = i;
+			break;
+		}
+	if (index < 0) {
+		printk(KERN_WARNING "Fail to find correlated MSI-X entry!\n");
+		return 0;
+	}
+
+	return index;
+}
+
 static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 {
 	struct kvm_assigned_dev_kernel *assigned_dev;
+	struct kvm *kvm;
+	int irq, i;
 
 	assigned_dev = container_of(work, struct kvm_assigned_dev_kernel,
 				    interrupt_work);
+	kvm = assigned_dev->kvm;
 
 	/* This is taken to safely inject irq inside the guest. When
 	 * the interrupt injection (or the ioapic code) uses a
 	 * 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_MSI) {
-		enable_irq(assigned_dev->host_irq);
-		assigned_dev->host_irq_disabled = false;
+	mutex_lock(&kvm->lock);
+	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
+		struct kvm_guest_msix_entry *guest_entries =
+			assigned_dev->guest_msix_entries;
+		for (i = 0; i < assigned_dev->entries_nr; i++) {
+			if (!(guest_entries[i].flags &
+					KVM_ASSIGNED_MSIX_PENDING))
+				continue;
+			guest_entries[i].flags &= ~KVM_ASSIGNED_MSIX_PENDING;
+			kvm_set_irq(assigned_dev->kvm,
+				    assigned_dev->irq_source_id,
+				    guest_entries[i].vector, 1);
+			irq = assigned_dev->host_msix_entries[i].vector;
+			if (irq != 0)
+				enable_irq(irq);
+			assigned_dev->host_irq_disabled = false;
+		}
+	} else {
+		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_MSI) {
+			enable_irq(assigned_dev->host_irq);
+			assigned_dev->host_irq_disabled = false;
+		}
 	}
+
 	mutex_unlock(&assigned_dev->kvm->lock);
 }
 
@@ -122,6 +166,14 @@ static irqreturn_t kvm_assigned_dev_intr(int irq, void *dev_id)
 	struct kvm_assigned_dev_kernel *assigned_dev =
 		(struct kvm_assigned_dev_kernel *) dev_id;
 
+	if (assigned_dev->irq_requested_type == KVM_ASSIGNED_DEV_MSIX) {
+		int index = find_index_from_host_irq(assigned_dev, irq);
+		if (index < 0)
+			return IRQ_HANDLED;
+		assigned_dev->guest_msix_entries[index].flags |=
+			KVM_ASSIGNED_MSIX_PENDING;
+	}
+
 	schedule_work(&assigned_dev->interrupt_work);
 
 	disable_irq_nosync(irq);
-- 
1.5.4.5

[PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Sheng Yang]

This patch finally enable MSI-X.

What we need for MSI-X:
1. Intercept one page in MMIO region of device. So that we can get guest desired
MSI-X table and set up the real one. Now this have been done by guest, and
transfer to kernel using ioctl KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY.

2. Information for incoming interrupt. Now one device can have more than one
interrupt, and they are all handled by one workqueue structure. So we need to
identify them. The previous patch enable gsi_msg_pending_bitmap get this done.

3. Mapping from host IRQ to guest gsi as well as guest gsi to real MSI/MSI-X
message address/data. We used same entry number for the host and guest here, so
that it's easy to find the correlated guest gsi.

What we lack for now:
1. The PCI spec said nothing can existed with MSI-X table in the same page of
MMIO region, except pending bits. The patch ignore pending bits as the first
step (so they are always 0 - no pending).

2. The PCI spec allowed to change MSI-X table dynamically. That means, the OS
can enable MSI-X, then mask one MSI-X entry, modify it, and unmask it. The patch
didn't support this, and Linux also don't work in this way.

3. The patch didn't implement MSI-X mask all and mask single entry. I would
implement the former in driver/pci/msi.c later. And for single entry, userspace
should have reposibility to handle it.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 arch/x86/include/asm/kvm.h |    1 +
 include/linux/kvm.h        |    8 ++++
 virt/kvm/kvm_main.c        |   98 +++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kvm.h b/arch/x86/include/asm/kvm.h
index dc3f6cf..125be8b 100644
--- a/arch/x86/include/asm/kvm.h
+++ b/arch/x86/include/asm/kvm.h
@@ -16,6 +16,7 @@
 #define __KVM_HAVE_MSI
 #define __KVM_HAVE_USER_NMI
 #define __KVM_HAVE_GUEST_DEBUG
+#define __KVM_HAVE_MSIX
 
 /* Architectural interrupt line count. */
 #define KVM_NR_INTERRUPTS 256
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index c210856..bbeb9a1 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -409,6 +409,9 @@ struct kvm_trace_rec {
 #ifdef __KVM_HAVE_DEVICE_ASSIGNMENT
 #define KVM_CAP_DEVICE_DEASSIGNMENT 27
 #endif
+#ifdef __KVM_HAVE_MSIX
+#define KVM_CAP_DEVICE_MSIX 28
+#endif
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -609,6 +612,11 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
 
+#define KVM_DEV_IRQ_ASSIGN_MSIX_ACTION  (KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX |\
+					KVM_DEV_IRQ_ASSIGN_MASK_MSIX)
+#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX  (1 << 1)
+#define KVM_DEV_IRQ_ASSIGN_MASK_MSIX    (1 << 2)
+
 struct kvm_assigned_msix_nr {
 	__u32 assigned_dev_id;
 	__u16 entry_nr;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e75855a..4d2be16 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -236,13 +236,33 @@ static void kvm_free_assigned_irq(struct kvm *kvm,
 	 * now, the kvm state is still legal for probably we also have to wait
 	 * interrupt_work done.
 	 */
-	disable_irq_nosync(assigned_dev->host_irq);
-	cancel_work_sync(&assigned_dev->interrupt_work);
+	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
+		int i;
+		for (i = 0; i < assigned_dev->entries_nr; i++)
+			disable_irq_nosync(assigned_dev->
+					   host_msix_entries[i].vector);
+
+		cancel_work_sync(&assigned_dev->interrupt_work);
 
-	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
+		for (i = 0; i < assigned_dev->entries_nr; i++)
+			free_irq(assigned_dev->host_msix_entries[i].vector,
+				 (void *)assigned_dev);
 
-	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)
-		pci_disable_msi(assigned_dev->dev);
+		assigned_dev->entries_nr = 0;
+		kfree(assigned_dev->host_msix_entries);
+		kfree(assigned_dev->guest_msix_entries);
+		pci_disable_msix(assigned_dev->dev);
+	} else {
+		/* Deal with MSI and INTx */
+		disable_irq_nosync(assigned_dev->host_irq);
+		cancel_work_sync(&assigned_dev->interrupt_work);
+
+		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);
+	}
 
 	assigned_dev->irq_requested_type = 0;
 }
@@ -373,6 +393,60 @@ static int assigned_device_update_msi(struct kvm *kvm,
 }
 #endif
 
+#ifdef __KVM_HAVE_MSIX
+static int assigned_device_update_msix(struct kvm *kvm,
+			struct kvm_assigned_dev_kernel *adev,
+			struct kvm_assigned_irq *airq)
+{
+	/* TODO Deal with KVM_DEV_IRQ_ASSIGNED_MASK_MSIX */
+	int i, r;
+
+	adev->ack_notifier.gsi = -1;
+
+	if (irqchip_in_kernel(kvm)) {
+		if (airq->flags & KVM_DEV_IRQ_ASSIGN_MASK_MSIX)
+			return -ENOTTY;
+
+		if (!(airq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX)) {
+			/* Guest disable MSI-X */
+			kvm_free_assigned_irq(kvm, adev);
+			if (msi2intx) {
+				pci_enable_msi(adev->dev);
+				if (adev->dev->msi_enabled)
+					return assigned_device_update_msi(kvm,
+							adev, airq);
+			}
+			return assigned_device_update_intx(kvm, adev, airq);
+		}
+
+		/* host_msix_entries and guest_msix_entries should have been
+		 * initialized */
+		if (adev->entries_nr == 0)
+			return -EINVAL;
+
+		kvm_free_assigned_irq(kvm, adev);
+
+		r = pci_enable_msix(adev->dev, adev->host_msix_entries,
+				    adev->entries_nr);
+		if (r)
+			return r;
+
+		for (i = 0; i < adev->entries_nr; i++) {
+			r = request_irq((adev->host_msix_entries + i)->vector,
+					kvm_assigned_dev_intr, 0,
+					"kvm_assigned_msix_device",
+					(void *)adev);
+			if (r)
+				return r;
+		}
+	}
+
+	adev->irq_requested_type |= KVM_ASSIGNED_DEV_MSIX;
+
+	return 0;
+}
+#endif
+
 static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 				   struct kvm_assigned_irq
 				   *assigned_irq)
@@ -417,12 +491,24 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 		}
 	}
 
-	if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
+	if (match->irq_requested_type & KVM_ASSIGNED_DEV_MSIX)
+		current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX;
+	else if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
 		 (match->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_MSI))
 		current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSI;
 
 	changed_flags = assigned_irq->flags ^ current_flags;
 
+#ifdef __KVM_HAVE_MSIX
+	if (changed_flags & KVM_DEV_IRQ_ASSIGN_MSIX_ACTION) {
+		r = assigned_device_update_msix(kvm, match, assigned_irq);
+		if (r) {
+			printk(KERN_WARNING "kvm: failed to execute "
+					"MSI-X action!\n");
+			goto out_release;
+		}
+	} else
+#endif
 	if ((changed_flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION) ||
 	    (msi2intx && match->dev->msi_enabled)) {
 #ifdef CONFIG_X86
-- 
1.5.4.5

Re: [PATCH 0/3 v4][Resend] MSI-X enabling

[Avi Kivity]

Sheng Yang wrote:
> Drop the first patch which corrected ABI error.
>   

Applied all, thanks.

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

[PATCH 0/3 v3] MSI-X enabling

[Sheng Yang]

Updated the patchset followed Marcelo and Avi's comments.

Please also review MSI/MSI-X userspace patch as well.

Thanks!
--
regards
Yang, Sheng

[PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Sheng Yang]

This patch finally enable MSI-X.

What we need for MSI-X:
1. Intercept one page in MMIO region of device. So that we can get guest desired
MSI-X table and set up the real one. Now this have been done by guest, and
transfer to kernel using ioctl KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY.

2. Information for incoming interrupt. Now one device can have more than one
interrupt, and they are all handled by one workqueue structure. So we need to
identify them. The previous patch enable gsi_msg_pending_bitmap get this done.

3. Mapping from host IRQ to guest gsi as well as guest gsi to real MSI/MSI-X
message address/data. We used same entry number for the host and guest here, so
that it's easy to find the correlated guest gsi.

What we lack for now:
1. The PCI spec said nothing can existed with MSI-X table in the same page of
MMIO region, except pending bits. The patch ignore pending bits as the first
step (so they are always 0 - no pending).

2. The PCI spec allowed to change MSI-X table dynamically. That means, the OS
can enable MSI-X, then mask one MSI-X entry, modify it, and unmask it. The patch
didn't support this, and Linux also don't work in this way.

3. The patch didn't implement MSI-X mask all and mask single entry. I would
implement the former in driver/pci/msi.c later. And for single entry, userspace
should have reposibility to handle it.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm.h |    8 ++++
 virt/kvm/kvm_main.c |  107 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index a2dfbe0..78480d0 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -440,6 +440,9 @@ struct kvm_irq_routing {
 };
 
 #endif
+#if defined(CONFIG_X86)
+#define KVM_CAP_DEVICE_MSIX 26
+#endif
 
 /*
  * ioctls for VM fds
@@ -597,6 +600,11 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
 
+#define KVM_DEV_IRQ_ASSIGN_MSIX_ACTION  (KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX |\
+					KVM_DEV_IRQ_ASSIGN_MASK_MSIX)
+#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX  (1 << 1)
+#define KVM_DEV_IRQ_ASSIGN_MASK_MSIX    (1 << 2)
+
 struct kvm_assigned_msix_nr {
 	__u32 assigned_dev_id;
 	__u16 entry_nr;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4010802..d3acb37 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -280,13 +280,33 @@ static void kvm_free_assigned_irq(struct kvm *kvm,
 	 * now, the kvm state is still legal for probably we also have to wait
 	 * interrupt_work done.
 	 */
-	disable_irq_nosync(assigned_dev->host_irq);
-	cancel_work_sync(&assigned_dev->interrupt_work);
+	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
+		int i;
+		for (i = 0; i < assigned_dev->entries_nr; i++)
+			disable_irq_nosync(assigned_dev->
+					   host_msix_entries[i].vector);
+
+		cancel_work_sync(&assigned_dev->interrupt_work);
+
+		for (i = 0; i < assigned_dev->entries_nr; i++)
+			free_irq(assigned_dev->host_msix_entries[i].vector,
+				 (void *)assigned_dev);
+
+		assigned_dev->entries_nr = 0;
+		kfree(assigned_dev->host_msix_entries);
+		kfree(assigned_dev->guest_msix_entries);
+		pci_disable_msix(assigned_dev->dev);
+	} else {
+		/* Deal with MSI and INTx */
+		disable_irq_nosync(assigned_dev->host_irq);
+		cancel_work_sync(&assigned_dev->interrupt_work);
 
-	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
+		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);
+		if (assigned_dev->irq_requested_type &
+				KVM_ASSIGNED_DEV_HOST_MSI)
+			pci_disable_msi(assigned_dev->dev);
+	}
 
 	assigned_dev->irq_requested_type = 0;
 }
@@ -415,6 +435,69 @@ static int assigned_device_update_msi(struct kvm *kvm,
 	adev->irq_requested_type |= KVM_ASSIGNED_DEV_HOST_MSI;
 	return 0;
 }
+
+static int assigned_device_update_msix(struct kvm *kvm,
+			struct kvm_assigned_dev_kernel *adev,
+			struct kvm_assigned_irq *airq)
+{
+	/* TODO Deal with KVM_DEV_IRQ_ASSIGNED_MASK_MSIX */
+	int i, r;
+
+	adev->ack_notifier.gsi = -1;
+
+	if (irqchip_in_kernel(kvm)) {
+		if (airq->flags & KVM_DEV_IRQ_ASSIGN_MASK_MSIX) {
+			printk(KERN_WARNING
+			       "kvm: unsupported mask MSI-X, flags 0x%x!\n",
+			       airq->flags);
+			return 0;
+		}
+
+		if (!(airq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX)) {
+			/* Guest disable MSI-X */
+			kvm_free_assigned_irq(kvm, adev);
+			if (msi2intx) {
+				pci_enable_msi(adev->dev);
+				if (adev->dev->msi_enabled)
+					return assigned_device_update_msi(kvm,
+							adev, airq);
+			}
+			return assigned_device_update_intx(kvm, adev, airq);
+		}
+
+		/* host_msix_entries and guest_msix_entries should have been
+		 * initialized */
+
+		if (adev->entries_nr == 0) {
+			printk(KERN_ERR
+			       "kvm: MSI-X entry not initialized!\n");
+			return -EFAULT;
+		}
+
+		kvm_free_assigned_irq(kvm, adev);
+
+		r = pci_enable_msix(adev->dev, adev->host_msix_entries,
+				    adev->entries_nr);
+		if (r) {
+			printk(KERN_ERR "Fail to enable MSI-X feature!\n");
+			return r;
+		}
+
+		for (i = 0; i < adev->entries_nr; i++) {
+			r = request_irq((adev->host_msix_entries + i)->vector,
+					kvm_assigned_dev_intr, 0,
+					"kvm_assigned_msix_device",
+					(void *)adev);
+			if (r)
+				return r;
+		}
+	}
+
+	adev->irq_requested_type |= KVM_ASSIGNED_DEV_MSIX;
+
+	return 0;
+}
+
 #endif
 
 static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
@@ -461,12 +544,24 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 		}
 	}
 
-	if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
+	if (match->irq_requested_type & KVM_ASSIGNED_DEV_MSIX)
+		current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX;
+	else if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
 		 (match->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_MSI))
 		current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSI;
 
 	changed_flags = assigned_irq->flags ^ current_flags;
 
+#ifdef CONFIG_X86
+	if (changed_flags & KVM_DEV_IRQ_ASSIGN_MSIX_ACTION) {
+		r = assigned_device_update_msix(kvm, match, assigned_irq);
+		if (r) {
+			printk(KERN_WARNING "kvm: failed to execute "
+					"MSI-X action!\n");
+			goto out_release;
+		}
+	} else
+#endif
 	if ((changed_flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION) ||
 	    (msi2intx && match->dev->msi_enabled)) {
 #ifdef CONFIG_X86
-- 
1.5.4.5

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Avi Kivity]

Sheng Yang wrote:
> This patch finally enable MSI-X.
>
> What we need for MSI-X:
> 1. Intercept one page in MMIO region of device. So that we can get guest desired
> MSI-X table and set up the real one. Now this have been done by guest, and
> transfer to kernel using ioctl KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY.
>
> 2. Information for incoming interrupt. Now one device can have more than one
> interrupt, and they are all handled by one workqueue structure. So we need to
> identify them. The previous patch enable gsi_msg_pending_bitmap get this done.
>
> 3. Mapping from host IRQ to guest gsi as well as guest gsi to real MSI/MSI-X
> message address/data. We used same entry number for the host and guest here, so
> that it's easy to find the correlated guest gsi.
>
> What we lack for now:
> 1. The PCI spec said nothing can existed with MSI-X table in the same page of
> MMIO region, except pending bits. The patch ignore pending bits as the first
> step (so they are always 0 - no pending).
>
> 2. The PCI spec allowed to change MSI-X table dynamically. That means, the OS
> can enable MSI-X, then mask one MSI-X entry, modify it, and unmask it. The patch
> didn't support this, and Linux also don't work in this way.
>
> 3. The patch didn't implement MSI-X mask all and mask single entry. I would
> implement the former in driver/pci/msi.c later. And for single entry, userspace
> should have reposibility to handle it.
>
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  include/linux/kvm.h |    8 ++++
>  virt/kvm/kvm_main.c |  107 ++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 109 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index a2dfbe0..78480d0 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -440,6 +440,9 @@ struct kvm_irq_routing {
>  };
>  
>  #endif
> +#if defined(CONFIG_X86)
> +#define KVM_CAP_DEVICE_MSIX 26
> +#endif
>   

We switched to a different way of depending on CONFIG_X86, see the other 
KVM_CAP defines.

>  
>  struct kvm_assigned_msix_nr {
>  	__u32 assigned_dev_id;
>  	__u16 entry_nr;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 4010802..d3acb37 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -280,13 +280,33 @@ static void kvm_free_assigned_irq(struct kvm *kvm,
>  	 * now, the kvm state is still legal for probably we also have to wait
>  	 * interrupt_work done.
>  	 */
> -	disable_irq_nosync(assigned_dev->host_irq);
> -	cancel_work_sync(&assigned_dev->interrupt_work);
> +	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
> +		int i;
> +		for (i = 0; i < assigned_dev->entries_nr; i++)
> +			disable_irq_nosync(assigned_dev->
> +					   host_msix_entries[i].vector);
> +
> +		cancel_work_sync(&assigned_dev->interrupt_work);
> +
> +		for (i = 0; i < assigned_dev->entries_nr; i++)
> +			free_irq(assigned_dev->host_msix_entries[i].vector,
> +				 (void *)assigned_dev);
> +
> +		assigned_dev->entries_nr = 0;
> +		kfree(assigned_dev->host_msix_entries);
> +		kfree(assigned_dev->guest_msix_entries);
> +		pci_disable_msix(assigned_dev->dev);
> +	} else {
> +		/* Deal with MSI and INTx */
> +		disable_irq_nosync(assigned_dev->host_irq);
> +		cancel_work_sync(&assigned_dev->interrupt_work);
>   

How about always have an array?  That will also allow us to deal with 
INTx where x=B,C,D.

Currently for MSI and INTx the array will hold just one active element.

>  
> -	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
> +		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);
> +		if (assigned_dev->irq_requested_type &
> +				KVM_ASSIGNED_DEV_HOST_MSI)
> +			pci_disable_msi(assigned_dev->dev);
> +	}
>   

All those flags and bits are worrying me.  Maybe each entry in the array 
can have an ops member, and disabling would work by calling 
->ops->disable().

We can do that later.

>  
>  	assigned_dev->irq_requested_type = 0;
>  }
> @@ -415,6 +435,69 @@ static int assigned_device_update_msi(struct kvm *kvm,
>  	adev->irq_requested_type |= KVM_ASSIGNED_DEV_HOST_MSI;
>  	return 0;
>  }
> +
> +static int assigned_device_update_msix(struct kvm *kvm,
> +			struct kvm_assigned_dev_kernel *adev,
> +			struct kvm_assigned_irq *airq)
> +{
> +	/* TODO Deal with KVM_DEV_IRQ_ASSIGNED_MASK_MSIX */
> +	int i, r;
> +
> +	adev->ack_notifier.gsi = -1;
> +
> +	if (irqchip_in_kernel(kvm)) {
> +		if (airq->flags & KVM_DEV_IRQ_ASSIGN_MASK_MSIX) {
> +			printk(KERN_WARNING
> +			       "kvm: unsupported mask MSI-X, flags 0x%x!\n",
> +			       airq->flags);
>   

error, not printk

> +			return 0;
> +		}
> +
> +		if (!(airq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX)) {
> +			/* Guest disable MSI-X */
> +			kvm_free_assigned_irq(kvm, adev);
> +			if (msi2intx) {
> +				pci_enable_msi(adev->dev);
> +				if (adev->dev->msi_enabled)
> +					return assigned_device_update_msi(kvm,
> +							adev, airq);
> +			}
> +			return assigned_device_update_intx(kvm, adev, airq);
> +		}
> +
> +		/* host_msix_entries and guest_msix_entries should have been
> +		 * initialized */
> +
> +		if (adev->entries_nr == 0) {
> +			printk(KERN_ERR
> +			       "kvm: MSI-X entry not initialized!\n");
> +			return -EFAULT;
>   

-EFAULT is for accesses to unmapped user memory; drop the printk.

> +		}
> +
> +		kvm_free_assigned_irq(kvm, adev);
> +
> +		r = pci_enable_msix(adev->dev, adev->host_msix_entries,
> +				    adev->entries_nr);
> +		if (r) {
> +			printk(KERN_ERR "Fail to enable MSI-X feature!\n");
>   

Drop the printk.

> +			return r;
> +		}
> +
> +		for (i = 0; i < adev->entries_nr; i++) {
> +			r = request_irq((adev->host_msix_entries + i)->vector,
> +					kvm_assigned_dev_intr, 0,
> +					"kvm_assigned_msix_device",
> +					(void *)adev);
> +			if (r)
> +				return r;
> +		}
> +	}
> +
> +	adev->irq_requested_type |= KVM_ASSIGNED_DEV_MSIX;
> +
> +	return 0;
> +}
> +
>  #endif
>  
>  static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
> @@ -461,12 +544,24 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
>  		}
>  	}
>  
> -	if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
> +	if (match->irq_requested_type & KVM_ASSIGNED_DEV_MSIX)
> +		current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX;
> +	else if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
>  		 (match->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_MSI))
>  		current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSI;
>  
>  	changed_flags = assigned_irq->flags ^ current_flags;
>  
> +#ifdef CONFIG_X86
> +	if (changed_flags & KVM_DEV_IRQ_ASSIGN_MSIX_ACTION) {
> +		r = assigned_device_update_msix(kvm, match, assigned_irq);
> +		if (r) {
> +			printk(KERN_WARNING "kvm: failed to execute "
> +					"MSI-X action!\n");
> +			goto out_release;
> +		}
> +	} else
> +#endif
>  	if ((changed_flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION) ||
>  	    (msi2intx && match->dev->msi_enabled)) {
>  #ifdef CONFIG_X86
>   


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Sheng Yang]

On Wed, Feb 18, 2009 at 10:45:19AM +0000, Avi Kivity wrote:
> Sheng Yang wrote:
>> index a2dfbe0..78480d0 100644
>> --- a/include/linux/kvm.h
>> +++ b/include/linux/kvm.h
>> @@ -440,6 +440,9 @@ struct kvm_irq_routing {
>>  };
>>   #endif
>> +#if defined(CONFIG_X86)
>> +#define KVM_CAP_DEVICE_MSIX 26
>> +#endif
>>   
>
> We switched to a different way of depending on CONFIG_X86, see the other  
> KVM_CAP defines.

Thanks to point it out. :)

>
>>   struct kvm_assigned_msix_nr {
>>  	__u32 assigned_dev_id;
>>  	__u16 entry_nr;
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 4010802..d3acb37 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -280,13 +280,33 @@ static void kvm_free_assigned_irq(struct kvm *kvm,
>>  	 * now, the kvm state is still legal for probably we also have to wait
>>  	 * interrupt_work done.
>>  	 */
>> -	disable_irq_nosync(assigned_dev->host_irq);
>> -	cancel_work_sync(&assigned_dev->interrupt_work);
>> +	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
>> +		int i;
>> +		for (i = 0; i < assigned_dev->entries_nr; i++)
>> +			disable_irq_nosync(assigned_dev->
>> +					   host_msix_entries[i].vector);
>> +
>> +		cancel_work_sync(&assigned_dev->interrupt_work);
>> +
>> +		for (i = 0; i < assigned_dev->entries_nr; i++)
>> +			free_irq(assigned_dev->host_msix_entries[i].vector,
>> +				 (void *)assigned_dev);
>> +
>> +		assigned_dev->entries_nr = 0;
>> +		kfree(assigned_dev->host_msix_entries);
>> +		kfree(assigned_dev->guest_msix_entries);
>> +		pci_disable_msix(assigned_dev->dev);
>> +	} else {
>> +		/* Deal with MSI and INTx */
>> +		disable_irq_nosync(assigned_dev->host_irq);
>> +		cancel_work_sync(&assigned_dev->interrupt_work);
>>   
>
> How about always have an array?  That will also allow us to deal with  
> INTx where x=B,C,D.
>
> Currently for MSI and INTx the array will hold just one active element.

So array, or bitmap? I remember I changed it to bitmap accounding to your
first comment...

OK. I think array is reasonable, but the length is a problem, as I did before. How long would you like?

-- 
regards
Yang, Sheng

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Avi Kivity]

Sheng Yang wrote:
>
>>>   struct kvm_assigned_msix_nr {
>>>  	__u32 assigned_dev_id;
>>>  	__u16 entry_nr;
>>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>>> index 4010802..d3acb37 100644
>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -280,13 +280,33 @@ static void kvm_free_assigned_irq(struct kvm *kvm,
>>>  	 * now, the kvm state is still legal for probably we also have to wait
>>>  	 * interrupt_work done.
>>>  	 */
>>> -	disable_irq_nosync(assigned_dev->host_irq);
>>> -	cancel_work_sync(&assigned_dev->interrupt_work);
>>> +	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
>>> +		int i;
>>> +		for (i = 0; i < assigned_dev->entries_nr; i++)
>>> +			disable_irq_nosync(assigned_dev->
>>> +					   host_msix_entries[i].vector);
>>> +
>>> +		cancel_work_sync(&assigned_dev->interrupt_work);
>>> +
>>> +		for (i = 0; i < assigned_dev->entries_nr; i++)
>>> +			free_irq(assigned_dev->host_msix_entries[i].vector,
>>> +				 (void *)assigned_dev);
>>> +
>>> +		assigned_dev->entries_nr = 0;
>>> +		kfree(assigned_dev->host_msix_entries);
>>> +		kfree(assigned_dev->guest_msix_entries);
>>> +		pci_disable_msix(assigned_dev->dev);
>>> +	} else {
>>> +		/* Deal with MSI and INTx */
>>> +		disable_irq_nosync(assigned_dev->host_irq);
>>> +		cancel_work_sync(&assigned_dev->interrupt_work);
>>>   
>>>       
>> How about always have an array?  That will also allow us to deal with  
>> INTx where x=B,C,D.
>>
>> Currently for MSI and INTx the array will hold just one active element.
>>     
>
> So array, or bitmap? I remember I changed it to bitmap accounding to your
> first comment...
>   

Which bitmap?  I'm confused.

I'm talking about unifying the existing array 
(assigned_dev->host_msix_entries[]) with ->host_irq.  Also since we need 
an array for INTx when a function uses INT[BCD].

So we'll have assigned_dev->host_irqs[], each entry can be INTx or MSI 
or MSIx.

> OK. I think array is reasonable, but the length is a problem, as I did before. How long would you like?
>   

MAX(4, KVM_MAX_MSIX_ENTRIES), no?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Sheng Yang]

On Wednesday 18 February 2009 20:36:10 Avi Kivity wrote:
> Sheng Yang wrote:
> >>>   struct kvm_assigned_msix_nr {
> >>>  	__u32 assigned_dev_id;
> >>>  	__u16 entry_nr;
> >>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >>> index 4010802..d3acb37 100644
> >>> --- a/virt/kvm/kvm_main.c
> >>> +++ b/virt/kvm/kvm_main.c
> >>> @@ -280,13 +280,33 @@ static void kvm_free_assigned_irq(struct kvm
> >>> *kvm, * now, the kvm state is still legal for probably we also have to
> >>> wait * interrupt_work done.
> >>>  	 */
> >>> -	disable_irq_nosync(assigned_dev->host_irq);
> >>> -	cancel_work_sync(&assigned_dev->interrupt_work);
> >>> +	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
> >>> +		int i;
> >>> +		for (i = 0; i < assigned_dev->entries_nr; i++)
> >>> +			disable_irq_nosync(assigned_dev->
> >>> +					   host_msix_entries[i].vector);
> >>> +
> >>> +		cancel_work_sync(&assigned_dev->interrupt_work);
> >>> +
> >>> +		for (i = 0; i < assigned_dev->entries_nr; i++)
> >>> +			free_irq(assigned_dev->host_msix_entries[i].vector,
> >>> +				 (void *)assigned_dev);
> >>> +
> >>> +		assigned_dev->entries_nr = 0;
> >>> +		kfree(assigned_dev->host_msix_entries);
> >>> +		kfree(assigned_dev->guest_msix_entries);
> >>> +		pci_disable_msix(assigned_dev->dev);
> >>> +	} else {
> >>> +		/* Deal with MSI and INTx */
> >>> +		disable_irq_nosync(assigned_dev->host_irq);
> >>> +		cancel_work_sync(&assigned_dev->interrupt_work);
> >>
> >> How about always have an array?  That will also allow us to deal with
> >> INTx where x=B,C,D.
> >>
> >> Currently for MSI and INTx the array will hold just one active element.
> >
> > So array, or bitmap? I remember I changed it to bitmap accounding to your
> > first comment...
>
> Which bitmap?  I'm confused.
>
> I'm talking about unifying the existing array
> (assigned_dev->host_msix_entries[]) with ->host_irq.  Also since we need
> an array for INTx when a function uses INT[BCD].
>
> So we'll have assigned_dev->host_irqs[], each entry can be INTx or MSI
> or MSIx.
>
> > OK. I think array is reasonable, but the length is a problem, as I did
> > before. How long would you like?
>
> MAX(4, KVM_MAX_MSIX_ENTRIES), no?

Oh, yeah, I misunderstood it(wrong context)...

Need more adjustment on the type, for host_msix_entries is used with 
pci_enable_msix. So I'd like to put it a bit later.

-- 
regards
Yang, Sheng

[PATCH 0/3 v2] MSI-X enabling

[Sheng Yang]

Based on former patchset.

[PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Sheng Yang]

This patch finally enable MSI-X.

What we need for MSI-X:
1. Intercept one page in MMIO region of device. So that we can get guest desired
MSI-X table and set up the real one. Now this have been done by guest, and
transfer to kernel using ioctl KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY.

2. Information for incoming interrupt. Now one device can have more than one
interrupt, and they are all handled by one workqueue structure. So we need to
identify them. The previous patch enable gsi_msg_pending_bitmap get this done.

3. Mapping from host IRQ to guest gsi as well as guest gsi to real MSI/MSI-X
message address/data. We used same entry number for the host and guest here, so
that it's easy to find the correlated guest gsi.

What we lack for now:
1. The PCI spec said nothing can existed with MSI-X table in the same page of
MMIO region, except pending bits. The patch ignore pending bits as the first
step (so they are always 0 - no pending).

2. The PCI spec allowed to change MSI-X table dynamically. That means, the OS
can enable MSI-X, then mask one MSI-X entry, modify it, and unmask it. The patch
didn't support this, and Linux also don't work in this way.

3. The patch didn't implement MSI-X mask all and mask single entry. I would
implement the former in driver/pci/msi.c later. And for single entry, userspace
should have reposibility to handle it.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm.h |    8 ++++
 virt/kvm/kvm_main.c |  107 ++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 109 insertions(+), 6 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 5200768..fcd08da 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -439,6 +439,9 @@ struct kvm_irq_routing {
 };
 
 #endif
+#if defined(CONFIG_X86)
+#define KVM_CAP_DEVICE_MSIX 26
+#endif
 
 /*
  * ioctls for VM fds
@@ -596,6 +599,11 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
 
+#define KVM_DEV_IRQ_ASSIGN_MSIX_ACTION  (KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX |\
+					KVM_DEV_IRQ_ASSIGN_MASK_MSIX)
+#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX  (1 << 1)
+#define KVM_DEV_IRQ_ASSIGN_MASK_MSIX    (1 << 2)
+
 struct kvm_assigned_msix_nr {
 	__u32 assigned_dev_id;
 	__u16 entry_nr;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 961603f..c774027 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -281,13 +281,33 @@ static void kvm_free_assigned_irq(struct kvm *kvm,
 	 * now, the kvm state is still legal for probably we also have to wait
 	 * interrupt_work done.
 	 */
-	disable_irq_nosync(assigned_dev->host_irq);
-	cancel_work_sync(&assigned_dev->interrupt_work);
+	if (assigned_dev->irq_requested_type & KVM_ASSIGNED_DEV_MSIX) {
+		int i;
+		for (i = 0; i < assigned_dev->entries_nr; i++)
+			disable_irq_nosync(assigned_dev->
+					   host_msix_entries[i].vector);
+
+		cancel_work_sync(&assigned_dev->interrupt_work);
+
+		for (i = 0; i < assigned_dev->entries_nr; i++)
+			free_irq(assigned_dev->host_msix_entries[i].vector,
+				 (void *)assigned_dev);
+
+		assigned_dev->entries_nr = 0;
+		kfree(assigned_dev->host_msix_entries);
+		kfree(assigned_dev->guest_msix_entries);
+		pci_disable_msix(assigned_dev->dev);
+	} else {
+		/* Deal with MSI and INTx */
+		disable_irq_nosync(assigned_dev->host_irq);
+		cancel_work_sync(&assigned_dev->interrupt_work);
 
-	free_irq(assigned_dev->host_irq, (void *)assigned_dev);
+		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);
+		if (assigned_dev->irq_requested_type &
+				KVM_ASSIGNED_DEV_HOST_MSI)
+			pci_disable_msi(assigned_dev->dev);
+	}
 
 	assigned_dev->irq_requested_type = 0;
 }
@@ -416,6 +436,69 @@ static int assigned_device_update_msi(struct kvm *kvm,
 	adev->irq_requested_type |= KVM_ASSIGNED_DEV_HOST_MSI;
 	return 0;
 }
+
+static int assigned_device_update_msix(struct kvm *kvm,
+			struct kvm_assigned_dev_kernel *adev,
+			struct kvm_assigned_irq *airq)
+{
+	/* TODO Deal with KVM_DEV_IRQ_ASSIGNED_MASK_MSIX */
+	int i, r;
+
+	adev->ack_notifier.gsi = -1;
+
+	if (irqchip_in_kernel(kvm)) {
+		if (airq->flags & KVM_DEV_IRQ_ASSIGN_MASK_MSIX) {
+			printk(KERN_WARNING
+			       "kvm: unsupported mask MSI-X, flags 0x%x!\n",
+			       airq->flags);
+			return 0;
+		}
+
+		if (!(airq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX)) {
+			/* Guest disable MSI-X */
+			kvm_free_assigned_irq(kvm, adev);
+			if (msi2intx) {
+				pci_enable_msi(adev->dev);
+				if (adev->dev->msi_enabled)
+					return assigned_device_update_msi(kvm,
+							adev, airq);
+			}
+			return assigned_device_update_intx(kvm, adev, airq);
+		}
+
+		/* host_msix_entries and guest_msix_entries should have been
+		 * initialized */
+
+		if (adev->entries_nr == 0) {
+			printk(KERN_ERR
+			       "kvm: MSI-X entry not initialized!\n");
+			return -EFAULT;
+		}
+
+		kvm_free_assigned_irq(kvm, adev);
+
+		r = pci_enable_msix(adev->dev, adev->host_msix_entries,
+				    adev->entries_nr);
+		if (r) {
+			printk(KERN_ERR "Fail to enable MSI-X feature!\n");
+			return r;
+		}
+
+		for (i = 0; i < adev->entries_nr; i++) {
+			r = request_irq((adev->host_msix_entries + i)->vector,
+					kvm_assigned_dev_intr, 0,
+					"kvm_assigned_msix_device",
+					(void *)adev);
+			if (r)
+				return r;
+		}
+	}
+
+	adev->irq_requested_type |= KVM_ASSIGNED_DEV_MSIX;
+
+	return 0;
+}
+
 #endif
 
 static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
@@ -462,12 +545,24 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 		}
 	}
 
-	if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
+	if (match->irq_requested_type & KVM_ASSIGNED_DEV_MSIX)
+		current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSIX;
+	else if ((match->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) &&
 		 (match->irq_requested_type & KVM_ASSIGNED_DEV_GUEST_MSI))
 		current_flags |= KVM_DEV_IRQ_ASSIGN_ENABLE_MSI;
 
 	changed_flags = assigned_irq->flags ^ current_flags;
 
+#ifdef CONFIG_X86
+	if (changed_flags & KVM_DEV_IRQ_ASSIGN_MSIX_ACTION) {
+		r = assigned_device_update_msix(kvm, match, assigned_irq);
+		if (r) {
+			printk(KERN_WARNING "kvm: failed to execute "
+					"MSI-X action!\n");
+			goto out_release;
+		}
+	} else
+#endif
 	if ((changed_flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION) ||
 	    (msi2intx && match->dev->msi_enabled)) {
 #ifdef CONFIG_X86
-- 
1.5.4.5

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Avi Kivity]

Sheng Yang wrote:
> This patch finally enable MSI-X.
>
> What we need for MSI-X:
> 1. Intercept one page in MMIO region of device. So that we can get guest desired
> MSI-X table and set up the real one. Now this have been done by guest, and
> transfer to kernel using ioctl KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY.
>
>   

Looking at the patch, you're doing that in userspace.  Good.

> 2. The PCI spec allowed to change MSI-X table dynamically. That means, the OS
> can enable MSI-X, then mask one MSI-X entry, modify it, and unmask it. The patch
> didn't support this, and Linux also don't work in this way.
>   

What about Windows?


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

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Sheng Yang]

On Wednesday 11 February 2009 20:48:55 Avi Kivity wrote:
> Sheng Yang wrote:
> > This patch finally enable MSI-X.
> >
> > What we need for MSI-X:
> > 1. Intercept one page in MMIO region of device. So that we can get guest
> > desired MSI-X table and set up the real one. Now this have been done by
> > guest, and transfer to kernel using ioctl KVM_SET_MSIX_NR and
> > KVM_SET_MSIX_ENTRY.
>
> Looking at the patch, you're doing that in userspace.  Good.
>
> > 2. The PCI spec allowed to change MSI-X table dynamically. That means,
> > the OS can enable MSI-X, then mask one MSI-X entry, modify it, and unmask
> > it. The patch didn't support this, and Linux also don't work in this way.
>
> What about Windows?

Not sure. Don't have Windows driver to test... And only Vista and later 
support MSI/MSI-X, not sure it would be so aggressive... We can add some debug 
if such condition happened.

-- 
regards
Yang, Sheng

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Marcelo Tosatti]

On Wed, Feb 11, 2009 at 04:08:51PM +0800, Sheng Yang wrote:
> This patch finally enable MSI-X.
> 
> What we need for MSI-X:
> 1. Intercept one page in MMIO region of device. So that we can get guest desired
> MSI-X table and set up the real one. Now this have been done by guest, and
> transfer to kernel using ioctl KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY.
> 
> 2. Information for incoming interrupt. Now one device can have more than one
> interrupt, and they are all handled by one workqueue structure. So we need to
> identify them. The previous patch enable gsi_msg_pending_bitmap get this done.
> 
> 3. Mapping from host IRQ to guest gsi as well as guest gsi to real MSI/MSI-X
> message address/data. We used same entry number for the host and guest here, so
> that it's easy to find the correlated guest gsi.
> 
> What we lack for now:
> 1. The PCI spec said nothing can existed with MSI-X table in the same page of
> MMIO region, except pending bits. The patch ignore pending bits as the first
> step (so they are always 0 - no pending).
> 
> 2. The PCI spec allowed to change MSI-X table dynamically. That means, the OS
> can enable MSI-X, then mask one MSI-X entry, modify it, and unmask it. The patch
> didn't support this, and Linux also don't work in this way.

Have you thought about supporting this with the current ioctl interface?
Point is that once ioctl's are in and userspace code uses it, you can't
change. So:

- If userspace calls kvm_vm_ioctl_set_msix_entry twice don't you leak
  memory (that is a bug actually unless I'm mistaken)? So in the future
  kvm_vm_ioctl_set_msix_entry will deallocate the currently allocated
  guest/host entries and replace with userspace supplied entries?

- interrupt context can read the table while kvm_vm_ioctl_set_msix_entry
  is modifying it. So you either need to forbid more than one
  kvm_vm_ioctl_set_msix_entry call in the lifetime of a guest (which
  you can later allow when you support MSI table change), or handle 
  accesses from multiple contexes now. It seems forbidding is enough for
  the moment from what you said.

But in general looks good to me (not familiar with the internals of
pci / msi).

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Sheng Yang]

On Friday 13 February 2009 04:40:05 Marcelo Tosatti wrote:
> On Wed, Feb 11, 2009 at 04:08:51PM +0800, Sheng Yang wrote:
> > This patch finally enable MSI-X.
> >
> > What we need for MSI-X:
> > 1. Intercept one page in MMIO region of device. So that we can get guest
> > desired MSI-X table and set up the real one. Now this have been done by
> > guest, and transfer to kernel using ioctl KVM_SET_MSIX_NR and
> > KVM_SET_MSIX_ENTRY.
> >
> > 2. Information for incoming interrupt. Now one device can have more than
> > one interrupt, and they are all handled by one workqueue structure. So we
> > need to identify them. The previous patch enable gsi_msg_pending_bitmap
> > get this done.
> >
> > 3. Mapping from host IRQ to guest gsi as well as guest gsi to real
> > MSI/MSI-X message address/data. We used same entry number for the host
> > and guest here, so that it's easy to find the correlated guest gsi.
> >
> > What we lack for now:
> > 1. The PCI spec said nothing can existed with MSI-X table in the same
> > page of MMIO region, except pending bits. The patch ignore pending bits
> > as the first step (so they are always 0 - no pending).
> >
> > 2. The PCI spec allowed to change MSI-X table dynamically. That means,
> > the OS can enable MSI-X, then mask one MSI-X entry, modify it, and unmask
> > it. The patch didn't support this, and Linux also don't work in this way.
>
> Have you thought about supporting this with the current ioctl interface?
> Point is that once ioctl's are in and userspace code uses it, you can't
> change. So:
>
> - If userspace calls kvm_vm_ioctl_set_msix_entry twice don't you leak
>   memory (that is a bug actually unless I'm mistaken)? So in the future
>   kvm_vm_ioctl_set_msix_entry will deallocate the currently allocated
>   guest/host entries and replace with userspace supplied entries?
>

The allocation only happen once, at the second time it would report error in 
current code. But allocate/deallocate is also acceptable for future.

> - interrupt context can read the table while kvm_vm_ioctl_set_msix_entry
>   is modifying it. So you either need to forbid more than one
>   kvm_vm_ioctl_set_msix_entry call in the lifetime of a guest (which
>   you can later allow when you support MSI table change), or handle
>   accesses from multiple contexes now. It seems forbidding is enough for
>   the moment from what you said.

Yeah.

But for the modifying the MSI-X table, the most critical problem is, current 
Linux didn't support it IIRC. So I have to disable MSI-X then enable it again 
with new table, and it would result in lost interrupt.

So seems the most reasonable method is to modify pci_enable_msix() and related 
function's action to support this...

-- 
regards
Yang, Sheng

>
> But in general looks good to me (not familiar with the internals of
> pci / msi).

Re: [PATCH 3/3] KVM: Enable MSI-X for KVM assigned device

[Marcelo Tosatti]

On Fri, Feb 13, 2009 at 01:29:42PM +0800, Sheng Yang wrote:
> The allocation only happen once, at the second time it would report error in 
> current code. But allocate/deallocate is also acceptable for future.

Oops, it OK. 

> > - interrupt context can read the table while kvm_vm_ioctl_set_msix_entry
> >   is modifying it. So you either need to forbid more than one
> >   kvm_vm_ioctl_set_msix_entry call in the lifetime of a guest (which
> >   you can later allow when you support MSI table change), or handle
> >   accesses from multiple contexes now. It seems forbidding is enough for
> >   the moment from what you said.
> 
> Yeah.
> 
> But for the modifying the MSI-X table, the most critical problem is, current 
> Linux didn't support it IIRC. So I have to disable MSI-X then enable it again 
> with new table, and it would result in lost interrupt.
> 
> So seems the most reasonable method is to modify pci_enable_msix() and related 
> function's action to support this...

Alright, then just reply "[PATCH 1/3] KVM: Ioctls for init MSI-X entry"
with the minor comments reworked so Avi can apply.

Thanks.