Re: [dpdk-dev] [PATCH 2/6] vfio: don't fail to DMA map if memory is already mapped

[Shahaf Shuler <shahafs@mellanox.com>]

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