Re: [PATCH 1/2] s390/cio: Fix vfio-ccw handling of recursive TICs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 22 Feb 2019 19:39:40 +0100
Eric Farman <farman@xxxxxxxxxxxxx> wrote:

> The routine ccwchain_calc_length() is tasked with looking at a
> channel program, seeing how many CCWs are chained together by
> the presence of the Chain-Command flag, and returning a count
> to the caller.
> 
> Previously, it also considered a Transfer-in-Channel CCW as being
> an appropriate mechanism for chaining.  The problem at the time
> was that the TIC CCW will almost certainly not go to the next CCW
> in memory (because the CC flag would be sufficient), and so
> advancing to the next 8 bytes will cause us to read potentially
> invalid memory.  So that comparison was removed, and the target
> of the TIC is processed as a new chain.
> 
> This is fine when a TIC goes to a new chain (consider a NOP+TIC to
> a channel program that is being redriven), but there is another
> scenario where this falls apart.  A TIC can be used to "rewind"
> a channel program, for example to find a particular record on a
> disk with various orientation CCWs.  In this case, we DO want to
> consider the memory after the TIC since the TIC will be skipped
> once the requested criteria is met.  This is due to the Status
> Modifier presented by the device, though software doesn't need to
> operate on it beyond understanding the behavior change of how the
> channel program is executed.
> 
> So to handle this, we will re-introduce the check for a TIC CCW
> but limit it by examining the target of the TIC.  If the TIC
> doesn't go back into the current chain, then current behavior
> applies; we should stop counting CCWs and let the target of the
> TIC be handled as a new chain.  But, if the TIC DOES go back into
> the current chain, then we need to keep looking at the memory after
> the TIC for when the channel breaks out of the TIC loop.  We can't
> use tic_target_chain_exists() because the chain in question hasn't
> been built yet, so we will redefine that comparison with some small
> functions to make it more readable and to permit refactoring later.

That's a very useful explanation of what's happening, nice.

> 
> Fixes: 405d566f98ae ("vfio-ccw: Don't assume there are more ccws after a TIC")
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> 
> ---
> 
> The commit in question is queued in the s390/features branch for
> the 5.1 merge window.
> ---
>  drivers/s390/cio/vfio_ccw_cp.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index ba08fe137c2e..a423bf4c4700 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -283,6 +283,33 @@ static long copy_ccw_from_iova(struct channel_program *cp,
>  
>  #define ccw_is_chain(_ccw) ((_ccw)->flags & (CCW_FLAG_CC | CCW_FLAG_DC))
>  
> +/*
> + * is_cpa_within_range()
> + *
> + * @cpa: channel program address being questioned
> + * @head: address of the beginning of a CCW chain
> + * @len: number of CCWs within the chain
> + *
> + * Determine whether the address of a CCW (whether a new chain,
> + * or the target of a TIC) falls within a range.

(including the end points)

> + *
> + * Returns 1 if yes, 0 if no.
> + */
> +static inline int is_cpa_within_range(u32 cpa, u32 head, int len)
> +{
> +	u32 tail = head + (len - 1) * sizeof(struct ccw1);
> +
> +	return (head <= cpa && cpa <= tail);
> +}
> +
> +static inline int is_tic_within_range(struct ccw1 *ccw, u32 head, int len)
> +{
> +	if (!ccw_is_tic(ccw))
> +		return 0;
> +
> +	return is_cpa_within_range(ccw->cda, head, len);
> +}
> +
>  static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len)
>  {
>  	struct ccwchain *chain;
> @@ -392,7 +419,15 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  			return -EOPNOTSUPP;
>  		}
>  
> -		if (!ccw_is_chain(ccw))
> +		/*
> +		 * We want to keep counting if the current CCW has the
> +		 * command-chaining flag enabled, or if it is a TIC CCW
> +		 * that loops back into the current chain.  The latter
> +		 * is used for device orientation, where the CCW PRIOR to
> +		 * the TIC can either jump to the TIC or a CCW immediately
> +		 * after the TIC, depending on the results of its operation.
> +		 */
> +		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
>  			break;
>  
>  		ccw++;

Looks good to me.



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux