From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6A567C04AB4 for ; Fri, 17 May 2019 14:06:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4681520873 for ; Fri, 17 May 2019 14:06:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728896AbfEQOGJ (ORCPT ); Fri, 17 May 2019 10:06:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45382 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728402AbfEQOGJ (ORCPT ); Fri, 17 May 2019 10:06:09 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B68EB18DF7A; Fri, 17 May 2019 14:06:08 +0000 (UTC) Received: from gondolin (dhcp-192-222.str.redhat.com [10.33.192.222]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7D4585DA63; Fri, 17 May 2019 14:06:07 +0000 (UTC) Date: Fri, 17 May 2019 16:06:04 +0200 From: Cornelia Huck To: Eric Farman Cc: Farhan Ali , Halil Pasic , Pierre Morel , linux-s390@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH v3 1/3] s390/cio: Don't pin vfio pages for empty transfers Message-ID: <20190517160604.62265254.cohuck@redhat.com> In-Reply-To: <4e4b46e6-3dfd-9ef7-71e9-4859ace10d25@linux.ibm.com> References: <20190516161403.79053-1-farman@linux.ibm.com> <20190516161403.79053-2-farman@linux.ibm.com> <20190517110635.5204a9e8.cohuck@redhat.com> <4e4b46e6-3dfd-9ef7-71e9-4859ace10d25@linux.ibm.com> Organization: Red Hat GmbH MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 17 May 2019 14:06:08 +0000 (UTC) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Fri, 17 May 2019 08:57:10 -0400 Eric Farman wrote: > On 5/17/19 5:06 AM, Cornelia Huck wrote: > > On Thu, 16 May 2019 18:14:01 +0200 > > Eric Farman 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 > >> --- > >> drivers/s390/cio/vfio_ccw_cp.c | 55 ++++++++++++++++++++++++++++++---- > >> 1 file changed, 50 insertions(+), 5 deletions(-) > >