On 02/25/2019 12:56 PM, Halil Pasic wrote:
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>
Thank you!
[..]
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.
I don't agree that this as a heuristic and I don't see the value of
including "what happens" when that answer comes from decades worth of
books of various classifications. Furthermore, I don't see anything in
POPS (SA22-7832-11 pp. 15-60 and 15-76/77) that implies we're excluding
something.
Perhaps I misunderstand you. Could you please explain your continued
concern about Status Modifier, because the above academic statement
combined with your earlier comment that "all the theoretically possible
cases where we could skip over a ccw that would terminate a chain (tic
or not chaining) if there was no status modifier" does not help me
understand the "heuristic" problem you have here.
A theoretically possible combination of CCWs is nice in, well, theory.
But the examples I discussed with you offline are more likely to result
in a Command Reject due to an invalid sequence (or other more painful
errors) than a useful channel program for a device to operate upon.
Hardly a heuristic.
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.
In practical usage, a backwards TIC of just one CCW is common. But I'm
not aware of anything architecturally that prevents us from jumping
farther up the chain via a TIC if we wanted, which is why I looked at
the entire range. If we did go back farther, we would still be most
interested in the CCW prior to the TIC because of the possible SM jump.
Regards,
Halil
break;
ccw++;
