Re: [PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

[Farhan Ali <alifm@linux.ibm.com>]

On 05/16/2019 12:14 PM, Eric Farman wrote:
> The skip flag of a CCW offers the possibility of data not being
> transferred, but is only meaningful for certain commands.
> Specifically, it is only applicable for a read, read backward, sense,
> or sense ID CCW and will be ignored for any other command code
> (SA22-7832-11 page 15-64, and figure 15-30 on page 15-75).
> 
> (A sense ID is xE4, while a sense is x04 with possible modifiers in the
> upper four bits.  So we will cover the whole "family" of sense CCWs.)
> 
> For those scenarios, since there is no requirement for the target
> address to be valid, we should skip the call to vfio_pin_pages() and
> rely on the IDAL address we have allocated/built for the channel
> program.  The fact that the individual IDAWs within the IDAL are
> invalid is fine, since they aren't actually checked in these cases.
> 
> Set pa_nr to zero when skipping the pfn_array_pin() call, since it is
> defined as the number of pages pinned and is used to determine
> whether to call vfio_unpin_pages() upon cleanup.
> 
> As we do this, since the pfn_array_pin() routine returns the number of
> pages pinned, and we might not be doing that, the logic for converting
> a CCW from direct-addressed to IDAL needs to ensure there is room for
> one IDAW in the IDAL being built since a zero-length IDAL isn't great.
> 
> Signed-off-by: Eric Farman<farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_cp.c | 55 ++++++++++++++++++++++++++++++----
>   1 file changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 086faf2dacd3..0467838aed23 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -294,6 +294,10 @@ static long copy_ccw_from_iova(struct channel_program *cp,
>   /*
>    * Helpers to operate ccwchain.
>    */
> +#define ccw_is_read(_ccw) (((_ccw)->cmd_code & 0x03) == 0x02)
> +#define ccw_is_read_backward(_ccw) (((_ccw)->cmd_code & 0x0F) == 0x0C)
> +#define ccw_is_sense(_ccw) (((_ccw)->cmd_code & 0x0F) == CCW_CMD_BASIC_SENSE)
> +
>   #define ccw_is_test(_ccw) (((_ccw)->cmd_code & 0x0F) == 0)
>   
>   #define ccw_is_noop(_ccw) ((_ccw)->cmd_code == CCW_CMD_NOOP)
> @@ -301,10 +305,39 @@ static long copy_ccw_from_iova(struct channel_program *cp,
>   #define ccw_is_tic(_ccw) ((_ccw)->cmd_code == CCW_CMD_TIC)
>   
>   #define ccw_is_idal(_ccw) ((_ccw)->flags & CCW_FLAG_IDA)
> -
> +#define ccw_is_skip(_ccw) ((_ccw)->flags & CCW_FLAG_SKIP)
>   
>   #define ccw_is_chain(_ccw) ((_ccw)->flags & (CCW_FLAG_CC | CCW_FLAG_DC))
>   
> +/*
> + * ccw_does_data_transfer()
> + *
> + * Determine whether a CCW will move any data, such that the guest pages
> + * would need to be pinned before performing the I/O.
> + *
> + * Returns 1 if yes, 0 if no.
> + */
> +static inline int ccw_does_data_transfer(struct ccw1 *ccw)
> +{
> +	/* If the skip flag is off, then data will be transferred */
> +	if (!ccw_is_skip(ccw))
> +		return 1;
> +
> +	/*
> +	 * If the skip flag is on, it is only meaningful if the command
> +	 * code is a read, read backward, sense, or sense ID.  In those
> +	 * cases, no data will be transferred.
> +	 */
> +	if (ccw_is_read(ccw) || ccw_is_read_backward(ccw))
> +		return 0;
> +
> +	if (ccw_is_sense(ccw))
> +		return 0;

Just out of curiosity, is there a reason we are checking ccw_is_sense in 
a separate if statement?

> +
> +	/* The skip flag is on, but it is ignored for this command code. */
> +	return 1;
> +}