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

Matthew Rosato
2022-12-15T21:19:32Z

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