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.
>
> Fixes: 405d566f98ae ("vfio-ccw: Don't assume there are more ccws after a TIC")
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx>
[..]
> 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))
AFAIU this is a heuristic. I would prefer this being a heuristics
pointed out clearly, along with the explanation of what happens if the
heuristics fails.
Your comment is suggesting that the case we care about is TIC pointing
to the CCW that immediately precedes it. The code however considers the
whole current chain.
Regards,
Halil
> break;
>
> ccw++;
