Re: [RFT][PATCH v2 3/9] vfio/ccw: Only pass in contiguous pages

[Eric Farman]

On Tue, 2022-07-05 at 23:27 -0700, Nicolin Chen wrote:
> This driver is the only caller of vfio_pin/unpin_pages that might
> pass
> in a non-contiguous PFN list, but in many cases it has a contiguous
> PFN
> list to process. So letting VFIO API handle a non-contiguous PFN list
> is actually counterproductive.
> 
> Add a pair of simple loops to pass in contiguous PFNs only, to have
> an
> efficient implementation in VFIO.
> 
> Signed-off-by: Nicolin Chen <nicolinc@xxxxxxxxxx>

Reviewed-by: Eric Farman <farman@xxxxxxxxxxxxx>

> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 70 +++++++++++++++++++++++++++-----
> --
>  1 file changed, 56 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> b/drivers/s390/cio/vfio_ccw_cp.c
> index 0c2be9421ab7..3b94863ad24e 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -90,6 +90,38 @@ static int pfn_array_alloc(struct pfn_array *pa,
> u64 iova, unsigned int len)
>  	return 0;
>  }
>  
> +/*
> + * pfn_array_unpin() - Unpin user pages in memory
> + * @pa: pfn_array on which to perform the operation
> + * @vdev: the vfio device to perform the operation
> + * @pa_nr: number of user pages to unpin
> + *
> + * Only unpin if any pages were pinned to begin with, i.e. pa_nr >
> 0,
> + * otherwise only clear pa->pa_nr
> + */
> +static void pfn_array_unpin(struct pfn_array *pa,
> +			    struct vfio_device *vdev, int pa_nr)
> +{
> +	int unpinned = 0, npage = 1;
> +
> +	while (unpinned < pa_nr) {
> +		unsigned long *first = &pa->pa_iova_pfn[unpinned];
> +		unsigned long *last = &first[npage];
> +
> +		if (unpinned + npage < pa_nr &&
> +		    *first + npage == *last) {
> +			npage++;
> +			continue;
> +		}
> +
> +		vfio_unpin_pages(vdev, first, npage);
> +		unpinned += npage;
> +		npage = 1;
> +	}
> +
> +	pa->pa_nr = 0;
> +}
> +
>  /*
>   * pfn_array_pin() - Pin user pages in memory
>   * @pa: pfn_array on which to perform the operation
> @@ -101,34 +133,44 @@ static int pfn_array_alloc(struct pfn_array
> *pa, u64 iova, unsigned int len)
>   */
>  static int pfn_array_pin(struct pfn_array *pa, struct vfio_device
> *vdev)
>  {
> +	int pinned = 0, npage = 1;
>  	int ret = 0;
>  
> -	ret = vfio_pin_pages(vdev, pa->pa_iova_pfn, pa->pa_nr,
> -			     IOMMU_READ | IOMMU_WRITE, pa->pa_pfn);
> +	while (pinned < pa->pa_nr) {
> +		unsigned long *first = &pa->pa_iova_pfn[pinned];
> +		unsigned long *last = &first[npage];
>  
> -	if (ret < 0) {
> -		goto err_out;
> -	} else if (ret > 0 && ret != pa->pa_nr) {
> -		vfio_unpin_pages(vdev, pa->pa_iova_pfn, ret);
> -		ret = -EINVAL;
> -		goto err_out;
> +		if (pinned + npage < pa->pa_nr &&
> +		    *first + npage == *last) {
> +			npage++;
> +			continue;
> +		}
> +
> +		ret = vfio_pin_pages(vdev, first, npage,
> +				     IOMMU_READ | IOMMU_WRITE,
> +				     &pa->pa_pfn[pinned]);
> +		if (ret < 0) {
> +			goto err_out;
> +		} else if (ret > 0 && ret != npage) {
> +			pinned += ret;
> +			ret = -EINVAL;
> +			goto err_out;
> +		}
> +		pinned += npage;
> +		npage = 1;
>  	}
>  
>  	return ret;
>  
>  err_out:
> -	pa->pa_nr = 0;
> -
> +	pfn_array_unpin(pa, vdev, pinned);
>  	return ret;
>  }
>  
>  /* Unpin the pages before releasing the memory. */
>  static void pfn_array_unpin_free(struct pfn_array *pa, struct
> vfio_device *vdev)
>  {
> -	/* Only unpin if any pages were pinned to begin with */
> -	if (pa->pa_nr)
> -		vfio_unpin_pages(vdev, pa->pa_iova_pfn, pa->pa_nr);
> -	pa->pa_nr = 0;
> +	pfn_array_unpin(pa, vdev, pa->pa_nr);
>  	kfree(pa->pa_iova_pfn);
>  }
>