[dpdk-dev,4/5] vfio: Do try setting IOMMU type if already set

Message ID 20170420072402.38106-5-aik@ozlabs.ru (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Alexey Kardashevskiy April 20, 2017, 7:24 a.m. UTC
  The existing code correctly checks if a container is set to a group and
does not try attaching a group to a container for a second/third/...
device from the same IOMMU group.

However it still tries to set IOMMU type to a container for every device
in a group which produces failure and closes container and group fds.

This moves vfio_set_iommu_type() and DMA mapping code under:
if (!(group_status.flags & VFIO_GROUP_FLAGS_CONTAINER_SET))

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 lib/librte_eal/linuxapp/eal/eal_vfio.c | 50 +++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 25 deletions(-)
  

Comments

Gowrishankar April 20, 2017, 7:31 p.m. UTC | #1
On Thursday 20 April 2017 12:54 PM, Alexey Kardashevskiy wrote:
> The existing code correctly checks if a container is set to a group and
> does not try attaching a group to a container for a second/third/...
> device from the same IOMMU group.
>
> However it still tries to set IOMMU type to a container for every device
> in a group which produces failure and closes container and group fds.
>
> This moves vfio_set_iommu_type() and DMA mapping code under:
> if (!(group_status.flags & VFIO_GROUP_FLAGS_CONTAINER_SET))
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Thanks, verified it for regression seen on current master for mentioned 
scenario.

Regards,
Gowrishankar

> ---
>   lib/librte_eal/linuxapp/eal/eal_vfio.c | 50 +++++++++++++++++-----------------
>   1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> index 6e2e84ca7..46f951f4d 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> @@ -298,33 +298,33 @@ vfio_setup_device(const char *sysfs_base, const char *dev_addr,
>   			clear_group(vfio_group_fd);
>   			return -1;
>   		}
> -	}
>
> -	/*
> -	 * pick an IOMMU type and set up DMA mappings for container
> -	 *
> -	 * needs to be done only once, only when first group is assigned to
> -	 * a container and only in primary process. Note this can happen several
> -	 * times with the hotplug functionality.
> -	 */
> -	if (internal_config.process_type == RTE_PROC_PRIMARY &&
> -			vfio_cfg.vfio_active_groups == 1) {
> -		/* select an IOMMU type which we will be using */
> -		const struct vfio_iommu_type *t =
> +		/*
> +		 * pick an IOMMU type and set up DMA mappings for container
> +		 *
> +		 * needs to be done only once, only when first group is assigned to
> +		 * a container and only in primary process. Note this can happen several
> +		 * times with the hotplug functionality.
> +		 */
> +		if (internal_config.process_type == RTE_PROC_PRIMARY &&
> +				vfio_cfg.vfio_active_groups == 1) {
> +			/* select an IOMMU type which we will be using */
> +			const struct vfio_iommu_type *t =
>   				vfio_set_iommu_type(vfio_cfg.vfio_container_fd);
> -		if (!t) {
> -			RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n", dev_addr);
> -			close(vfio_group_fd);
> -			clear_group(vfio_group_fd);
> -			return -1;
> -		}
> -		ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
> -		if (ret) {
> -			RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
> -					"error %i (%s)\n", dev_addr, errno, strerror(errno));
> -			close(vfio_group_fd);
> -			clear_group(vfio_group_fd);
> -			return -1;
> +			if (!t) {
> +				RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n", dev_addr);
> +				close(vfio_group_fd);
> +				clear_group(vfio_group_fd);
> +				return -1;
> +			}
> +			ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
> +			if (ret) {
> +				RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
> +						"error %i (%s)\n", dev_addr, errno, strerror(errno));
> +				close(vfio_group_fd);
> +				clear_group(vfio_group_fd);
> +				return -1;
> +			}
>   		}
>   	}
>
  
Andrew Rybchenko April 21, 2017, 8:54 a.m. UTC | #2
On 04/20/2017 10:24 AM, Alexey Kardashevskiy wrote:
> The existing code correctly checks if a container is set to a group and
> does not try attaching a group to a container for a second/third/...
> device from the same IOMMU group.
>
> However it still tries to set IOMMU type to a container for every device
> in a group which produces failure and closes container and group fds.
>
> This moves vfio_set_iommu_type() and DMA mapping code under:
> if (!(group_status.flags & VFIO_GROUP_FLAGS_CONTAINER_SET))
>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>   lib/librte_eal/linuxapp/eal/eal_vfio.c | 50 +++++++++++++++++-----------------
>   1 file changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> index 6e2e84ca7..46f951f4d 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> @@ -298,33 +298,33 @@ vfio_setup_device(const char *sysfs_base, const char *dev_addr,
>   			clear_group(vfio_group_fd);
>   			return -1;
>   		}
> -	}
>   
> -	/*
> -	 * pick an IOMMU type and set up DMA mappings for container
> -	 *
> -	 * needs to be done only once, only when first group is assigned to
> -	 * a container and only in primary process. Note this can happen several
> -	 * times with the hotplug functionality.
> -	 */
> -	if (internal_config.process_type == RTE_PROC_PRIMARY &&
> -			vfio_cfg.vfio_active_groups == 1) {
> -		/* select an IOMMU type which we will be using */
> -		const struct vfio_iommu_type *t =
> +		/*
> +		 * pick an IOMMU type and set up DMA mappings for container
> +		 *
> +		 * needs to be done only once, only when first group is assigned to
> +		 * a container and only in primary process. Note this can happen several
> +		 * times with the hotplug functionality.
> +		 */
> +		if (internal_config.process_type == RTE_PROC_PRIMARY &&
> +				vfio_cfg.vfio_active_groups == 1) {
> +			/* select an IOMMU type which we will be using */
> +			const struct vfio_iommu_type *t =
>   				vfio_set_iommu_type(vfio_cfg.vfio_container_fd);
> -		if (!t) {
> -			RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n", dev_addr);
> -			close(vfio_group_fd);
> -			clear_group(vfio_group_fd);
> -			return -1;
> -		}
> -		ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
> -		if (ret) {
> -			RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
> -					"error %i (%s)\n", dev_addr, errno, strerror(errno));
> -			close(vfio_group_fd);
> -			clear_group(vfio_group_fd);
> -			return -1;
> +			if (!t) {
> +				RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n", dev_addr);
> +				close(vfio_group_fd);
> +				clear_group(vfio_group_fd);
> +				return -1;
> +			}
> +			ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
> +			if (ret) {
> +				RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
> +						"error %i (%s)\n", dev_addr, errno, strerror(errno));
> +				close(vfio_group_fd);
> +				clear_group(vfio_group_fd);
> +				return -1;
> +			}
>   		}
>   	}

It looks like a duplicate of the earlier submitted patch 
http://dpdk.org/ml/archives/dev/2017-April/063077.html

Andrew.
  
Alexey Kardashevskiy April 26, 2017, 7:50 a.m. UTC | #3
On 21/04/17 18:54, Andrew Rybchenko wrote:
> On 04/20/2017 10:24 AM, Alexey Kardashevskiy wrote:
>> The existing code correctly checks if a container is set to a group and
>> does not try attaching a group to a container for a second/third/...
>> device from the same IOMMU group.
>>
>> However it still tries to set IOMMU type to a container for every device
>> in a group which produces failure and closes container and group fds.
>>
>> This moves vfio_set_iommu_type() and DMA mapping code under:
>> if (!(group_status.flags & VFIO_GROUP_FLAGS_CONTAINER_SET))
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_vfio.c | 50
>> +++++++++++++++++-----------------
>>   1 file changed, 25 insertions(+), 25 deletions(-)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c
>> b/lib/librte_eal/linuxapp/eal/eal_vfio.c
>> index 6e2e84ca7..46f951f4d 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
>> @@ -298,33 +298,33 @@ vfio_setup_device(const char *sysfs_base, const
>> char *dev_addr,
>>               clear_group(vfio_group_fd);
>>               return -1;
>>           }
>> -    }
>>   -    /*
>> -     * pick an IOMMU type and set up DMA mappings for container
>> -     *
>> -     * needs to be done only once, only when first group is assigned to
>> -     * a container and only in primary process. Note this can happen
>> several
>> -     * times with the hotplug functionality.
>> -     */
>> -    if (internal_config.process_type == RTE_PROC_PRIMARY &&
>> -            vfio_cfg.vfio_active_groups == 1) {
>> -        /* select an IOMMU type which we will be using */
>> -        const struct vfio_iommu_type *t =
>> +        /*
>> +         * pick an IOMMU type and set up DMA mappings for container
>> +         *
>> +         * needs to be done only once, only when first group is assigned to
>> +         * a container and only in primary process. Note this can happen
>> several
>> +         * times with the hotplug functionality.
>> +         */
>> +        if (internal_config.process_type == RTE_PROC_PRIMARY &&
>> +                vfio_cfg.vfio_active_groups == 1) {
>> +            /* select an IOMMU type which we will be using */
>> +            const struct vfio_iommu_type *t =
>>                   vfio_set_iommu_type(vfio_cfg.vfio_container_fd);
>> -        if (!t) {
>> -            RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n",
>> dev_addr);
>> -            close(vfio_group_fd);
>> -            clear_group(vfio_group_fd);
>> -            return -1;
>> -        }
>> -        ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
>> -        if (ret) {
>> -            RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
>> -                    "error %i (%s)\n", dev_addr, errno, strerror(errno));
>> -            close(vfio_group_fd);
>> -            clear_group(vfio_group_fd);
>> -            return -1;
>> +            if (!t) {
>> +                RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n",
>> dev_addr);
>> +                close(vfio_group_fd);
>> +                clear_group(vfio_group_fd);
>> +                return -1;
>> +            }
>> +            ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
>> +            if (ret) {
>> +                RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
>> +                        "error %i (%s)\n", dev_addr, errno,
>> strerror(errno));
>> +                close(vfio_group_fd);
>> +                clear_group(vfio_group_fd);
>> +                return -1;
>> +            }
>>           }
>>       }
> 
> It looks like a duplicate of the earlier submitted patch
> http://dpdk.org/ml/archives/dev/2017-April/063077.html


It is a duplicate indeed, what needs to be done to have it accepted in the
mainline tree?
  
Burakov, Anatoly April 26, 2017, 8:27 a.m. UTC | #4
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Alexey
> Kardashevskiy
> Sent: Wednesday, April 26, 2017 8:51 AM
> To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> Cc: JPF@zurich.ibm.com; Gowrishankar Muthukrishnan
> <gowrishankar.m@in.ibm.com>
> Subject: Re: [dpdk-dev] [PATCH dpdk 4/5] vfio: Do try setting IOMMU type if
> already set
> 
> On 21/04/17 18:54, Andrew Rybchenko wrote:
> > On 04/20/2017 10:24 AM, Alexey Kardashevskiy wrote:
> >> The existing code correctly checks if a container is set to a group
> >> and does not try attaching a group to a container for a second/third/...
> >> device from the same IOMMU group.
> >>
> >> However it still tries to set IOMMU type to a container for every
> >> device in a group which produces failure and closes container and group
> fds.
> >>
> >> This moves vfio_set_iommu_type() and DMA mapping code under:
> >> if (!(group_status.flags & VFIO_GROUP_FLAGS_CONTAINER_SET))
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >>   lib/librte_eal/linuxapp/eal/eal_vfio.c | 50
> >> +++++++++++++++++-----------------
> >>   1 file changed, 25 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> >> b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> >> index 6e2e84ca7..46f951f4d 100644
> >> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> >> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> >> @@ -298,33 +298,33 @@ vfio_setup_device(const char *sysfs_base,
> const
> >> char *dev_addr,
> >>               clear_group(vfio_group_fd);
> >>               return -1;
> >>           }
> >> -    }
> >>   -    /*
> >> -     * pick an IOMMU type and set up DMA mappings for container
> >> -     *
> >> -     * needs to be done only once, only when first group is assigned to
> >> -     * a container and only in primary process. Note this can happen
> >> several
> >> -     * times with the hotplug functionality.
> >> -     */
> >> -    if (internal_config.process_type == RTE_PROC_PRIMARY &&
> >> -            vfio_cfg.vfio_active_groups == 1) {
> >> -        /* select an IOMMU type which we will be using */
> >> -        const struct vfio_iommu_type *t =
> >> +        /*
> >> +         * pick an IOMMU type and set up DMA mappings for container
> >> +         *
> >> +         * needs to be done only once, only when first group is assigned to
> >> +         * a container and only in primary process. Note this can
> >> + happen
> >> several
> >> +         * times with the hotplug functionality.
> >> +         */
> >> +        if (internal_config.process_type == RTE_PROC_PRIMARY &&
> >> +                vfio_cfg.vfio_active_groups == 1) {
> >> +            /* select an IOMMU type which we will be using */
> >> +            const struct vfio_iommu_type *t =
> >>                   vfio_set_iommu_type(vfio_cfg.vfio_container_fd);
> >> -        if (!t) {
> >> -            RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n",
> >> dev_addr);
> >> -            close(vfio_group_fd);
> >> -            clear_group(vfio_group_fd);
> >> -            return -1;
> >> -        }
> >> -        ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
> >> -        if (ret) {
> >> -            RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
> >> -                    "error %i (%s)\n", dev_addr, errno, strerror(errno));
> >> -            close(vfio_group_fd);
> >> -            clear_group(vfio_group_fd);
> >> -            return -1;
> >> +            if (!t) {
> >> +                RTE_LOG(ERR, EAL, "  %s failed to select IOMMU
> >> + type\n",
> >> dev_addr);
> >> +                close(vfio_group_fd);
> >> +                clear_group(vfio_group_fd);
> >> +                return -1;
> >> +            }
> >> +            ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
> >> +            if (ret) {
> >> +                RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
> >> +                        "error %i (%s)\n", dev_addr, errno,
> >> strerror(errno));
> >> +                close(vfio_group_fd);
> >> +                clear_group(vfio_group_fd);
> >> +                return -1;
> >> +            }
> >>           }
> >>       }
> >
> > It looks like a duplicate of the earlier submitted patch
> > http://dpdk.org/ml/archives/dev/2017-April/063077.html
> 
> 
> It is a duplicate indeed, what needs to be done to have it accepted in the
> mainline tree?

I think Alejandro Lucero (CC'd) mentioned that he has another patch for similar issue that he's working on (for ports being
assigned to the same IOMMU group). That patch would presumably fix this issue as well, so I'm inclined to
wait for that one. If that's not happening, then we can accept either this one or the earlier ones, and fix the
remaining issues (I have a setup where I can attempt to reproduce the issue, so that should be quick enough).

Thanks,
Anatoly
  
Alejandro Lucero April 26, 2017, 8:45 a.m. UTC | #5
On Wed, Apr 26, 2017 at 9:27 AM, Burakov, Anatoly <anatoly.burakov@intel.com
> wrote:

> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Alexey
> > Kardashevskiy
> > Sent: Wednesday, April 26, 2017 8:51 AM
> > To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> > Cc: JPF@zurich.ibm.com; Gowrishankar Muthukrishnan
> > <gowrishankar.m@in.ibm.com>
> > Subject: Re: [dpdk-dev] [PATCH dpdk 4/5] vfio: Do try setting IOMMU type
> if
> > already set
> >
> > On 21/04/17 18:54, Andrew Rybchenko wrote:
> > > On 04/20/2017 10:24 AM, Alexey Kardashevskiy wrote:
> > >> The existing code correctly checks if a container is set to a group
> > >> and does not try attaching a group to a container for a
> second/third/...
> > >> device from the same IOMMU group.
> > >>
> > >> However it still tries to set IOMMU type to a container for every
> > >> device in a group which produces failure and closes container and
> group
> > fds.
> > >>
> > >> This moves vfio_set_iommu_type() and DMA mapping code under:
> > >> if (!(group_status.flags & VFIO_GROUP_FLAGS_CONTAINER_SET))
> > >>
> > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > >> ---
> > >>   lib/librte_eal/linuxapp/eal/eal_vfio.c | 50
> > >> +++++++++++++++++-----------------
> > >>   1 file changed, 25 insertions(+), 25 deletions(-)
> > >>
> > >> diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > >> b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > >> index 6e2e84ca7..46f951f4d 100644
> > >> --- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > >> +++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
> > >> @@ -298,33 +298,33 @@ vfio_setup_device(const char *sysfs_base,
> > const
> > >> char *dev_addr,
> > >>               clear_group(vfio_group_fd);
> > >>               return -1;
> > >>           }
> > >> -    }
> > >>   -    /*
> > >> -     * pick an IOMMU type and set up DMA mappings for container
> > >> -     *
> > >> -     * needs to be done only once, only when first group is assigned
> to
> > >> -     * a container and only in primary process. Note this can happen
> > >> several
> > >> -     * times with the hotplug functionality.
> > >> -     */
> > >> -    if (internal_config.process_type == RTE_PROC_PRIMARY &&
> > >> -            vfio_cfg.vfio_active_groups == 1) {
> > >> -        /* select an IOMMU type which we will be using */
> > >> -        const struct vfio_iommu_type *t =
> > >> +        /*
> > >> +         * pick an IOMMU type and set up DMA mappings for container
> > >> +         *
> > >> +         * needs to be done only once, only when first group is
> assigned to
> > >> +         * a container and only in primary process. Note this can
> > >> + happen
> > >> several
> > >> +         * times with the hotplug functionality.
> > >> +         */
> > >> +        if (internal_config.process_type == RTE_PROC_PRIMARY &&
> > >> +                vfio_cfg.vfio_active_groups == 1) {
> > >> +            /* select an IOMMU type which we will be using */
> > >> +            const struct vfio_iommu_type *t =
> > >>                   vfio_set_iommu_type(vfio_cfg.vfio_container_fd);
> > >> -        if (!t) {
> > >> -            RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n",
> > >> dev_addr);
> > >> -            close(vfio_group_fd);
> > >> -            clear_group(vfio_group_fd);
> > >> -            return -1;
> > >> -        }
> > >> -        ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
> > >> -        if (ret) {
> > >> -            RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
> > >> -                    "error %i (%s)\n", dev_addr, errno,
> strerror(errno));
> > >> -            close(vfio_group_fd);
> > >> -            clear_group(vfio_group_fd);
> > >> -            return -1;
> > >> +            if (!t) {
> > >> +                RTE_LOG(ERR, EAL, "  %s failed to select IOMMU
> > >> + type\n",
> > >> dev_addr);
> > >> +                close(vfio_group_fd);
> > >> +                clear_group(vfio_group_fd);
> > >> +                return -1;
> > >> +            }
> > >> +            ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
> > >> +            if (ret) {
> > >> +                RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
> > >> +                        "error %i (%s)\n", dev_addr, errno,
> > >> strerror(errno));
> > >> +                close(vfio_group_fd);
> > >> +                clear_group(vfio_group_fd);
> > >> +                return -1;
> > >> +            }
> > >>           }
> > >>       }
> > >
> > > It looks like a duplicate of the earlier submitted patch
> > > http://dpdk.org/ml/archives/dev/2017-April/063077.html
> >
> >
> > It is a duplicate indeed, what needs to be done to have it accepted in
> the
> > mainline tree?
>
> I think Alejandro Lucero (CC'd) mentioned that he has another patch for
> similar issue that he's working on (for ports being
> assigned to the same IOMMU group). That patch would presumably fix this
> issue as well, so I'm inclined to
> wait for that one. If that's not happening, then we can accept either this
> one or the earlier ones, and fix the
> remaining issues (I have a setup where I can attempt to reproduce the
> issue, so that should be quick enough).
>
>
I was counting on first submitted patch being accepted.

There is another thing to solve which is to unplug ports when there are
more than one (already plugged) in an IOMMU group. I have a patch for this
but I can not test it for that scenario, just for the "normal" one with one
port/device per IOMMU group. I'm working on having a way for testing the
first case.

Once I have said that, I would like to comment code after the patch fixing
the plug issue is fine. What I mean is, unplugging a device when more than
one device already plugged from the same IOMMU group do not break things.
Of course, I can only say that from a theoretical point of view after
studying the kernel VFIO code. The only problem is after that, none else
device hotplug can happen. I know that is not a good thing because it
requires to restart the app, but better than breaking the app with a crash.

I will submit later today the patch for fixing the unplug issue. Maybe
someone can test that asap while I get some way for doing it.



> Thanks,
> Anatoly
>
  
Burakov, Anatoly April 26, 2017, 8:58 a.m. UTC | #6
Hi Alejandro,

> I was counting on first submitted patch being accepted. 


OK, that's on me then.

> There is another thing to solve which is to unplug ports when there are more than one (already plugged) in an IOMMU group. I have a patch for this but I can not test it for that scenario, just for the "normal" one with one port/device per IOMMU group. > I'm working on having a way for testing the first case.


Have you tried older kernel versions yet? Those seem to work for me.

> The only problem is after that, none else device hotplug can happen. I know that is not a good thing because it requires to restart the app, but better than breaking the app with a crash.


Presumably, you're only talking about VFIO hotplug, and not hotplug in general? Is there anything that can be done to fix this issue in the long run? 
 
Thanks,
Anatoly
  
Alejandro Lucero April 26, 2017, 10:24 a.m. UTC | #7
On Wed, Apr 26, 2017 at 9:58 AM, Burakov, Anatoly <anatoly.burakov@intel.com
> wrote:

> Hi Alejandro,
>
> > I was counting on first submitted patch being accepted.
>
> OK, that's on me then.
>
> > There is another thing to solve which is to unplug ports when there are
> more than one (already plugged) in an IOMMU group. I have a patch for this
> but I can not test it for that scenario, just for the "normal" one with one
> port/device per IOMMU group. > I'm working on having a way for testing the
> first case.
>
> Have you tried older kernel versions yet? Those seem to work for me.
>
>
I'm trying that one but run in some problems. Likely that is the quickest
way although I'm thinking on adding some option to the kernel for people
with such a hardware being able to test that option.



> > The only problem is after that, none else device hotplug can happen. I
> know that is not a good thing because it requires to restart the app, but
> better than breaking the app with a crash.
>
> Presumably, you're only talking about VFIO hotplug, and not hotplug in
> general? Is there anything that can be done to fix this issue in the long
> run?
>
>

Yes, just talking about VFIO. I think that patch I have should solve the
issue. It is just a matter of tracking how many devices are in use on a
vfio group, so just adding a new field to struct vfio_group and checking
number of devices when unplugging should be enough. Just when is the only
device plugged should close the vfio group.


> Thanks,
> Anatoly
>
>
  

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_vfio.c b/lib/librte_eal/linuxapp/eal/eal_vfio.c
index 6e2e84ca7..46f951f4d 100644
--- a/lib/librte_eal/linuxapp/eal/eal_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_vfio.c
@@ -298,33 +298,33 @@  vfio_setup_device(const char *sysfs_base, const char *dev_addr,
 			clear_group(vfio_group_fd);
 			return -1;
 		}
-	}
 
-	/*
-	 * pick an IOMMU type and set up DMA mappings for container
-	 *
-	 * needs to be done only once, only when first group is assigned to
-	 * a container and only in primary process. Note this can happen several
-	 * times with the hotplug functionality.
-	 */
-	if (internal_config.process_type == RTE_PROC_PRIMARY &&
-			vfio_cfg.vfio_active_groups == 1) {
-		/* select an IOMMU type which we will be using */
-		const struct vfio_iommu_type *t =
+		/*
+		 * pick an IOMMU type and set up DMA mappings for container
+		 *
+		 * needs to be done only once, only when first group is assigned to
+		 * a container and only in primary process. Note this can happen several
+		 * times with the hotplug functionality.
+		 */
+		if (internal_config.process_type == RTE_PROC_PRIMARY &&
+				vfio_cfg.vfio_active_groups == 1) {
+			/* select an IOMMU type which we will be using */
+			const struct vfio_iommu_type *t =
 				vfio_set_iommu_type(vfio_cfg.vfio_container_fd);
-		if (!t) {
-			RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n", dev_addr);
-			close(vfio_group_fd);
-			clear_group(vfio_group_fd);
-			return -1;
-		}
-		ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
-		if (ret) {
-			RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
-					"error %i (%s)\n", dev_addr, errno, strerror(errno));
-			close(vfio_group_fd);
-			clear_group(vfio_group_fd);
-			return -1;
+			if (!t) {
+				RTE_LOG(ERR, EAL, "  %s failed to select IOMMU type\n", dev_addr);
+				close(vfio_group_fd);
+				clear_group(vfio_group_fd);
+				return -1;
+			}
+			ret = t->dma_map_func(vfio_cfg.vfio_container_fd);
+			if (ret) {
+				RTE_LOG(ERR, EAL, "  %s DMA remapping failed, "
+						"error %i (%s)\n", dev_addr, errno, strerror(errno));
+				close(vfio_group_fd);
+				clear_group(vfio_group_fd);
+				return -1;
+			}
 		}
 	}