Re: [Qemu-devel] [PATCH v13 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops

[Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>]

* Kirti Wankhede <kwankhede@nvidia.com> [2016-11-15 20:59:48 +0530]:

Hi Kirti,

> Added APIs for pining and unpining set of pages. These call back into
> backend iommu module to actually pin and unpin pages.
> Added two new callback functions to struct vfio_iommu_driver_ops. Backend
> IOMMU module that supports pining and unpinning pages for mdev devices
> should provide these functions.
> 
> Renamed static functions in vfio_type1_iommu.c to resolve conflicts
> 
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Neo Jia <cjia@nvidia.com>
> Change-Id: Ia7417723aaae86bec2959ad9ae6c2915ddd340e0
> ---
>  drivers/vfio/vfio.c             | 103 ++++++++++++++++++++++++++++++++++++++++
>  drivers/vfio/vfio_iommu_type1.c |  20 ++++----
>  include/linux/vfio.h            |  14 +++++-
>  3 files changed, 126 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> index 2e83bdf007fe..3bf8a01bf67b 100644
> --- a/drivers/vfio/vfio.c
> +++ b/drivers/vfio/vfio.c
> @@ -1799,6 +1799,109 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset)
>  }
>  EXPORT_SYMBOL_GPL(vfio_info_cap_shift);
> 
> +
> +/*
> + * Pin a set of guest PFNs and return their associated host PFNs for local
> + * domain only.
> + * @dev [in] : device
> + * @user_pfn [in]: array of user/guest PFNs to be unpinned. Number of user/guest
> + *		  PFNs should not be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
Move the second sentence to the @npage section?

> + * @npage [in] :count of elements in array.  This count should not be greater
> + *		than PAGE_SIZE.
And remove the second sentence here.

> + * @prot [in] : protection flags
> + * @phys_pfn[out] : array of host PFNs
nit:
I saw three differnt styles here:
 @xxx [in] :xxxxxxx
 @xxx [in]: xxxxxxx
 @xxx[out]: xxxxxxx

Frankly speeking, I didn't think the [in|out] flags helps much.

> + * Return error or number of pages pinned.
> + */
> +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 || !npage)
> +		return -EINVAL;
> +
> +	if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
> +		return -E2BIG;
> +
> +	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 = -ENOTTY;
> +
> +	up_read(&container->group_lock);
> +	vfio_group_try_dissolve_container(group);
> +
> +err_pin_pages:
> +	vfio_group_put(group);
> +	return ret;
> +}
> +EXPORT_SYMBOL(vfio_pin_pages);
> +
> +/*
> + * Unpin set of host PFNs for local domain only.
> + * @dev [in] : device
> + * @user_pfn [in]: array of user/guest PFNs to be unpinned. Number of user/guest
> + *		  PFNs should not be greater than VFIO_PIN_PAGES_MAX_ENTRIES.
> + * @npage [in] :count of elements in array.  This count should not be greater
> + *		than PAGE_SIZE.
Same nits as above here.

> + * Return error or number of pages unpinned.
> + */
[...]

> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 0ecae0b1cd34..420cdc928786 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -75,7 +75,11 @@ struct vfio_iommu_driver_ops {
>  					struct iommu_group *group);
>  	void		(*detach_group)(void *iommu_data,
>  					struct iommu_group *group);
> -
> +	int		(*pin_pages)(void *iommu_data, unsigned long *user_pfn,
> +				     int npage, int prot,
> +				     unsigned long *phys_pfn);
> +	int		(*unpin_pages)(void *iommu_data,
> +				       unsigned long *user_pfn, int npage);
>  };
> 
>  extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);
> @@ -127,6 +131,14 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
>  }
>  #endif /* CONFIG_EEH */
> 
> +#define VFIO_PIN_PAGES_MAX_ENTRIES	(PAGE_SIZE/sizeof(unsigned long))
> +
> +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,
> +			    int npage);
> +
Move this hunk up to the "External user API" section?

>  /*
>   * IRQfd - generic
>   */
> -- 
> 2.7.0
> 

The code looks good to me.

-- 
Dong Jia