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

[Eric Farman <farman@linux.ibm.com>]

On 5/16/19 5:59 AM, Cornelia Huck wrote:
> On Wed, 15 May 2019 16:08:18 -0400
> Farhan Ali <alifm@linux.ibm.com> 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 <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.
> 
> 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 <farman@linux.ibm.com>
>>>>> ---
>>>>>    drivers/s390/cio/vfio_ccw_cp.c | 26 ++++++++------------------
>>>>>    1 file changed, 8 insertions(+), 18 deletions(-)
>>>>   
>>>    
>>
>