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

Matthew Rosato
2022-12-19T19:49:39Z

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,