MSI enhancement

PATCH In review Sheng Yang 2 participants 23 messages Dec 23, 2008 – Dec 28, 2008 KVM

lore

Share & tools

lore

Sheng Yang

author

2008-12-23 08:00 UTC #0 lore raw

cover MSI enhancement

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

Sheng Yang

author

2008-12-23 08:00 UTC #1 lore raw

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

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 <[email protected]>

---
 include/linux/kvm.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

include/linux/kvm.h +2 -1

index ef7f98e..5b965f6 100644
@@ -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

Signature · 1 line

1.5.4.5

Marcelo Tosatti

2008-12-23 17:32 UTC #2 lore raw

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

Quoted 14 lines · 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 <[email protected]>
---
 include/linux/kvm.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
include/linux/kvm.h +0 -0
index ef7f98e..5b965f6 100644

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

Quoted 6 lines

include/linux/kvm.h +2 -1

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

Sheng Yang

author

2008-12-24 02:24 UTC #3 lore raw

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

Quoted 27 lines · On Wednesday 24 December 2008 01:32:24 Marcelo Tosatti wrote:

Quoted 15 lines · 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 <[email protected]>
---
 include/linux/kvm.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
include/linux/kvm.h +0 -0
index ef7f98e..5b965f6 100644
include/linux/kvm.h struct kvm_assigned_irq { · line 548
Quoted 6 lines
include/linux/kvm.h +2 -1
@@ -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.

Quoted 6 lines

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:

virt/kvm/kvm_main.c +1 -2

index ffd261d..35bc81e 100644
@@ -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.

Signature · 2 lines

regards
Yang, Sheng

Marcelo Tosatti

2008-12-27 19:24 UTC #4 lore raw

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

Quoted 31 lines · Sheng Yang

Quoted 30 lines · On Wednesday 24 December 2008 01:32:24 Marcelo Tosatti wrote:
Quoted 15 lines · 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 <[email protected]>
---
 include/linux/kvm.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
include/linux/kvm.h +0 -0
index ef7f98e..5b965f6 100644
include/linux/kvm.h struct kvm_assigned_irq { · line 548
Quoted 6 lines
include/linux/kvm.h +2 -1
@@ -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?

Quoted 8 lines

Quoted 3 lines
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.

Sheng Yang

author

2008-12-23 08:00 UTC #5 lore raw

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

In reply to Sheng Yang

Which is more convenient...

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/kvm_main.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

virt/kvm/kvm_main.c +2 -8

index ffd261d..cd84b3e 100644
@@ -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)

Signature · 1 line

1.5.4.5

Marcelo Tosatti

2008-12-23 15:18 UTC #6 lore raw

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

Hi Sheng,

Quoted 25 lines · Sheng Yang

Which is more convenient...

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/kvm_main.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

virt/kvm/kvm_main.c +1 -5

index ffd261d..cd84b3e 100644
@@ -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;

virt/kvm/kvm_main.c static int assigned_device_update_msi(struct kvm *kvm, · line 341

Quoted 10 lines

virt/kvm/kvm_main.c +1 -3

@@ -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) ?

Sheng Yang

author

2008-12-25 08:42 UTC #7 lore raw

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

Quoted 67 lines · On Tuesday 23 December 2008 23:18:43 Marcelo Tosatti wrote:

Hi Sheng,
Quoted 35 lines · Sheng Yang
Which is more convenient...
Signed-off-by Sheng Yang <[email protected]>
---
 virt/kvm/kvm_main.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)
virt/kvm/kvm_main.c +2 -8
index ffd261d..cd84b3e 100644
@@ -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);
Quoted 1 line
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)

Quoted 2 lines

deadlock.

Nice catch! I've updated the patchset to address this, take a look? :)

Signature · 2 lines

regards
Yang, Sheng

Sheng Yang

author

2008-12-23 08:00 UTC #8 lore raw

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

In reply to 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 <[email protected]>

---
 virt/kvm/kvm_main.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

virt/kvm/kvm_main.c +10 -1

index cd84b3e..10cf7e1 100644
@@ -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);

Signature · 1 line

1.5.4.5

Sheng Yang

author

2008-12-23 08:00 UTC #9 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

include/linux/kvm.h +12 -0

index 5b965f6..b091a86 100644
@@ -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

include/linux/kvm_host.h +16 -0

index d63e9a4..0e5741a 100644
@@ -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,

virt/kvm/irq_comm.c +70 -0

73 lines not on this page · raw message

virt/kvm/kvm_main.c +63 -0

87 lines not on this page · raw message

Signature · 1 line

1.5.4.5

Marcelo Tosatti

2008-12-23 17:55 UTC #10 lore raw

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

Quoted 30 lines · 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 <[email protected]>
---
 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(-)
include/linux/kvm.h +1 -0
index 5b965f6..b091a86 100644
@@ -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

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

Quoted 5 lines

include/linux/kvm.h +2 -0

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

include/linux/kvm.h struct kvm { · line 139

Quoted 10 lines

include/linux/kvm.h +5 -0

+++ 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?

Quoted 6 lines

include/linux/kvm.h +5 -0

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

include/linux/kvm.h struct kvm {

Quoted 10 lines

include/linux/kvm.h +10 -0

+
+	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?

Quoted 6 lines

include/linux/kvm.h +6 -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;
+

include/linux/kvm.h

Quoted 10 lines

include/linux/kvm.h +10 -0

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

Sheng Yang

author

2008-12-24 02:31 UTC #11 lore raw

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

Quoted 29 lines · On Wednesday 24 December 2008 01:55:42 Marcelo Tosatti wrote:

Quoted 28 lines · 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 <[email protected]>
---
 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(-)
include/linux/kvm.h +1 -0
index 5b965f6..b091a86 100644
@@ -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
 

Quoted 10 lines

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

Quoted 8 lines

include/linux/kvm.h +1 -0

 /*
  * 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.

Quoted 4 lines

include/linux/kvm.h +1 -0

+++ b/include/linux/kvm_host.h

@@ -132,6 +132,10 @@ struct kvm {
 	unsigned long mmu_notifier_seq;
 	long mmu_notifier_count;

Quoted 8 lines

include/linux/kvm.h struct kvm { · line 139
Quoted 6 lines
include/linux/kvm.h +4 -0
 #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...

Quoted 11 lines

include/linux/kvm.h +9 -0

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

Quoted 8 lines

include/linux/kvm.h

Quoted 6 lines

include/linux/kvm.h +6 -0

+	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...)

Quoted 12 lines

include/linux/kvm.h +11 -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;
+

Quoted 8 lines

include/linux/kvm.h
Quoted 5 lines
include/linux/kvm.h +5 -0
+	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?

Signature · 2 lines

regards
Yang, Sheng

Sheng Yang

author

2008-12-25 01:59 UTC #12 lore raw

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

Quoted 79 lines · On Wednesday 24 December 2008 10:31:00 Sheng Yang wrote:

Quoted 29 lines · On Wednesday 24 December 2008 01:55:42 Marcelo Tosatti wrote:
Quoted 28 lines · 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 <[email protected]>
---
 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(-)
include/linux/kvm.h +1 -0
index 5b965f6..b091a86 100644
@@ -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
 
Quoted 11 lines
include/linux/kvm.h struct kvm_trace_rec { · line 433
Quoted 9 lines
include/linux/kvm.h +1 -0
 /*
  * 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.
Quoted 4 lines
Quoted 4 lines
include/linux/kvm.h +1 -0
+++ b/include/linux/kvm_host.h
@@ -132,6 +132,10 @@ struct kvm {
 	unsigned long mmu_notifier_seq;
 	long mmu_notifier_count;
Quoted 8 lines
include/linux/kvm.h struct kvm { · line 139
Quoted 6 lines
include/linux/kvm.h +4 -0
 #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...
Quoted 10 lines
Quoted 10 lines
include/linux/kvm.h +9 -0
 }
+
+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);
Quoted 8 lines
include/linux/kvm.h
Quoted 6 lines
include/linux/kvm.h +6 -0
+	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?

Quoted 24 lines

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...)
Quoted 19 lines
Quoted 6 lines
include/linux/kvm.h +6 -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;
+
include/linux/kvm.h
Quoted 10 lines
include/linux/kvm.h +10 -0
+	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. :)

Signature · 2 lines

regards
Yang, Sheng

Marcelo Tosatti

2008-12-27 19:27 UTC #13 lore raw

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

Quoted 10 lines · Sheng Yang

Quoted 10 lines
Quoted 9 lines
virt/kvm/irq_comm.c void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id) · line 116
Quoted 7 lines
virt/kvm/irq_comm.c +7 -0
+	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?

Quoted 3 lines

Quoted 3 lines
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.

Quoted 21 lines

Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
virt/kvm/kvm_main.c +11 -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;
+
Quoted 9 lines
Quoted 8 lines
virt/kvm/kvm_main.c static int kvm_vcpu_ioctl_set_sigmask(struct kvm_vcpu *vcpu, sigset_t *sigset) · line 1600
Quoted 5 lines
virt/kvm/kvm_main.c +5 -0
+	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...

Quoted 2 lines

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?

Sheng Yang

author

2008-12-28 11:14 UTC #14 lore raw

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

Quoted 32 lines · Marcelo Tosatti

Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
virt/kvm/kvm_main.c +11 -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;
+
Quoted 9 lines
Quoted 8 lines
virt/kvm/kvm_main.c static int kvm_vcpu_ioctl_set_sigmask(struct kvm_vcpu *vcpu, sigset_t *sigset) · line 1600
Quoted 5 lines
virt/kvm/kvm_main.c +5 -0
+	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...
Quoted 2 lines
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:
virt/kvm/kvm_main.c +6 -0
+           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;

Quoted 2 lines

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

Sheng Yang

author

2008-12-23 08:00 UTC #15 lore raw

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

In reply to 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 <[email protected]>

---
 include/linux/kvm_host.h |    1 -
 virt/kvm/kvm_main.c      |   33 ++++++++++++++++++++-------------
 2 files changed, 20 insertions(+), 14 deletions(-)

include/linux/kvm_host.h +0 -1

index 0e5741a..aa2606b 100644
@@ -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)

virt/kvm/kvm_main.c +20 ~~-13~~

index db9de47..50b3ff6 100644
@@ -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 {
 		/*

Signature · 1 line

1.5.4.5

Marcelo Tosatti

2008-12-23 18:05 UTC #16 lore raw

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

Quoted 19 lines · 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 <[email protected]>

---
 include/linux/kvm_host.h |    1 -
 virt/kvm/kvm_main.c      |   33 ++++++++++++++++++++-------------
 2 files changed, 20 insertions(+), 14 deletions(-)

virt/kvm/kvm_main.c +0 -2

index db9de47..50b3ff6 100644
@@ -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;

virt/kvm/kvm_main.c static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev) · line 96

Quoted 10 lines

virt/kvm/kvm_main.c +2 -8

-	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?

virt/kvm/kvm_main.c static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev) · line 119

Quoted 5 lines

virt/kvm/kvm_main.c +2 -0

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

Sheng Yang

author

2008-12-24 02:41 UTC #17 lore raw

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

Quoted 24 lines · On Wednesday 24 December 2008 02:05:15 Marcelo Tosatti wrote:

Quoted 23 lines · 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 <[email protected]>

---
 include/linux/kvm_host.h |    1 -
 virt/kvm/kvm_main.c      |   33 ++++++++++++++++++++-------------
 2 files changed, 20 insertions(+), 14 deletions(-)

virt/kvm/kvm_main.c +0 -6

index db9de47..50b3ff6 100644
@@ -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);

Quoted 8 lines

virt/kvm/kvm_main.c static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev) · line 96
Quoted 6 lines
virt/kvm/kvm_main.c +2 -4
-	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!

Quoted 9 lines

virt/kvm/kvm_main.c static void assigned_device_msi_dispatch(struct kvm_assigned_dev_kernel *dev) · line 119
Quoted 6 lines
virt/kvm/kvm_main.c +2 -0
+	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:

Quoted 11 lines

		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)

Signature · 2 lines

regards
Yang, Sheng

Sheng Yang

author

2008-12-23 08:00 UTC #18 lore raw

PATCH 6/8 KVM: Improve MSI dispatch function

In reply to Sheng Yang

Prepare to merge with kvm_set_irq().

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/kvm_main.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

virt/kvm/kvm_main.c +4 -4

index 50b3ff6..49848cb 100644
@@ -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;
 	}

Signature · 1 line

1.5.4.5

Sheng Yang

author

2008-12-23 08:00 UTC #19 lore raw

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

In reply to Sheng Yang

Signed-off-by Sheng Yang <[email protected]>

---
 virt/kvm/irq_comm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

virt/kvm/irq_comm.c +1 -1

index e95ec3f..f7dcf70 100644
@@ -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

Signature · 1 line

1.5.4.5

Sheng Yang

author

2008-12-23 08:00 UTC #20 lore raw

PATCH 8/8 KVM: Merge MSI handling to kvm_set_irq

In reply to Sheng Yang

Using kvm_set_irq to handle all interrupt injection.

Signed-off-by Sheng Yang <[email protected]>

---
 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(-)

include/linux/kvm_host.h +1 -1

index aa2606b..5b671b6 100644
@@ -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);

virt/kvm/irq_comm.c +81 ~~-15~~

109 lines not on this page · raw message

virt/kvm/kvm_main.c +6 ~~-69~~

96 lines not on this page · raw message

Signature · 1 line

1.5.4.5

Marcelo Tosatti

2008-12-23 18:10 UTC #21 lore raw

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

Quoted 12 lines · Sheng Yang

Using kvm_set_irq to handle all interrupt injection.

Signed-off-by Sheng Yang <[email protected]>

---
 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(-)

virt/kvm/irq_comm.c line 24

Quoted 6 lines

virt/kvm/irq_comm.c +1 -1

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

virt/kvm/irq_comm.c line 27

Quoted 3 lines

virt/kvm/irq_comm.c +0 -3

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

Sheng Yang

author

2008-12-24 02:45 UTC #22 lore raw

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

Quoted 17 lines · On Wednesday 24 December 2008 02:10:20 Marcelo Tosatti wrote:

Quoted 16 lines · Sheng Yang
Using kvm_set_irq to handle all interrupt injection.
Signed-off-by Sheng Yang <[email protected]>
---
 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(-)
virt/kvm/irq_comm.c +0 -0
@@ -20,28 +20,94 @@
  */
 
 

Quoted 5 lines

virt/kvm/irq_comm.c line 70
Quoted 3 lines
virt/kvm/irq_comm.c +1 -1
 #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! :)

Quoted 7 lines

virt/kvm/irq_comm.c line 27
Quoted 4 lines
virt/kvm/irq_comm.c +0 -3
-#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.

Signature · 2 lines

regards
Yang, Sheng