From patchwork Wed Apr 27 06:21:19 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Quan Xu X-Patchwork-Id: 8952761 Return-Path: X-Original-To: patchwork-xen-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 0EB0ABF29F for ; Wed, 27 Apr 2016 06:23:52 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 24D93201FE for ; Wed, 27 Apr 2016 06:23:51 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 274B0201EF for ; Wed, 27 Apr 2016 06:23:50 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1avIqn-0007S3-P5; Wed, 27 Apr 2016 06:21:29 +0000 Received: from mail6.bemta6.messagelabs.com ([85.158.143.247]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1avIqm-0007Rx-U4 for xen-devel@lists.xen.org; Wed, 27 Apr 2016 06:21:29 +0000 Received: from [85.158.143.35] by server-3.bemta-6.messagelabs.com id C3/5F-07120-86A50275; Wed, 27 Apr 2016 06:21:28 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrDKsWRWlGSWpSXmKPExsXS1tbhqJsepRB ucOWCusWSj4tZHBg9ju7+zRTAGMWamZeUX5HAmrG24QtzwUzJihsfeBoY5wh3MXJyCAlUSvxb 08MOYksI8EocWTaDFcL2l2hZ/Ie5i5ELqKaBUWLK511sEA17GSXaPnNDJHYzSuz9/50JwlnHK HHyyW2wdjYBFYkZze/AxooIKEv0/vrNAmIzC5xhlHj7PRWkQVigj1Fi1cHvLCCOiEA/o8SWmR OYIDqcJKbte8UIYrMIqEr0fWgFs3kFgiWuLz7GCrFuJZPE1vnNYEdxCthLTFg1DWwFo4CYxPd Ta5gg1olL3HoynwniIwGJJXvOM0PYohIvH/+D+lRR4u96iAXMAjoSC3Z/YoOwtSWWLXzNDLFY UOLkzCdA8zmAFstJbPzBNYFRahaSDbOQdM9C0j0LSfcCRpZVjOrFqUVlqUW6RnpJRZnpGSW5i Zk5uoYGZnq5qcXFiempOYlJxXrJ+bmbGIGxygAEOxiX/XU6xCjJwaQkyjvdXyFciC8pP6UyI7 E4I76oNCe1+BCjDAeHkgRvTiRQTrAoNT21Ii0zB5g0YNISHDxKIrw3IoDSvMUFibnFmekQqVO MilLivJNB+gRAEhmleXBtsER1iVFWSpiXEegQIZ6C1KLczBJU+VeM4hyMSsK8k0Cm8GTmlcBN fwW0mAlo8eVDsiCLSxIRUlINjNMXHRX6Fxc2ecqxSc6zDmUHiErxMeq5T6lyXGz9612jje2kc /Y+p8OPhrNu8l+//d+HWe4bn3hM17731MJGW/1i4OSKzV6X7t/titPtWTL5o/NZxxe7Asum8M Xw37k92eWnCafCUY7gFRE9txfXeb3rFL9ye9OKD/6H7JIPzPPgOux444Bu4RIlluKMREMt5qL iRACTspvwTwMAAA== X-Env-Sender: quan.xu@intel.com X-Msg-Ref: server-10.tower-21.messagelabs.com!1461738086!11362116!1 X-Originating-IP: [134.134.136.65] X-SpamReason: No, hits=0.0 required=7.0 tests= X-StarScan-Received: X-StarScan-Version: 8.34; banners=-,-,- X-VirusChecked: Checked Received: (qmail 14892 invoked from network); 27 Apr 2016 06:21:27 -0000 Received: from mga03.intel.com (HELO mga03.intel.com) (134.134.136.65) by server-10.tower-21.messagelabs.com with SMTP; 27 Apr 2016 06:21:27 -0000 Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP; 26 Apr 2016 23:21:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,540,1455004800"; d="scan'208";a="953801796" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga001.fm.intel.com with ESMTP; 26 Apr 2016 23:21:21 -0700 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 26 Apr 2016 23:21:21 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 26 Apr 2016 23:21:21 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.136]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.184]) with mapi id 14.03.0248.002; Wed, 27 Apr 2016 14:21:19 +0800 From: "Xu, Quan" To: Jan Beulich Thread-Topic: [Xen-devel] [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones). Thread-Index: AQHRntv+9RYR2TdmD0utMHB+fBZ/LZ+cKz2A//+E2wCAAaPeIA== Date: Wed, 27 Apr 2016 06:21:19 +0000 Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B894F62@SHSMSX101.ccr.corp.intel.com> References: <1460988011-17758-1-git-send-email-quan.xu@intel.com> <1460988011-17758-8-git-send-email-quan.xu@intel.com> <571E0B4802000078000E53FA@prv-mh.provo.novell.com> <945CA011AD5F084CBEA3E851C0AB28894B89485D@SHSMSX101.ccr.corp.intel.com> <571F7FCA02000078000E5D83@prv-mh.provo.novell.com> In-Reply-To: <571F7FCA02000078000E5D83@prv-mh.provo.novell.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Cc: Julien Grall , Stefano Stabellini , "dario.faggioli@citrix.com" , "xen-devel@lists.xen.org" Subject: Re: [Xen-devel] [PATCH v2 07/11] IOMMU/MMU: propagate IOMMU Device-TLB flush error up to iommu_iotlb_flush{, _all} (top level ones). X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On April 26, 2016 8:49 PM, Jan Beulich wrote: > >>> On 26.04.16 at 14:23, wrote: > > On April 25, 2016 6:19 PM, Jan Beulich wrote: > >> >>> On 18.04.16 at 16:00, wrote: > >> > --- a/xen/common/memory.c > >> > +++ b/xen/common/memory.c > >> > @@ -678,8 +678,9 @@ static int xenmem_add_to_physmap(struct > domain > >> *d, > >> > if ( need_iommu(d) ) > >> > { > >> > this_cpu(iommu_dont_flush_iotlb) = 0; > >> > - iommu_iotlb_flush(d, xatp->idx - done, done); > >> > - iommu_iotlb_flush(d, xatp->gpfn - done, done); > >> > + rc = iommu_iotlb_flush(d, xatp->idx - done, done); > >> > + if ( !rc ) > >> > + rc = iommu_iotlb_flush(d, xatp->gpfn - done, done); > >> > >> And again - best effort flushing in all cases. I.e. you shouldn't > >> bypass the second one if the first one failed, but instead properly > >> accumulate the return value, and also again not clobber any negative value > rc may already hold. > > > > > > Thanks for correcting me. I did like accumulating the return value. > > :( I will follow your suggestion in next v3. > > > >> What's worse (and makes me wonder whether this got tested) - you also > >> clobber the positive return value this function may produce, even in > >> the case the flushes succeed (and hence the function as a whole was > successful). > >> > > I have tested it with previous patches (i.e. 1st patch), launching a > > VM with PT ATS device to invoke this call tree. > > btw, I fix the positive issue in 1st patch. > > I know this is not strict, as I assume this patch set will be > > committed at the same time. > > Hmm, the "positive" here has nothing to do with the "positive" in patch 1. > Please just have a look at xenmem_add_to_physmap() as a whole. > Thanks for reminding me. The 'positive' is ' rc = start + done'.. Think twice, I found I need two other new return values for this function (correct me if I am wrong!). If the first iommu_iotlb_flush() is failed, I shouldn't accumulate the return value of the second iommu_iotlb_flush() -- but instead properly accumulate the return value. The following is my modification: Quan --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -640,6 +640,10 @@ static int xenmem_add_to_physmap(struct domain *d, unsigned int done = 0; long rc = 0; +#ifdef CONFIG_HAS_PASSTHROUGH + int ret, rv; +#endif + if ( xatp->space != XENMAPSPACE_gmfn_range ) return xenmem_add_to_physmap_one(d, xatp->space, DOMID_INVALID, xatp->idx, xatp->gpfn); @@ -678,8 +682,15 @@ static int xenmem_add_to_physmap(struct domain *d, if ( need_iommu(d) ) { this_cpu(iommu_dont_flush_iotlb) = 0; - iommu_iotlb_flush(d, xatp->idx - done, done); - iommu_iotlb_flush(d, xatp->gpfn - done, done); + ret = iommu_iotlb_flush(d, xatp->idx - done, done); + + if ( rc >= 0 && ret < 0 ) + rc = ret; + + rv = iommu_iotlb_flush(d, xatp->gpfn - done, done); + + if ( rc >=0 && ret == 0 && rv < 0 ) + rc = rv; } #endif