[dpdk-dev] [PATCH v3 3/6] pci: move resource mapping to the PCI bus

[Kinsella, Ray]

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.marchand at redhat.com>
> Acked-by: Andrew Rybchenko <arybchenko at 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 <mdr at ashroe.eu>