Re: VFIO no-iommu

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 2015-12-21 at 12:22 -0700, Alex Williamson wrote:
> On Mon, 2015-12-21 at 11:46 +0000, Yigit, Ferruh wrote:
> > On Fri, Dec 18, 2015 at 02:50:17PM -0700, Alex Williamson wrote:
> > > On Fri, 2015-12-18 at 07:38 -0700, Alex Williamson wrote:
> > > > On Fri, 2015-12-18 at 10:43 +0000, Yigit, Ferruh wrote:
> > > > > On Thu, Dec 17, 2015 at 09:43:59AM -0700, Alex Williamson
> > > > > wrote:
> > > > > <...>
> > > > > > > > > > > 
> > > > > > > > > > > Also I need to disable VFIO_CHECK_EXTENSION
> > > > > > > > > > > ioctl,
> > > > > > > > > > > because in
> > > > > > > > > > > vfio
> > > > > > > > > > > module,
> > > > > > > > > > > container->noiommu is not set before doing a
> > > > > > > > > > > vfio_group_set_container()
> > > > > > > > > > > and vfio_for_each_iommu_driver selects wrong
> > > > > > > > > > > driver.
> > > > > > > > > > 
> > > > > > > > > > Running CHECK_EXTENSION on a container without the
> > > > > > > > > > group
> > > > > > > > > > attached is
> > > > > > > > > > only going to tell you what extensions vfio is
> > > > > > > > > > capable
> > > > > > > > > > of,
> > > > > > > > > > not
> > > > > > > > > > necessarily what extensions are available to you
> > > > > > > > > > with
> > > > > > > > > > that
> > > > > > > > > > group.
> > > > > > > > > > Is this just a general dpdk- vfio ordering bug?
> > > > > > > > > 
> > > > > > > > > Yes, that is how VFIO was implemented in DPDK. I was
> > > > > > > > > under
> > > > > > > > > the
> > > > > > > > > impression that checking extension before assigning
> > > > > > > > > devices
> > > > > > > > > was
> > > > > > > > > the
> > > > > > > > > correct way to do things, so as to not to try
> > > > > > > > > anything
> > > > > > > > > we
> > > > > > > > > know
> > > > > > > > > would
> > > > > > > > > fail anyway. Does this imply that CHECK_EXTENSION
> > > > > > > > > needs
> > > > > > > > > to
> > > > > > > > > be
> > > > > > > > > called
> > > > > > > > > on both container and groups (or just on groups)?
> > > > > > > > 
> > > > > > > > Hmm, in Documentation/vfio.txt we do give the following
> > > > > > > > algorithm:
> > > > > > > > 
> > > > > > > >         if (ioctl(container, VFIO_GET_API_VERSION) !=
> > > > > > > > VFIO_API_VERSION)
> > > > > > > >                 /* Unknown API version */
> > > > > > > > 
> > > > > > > >         if (!ioctl(container, VFIO_CHECK_EXTENSION,
> > > > > > > > VFIO_TYPE1_IOMMU))
> > > > > > > >                 /* Doesn't support the IOMMU driver we
> > > > > > > > want.
> > > > > > > > */
> > > > > > > >         ...
> > > > > > > > 
> > > > > > > > That's just going to query each iommu driver and we
> > > > > > > > can't
> > > > > > > > yet
> > > > > > > > say
> > > > > > > > whether
> > > > > > > > the group the user attaches to the container later will
> > > > > > > > actually
> > > > > > > > support that
> > > > > > > > extension until we try to do it, that would come at
> > > > > > > > VFIO_SET_IOMMU.
> > > > > > > >  So is
> > > > > > > > it perhaps a vfio bug that we're not advertising no-
> > > > > > > > iommu
> > > > > > > > until
> > > > > > > > the
> > > > > > > > group is
> > > > > > > > attached?  After all, we are capable of it with just an
> > > > > > > > empty
> > > > > > > > container, just
> > > > > > > > like we are with type1, but we're going to fail
> > > > > > > > SET_IOMMU
> > > > > > > > for
> > > > > > > > the
> > > > > > > > wrong
> > > > > > > > combination.
> > > > > > > >  This is exactly the sort of thing that makes me glad
> > > > > > > > we
> > > > > > > > reverted
> > > > > > > > it without
> > > > > > > > feedback from a working user driver.  Thanks,
> > > > > > > 
> > > > > > > Whether it should be considered a "bug" in VFIO or "by
> > > > > > > design"
> > > > > > > is
> > > > > > > up
> > > > > > > to you, of course, but at least according to the VFIO
> > > > > > > documentation,
> > > > > > > we are meant to check for type 1 extension and then
> > > > > > > attach
> > > > > > > devices,
> > > > > > > so it would be expected to get VFIO_NOIOMMU_IOMMU marked
> > > > > > > as
> > > > > > > supported
> > > > > > > even without any devices attached to the container (just
> > > > > > > like
> > > > > > > we
> > > > > > > get
> > > > > > > type 1 as supported without any devices attached). Having
> > > > > > > said
> > > > > > > that,
> > > > > > > if it was meant to attach devices first and then check
> > > > > > > the
> > > > > > > extensions, then perhaps the documentation should also
> > > > > > > point
> > > > > > > out
> > > > > > > that
> > > > > > > fact (or perhaps I missed that detail in my readings of
> > > > > > > the
> > > > > > > docs,
> > > > > > > in
> > > > > > > which case my apologies).
> > > > > > 
> > > > > > Hi Anatoly,
> > > > > > 
> > > > > > Does the below patch make it behave more like you'd expect.
> > > > > >  This
> > > > > > applies to v4.4-rc4, I'd fold this into the base patch if
> > > > > > we
> > > > > > reincorporate it to a future kernel.  Thanks,
> > > > > > 
> > > > > > Alex
> > > > > > 
> > > > > > commit 88d4dcb6b77624965f0b45b5cd305a2b4a105c94
> > > > > > Author: Alex Williamson <alex.williamson@redhat.com>
> > > > > > Date:   Wed Dec 16 19:02:01 2015 -0700
> > > > > > 
> > > > > >     vfio: Fix no-iommu CHECK_EXTENSION
> > > > > >     
> > > > > >     Previously the no-iommu iommu driver was only visible
> > > > > > when
> > > > > > the
> > > > > >     container had an attached no-iommu group.  This means
> > > > > > that
> > > > > >     CHECK_EXTENSION on and empty container couldn't report
> > > > > > the
> > > > > > possibility
> > > > > >     of using VFIO_NOIOMMU_IOMMU.  We report TYPE1 whether
> > > > > > or
> > > > > > not
> > > > > > the user
> > > > > >     can make use of it with the group, so this is
> > > > > > inconsistent.  Add the
> > > > > >     no-iommu iommu to the list of iommu drivers when
> > > > > > enabled
> > > > > > via
> > > > > > module
> > > > > >     option, but skip all the others if the container is
> > > > > > attached
> > > > > > to
> > > > > > a
> > > > > >     no-iommu groups.  Note that tainting is now done with
> > > > > > the
> > > > > > "unsafe"
> > > > > >     module callback rather than explictly within vfio.
> > > > > >     
> > > > > >     Also fixes module option and module description name
> > > > > > inconsistency.
> > > > > >     
> > > > > >     Also make vfio_noiommu_ops const.
> > > > > >     
> > > > > >     Signed-off-by: Alex Williamson <alex.williamson@redhat.
> > > > > > co
> > > > > > m>
> > > > > 
> > > > > Hi Alex,
> > > > > 
> > > > > I got following crash with this update:
> > > 
> > > Let's try this one:
> > > 
> > > commit 8ff839c6ffe9f3b50b50f1cc87e7afbf23171f05
> > > Author: Alex Williamson <alex.williamson@redhat.com>
> > > Date:   Fri Dec 18 14:45:55 2015 -0700
> > > 
> > >     v2 vfio fix no-iommu CHECK_EXTENSION
> > >     
> > >     Register and unregister the no-iommu iommu backend at module
> > >     initialization and exit, but disable it unless enabled via
> > > module
> > >     option.  Rather than modify the iommu driver walk,
> > > selectively
> > > skip
> > >     combinations that aren't supported.  CHECK_EXTENSION on a
> > > container
> > >     without any groups attached exposes all possible
> > > extensions.  Once a
> > >     group is attached, the no-iommu backend is skipped for
> > > regular
> > > groups
> > >     and regular iommu backends are skipped for no-iommu groups.
> > >     
> > >     This would be folded into a single patch to re-propose vfio
> > > no-
> > > iommu
> > >     mode upstream.
> > >     
> > >     Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > 
> > Hi Alex,
> > 
> > Thank you for the update. I have tested this on both no-iommu and
> > iommu environment
> > and worked successfully. I believe this approach is better because
> > it
> > is simpler.
> > 
> > From DPDK point of view, only update to support vfio no-iommu is:
> > to
> > use new group names
> > and disable DMA mapping.
> > 
> > If VFIO module compiled with "CONFIG_VFIO_NOIOMMU=y" by default,
> > that
> > makes things easier
> > for DPDK, in no-iommu environment inserting vfio module with proper
> > parameter makes it
> > available for DPDK.
> 
> Thanks for the update Ferruh.  Also note that the module option is
> dynamically settable so that it can support running with statically
> compiled modules where you may not know whether or not to enable no-
> iommu until after boot, or where unloading and re-loading a module
> might not be an option.  It will be up to each distro to decide
> whether
> to enable the config option, but I think we at least highlight the
> existing support issue for non-iommu protected userspace drivers,
> which
> is something that was not at all clear with the uio driver approach.


Hi,

I've re-posted the unified patch upstream and it should start showing
up in the next linux-next build.  I expect the dpdk code won't be
merged until after this gets back into a proper kernel, but could we
get the dpdk modifications posted as rfc for others looking to try it?
 Thanks,

Alex