[PATCH 0/8] MSI enhancement

[PATCH 0/8] MSI enhancement

[Sheng Yang]

Hi Avi

Here is the first part of MSI-X enabling patchset. This one:

1. Add gsi_msg mapping mechanism, which gsi can used to indicated a MSI
interrupt.(Notice API/ABI changed a little, but we don't have userspace patch
now, so it should be OK.)

2. Provide MSI disable capability.

The upcoming MSI-X enabling patchset depends on this one. I splited them into
two batches just because it's too large...

Thanks!
--
regards
Yang, Sheng

[PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq

[Sheng Yang]

For MSI disable feature later.

Notice I changed ABI here, but due to no userspace patch, I think it's OK.

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

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index ef7f98e..5b965f6 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -544,6 +544,7 @@ struct kvm_assigned_irq {
 
 #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
 
-#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
+#define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	(1 << 0)
+#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 1)
 
 #endif
-- 
1.5.4.5

[PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq

[Sheng Yang]

Which is more convenient...

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ffd261d..cd84b3e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -284,11 +284,7 @@ static int assigned_device_update_intx(struct kvm *kvm,
 		return 0;
 
 	if (irqchip_in_kernel(kvm)) {
-		if (!msi2intx &&
-		    adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) {
-			free_irq(adev->host_irq, (void *)kvm);
-			pci_disable_msi(adev->dev);
-		}
+		kvm_free_assigned_irq(kvm, adev);
 
 		if (!capable(CAP_SYS_RAWIO))
 			return -EPERM;
@@ -339,9 +335,7 @@ static int assigned_device_update_msi(struct kvm *kvm,
 
 	if (irqchip_in_kernel(kvm)) {
 		if (!msi2intx) {
-			if (adev->irq_requested_type &
-					KVM_ASSIGNED_DEV_HOST_INTX)
-				free_irq(adev->host_irq, (void *)adev);
+			kvm_free_assigned_irq(kvm, adev);
 
 			r = pci_enable_msi(adev->dev);
 			if (r)
-- 
1.5.4.5

[PATCH 3/8] KVM: Add support to disable MSI for assigned device

[Sheng Yang]

In msi2intx mode, MSI is always enabled. But for non-msi2intx mode, we have to
disable MSI if guest require to do so.

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cd84b3e..10cf7e1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -328,6 +328,15 @@ static int assigned_device_update_msi(struct kvm *kvm,
 		adev->irq_requested_type &= ~KVM_ASSIGNED_DEV_GUEST_MSI;
 		adev->guest_irq = airq->guest_irq;
 		adev->ack_notifier.gsi = airq->guest_irq;
+	} else {
+		/*
+		 * Guest require to disable device MSI, we disable MSI and
+		 * re-enable INTx by default again. Notice it's only for
+		 * non-msi2intx.
+		 */
+		kvm_free_assigned_irq(kvm, adev);
+		assigned_device_update_intx(kvm, adev, airq);
+		return 0;
 	}
 
 	if (adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI)
@@ -400,7 +409,7 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
 	}
 
 	if ((!msi2intx &&
-	     (assigned_irq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSI)) ||
+	     (assigned_irq->flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION)) ||
 	    (msi2intx && match->dev->msi_enabled)) {
 #ifdef CONFIG_X86
 		r = assigned_device_update_msi(kvm, match, assigned_irq);
-- 
1.5.4.5

[PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI

[Sheng Yang]

Avi's purpose, to use single kvm_set_irq() to deal with all interrupt, including
MSI. So here is it.

struct gsi_msg is a mapping from a special gsi(with KVM_GSI_MSG_MASK) to
MSI/MSI-X message address/data.

Now we support up to 256 gsi_msg mapping, and gsi_msg is allocated by kernel and
provide two ioctls to userspace, which is more flexiable.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm.h      |   12 ++++++++
 include/linux/kvm_host.h |   16 ++++++++++
 virt/kvm/irq_comm.c      |   70 ++++++++++++++++++++++++++++++++++++++++++++++
 virt/kvm/kvm_main.c      |   63 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 161 insertions(+), 0 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 5b965f6..b091a86 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -394,6 +394,7 @@ struct kvm_trace_rec {
 #define KVM_CAP_USER_NMI 22
 #endif
 #define KVM_CAP_SET_GUEST_DEBUG 23
+#define KVM_CAP_GSI_MSG 24
 
 /*
  * ioctls for VM fds
@@ -427,6 +428,8 @@ struct kvm_trace_rec {
 				   struct kvm_assigned_pci_dev)
 #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
 			    struct kvm_assigned_irq)
+#define KVM_REQUEST_GSI_MSG _IOWR(KVMIO, 0x71, struct kvm_assigned_gsi_msg)
+#define KVM_FREE_GSI_MSG _IOR(KVMIO, 0x72, __u32)
 
 /*
  * ioctls for vcpu fds
@@ -547,4 +550,13 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	(1 << 0)
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 1)
 
+struct kvm_assigned_gsi_msg {
+	__u32 gsi;
+	struct {
+		__u32 addr_lo;
+		__u32 addr_hi;
+		__u32 data;
+	} msg;
+};
+
 #endif
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index d63e9a4..0e5741a 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -132,6 +132,10 @@ struct kvm {
 	unsigned long mmu_notifier_seq;
 	long mmu_notifier_count;
 #endif
+	struct hlist_head gsi_msg_list;
+	struct mutex gsi_msg_lock;
+#define KVM_NR_GSI_MSG	    256
+	DECLARE_BITMAP(gsi_msg_bitmap, KVM_NR_GSI_MSG);
 };
 
 /* The guest did something we don't support. */
@@ -319,6 +323,14 @@ struct kvm_assigned_dev_kernel {
 	struct pci_dev *dev;
 	struct kvm *kvm;
 };
+
+#define KVM_GSI_MSG_MASK    0x1000000ull
+struct kvm_gsi_msg {
+	u32 gsi;
+	struct msi_msg msg;
+	struct hlist_node link;
+};
+
 void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
@@ -326,6 +338,10 @@ void kvm_register_irq_ack_notifier(struct kvm *kvm,
 void kvm_unregister_irq_ack_notifier(struct kvm_irq_ack_notifier *kian);
 int kvm_request_irq_source_id(struct kvm *kvm);
 void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id);
+int kvm_update_gsi_msg(struct kvm *kvm, struct kvm_gsi_msg *gsi_msg);
+struct kvm_gsi_msg *kvm_find_gsi_msg(struct kvm *kvm, u32 gsi);
+void kvm_free_gsi_msg(struct kvm *kvm, struct kvm_gsi_msg *gsi_msg);
+void kvm_free_gsi_msg_list(struct kvm *kvm);
 
 #ifdef CONFIG_DMAR
 int kvm_iommu_map_pages(struct kvm *kvm, gfn_t base_gfn,
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index aa5d1e5..e95ec3f 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -99,3 +99,73 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id)
 		clear_bit(irq_source_id, &kvm->arch.irq_states[i]);
 	clear_bit(irq_source_id, &kvm->arch.irq_sources_bitmap);
 }
+
+int kvm_update_gsi_msg(struct kvm *kvm, struct kvm_gsi_msg *gsi_msg)
+{
+	struct kvm_gsi_msg *found_msg, *new_gsi_msg;
+	int r, gsi;
+
+	mutex_lock(&kvm->gsi_msg_lock);
+	/* Find whether we need a update or a new entry */
+	found_msg = kvm_find_gsi_msg(kvm, gsi_msg->gsi);
+	if (found_msg)
+		*found_msg = *gsi_msg;
+	else {
+		gsi = find_first_zero_bit(kvm->gsi_msg_bitmap, KVM_NR_GSI_MSG);
+		if (gsi >= KVM_NR_GSI_MSG) {
+			r = -EFAULT;
+			goto out;
+		}
+		__set_bit(gsi, kvm->gsi_msg_bitmap);
+		gsi_msg->gsi = gsi | KVM_GSI_MSG_MASK;
+		new_gsi_msg = kzalloc(sizeof(*new_gsi_msg), GFP_KERNEL);
+		if (!new_gsi_msg) {
+			r = -ENOMEM;
+			goto out;
+		}
+		*new_gsi_msg = *gsi_msg;
+		hlist_add_head(&new_gsi_msg->link, &kvm->gsi_msg_list);
+	}
+	r = 0;
+out:
+	mutex_unlock(&kvm->gsi_msg_lock);
+	return r;
+}
+
+/* Call with kvm->gsi_msg_lock hold */
+struct kvm_gsi_msg *kvm_find_gsi_msg(struct kvm *kvm, u32 gsi)
+{
+	struct kvm_gsi_msg *gsi_msg;
+	struct hlist_node *n;
+
+	if (!(gsi & KVM_GSI_MSG_MASK))
+		return NULL;
+	hlist_for_each_entry(gsi_msg, n, &kvm->gsi_msg_list, link)
+		if (gsi_msg->gsi == gsi)
+			goto out;
+	gsi_msg = NULL;
+out:
+	return gsi_msg;
+}
+
+/* Call with kvm->gsi_msg_lock hold */
+void kvm_free_gsi_msg(struct kvm *kvm, struct kvm_gsi_msg *gsi_msg)
+{
+	if (!gsi_msg)
+		return;
+	__clear_bit(gsi_msg->gsi & ~KVM_GSI_MSG_MASK, kvm->gsi_msg_bitmap);
+	hlist_del(&gsi_msg->link);
+	kfree(gsi_msg);
+}
+
+void kvm_free_gsi_msg_list(struct kvm *kvm)
+{
+	struct kvm_gsi_msg *gsi_msg;
+	struct hlist_node *pos, *n;
+
+	mutex_lock(&kvm->gsi_msg_lock);
+	hlist_for_each_entry_safe(gsi_msg, pos, n, &kvm->gsi_msg_list, link)
+		kvm_free_gsi_msg(kvm, gsi_msg);
+	mutex_unlock(&kvm->gsi_msg_lock);
+}
+
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 10cf7e1..db9de47 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -814,6 +814,8 @@ static struct kvm *kvm_create_vm(void)
 #ifdef KVM_COALESCED_MMIO_PAGE_OFFSET
 	kvm_coalesced_mmio_init(kvm);
 #endif
+	INIT_HLIST_HEAD(&kvm->gsi_msg_list);
+	mutex_init(&kvm->gsi_msg_lock);
 out:
 	return kvm;
 }
@@ -851,6 +853,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
 {
 	struct mm_struct *mm = kvm->mm;
 
+	kvm_free_gsi_msg_list(kvm);
 	spin_lock(&kvm_lock);
 	list_del(&kvm->vm_list);
 	spin_unlock(&kvm_lock);
@@ -1579,6 +1582,42 @@ static int kvm_vcpu_ioctl_set_sigmask(struct kvm_vcpu *vcpu, sigset_t *sigset)
 	return 0;
 }
 
+static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
+					struct kvm_assigned_gsi_msg *agsi_msg)
+{
+	struct kvm_gsi_msg gsi_msg;
+	int r;
+
+	gsi_msg.gsi = agsi_msg->gsi;
+	gsi_msg.msg.address_lo = agsi_msg->msg.addr_lo;
+	gsi_msg.msg.address_hi = agsi_msg->msg.addr_hi;
+	gsi_msg.msg.data = agsi_msg->msg.data;
+
+	r = kvm_update_gsi_msg(kvm, &gsi_msg);
+	if (r == 0)
+		agsi_msg->gsi = gsi_msg.gsi;
+	return r;
+}
+
+static int kvm_vm_ioctl_free_gsi_msg(struct kvm *kvm, u32 gsi)
+{
+	struct kvm_gsi_msg *gsi_msg;
+	int r;
+
+	mutex_lock(&kvm->gsi_msg_lock);
+	gsi_msg = kvm_find_gsi_msg(kvm, gsi);
+	if (!gsi_msg) {
+		printk(KERN_WARNING "kvm: non-exist gsi->msi_msg mapping!");
+		r = -EINVAL;
+		goto out;
+	}
+	kvm_free_gsi_msg(kvm, gsi_msg);
+	r = 0;
+out:
+	mutex_unlock(&kvm->gsi_msg_lock);
+	return r;
+}
+
 static long kvm_vcpu_ioctl(struct file *filp,
 			   unsigned int ioctl, unsigned long arg)
 {
@@ -1861,6 +1900,30 @@ static long kvm_vm_ioctl(struct file *filp,
 		break;
 	}
 #endif
+	case KVM_REQUEST_GSI_MSG: {
+		struct kvm_assigned_gsi_msg agsi_msg;
+		r = -EFAULT;
+		if (copy_from_user(&agsi_msg, argp, sizeof agsi_msg))
+			goto out;
+		r = kvm_vm_ioctl_request_gsi_msg(kvm, &agsi_msg);
+		if (r)
+			goto out;
+		r = -EFAULT;
+		if (copy_to_user(argp, &agsi_msg, sizeof agsi_msg))
+			goto out;
+		r = 0;
+		break;
+	}
+	case KVM_FREE_GSI_MSG: {
+		unsigned long guest_gsi;
+		r = -EFAULT;
+		if (copy_from_user(&guest_gsi, argp, sizeof guest_gsi))
+			goto out;
+		r = kvm_vm_ioctl_free_gsi_msg(kvm, guest_gsi);
+		if (r)
+			goto out;
+		break;
+	}
 	default:
 		r = kvm_arch_vm_ioctl(filp, ioctl, arg);
 	}
-- 
1.5.4.5

[PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment

[Sheng Yang]

Convert MSI userspace interface to support gsi_msg mapping(and nobody should
be the user of the old interface...).

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

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 0e5741a..aa2606b 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -313,7 +313,6 @@ struct kvm_assigned_dev_kernel {
 	int host_irq;
 	bool host_irq_disabled;
 	int guest_irq;
-	struct msi_msg guest_msi;
 #define KVM_ASSIGNED_DEV_GUEST_INTX	(1 << 0)
 #define KVM_ASSIGNED_DEV_GUEST_MSI	(1 << 1)
 #define KVM_ASSIGNED_DEV_HOST_INTX	(1 << 8)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index db9de47..50b3ff6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -92,20 +92,28 @@ static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
 	int vcpu_id;
 	struct kvm_vcpu *vcpu;
 	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
-	int dest_id = (dev->guest_msi.address_lo & MSI_ADDR_DEST_ID_MASK)
-			>> MSI_ADDR_DEST_ID_SHIFT;
-	int vector = (dev->guest_msi.data & MSI_DATA_VECTOR_MASK)
-			>> MSI_DATA_VECTOR_SHIFT;
-	int dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT,
-				(unsigned long *)&dev->guest_msi.address_lo);
-	int trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT,
-				(unsigned long *)&dev->guest_msi.data);
-	int delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
-				(unsigned long *)&dev->guest_msi.data);
+	struct kvm_gsi_msg *gsi_msg =
+			kvm_find_gsi_msg(dev->kvm, dev->guest_irq);
+	int dest_id, vector, dest_mode, trig_mode, delivery_mode;
 	u32 deliver_bitmask;
 
 	BUG_ON(!ioapic);
 
+	if (!gsi_msg) {
+		printk(KERN_WARNING "kvm: fail to find correlated gsi_msg\n");
+		return;
+	}
+
+	dest_id = (gsi_msg->msg.address_lo & MSI_ADDR_DEST_ID_MASK)
+			>> MSI_ADDR_DEST_ID_SHIFT;
+	vector = (gsi_msg->msg.data & MSI_DATA_VECTOR_MASK)
+			>> MSI_DATA_VECTOR_SHIFT;
+	dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT,
+				(unsigned long *)&gsi_msg->msg.address_lo);
+	trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT,
+				(unsigned long *)&gsi_msg->msg.data);
+	delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
+				(unsigned long *)&gsi_msg->msg.data);
 	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
 				dest_id, dest_mode);
 	/* IOAPIC delivery mode value is the same as MSI here */
@@ -316,17 +324,16 @@ static int assigned_device_update_msi(struct kvm *kvm,
 {
 	int r;
 
+	adev->guest_irq = airq->guest_irq;
+
 	if (airq->flags & KVM_DEV_IRQ_ASSIGN_ENABLE_MSI) {
 		/* x86 don't care upper address of guest msi message addr */
 		adev->irq_requested_type |= KVM_ASSIGNED_DEV_GUEST_MSI;
 		adev->irq_requested_type &= ~KVM_ASSIGNED_DEV_GUEST_INTX;
-		adev->guest_msi.address_lo = airq->guest_msi.addr_lo;
-		adev->guest_msi.data = airq->guest_msi.data;
 		adev->ack_notifier.gsi = -1;
 	} else if (msi2intx) {
 		adev->irq_requested_type |= KVM_ASSIGNED_DEV_GUEST_INTX;
 		adev->irq_requested_type &= ~KVM_ASSIGNED_DEV_GUEST_MSI;
-		adev->guest_irq = airq->guest_irq;
 		adev->ack_notifier.gsi = airq->guest_irq;
 	} else {
 		/*
-- 
1.5.4.5

[PATCH 6/8] KVM: Improve MSI dispatch function

[Sheng Yang]

Prepare to merge with kvm_set_irq().

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 50b3ff6..49848cb 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -87,13 +87,13 @@ static bool kvm_rebooting;
 #ifdef KVM_CAP_DEVICE_ASSIGNMENT
 
 #ifdef CONFIG_X86
-static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
+static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev, u32 gsi)
 {
 	int vcpu_id;
 	struct kvm_vcpu *vcpu;
 	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
 	struct kvm_gsi_msg *gsi_msg =
-			kvm_find_gsi_msg(dev->kvm, dev->guest_irq);
+			kvm_find_gsi_msg(dev->kvm, gsi);
 	int dest_id, vector, dest_mode, trig_mode, delivery_mode;
 	u32 deliver_bitmask;
 
@@ -141,7 +141,7 @@ static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
 	}
 }
 #else
-static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev) {}
+static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev, u32 gsi) {}
 #endif
 
 static struct kvm_assigned_dev_kernel *kvm_find_assigned_dev(struct list_head *head,
@@ -176,7 +176,7 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 			    assigned_dev->guest_irq, 1);
 	else if (assigned_dev->irq_requested_type &
 				KVM_ASSIGNED_DEV_GUEST_MSI) {
-		assigned_device_msi_dispatch(assigned_dev);
+		assigned_device_msi_dispatch(assigned_dev, assigned_dev->guest_irq);
 		enable_irq(assigned_dev->host_irq);
 		assigned_dev->host_irq_disabled = false;
 	}
-- 
1.5.4.5

[PATCH 7/8] KVM: Using ioapic_irqchip() macro for kvm_set_irq

[Sheng Yang]

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

diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index e95ec3f..f7dcf70 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -39,7 +39,7 @@ void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
 	 * IOAPIC.  So set the bit in both. The guest will ignore
 	 * writes to the unused one.
 	 */
-	kvm_ioapic_set_irq(kvm->arch.vioapic, irq, !!(*irq_state));
+	kvm_ioapic_set_irq(ioapic_irqchip(kvm), irq, !!(*irq_state));
 #ifdef CONFIG_X86
 	kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
 #endif
-- 
1.5.4.5

[PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq

[Sheng Yang]

Using kvm_set_irq to handle all interrupt injection.

Signed-off-by: Sheng Yang <sheng@linux.intel.com>
---
 include/linux/kvm_host.h |    2 +-
 virt/kvm/irq_comm.c      |   96 ++++++++++++++++++++++++++++++++++++++-------
 virt/kvm/kvm_main.c      |   75 +++---------------------------------
 3 files changed, 88 insertions(+), 85 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index aa2606b..5b671b6 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -330,7 +330,7 @@ struct kvm_gsi_msg {
 	struct hlist_node link;
 };
 
-void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level);
+void kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 gsi, int level);
 void kvm_notify_acked_irq(struct kvm *kvm, unsigned gsi);
 void kvm_register_irq_ack_notifier(struct kvm *kvm,
 				   struct kvm_irq_ack_notifier *kian);
diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
index f7dcf70..9ca258f 100644
--- a/virt/kvm/irq_comm.c
+++ b/virt/kvm/irq_comm.c
@@ -20,28 +20,94 @@
  */
 
 #include <linux/kvm_host.h>
+
+#ifdef CONFIG_X86
+#include <asm/msidef.h>
+#endif
+
 #include "irq.h"
 
 #include "ioapic.h"
 
 /* This should be called with the kvm->lock mutex held */
-void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
+void kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 gsi, int level)
 {
-	unsigned long *irq_state = (unsigned long *)&kvm->arch.irq_states[irq];
-
-	/* Logical OR for level trig interrupt */
-	if (level)
-		set_bit(irq_source_id, irq_state);
-	else
-		clear_bit(irq_source_id, irq_state);
-
-	/* Not possible to detect if the guest uses the PIC or the
-	 * IOAPIC.  So set the bit in both. The guest will ignore
-	 * writes to the unused one.
-	 */
-	kvm_ioapic_set_irq(ioapic_irqchip(kvm), irq, !!(*irq_state));
+	unsigned long *irq_state;
+#ifdef CONFIG_X86
+	int vcpu_id;
+	struct kvm_vcpu *vcpu;
+	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
+	struct kvm_gsi_msg *gsi_msg;
+	int dest_id, vector, dest_mode, trig_mode, delivery_mode;
+	u32 deliver_bitmask;
+
+	BUG_ON(!ioapic);
+#endif
+
+	if (!(gsi & KVM_GSI_MSG_MASK)) {
+		int irq = gsi;
+
+		irq_state = (unsigned long *)&kvm->arch.irq_states[irq];
+
+		/* Logical OR for level trig interrupt */
+		if (level)
+			set_bit(irq_source_id, irq_state);
+		else
+			clear_bit(irq_source_id, irq_state);
+
+		/* Not possible to detect if the guest uses the PIC or the
+		 * IOAPIC.  So set the bit in both. The guest will ignore
+		 * writes to the unused one.
+		 */
+		kvm_ioapic_set_irq(ioapic, irq, !!(*irq_state));
+#ifdef CONFIG_X86
+		kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
+#endif
+		return;
+	}
+
 #ifdef CONFIG_X86
-	kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
+	gsi_msg = kvm_find_gsi_msg(kvm, gsi);
+	if (!gsi_msg) {
+		printk(KERN_WARNING "kvm: fail to find correlated gsi_msg\n");
+		return;
+	}
+
+	dest_id = (gsi_msg->msg.address_lo & MSI_ADDR_DEST_ID_MASK)
+			>> MSI_ADDR_DEST_ID_SHIFT;
+	vector = (gsi_msg->msg.data & MSI_DATA_VECTOR_MASK)
+			>> MSI_DATA_VECTOR_SHIFT;
+	dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT,
+				(unsigned long *)&gsi_msg->msg.address_lo);
+	trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT,
+				(unsigned long *)&gsi_msg->msg.data);
+	delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
+				(unsigned long *)&gsi_msg->msg.data);
+	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
+				dest_id, dest_mode);
+	/* IOAPIC delivery mode value is the same as MSI here */
+	switch (delivery_mode) {
+	case IOAPIC_LOWEST_PRIORITY:
+		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 IOAPIC_FIXED:
+		for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) {
+			if (!(deliver_bitmask & (1 << vcpu_id)))
+				continue;
+			deliver_bitmask &= ~(1 << vcpu_id);
+			vcpu = ioapic->kvm->vcpus[vcpu_id];
+			if (vcpu)
+				kvm_apic_set_irq(vcpu, vector, trig_mode);
+		}
+		break;
+	default:
+		printk(KERN_INFO "kvm: unsupported MSI delivery mode\n");
+	}
 #endif
 }
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 49848cb..e39c57a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -47,10 +47,6 @@
 #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
@@ -86,64 +82,6 @@ static bool kvm_rebooting;
 
 #ifdef KVM_CAP_DEVICE_ASSIGNMENT
 
-#ifdef CONFIG_X86
-static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev, u32 gsi)
-{
-	int vcpu_id;
-	struct kvm_vcpu *vcpu;
-	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
-	struct kvm_gsi_msg *gsi_msg =
-			kvm_find_gsi_msg(dev->kvm, gsi);
-	int dest_id, vector, dest_mode, trig_mode, delivery_mode;
-	u32 deliver_bitmask;
-
-	BUG_ON(!ioapic);
-
-	if (!gsi_msg) {
-		printk(KERN_WARNING "kvm: fail to find correlated gsi_msg\n");
-		return;
-	}
-
-	dest_id = (gsi_msg->msg.address_lo & MSI_ADDR_DEST_ID_MASK)
-			>> MSI_ADDR_DEST_ID_SHIFT;
-	vector = (gsi_msg->msg.data & MSI_DATA_VECTOR_MASK)
-			>> MSI_DATA_VECTOR_SHIFT;
-	dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT,
-				(unsigned long *)&gsi_msg->msg.address_lo);
-	trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT,
-				(unsigned long *)&gsi_msg->msg.data);
-	delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
-				(unsigned long *)&gsi_msg->msg.data);
-	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
-				dest_id, dest_mode);
-	/* IOAPIC delivery mode value is the same as MSI here */
-	switch (delivery_mode) {
-	case IOAPIC_LOWEST_PRIORITY:
-		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 IOAPIC_FIXED:
-		for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) {
-			if (!(deliver_bitmask & (1 << vcpu_id)))
-				continue;
-			deliver_bitmask &= ~(1 << vcpu_id);
-			vcpu = ioapic->kvm->vcpus[vcpu_id];
-			if (vcpu)
-				kvm_apic_set_irq(vcpu, vector, trig_mode);
-		}
-		break;
-	default:
-		printk(KERN_INFO "kvm: unsupported MSI delivery mode\n");
-	}
-}
-#else
-static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev, u32 gsi) {}
-#endif
-
 static struct kvm_assigned_dev_kernel *kvm_find_assigned_dev(struct list_head *head,
 						      int assigned_dev_id)
 {
@@ -170,16 +108,15 @@ static void kvm_assigned_dev_interrupt_work_handler(struct work_struct *work)
 	 * finer-grained lock, update this
 	 */
 	mutex_lock(&assigned_dev->kvm->lock);
-	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, assigned_dev->guest_irq);
+
+	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);
 	kvm_put_kvm(assigned_dev->kvm);
 }
-- 
1.5.4.5

Re: [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq

[Marcelo Tosatti]

Hi Sheng,

On Tue, Dec 23, 2008 at 04:00:25PM +0800, Sheng Yang wrote:
> Which is more convenient...
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  virt/kvm/kvm_main.c |   10 ++--------
>  1 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ffd261d..cd84b3e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -284,11 +284,7 @@ static int assigned_device_update_intx(struct kvm *kvm,
>  		return 0;
>  
>  	if (irqchip_in_kernel(kvm)) {
> -		if (!msi2intx &&
> -		    adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) {
> -			free_irq(adev->host_irq, (void *)kvm);
> -			pci_disable_msi(adev->dev);
> -		}
> +		kvm_free_assigned_irq(kvm, adev);
>  
>  		if (!capable(CAP_SYS_RAWIO))
>  			return -EPERM;
> @@ -339,9 +335,7 @@ static int assigned_device_update_msi(struct kvm *kvm,
>  
>  	if (irqchip_in_kernel(kvm)) {
>  		if (!msi2intx) {
> -			if (adev->irq_requested_type &
> -					KVM_ASSIGNED_DEV_HOST_INTX)
> -				free_irq(adev->host_irq, (void *)adev);
> +			kvm_free_assigned_irq(kvm, adev);
>  
>  			r = pci_enable_msi(adev->dev);
>  			if (r)

Regarding kvm_free_assigned_irq and
assigned_device_update_msi/update_intx:

        if (cancel_work_sync(&assigned_dev->interrupt_work))
                /* We had pending work. That means we will have to take
                 * care of kvm_put_kvm.
                 */ 
                kvm_put_kvm(kvm);
         
        free_irq(assigned_dev->host_irq, (void *)assigned_dev);

What prevents the host IRQ from being triggered between kvm_put_kvm and
free_irq?

Also, if the kvm_put_kvm(kvm) from
kvm_assigned_dev_interrupt_work_handler happens to be the last one,
can't this happen:

- kvm_assigned_dev_interrupt_work_handler
- kvm_put_kvm
- kvm_destroy_vm
- kvm_arch_destroy_vm
- kvm_free_all_assigned_devices
- kvm_free_assigned_device
- kvm_free_assigned_irq
- cancel_work_sync(&assigned_dev->interrupt_work)

deadlock.
    
The msi2intx parameter is somewhat odd. Wouldnt it be more versatile
if controlled from userspace (and per guest) ?

Re: [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq

[Marcelo Tosatti]

On Tue, Dec 23, 2008 at 04:00:24PM +0800, Sheng Yang wrote:
> For MSI disable feature later.
> 
> Notice I changed ABI here, but due to no userspace patch, I think it's OK.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  include/linux/kvm.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index ef7f98e..5b965f6 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -544,6 +544,7 @@ struct kvm_assigned_irq {
>  
>  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
>  
> -#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
> +#define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	(1 << 0)
> +#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 1)

This is a little confusing. KVM_DEV_IRQ_ASSIGN_MSI_ACTION is assigned
from userspace, and in the patchset used in conjunction with msi2intx
which is a module parameter.

Is there anything that blocks control of msi2intx translate behaviour
from userspace?

Perhaps add a KVM_DEV_IRQ_UNASSIGN ioctl, and pass the desired
guest/host irq types on the IRQ_ASSIGN ioctl, thus removing some of the
kernel complexity.

Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI

[Marcelo Tosatti]

On Tue, Dec 23, 2008 at 04:00:27PM +0800, Sheng Yang wrote:
> Avi's purpose, to use single kvm_set_irq() to deal with all interrupt, including
> MSI. So here is it.
> 
> struct gsi_msg is a mapping from a special gsi(with KVM_GSI_MSG_MASK) to
> MSI/MSI-X message address/data.
> 
> Now we support up to 256 gsi_msg mapping, and gsi_msg is allocated by kernel and
> provide two ioctls to userspace, which is more flexiable.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  include/linux/kvm.h      |   12 ++++++++
>  include/linux/kvm_host.h |   16 ++++++++++
>  virt/kvm/irq_comm.c      |   70 ++++++++++++++++++++++++++++++++++++++++++++++
>  virt/kvm/kvm_main.c      |   63 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 161 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 5b965f6..b091a86 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -394,6 +394,7 @@ struct kvm_trace_rec {
>  #define KVM_CAP_USER_NMI 22
>  #endif
>  #define KVM_CAP_SET_GUEST_DEBUG 23
> +#define KVM_CAP_GSI_MSG 24
>  
>  /*
>   * ioctls for VM fds
> @@ -427,6 +428,8 @@ struct kvm_trace_rec {
>  				   struct kvm_assigned_pci_dev)
>  #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
>  			    struct kvm_assigned_irq)
> +#define KVM_REQUEST_GSI_MSG _IOWR(KVMIO, 0x71, struct kvm_assigned_gsi_msg)
> +#define KVM_FREE_GSI_MSG _IOR(KVMIO, 0x72, __u32)

Wrap the __u32 into a struct. Could use kvm_assigned_gsi_msg itself.

> +++ b/include/linux/kvm_host.h
> @@ -132,6 +132,10 @@ struct kvm {
>  	unsigned long mmu_notifier_seq;
>  	long mmu_notifier_count;
>  #endif
> +	struct hlist_head gsi_msg_list;
> +	struct mutex gsi_msg_lock;
> +#define KVM_NR_GSI_MSG	    256
> +	DECLARE_BITMAP(gsi_msg_bitmap, KVM_NR_GSI_MSG);
>  };

This is platform specific data. Can't it live in kvm_arch?

>  }
> +
> +int kvm_update_gsi_msg(struct kvm *kvm, struct kvm_gsi_msg *gsi_msg)
> +{
> +	struct kvm_gsi_msg *found_msg, *new_gsi_msg;
> +	int r, gsi;
> +
> +	mutex_lock(&kvm->gsi_msg_lock);
> +	/* Find whether we need a update or a new entry */
> +	found_msg = kvm_find_gsi_msg(kvm, gsi_msg->gsi);
> +	if (found_msg)
> +		*found_msg = *gsi_msg;
> +	else {
> +		gsi = find_first_zero_bit(kvm->gsi_msg_bitmap, KVM_NR_GSI_MSG);
> +		if (gsi >= KVM_NR_GSI_MSG) {
> +			r = -EFAULT;

ENOSPC?

> +static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
> +					struct kvm_assigned_gsi_msg *agsi_msg)
> +{
> +	struct kvm_gsi_msg gsi_msg;
> +	int r;
> +
> +	gsi_msg.gsi = agsi_msg->gsi;
> +	gsi_msg.msg.address_lo = agsi_msg->msg.addr_lo;
> +	gsi_msg.msg.address_hi = agsi_msg->msg.addr_hi;
> +	gsi_msg.msg.data = agsi_msg->msg.data;
> +
> +	r = kvm_update_gsi_msg(kvm, &gsi_msg);
> +	if (r == 0)
> +		agsi_msg->gsi = gsi_msg.gsi;
> +	return r;
> +}

Can't see the purpose of this function. Why preserve the user-passed GSI
value in case of failure? It will return an error anyway...

Re: [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment

[Marcelo Tosatti]

On Tue, Dec 23, 2008 at 04:00:28PM +0800, Sheng Yang wrote:
> Convert MSI userspace interface to support gsi_msg mapping(and nobody should
> be the user of the old interface...).
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  include/linux/kvm_host.h |    1 -
>  virt/kvm/kvm_main.c      |   33 ++++++++++++++++++++-------------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index db9de47..50b3ff6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -92,20 +92,28 @@ static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev)
>  	int vcpu_id;
>  	struct kvm_vcpu *vcpu;
>  	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
> -	int dest_id = (dev->guest_msi.address_lo & MSI_ADDR_DEST_ID_MASK)
> -			>> MSI_ADDR_DEST_ID_SHIFT;
> -	int vector = (dev->guest_msi.data & MSI_DATA_VECTOR_MASK)
> -			>> MSI_DATA_VECTOR_SHIFT;
> -	int dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT,
> -				(unsigned long *)&dev->guest_msi.address_lo);
> -	int trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT,
> -				(unsigned long *)&dev->guest_msi.data);
> -	int delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
> -				(unsigned long *)&dev->guest_msi.data);
> +	struct kvm_gsi_msg *gsi_msg =
> +			kvm_find_gsi_msg(dev->kvm, dev->guest_irq);

kvm_find_gsi_msg requires the gsi_msg mutex held?

> +	delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
> +				(unsigned long *)&gsi_msg->msg.data);
>  	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
>  				dest_id, dest_mode);
>  	/* IOAPIC delivery mode value is the same as MSI here */

Please unify the IOAPIC specific code in assigned_device_msi_dispatch
with ioapic_deliver.

Re: [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq

[Marcelo Tosatti]

On Tue, Dec 23, 2008 at 04:00:31PM +0800, Sheng Yang wrote:
> Using kvm_set_irq to handle all interrupt injection.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  include/linux/kvm_host.h |    2 +-
>  virt/kvm/irq_comm.c      |   96 ++++++++++++++++++++++++++++++++++++++-------
>  virt/kvm/kvm_main.c      |   75 +++---------------------------------
>  3 files changed, 88 insertions(+), 85 deletions(-)
> 
> --- a/virt/kvm/irq_comm.c
> +++ b/virt/kvm/irq_comm.c
> @@ -20,28 +20,94 @@
>   */
>  

>  #ifdef CONFIG_X86
> -	kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
> +	gsi_msg = kvm_find_gsi_msg(kvm, gsi);

It was nicer isolated in assigned_device_msi_dispatch.

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

And there's quite some x86 specific code sneaking into virt/kvm. Ideally
platform specific parts should be hidden behind interfaces.

Re: [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq

[Sheng Yang]

On Wednesday 24 December 2008 01:32:24 Marcelo Tosatti wrote:
> On Tue, Dec 23, 2008 at 04:00:24PM +0800, Sheng Yang wrote:
> > For MSI disable feature later.
> >
> > Notice I changed ABI here, but due to no userspace patch, I think it's
> > OK.
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  include/linux/kvm.h |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> > index ef7f98e..5b965f6 100644
> > --- a/include/linux/kvm.h
> > +++ b/include/linux/kvm.h
> > @@ -544,6 +544,7 @@ struct kvm_assigned_irq {
> >
> >  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
> >
> > -#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
> > +#define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	(1 << 0)
> > +#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 1)
>
> This is a little confusing. KVM_DEV_IRQ_ASSIGN_MSI_ACTION is assigned
> from userspace, and in the patchset used in conjunction with msi2intx
> which is a module parameter.
>
> Is there anything that blocks control of msi2intx translate behaviour
> from userspace?
>
> Perhaps add a KVM_DEV_IRQ_UNASSIGN ioctl, and pass the desired
> guest/host irq types on the IRQ_ASSIGN ioctl, thus removing some of the
> kernel complexity.

Marcelo, Thanks for reviewing.

msi2intx module parameter is mostly a debug assist rather than a real option, 
so we won't want this to be a real option controlled by userspace program. 

Another simple solution for the conjunction is:

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index ffd261d..35bc81e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -405,8 +405,7 @@ static int kvm_vm_ioctl_assign_irq(struct kvm *kvm,
                }
        }

-       if ((!msi2intx &&
-            (assigned_irq->flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION)) ||
+       if ((assigned_irq->flags & KVM_DEV_IRQ_ASSIGN_MSI_ACTION) ||
            (msi2intx && match->dev->msi_enabled)) {

So we wouldn't confusion in the semantic.

About MSI disable side, maybe leave some judgment to userspace can reduce the 
complexity, I will try it.

-- 
regards
Yang, Sheng

Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI

[Sheng Yang]

On Wednesday 24 December 2008 01:55:42 Marcelo Tosatti wrote:
> On Tue, Dec 23, 2008 at 04:00:27PM +0800, Sheng Yang wrote:
> > Avi's purpose, to use single kvm_set_irq() to deal with all interrupt,
> > including MSI. So here is it.
> >
> > struct gsi_msg is a mapping from a special gsi(with KVM_GSI_MSG_MASK) to
> > MSI/MSI-X message address/data.
> >
> > Now we support up to 256 gsi_msg mapping, and gsi_msg is allocated by
> > kernel and provide two ioctls to userspace, which is more flexiable.
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  include/linux/kvm.h      |   12 ++++++++
> >  include/linux/kvm_host.h |   16 ++++++++++
> >  virt/kvm/irq_comm.c      |   70
> > ++++++++++++++++++++++++++++++++++++++++++++++ virt/kvm/kvm_main.c      |
> >   63 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 161
> > insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> > index 5b965f6..b091a86 100644
> > --- a/include/linux/kvm.h
> > +++ b/include/linux/kvm.h
> > @@ -394,6 +394,7 @@ struct kvm_trace_rec {
> >  #define KVM_CAP_USER_NMI 22
> >  #endif
> >  #define KVM_CAP_SET_GUEST_DEBUG 23
> > +#define KVM_CAP_GSI_MSG 24
> >
> >  /*
> >   * ioctls for VM fds
> > @@ -427,6 +428,8 @@ struct kvm_trace_rec {
> >  				   struct kvm_assigned_pci_dev)
> >  #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
> >  			    struct kvm_assigned_irq)
> > +#define KVM_REQUEST_GSI_MSG _IOWR(KVMIO, 0x71, struct
> > kvm_assigned_gsi_msg) +#define KVM_FREE_GSI_MSG _IOR(KVMIO, 0x72, __u32)
>
> Wrap the __u32 into a struct. Could use kvm_assigned_gsi_msg itself.

OK. 

> > +++ b/include/linux/kvm_host.h
> > @@ -132,6 +132,10 @@ struct kvm {
> >  	unsigned long mmu_notifier_seq;
> >  	long mmu_notifier_count;
> >  #endif
> > +	struct hlist_head gsi_msg_list;
> > +	struct mutex gsi_msg_lock;
> > +#define KVM_NR_GSI_MSG	    256
> > +	DECLARE_BITMAP(gsi_msg_bitmap, KVM_NR_GSI_MSG);
> >  };
>
> This is platform specific data. Can't it live in kvm_arch?

No... It's not platform specific data, at least, we want it to use with x86, 
IA64 and virtio, that's why I make it so generic...
>
> >  }
> > +
> > +int kvm_update_gsi_msg(struct kvm *kvm, struct kvm_gsi_msg *gsi_msg)
> > +{
> > +	struct kvm_gsi_msg *found_msg, *new_gsi_msg;
> > +	int r, gsi;
> > +
> > +	mutex_lock(&kvm->gsi_msg_lock);
> > +	/* Find whether we need a update or a new entry */
> > +	found_msg = kvm_find_gsi_msg(kvm, gsi_msg->gsi);
> > +	if (found_msg)
> > +		*found_msg = *gsi_msg;
> > +	else {
> > +		gsi = find_first_zero_bit(kvm->gsi_msg_bitmap, KVM_NR_GSI_MSG);
> > +		if (gsi >= KVM_NR_GSI_MSG) {
> > +			r = -EFAULT;
>
> ENOSPC?

OK. (Though I am confusing with all kinds of ERR all the time, ENOSPC show "No 
space left in the device"... And last time somebody told me "ENOTTY" means 
something is not available...)
>
> > +static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
> > +					struct kvm_assigned_gsi_msg *agsi_msg)
> > +{
> > +	struct kvm_gsi_msg gsi_msg;
> > +	int r;
> > +
> > +	gsi_msg.gsi = agsi_msg->gsi;
> > +	gsi_msg.msg.address_lo = agsi_msg->msg.addr_lo;
> > +	gsi_msg.msg.address_hi = agsi_msg->msg.addr_hi;
> > +	gsi_msg.msg.data = agsi_msg->msg.data;
> > +
> > +	r = kvm_update_gsi_msg(kvm, &gsi_msg);
> > +	if (r == 0)
> > +		agsi_msg->gsi = gsi_msg.gsi;
> > +	return r;
> > +}
>
> Can't see the purpose of this function. Why preserve the user-passed GSI
> value in case of failure? It will return an error anyway...

Yeah, for the MSI(due to we emulated the configuration space in the userspace) 
and virtio.

Why it would return a error? I missed something?

-- 
regards
Yang, Sheng

Re: [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment

[Sheng Yang]

On Wednesday 24 December 2008 02:05:15 Marcelo Tosatti wrote:
> On Tue, Dec 23, 2008 at 04:00:28PM +0800, Sheng Yang wrote:
> > Convert MSI userspace interface to support gsi_msg mapping(and nobody
> > should be the user of the old interface...).
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  include/linux/kvm_host.h |    1 -
> >  virt/kvm/kvm_main.c      |   33 ++++++++++++++++++++-------------
> >  2 files changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index db9de47..50b3ff6 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -92,20 +92,28 @@ static void assigned_device_msi_dispatch(struct
> > kvm_assigned_dev_kernel *dev) int vcpu_id;
> >  	struct kvm_vcpu *vcpu;
> >  	struct kvm_ioapic *ioapic = ioapic_irqchip(dev->kvm);
> > -	int dest_id = (dev->guest_msi.address_lo & MSI_ADDR_DEST_ID_MASK)
> > -			>> MSI_ADDR_DEST_ID_SHIFT;
> > -	int vector = (dev->guest_msi.data & MSI_DATA_VECTOR_MASK)
> > -			>> MSI_DATA_VECTOR_SHIFT;
> > -	int dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT,
> > -				(unsigned long *)&dev->guest_msi.address_lo);
> > -	int trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT,
> > -				(unsigned long *)&dev->guest_msi.data);
> > -	int delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
> > -				(unsigned long *)&dev->guest_msi.data);
> > +	struct kvm_gsi_msg *gsi_msg =
> > +			kvm_find_gsi_msg(dev->kvm, dev->guest_irq);
>
> kvm_find_gsi_msg requires the gsi_msg mutex held?

Missed, thanks!
>
> > +	delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
> > +				(unsigned long *)&gsi_msg->msg.data);
> >  	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
> >  				dest_id, dest_mode);
> >  	/* IOAPIC delivery mode value is the same as MSI here */
>
> Please unify the IOAPIC specific code in assigned_device_msi_dispatch
> with ioapic_deliver.

I meant to do this, but seems it won't reduce the complexity much... We got a 
large number of input(delivery mode), and two or three kind of output(for > 
FIXED, LOWPRI, and NMI for IOAPIC). What I can think of is unify output to a
delivery_mask, and use something like:
>
> 		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) {
> 				r = xxx_inj_irq(ioapic, vcpu, vector,
> 					       trig_mode, delivery_mode);
> 			}
> 		}

And xxx_inj_irq() need a judgment for the delivery mode NMI in ioapic...

Maybe we can reduce ~20 lines by refactoring this. I will give a try... (Um, 
duplicate is always unbearable thing)

-- 
regards
Yang, Sheng

Re: [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq

[Sheng Yang]

On Wednesday 24 December 2008 02:10:20 Marcelo Tosatti wrote:
> On Tue, Dec 23, 2008 at 04:00:31PM +0800, Sheng Yang wrote:
> > Using kvm_set_irq to handle all interrupt injection.
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  include/linux/kvm_host.h |    2 +-
> >  virt/kvm/irq_comm.c      |   96
> > ++++++++++++++++++++++++++++++++++++++------- virt/kvm/kvm_main.c      | 
> >  75 +++--------------------------------- 3 files changed, 88
> > insertions(+), 85 deletions(-)
> >
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -20,28 +20,94 @@
> >   */
> >
> >
> >  #ifdef CONFIG_X86
> > -	kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
> > +	gsi_msg = kvm_find_gsi_msg(kvm, gsi);
>
> It was nicer isolated in assigned_device_msi_dispatch.

Um... The gsi_msg layer existed for this... 

AD: No matter it's MSI, MSI-X or IOAPIC, do kvm_set_irq() simply, you would 
get a interrupt! :)
>
> > -#ifdef CONFIG_X86
> > -#include <asm/msidef.h>
> > -#endif
>
> And there's quite some x86 specific code sneaking into virt/kvm. Ideally
> platform specific parts should be hidden behind interfaces.

Sorry for the #ifdef, we would discard it after we enable MSI for IA64 which 
share the code mostly.

-- 
regards
Yang, Sheng

Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI

[Sheng Yang]

On Wednesday 24 December 2008 10:31:00 Sheng Yang wrote:
> On Wednesday 24 December 2008 01:55:42 Marcelo Tosatti wrote:
> > On Tue, Dec 23, 2008 at 04:00:27PM +0800, Sheng Yang wrote:
> > > Avi's purpose, to use single kvm_set_irq() to deal with all interrupt,
> > > including MSI. So here is it.
> > >
> > > struct gsi_msg is a mapping from a special gsi(with KVM_GSI_MSG_MASK)
> > > to MSI/MSI-X message address/data.
> > >
> > > Now we support up to 256 gsi_msg mapping, and gsi_msg is allocated by
> > > kernel and provide two ioctls to userspace, which is more flexiable.
> > >
> > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > ---
> > >  include/linux/kvm.h      |   12 ++++++++
> > >  include/linux/kvm_host.h |   16 ++++++++++
> > >  virt/kvm/irq_comm.c      |   70
> > > ++++++++++++++++++++++++++++++++++++++++++++++ virt/kvm/kvm_main.c     
> > > | 63 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 161
> > > insertions(+), 0 deletions(-)
> > >
> > > diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> > > index 5b965f6..b091a86 100644
> > > --- a/include/linux/kvm.h
> > > +++ b/include/linux/kvm.h
> > > @@ -394,6 +394,7 @@ struct kvm_trace_rec {
> > >  #define KVM_CAP_USER_NMI 22
> > >  #endif
> > >  #define KVM_CAP_SET_GUEST_DEBUG 23
> > > +#define KVM_CAP_GSI_MSG 24
> > >
> > >  /*
> > >   * ioctls for VM fds
> > > @@ -427,6 +428,8 @@ struct kvm_trace_rec {
> > >  				   struct kvm_assigned_pci_dev)
> > >  #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
> > >  			    struct kvm_assigned_irq)
> > > +#define KVM_REQUEST_GSI_MSG _IOWR(KVMIO, 0x71, struct
> > > kvm_assigned_gsi_msg) +#define KVM_FREE_GSI_MSG _IOR(KVMIO, 0x72,
> > > __u32)
> >
> > Wrap the __u32 into a struct. Could use kvm_assigned_gsi_msg itself.
>
> OK.
>
> > > +++ b/include/linux/kvm_host.h
> > > @@ -132,6 +132,10 @@ struct kvm {
> > >  	unsigned long mmu_notifier_seq;
> > >  	long mmu_notifier_count;
> > >  #endif
> > > +	struct hlist_head gsi_msg_list;
> > > +	struct mutex gsi_msg_lock;
> > > +#define KVM_NR_GSI_MSG	    256
> > > +	DECLARE_BITMAP(gsi_msg_bitmap, KVM_NR_GSI_MSG);
> > >  };
> >
> > This is platform specific data. Can't it live in kvm_arch?
>
> No... It's not platform specific data, at least, we want it to use with
> x86, IA64 and virtio, that's why I make it so generic...
>
> > >  }
> > > +
> > > +int kvm_update_gsi_msg(struct kvm *kvm, struct kvm_gsi_msg *gsi_msg)
> > > +{
> > > +	struct kvm_gsi_msg *found_msg, *new_gsi_msg;
> > > +	int r, gsi;
> > > +
> > > +	mutex_lock(&kvm->gsi_msg_lock);
> > > +	/* Find whether we need a update or a new entry */
> > > +	found_msg = kvm_find_gsi_msg(kvm, gsi_msg->gsi);
> > > +	if (found_msg)
> > > +		*found_msg = *gsi_msg;
> > > +	else {
> > > +		gsi = find_first_zero_bit(kvm->gsi_msg_bitmap, KVM_NR_GSI_MSG);
> > > +		if (gsi >= KVM_NR_GSI_MSG) {
> > > +			r = -EFAULT;
> >
> > ENOSPC?
>
> OK. (Though I am confusing with all kinds of ERR all the time, ENOSPC show
> "No space left in the device"... And last time somebody told me "ENOTTY"
> means something is not available...)
>
> > > +static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
> > > +					struct kvm_assigned_gsi_msg *agsi_msg)
> > > +{
> > > +	struct kvm_gsi_msg gsi_msg;
> > > +	int r;
> > > +
> > > +	gsi_msg.gsi = agsi_msg->gsi;
> > > +	gsi_msg.msg.address_lo = agsi_msg->msg.addr_lo;
> > > +	gsi_msg.msg.address_hi = agsi_msg->msg.addr_hi;
> > > +	gsi_msg.msg.data = agsi_msg->msg.data;
> > > +
> > > +	r = kvm_update_gsi_msg(kvm, &gsi_msg);
> > > +	if (r == 0)
> > > +		agsi_msg->gsi = gsi_msg.gsi;
> > > +	return r;
> > > +}
> >
> > Can't see the purpose of this function. Why preserve the user-passed GSI
> > value in case of failure? It will return an error anyway...
>
Oh, we preserved the user-passed GSI in case of userspace want to update one 
entry. :)

-- 
regards
Yang, Sheng

Re: [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq

[Sheng Yang]

On Tuesday 23 December 2008 23:18:43 Marcelo Tosatti wrote:
> Hi Sheng,
>
> On Tue, Dec 23, 2008 at 04:00:25PM +0800, Sheng Yang wrote:
> > Which is more convenient...
> >
> > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > ---
> >  virt/kvm/kvm_main.c |   10 ++--------
> >  1 files changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index ffd261d..cd84b3e 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -284,11 +284,7 @@ static int assigned_device_update_intx(struct kvm
> > *kvm, return 0;
> >
> >  	if (irqchip_in_kernel(kvm)) {
> > -		if (!msi2intx &&
> > -		    adev->irq_requested_type & KVM_ASSIGNED_DEV_HOST_MSI) {
> > -			free_irq(adev->host_irq, (void *)kvm);
> > -			pci_disable_msi(adev->dev);
> > -		}
> > +		kvm_free_assigned_irq(kvm, adev);
> >
> >  		if (!capable(CAP_SYS_RAWIO))
> >  			return -EPERM;
> > @@ -339,9 +335,7 @@ static int assigned_device_update_msi(struct kvm
> > *kvm,
> >
> >  	if (irqchip_in_kernel(kvm)) {
> >  		if (!msi2intx) {
> > -			if (adev->irq_requested_type &
> > -					KVM_ASSIGNED_DEV_HOST_INTX)
> > -				free_irq(adev->host_irq, (void *)adev);
> > +			kvm_free_assigned_irq(kvm, adev);
> >
> >  			r = pci_enable_msi(adev->dev);
> >  			if (r)
>
> Regarding kvm_free_assigned_irq and
> assigned_device_update_msi/update_intx:
>
>         if (cancel_work_sync(&assigned_dev->interrupt_work))
>                 /* We had pending work. That means we will have to take
>                  * care of kvm_put_kvm.
>                  */
>                 kvm_put_kvm(kvm);
>
>         free_irq(assigned_dev->host_irq, (void *)assigned_dev);
>
> What prevents the host IRQ from being triggered between kvm_put_kvm and
> free_irq?
>
> Also, if the kvm_put_kvm(kvm) from
> kvm_assigned_dev_interrupt_work_handler happens to be the last one,
> can't this happen:
>
> - kvm_assigned_dev_interrupt_work_handler
> - kvm_put_kvm
> - kvm_destroy_vm
> - kvm_arch_destroy_vm
> - kvm_free_all_assigned_devices
> - kvm_free_assigned_device
> - kvm_free_assigned_irq
> - cancel_work_sync(&assigned_dev->interrupt_work)
>
> deadlock.
>
Nice catch! I've updated the patchset to address this, take a look? :)

-- 
regards
Yang, Sheng

Re: [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq

[Marcelo Tosatti]

On Wed, Dec 24, 2008 at 10:24:34AM +0800, Sheng Yang wrote:
> On Wednesday 24 December 2008 01:32:24 Marcelo Tosatti wrote:
> > On Tue, Dec 23, 2008 at 04:00:24PM +0800, Sheng Yang wrote:
> > > For MSI disable feature later.
> > >
> > > Notice I changed ABI here, but due to no userspace patch, I think it's
> > > OK.
> > >
> > > Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> > > ---
> > >  include/linux/kvm.h |    3 ++-
> > >  1 files changed, 2 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> > > index ef7f98e..5b965f6 100644
> > > --- a/include/linux/kvm.h
> > > +++ b/include/linux/kvm.h
> > > @@ -544,6 +544,7 @@ struct kvm_assigned_irq {
> > >
> > >  #define KVM_DEV_ASSIGN_ENABLE_IOMMU	(1 << 0)
> > >
> > > -#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 0)
> > > +#define KVM_DEV_IRQ_ASSIGN_MSI_ACTION	(1 << 0)
> > > +#define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI	(1 << 1)
> >
> > This is a little confusing. KVM_DEV_IRQ_ASSIGN_MSI_ACTION is assigned
> > from userspace, and in the patchset used in conjunction with msi2intx
> > which is a module parameter.
> >
> > Is there anything that blocks control of msi2intx translate behaviour
> > from userspace?
> >
> > Perhaps add a KVM_DEV_IRQ_UNASSIGN ioctl, and pass the desired
> > guest/host irq types on the IRQ_ASSIGN ioctl, thus removing some of the
> > kernel complexity.
> 
> Marcelo, Thanks for reviewing.
> 
> msi2intx module parameter is mostly a debug assist rather than a real option, 
> so we won't want this to be a real option controlled by userspace program.

It seems msi2intx (as a module parameter) is not necessary.
You can achieve the same from userspace by configuring
KVM_DEV_IRQ_ASSIGN_ENABLE_MSI/MSI_ACTION, no? The module parameter seems
a weird interface to me (even if it is a debug one).

But its not a big deal, if you think it is worthwhile to keep it that
way.

Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI

[Marcelo Tosatti]

On Thu, Dec 25, 2008 at 09:59:45AM +0800, Sheng Yang wrote:
> > > > +	found_msg = kvm_find_gsi_msg(kvm, gsi_msg->gsi);
> > > > +	if (found_msg)
> > > > +		*found_msg = *gsi_msg;
> > > > +	else {
> > > > +		gsi = find_first_zero_bit(kvm->gsi_msg_bitmap, KVM_NR_GSI_MSG);
> > > > +		if (gsi >= KVM_NR_GSI_MSG) {
> > > > +			r = -EFAULT;
> > >
> > > ENOSPC?
> >
> > OK. (Though I am confusing with all kinds of ERR all the time, ENOSPC show
> > "No space left in the device"... And last time somebody told me "ENOTTY"
> > means something is not available...)

Not sure what the most appropriate error is here.

> >
> > > > +static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
> > > > +					struct kvm_assigned_gsi_msg *agsi_msg)
> > > > +{
> > > > +	struct kvm_gsi_msg gsi_msg;
> > > > +	int r;
> > > > +
> > > > +	gsi_msg.gsi = agsi_msg->gsi;
> > > > +	gsi_msg.msg.address_lo = agsi_msg->msg.addr_lo;
> > > > +	gsi_msg.msg.address_hi = agsi_msg->msg.addr_hi;
> > > > +	gsi_msg.msg.data = agsi_msg->msg.data;
> > > > +
> > > > +	r = kvm_update_gsi_msg(kvm, &gsi_msg);
> > > > +	if (r == 0)
> > > > +		agsi_msg->gsi = gsi_msg.gsi;
> > > > +	return r;
> > > > +}
> > >
> > > Can't see the purpose of this function. Why preserve the user-passed GSI
> > > value in case of failure? It will return an error anyway...
> >
> Oh, we preserved the user-passed GSI in case of userspace want to update one 
> entry. :)

The structure is not copied back to userspace in case of failure anyway:

+           r = kvm_vm_ioctl_request_gsi_msg(kvm, &agsi_msg);
+               if (r)
+                       goto out;
+               r = -EFAULT;
+           if (copy_to_user(argp, &agsi_msg, sizeof agsi_msg))
+                   goto out;

So I don't see the point of doing this?

Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI

[Sheng Yang]

On Sat, Dec 27, 2008 at 05:27:25PM -0200, Marcelo Tosatti wrote:
> > > > > +static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
> > > > > +					struct kvm_assigned_gsi_msg *agsi_msg)
> > > > > +{
> > > > > +	struct kvm_gsi_msg gsi_msg;
> > > > > +	int r;
> > > > > +
> > > > > +	gsi_msg.gsi = agsi_msg->gsi;
> > > > > +	gsi_msg.msg.address_lo = agsi_msg->msg.addr_lo;
> > > > > +	gsi_msg.msg.address_hi = agsi_msg->msg.addr_hi;
> > > > > +	gsi_msg.msg.data = agsi_msg->msg.data;
> > > > > +
> > > > > +	r = kvm_update_gsi_msg(kvm, &gsi_msg);
> > > > > +	if (r == 0)
> > > > > +		agsi_msg->gsi = gsi_msg.gsi;
> > > > > +	return r;
> > > > > +}
> > > >
> > > > Can't see the purpose of this function. Why preserve the user-passed GSI
> > > > value in case of failure? It will return an error anyway...
> > >
> > Oh, we preserved the user-passed GSI in case of userspace want to update one 
> > entry. :)
> 
> The structure is not copied back to userspace in case of failure anyway:
> 
> +           r = kvm_vm_ioctl_request_gsi_msg(kvm, &agsi_msg);
> +               if (r)
> +                       goto out;
> +               r = -EFAULT;
> +           if (copy_to_user(argp, &agsi_msg, sizeof agsi_msg))
> +                   goto out;
> 
> So I don't see the point of doing this?
> 

Oh, Marcelo, finally I got your meaning. I misunderstand you at first time.
And now I think you just means: Why "if (r == 0)" is there? It's unnecessary!

Yes your are right. It can be called a little habit, and preseved value is
useless at all, and maybe I just want to indicate what's the legal return
value here. And from the other side, "if (r != 0)", the value should be
meaningless and assigning it to another variable may not proper.

--
regards
Yang, Sheng