On 17/09/2020 12:28, David Marchand wrote:
> As reported during 20.08 work for Windows, the pci_map_resource API was
> built with the assumption that its flags would be passed to mmap().
>
> This introduced a regression when adding the rte_mem_map API as reported
> in the workaround commit 9d2b24593724 ("pci: keep API compatibility with
> mmap values").
>
> This API was only used in the PCI bus code, so move it there.
>
> There is no code change happening during the move.
> The only change is in the pci_map_resource description where the
> additional flags are now documented as rte_mem_map API flags:
> - * The additional flags for the mapping range.
> + * The additional rte_mem_map() flags for the mapping range.
>
> Signed-off-by: David Marchand <david.march...@redhat.com>
> Acked-by: Andrew Rybchenko <arybche...@solarflare.com>
> ---
> doc/guides/rel_notes/deprecation.rst | 11 -----
> doc/guides/rel_notes/release_20_11.rst | 6 +++
> drivers/bus/pci/linux/pci_init.h | 2 +
> drivers/bus/pci/linux/pci_uio.c | 1 +
> drivers/bus/pci/pci_common.c | 41 ++++++++++++++++
> drivers/bus/pci/private.h | 65 +++++++++++++++++++++++++
> lib/librte_pci/rte_pci.c | 42 ----------------
> lib/librte_pci/rte_pci.h | 66 --------------------------
> lib/librte_pci/rte_pci_version.map | 2 -
> 9 files changed, 115 insertions(+), 121 deletions(-)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 83ba567632..8fca461045 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -113,17 +113,6 @@ Deprecation Notices
> us extending existing enum/define.
> One solution can be using a fixed size array instead of ``.*MAX.*`` value.
>
> -* pci: The PCI resources map API (``pci_map_resource`` and
> - ``pci_unmap_resource``) was not abstracting the Unix mmap flags (see the
> - workaround for Windows support implemented in the commit
> - 9d2b24593724 ("pci: keep API compatibility with mmap values")).
> - This API will be removed from the public API in 20.11 and moved to the PCI
> - bus driver along with the PCI resources lists and associated structures
> - (``pci_map``, ``pci_msix_table``, ``mapped_pci_resource`` and
> - ``mapped_pci_res_list``).
> - With this removal, there won't be a need for the mentioned workaround which
> - will be reverted.
> -
> * mbuf: Some fields will be converted to dynamic API in DPDK 20.11
> in order to reserve more space for the dynamic fields, as explained in
> `this presentation <https://www.youtube.com/watch?v=Ttl6MlhmzWY>`_.
> diff --git a/doc/guides/rel_notes/release_20_11.rst
> b/doc/guides/rel_notes/release_20_11.rst
> index a76e7a2941..185eeae731 100644
> --- a/doc/guides/rel_notes/release_20_11.rst
> +++ b/doc/guides/rel_notes/release_20_11.rst
> @@ -90,6 +90,12 @@ API Changes
> * pci: Removed the ``rte_kernel_driver`` enum defined in rte_dev.h and
> replaced with a private enum in the PCI subsystem.
>
> +* pci: Removed the PCI resources map API from the public API
> + (``pci_map_resource`` and ``pci_unmap_resource``) and moved it to the
> + PCI bus driver along with the PCI resources lists and associated structures
> + (``pci_map``, ``pci_msix_table``, ``mapped_pci_resource`` and
> + ``mapped_pci_res_list``).
> +
> * mbuf: Removed the unioned field ``refcnt_atomic`` from
> the structures ``rte_mbuf`` and ``rte_mbuf_ext_shared_info``.
> The field ``refcnt`` is remaining from the old unions.
> diff --git a/drivers/bus/pci/linux/pci_init.h
> b/drivers/bus/pci/linux/pci_init.h
> index c2e603a374..dcea726186 100644
> --- a/drivers/bus/pci/linux/pci_init.h
> +++ b/drivers/bus/pci/linux/pci_init.h
> @@ -7,6 +7,8 @@
>
> #include <rte_vfio.h>
>
> +#include "private.h"
> +
> /** IO resource type: */
> #define IORESOURCE_IO 0x00000100
> #define IORESOURCE_MEM 0x00000200
> diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
> index a354920d5f..9ab20a0b25 100644
> --- a/drivers/bus/pci/linux/pci_uio.c
> +++ b/drivers/bus/pci/linux/pci_uio.c
> @@ -25,6 +25,7 @@
>
> #include "eal_filesystem.h"
> #include "pci_init.h"
> +#include "private.h"
>
> void *pci_map_addr = NULL;
>
> diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
> index dddf2b2aad..3a2ae07958 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -19,6 +19,7 @@
> #include <rte_per_lcore.h>
> #include <rte_memory.h>
> #include <rte_eal.h>
> +#include <rte_eal_paging.h>
> #include <rte_string_fns.h>
> #include <rte_common.h>
> #include <rte_devargs.h>
> @@ -79,6 +80,46 @@ pci_name_set(struct rte_pci_device *dev)
> dev->device.name = dev->name;
> }
>
> +/* map a particular resource from a file */
> +void *
> +pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size,
> + int additional_flags)
> +{
> + void *mapaddr;
> +
> + /* Map the PCI memory resource of device */
> + mapaddr = rte_mem_map(requested_addr, size,
> + RTE_PROT_READ | RTE_PROT_WRITE,
> + RTE_MAP_SHARED | additional_flags, fd, offset);
> + if (mapaddr == NULL) {
> + RTE_LOG(ERR, EAL,
> + "%s(): cannot map resource(%d, %p, 0x%zx, 0x%llx): %s
> (%p)\n",
> + __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);
> +
> + return mapaddr;
> +}
> +
> +/* unmap a particular resource */
> +void
> +pci_unmap_resource(void *requested_addr, size_t size)
> +{
> + if (requested_addr == NULL)
> + return;
> +
> + /* Unmap the PCI memory resource of device */
> + if (rte_mem_unmap(requested_addr, size)) {
> + RTE_LOG(ERR, EAL, "%s(): cannot mem unmap(%p, %#zx): %s\n",
> + __func__, requested_addr, size,
> + rte_strerror(rte_errno));
> + } else
> + RTE_LOG(DEBUG, EAL, " PCI memory unmapped at %p\n",
> + requested_addr);
> +}
> /*
> * Match the PCI Driver and Device using the ID Table
> */
> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
> index 367cdd9a65..7c89744b66 100644
> --- a/drivers/bus/pci/private.h
> +++ b/drivers/bus/pci/private.h
> @@ -81,6 +81,71 @@ void rte_pci_insert_device(struct rte_pci_device
> *exist_pci_dev,
> */
> int pci_update_device(const struct rte_pci_addr *addr);
>
> +/**
> + * A structure describing a PCI mapping.
> + */
> +struct pci_map {
> + void *addr;
> + char *path;
> + uint64_t offset;
> + uint64_t size;
> + uint64_t phaddr;
> +};
> +
> +struct pci_msix_table {
> + int bar_index;
> + uint32_t offset;
> + uint32_t size;
> +};
> +
> +/**
> + * A structure describing a mapped PCI resource.
> + * For multi-process we need to reproduce all PCI mappings in secondary
> + * processes, so save them in a tailq.
> + */
> +struct mapped_pci_resource {
> + TAILQ_ENTRY(mapped_pci_resource) next;
> +
> + struct rte_pci_addr pci_addr;
> + char path[PATH_MAX];
> + int nb_maps;
> + struct pci_map maps[PCI_MAX_RESOURCE];
> + struct pci_msix_table msix_table;
> +};
> +
> +/** mapped pci device list */
> +TAILQ_HEAD(mapped_pci_res_list, mapped_pci_resource);
> +
> +/**
> + * Map a particular resource from a file.
> + *
> + * @param requested_addr
> + * The starting address for the new mapping range.
> + * @param fd
> + * The file descriptor.
> + * @param offset
> + * The offset for the mapping range.
> + * @param size
> + * The size for the mapping range.
> + * @param additional_flags
> + * The additional rte_mem_map() flags for the mapping range.
> + * @return
> + * - On success, the function returns a pointer to the mapped area.
> + * - On error, MAP_FAILED is returned.
> + */
> +void *pci_map_resource(void *requested_addr, int fd, off_t offset,
> + size_t size, int additional_flags);
> +
> +/**
> + * Unmap a particular resource.
> + *
> + * @param requested_addr
> + * The address for the unmapping range.
> + * @param size
> + * The size for the unmapping range.
> + */
> +void pci_unmap_resource(void *requested_addr, size_t size);
> +
> /**
> * Map the PCI resource of a PCI device in virtual memory
> *
> diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c
> index 1d1cbc75ac..c91be8b167 100644
> --- a/lib/librte_pci/rte_pci.c
> +++ b/lib/librte_pci/rte_pci.c
> @@ -144,45 +144,3 @@ rte_pci_addr_parse(const char *str, struct rte_pci_addr
> *addr)
> return 0;
> return -1;
> }
> -
> -
> -/* map a particular resource from a file */
> -void *
> -pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size,
> - int additional_flags)
> -{
> - void *mapaddr;
> -
> - /* Map the PCI memory resource of device */
> - mapaddr = rte_mem_map(requested_addr, size,
> - RTE_PROT_READ | RTE_PROT_WRITE,
> - RTE_MAP_SHARED | additional_flags, fd, offset);
> - if (mapaddr == NULL) {
> - RTE_LOG(ERR, EAL,
> - "%s(): cannot map resource(%d, %p, 0x%zx, 0x%llx): %s
> (%p)\n",
> - __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);
> -
> - return mapaddr;
> -}
> -
> -/* unmap a particular resource */
> -void
> -pci_unmap_resource(void *requested_addr, size_t size)
> -{
> - if (requested_addr == NULL)
> - return;
> -
> - /* Unmap the PCI memory resource of device */
> - if (rte_mem_unmap(requested_addr, size)) {
> - RTE_LOG(ERR, EAL, "%s(): cannot mem unmap(%p, %#zx): %s\n",
> - __func__, requested_addr, size,
> - rte_strerror(rte_errno));
> - } else
> - RTE_LOG(DEBUG, EAL, " PCI memory unmapped at %p\n",
> - requested_addr);
> -}
> diff --git a/lib/librte_pci/rte_pci.h b/lib/librte_pci/rte_pci.h
> index a03235da1f..567c8cd68d 100644
> --- a/lib/librte_pci/rte_pci.h
> +++ b/lib/librte_pci/rte_pci.h
> @@ -64,42 +64,6 @@ struct rte_pci_addr {
> #define PCI_ANY_ID (0xffff)
> #define RTE_CLASS_ANY_ID (0xffffff)
>
> -/**
> - * A structure describing a PCI mapping.
> - */
> -struct pci_map {
> - void *addr;
> - char *path;
> - uint64_t offset;
> - uint64_t size;
> - uint64_t phaddr;
> -};
> -
> -struct pci_msix_table {
> - int bar_index;
> - uint32_t offset;
> - uint32_t size;
> -};
> -
> -/**
> - * A structure describing a mapped PCI resource.
> - * For multi-process we need to reproduce all PCI mappings in secondary
> - * processes, so save them in a tailq.
> - */
> -struct mapped_pci_resource {
> - TAILQ_ENTRY(mapped_pci_resource) next;
> -
> - struct rte_pci_addr pci_addr;
> - char path[PATH_MAX];
> - int nb_maps;
> - struct pci_map maps[PCI_MAX_RESOURCE];
> - struct pci_msix_table msix_table;
> -};
> -
> -
> -/** mapped pci device list */
> -TAILQ_HEAD(mapped_pci_res_list, mapped_pci_resource);
> -
> /**
> * Utility function to write a pci device name, this device name can later be
> * used to retrieve the corresponding rte_pci_addr using eal_parse_pci_*
> @@ -145,36 +109,6 @@ int rte_pci_addr_cmp(const struct rte_pci_addr *addr,
> */
> int rte_pci_addr_parse(const char *str, struct rte_pci_addr *addr);
>
> -/**
> - * Map a particular resource from a file.
> - *
> - * @param requested_addr
> - * The starting address for the new mapping range.
> - * @param fd
> - * The file descriptor.
> - * @param offset
> - * The offset for the mapping range.
> - * @param size
> - * The size for the mapping range.
> - * @param additional_flags
> - * The additional flags for the mapping range.
> - * @return
> - * - On success, the function returns a pointer to the mapped area.
> - * - On error, MAP_FAILED is returned.
> - */
> -void *pci_map_resource(void *requested_addr, int fd, off_t offset,
> - size_t size, int additional_flags);
> -
> -/**
> - * Unmap a particular resource.
> - *
> - * @param requested_addr
> - * The address for the unmapping range.
> - * @param size
> - * The size for the unmapping range.
> - */
> -void pci_unmap_resource(void *requested_addr, size_t size);
> -
> #ifdef __cplusplus
> }
> #endif
> diff --git a/lib/librte_pci/rte_pci_version.map
> b/lib/librte_pci/rte_pci_version.map
> index cd77c9dc9e..1db19a5122 100644
> --- a/lib/librte_pci/rte_pci_version.map
> +++ b/lib/librte_pci/rte_pci_version.map
> @@ -1,8 +1,6 @@
> DPDK_21 {
> global:
>
> - pci_map_resource;
> - pci_unmap_resource;
> rte_pci_addr_cmp;
> rte_pci_addr_parse;
> rte_pci_device_name;
>
Acked-by: Ray Kinsella <m...@ashroe.eu>