From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/3] KVM: Ioctls for init MSI-X entry Date: Wed, 18 Feb 2009 10:32:04 +0000 Message-ID: <499BE3A4.3010402@redhat.com> References: <1234950262-4142-1-git-send-email-sheng@linux.intel.com> <1234950262-4142-2-git-send-email-sheng@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Sheng Yang Return-path: Received: from mx2.redhat.com ([66.187.237.31]:41775 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751913AbZBRKbh (ORCPT ); Wed, 18 Feb 2009 05:31:37 -0500 In-Reply-To: <1234950262-4142-2-git-send-email-sheng@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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; > + r = kvm_vm_ioctl_set_msix_entry(kvm, &entry); > + if (r) > + goto out; > + break; > + } > #endif > +#endif /* KVM_CAP_IRQ_ROUTING */ > default: > r = kvm_arch_vm_ioctl(filp, ioctl, arg); > } > -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.