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.