Re: [PATCH dpdk 4/5] vfio: Do try setting IOMMU type if already set

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

> 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