Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7a455f070ce9afde6ee392f3c136acc144d7f08e.camel@linux.ibm.com>
Subject: Re: [RFT][PATCH v2 3/9] vfio/ccw: Only pass in contiguous pages
From:   Eric Farman <farman@linux.ibm.com>
To:     Nicolin Chen <nicolinc@nvidia.com>, kwankhede@nvidia.com,
        corbet@lwn.net, hca@linux.ibm.com, gor@linux.ibm.com,
        agordeev@linux.ibm.com, borntraeger@linux.ibm.com,
        svens@linux.ibm.com, zhenyuw@linux.intel.com, zhi.a.wang@intel.com,
        jani.nikula@linux.intel.com, joonas.lahtinen@linux.intel.com,
        rodrigo.vivi@intel.com, tvrtko.ursulin@linux.intel.com,
        airlied@linux.ie, daniel@ffwll.ch, mjrosato@linux.ibm.com,
        pasic@linux.ibm.com, vneethv@linux.ibm.com, oberpar@linux.ibm.com,
        freude@linux.ibm.com, akrowiak@linux.ibm.com,
        jjherne@linux.ibm.com, alex.williamson@redhat.com,
        cohuck@redhat.com, jgg@nvidia.com, kevin.tian@intel.com,
        hch@infradead.org
Cc:     jchrist@linux.ibm.com, kvm@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-s390@vger.kernel.org, intel-gvt-dev@lists.freedesktop.org,
        intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Date:   Fri, 08 Jul 2022 16:25:30 -0400
In-Reply-To: <20220706062759.24946-4-nicolinc@nvidia.com>
References: <20220706062759.24946-1-nicolinc@nvidia.com>
         <20220706062759.24946-4-nicolinc@nvidia.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@nvidia.com>

Reviewed-by: Eric Farman <farman@linux.ibm.com>

> ---
>  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);
>  }
>