From patchwork Wed Mar 30 02:28:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Quan Xu X-Patchwork-Id: 8692671 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 D9143C0553 for ; Wed, 30 Mar 2016 02:31:11 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D32CE202EB for ; Wed, 30 Mar 2016 02:31:10 +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 CAD2F202AE for ; Wed, 30 Mar 2016 02:31:09 +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 1al5ry-000805-0P; Wed, 30 Mar 2016 02:28:30 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1al5rw-0007zy-M6 for xen-devel@lists.xen.org; Wed, 30 Mar 2016 02:28:28 +0000 Received: from [85.158.139.211] by server-5.bemta-5.messagelabs.com id E9/12-03597-BC93BF65; Wed, 30 Mar 2016 02:28:27 +0000 X-Env-Sender: quan.xu@intel.com X-Msg-Ref: server-13.tower-206.messagelabs.com!1459304906!31694808!1 X-Originating-IP: [134.134.136.20] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogMTM0LjEzNC4xMzYuMjAgPT4gMzU1MzU4\n X-StarScan-Received: X-StarScan-Version: 8.11; banners=-,-,- X-VirusChecked: Checked Received: (qmail 22905 invoked from network); 30 Mar 2016 02:28:26 -0000 Received: from mga02.intel.com (HELO mga02.intel.com) (134.134.136.20) by server-13.tower-206.messagelabs.com with SMTP; 30 Mar 2016 02:28:26 -0000 Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 29 Mar 2016 19:28:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,414,1455004800"; d="scan'208";a="921469017" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga001.jf.intel.com with ESMTP; 29 Mar 2016 19:28:16 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 29 Mar 2016 19:28:16 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.136]) by shsmsx102.ccr.corp.intel.com ([169.254.2.232]) with mapi id 14.03.0248.002; Wed, 30 Mar 2016 10:28:13 +0800 From: "Xu, Quan" To: Jan Beulich Thread-Topic: [PATCH 1/2] IOMMU/MMU: Adjust top level functions for VT-d Device-TLB flush error. Thread-Index: AQHRgBo982NKXxx04EWxfFqRN8Rv2Z9dWroAgBDSiECAAWNlgIAA9ofQ Date: Wed, 30 Mar 2016 02:28:13 +0000 Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B871005@SHSMSX101.ccr.corp.intel.com> References: <1458197676-60696-1-git-send-email-quan.xu@intel.com> <1458197676-60696-2-git-send-email-quan.xu@intel.com> <56EAF41E02000078000DE065@prv-mh.provo.novell.com> <945CA011AD5F084CBEA3E851C0AB28894B86D176@SHSMSX101.ccr.corp.intel.com> <56FA48E302000078000E0B40@prv-mh.provo.novell.com> In-Reply-To: <56FA48E302000078000E0B40@prv-mh.provo.novell.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzE0ZDJhMzQtNTliYS00ZTNkLWFmOTItOGE1MmFjM2VlMjVmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IkhQNHNXcTBDTm1iTnpGektDaVJ3Y0JYY2pWaENWamFGd2ZFc09XdU84azg9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Cc: "Tian, Kevin" , "Wu, Feng" , George Dunlap , Liu Jinsong , Dario Faggioli , "xen-devel@lists.xen.org" , "Nakajima, Jun" , Andrew Cooper , Keir Fraser Subject: Re: [Xen-devel] [PATCH 1/2] IOMMU/MMU: Adjust top level functions for VT-d Device-TLB flush error. 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 March 29, 2016 3:21pm, wrote: > >>> On 28.03.16 at 05:33, wrote: > > On March 18, 2016 1:15am, wrote: > >> >>> On 17.03.16 at 07:54, wrote: > >> > --- a/xen/common/grant_table.c > >> > +++ b/xen/common/grant_table.c > >> > @@ -932,8 +932,9 @@ __gnttab_map_grant_ref( > >> > { > >> > nr_gets++; > >> > (void)get_page(pg, rd); > >> > - if ( !(op->flags & GNTMAP_readonly) ) > >> > - get_page_type(pg, PGT_writable_page); > >> > + if ( !(op->flags & GNTMAP_readonly) && > >> > + !get_page_type(pg, PGT_writable_page) ) > >> > + goto could_not_pin; > >> > >> This needs explanation, as it doesn't look related to what your > >> actual goal is: If an error was possible here, I think this would be > >> a security issue. However, as also kind of documented by the > >> explicitly ignored return value from get_page(), it is my understanding there > here we only obtain an _extra_ reference. > >> > > > > For this point, I inferred from: > > map_vcpu_info() > > { > > ... > > if ( !get_page_type(page, PGT_writable_page) ) > > { > > put_page(page); > > return -EINVAL; > > } > > ... > > } > > , then for get_page_type(), I think the return value: > > 0 -- error, > > 1-- right. > > > > So if get_page_type() is failed, we should goto could_not_pin. > > Did you read my reply at all? The explanation I'm expecting here is why error > checking is all of the sudden needed _at all_. > Sorry for my stupid reply. As in this version, before the open discussion, I try to return the iommu_{,un}map_page() error in this call tree: iommu_{,un}map_page() -- __get_page_type() -- get_page_type()--- then, in this point, I try to deal with this iommu_{,un}map_page() error. > > btw, there is another issue in the call path: > > iommu_{,un}map_page() -- __get_page_type() -- get_page_type()--- > > > > > > I tried to return iommu_{,un}map_page() error code in > > __get_page_type(), is it right? > > If the operation got fully rolled back - yes. Whether fully rolling back is feasible > there though is - see the respective discussion - an open question. > For the open question, does it refer to as below: """ As said, we first need to settle on an abstract model. Do we want IOMMU mapping failures to be fatal to the domain (perhaps with the exception of the hardware one)? I think we do, and for the hardware domain we'd do things on a best effort basis (always erring on the side of unmapping). Which would probably mean crashing the domain could be centralized in iommu_{,un}map_page(). How much roll back would then still be needed in callers of these functions for the hardware domain's sake would need to be seen. """ I hope it is yes. I read all of your emails again and again, I found I did get the point until this Monday. I am summarizing it and would send out in a new thread. > >> > --- a/xen/drivers/passthrough/x86/iommu.c > >> > +++ b/xen/drivers/passthrough/x86/iommu.c > >> > @@ -104,7 +104,11 @@ int arch_iommu_populate_page_table(struct > >> domain *d) > >> > this_cpu(iommu_dont_flush_iotlb) = 0; > >> > > >> > if ( !rc ) > >> > - iommu_iotlb_flush_all(d); > >> > + { > >> > + rc = iommu_iotlb_flush_all(d); > >> > + if ( rc ) > >> > + iommu_teardown(d); > >> > + } > >> > else if ( rc != -ERESTART ) > >> > iommu_teardown(d); > >> > >> Why can't you just use the existing call to iommu_teardown(), by > >> simply > > deleting > >> the "else"? > >> > > > > Just check it, could I modify it as below: > > --- a/xen/drivers/passthrough/x86/iommu.c > > +++ b/xen/drivers/passthrough/x86/iommu.c > > @@ -105,7 +105,8 @@ int arch_iommu_populate_page_table(struct domain > > *d) > > > > if ( !rc ) > > iommu_iotlb_flush_all(d); > > - else if ( rc != -ERESTART ) > > + > > + if ( rc != -ERESTART ) > > iommu_teardown(d); > > Clearly not - not only are you losing the return value of > iommu_iotlb_flush_all() now, you would then also call > iommu_teardown() in the "success" case. My comment was related to code > structure, yet you seem to have taken it literally. > Then, what about this one: IMO, my original modification is correct and redundant with 2 'iommu_teardown()'.. If this is still the correct one, could you help me send out the correct one? Quan --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -104,8 +104,9 @@ int arch_iommu_populate_page_table(struct domain *d) this_cpu(iommu_dont_flush_iotlb) = 0; if ( !rc ) - iommu_iotlb_flush_all(d); - else if ( rc != -ERESTART ) + rc = iommu_iotlb_flush_all(d); + + if ( !rc && rc != -ERESTART ) iommu_teardown(d);