Re: [dpdk-dev] [PATCH 1/6] vfio: allow DMA map of memory for the default vfio fd

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hello Shahaf,

On Wed, Feb 13, 2019 at 11:10:21AM +0200, Shahaf Shuler wrote:
> Enable users the option to call rte_vfio_dma_map with request to map
> to the default vfio fd.
> 
> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>  lib/librte_eal/common/include/rte_vfio.h |  6 ++++--
>  lib/librte_eal/linuxapp/eal/eal_vfio.c   | 14 ++++++++++++--
>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_vfio.h b/lib/librte_eal/common/include/rte_vfio.h
> index cae96fab90..2a6827012f 100644
> --- a/lib/librte_eal/common/include/rte_vfio.h
> +++ b/lib/librte_eal/common/include/rte_vfio.h
> @@ -347,7 +347,8 @@ rte_vfio_container_group_unbind(int container_fd, int iommu_group_num);
>   * Perform DMA mapping for devices in a container.
>   *
>   * @param container_fd
> - *   the specified container fd
> + *   the specified container fd. In case of -1 the default container
> + *   fd will be used.

I think

#define RTE_VFIO_DEFAULT_CONTAINER_FD (-1)

might help reading the code that will later call these functions.

>   *
>   * @param vaddr
>   *   Starting virtual address of memory to be mapped.
> @@ -370,7 +371,8 @@ rte_vfio_container_dma_map(int container_fd, uint64_t vaddr,
>   * Perform DMA unmapping for devices in a container.
>   *
>   * @param container_fd
> - *   the specified container fd
> + *   the specified container fd. In case of -1 the default container
> + *   fd will be used.
>   *
>   * @param vaddr
>   *   Starting virtual address of memory to be unmapped.
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> index c821e83826..48ca9465d4 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> @@ -1897,7 +1897,12 @@ rte_vfio_container_dma_map(int container_fd, uint64_t vaddr, uint64_t iova,
>  		return -1;
>  	}
>  
> -	vfio_cfg = get_vfio_cfg_by_container_fd(container_fd);
> +	if (container_fd > 0) {

Should it not be container_fd >= 0? This seems inconsistent with the doc
above. Reading the code quickly, it's not clear that the container_fd==0
would be at vfio_cfgs[0], so this seems wrong.

> +		vfio_cfg = get_vfio_cfg_by_container_fd(container_fd);
> +	} else {
> +		/* when no fd provided use the default. */
> +		vfio_cfg = &vfio_cfgs[0];
> +	}

Can you use:

vfio_cfg = default_vfio_cfg;

instead? Then the comment is redundant.
Actually, to keep with my comment above, it might be better to simply
have

    if (container_fd == RTE_VFIO_DEFAULT_CONTAINER_FD)
            vfio_cfg = default_vfio_cfg;
    else
            vfio_cfg = get_vfio_cfg_by_group_num(container_fd);

>  	if (vfio_cfg == NULL) {
>  		RTE_LOG(ERR, EAL, "Invalid container fd\n");
>  		return -1;
> @@ -1917,7 +1922,12 @@ rte_vfio_container_dma_unmap(int container_fd, uint64_t vaddr, uint64_t iova,
>  		return -1;
>  	}
>  
> -	vfio_cfg = get_vfio_cfg_by_container_fd(container_fd);
> +	if (container_fd > 0) {
> +		vfio_cfg = get_vfio_cfg_by_container_fd(container_fd);
> +	} else {
> +		/* when no fd provided use the default. */
> +		vfio_cfg = &vfio_cfgs[0];
> +	}
>  	if (vfio_cfg == NULL) {
>  		RTE_LOG(ERR, EAL, "Invalid container fd\n");
>  		return -1;
> -- 
> 2.12.0
> 

-- 
Gaëtan Rivet
6WIND