Re: [PATCH v1 12/16] vfio/ccw: calculate number of IDAWs regardless of format

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

On 11/21/22 4:40 PM, Eric Farman wrote:
> The idal_nr_words() routine works well for 4K IDAWs, but lost its
> ability to handle the old 2K formats with the removal of 31-bit
> builds in commit 5a79859ae0f3 ("s390: remove 31 bit support").
> 
> Since there's nothing preventing a guest from generating this IDAW
> format, let's re-introduce the math for them and use both when
> calculating the number of IDAWs based on the bits specified in
> the ORB.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

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

> ---
>  arch/s390/include/asm/idals.h  | 12 ++++++++++++
>  drivers/s390/cio/vfio_ccw_cp.c | 17 ++++++++++++++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/s390/include/asm/idals.h b/arch/s390/include/asm/idals.h
> index 40eae2c08d61..0a05a893aedb 100644
> --- a/arch/s390/include/asm/idals.h
> +++ b/arch/s390/include/asm/idals.h
> @@ -23,6 +23,9 @@
>  #define IDA_SIZE_LOG 12 /* 11 for 2k , 12 for 4k */
>  #define IDA_BLOCK_SIZE (1L<<IDA_SIZE_LOG)
>  
> +#define IDA_2K_SIZE_LOG 11
> +#define IDA_2K_BLOCK_SIZE (1L << IDA_2K_SIZE_LOG)
> +
>  /*
>   * Test if an address/length pair needs an idal list.
>   */
> @@ -42,6 +45,15 @@ static inline unsigned int idal_nr_words(void *vaddr, unsigned int length)
>  		(IDA_BLOCK_SIZE-1)) >> IDA_SIZE_LOG;
>  }
>  
> +/*
> + * Return the number of 2K IDA words needed for an address/length pair.
> + */
> +static inline unsigned int idal_2k_nr_words(void *vaddr, unsigned int length)
> +{
> +	return ((__pa(vaddr) & (IDA_2K_BLOCK_SIZE-1)) + length +
> +		(IDA_2K_BLOCK_SIZE-1)) >> IDA_2K_SIZE_LOG;
> +}
> +
>  /*
>   * Create the list of idal words for an address/length pair.
>   */
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 53246f4f95f7..6839e7195182 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -502,6 +502,13 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw,
>   *
>   * @ccw: The Channel Command Word being translated
>   * @cp: Channel Program being processed
> + *
> + * The ORB is examined, since it specifies what IDAWs could actually be
> + * used by any CCW in the channel program, regardless of whether or not
> + * the CCW actually does. An ORB that does not specify Format-2-IDAW
> + * Control could still contain a CCW with an IDAL, which would be
> + * Format-1 and thus only move 2K with each IDAW. Thus all CCWs within
> + * the channel program must follow the same size requirements.
>   */
>  static int ccw_count_idaws(struct ccw1 *ccw,
>  			   struct channel_program *cp)
> @@ -529,7 +536,15 @@ static int ccw_count_idaws(struct ccw1 *ccw,
>  		iova = ccw->cda;
>  	}
>  
> -	return idal_nr_words((void *)iova, bytes);
> +	/* Format-1 IDAWs operate on 2K each */
> +	if (!cp->orb.cmd.c64)
> +		return idal_2k_nr_words((void *)iova, bytes);
> +
> +	/* Format-2 IDAWs operate on either 2K or 4K */
> +	if (cp->orb.cmd.i2k)
> +		return idal_2k_nr_words((void *)iova, bytes);
> +	else

Nit: The else is unnecessary, just unconditionally return idal_nr_words if you reach the end of the function.

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

> +		return idal_nr_words((void *)iova, bytes);
>  }
>  
>  static int ccwchain_fetch_ccw(struct ccw1 *ccw,