Re: [PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers

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

On 5/17/19 10:06 AM, Cornelia Huck wrote:
> On Fri, 17 May 2019 08:57:10 -0400
> Eric Farman <farman@linux.ibm.com> wrote:
> 
>> On 5/17/19 5:06 AM, Cornelia Huck wrote:
>>> On Thu, 16 May 2019 18:14:01 +0200
>>> Eric Farman <farman@linux.ibm.com> wrote:
>>>   
>>>> The skip flag of a CCW offers the possibility of data not being
>>>> transferred, but is only meaningful for certain commands.
>>>> Specifically, it is only applicable for a read, read backward, sense,
>>>> or sense ID CCW and will be ignored for any other command code
>>>> (SA22-7832-11 page 15-64, and figure 15-30 on page 15-75).
>>>>
>>>> (A sense ID is xE4, while a sense is x04 with possible modifiers in the
>>>> upper four bits.  So we will cover the whole "family" of sense CCWs.)
>>>>
>>>> For those scenarios, since there is no requirement for the target
>>>> address to be valid, we should skip the call to vfio_pin_pages() and
>>>> rely on the IDAL address we have allocated/built for the channel
>>>> program.  The fact that the individual IDAWs within the IDAL are
>>>> invalid is fine, since they aren't actually checked in these cases.
>>>>
>>>> Set pa_nr to zero when skipping the pfn_array_pin() call, since it is
>>>> defined as the number of pages pinned and is used to determine
>>>> whether to call vfio_unpin_pages() upon cleanup.
>>>>
>>>> As we do this, since the pfn_array_pin() routine returns the number of
>>>> pages pinned, and we might not be doing that, the logic for converting
>>>> a CCW from direct-addressed to IDAL needs to ensure there is room for
>>>> one IDAW in the IDAL being built since a zero-length IDAL isn't great.  
>>>
>>> I have now read this sentence several times and that this and that
>>> confuses me :)  
>>
>> I have read this code for several months and I'm still confused.  :)
> 
> Lol, I guess you are not alone :)
> 
>>
>>> What are we doing, and what is the thing that we might
>>> not be doing?  
>>
>> In the codepath that converts a direct-addressed CCW into an indirect
>> one, we currently rely on the returned value from pfn_array_pin() to
>> tell us how many pages were pinned, and thus how big of an IDAL to
>> allocate.  But since this patch causes us to skip the call to
>> pfn_array_pin() for certain CCWs, using that value would be zero
>> (leftover from pfn_array_alloc()) and thus would be weird to pass to the
>> kcalloc() for our IDAL.  We definitely want to allocate our own IDAL so
>> that CCW.CDA contains a valid address, regardless of whether the IDAWs
>> will be populated or not, so we calculate the number of pages ourselves
>> here.
>>
>> (Sidebar, the above is not a concern for the IDAL-to-IDAL codepath,
>> since it has already calculated the size of the IDAL from the guest CCW
>> and is going page-by-page through it.)
>>
>> pfn_array_pin() doesn't return "partial pin" counts.  If we ask for 10
>> pages to be pinned and it only does 5, we're going to get an error that
>> we have to clean up from, rather than carrying on as if "up to 10" pages
>> pinned was acceptable.  To say that another way, there's no SLI bit for
>> the vfio_pin_pages() call, so it's not necessary to rely on the count
>> being returned if we ourselves calculate it.
>>
>> So, with that...  Maybe the paragraph in question should be something
>> like this?
>>
>> ---8<---
>> The pfn_array_pin() routine returns the number of pages that were
>> pinned, but now might be skipped for some CCWs.  Thus we need to
>> calculate the expected number of pages ourselves such that we are
>> guaranteed to allocate a reasonable number of IDAWs, which will
>> provide a valid address in CCW.CDA regardless of whether the IDAWs
>> are filled in with pinned/translated addresses or not.
> 
> Much better, thanks!
> 
> I can change the description when picking up, if no reason for a respin
> comes up (series seems sane to me so far).

I appreciate that, thank you!  Looking forward to what others may say.

 - Eric

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