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). > > > > >> > >> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx> > >> --- > >> drivers/s390/cio/vfio_ccw_cp.c | 55 ++++++++++++++++++++++++++++++---- > >> 1 file changed, 50 insertions(+), 5 deletions(-) > >
