* [PATCH 0/8] MSI enhancement
@ 2008-12-23 8:00 Sheng Yang
2008-12-23 8:00 ` [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq Sheng Yang
` (7 more replies)
0 siblings, 8 replies; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
@ 2008-12-23 8:00 ` Sheng Yang
2008-12-23 17:32 ` Marcelo Tosatti
2008-12-23 8:00 ` [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq Sheng Yang
` (6 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
2008-12-23 8:00 ` [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq Sheng Yang
@ 2008-12-23 8:00 ` Sheng Yang
2008-12-23 15:18 ` Marcelo Tosatti
2008-12-23 8:00 ` [PATCH 3/8] KVM: Add support to disable MSI for assigned device Sheng Yang
` (5 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 3/8] KVM: Add support to disable MSI for assigned device
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
2008-12-23 8:00 ` [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq Sheng Yang
2008-12-23 8:00 ` [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq Sheng Yang
@ 2008-12-23 8:00 ` Sheng Yang
2008-12-23 8:00 ` [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI Sheng Yang
` (4 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
` (2 preceding siblings ...)
2008-12-23 8:00 ` [PATCH 3/8] KVM: Add support to disable MSI for assigned device Sheng Yang
@ 2008-12-23 8:00 ` Sheng Yang
2008-12-23 17:55 ` Marcelo Tosatti
2008-12-23 8:00 ` [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment Sheng Yang
` (3 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
` (3 preceding siblings ...)
2008-12-23 8:00 ` [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI Sheng Yang
@ 2008-12-23 8:00 ` Sheng Yang
2008-12-23 18:05 ` Marcelo Tosatti
2008-12-23 8:00 ` [PATCH 6/8] KVM: Improve MSI dispatch function Sheng Yang
` (2 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 6/8] KVM: Improve MSI dispatch function
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
` (4 preceding siblings ...)
2008-12-23 8:00 ` [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment Sheng Yang
@ 2008-12-23 8:00 ` Sheng Yang
2008-12-23 8:00 ` [PATCH 7/8] KVM: Using ioapic_irqchip() macro for kvm_set_irq Sheng Yang
2008-12-23 8:00 ` [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq Sheng Yang
7 siblings, 0 replies; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 7/8] KVM: Using ioapic_irqchip() macro for kvm_set_irq
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
` (5 preceding siblings ...)
2008-12-23 8:00 ` [PATCH 6/8] KVM: Improve MSI dispatch function Sheng Yang
@ 2008-12-23 8:00 ` Sheng Yang
2008-12-23 8:00 ` [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq Sheng Yang
7 siblings, 0 replies; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
` (6 preceding siblings ...)
2008-12-23 8:00 ` [PATCH 7/8] KVM: Using ioapic_irqchip() macro for kvm_set_irq Sheng Yang
@ 2008-12-23 8:00 ` Sheng Yang
2008-12-23 18:10 ` Marcelo Tosatti
7 siblings, 1 reply; 23+ messages in thread
From: Sheng Yang @ 2008-12-23 8:00 UTC (permalink / raw)
To: Avi Kivity; +Cc: kvm, 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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq
2008-12-23 8:00 ` [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq Sheng Yang
@ 2008-12-23 15:18 ` Marcelo Tosatti
2008-12-25 8:42 ` Sheng Yang
0 siblings, 1 reply; 23+ messages in thread
From: Marcelo Tosatti @ 2008-12-23 15:18 UTC (permalink / raw)
To: Sheng Yang; +Cc: Avi Kivity, kvm
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) ?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq
2008-12-23 8:00 ` [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq Sheng Yang
@ 2008-12-23 17:32 ` Marcelo Tosatti
2008-12-24 2:24 ` Sheng Yang
0 siblings, 1 reply; 23+ messages in thread
From: Marcelo Tosatti @ 2008-12-23 17:32 UTC (permalink / raw)
To: Sheng Yang; +Cc: Avi Kivity, kvm
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI
2008-12-23 8:00 ` [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI Sheng Yang
@ 2008-12-23 17:55 ` Marcelo Tosatti
2008-12-24 2:31 ` Sheng Yang
0 siblings, 1 reply; 23+ messages in thread
From: Marcelo Tosatti @ 2008-12-23 17:55 UTC (permalink / raw)
To: Sheng Yang; +Cc: Avi Kivity, kvm
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...
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment
2008-12-23 8:00 ` [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment Sheng Yang
@ 2008-12-23 18:05 ` Marcelo Tosatti
2008-12-24 2:41 ` Sheng Yang
0 siblings, 1 reply; 23+ messages in thread
From: Marcelo Tosatti @ 2008-12-23 18:05 UTC (permalink / raw)
To: Sheng Yang; +Cc: Avi Kivity, kvm
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq
2008-12-23 8:00 ` [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq Sheng Yang
@ 2008-12-23 18:10 ` Marcelo Tosatti
2008-12-24 2:44 ` Sheng Yang
0 siblings, 1 reply; 23+ messages in thread
From: Marcelo Tosatti @ 2008-12-23 18:10 UTC (permalink / raw)
To: Sheng Yang; +Cc: Avi Kivity, kvm
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq
2008-12-23 17:32 ` Marcelo Tosatti
@ 2008-12-24 2:24 ` Sheng Yang
2008-12-27 19:24 ` Marcelo Tosatti
0 siblings, 1 reply; 23+ messages in thread
From: Sheng Yang @ 2008-12-24 2:24 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Avi Kivity, kvm
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI
2008-12-23 17:55 ` Marcelo Tosatti
@ 2008-12-24 2:31 ` Sheng Yang
2008-12-25 1:59 ` Sheng Yang
0 siblings, 1 reply; 23+ messages in thread
From: Sheng Yang @ 2008-12-24 2:31 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Avi Kivity, kvm
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment
2008-12-23 18:05 ` Marcelo Tosatti
@ 2008-12-24 2:41 ` Sheng Yang
0 siblings, 0 replies; 23+ messages in thread
From: Sheng Yang @ 2008-12-24 2:41 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Avi Kivity, kvm
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq
2008-12-23 18:10 ` Marcelo Tosatti
@ 2008-12-24 2:44 ` Sheng Yang
0 siblings, 0 replies; 23+ messages in thread
From: Sheng Yang @ 2008-12-24 2:44 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Avi Kivity, kvm, Xiantao Zhang
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI
2008-12-24 2:31 ` Sheng Yang
@ 2008-12-25 1:59 ` Sheng Yang
2008-12-27 19:27 ` Marcelo Tosatti
0 siblings, 1 reply; 23+ messages in thread
From: Sheng Yang @ 2008-12-25 1:59 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Avi Kivity, kvm
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq
2008-12-23 15:18 ` Marcelo Tosatti
@ 2008-12-25 8:42 ` Sheng Yang
0 siblings, 0 replies; 23+ messages in thread
From: Sheng Yang @ 2008-12-25 8:42 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Avi Kivity, kvm
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq
2008-12-24 2:24 ` Sheng Yang
@ 2008-12-27 19:24 ` Marcelo Tosatti
0 siblings, 0 replies; 23+ messages in thread
From: Marcelo Tosatti @ 2008-12-27 19:24 UTC (permalink / raw)
To: Sheng Yang; +Cc: Avi Kivity, kvm
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI
2008-12-25 1:59 ` Sheng Yang
@ 2008-12-27 19:27 ` Marcelo Tosatti
2008-12-28 11:14 ` Sheng Yang
0 siblings, 1 reply; 23+ messages in thread
From: Marcelo Tosatti @ 2008-12-27 19:27 UTC (permalink / raw)
To: Sheng Yang; +Cc: Avi Kivity, kvm
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?
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI
2008-12-27 19:27 ` Marcelo Tosatti
@ 2008-12-28 11:14 ` Sheng Yang
0 siblings, 0 replies; 23+ messages in thread
From: Sheng Yang @ 2008-12-28 11:14 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Avi Kivity, kvm
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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2008-12-28 11:14 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-12-23 8:00 [PATCH 0/8] MSI enhancement Sheng Yang
2008-12-23 8:00 ` [PATCH 1/8] KVM: Add MSI_ACTION flag for assigned irq Sheng Yang
2008-12-23 17:32 ` Marcelo Tosatti
2008-12-24 2:24 ` Sheng Yang
2008-12-27 19:24 ` Marcelo Tosatti
2008-12-23 8:00 ` [PATCH 2/8] KVM: Use kvm_free_assigned_irq() for free irq Sheng Yang
2008-12-23 15:18 ` Marcelo Tosatti
2008-12-25 8:42 ` Sheng Yang
2008-12-23 8:00 ` [PATCH 3/8] KVM: Add support to disable MSI for assigned device Sheng Yang
2008-12-23 8:00 ` [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI Sheng Yang
2008-12-23 17:55 ` Marcelo Tosatti
2008-12-24 2:31 ` Sheng Yang
2008-12-25 1:59 ` Sheng Yang
2008-12-27 19:27 ` Marcelo Tosatti
2008-12-28 11:14 ` Sheng Yang
2008-12-23 8:00 ` [PATCH 5/8] KVM: Using gsi_msg mapping for MSI device assignment Sheng Yang
2008-12-23 18:05 ` Marcelo Tosatti
2008-12-24 2:41 ` Sheng Yang
2008-12-23 8:00 ` [PATCH 6/8] KVM: Improve MSI dispatch function Sheng Yang
2008-12-23 8:00 ` [PATCH 7/8] KVM: Using ioapic_irqchip() macro for kvm_set_irq Sheng Yang
2008-12-23 8:00 ` [PATCH 8/8] KVM: Merge MSI handling to kvm_set_irq Sheng Yang
2008-12-23 18:10 ` Marcelo Tosatti
2008-12-24 2:44 ` Sheng Yang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.