From patchwork Mon Mar 28 03:33:07 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Quan Xu X-Patchwork-Id: 8677731 Return-Path: X-Original-To: patchwork-xen-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id CA9229F36E for ; Mon, 28 Mar 2016 03:36:00 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id CCD2020253 for ; Mon, 28 Mar 2016 03:35:59 +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 C7BC92024C for ; Mon, 28 Mar 2016 03:35:58 +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 1akNva-0007Ia-0w; Mon, 28 Mar 2016 03:33:18 +0000 Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1akNvY-0007IU-An for xen-devel@lists.xen.org; Mon, 28 Mar 2016 03:33:16 +0000 Received: from [193.109.254.147] by server-3.bemta-14.messagelabs.com id EE/1C-03304-BF5A8F65; Mon, 28 Mar 2016 03:33:15 +0000 X-Env-Sender: quan.xu@intel.com X-Msg-Ref: server-16.tower-27.messagelabs.com!1459135993!33991986!1 X-Originating-IP: [192.55.52.88] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogMTkyLjU1LjUyLjg4ID0+IDM3NDcyNQ==\n X-StarScan-Received: X-StarScan-Version: 8.11; banners=-,-,- X-VirusChecked: Checked Received: (qmail 6783 invoked from network); 28 Mar 2016 03:33:14 -0000 Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by server-16.tower-27.messagelabs.com with SMTP; 28 Mar 2016 03:33:14 -0000 Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga101.fm.intel.com with ESMTP; 27 Mar 2016 20:33:13 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,404,1455004800"; d="scan'208";a="676563979" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by FMSMGA003.fm.intel.com with ESMTP; 27 Mar 2016 20:33:13 -0700 Received: from fmsmsx112.amr.corp.intel.com (10.18.116.6) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 27 Mar 2016 20:33:12 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX112.amr.corp.intel.com (10.18.116.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 27 Mar 2016 20:33:12 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.136]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.42]) with mapi id 14.03.0248.002; Mon, 28 Mar 2016 11:33:08 +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: AQHRgBo982NKXxx04EWxfFqRN8Rv2Z9dWroAgBDSiEA= Date: Mon, 28 Mar 2016 03:33:07 +0000 Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B86D176@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> In-Reply-To: <56EAF41E02000078000DE065@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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNWNjODI1ZTMtYjliZi00YTkzLWI0ZjUtNjllZTVhYzBhYTA0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IjNLWDJldVdWR3orYm44VVJPUkcxaWdpWjFTVW1Xc3BnNENLK1dHczFQTE09In0= 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 18, 2016 1:15am, wrote: > >>> On 17.03.16 at 07:54, wrote: > > @@ -53,11 +55,21 @@ static int device_power_down(void) > > > > ioapic_suspend(); > > > > - iommu_suspend(); > > + err = iommu_suspend(); > > + if ( err ) > > + goto iommu_suspend_error; > > > > lapic_suspend(); > > > > return 0; > > + > > +iommu_suspend_error: > > Labels indented by at least one space please. > Good, I wasn't aware of it. > > --- a/xen/arch/x86/mm/p2m-ept.c > > +++ b/xen/arch/x86/mm/p2m-ept.c > > @@ -830,7 +830,15 @@ out: > > { > > if ( iommu_flags ) > > for ( i = 0; i < (1 << order); i++ ) > > - iommu_map_page(d, gfn + i, mfn_x(mfn) + i, > iommu_flags); > > + { > > + rc = iommu_map_page(d, gfn + i, mfn_x(mfn) + i, > iommu_flags); > > + if ( rc ) > > + { > > + while ( i-- > 0 ) > > + iommu_unmap_page(d, gfn + i); > > + break; > > + } > > + } > > else > > for ( i = 0; i < (1 << order); i++ ) > > iommu_unmap_page(d, gfn + i); > > Earlier on in the PV mm code you also checked iommu_unmap_page()'s return > code - why not here (and also in p2m-pt.c)? > > Also I'm quite unhappy about the inconsistent state you leave things > in: You unmap from the IOMMU, return an error, but leave the EPT entry in > place. > As I mentioned for the abstract model, For iommu_{,un}map_page(), we'd better fix it as a normal error, as the error is not only from iommu flush, .e.g, '-ENOMEM'. So, we need to {,un}map from the IOMMU, return an error, and roll back the failed operation. For iommu_unmap_page, it is still under discussion in another thread. I think we could hold it on, waiting for another discussion. > > --- 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. 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? > > --- 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 the pattern repeats - you now return an error, but you don't roll back the > now failed operation. But wait - maybe that intended: > Are you meaning to crash the guest in such cases (somewhere deep in the flush > code)? If so, I think that's fine, but you absolutely would need to say so in the > commit message. > Yes, I should enhance the commit message. > > --- 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: Quan --- 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);