From patchwork Wed Apr 27 08:49:17 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Quan Xu X-Patchwork-Id: 8953801 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 D6D29BF29F for ; Wed, 27 Apr 2016 08:51:47 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D7BA4200F3 for ; Wed, 27 Apr 2016 08:51:46 +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 D6BB0200F0 for ; Wed, 27 Apr 2016 08:51:45 +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 1avL9x-0005bX-4P; Wed, 27 Apr 2016 08:49:25 +0000 Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1avL9v-0005bR-Si for xen-devel@lists.xen.org; Wed, 27 Apr 2016 08:49:23 +0000 Received: from [193.109.254.147] by server-4.bemta-14.messagelabs.com id B9/38-03277-31D70275; Wed, 27 Apr 2016 08:49:23 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrFKsWRWlGSWpSXmKPExsVywNwkQleoViH cYP8vMYslHxezODB6HN39mymAMYo1My8pvyKBNeP4yf0sBT3aFQsbDzM2MPbJdTFycggJVEjM mNbJBGJLCPBKHFk2gxXC9peYePk1excjF1BNA6PE1jUH2SCcPYwSXXMPskJ072aUOHKKDyKxj lFi1suVLCAJNgEViRnN79hBbBEBZYneX79ZQIqYBXYwSXz8cQ8sISwQL/F2wzdGiKIEiXf3ng JN5QCyjSTm7PAEMVkEVCUOvE4CqeAVCJbYf+M9M8SuNYwSX89vYANJcArYS7y4OgvsIEYBMYn vp9aAvcMsIC5x68l8qNcEJJbsOc8MYYtKvHz8D+pNRYm/61sZIep1JBbs/sQGYWtLLFv4mhli saDEyZlPWEDuERKQk9j4g2sCo9QsJBtmIemehaR7FpLuBYwsqxjVi1OLylKLdE31kooy0zNKc hMzc3QNDU30clOLixPTU3MSk4r1kvNzNzEC45QBCHYwrlvsfIhRkoNJSZQ3OlohXIgvKT+lMi OxOCO+qDQntfgQowwHh5IEr1kNUE6wKDU9tSItMweYMGDSEhw8SiK816qB0rzFBYm5xZnpEKl TjIpS4rwWIH0CIImM0jy4NliSusQoKyXMywh0iBBPQWpRbmYJqvwrRnEORiVh3iiQKTyZeSVw 018BLWYCWnz5kCzI4pJEhJRUA+PMH9dWrA3OPHGE/XPunn3Fm26ul3nM+jT1RkpzgfMejiXpH OXiu9ns4sKUfxi37togWLPwhcZX56Sg40Xv5zZ0nTN90PVATsHsq18Ve0r1dq7yM4ltZu5fDp 6yPm6x6L3b0VtOHZ86sp1Zvp2PFOGTz3fQ2i91XT298stP1qP660uP7vz6M0CJpTgj0VCLuag 4EQAJWL7fTQMAAA== X-Env-Sender: quan.xu@intel.com X-Msg-Ref: server-15.tower-27.messagelabs.com!1461746961!38047444!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.34; banners=-,-,- X-VirusChecked: Checked Received: (qmail 6118 invoked from network); 27 Apr 2016 08:49:21 -0000 Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by server-15.tower-27.messagelabs.com with SMTP; 27 Apr 2016 08:49:21 -0000 Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 27 Apr 2016 01:49:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,540,1455004800"; d="scan'208";a="793326322" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga003.jf.intel.com with ESMTP; 27 Apr 2016 01:49:20 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 27 Apr 2016 01:49:20 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX154.amr.corp.intel.com (10.18.116.70) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 27 Apr 2016 01:49:19 -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, 27 Apr 2016 16:49:17 +0800 From: "Xu, Quan" To: Jan Beulich Thread-Topic: [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping Thread-Index: AQHRntfycDYsp0E9pk630hGQ31LDXp+dcoog Date: Wed, 27 Apr 2016 08:49:17 +0000 Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B895077@SHSMSX101.ccr.corp.intel.com> References: <1460988011-17758-1-git-send-email-quan.xu@intel.com> <1460988011-17758-4-git-send-email-quan.xu@intel.com> <571E048802000078000E5371@prv-mh.provo.novell.com> In-Reply-To: <571E048802000078000E5371@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: "Tian, Kevin" , Keir Fraser , George Dunlap , Andrew Cooper , "dario.faggioli@citrix.com" , "xen-devel@lists.xen.org" , "Nakajima, Jun" Subject: Re: [Xen-devel] [PATCH v2 03/11] IOMMU/MMU: enhance the call trees of IOMMU unmapping and mapping 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 25, 2016 5:51 PM, Jan Beulich wrote: > >>> On 18.04.16 at 16:00, wrote: > > --- a/xen/arch/x86/mm.c > > +++ b/xen/arch/x86/mm.c > > @@ -2467,7 +2467,7 @@ static int __get_page_type(struct page_info > *page, unsigned long type, > > int preemptible) { > > unsigned long nx, x, y = page->u.inuse.type_info; > > - int rc = 0; > > + int rc = 0, ret = 0; > > > > ASSERT(!(type & ~(PGT_type_mask | PGT_pae_xen_l2))); > > > > @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info > *page, unsigned long type, > > if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) ) > > { > > if ( (x & PGT_type_mask) == PGT_writable_page ) > > - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page))); > > + ret = iommu_unmap_page(d, mfn_to_gmfn(d, > > + page_to_mfn(page))); > > else if ( type == PGT_writable_page ) > > - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), > > - page_to_mfn(page), > > - IOMMUF_readable|IOMMUF_writable); > > + ret = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), > > + page_to_mfn(page), > > + > > + IOMMUF_readable|IOMMUF_writable); > > } > > } > > > > @@ -2599,6 +2599,9 @@ static int __get_page_type(struct page_info > *page, unsigned long type, > > if ( (x & PGT_partial) && !(nx & PGT_partial) ) > > put_page(page); > > > > + if ( unlikely(ret) ) > > + rc = ret; > > Please don't overwrite the more relevant "rc" value in situations like this in > case that is already non-zero. In this specific case you can actually get away > without introducing a second error code variable, since the only place rc gets > altered is between the two hunks above, and overwriting the rc value from > map/unmap is then exactly what we want (but I'd much appreciate if you > added a comment to this effect). > Make sense. I hope I have got your point.. then, I will modify it as below: btw, I am clumsy at comment, I'd be pleased if you could help me enhance it. > > --- a/xen/arch/x86/mm/p2m-ept.c > > +++ b/xen/arch/x86/mm/p2m-ept.c > > @@ -665,7 +665,7 @@ ept_set_entry(struct p2m_domain *p2m, unsigned > long gfn, mfn_t mfn, > > ept_entry_t *table, *ept_entry = NULL; > > unsigned long gfn_remainder = gfn; > > unsigned int i, target = order / EPT_TABLE_ORDER; > > - int ret, rc = 0; > > + int ret, err = 0, rc = 0; > > Just like Kevin, and despite your reply to him I do not see the need fir the 3rd > error indicator variable here: > Agreed. > > @@ -182,10 +184,20 @@ void __hwdom_init iommu_hwdom_init(struct > domain *d) > > ((page->u.inuse.type_info & PGT_type_mask) > > == PGT_writable_page) ) > > mapping |= IOMMUF_writable; > > - hd->platform_ops->map_page(d, gfn, mfn, mapping); > > + > > + ret = hd->platform_ops->map_page(d, gfn, mfn, mapping); > > + > > + if ( unlikely(ret) ) > > + rc = ret; > > + > > if ( !(i++ & 0xfffff) ) > > process_pending_softirqs(); > > } > > + > > + if ( rc ) > > + printk(XENLOG_G_ERR > > + "dom%d: IOMMU mapping is failed for hardware domain.", > > + d->domain_id); > > Leaving the admin / developer with no indication of at least one specific case > of a page where a failure occurred. As said in various places before: Log > messages should provide useful information for at least getting started at > investigating the issue, without first of all having to further instrument the > code. > > In any event the "is" should be dropped from the text. > What about: + if ( rc ) + printk(XENLOG_WARNING + "iommu_hwdom_init: IOMMU mapping failed for dom%d.", + d->domain_id); If this is still not what you want, could you help me enhance it and I can follow it as a pattern. Sorry to bother you with such a case. Quan --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2578,11 +2578,11 @@ static int __get_page_type(struct page_info *page, unsigned long type, if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) ) { if ( (x & PGT_type_mask) == PGT_writable_page ) - iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page))); + rc = iommu_unmap_page(d, mfn_to_gmfn(d, page_to_mfn(page))); else if ( type == PGT_writable_page ) - iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), - page_to_mfn(page), - IOMMUF_readable|IOMMUF_writable); + rc = iommu_map_page(d, mfn_to_gmfn(d, page_to_mfn(page)), + page_to_mfn(page), + IOMMUF_readable|IOMMUF_writable); } } @@ -2593,6 +2593,8 @@ static int __get_page_type(struct page_info *page, unsigned long type, page->nr_validated_ptes = 0; page->partial_pte = 0; } + + /* Overwrite the rc value from IOMMU map and unmap. */ rc = alloc_page_type(page, type, preemptible); }