Re: [PATCH v1 14/16] vfio/ccw: handle a guest Format-1 IDAL

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

On 11/21/22 4:40 PM, Eric Farman wrote:
> There are two scenarios that need to be addressed here.
> 
> First, an ORB that does NOT have the Format-2 IDAL bit set could
> have both a direct-addressed CCW and an indirect-data-address CCW
> chained together. This means that the IDA CCW will contain a
> Format-1 IDAL, and can be easily converted to a 2K Format-2 IDAL.
> But it also means that the direct-addressed CCW needs to be
> converted to the same 2K Format-2 IDAL for consistency with the
> ORB settings.
> 
> Secondly, a Format-1 IDAL is comprised of 31-bit addresses.
> Thus, we need to cast this IDAL to a pointer of ints while
> populating the list of addresses that are sent to vfio.
> 
> Since the result of both of these is the use of the 2K IDAL
> variants, and the output of vfio-ccw is always a Format-2 IDAL
> (in order to use 64-bit addresses), make sure that the correct
> control bit gets set in the ORB when these scenarios occur.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  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 90685cee85db..9527f3d8da77 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -222,6 +222,8 @@ static void convert_ccw0_to_ccw1(struct ccw1 *source, unsigned long len)
>  	}
>  }
>  
> +#define idal_is_2k(_cp) (!_cp->orb.cmd.c64 || _cp->orb.cmd.i2k)
> +
>  /*
>   * Helpers to operate ccwchain.
>   */
> @@ -504,8 +506,9 @@ static unsigned long *get_guest_idal(struct ccw1 *ccw,
>  	struct vfio_device *vdev =
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	unsigned long *idaws;
> +	unsigned int *idaws_f1;

I wonder if we should be using explicit u64/u32 here since we are dealing with hardware-architected data sizes and specifically want to index by 32- or 64-bits.  Honestly, there's probably a number of other spots in vfio-ccw where that might make sense so it would also be OK to look into that as a follow-on.

Otherwise, LGTM

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

>  	int idal_len = idaw_nr * sizeof(*idaws);
> -	int idaw_size = PAGE_SIZE;
> +	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
>  	int idaw_mask = ~(idaw_size - 1);
>  	int i, ret;
>  
> @@ -527,8 +530,10 @@ static unsigned long *get_guest_idal(struct ccw1 *ccw,
>  			for (i = 1; i < idaw_nr; i++)
>  				idaws[i] = (idaws[i - 1] + idaw_size) & idaw_mask;
>  		} else {
> -			kfree(idaws);
> -			return NULL;
> +			idaws_f1 = (unsigned int *)idaws;
> +			idaws_f1[0] = ccw->cda;
> +			for (i = 1; i < idaw_nr; i++)
> +				idaws_f1[i] = (idaws_f1[i - 1] + idaw_size) & idaw_mask;
>  		}
>  	}
>  
> @@ -593,6 +598,7 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
>  	struct vfio_device *vdev =
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	unsigned long *idaws;
> +	unsigned int *idaws_f1;
>  	int ret;
>  	int idaw_nr;
>  	int i;
> @@ -623,9 +629,12 @@ static int ccwchain_fetch_ccw(struct ccw1 *ccw,
>  	 * Copy guest IDAWs into page_array, in case the memory they
>  	 * occupy is not contiguous.
>  	 */
> +	idaws_f1 = (unsigned int *)idaws;
>  	for (i = 0; i < idaw_nr; i++) {
>  		if (cp->orb.cmd.c64)
>  			pa->pa_iova[i] = idaws[i];
> +		else
> +			pa->pa_iova[i] = idaws_f1[i];
>  	}
>  
>  	if (ccw_does_data_transfer(ccw)) {
> @@ -846,7 +855,11 @@ union orb *cp_get_orb(struct channel_program *cp, struct subchannel *sch)
>  
>  	/*
>  	 * Everything built by vfio-ccw is a Format-2 IDAL.
> +	 * If the input was a Format-1 IDAL, indicate that
> +	 * 2K Format-2 IDAWs were created here.
>  	 */
> +	if (!orb->cmd.c64)
> +		orb->cmd.i2k = 1;
>  	orb->cmd.c64 = 1;
>  
>  	if (orb->cmd.lpm == 0)