From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <alex.williamson@redhat.com> Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 89E7D2A5F for <dev@dpdk.org>; Tue, 22 Dec 2015 21:20:10 +0100 (CET) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id B4520C00126D; Tue, 22 Dec 2015 20:20:09 +0000 (UTC) Received: from t450s.home (ovpn-113-28.phx2.redhat.com [10.3.113.28]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tBMKK9kc023286; Tue, 22 Dec 2015 15:20:09 -0500 Message-ID: <1450815609.2950.8.camel@redhat.com> From: Alex Williamson <alex.williamson@redhat.com> To: "Yigit, Ferruh" <ferruh.yigit@intel.com> Date: Tue, 22 Dec 2015 13:20:09 -0700 In-Reply-To: <1450725743.3781.56.camel@redhat.com> References: <1450198398.6042.32.camel@redhat.com> <20151216040408.GA18363@sivlogin002.ir.intel.com> <1450240711.2674.11.camel@redhat.com> <C6ECDF3AB251BE4894318F4E45123697820B296D@IRSMSX109.ger.corp.intel.com> <1450285912.2674.22.camel@redhat.com> <C6ECDF3AB251BE4894318F4E45123697820B2B8B@IRSMSX109.ger.corp.intel.com> <1450370639.2674.93.camel@redhat.com> <20151218104310.GA11371@sivlogin002.ir.intel.com> <1450449520.2674.162.camel@redhat.com> <1450475417.2674.167.camel@redhat.com> <20151221114643.GA30129@sivlogin002.ir.intel.com> <1450725743.3781.56.camel@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 Cc: "dev@dpdk.org" <dev@dpdk.org> Subject: Re: [dpdk-dev] VFIO no-iommu X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Tue, 22 Dec 2015 20:20:10 -0000 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