From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 38938A052A; Fri, 10 Jul 2020 15:34:30 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 148B61DCBF; Fri, 10 Jul 2020 15:34:30 +0200 (CEST) Received: from us-smtp-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.120]) by dpdk.org (Postfix) with ESMTP id 094F81D739 for ; Fri, 10 Jul 2020 15:34:27 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1594388067; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=DfmjWeDhKQGrsiLQriO3tLlhLszEKKhId3YRjvyKmW8=; b=JRCwa03lTLCruOHQ1c48ZI+RD9xrm/QnYN7+NdbBI5gbM+fuB11KDhjJ1Yqhgnp+f5KH1p 0FpVOxdFZ6KMGQQ6/dflawyqU7MPtDpU+fVKlA+fWkk9CFXwzgWehkZyn05pRo7UCVRyNd hB9uUIyQBNbDFpq3WPy1PufLLTZhrVE= Received: from mail-ua1-f70.google.com (mail-ua1-f70.google.com [209.85.222.70]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-208-b4SgOlhDMGy7kEVL-4OR6w-1; Fri, 10 Jul 2020 09:34:25 -0400 X-MC-Unique: b4SgOlhDMGy7kEVL-4OR6w-1 Received: by mail-ua1-f70.google.com with SMTP id o9so2439418uar.22 for ; Fri, 10 Jul 2020 06:34:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=DfmjWeDhKQGrsiLQriO3tLlhLszEKKhId3YRjvyKmW8=; b=OQPl9Hg3mfkNdXfQl2B8gzx4kcnxtcJz6RoWb19UThl0xM38Q/472UUGHt5py06iQ9 RfxDmSyWgOGZfaHcUd76rumKfESOfAd1xs6fkUrzILNp5wHnj+cy2E3s2iqHjQdW3uAs ipQvybkU5ZMf/BsA6o+4pbUpGclMOko+FBDFtcVvK23a7fe5SOlfayXeMik3kBDZ5iRy ZJNEDB/1Tt0wO0koE6bfbHlff3jd6WVBF8qcKTwVBaDz9LNr6J3Y+69HkebbXXUA44Y3 FP9E69SPTlmR9h0T53rL/voJoxnAa0ljI9jRVhxHLiWIgJmP9ChwrZeZSN5pj77ST7aU s91w== X-Gm-Message-State: AOAM533mVHU+EzW5jf27pdJWN0f/0FI2dYH1VNCsWEqmXQukDfS1aduR auAq8V/+cXl+0PSpVdSoTSbMYCC/NF83CuND7/3HBjgoyu+s0uYMwJe3urXnDYOosZ3inGBGvnh CxmYnifuS4EfWYnKDuTc= X-Received: by 2002:ab0:5a72:: with SMTP id m47mr13030622uad.86.1594388065231; Fri, 10 Jul 2020 06:34:25 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwPNWtVwv5sI/7U9ZTHSUG02KvbX5WbyVncWfRsPGo6erRxnpVKAfZFirE8PxEdvBdxP4wR8sGZqV2QQu2NSZ8= X-Received: by 2002:ab0:5a72:: with SMTP id m47mr13030588uad.86.1594388064809; Fri, 10 Jul 2020 06:34:24 -0700 (PDT) MIME-Version: 1.0 References: <20200710115324.3902559-1-thomas@monjalon.net> In-Reply-To: <20200710115324.3902559-1-thomas@monjalon.net> From: David Marchand Date: Fri, 10 Jul 2020 15:34:13 +0200 Message-ID: To: Thomas Monjalon Cc: dev , "Yigit, Ferruh" , Gaetan Rivet , "Zhang, AlvinX" , Beilei Xing , Jeff Guo , "Burakov, Anatoly" , Bruce Richardson , Dmitry Kozlyuk , navasile@linux.microsoft.com, "Dmitry Malloy (MESHCHANINOV)" , Pallavi Kadam , Tal Shnaiderman X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH] pci: keep API compatibility with mmap values X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Jul 10, 2020 at 1:53 PM Thomas Monjalon wrote: > > The function pci_map_resource() returns MAP_FAILED in case of error. > When replacing the call to mmap() by rte_mem_map(), > the error code became NULL, breaking the API. > This function is probably not used outside of DPDK, > but it is still a problem for two reasons: > - the deprecation process was not followed > - the Linux function pci_vfio_mmap_bar() is broken for i40e > > The error code is reverted to the Unix value MAP_FAILED. > Windows needs to define this special value (-1 as in Unix). > After proper deprecation process, the API could be changed again > if really needed. > > Because of the switch from mmap() to rte_mem_map(), > another part of the API was changed: "int additional_flags" > are defined as "additional flags for the mapping range" > without mentioning it was directly used in mmap(). > Currently it is directly used in rte_mem_map(), > that's why the values rte_map_flags must be mapped (sic) on the mmap ones > in case of Unix OS. > > These are side effects of a badly defined API using Unix values. > > Bugzilla ID: 503 > Fixes: 2fd3567e5425 ("pci: use OS generic memory mapping functions") > Cc: talshn@mellanox.com > > Reported-by: David Marchand > Signed-off-by: Thomas Monjalon > --- > drivers/bus/pci/bsd/pci.c | 2 +- > drivers/bus/pci/linux/pci_uio.c | 2 +- > drivers/bus/pci/linux/pci_vfio.c | 4 ++-- > drivers/bus/pci/pci_common_uio.c | 2 +- > lib/librte_eal/include/rte_eal_paging.h | 8 ++++++++ > lib/librte_eal/windows/include/sys/mman.h | 9 +++++++++ > lib/librte_pci/rte_pci.c | 1 + > lib/librte_pci/rte_pci.h | 2 +- > 8 files changed, 24 insertions(+), 6 deletions(-) > create mode 100644 lib/librte_eal/windows/include/sys/mman.h > > diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c > index 8bc473eb9a..6ec27b4b5b 100644 > --- a/drivers/bus/pci/bsd/pci.c > +++ b/drivers/bus/pci/bsd/pci.c > @@ -192,7 +192,7 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx, > mapaddr = pci_map_resource(NULL, fd, (off_t)offset, > (size_t)dev->mem_resource[res_idx].len, 0); > close(fd); > - if (mapaddr == NULL) > + if (mapaddr == MAP_FAILED) > goto error; > > maps[map_idx].phaddr = dev->mem_resource[res_idx].phys_addr; > diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c > index b622001539..097dc19225 100644 > --- a/drivers/bus/pci/linux/pci_uio.c > +++ b/drivers/bus/pci/linux/pci_uio.c > @@ -345,7 +345,7 @@ pci_uio_map_resource_by_index(struct rte_pci_device *dev, int res_idx, > mapaddr = pci_map_resource(pci_map_addr, fd, 0, > (size_t)dev->mem_resource[res_idx].len, 0); > close(fd); > - if (mapaddr == NULL) > + if (mapaddr == MAP_FAILED) > goto error; > > pci_map_addr = RTE_PTR_ADD(mapaddr, > diff --git a/drivers/bus/pci/linux/pci_vfio.c b/drivers/bus/pci/linux/pci_vfio.c > index fdeb9a8caf..07e072e13f 100644 > --- a/drivers/bus/pci/linux/pci_vfio.c > +++ b/drivers/bus/pci/linux/pci_vfio.c > @@ -566,7 +566,7 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res, > } > > /* if there's a second part, try to map it */ > - if (map_addr != NULL > + if (map_addr != MAP_FAILED > && memreg[1].offset && memreg[1].size) { > void *second_addr = RTE_PTR_ADD(bar_addr, > (uintptr_t)(memreg[1].offset - > @@ -578,7 +578,7 @@ pci_vfio_mmap_bar(int vfio_dev_fd, struct mapped_pci_resource *vfio_res, > RTE_MAP_FORCE_ADDRESS); > } > > - if (map_addr == NULL) { > + if (map_addr == NULL || map_addr == MAP_FAILED) { > munmap(bar_addr, bar->size); > bar_addr = MAP_FAILED; > RTE_LOG(ERR, EAL, "Failed to map pci BAR%d\n", > diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c > index 793dfd0a7c..f4dca9da91 100644 > --- a/drivers/bus/pci/pci_common_uio.c > +++ b/drivers/bus/pci/pci_common_uio.c > @@ -58,7 +58,7 @@ pci_uio_map_secondary(struct rte_pci_device *dev) > "Cannot mmap device resource file %s to address: %p\n", > uio_res->maps[i].path, > uio_res->maps[i].addr); > - if (mapaddr != NULL) { > + if (mapaddr != MAP_FAILED) { > /* unmap addrs correctly mapped */ > for (j = 0; j < i; j++) > pci_unmap_resource( > diff --git a/lib/librte_eal/include/rte_eal_paging.h b/lib/librte_eal/include/rte_eal_paging.h > index ed98e70e9e..680a7f2505 100644 > --- a/lib/librte_eal/include/rte_eal_paging.h > +++ b/lib/librte_eal/include/rte_eal_paging.h > @@ -3,6 +3,7 @@ > */ > > #include > +#include > > #include > > @@ -22,6 +23,7 @@ enum rte_mem_prot { > > /** Additional flags for memory mapping. */ > enum rte_map_flags { > +#ifdef RTE_EXEC_ENV_WINDOWS > /** Changes to the mapped memory are visible to other processes. */ > RTE_MAP_SHARED = 1 << 0, > /** Mapping is not backed by a regular file. */ > @@ -35,6 +37,12 @@ enum rte_map_flags { > * it is not required to do so, thus mapping with this flag may fail. > */ > RTE_MAP_FORCE_ADDRESS = 1 << 3 > +#else /* map mmap flags because they are exposed in pci_map_resource() API */ > + RTE_MAP_SHARED = MAP_SHARED, > + RTE_MAP_ANONYMOUS = MAP_ANONYMOUS, > + RTE_MAP_PRIVATE = MAP_PRIVATE, > + RTE_MAP_FORCE_ADDRESS = MAP_FIXED, > +#endif > }; > > /** > diff --git a/lib/librte_eal/windows/include/sys/mman.h b/lib/librte_eal/windows/include/sys/mman.h > new file mode 100644 > index 0000000000..0b4b10df1f > --- /dev/null > +++ b/lib/librte_eal/windows/include/sys/mman.h > @@ -0,0 +1,9 @@ > +/* SPDX-License-Identifier: BSD-3-Clause > + * Copyright 2020 Mellanox Technologies, Ltd > + */ > + > +/* > + * The syscall mmap does not exist on Windows, > + * but this error code is used in a badly defined DPDK API for PCI mapping. > + */ > +#define MAP_FAILED ((void *) -1) > diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c > index d8272b9076..1d1cbc75ac 100644 > --- a/lib/librte_pci/rte_pci.c > +++ b/lib/librte_pci/rte_pci.c > @@ -163,6 +163,7 @@ pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size, > __func__, fd, requested_addr, size, > (unsigned long long)offset, > rte_strerror(rte_errno), mapaddr); > + mapaddr = MAP_FAILED; /* API uses mmap error code */ > } else > RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr); > > diff --git a/lib/librte_pci/rte_pci.h b/lib/librte_pci/rte_pci.h > index 104b2bb858..a03235da1f 100644 > --- a/lib/librte_pci/rte_pci.h > +++ b/lib/librte_pci/rte_pci.h > @@ -160,7 +160,7 @@ int rte_pci_addr_parse(const char *str, struct rte_pci_addr *addr); > * The additional flags for the mapping range. > * @return > * - On success, the function returns a pointer to the mapped area. > - * - On error, NULL is returned. > + * - On error, MAP_FAILED is returned. > */ > void *pci_map_resource(void *requested_addr, int fd, off_t offset, > size_t size, int additional_flags); > -- > 2.27.0 > As we discussed offlist, I am not really ecstatic about this. But the function was exposed, so no breaking. Reviewed-by: David Marchand -- David Marchand