| Message ID | 20190222183941.29596-2-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | Fix vfio-ccw handling of TIC recursion | expand |
On Fri, 22 Feb 2019 19:39:40 +0100 Eric Farman <farman@linux.ibm.com> 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@linux.ibm.com> > > --- > > 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.
On 02/22/2019 01:39 PM, Eric Farman 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@linux.ibm.com> > > --- > > 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. > + * > + * 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; Shouldn't this be cnt - 1? so the tail points to the beginning of the last ccw in the chain? I guess it doesn't matter too much since the range check will still work. > > ccw++; >
On 02/25/2019 10:53 AM, Farhan Ali wrote: > > > On 02/22/2019 01:39 PM, Eric Farman 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@linux.ibm.com> >> >> --- >> >> 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. >> + * >> + * 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; [1] (This comment is out of sequence, read below first) Something like... if (len < 1 || ccw->cda == head) return -EIO; // FIXME Handle non-binary response in caller >> + >> + 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; > > Shouldn't this be cnt - 1? so the tail points to the beginning of the > last ccw in the chain? I guess it doesn't matter too much since the > range check will still work. Hrm, yeah I think you're right because we pre-increment the count. But, this scenario is only interesting if the CCW we're processing is a TIC; otherwise it's a normal end of chain. Since POPs (p. 15-77) says a TIC can't point to another TIC, it follows that pointing to itself is equally bad (and pointless, because it becomes an infinite loop). Simply changing this to "cnt-1" won't fix our handling of that invalid channel program, since we'll end up with a second chain consisting of just a TIC onto itself. And for that chain, we'll have "cnt = 1", decremented to zero here, and then decremented to -1 in is_cpa_within_range(). Not ideal. So, if we change it to "cnt - 1" here, we need to do another thing (see [1] above). I'd suggest we revisit that when we get some better test suites run across these invalid scenarios. >> ccw++; >>
On Fri, 22 Feb 2019 19:39:40 +0100 Eric Farman <farman@linux.ibm.com> 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@linux.ibm.com> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> [..] > 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++;
On 02/25/2019 12:56 PM, Halil Pasic wrote: > On Fri, 22 Feb 2019 19:39:40 +0100 > Eric Farman <farman@linux.ibm.com> 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@linux.ibm.com> > > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> 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++; >
On Mon, 25 Feb 2019 21:43:31 -0500 Eric Farman <farman@linux.ibm.com> wrote: > On 02/25/2019 12:56 PM, Halil Pasic wrote: > > On Fri, 22 Feb 2019 19:39:40 +0100 > > Eric Farman <farman@linux.ibm.com> wrote: > >> @@ -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. FWIW, I consider the comment to be fine as-is, and I'd prefer not to over-complicate the description here. I'm going to queue this and send a pull request, unless someone points out something obviously wrong to me.
On Tue, 26 Feb 2019 18:26:25 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Mon, 25 Feb 2019 21:43:31 -0500 > Eric Farman <farman@linux.ibm.com> wrote: > > > On 02/25/2019 12:56 PM, Halil Pasic wrote: > > > On Fri, 22 Feb 2019 19:39:40 +0100 > > > Eric Farman <farman@linux.ibm.com> wrote: > > > >> @@ -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. > > FWIW, I consider the comment to be fine as-is, and I'd prefer not to > over-complicate the description here. > > I'm going to queue this and send a pull request, unless someone points > out something obviously wrong to me. > If there were something obviously wrong in my opinion I would not have r-b-ed the patch. Just pointed out that there is a small disconnect between the comment and the code. Regards, Halil
On 02/26/2019 12:26 PM, Cornelia Huck wrote: > On Mon, 25 Feb 2019 21:43:31 -0500 > Eric Farman <farman@linux.ibm.com> wrote: > >> On 02/25/2019 12:56 PM, Halil Pasic wrote: >>> On Fri, 22 Feb 2019 19:39:40 +0100 >>> Eric Farman <farman@linux.ibm.com> wrote: > >>>> @@ -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. > > FWIW, I consider the comment to be fine as-is, and I'd prefer not to > over-complicate the description here. > > I'm going to queue this and send a pull request, unless someone points > out something obviously wrong to me. > Thanks, Connie.
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. + * + * 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++;
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@linux.ibm.com> --- 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(-)