diff mbox series

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

Message ID 0f98e0a06c535d58ac8d2c39645b07eb966ec5ec.1550048187.git.shahafs@mellanox.com (mailing list archive)
State Superseded, archived
Headers
Series introduce DMA memory mapping for external memory |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/intel-Performance-Testing success Performance Testing PASS
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Shahaf Shuler Feb. 13, 2019, 9:10 a.m. UTC 
  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(-)

Comments

Gaëtan Rivet Feb. 13, 2019, 9:45 a.m. UTC | #1 
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 Feb. 13, 2019, 11:38 a.m. UTC | #2 
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@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
Burakov, Anatoly Feb. 13, 2019, 2:41 p.m. UTC | #3 
On 13-Feb-19 9:10 AM, 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 haven't looked at the patchset in depth yet, however, this changes the 
public API, so warrants a note in release notes.
Shahaf Shuler Feb. 13, 2019, 3:23 p.m. UTC | #4 
Wednesday, February 13, 2019 11:45 AM, Gaëtan Rivet:
> Subject: Re: [PATCH 1/6] vfio: allow DMA map of memory for the default vfio
> fd
> 
> Hello Shahaf,

Hi Gaetan.

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

[...]

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

Good suggestion. Will adjust in v2. 

> >  	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
diff mbox series

Patch

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.
  *
  * @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) {
+		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;
@@ -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;