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 D2AE243A05; Tue, 30 Jan 2024 05:00:50 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A79E34067C; Tue, 30 Jan 2024 05:00:50 +0100 (CET) Received: from szxga06-in.huawei.com (szxga06-in.huawei.com [45.249.212.32]) by mails.dpdk.org (Postfix) with ESMTP id 8E1054029A; Tue, 30 Jan 2024 05:00:48 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.163.17]) by szxga06-in.huawei.com (SkyGuard) with ESMTP id 4TPBK14Nkmz1vsl8; Tue, 30 Jan 2024 12:00:21 +0800 (CST) Received: from dggpeml500024.china.huawei.com (unknown [7.185.36.10]) by mail.maildlp.com (Postfix) with ESMTPS id 27D6B1A0178; Tue, 30 Jan 2024 12:00:46 +0800 (CST) Received: from [10.67.121.161] (10.67.121.161) by dggpeml500024.china.huawei.com (7.185.36.10) 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 12:00:45 +0800 Subject: Re: [PATCH v2 1/2] bus/pci: fix secondary process PCI uio resource map problem To: Chaoyong He , CC: , Zerun Fu , , , Long Wu , Peng Zhang References: <20240124104523.2022242-1-chaoyong.he@corigine.com> <20240129092231.3531217-1-chaoyong.he@corigine.com> <20240129092231.3531217-2-chaoyong.he@corigine.com> From: fengchengwen Message-ID: <6b4bbd0f-e216-736c-ea51-a91339de11da@huawei.com> Date: Tue, 30 Jan 2024 12:00:45 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <20240129092231.3531217-2-chaoyong.he@corigine.com> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.121.161] X-ClientProxiedBy: dggems701-chm.china.huawei.com (10.3.19.178) To dggpeml500024.china.huawei.com (7.185.36.10) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi, Nice to see this fix. ps: our team also founds this problem, and the bugfix is still being reviewed internally. As for this patch, I prefer not extract subfunction, because the father is simple and just need do some minor adjustment. map_idx = 0; for (i = 0; i != PCI_MAX_RESOURCE; i++) { /* skip empty BAR */ if (dev->mem_resource[i].phys_addr == 0) continue; ... // use map_idx instead i map_idx++; dev->mem_resource[i].addr = mapaddr; } Also suggest use pci_uio_find_resource() to refactor pci_uio_map_secondary() function. Thanks 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") > 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 > --- > 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); > + } > + 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; > } >