Re: [PATCH v1 06/16] vfio/ccw: simplify CCW chain fetch routines

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

On 11/21/22 4:40 PM, Eric Farman wrote:
> The act of processing a fetched CCW has two components:
> 
>  1) Process a Transfer-in-channel (TIC) CCW
>  2) Process any other CCW
> 
> The former needs to look at whether the TIC jumps backwards into
> the current channel program or forwards into a new one segment,

unnecessary word 'one'?

> while the latter just processes the CCW data address itself.
> 
> Rather than passing the chain segment and index within it to the
> handlers for the above, and requiring each to calculate the
> elements it needs, simply pass the needed pointers directly.
> 
> For the TIC, that means the CCW being processed and the location
> of the entire channel program which holds all segments. For the
> other CCWs, the page_array pointer is also needed to perform the
> page pinning, etc.
> 
> While at it, rename ccwchain_fetch_direct to _ccw, to indicate
> what it is. The name "_direct" is historical, when it used to
> process a direct-addressed CCW, but IDAs are processed here too.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

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

> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 1eacbb8dc860..d41d94cecdf8 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -482,11 +482,9 @@ static int ccwchain_loop_tic(struct ccwchain *chain, struct channel_program *cp)
>  	return 0;
>  }
>  
> -static int ccwchain_fetch_tic(struct ccwchain *chain,
> -			      int idx,
> +static int ccwchain_fetch_tic(struct ccw1 *ccw,
>  			      struct channel_program *cp)
>  {
> -	struct ccw1 *ccw = chain->ch_ccw + idx;
>  	struct ccwchain *iter;
>  	u32 ccw_head;
>  
> @@ -502,14 +500,12 @@ static int ccwchain_fetch_tic(struct ccwchain *chain,
>  	return -EFAULT;
>  }
>  
> -static int ccwchain_fetch_direct(struct ccwchain *chain,
> -				 int idx,
> -				 struct channel_program *cp)
> +static int ccwchain_fetch_ccw(struct ccw1 *ccw,
> +			      struct page_array *pa,
> +			      struct channel_program *cp)
>  {
>  	struct vfio_device *vdev =
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
> -	struct ccw1 *ccw;
> -	struct page_array *pa;
>  	u64 iova;
>  	unsigned long *idaws;
>  	int ret;
> @@ -517,8 +513,6 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
>  	int idaw_nr, idal_len;
>  	int i;
>  
> -	ccw = chain->ch_ccw + idx;
> -
>  	if (ccw->count)
>  		bytes = ccw->count;
>  
> @@ -548,7 +542,6 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
>  	 * required for the data transfer, since we only only support
>  	 * 4K IDAWs today.
>  	 */
> -	pa = chain->ch_pa + idx;
>  	ret = page_array_alloc(pa, iova, bytes);
>  	if (ret < 0)
>  		goto out_free_idaws;
> @@ -604,16 +597,15 @@ static int ccwchain_fetch_direct(struct ccwchain *chain,
>   * and to get rid of the cda 2G limitiaion of ccw1, we'll translate
>   * direct ccws to idal ccws.
>   */
> -static int ccwchain_fetch_one(struct ccwchain *chain,
> -			      int idx,
> +static int ccwchain_fetch_one(struct ccw1 *ccw,
> +			      struct page_array *pa,
>  			      struct channel_program *cp)
> -{
> -	struct ccw1 *ccw = chain->ch_ccw + idx;
>  
> +{
>  	if (ccw_is_tic(ccw))
> -		return ccwchain_fetch_tic(chain, idx, cp);
> +		return ccwchain_fetch_tic(ccw, cp);
>  
> -	return ccwchain_fetch_direct(chain, idx, cp);
> +	return ccwchain_fetch_ccw(ccw, pa, cp);
>  }
>  
>  /**
> @@ -736,6 +728,8 @@ void cp_free(struct channel_program *cp)
>  int cp_prefetch(struct channel_program *cp)
>  {
>  	struct ccwchain *chain;
> +	struct ccw1 *ccw;
> +	struct page_array *pa;
>  	int len, idx, ret;
>  
>  	/* this is an error in the caller */
> @@ -745,7 +739,10 @@ int cp_prefetch(struct channel_program *cp)
>  	list_for_each_entry(chain, &cp->ccwchain_list, next) {
>  		len = chain->ch_len;
>  		for (idx = 0; idx < len; idx++) {
> -			ret = ccwchain_fetch_one(chain, idx, cp);
> +			ccw = chain->ch_ccw + idx;
> +			pa = chain->ch_pa + idx;
> +
> +			ret = ccwchain_fetch_one(ccw, pa, cp);
>  			if (ret)
>  				goto out_err;
>  		}