diff mbox series

[2/6] vfio: don't fail to DMA map if memory is already mapped

Message ID d7d3ce0bdb942c84df2a6c23282df4617378dd45.1550048188.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-compilation success Compilation OK

Commit Message

Shahaf Shuler Feb. 13, 2019, 9:10 a.m. UTC 
  Currently vfio DMA map function will fail in case the same memory
segment is mapped twice.

This is too strict, as this is not an error to map the same memory
twice.

Instead, use the kernel return value to detect such state and have the
DMA function to return as successful.

For type1 mapping the kernel driver first implementation returns EBUSY
and since kernel 3.11 returns EEXISTS. For spapr mapping EBUSY is returned
since kernel 4.10.

Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
---
 lib/librte_eal/linuxapp/eal/eal_vfio.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Gaëtan Rivet Feb. 13, 2019, 9:58 a.m. UTC | #1 
On Wed, Feb 13, 2019 at 11:10:22AM +0200, Shahaf Shuler wrote:
> Currently vfio DMA map function will fail in case the same memory
> segment is mapped twice.
> 
> This is too strict, as this is not an error to map the same memory
> twice.
> 
> Instead, use the kernel return value to detect such state and have the
> DMA function to return as successful.
> 
> For type1 mapping the kernel driver first implementation returns EBUSY
> and since kernel 3.11 returns EEXISTS. For spapr mapping EBUSY is returned
> since kernel 4.10.
> 

What is the earliest version supported by DPDK? I thought 3.10 was
dropped, should we care about the 3.11 return value?

> Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_vfio.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> index 48ca9465d4..2a2d655b37 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> @@ -1263,7 +1263,11 @@ vfio_type1_dma_mem_map(int vfio_container_fd, uint64_t vaddr, uint64_t iova,
>  				VFIO_DMA_MAP_FLAG_WRITE;
>  
>  		ret = ioctl(vfio_container_fd, VFIO_IOMMU_MAP_DMA, &dma_map);
> -		if (ret) {
> +		/**
> +		 * In case the mapping was already done EEXIST will be
> +		 * returned from kernel.
> +		 */
> +		if ((ret != -EEXIST) && (ret != -EBUSY)) {

Won't a ret == 0 trigger the error then?

It seems ifdef about linux versions are not common in vfio code, but
bar that I think it would be cleaner to restrict the acceptable
error to it.

When a version will be dropped it will be much easier to remove the
related code instead of digging in the commit logs, and leaving both
would not be clean.

>  			RTE_LOG(ERR, EAL, "  cannot set up DMA remapping, error %i (%s)\n",
>  				errno, strerror(errno));
>  				return -1;
> @@ -1324,7 +1328,11 @@ vfio_spapr_dma_do_map(int vfio_container_fd, uint64_t vaddr, uint64_t iova,
>  				VFIO_DMA_MAP_FLAG_WRITE;
>  
>  		ret = ioctl(vfio_container_fd, VFIO_IOMMU_MAP_DMA, &dma_map);
> -		if (ret) {
> +		/**
> +		 * In case the mapping was already done EBUSY will be
> +		 * returned from kernel.
> +		 */
> +		if (ret != -EBUSY) {
>  			RTE_LOG(ERR, EAL, "  cannot set up DMA remapping, error %i (%s)\n",
>  				errno, strerror(errno));
>  				return -1;
> -- 
> 2.12.0
>
Shahaf Shuler Feb. 13, 2019, 7:52 p.m. UTC | #2 
Wednesday, February 13, 2019 11:59 AM, Gaëtan RiveT:
> Subject: Re: [PATCH 2/6] vfio: don't fail to DMA map if memory is already
> mapped
> 
> On Wed, Feb 13, 2019 at 11:10:22AM +0200, Shahaf Shuler wrote:
> > Currently vfio DMA map function will fail in case the same memory
> > segment is mapped twice.
> >
> > This is too strict, as this is not an error to map the same memory
> > twice.
> >
> > Instead, use the kernel return value to detect such state and have the
> > DMA function to return as successful.
> >
> > For type1 mapping the kernel driver first implementation returns EBUSY
> > and since kernel 3.11 returns EEXISTS. For spapr mapping EBUSY is
> > returned since kernel 4.10.
> >
> 
> What is the earliest version supported by DPDK? I thought 3.10 was dropped,
> should we care about the 3.11 return value?

According to DPDK doc it is 3.16, however compatibility for Centos/RH 7 should be kept.
I have looked on Centos 7 source code (linux-3.10.0-957.5.1.el7), it uses EEXIST for the vfio type 1. 

So I think, I can drop the EBUSY error check and leave only the EEXIST one. 


> 
> > Signed-off-by: Shahaf Shuler <shahafs@mellanox.com>
> > ---
> >  lib/librte_eal/linuxapp/eal/eal_vfio.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > index 48ca9465d4..2a2d655b37 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > @@ -1263,7 +1263,11 @@ vfio_type1_dma_mem_map(int
> vfio_container_fd, uint64_t vaddr, uint64_t iova,
> >  				VFIO_DMA_MAP_FLAG_WRITE;
> >
> >  		ret = ioctl(vfio_container_fd, VFIO_IOMMU_MAP_DMA,
> &dma_map);
> > -		if (ret) {
> > +		/**
> > +		 * In case the mapping was already done EEXIST will be
> > +		 * returned from kernel.
> > +		 */
> > +		if ((ret != -EEXIST) && (ret != -EBUSY)) {
> 
> Won't a ret == 0 trigger the error then?

Yes good catch. 

> 
> It seems ifdef about linux versions are not common in vfio code, but bar that
> I think it would be cleaner to restrict the acceptable error to it.
> 
> When a version will be dropped it will be much easier to remove the related
> code instead of digging in the commit logs, and leaving both would not be
> clean.

Relaying on kernel version is not safe enough. Many distro backport to their kernel w/o updating its version. This will lead to unexpected behavior. 

> 
> >  			RTE_LOG(ERR, EAL, "  cannot set up DMA remapping,
> error %i (%s)\n",
> >  				errno, strerror(errno));
> >  				return -1;
> > @@ -1324,7 +1328,11 @@ vfio_spapr_dma_do_map(int vfio_container_fd,
> uint64_t vaddr, uint64_t iova,
> >  				VFIO_DMA_MAP_FLAG_WRITE;
> >
> >  		ret = ioctl(vfio_container_fd, VFIO_IOMMU_MAP_DMA,
> &dma_map);
> > -		if (ret) {
> > +		/**
> > +		 * In case the mapping was already done EBUSY will be
> > +		 * returned from kernel.
> > +		 */
> > +		if (ret != -EBUSY) {
> >  			RTE_LOG(ERR, EAL, "  cannot set up DMA remapping,
> error %i (%s)\n",
> >  				errno, strerror(errno));
> >  				return -1;
> > --
> > 2.12.0
> >
> 
> --
> Gaëtan Rivet
> 6WIND
diff mbox series

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
index 48ca9465d4..2a2d655b37 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
@@ -1263,7 +1263,11 @@  vfio_type1_dma_mem_map(int vfio_container_fd, uint64_t vaddr, uint64_t iova,
 				VFIO_DMA_MAP_FLAG_WRITE;
 
 		ret = ioctl(vfio_container_fd, VFIO_IOMMU_MAP_DMA, &dma_map);
-		if (ret) {
+		/**
+		 * In case the mapping was already done EEXIST will be
+		 * returned from kernel.
+		 */
+		if ((ret != -EEXIST) && (ret != -EBUSY)) {
 			RTE_LOG(ERR, EAL, "  cannot set up DMA remapping, error %i (%s)\n",
 				errno, strerror(errno));
 				return -1;
@@ -1324,7 +1328,11 @@  vfio_spapr_dma_do_map(int vfio_container_fd, uint64_t vaddr, uint64_t iova,
 				VFIO_DMA_MAP_FLAG_WRITE;
 
 		ret = ioctl(vfio_container_fd, VFIO_IOMMU_MAP_DMA, &dma_map);
-		if (ret) {
+		/**
+		 * In case the mapping was already done EBUSY will be
+		 * returned from kernel.
+		 */
+		if (ret != -EBUSY) {
 			RTE_LOG(ERR, EAL, "  cannot set up DMA remapping, error %i (%s)\n",
 				errno, strerror(errno));
 				return -1;