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 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.  :)

> 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.

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