Subject: Re: [PATCH v11 05/22] vfio iommu: Added pin and unpin callback
 functions to vfio_iommu_driver_ops
To: Alex Williamson <alex.williamson@redhat.com>
References: <1478293856-8191-1-git-send-email-kwankhede@nvidia.com>
 <1478293856-8191-6-git-send-email-kwankhede@nvidia.com>
 <20161107123649.0f3d56e9@t450s.home>
CC: <pbonzini@redhat.com>, <kraxel@redhat.com>, <cjia@nvidia.com>,
        <qemu-devel@nongnu.org>, <kvm@vger.kernel.org>, <kevin.tian@intel.com>,
        <jike.song@intel.com>, <bjsdjshi@linux.vnet.ibm.com>,
        <linux-kernel@vger.kernel.org>
From: Kirti Wankhede <kwankhede@nvidia.com>
Message-ID: <f289d950-6c1a-69f8-4658-4a7a85ea8c0e@nvidia.com>
Date: Tue, 8 Nov 2016 19:25:35 +0530
MIME-Version: 1.0
In-Reply-To: <20161107123649.0f3d56e9@t450s.home>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3046
Lines: 103


On 11/8/2016 1:06 AM, Alex Williamson wrote:
> On Sat, 5 Nov 2016 02:40:39 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
...
>> +int vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
>> +		   int npage, int prot, unsigned long *phys_pfn)
>> +{
>> +	struct vfio_container *container;
>> +	struct vfio_group *group;
>> +	struct vfio_iommu_driver *driver;
>> +	int ret;
>> +
>> +	if (!dev || !user_pfn || !phys_pfn)
>> +		return -EINVAL;
>> +
>> +	group = vfio_group_get_from_dev(dev);
>> +	if (IS_ERR(group))
>> +		return PTR_ERR(group);
>> +
>> +	ret = vfio_group_add_container_user(group);
>> +	if (ret)
>> +		goto err_pin_pages;
>> +
>> +	container = group->container;
>> +	down_read(&container->group_lock);
>> +
>> +	driver = container->iommu_driver;
>> +	if (likely(driver && driver->ops->pin_pages))
>> +		ret = driver->ops->pin_pages(container->iommu_data, user_pfn,
>> +					     npage, prot, phys_pfn);
>> +	else
>> +		ret = -EINVAL;
> 
> -ENOTTY might be a more appropriate error return here and below since
> we're not signaling invalid argument, we're signaling lack of support.
> 

Used -EINVAL in sync with other driver->ops like read, write and mmap.
Changing it to -ENOTTY as you suggested above since these ops are optional.

...

>> -static long vfio_pin_pages(unsigned long vaddr, long npage,
>> -			   int prot, unsigned long *pfn_base)
>> +static long __vfio_pin_pages_remote(unsigned long vaddr, long npage,
>> +				    int prot, unsigned long *pfn_base)
> 
> nit, what is the additional underscore prefix intended to imply?
> Appending _remote is sufficient to avoid the symbol conflict.
>

This function name changed in review process from start, we started with
changing to __vfio_pin_pages and then added _remote to it later. We can
remove '__' from it. Updating.

...

>> -
>> +	int		(*pin_pages)(void *iommu_data, unsigned long *user_pfn,
>> +				     int npage, int prot,
>> +				     unsigned long *phys_pfn);
>> +	int		(*unpin_pages)(void *iommu_data,
> 
> Are we changing from long to int here simply because of the absurdity
> in passing in more than a 2^31 entry array, that would already consume
> more than 16GB itself?
> 

These are on demand pin/unpin request, will that request go beyond 16GB
limit? For Nvidia vGPU solution, pin request will not go beyond this limit.

>> +				       unsigned long *user_pfn,
>> +				       unsigned long *pfn,
> 
> nit, use phys_pfn so as to match the pin function?
> 

Ok.

>> +				       int npage);
>>  };
>>  
>>  extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
>> @@ -127,6 +133,12 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
>>  }
>>  #endif /* CONFIG_EEH */
>>  
>> +extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn,
>> +			  int npage, int prot, unsigned long *phys_pfn);
>> +
>> +extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn,
>> +			    unsigned long *pfn, int npage);
>> +
>>  /*
>>   * IRQfd - generic
>>   */
> 

Thanks,
Kirti