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

[Avi Kivity <avi@redhat.com>]

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.