On 5/17/19 10:06 AM, Cornelia Huck wrote: > On Fri, 17 May 2019 08:57:10 -0400 > Eric Farman <farman@xxxxxxxxxxxxx> wrote: > >> On 5/17/19 5:06 AM, Cornelia Huck wrote: >>> On Thu, 16 May 2019 18:14:01 +0200 >>> Eric Farman <farman@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> >>>> --- >>>> drivers/s390/cio/vfio_ccw_cp.c | 55 ++++++++++++++++++++++++++++++---- >>>> 1 file changed, 50 insertions(+), 5 deletions(-) >>> >
