Re: [dpdk-stable] patch 'vfio: do not needlessly setup device in secondary process' has been queued to LTS release 17.11.5

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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 <anatoly.burakov@intel.com>
> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; dpdk stable <stable@dpdk.org>
> Subject: Re: [dpdk-stable] patch 'vfio: do not needlessly setup device in
> secondary process' has been queued to LTS release 17.11.5
> 
> Hi,
> 
> 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.
> 
> From the next stable release cycle, we will not include such patches which
> 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 if
> needed.
> 
> This was the reason for my email and I was waiting for replies.
> 
> As you say it is needed, it has been re-applied to stable/17.11
> 
> 
> Thanks,
> Yongseok
> 
> > On Jan 8, 2019, at 8:47 AM, Burakov, Anatoly <anatoly.burakov@intel.com>
> 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 <dariusz.stojaczyk@intel.com>
> >> Cc: Burakov, Anatoly <anatoly.burakov@intel.com>; Maxime Coquelin
> >> <maxime.coquelin@redhat.com>; dpdk stable <stable@dpdk.org>
> >> 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 me know.
> >> Then I'll take it back.
> >>
> >>
> >> Thanks,
> >> Yongseok
> >>
> >>
> >>> On Nov 29, 2018, at 3:12 PM, Yongseok Koh <yskoh@mellanox.com>
> >> 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=http%3A%2F%2Fdpd
> >> k.org%2Fbrowse%2Fdpdk-
> >>
> stable&amp;data=02%7C01%7Cyskoh%40mellanox.com%7C984a5148ee3f4d
> >>
> 0911c508d65650a735%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7
> >>
> C636791301732433081&amp;sdata=xSYO3rfdcp080xZsdFughJwuHl10CW%2B6
> >> njLe8SLNR5o%3D&amp;reserved=0 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 <dariusz.stojaczyk@intel.com>
> >>> 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 <dariusz.stojaczyk@intel.com>
> >>> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> >>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> ---
> >>> 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 = 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 == NULL) {
> >>> 		RTE_LOG(ERR, EAL, "  %s cannot find TAILQ entry for PCI
> >> device!\n",
> >>> 				pci_addr);
> >>> -		goto err_vfio_dev_fd;
> >>> +		return -1;
> >>> 	}
> >>>
> >>> +	ret = rte_vfio_setup_device(rte_pci_get_sysfs_path(), pci_addr,
> >>> +					&vfio_dev_fd, &device_info);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> 	/* map BARs */
> >>> 	maps = 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 <dariusz.stojaczyk@intel.com>
> >>> 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 == NULL) {
> >>> 		RTE_LOG(ERR, EAL, "  %s cannot find TAILQ entry for PCI
> >> device!\n",
> >>> 				pci_addr);
> >