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 Tue, 26 Feb 2019 18:26:25 +0100
Cornelia Huck <cohuck@xxxxxxxxxx> wrote:

> On Mon, 25 Feb 2019 21:43:31 -0500
> Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> 
> > On 02/25/2019 12:56 PM, Halil Pasic wrote:
> > > On Fri, 22 Feb 2019 19:39:40 +0100
> > > Eric Farman <farman@xxxxxxxxxxxxx> 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




[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