Re: [PATCH 1/1] s390: vfio-ccw: push down unsupported IDA check

[Cornelia Huck]

On Wed,  9 May 2018 19:36:47 +0200
Halil Pasic <pasic@xxxxxxxxxxxxx> wrote:

> There is at least one relevant control program (CP) that don't set the

I'd prefer not to talk about 'control program' here, as it is not a
term commonly used in Linux. Call it 'guest'?

Also, s/don't/doesn't/


> IDA flags in the ORB as we would like them, but never uses any IDA. So
> instead of saying -EOPNOTSUPP when observing an ORB such that a channel
> program specified by it could be a not supported one, let us say
> -EOPNOTSUPP only if the channel program is a not supported one.
> 
> Of course, the real solution would be doing proper translation for all
> IDA. This is possible, but given the current code not straight forward.

I agree, this seems useful for now, but we really need to support the
different ida flags to be fully architecture compliant.

> 
> Signed-off-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
> Tested-by: Jason J. Herne <jjherne@xxxxxxxxxxxxx>
> ---
> 
> QEMU counterpart comming soon.
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 2c7550797ec2..adfff492dc83 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -365,6 +365,9 @@ static void cp_unpin_free(struct channel_program *cp)
>   * This is the chain length not considering any TICs.
>   * You need to do a new round for each TIC target.
>   *
> + * The program is also validated for absence of not yet supported
> + * indirect data addressing scenarios.
> + *
>   * Returns: the length of the ccw chain or -errno.
>   */
>  static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
> @@ -391,6 +394,14 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  	do {
>  		cnt++;
>  
> +		/*
> +		 * 2k byte block IDAWs (fmt1 or fmt2) are not yet supported.
> +		 * There are however CPs that don't use IDA at all, and can
> +		 * benefit from not failing until failure is eminent.

The second sentence is confusing (What is 'CP' referring to here?
'Control program' or struct channel_program?)

What about:

"As we don't want to fail direct addressing even if the orb specified
one of the unsupported formats, we defer checking for IDAWs in
unsupported formats to here."

> +		 */
> +		if ((!cp->orb.cmd.c64 || cp->orb.cmd.i2k) && ccw_is_idal(ccw))
> +			return -EOPNOTSUPP;
> +
>  		if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw)))
>  			break;
>  
> @@ -656,10 +667,8 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>  	/*
>  	 * XXX:
>  	 * Only support prefetch enable mode now.
> -	 * Only support 64bit addressing idal.
> -	 * Only support 4k IDAW.
>  	 */
> -	if (!orb->cmd.pfch || !orb->cmd.c64 || orb->cmd.i2k)
> +	if (!orb->cmd.pfch)
>  		return -EOPNOTSUPP;
>  
>  	INIT_LIST_HEAD(&cp->ccwchain_list);
> @@ -688,6 +697,10 @@ int cp_init(struct channel_program *cp, struct device *mdev, union orb *orb)
>  	ret = ccwchain_loop_tic(chain, cp);
>  	if (ret)
>  		cp_unpin_free(cp);
> +	/* It is safe to force: if not set but idals used
> +	 * ccwchain_calc_length returns an error.

s/returns/already returned/ ?

> +	 */
> +	cp->orb.cmd.c64 = 1;
>  
>  	return ret;
>  }

The patch looks sane, I have only issues with the description/comments.