Re: [PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw

From: Farhan Ali <alifm@linux.ibm.com>
To: Eric Farman <farman@linux.ibm.com>, Cornelia Huck <cohuck@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>,
	Pierre Morel <pmorel@linux.ibm.com>,
	linux-s390@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH v2 5/7] s390/cio: Allow zero-length CCWs in vfio-ccw
Date: Wed, 15 May 2019 16:08:18 -0400	[thread overview]
Message-ID: <39c7904f-7f9b-473d-201d-8d6aae4c490b@linux.ibm.com> (raw)
In-Reply-To: <f309cad9-9265-e276-8d57-8b6387f6fed7@linux.ibm.com>

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 <farman@linux.ibm.com> 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.

Thanks
Farhan

>>>
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>>   drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
>>>   1 file changed, 8 insertions(+), 18 deletions(-)
>>
> 