[1/6] vfio: allow DMA map of memory for the default vfio fd
Checks
Commit Message
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
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
>
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
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.
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
@@ -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.
@@ -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;