Re: [PATCH v1 15/16] vfio/ccw: don't group contiguous pages on 2K IDAWs

[Matthew Rosato <mjrosato@linux.ibm.com>]

On 11/21/22 4:40 PM, Eric Farman wrote:
> The vfio_pin_pages() interface allows contiguous pages to be
> pinned as a single request, which is great for the 4K pages
> that are normally processed. Old IDA formats operate on 2K
> chunks, which makes this logic more difficult.
> 
> Since these formats are rare, let's just invoke the page
> pinning one-at-a-time, instead of trying to group them.
> We can rework this code at a later date if needed.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 9527f3d8da77..3829c346583c 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -88,7 +88,7 @@ static int page_array_alloc(struct page_array *pa, unsigned int len)
>   * otherwise only clear pa->pa_nr
>   */
>  static void page_array_unpin(struct page_array *pa,
> -			     struct vfio_device *vdev, int pa_nr)
> +			     struct vfio_device *vdev, int pa_nr, bool unaligned)

Please add 'unaligned' the comment block above this function with a short explanation...

>  {
>  	int unpinned = 0, npage = 1;
>  
> @@ -97,7 +97,8 @@ static void page_array_unpin(struct page_array *pa,
>  		dma_addr_t *last = &first[npage];
>  
>  		if (unpinned + npage < pa_nr &&
> -		    *first + npage * PAGE_SIZE == *last) {
> +		    *first + npage * PAGE_SIZE == *last &&
> +		    !unaligned) {
>  			npage++;
>  			continue;
>  		}
> @@ -119,7 +120,7 @@ static void page_array_unpin(struct page_array *pa,
>   * If the pin request partially succeeds, or fails completely,
>   * all pages are left unpinned and a negative error value is returned.
>   */
> -static int page_array_pin(struct page_array *pa, struct vfio_device *vdev)
> +static int page_array_pin(struct page_array *pa, struct vfio_device *vdev, bool unaligned)

...  Also here.  Otherwise, I agree re-work can be done later since this is not a common case.

With those changes:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

>  {
>  	int pinned = 0, npage = 1;
>  	int ret = 0;
> @@ -129,7 +130,8 @@ static int page_array_pin(struct page_array *pa, struct vfio_device *vdev)
>  		dma_addr_t *last = &first[npage];
>  
>  		if (pinned + npage < pa->pa_nr &&
> -		    *first + npage * PAGE_SIZE == *last) {
> +		    *first + npage * PAGE_SIZE == *last &&
> +		    !unaligned) {
>  			npage++;
>  			continue;
>  		}
> @@ -151,14 +153,14 @@ static int page_array_pin(struct page_array *pa, struct vfio_device *vdev)
>  	return ret;
>  
>  err_out:
> -	page_array_unpin(pa, vdev, pinned);
> +	page_array_unpin(pa, vdev, pinned, unaligned);
>  	return ret;
>  }
>  
>  /* Unpin the pages before releasing the memory. */
> -static void page_array_unpin_free(struct page_array *pa, struct vfio_device *vdev)
> +static void page_array_unpin_free(struct page_array *pa, struct vfio_device *vdev, bool unaligned)
>  {
> -	page_array_unpin(pa, vdev, pa->pa_nr);
> +	page_array_unpin(pa, vdev, pa->pa_nr, unaligned);
>  	kfree(pa->pa_page);
>  	kfree(pa->pa_iova);
>  }
> @@ -638,7 +640,7 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
>  	}
>  
>  	if (ccw_does_data_transfer(ccw)) {
> -		ret = page_array_pin(pa, vdev);
> +		ret = page_array_pin(pa, vdev, idal_is_2k(cp));
>  		if (ret < 0)
>  			goto out_unpin;
>  	} else {
> @@ -654,7 +656,7 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
>  	return 0;
>  
>  out_unpin:
> -	page_array_unpin_free(pa, vdev);
> +	page_array_unpin_free(pa, vdev, idal_is_2k(cp));
>  out_free_idaws:
>  	kfree(idaws);
>  out_init:
> @@ -752,7 +754,7 @@ void cp_free(struct channel_program *cp)
>  	cp->initialized = false;
>  	list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
>  		for (i = 0; i < chain->ch_len; i++) {
> -			page_array_unpin_free(chain->ch_pa + i, vdev);
> +			page_array_unpin_free(chain->ch_pa + i, vdev, idal_is_2k(cp));
>  			ccwchain_cda_free(chain, i);
>  		}
>  		ccwchain_free(chain);