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

2009-02-18 Thread Avi Kivity 

Sheng Yang wrote:

Introduce KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY two ioctls.

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

MSI-X should be well initialzed before enabling.

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

  


Sorry, this has been reviewed quite a bit but I found a few issues:


diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 2163b3d..a2dfbe0 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -475,6 +475,8 @@ struct kvm_irq_routing {
 #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
struct kvm_assigned_irq)
 #define KVM_REINJECT_CONTROL  _IO(KVMIO, 0x71)
+#define KVM_SET_MSIX_NR _IOR(KVMIO, 0x72, struct kvm_assigned_msix_nr)
+#define KVM_SET_MSIX_ENTRY _IOR(KVMIO, 0x73, struct kvm_assigned_msix_entry)
  


KVM_SET_ASSIGNED_... so it's associated with device assignment, not generic.

Should be _IOW, not _IOR.  Looks like KVM_ASSIGN_IRQ is broken...

 
+static int kvm_vm_ioctl_set_msix_nr(struct kvm *kvm,

+   struct kvm_assigned_msix_nr *entry_nr)
+{
+   int r = 0;
+   struct kvm_assigned_dev_kernel *adev;
+
+   mutex_lock(kvm-lock);
+
+   adev = kvm_find_assigned_dev(kvm-arch.assigned_dev_head,
+ entry_nr-assigned_dev_id);
+   if (!adev) {
+   r = -EINVAL;
+   goto msix_nr_out;
+   }
+
+   if (adev-entries_nr == 0) {
+   adev-entries_nr = entry_nr-entry_nr;
+   if (adev-entries_nr == 0 ||
+   adev-entries_nr = KVM_MAX_MSIX_PER_DEV)
+   goto msix_nr_out;
  


r == 0 here, needs a meaningful error number.


+
+   adev-host_msix_entries = kzalloc(sizeof(struct msix_entry) *
+   entry_nr-entry_nr,
+   GFP_KERNEL);
+   if (!adev-host_msix_entries) {
+   printk(KERN_ERR no memory for host msix entries!\n);
+   r = -ENOMEM;
  


Drop the printk, -ENOMEM is enough.


+   goto msix_nr_out;
+   }
+   adev-guest_msix_entries = kzalloc(sizeof(struct msix_entry) *
+   entry_nr-entry_nr,
+   GFP_KERNEL);
+   if (!adev-guest_msix_entries) {
+   printk(KERN_ERR no memory for host msix entries!\n);
  


Ditto.


+   kfree(adev-host_msix_entries);
+   r = -ENOMEM;
+   goto msix_nr_out;
+   }
+   } else
+   printk(KERN_WARNING kvm: not allow recall set msix nr!\n);
  


Drop printk, add error.


+msix_nr_out:
+   mutex_unlock(kvm-lock);
+   return r;
+}
+
+static int kvm_vm_ioctl_set_msix_entry(struct kvm *kvm,
+  struct kvm_assigned_msix_entry *entry)
+{
+   int r = 0, i;
+   struct kvm_assigned_dev_kernel *adev;
+
+   mutex_lock(kvm-lock);
+
+   adev = kvm_find_assigned_dev(kvm-arch.assigned_dev_head,
+ entry-assigned_dev_id);
+
+   if (!adev) {
+   r = -EINVAL;
+   goto msix_entry_out;
+   }
+
+   for (i = 0; i  adev-entries_nr; i++)
+   if (adev-guest_msix_entries[i].vector == 0 ||
+   adev-guest_msix_entries[i].entry == entry-entry) {
+   adev-guest_msix_entries[i].entry = entry-entry;
+   adev-guest_msix_entries[i].vector = entry-gsi;
+   adev-host_msix_entries[i].entry = entry-entry;
+   break;
+   }
+   if (i == adev-entries_nr) {
+   printk(KERN_ERR kvm: Too much entries for MSI-X!\n);
  


Drop.


+   r = -ENOSPC;
+   goto msix_entry_out;
+   }
+
+msix_entry_out:
+   mutex_unlock(kvm-lock);
+
+   return r;
+}
+
 static long kvm_vcpu_ioctl(struct file *filp,
   unsigned int ioctl, unsigned long arg)
 {
@@ -1917,7 +1998,29 @@ static long kvm_vm_ioctl(struct file *filp,
vfree(entries);
break;
}
+#ifdef KVM_CAP_DEVICE_MSIX
+   case KVM_SET_MSIX_NR: {
+   struct kvm_assigned_msix_nr entry_nr;
+   r = -EFAULT;
+   if (copy_from_user(entry_nr, argp, sizeof entry_nr))
+   goto out;
+   r = kvm_vm_ioctl_set_msix_nr(kvm, entry_nr);
+   if (r)
+   goto out;
+   break;
+   }
+   case KVM_SET_MSIX_ENTRY: {
+   struct kvm_assigned_msix_entry entry;
+   r = -EFAULT;
+   if (copy_from_user(entry, argp, sizeof entry))
+   goto out;
+

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

2009-02-16 Thread Marcelo Tosatti 
On Mon, Feb 16, 2009 at 01:49:29PM +0800, Sheng Yang wrote:
 (oops... fix a typo)
 
 Introduce KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY two ioctls.
 
 This two ioctls are used by userspace to specific guest device MSI-X entry
 number and correlate MSI-X entry with GSI during the initialization stage.
 
 MSI-X should be well initialzed before enabling.
 
 Don't support change MSI-X entry number for now.
 
 Signed-off-by: Sheng Yang sh...@linux.intel.com
 ---
  include/linux/kvm.h  |   16 +++
  include/linux/kvm_host.h |3 +
  virt/kvm/kvm_main.c  |  103 
 ++
  3 files changed, 122 insertions(+), 0 deletions(-)
 
 diff --git a/include/linux/kvm.h b/include/linux/kvm.h
 index 2163b3d..a2dfbe0 100644
 --- a/include/linux/kvm.h
 +++ b/include/linux/kvm.h
 @@ -475,6 +475,8 @@ struct kvm_irq_routing {
  #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
   struct kvm_assigned_irq)
  #define KVM_REINJECT_CONTROL  _IO(KVMIO, 0x71)
 +#define KVM_SET_MSIX_NR _IOR(KVMIO, 0x72, struct kvm_assigned_msix_nr)
 +#define KVM_SET_MSIX_ENTRY _IOR(KVMIO, 0x73, struct kvm_assigned_msix_entry)
  
  /*
   * ioctls for vcpu fds
 @@ -595,4 +597,18 @@ struct kvm_assigned_irq {
  #define KVM_DEV_IRQ_ASSIGN_MSI_ACTIONKVM_DEV_IRQ_ASSIGN_ENABLE_MSI
  #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI(1  0)
  
 +struct kvm_assigned_msix_nr {
 + __u32 assigned_dev_id;
 + __u16 entry_nr;
 + __u16 padding;
 +};
 +
 +#define KVM_MAX_MSIX_PER_DEV 512
 +struct kvm_assigned_msix_entry {
 + __u32 assigned_dev_id;
 + __u32 gsi;
 + __u16 entry; /* The index of entry in the MSI-X table */
 + __u16 padding[3];
 +};
 +
  #endif
 diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
 index 7c7096d..ac4d63c 100644
 --- a/include/linux/kvm_host.h
 +++ b/include/linux/kvm_host.h
 @@ -326,9 +326,12 @@ struct kvm_assigned_dev_kernel {
   int assigned_dev_id;
   int host_busnr;
   int host_devfn;
 + int entries_nr;
   int host_irq;
   bool host_irq_disabled;
 + struct msix_entry *host_msix_entries;
   int guest_irq;
 + struct msix_entry *guest_msix_entries;
  #define KVM_ASSIGNED_DEV_GUEST_INTX  (1  0)
  #define KVM_ASSIGNED_DEV_GUEST_MSI   (1  1)
  #define KVM_ASSIGNED_DEV_HOST_INTX   (1  8)
 diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
 index 266bdaf..bb4aa73 100644
 --- a/virt/kvm/kvm_main.c
 +++ b/virt/kvm/kvm_main.c
 @@ -1593,6 +1593,87 @@ static int kvm_vcpu_ioctl_set_sigmask(struct kvm_vcpu 
 *vcpu, sigset_t *sigset)
   return 0;
  }
  
 +static int kvm_vm_ioctl_set_msix_nr(struct kvm *kvm,
 + struct kvm_assigned_msix_nr *entry_nr)
 +{
 + int r = 0;
 + struct kvm_assigned_dev_kernel *adev;
 +
 + mutex_lock(kvm-lock);
 +
 + adev = kvm_find_assigned_dev(kvm-arch.assigned_dev_head,
 +   entry_nr-assigned_dev_id);
 + if (!adev) {
 + r = -EINVAL;
 + goto msix_nr_out;
 + }
 +
 + if (adev-entries_nr == 0) {
 + adev-entries_nr = entry_nr-entry_nr;
 + if (adev-entries_nr == 0 ||

= 0 ? 

 + adev-entries_nr = KVM_MAX_MSIX_PER_DEV)

Otherwise looks OK, thanks.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2009-02-16 Thread Sheng Yang 
On Tuesday 17 February 2009 07:23:23 Marcelo Tosatti wrote:
 On Mon, Feb 16, 2009 at 01:49:29PM +0800, Sheng Yang wrote:
  (oops... fix a typo)
 
  Introduce KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY two ioctls.
 
  This two ioctls are used by userspace to specific guest device MSI-X
  entry number and correlate MSI-X entry with GSI during the initialization
  stage.
 
  MSI-X should be well initialzed before enabling.
 
  Don't support change MSI-X entry number for now.
 
  Signed-off-by: Sheng Yang sh...@linux.intel.com
  ---
   include/linux/kvm.h  |   16 +++
   include/linux/kvm_host.h |3 +
   virt/kvm/kvm_main.c  |  103
  ++ 3 files changed, 122
  insertions(+), 0 deletions(-)
 
  diff --git a/include/linux/kvm.h b/include/linux/kvm.h
  index 2163b3d..a2dfbe0 100644
  --- a/include/linux/kvm.h
  +++ b/include/linux/kvm.h
  @@ -475,6 +475,8 @@ struct kvm_irq_routing {
   #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
  struct kvm_assigned_irq)
   #define KVM_REINJECT_CONTROL  _IO(KVMIO, 0x71)
  +#define KVM_SET_MSIX_NR _IOR(KVMIO, 0x72, struct kvm_assigned_msix_nr)
  +#define KVM_SET_MSIX_ENTRY _IOR(KVMIO, 0x73, struct
  kvm_assigned_msix_entry)
 
   /*
* ioctls for vcpu fds
  @@ -595,4 +597,18 @@ struct kvm_assigned_irq {
   #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION  KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
   #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI  (1  0)
 
  +struct kvm_assigned_msix_nr {
  +   __u32 assigned_dev_id;
  +   __u16 entry_nr;
  +   __u16 padding;
  +};
  +
  +#define KVM_MAX_MSIX_PER_DEV   512
  +struct kvm_assigned_msix_entry {
  +   __u32 assigned_dev_id;
  +   __u32 gsi;
  +   __u16 entry; /* The index of entry in the MSI-X table */
  +   __u16 padding[3];
  +};
  +
   #endif
  diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
  index 7c7096d..ac4d63c 100644
  --- a/include/linux/kvm_host.h
  +++ b/include/linux/kvm_host.h
  @@ -326,9 +326,12 @@ struct kvm_assigned_dev_kernel {
  int assigned_dev_id;
  int host_busnr;
  int host_devfn;
  +   int entries_nr;
  int host_irq;
  bool host_irq_disabled;
  +   struct msix_entry *host_msix_entries;
  int guest_irq;
  +   struct msix_entry *guest_msix_entries;
   #define KVM_ASSIGNED_DEV_GUEST_INTX(1  0)
   #define KVM_ASSIGNED_DEV_GUEST_MSI (1  1)
   #define KVM_ASSIGNED_DEV_HOST_INTX (1  8)
  diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
  index 266bdaf..bb4aa73 100644
  --- a/virt/kvm/kvm_main.c
  +++ b/virt/kvm/kvm_main.c
  @@ -1593,6 +1593,87 @@ static int kvm_vcpu_ioctl_set_sigmask(struct
  kvm_vcpu *vcpu, sigset_t *sigset) return 0;
   }
 
  +static int kvm_vm_ioctl_set_msix_nr(struct kvm *kvm,
  +   struct kvm_assigned_msix_nr *entry_nr)
  +{
  +   int r = 0;
  +   struct kvm_assigned_dev_kernel *adev;
  +
  +   mutex_lock(kvm-lock);
  +
  +   adev = kvm_find_assigned_dev(kvm-arch.assigned_dev_head,
  + entry_nr-assigned_dev_id);
  +   if (!adev) {
  +   r = -EINVAL;
  +   goto msix_nr_out;
  +   }
  +
  +   if (adev-entries_nr == 0) {
  +   adev-entries_nr = entry_nr-entry_nr;
  +   if (adev-entries_nr == 0 ||

 = 0 ?

Oh, yeah, that's right. :)

I would change the type of adev-entries_nr to unsigned int.

-- 
regards
Yang, Sheng


  +   adev-entries_nr = KVM_MAX_MSIX_PER_DEV)

 Otherwise looks OK, thanks.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2009-02-12 Thread Marcelo Tosatti 
On Thu, Feb 12, 2009 at 06:07:48PM +0800, Sheng Yang wrote:
 Introduce KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY two ioctls.
 
 This two ioctls are used by userspace to specific guest device MSI-X entry
 number and correlate MSI-X entry with GSI during the initialization stage.
 
 MSI-X should be well initialzed before enabling.
 
 Don't support change MSI-X entry number for now.
 
 Signed-off-by: Sheng Yang sh...@linux.intel.com

 + adev-host_msix_entries = kzalloc(sizeof(struct msix_entry) *
 + entry_nr-entry_nr,
 + GFP_KERNEL);

 + if (!adev-host_msix_entries) {

Please verify its within a sane limit. Like the msr entry ioctl code.
Also free host_msix_entries if guest_msix_entries allocation fails.

Otherwise seems OK.

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2009-02-11 Thread Avi Kivity 

Sheng Yang wrote:

Introduce KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY two ioctls.

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

MSI-X should be well initialzed before enabling.

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

Signed-off-by: Sheng Yang sh...@linux.intel.com
---
 include/linux/kvm.h  |   14 +++
 include/linux/kvm_host.h |3 +
 virt/kvm/kvm_main.c  |   96 ++
 3 files changed, 113 insertions(+), 0 deletions(-)

diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index c1425ab..5200768 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -474,6 +474,8 @@ struct kvm_irq_routing {
 #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
struct kvm_assigned_irq)
 #define KVM_REINJECT_CONTROL  _IO(KVMIO, 0x71)
+#define KVM_SET_MSIX_NR _IOR(KVMIO, 0x72, struct kvm_assigned_msix_nr)
+#define KVM_SET_MSIX_ENTRY _IOR(KVMIO, 0x73, struct kvm_assigned_msix_entry)
 
 /*

  * ioctls for vcpu fds
@@ -594,4 +596,16 @@ struct kvm_assigned_irq {
 #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION  KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
 #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI  (1  0)
 
+struct kvm_assigned_msix_nr {

+   __u32 assigned_dev_id;
+   __u16 entry_nr;
+};
+
+struct kvm_assigned_msix_entry {
+   __u32 assigned_dev_id;
+   __u32 gsi;
+   __u16 entry;
+   __u16 pos;
+}


I'm guessing the intent here is msi-x number 'pos' on the host assigned 
device will be injected to the guest as gsi 'gsi'?  What's the meaning 
of 'entry' and 'pos'?


Both structures need better padding and some documentation.

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

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2009-02-11 Thread Sheng Yang 
On Wednesday 11 February 2009 20:44:57 Avi Kivity wrote:
 Sheng Yang wrote:
  Introduce KVM_SET_MSIX_NR and KVM_SET_MSIX_ENTRY two ioctls.
 
  This two ioctls are used by userspace to specific guest device MSI-X
  entry number and correlate MSI-X entry with GSI during the initialization
  stage.
 
  MSI-X should be well initialzed before enabling.
 
  Don't support change MSI-X entry number for now.
 
  Signed-off-by: Sheng Yang sh...@linux.intel.com
  ---
   include/linux/kvm.h  |   14 +++
   include/linux/kvm_host.h |3 +
   virt/kvm/kvm_main.c  |   96
  ++ 3 files changed, 113
  insertions(+), 0 deletions(-)
 
  diff --git a/include/linux/kvm.h b/include/linux/kvm.h
  index c1425ab..5200768 100644
  --- a/include/linux/kvm.h
  +++ b/include/linux/kvm.h
  @@ -474,6 +474,8 @@ struct kvm_irq_routing {
   #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
  struct kvm_assigned_irq)
   #define KVM_REINJECT_CONTROL  _IO(KVMIO, 0x71)
  +#define KVM_SET_MSIX_NR _IOR(KVMIO, 0x72, struct kvm_assigned_msix_nr)
  +#define KVM_SET_MSIX_ENTRY _IOR(KVMIO, 0x73, struct
  kvm_assigned_msix_entry)
 
   /*
* ioctls for vcpu fds
  @@ -594,4 +596,16 @@ struct kvm_assigned_irq {
   #define KVM_DEV_IRQ_ASSIGN_MSI_ACTION  KVM_DEV_IRQ_ASSIGN_ENABLE_MSI
   #define KVM_DEV_IRQ_ASSIGN_ENABLE_MSI  (1  0)
 
  +struct kvm_assigned_msix_nr {
  +   __u32 assigned_dev_id;
  +   __u16 entry_nr;
  +};
  +
  +struct kvm_assigned_msix_entry {
  +   __u32 assigned_dev_id;
  +   __u32 gsi;
  +   __u16 entry;
  +   __u16 pos;
  +}

 I'm guessing the intent here is msi-x number 'pos' on the host assigned
 device will be injected to the guest as gsi 'gsi'?  What's the meaning
 of 'entry' and 'pos'?

Sorry for the ambiguous. entry is the index in device MSI-X table(including 
empty entry); pos is without the empty entry, and should be less than the 
total valid entry numbers that table have. pos is mostly a assistant here, I 
think it can be estimated.

 Both structures need better padding and some documentation.

Of course. Would update the patches.

-- 
regards
Yang, Sheng
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html