From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id E5C181B1F3 for ; Wed, 9 Jan 2019 09:55:27 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jan 2019 00:55:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.56,456,1539673200"; d="scan'208";a="134307023" Received: from irsmsx152.ger.corp.intel.com ([163.33.192.66]) by fmsmga004.fm.intel.com with ESMTP; 09 Jan 2019 00:55:26 -0800 Received: from irsmsx112.ger.corp.intel.com (10.108.20.5) by IRSMSX152.ger.corp.intel.com (163.33.192.66) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 9 Jan 2019 08:55:24 +0000 Received: from irsmsx107.ger.corp.intel.com ([169.254.10.127]) by irsmsx112.ger.corp.intel.com ([169.254.1.84]) with mapi id 14.03.0415.000; Wed, 9 Jan 2019 08:53:25 +0000 From: "Burakov, Anatoly" To: Yongseok Koh CC: "Stojaczyk, Dariusz" , Maxime Coquelin , dpdk stable Thread-Topic: [dpdk-stable] patch 'vfio: do not needlessly setup device in secondary process' has been queued to LTS release 17.11.5 Thread-Index: AQHUl/pd8APcDsNkLE2BXO+cK+8VxqWls7PAgAAiQgCAAOuS4A== Date: Wed, 9 Jan 2019 08:53:25 +0000 Message-ID: References: <20181129231202.30436-1-yskoh@mellanox.com> <20181129231202.30436-127-yskoh@mellanox.com> <96077BDF-660B-4817-81AD-05A2C22A73E8@mellanox.com> <8E58514F-0E21-4EBD-8800-38D7E9FD59AF@mellanox.com> In-Reply-To: <8E58514F-0E21-4EBD-8800-38D7E9FD59AF@mellanox.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYmFjODg3NTctODY0Ny00ZGI5LWFkMTMtZDRmNTc1ZDBhZWMwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiSXRhV2tWWWg2dWt0c3kzSGtLc0Z0K0R1YTFQSjBlSE0rMXRcL0NsSHdha0RvRHFsU2pQZDJ6M1c3MUhCK2RvalMifQ== dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-stable] patch 'vfio: do not needlessly setup device in secondary process' has been queued to LTS release 17.11.5 X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Jan 2019 08:55:28 -0000 Ah, OK, good to know! Thanks! Thanks, Anatoly > -----Original Message----- > From: Yongseok Koh [mailto:yskoh@mellanox.com] > Sent: Tuesday, January 8, 2019 6:50 PM > To: Burakov, Anatoly > Cc: Stojaczyk, Dariusz ; Maxime Coquelin > ; dpdk stable > Subject: Re: [dpdk-stable] patch 'vfio: do not needlessly setup device in > secondary process' has been queued to LTS release 17.11.5 >=20 > Hi, >=20 > The reason is because the upstream patch doesn't have Cc: > stable@dpdk.org. > Adding the tag is at author's discretion. If a patch doesn't have it, we = regard > the author doesn't want it to be included in stable releases. > We still urge developers to carefully put the tag for bug fixes. In my > experience, a fix having the tag sometimes had to be excluded. That is > another type of mistake. >=20 > From the next stable release cycle, we will not include such patches whic= h > have "fix" keyword or "Fixes:" tag but no Cc:stable tag at first. Instead= , we > will look into such fixes one by one. And we will ask for author's help i= f > needed. >=20 > This was the reason for my email and I was waiting for replies. >=20 > As you say it is needed, it has been re-applied to stable/17.11 >=20 >=20 > Thanks, > Yongseok >=20 > > On Jan 8, 2019, at 8:47 AM, Burakov, Anatoly > wrote: > > > > Hi, > > > > What's the reason for "mistakenly" merging this patch? This patch fixes= a > real bug. Why revert? > > > > Thanks, > > Anatoly > > > > > >> -----Original Message----- > >> From: Yongseok Koh [mailto:yskoh@mellanox.com] > >> Sent: Thursday, December 20, 2018 12:22 AM > >> To: Stojaczyk, Dariusz > >> Cc: Burakov, Anatoly ; Maxime Coquelin > >> ; dpdk stable > >> Subject: Re: [dpdk-stable] patch 'vfio: do not needlessly setup > >> device in secondary process' has been queued to LTS release 17.11.5 > >> > >> Hi, > >> > >> This patch is being removed from stable/17.11 as it was mistakenly > merged. > >> Patches having 'fix' keyword in the title were merged even though > >> those don't have "Cc: stable@dpdk.org" tag in the commit message. > >> > >> If you think this patch is still needed for stable/17.11, please let m= e know. > >> Then I'll take it back. > >> > >> > >> Thanks, > >> Yongseok > >> > >> > >>> On Nov 29, 2018, at 3:12 PM, Yongseok Koh > >> wrote: > >>> > >>> Hi, > >>> > >>> FYI, your patch has been queued to LTS release 17.11.5 > >>> > >>> Note it hasn't been pushed to > >> > https://emea01.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fdpd > >> k.org%2Fbrowse%2Fdpdk- > >> > stable&data=3D02%7C01%7Cyskoh%40mellanox.com%7C984a5148ee3f4d > >> > 0911c508d65650a735%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7 > >> > C636791301732433081&sdata=3DxSYO3rfdcp080xZsdFughJwuHl10CW%2B6 > >> njLe8SLNR5o%3D&reserved=3D0 yet. > >>> It will be pushed if I get no objections before 12/01/18. So please > >>> shout if anyone has objections. > >>> > >>> Also note that after the patch there's a diff of the upstream commit > >>> vs the patch applied to the branch. If the code is different (ie: > >>> not only metadata diffs), due for example to a change in context or > >>> macro > >> names, please double check it. > >>> > >>> Thanks. > >>> > >>> Yongseok > >>> > >>> --- > >>> From ff44ba8002e2db8c02eb769d547bfd7659af14e1 Mon Sep 17 > 00:00:00 > >> 2001 > >>> From: Darek Stojaczyk > >>> Date: Wed, 21 Nov 2018 19:41:32 +0100 > >>> Subject: [PATCH] vfio: do not needlessly setup device in secondary > >>> process > >>> > >>> [ upstream commit 047e3f9f2a4a4b73da86b707af8a32039ba1cad7 ] > >>> > >>> Setting up a device that wasn't setup in the primary process will > >>> possibly break the primary process. That's because the IPC message > >>> to retrieve the group fd in the primary will also *open* that group > >>> if it wasn't opened before. Even though the secondary process closes > >>> that fd soon after as a part of its error handling path, the primary > >>> process leaks it. > >>> > >>> What's worse, opening that fd on the primary will increment the > >>> process-local counter of opened groups. > >>> If it was 0 before, then the group will never be added to the vfio > >>> container, nor dpdk memory will be ever mapped. > >>> > >>> This patch moves the proper error checks earlier in the code to > >>> fully prevent setting up devices in secondary processes that weren't > >>> setup in the primary process. > >>> > >>> Fixes: 2f4adfad0a69 ("vfio: add multiprocess support") > >>> > >>> Signed-off-by: Darek Stojaczyk > >>> Acked-by: Anatoly Burakov > >>> Reviewed-by: Maxime Coquelin > >>> --- > >>> drivers/bus/pci/linux/pci_vfio.c | 12 ++++++------ > >>> 1 file changed, 6 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/bus/pci/linux/pci_vfio.c > >>> b/drivers/bus/pci/linux/pci_vfio.c > >>> index 745db260c..654897284 100644 > >>> --- a/drivers/bus/pci/linux/pci_vfio.c > >>> +++ b/drivers/bus/pci/linux/pci_vfio.c > >>> @@ -580,11 +580,6 @@ pci_vfio_map_resource_secondary(struct > >> rte_pci_device *dev) > >>> snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT, > >>> loc->domain, loc->bus, loc->devid, loc->function); > >>> > >>> - ret =3D rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr, > >>> - &vfio_dev_fd, &device_info); > >>> - if (ret) > >>> - return ret; > >>> - > >>> /* if we're in a secondary process, just find our tailq entry */ > >>> TAILQ_FOREACH(vfio_res, vfio_res_list, next) { > >>> if (rte_pci_addr_cmp(&vfio_res->pci_addr, > >>> @@ -596,9 +591,14 @@ pci_vfio_map_resource_secondary(struct > >> rte_pci_device *dev) > >>> if (vfio_res =3D=3D NULL) { > >>> RTE_LOG(ERR, EAL, " %s cannot find TAILQ entry for PCI > >> device!\n", > >>> pci_addr); > >>> - goto err_vfio_dev_fd; > >>> + return -1; > >>> } > >>> > >>> + ret =3D rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr, > >>> + &vfio_dev_fd, &device_info); > >>> + if (ret) > >>> + return ret; > >>> + > >>> /* map BARs */ > >>> maps =3D vfio_res->maps; > >>> > >>> -- > >>> 2.11.0 > >>> > >>> --- > >>> Diff of the applied patch vs upstream commit (please double-check if > >>> non- > >> empty: > >>> --- > >>> --- - 2018-11-29 15:01:50.745484864 -0800 > >>> +++ 0127-vfio-do-not-needlessly-setup-device-in-secondary-pro.patch > >> 2018-11-29 15:01:45.335961000 -0800 > >>> @@ -1,8 +1,10 @@ > >>> -From 047e3f9f2a4a4b73da86b707af8a32039ba1cad7 Mon Sep 17 > 00:00:00 > >>> 2001 > >>> +From ff44ba8002e2db8c02eb769d547bfd7659af14e1 Mon Sep 17 > 00:00:00 > >>> +2001 > >>> From: Darek Stojaczyk > >>> Date: Wed, 21 Nov 2018 19:41:32 +0100 > >>> Subject: [PATCH] vfio: do not needlessly setup device in secondary > >>> process > >>> > >>> +[ upstream commit 047e3f9f2a4a4b73da86b707af8a32039ba1cad7 ] > >>> + > >>> Setting up a device that wasn't setup in the primary process will > >>> possibly break the primary process. That's because the IPC message > >>> to retrieve the group fd in the @@ -31,10 +33,10 @@ > >>> 1 file changed, 6 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/bus/pci/linux/pci_vfio.c > >>> b/drivers/bus/pci/linux/pci_vfio.c > >>> -index ffd26f195..54a4c959e 100644 > >>> +index 745db260c..654897284 100644 > >>> --- a/drivers/bus/pci/linux/pci_vfio.c > >>> +++ b/drivers/bus/pci/linux/pci_vfio.c > >>> -@@ -794,11 +794,6 @@ pci_vfio_map_resource_secondary(struct > >>> rte_pci_device *dev) > >>> +@@ -580,11 +580,6 @@ pci_vfio_map_resource_secondary(struct > >>> +rte_pci_device *dev) > >>> snprintf(pci_addr, sizeof(pci_addr), PCI_PRI_FMT, > >>> loc->domain, loc->bus, loc->devid, loc->function); > >>> > >>> @@ -46,7 +48,7 @@ > >>> /* if we're in a secondary process, just find our tailq entry */ > >>> TAILQ_FOREACH(vfio_res, vfio_res_list, next) { > >>> if (rte_pci_addr_cmp(&vfio_res->pci_addr, > >>> -@@ -810,9 +805,14 @@ pci_vfio_map_resource_secondary(struct > >>> rte_pci_device *dev) > >>> +@@ -596,9 +591,14 @@ pci_vfio_map_resource_secondary(struct > >>> +rte_pci_device *dev) > >>> if (vfio_res =3D=3D NULL) { > >>> RTE_LOG(ERR, EAL, " %s cannot find TAILQ entry for PCI > >> device!\n", > >>> pci_addr); > >