Re: [PATCH v1 08/16] vfio/ccw: pass page count to page_array struct

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

On 11/21/22 4:40 PM, Eric Farman wrote:
> The allocation of our page_array struct calculates the number
> of 4K pages that would be needed to hold a certain number of
> bytes. But, since the number of pages that will be pinned is
> also calculated by the length of the IDAL, this logic is
> unnecessary. Let's pass that information in directly, and
> avoid the math within the allocator.
> 
> Also, let's make this two allocations instead of one,
> to make it apparent what's happening within here.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 4b6b5f9dc92d..66e890441163 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -43,7 +43,7 @@ struct ccwchain {
>   * page_array_alloc() - alloc memory for page array
>   * @pa: page_array on which to perform the operation
>   * @iova: target guest physical address
> - * @len: number of bytes that should be pinned from @iova
> + * @len: number of pages that should be pinned from @iova
>   *
>   * Attempt to allocate memory for page array.
>   *
> @@ -63,18 +63,20 @@ static int page_array_alloc(struct page_array *pa, u64 iova, unsigned int len)
>  	if (pa->pa_nr || pa->pa_iova)
>  		return -EINVAL;
>  
> -	pa->pa_nr = ((iova & ~PAGE_MASK) + len + (PAGE_SIZE - 1)) >> PAGE_SHIFT;
> -	if (!pa->pa_nr)
> +	if (!len)

Seems like a weird way to check this.  if (len == 0) ?

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

>  		return -EINVAL;
>  
> -	pa->pa_iova = kcalloc(pa->pa_nr,
> -			      sizeof(*pa->pa_iova) + sizeof(*pa->pa_page),
> -			      GFP_KERNEL);
> -	if (unlikely(!pa->pa_iova)) {
> -		pa->pa_nr = 0;
> +	pa->pa_nr = len;
> +
> +	pa->pa_iova = kcalloc(len, sizeof(*pa->pa_iova), GFP_KERNEL);
> +	if (!pa->pa_iova)
> +		return -ENOMEM;
> +
> +	pa->pa_page = kcalloc(len, sizeof(*pa->pa_page), GFP_KERNEL);
> +	if (!pa->pa_page) {
> +		kfree(pa->pa_iova);
>  		return -ENOMEM;
>  	}
> -	pa->pa_page = (struct page **)&pa->pa_iova[pa->pa_nr];
>  
>  	pa->pa_iova[0] = iova;
>  	pa->pa_page[0] = NULL;
> @@ -167,6 +169,7 @@ static int page_array_pin(struct page_array *pa, struct vfio_device *vdev)
>  static void page_array_unpin_free(struct page_array *pa, struct vfio_device *vdev)
>  {
>  	page_array_unpin(pa, vdev, pa->pa_nr);
> +	kfree(pa->pa_page);
>  	kfree(pa->pa_iova);
>  }
>  
> @@ -545,7 +548,7 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
>  	 * required for the data transfer, since we only only support
>  	 * 4K IDAWs today.
>  	 */
> -	ret = page_array_alloc(pa, iova, bytes);
> +	ret = page_array_alloc(pa, iova, idaw_nr);
>  	if (ret < 0)
>  		goto out_free_idaws;
>