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

[Gaëtan Rivet]

On Wed, Feb 13, 2019 at 10:45:05AM +0100, Gaëtan Rivet wrote:
> 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 at 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);
> 

Errm, copy error, this line should be

              vfio_cfg = get_vfio_cfg_by_container_fd(container_fd);

of course.

> >  	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

-- 
Gaëtan Rivet
6WIND