From patchwork Thu May 12 05:16:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Quan Xu X-Patchwork-Id: 9076721 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 1E47FBF29F for ; Thu, 12 May 2016 05:19:01 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0514B201ED for ; Thu, 12 May 2016 05:19:00 +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 C3BC9201DD for ; Thu, 12 May 2016 05:18:54 +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 1b0iys-0003OJ-9l; Thu, 12 May 2016 05:16:14 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1b0iyq-0003OD-U7 for xen-devel@lists.xen.org; Thu, 12 May 2016 05:16:13 +0000 Received: from [85.158.139.211] by server-9.bemta-5.messagelabs.com id DB/87-12567-C9114375; Thu, 12 May 2016 05:16:12 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrBKsWRWlGSWpSXmKPExsVywNwkQne2oEm 4wZeNChZLPi5mcWD0OLr7N1MAYxRrZl5SfkUCa8b7D8+YC3pNKratPcjewHhfrYuRk0NIoEJi 9uU5zCC2hACvxJFlM1ghbH+J889+sXQxcgHVNDBKrN21hBHC2c0osWzfQyYIZy2jxMbPe5lAW tgEVCRmNL9jB7FFBJQlen/9BmtnFtjBKPHs4CywHcICKRJPm9ewQRSlSqx5t4MFwnaTeN69nB HEZhFQlZjZeg5sKK9AsMTym/1sENtWMUkse/QT7EBOAXuJOR0zwIYyCohJfD+1BqyBWUBc4ta T+UwQTwhILNlzHuo5UYmXj/9BPbeQWeJ6nzeELS1xbN11qLiixN/1rYwQc3QkFuz+xAZha0ss W/iaGeIgQYmTM58AHc0BdJCcxMYfXBMYpWch2TwLSfcsJN2zkHQvYGRZxahenFpUllqka6yXV JSZnlGSm5iZo2toYKqXm1pcnJiempOYVKyXnJ+7iREYwwxAsINx7z+nQ4ySHExKorxSfCbhQn xJ+SmVGYnFGfFFpTmpxYcYZTg4lCR4ZwkA5QSLUtNTK9Iyc4DJBCYtwcGjJMJ7ESTNW1yQmFu cmQ6ROsWoKCXOexMkIQCSyCjNg2uDJbBLjLJSwryMQIcI8RSkFuVmlqDKv2IU52BUEua9BTKF JzOvBG76K6DFTECLq68bgSwuSURISTUwaol4bJrYmhPz6Hvn7gOpz9KdNv3I65op+zD6rfJM4 +WXGSrltsvfms/AaHX/iLTe6RQZy1VXBc/d+OqkoMz09f4OvlXKpX/nc91eIGx/WrnqTLjvzm U3gm/LXRIWSlTeLqW98nrS4tc7uWfK/khYINI7V67Glr02aY3dYcHZkd0mLE28c6sYlViKMxI NtZiLihMBiYjusFsDAAA= X-Env-Sender: quan.xu@intel.com X-Msg-Ref: server-7.tower-206.messagelabs.com!1463030170!38977315!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 6630 invoked from network); 12 May 2016 05:16:10 -0000 Received: from mga01.intel.com (HELO mga01.intel.com) (192.55.52.88) by server-7.tower-206.messagelabs.com with SMTP; 12 May 2016 05:16:10 -0000 Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 11 May 2016 22:16:10 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,609,1455004800"; d="scan'208";a="804487000" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 11 May 2016 22:16:09 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 11 May 2016 22:16:08 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.148]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.58]) with mapi id 14.03.0248.002; Thu, 12 May 2016 13:16:06 +0800 From: "Xu, Quan" To: Jan Beulich Thread-Topic: [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping Thread-Index: AQHRp3V/jJ9krh46w0O1usGcQR7hK5+xZ8yAgAIGCMD//4Y2gIABsjOQ Date: Thu, 12 May 2016 05:16:06 +0000 Message-ID: <945CA011AD5F084CBEA3E851C0AB28894B8AD3F2@SHSMSX101.ccr.corp.intel.com> References: <1462524880-67205-1-git-send-email-quan.xu@intel.com> <1462524880-67205-11-git-send-email-quan.xu@intel.com> <5731C60902000078000E9F37@prv-mh.provo.novell.com> <945CA011AD5F084CBEA3E851C0AB28894B8AC907@SHSMSX101.ccr.corp.intel.com> <5733126D02000078000EA7BF@prv-mh.provo.novell.com> In-Reply-To: <5733126D02000078000EA7BF@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: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTYxZjEyOWEtMGU5Ni00MGYzLTg1YjItOGY5NGQxOTUxMmY2IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImgxaXI3Nk9vTVo4bnIzcTFpNXROMTlwTFdETkUwa2lTNXN3XC9cL25TNWpydz0ifQ== x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Cc: "dario.faggioli@citrix.com" , "Wu, Feng" , "Tian, Kevin" , "xen-devel@lists.xen.org" Subject: Re: [Xen-devel] [PATCH v4 10/10] vt-d: propagate error up to ME phantom function mapping and unmapping 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 May 11, 2016 5:07 PM, Jan Beulich wrote: > >>> On 11.05.16 at 10:35, wrote: > > On May 10, 2016 5:29 PM, Jan Beulich wrote: > >> >>> On 06.05.16 at 10:54, wrote: > >> > @@ -1430,7 +1430,12 @@ int domain_context_mapping_one( > >> > unmap_vtd_domain_page(context_entries); > >> > > >> > if ( !seg ) > >> > - me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC); > >> > + { > >> > + ret = me_wifi_quirk(domain, bus, devfn, > >> > + MAP_ME_PHANTOM_FUNC); > >> > + > >> > + if ( !rc ) > >> > + rc = ret; > >> > + } > >> > >> Is there any use in calling this function if an earlier error occurred? > >> If not, > > > > It is no use. > > With this I don't understand ... > > > We may need to consider this call tree: > > $ > > me_wifi_quirk()--domain_context_mapping_one()-- > domain_context_mapping( > > )--reass > > ign_device_ownership()--... > > > > Then, what about dropping this patch? Leave it as is, or remove ' > > __must_check' annotation and propagate error up to the above call tree > > only? > > ... this. If calling the function is pointless if an earlier error occurred, why don't > you just check rc to be zero alongside the !seg check? > --- Good idea. --- Taken together, there are 3 call trees to me_wifi_quirk(): 1). ...--me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--setup_hwdom_device() There is no use in calling this function if an earlier error occurred. The change can be more lightweight (the detailed change is pending). 2). me_wifi_quirk()--domain_context_unmap_one()--... As you mentioned, while in the unmap case it should probably stay as is, to fit the "best effort" theme. Then I need to remove the __must_check annotation of me_wifi_quirk(). 3). me_wifi_quirk()--domain_context_mapping_one()--domain_context_mapping()--reassign_device_ownership() This is not an earlier error, so we need propagate the error up to the call tree (the detailed change is pending). The below is based on previous v4 p1...p9: --- -- 1.9.1 Quan diff --git a/xen/drivers/passthrough/vtd/extern.h b/xen/drivers/passthrough/vtd/extern.h index cbe0286..d4d37c3 100644 --- a/xen/drivers/passthrough/vtd/extern.h +++ b/xen/drivers/passthrough/vtd/extern.h @@ -91,7 +91,7 @@ int is_igd_vt_enabled_quirk(void); void platform_quirks_init(void); void vtd_ops_preamble_quirk(struct iommu* iommu); void vtd_ops_postamble_quirk(struct iommu* iommu); -void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map); +int me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map); void pci_vtd_quirk(const struct pci_dev *); bool_t platform_supports_intremap(void); bool_t platform_supports_x2apic(void); diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 29cf870..0ac7894 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1429,8 +1429,8 @@ int domain_context_mapping_one( unmap_vtd_domain_page(context_entries); - if ( !seg ) - me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC); + if ( !seg && !rc ) + rc = me_wifi_quirk(domain, bus, devfn, MAP_ME_PHANTOM_FUNC); return rc; } diff --git a/xen/drivers/passthrough/vtd/quirks.c b/xen/drivers/passthrough/vtd/quirks.c index 473d1fc..3606b52 100644 --- a/xen/drivers/passthrough/vtd/quirks.c +++ b/xen/drivers/passthrough/vtd/quirks.c @@ -331,10 +331,12 @@ void __init platform_quirks_init(void) * assigning Intel integrated wifi device to a guest. */ -static void map_me_phantom_function(struct domain *domain, u32 dev, int map) +static int __must_check map_me_phantom_function(struct domain *domain, + u32 dev, int map) { struct acpi_drhd_unit *drhd; struct pci_dev *pdev; + int rc; /* find ME VT-d engine base on a real ME device */ pdev = pci_get_pdev(0, 0, PCI_DEVFN(dev, 0)); @@ -342,23 +344,27 @@ static void map_me_phantom_function(struct domain *domain, u32 dev, int map) /* map or unmap ME phantom function */ if ( map ) - domain_context_mapping_one(domain, drhd->iommu, 0, - PCI_DEVFN(dev, 7), NULL); + rc = domain_context_mapping_one(domain, drhd->iommu, 0, + PCI_DEVFN(dev, 7), NULL); else - domain_context_unmap_one(domain, drhd->iommu, 0, - PCI_DEVFN(dev, 7)); + rc = domain_context_unmap_one(domain, drhd->iommu, 0, + PCI_DEVFN(dev, 7)); + + return rc; } -void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) +int me_wifi_quirk(struct domain *domain, + u8 bus, u8 devfn, int map) { u32 id; + int rc = 0; id = pci_conf_read32(0, 0, 0, 0, 0); if ( IS_CTG(id) ) { /* quit if ME does not exist */ if ( pci_conf_read32(0, 0, 3, 0, 0) == 0xffffffff ) - return; + return 0; /* if device is WLAN device, map ME phantom device 0:3.7 */ id = pci_conf_read32(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0); @@ -372,7 +378,7 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) case 0x423b8086: case 0x423c8086: case 0x423d8086: - map_me_phantom_function(domain, 3, map); + rc = map_me_phantom_function(domain, 3, map); break; default: break; @@ -382,7 +388,7 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) { /* quit if ME does not exist */ if ( pci_conf_read32(0, 0, 22, 0, 0) == 0xffffffff ) - return; + return 0; /* if device is WLAN device, map ME phantom device 0:22.7 */ id = pci_conf_read32(0, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), 0); @@ -398,12 +404,14 @@ void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map) case 0x42388086: /* Puma Peak */ case 0x422b8086: case 0x422c8086: - map_me_phantom_function(domain, 22, map); + rc = map_me_phantom_function(domain, 22, map); break; default: break; } } + + return rc; } void pci_vtd_quirk(const struct pci_dev *pdev)