From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7CF6A43A08 for ; Tue, 30 Jan 2024 08:12:25 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 766B7402B1; Tue, 30 Jan 2024 08:12:25 +0100 (CET) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id 19D684029A for ; Tue, 30 Jan 2024 08:12:23 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.88.214]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4TPGXR4s1Mz1gy8R for ; Tue, 30 Jan 2024 15:10:31 +0800 (CST) Received: from dggpeml500011.china.huawei.com (unknown [7.185.36.84]) by mail.maildlp.com (Postfix) with ESMTPS id E50141A016C for ; Tue, 30 Jan 2024 15:12:20 +0800 (CST) Received: from [10.67.121.193] (10.67.121.193) by dggpeml500011.china.huawei.com (7.185.36.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.35; Tue, 30 Jan 2024 15:12:20 +0800 Message-ID: <5e9732be-61ce-479c-80d6-e1559aee1a01@huawei.com> Date: Tue, 30 Jan 2024 15:12:20 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem Content-Language: en-US To: References: <20240124104523.2022242-1-chaoyong.he@corigine.com> <20240129092231.3531217-1-chaoyong.he@corigine.com> <20240129092231.3531217-2-chaoyong.he@corigine.com> From: huangdengdui In-Reply-To: <20240129092231.3531217-2-chaoyong.he@corigine.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.193] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpeml500011.china.huawei.com (7.185.36.84) X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org On 2024/1/29 17:22, Chaoyong He wrote: > From: Zerun Fu > > For the primary process, the logic loops all BARs and will skip > the map of BAR with an invalid physical address (0), also will > assign 'uio_res->nb_maps' with the real mapped BARs number. But > for the secondary process, instead of loops all BARs, the logic > using the 'uio_res->nb_map' as index. If the device uses continuous > BARs there will be no problem, whereas if it uses discrete BARs, > it will lead to mapping errors. > > Fix this problem by also loops all BARs and skip the map of BAR > with an invalid physical address in secondary process. > > Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")Hi, Chaoyong This patch is just a code refactoring, this bug is caused by the following patch: Fixes: e6cf7bee1c77 ("bus/pci: fix UIO resource access from secondary process") > Cc: mukawa@igel.co.jp > Cc: stable@dpdk.org > > Signed-off-by: Zerun Fu > Reviewed-by: Chaoyong He > Reviewed-by: Long Wu > Reviewed-by: Peng Zhang test by the igb_uio driver on the ARM. Tested-by: Dengdui Huang > --- > drivers/bus/pci/pci_common_uio.c | 94 ++++++++++++++++++++------------ > 1 file changed, 60 insertions(+), 34 deletions(-) > > diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c > index 76c661f054..fcd8a49daf 100644 > --- a/drivers/bus/pci/pci_common_uio.c > +++ b/drivers/bus/pci/pci_common_uio.c > @@ -23,10 +23,57 @@ static struct rte_tailq_elem rte_uio_tailq = { > }; > EAL_REGISTER_TAILQ(rte_uio_tailq) > > +static int > +pci_uio_map_secondary_resource_by_index(struct rte_pci_device *dev, > + int res_idx, struct mapped_pci_resource *uio_res, int map_idx) > +{ > + int fd, i; > + > + if (map_idx >= uio_res->nb_maps) > + return -1; > + > + /* > + * open devname, to mmap it > + */ > + fd = open(uio_res->maps[map_idx].path, O_RDWR); > + if (fd < 0) { > + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", > + uio_res->maps[map_idx].path, strerror(errno)); > + return -1; > + } > + > + void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr, > + fd, (off_t)uio_res->maps[map_idx].offset, > + (size_t)uio_res->maps[map_idx].size, 0); > + > + /* fd is not needed in secondary process, close it */ > + close(fd); > + if (mapaddr != uio_res->maps[map_idx].addr) { > + RTE_LOG(ERR, EAL, > + "Cannot mmap device resource file %s to address: %p\n", > + uio_res->maps[map_idx].path, > + uio_res->maps[map_idx].addr); > + if (mapaddr != NULL) { > + /* unmap addrs correctly mapped */ > + for (i = 0; i < map_idx; i++) > + pci_unmap_resource( > + uio_res->maps[i].addr, > + (size_t)uio_res->maps[i].size); > + /* unmap addr wrongly mapped */ > + pci_unmap_resource(mapaddr, > + (size_t)uio_res->maps[map_idx].size); > + } When mapaddr == NULL, the addrs correctly mapped should also be unmaped It may be better to make the following changes for (i = 0; i < map_idx; i++) pci_unmap_resource(uio_res->maps[i].addr, (size_t)uio_res->maps[i].size); if (mapaddr != NULL) pci_unmap_resource(mapaddr, (size_t)uio_res->maps[map_idx].size); > + return -1; > + } > + dev->mem_resource[res_idx].addr = mapaddr; > + > + return 0; > +} > + > static int > pci_uio_map_secondary(struct rte_pci_device *dev) > { > - int fd, i, j; > + int map_idx = 0, res_idx, ret; > struct mapped_pci_resource *uio_res; > struct mapped_pci_res_list *uio_res_list = > RTE_TAILQ_CAST(rte_uio_tailq.head, mapped_pci_res_list); > @@ -37,41 +84,20 @@ pci_uio_map_secondary(struct rte_pci_device *dev) > if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr)) > continue; > > - for (i = 0; i != uio_res->nb_maps; i++) { > - /* > - * open devname, to mmap it > - */ > - fd = open(uio_res->maps[i].path, O_RDWR); > - if (fd < 0) { > - RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", > - uio_res->maps[i].path, strerror(errno)); > - return -1; > + /* Map all BARs */ > + for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) { > + /* skip empty BAR */ > + if (dev->mem_resource[res_idx].phys_addr == 0) > + continue; > + > + ret = pci_uio_map_secondary_resource_by_index(dev, res_idx, > + uio_res, map_idx); > + if (ret < 0) { > + RTE_LOG(ERR, EAL, "Failed to map resources\n"); > + return ret; > } > > - void *mapaddr = pci_map_resource(uio_res->maps[i].addr, > - fd, (off_t)uio_res->maps[i].offset, > - (size_t)uio_res->maps[i].size, 0); > - > - /* fd is not needed in secondary process, close it */ > - close(fd); > - if (mapaddr != uio_res->maps[i].addr) { > - RTE_LOG(ERR, EAL, > - "Cannot mmap device resource file %s to address: %p\n", > - uio_res->maps[i].path, > - uio_res->maps[i].addr); > - if (mapaddr != NULL) { > - /* unmap addrs correctly mapped */ > - for (j = 0; j < i; j++) > - pci_unmap_resource( > - uio_res->maps[j].addr, > - (size_t)uio_res->maps[j].size); > - /* unmap addr wrongly mapped */ > - pci_unmap_resource(mapaddr, > - (size_t)uio_res->maps[i].size); > - } > - return -1; > - } > - dev->mem_resource[i].addr = mapaddr; > + map_idx++; > } > return 0; > }