On Thu, 21 Feb 2019 12:10:32 -0500 Eric Farman <farman@xxxxxxxxxxxxx> wrote: > > > On 02/21/2019 10:38 AM, Eric Farman wrote: > > > > > > On 02/21/2019 07:55 AM, Halil Pasic wrote: > >> On Thu, 21 Feb 2019 10:30:01 +0100 > >> Cornelia Huck <cohuck@xxxxxxxxxx> wrote: > >> > >>> On Wed, 20 Feb 2019 22:02:10 -0500 > >>> Eric Farman <farman@xxxxxxxxxxxxx> wrote: > >>> > >>>> On 02/20/2019 05:35 PM, Eric Farman wrote: > >>>>> > >>>>> > >>>>> On 02/20/2019 11:44 AM, Cornelia Huck wrote: > >>>>>> On Wed, 20 Feb 2019 11:25:46 -0500 > >>>>>> Eric Farman <farman@xxxxxxxxxxxxx> wrote: > >>>>>>> On 02/20/2019 07:44 AM, Cornelia Huck wrote: > >>>>>>>> On Wed, 20 Feb 2019 06:29:38 -0500 > >>>>>>>> Eric Farman <farman@xxxxxxxxxxxxx> wrote: > >>>>>>>>> On 02/20/2019 04:48 AM, Cornelia Huck wrote: > >>>>>>>>>> On Tue, 19 Feb 2019 21:49:07 -0500 > >>>>>>>>>> Eric Farman <farman@xxxxxxxxxxxxx> wrote: > >>>>>>>>>>> Hi Connie, Farhan, > >>>>>>>>>>> > >>>>>>>>>>> On 02/04/2019 12:06 PM, Cornelia Huck wrote: > >>>>>>>>>>>> From: Farhan Ali <alifm@xxxxxxxxxxxxx> > >>>>>>>>>>>> > >>>>>>>>>>>> When trying to calculate the length of a ccw chain, we assume > >>>>>>>>>>>> there are ccws after a TIC. This can lead to overcounting and > >>>>>>>>>>>> copying garbage data from guest memory. > >>>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx> > >>>>>>>>>>>> Message-Id: > >>>>>>>>>>>> <d63748c1f1b03147bcbf401596638627a5e35ef7.1548082107.git.alifm@xxxxxxxxxxxxx> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> Reviewed-by: Halil Pasic <pasic@xxxxxxxxxxxxx> > >>>>>>>>>>>> Signed-off-by: Cornelia Huck <cohuck@xxxxxxxxxx> > >>>>>>>>>>>> --- > >>>>>>>>>>>> drivers/s390/cio/vfio_ccw_cp.c | 2 +- > >>>>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>>>>>>>> > >>>>>>>>>>>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c > >>>>>>>>>>>> b/drivers/s390/cio/vfio_ccw_cp.c > >>>>>>>>>>>> index 70a006ba4d05..ba08fe137c2e 100644 > >>>>>>>>>>>> --- a/drivers/s390/cio/vfio_ccw_cp.c > >>>>>>>>>>>> +++ b/drivers/s390/cio/vfio_ccw_cp.c > >>>>>>>>>>>> @@ -392,7 +392,7 @@ static int ccwchain_calc_length(u64 iova, > >>>>>>>>>>>> struct channel_program *cp) > >>>>>>>>>>>> return -EOPNOTSUPP; > >>>>>>>>>>>> } > >>>>>>>>>>>> - if ((!ccw_is_chain(ccw)) && (!ccw_is_tic(ccw))) > >>>>>>>>>>>> + if (!ccw_is_chain(ccw)) > >>>>>>>> > >>>>>>>> OK, this function now returns the length of the chain excluding the > >>>>>>>> last tic. > >>>>>> > >>>>>> Hm, not so sure about that anymore. I'm a bit tired, please apply > >>>>>> salt > >>>>>> where needed to what I'm seeing. > >>>>> > >>>>> I didn't get as far as I had hoped today, so I'm going to need to > >>>>> reset > >>>>> to tomorrow, have coffee, and try again. But it does seem that with > >>>>> this patch, we calculate the length of the chain up to and > >>>>> including the > >>>>> TIC, and nothing beyond it. > >>>>> > >>>>> That is, during the boot process we calculate a chain length of "3" > >>>>> for > >>>>> a SEEK + SIDE + TIC that QEMU built. > >>> > >>> Hm... so it looks like that code does what it says on the tin. But why > >>> are we missing the second round, that should give us a chain with the > >>> forth ccw? Are we missing a loop somewhere? > >> > > > > I think this is close... More below. > > > >> > >> Have you seen this email of mine: > >> https://www.spinics.net/lists/kvm/msg182535.html > >> (MID: Message-Id: <20190220143729.6dda5665@oc2783563651>)? > >> > >> Eric told me the TIC points to the SIDE. > > > > I thought I put that in one of my responses, but regardless it's what > > the bios code builds so it's not new news. > > > > I.e the channel program says > >> loop until SIDE presents a status modifier the jump over the TIC and > >> continue with the READ. > > > > I needed to get past the incorrect length error before I could give > > Halil's email any consideration. Now that that is cleared up, we can > > look into his idea. > > > >> > >> The point is the fourth ccw is only reachable if we jump over the TIC > >> because of status modifier (from device). Currently we do not seem to > >> care about this 'jump over a ccw' possibility. IMHO that is where this > >> problem comes from. > > > > Well, sort of... We probably care about the "jump over a CCW" > > possibility if the TIC introduces any recursion. If it goes "somewhere > > new", then the likelihood of the Status Modifier coming into play is low. > > > > And besides, in this scenario we don't know about a potential Status > > Modifier until the SIDE runs, which means while we're building the > > channel program we don't know whether the memory after the TIC is > > interesting or not. Reverting this patch makes the "recursion" scenario > > (which the bios introduces, and thus brings about the importance of the > > Status Modifier) work again, but then we have difficulties when the TIC > > branches somewhere else. (See the use of NOP+TIC chains in error > > recovery.) > > > > If you're suggesting adding code to handle a Status Modifier, I would > > ask why the interrupt handler should redrive things after the TIC, when > > it would be simpler to detect the TIC recursion and planning for the > > possibility there. That would seem simpler than leaving a window open > > between (for example) orientation and data transfer. > > > > The difficulty, as Connie pointed out, is that we're still building the > > current chain here, which implies we can't rely on > > tic_target_chain_exists() to determine whether we are dealing with a TIC > > loop or not. I had some ideas this morning during the commute that > > might apply here, and warrant some experimentation. Let me cobble this > > together and see what comes from it. > > Well, taking the TIC-boundary logic from tic_target_chain_exists() and > stuffing it into ccwchain_calc_length() seems to do the trick for seeing > if we're sending a TIC back into our current chain, and makes the dasd > boot work again (the following is built on top of this patch, and is > surely whitespace damaged): > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c > index ba08fe137c2e..a4abe7519b7e 100644 > --- a/drivers/s390/cio/vfio_ccw_cp.c > +++ b/drivers/s390/cio/vfio_ccw_cp.c > @@ -362,6 +362,7 @@ static int ccwchain_calc_length(u64 iova, struct > channel_program *cp) > { > struct ccw1 *ccw, *p; > int cnt; > + u64 tail; > > /* > * Copy current chain from guest to host kernel. > @@ -392,7 +393,9 @@ static int ccwchain_calc_length(u64 iova, struct > channel_program *cp) > return -EOPNOTSUPP; > } > > - if (!ccw_is_chain(ccw)) > + tail = iova + (cnt - 1) * sizeof(struct ccw1); > + > + if (!(ccw_is_chain(ccw) || (ccw_is_tic(ccw) && iova <= > ccw->cda && ccw->cda <= tail))) After De Morgan this looks like !ccw_is_chain(ccw)) && !(ccw_is_tic(ccw) && tic_points_in_this_chain()) which only differs form the state without the Farhan fix by not carrying on for tic's that point outside of this chain. Would certainly catch the problem you discovered. Of course there is no guarantee that this covers 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. If you like I can construct an example where this heuristic fails (at least I think so). If this is the direction we are going to take I think "vfio-ccw: Don't assume there are more ccws after a TIC" should be superseded by this. And we need a comment that explains the second part of the expression. Regards, Halil > break; > > ccw++; > > I need to spend some more time looking over whether we should look at > other chains by also calling tic_target_chain_exists(), and of course > see what happens to this when we run the original I/O exercisers that > led Farhan to the original patch. But it looks promising. Thoughts? > > - Eric >
