From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw References: <20190514234248.36203-1-farman@linux.ibm.com> <20190514234248.36203-6-farman@linux.ibm.com> <20190515142339.12065a1d.cohuck@redhat.com> <39c7904f-7f9b-473d-201d-8d6aae4c490b@linux.ibm.com> <20190516115946.11d18510.cohuck@redhat.com> From: Eric Farman Date: Thu, 16 May 2019 06:48:18 -0400 MIME-Version: 1.0 In-Reply-To: <20190516115946.11d18510.cohuck@redhat.com> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Message-Id: <5db87f05-3b20-3c49-a730-318afa59ecfe@linux.ibm.com> Sender: kvm-owner@vger.kernel.org List-Archive: List-Post: To: Cornelia Huck , Farhan Ali Cc: Halil Pasic , Pierre Morel , linux-s390@vger.kernel.org, kvm@vger.kernel.org List-ID: On 5/16/19 5:59 AM, Cornelia Huck wrote: > On Wed, 15 May 2019 16:08:18 -0400 > Farhan Ali wrote: > >> On 05/15/2019 11:04 AM, Eric Farman wrote: >>> >>> >>> On 5/15/19 8:23 AM, Cornelia Huck wrote: >>>> On Wed, 15 May 2019 01:42:46 +0200 >>>> Eric Farman wrote: >>>> >>>>> It is possible that a guest might issue a CCW with a length of zero, >>>>> and will expect a particular response.  Consider this chain: >>>>> >>>>>     Address   Format-1 CCW >>>>>     --------  ----------------- >>>>>   0 33110EC0  346022CC 33177468 >>>>>   1 33110EC8  CF200000 3318300C >>>>> >>>>> CCW[0] moves a little more than two pages, but also has the >>>>> Suppress Length Indication (SLI) bit set to handle the expectation >>>>> that considerably less data will be moved.  CCW[1] also has the SLI >>>>> bit set, and has a length of zero.  Once vfio-ccw does its magic, >>>>> the kernel issues a start subchannel on behalf of the guest with this: >>>>> >>>>>     Address   Format-1 CCW >>>>>     --------  ----------------- >>>>>   0 021EDED0  346422CC 021F0000 >>>>>   1 021EDED8  CF240000 3318300C >>>>> >>>>> Both CCWs were converted to an IDAL and have the corresponding flags >>>>> set (which is by design), but only the address of the first data >>>>> address is converted to something the host is aware of.  The second >>>>> CCW still has the address used by the guest, which happens to be (A) >>>>> (probably) an invalid address for the host, and (B) an invalid IDAW >>>>> address (doubleword boundary, etc.). >>>>> >>>>> While the I/O fails, it doesn't fail correctly.  In this example, we >>>>> would receive a program check for an invalid IDAW address, instead of >>>>> a unit check for an invalid command. >>>>> >>>>> To fix this, revert commit 4cebc5d6a6ff ("vfio: ccw: validate the >>>>> count field of a ccw before pinning") and allow the individual fetch >>>>> routines to process them like anything else.  We'll make a slight >>>>> adjustment to our allocation of the pfn_array (for direct CCWs) or >>>>> IDAL (for IDAL CCWs) memory, so that we have room for at least one >>>>> address even though no data will be transferred. >>>>> >>>>> Note that this doesn't provide us with a channel program that will >>>>> fail in the expected way.  Since our length is zero, vfio_pin_pages() >>> >>> s/is/was/ >>> >>>>> returns -EINVAL and cp_prefetch() will thus fail.  This will be fixed >>>>> in the next patch. >>>> >>>> So, this failed before, and still fails, just differently? >>> >>> Probably.  If the guest gave us a valid address, the pin might actually >>> work now whereas before it would fail because the length was zero.  If >>> the address were also invalid, >>> >>> >IOW, this >>>> has no effect on bisectability? >>> >>> I think so, but I suppose that either (A) patch 5 and 6 could be >>> squashed together, or (B) I could move the "set pa_nr to zero" (or more >>> accurately, set it to ccw->count) pieces from patch 6 into this patch, >>> so that the vfio_pin_pages() call occurs like it does today. >>> >>>> >> >> While going through patch 5, I was confused as to why we need to pin >> pages if we are only trying to translate the addresses and no data >> transfer will take place with count==0. Well, you answer that in patch 6 :) >> >> So maybe it might be better to move parts of patch 6 to 5 or squash >> them, or maybe reverse the order. > > I think this will get a bit unwieldy of squashed, so what about simply > moving code from 6 to 5? I think people are confused enough by the two > patches to make a change look like a good idea. Agreed. I swapped them locally yesterday to see how bad that work might become, and I think got them reworked to fit properly. Will be making sure they don't break anything in this order today, but shouldn't take long to be sure. > > (I can queue patches 1-4 to get them out of the way :) I wouldn't mind that. :) - Eric > >> >> Thanks >> Farhan >> >> >>>>> >>>>> Signed-off-by: Eric Farman >>>>> --- >>>>>   drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------ >>>>>   1 file changed, 8 insertions(+), 18 deletions(-) >>>> >>> >> >