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

[Alejandro Lucero <alejandro.lucero@netronome.com>]

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
>