From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54484) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c76Kt-0005Pa-VX for qemu-devel@nongnu.org; Wed, 16 Nov 2016 14:57:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c76Kr-0000nL-A1 for qemu-devel@nongnu.org; Wed, 16 Nov 2016 14:57:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53494) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c76Kr-0000nD-2t for qemu-devel@nongnu.org; Wed, 16 Nov 2016 14:57:33 -0500 Date: Wed, 16 Nov 2016 21:57:30 +0200 From: "Michael S. Tsirkin" Message-ID: <20161116215519-mutt-send-email-mst@kernel.org> References: <1478603064-32562-1-git-send-email-bd.aviv@gmail.com> <20161110171338-mutt-send-email-mst@kernel.org> <20161110083021.77ed931a@t450s.home> <20161110175107-mutt-send-email-mst@kernel.org> <20161110090413.62d7daab@t450s.home> <20161110211742-mutt-send-email-mst@kernel.org> <20161110124447.1a88ac93@t450s.home> <20161116153420-mutt-send-email-mst@kernel.org> <20161116083442.5972fa38@t450s.home> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 0/3] IOMMU: intel_iommu support map and unmap notifications List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Aviv B.D." Cc: Alex Williamson , Jan Kiszka , qemu-devel@nongnu.org, Peter Xu On Wed, Nov 16, 2016 at 09:50:46PM +0200, Aviv B.D. wrote: >=20 >=20 > On Wed, Nov 16, 2016 at 5:34 PM, Alex Williamson > wrote: >=20 > On Wed, 16 Nov 2016 15:54:56 +0200 > "Michael S. Tsirkin" wrote: >=20 > > On Thu, Nov 10, 2016 at 12:44:47PM -0700, Alex Williamson wrote: > > > On Thu, 10 Nov 2016 21:20:36 +0200 > > > "Michael S. Tsirkin" wrote: > > > > > > > On Thu, Nov 10, 2016 at 09:04:13AM -0700, Alex Williamson wro= te: > > > > > On Thu, 10 Nov 2016 17:54:35 +0200 > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > On Thu, Nov 10, 2016 at 08:30:21AM -0700, Alex Williamson= wrote: > > > > > > > On Thu, 10 Nov 2016 17:14:24 +0200 > > > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > > > > > On Tue, Nov 08, 2016 at 01:04:21PM +0200, Aviv B.D wr= ote: > > > > > > > > > From: "Aviv Ben-David" > > > > > > > > > > > > > > > > > > * Advertize Cache Mode capability in iommu cap regi= ster. > > > > > > > > >=A0 =A0This capability is controlled by "cache-mode"= property of > intel-iommu device. > > > > > > > > >=A0 =A0To enable this option call QEMU with "-device > intel-iommu,cache-mode=3Dtrue". > > > > > > > > > > > > > > > > > > * On page cache invalidation in intel vIOMMU, check= if the > domain belong to > > > > > > > > >=A0 =A0registered notifier, and notify accordingly. > > > > > > > > > > > > > > > > This looks sane I think. Alex, care to comment? > > > > > > > > Merging will have to wait until after the release. > > > > > > > > Pls remember to re-test and re-ping then. > > > > > > > > > > > > > > I don't think it's suitable for upstream until there's = a > reasonable > > > > > > > replay mechanism > > > > > > > > > > > > Could you pls clarify what do you mean by replay? > > > > > > Is this when you attach a device by hotplug to > > > > > > a running system? > > > > > > > > > > > > If yes this can maybe be addressed by disabling hotplug > temporarily. > > > > > > > > > > No, hotplug is not required, moving a device between existi= ng > domains > > > > > requires replay, ie. actually using it for nested device > assignment. > > > > > > > > Good point, that one is a correctness thing. Aviv, > > > > could you add this in TODO list in a cover letter pls? > > > > > > > > > > > and we straighten out whether it's expected to get > > > > > > > multiple notifies and the notif-ee is responsible for f= iltering > > > > > > > them or if the notif-er should do filtering. > > > > > > > > > > > > OK this is a documentation thing. > > > > > > > > > > Well no, it needs to be decided and if necessary implemente= d. > > > > > > > > Let's assume it's the notif-ee for now. Less is more and all = that. > > > > > > I think this is opposite of the approach dwg suggested. > > > > > > > > > >=A0 Without those, this is > > > > > > > effectively just an RFC. > > > > > > > > > > > > It's infrastructure without users so it doesn't break thi= ngs, > > > > > > I'm more interested in seeing whether it's broken in > > > > > > some way than whether it's complete. > > > > > > > > > > If it allows use with vfio but doesn't fully implement the = complete > set > > > > > of interfaces, it does break things.=A0 We currently preven= t viommu > usage > > > > > with vfio because it is incomplete. > > > > > > > > Right - that bit is still in as far as I can see. > > > > > > Nope, 3/3 changes vtd_iommu_notify_flag_changed() to allow use = with > > > vfio even though it's still incomplete.=A0 We would at least ne= ed > > > something like a replay callback for VT-d that triggers an abor= t if you > > > still want to accept it incomplete.=A0 Thanks, > > > > > > Alex > > > > IIUC practically things seems to work, right? >=20 > AFAIK, no. > =20 > > So how about disabling by default with a flag for people that wan= t to > > experiment with it? > > E.g. x-vfio-allow-broken-translations ? >=20 > We've already been through one round of "intel-iommu is incomplete = for > use with device assignment, how can we prevent it from being used", > which led to the notify_flag_changed callback on MemoryRegionIOMMUO= ps. > This series now claims to fix that yet still doesn't provide a > mechanism to do memory_region_iommu_replay() given that VT-d has a = much > larger address width.=A0 Why is the onus on vfio to resolve this or > provide some sort of workaround?=A0 vfio is using the QEMU iommu > interface correctly, intel-iommu is still incomplete. The least it > could do is add an optional replay callback to MemoryRegionIOMMUOps > that supersedes the existing memory_region_iommu_replay() code and > triggers an abort when it gets called.=A0 I don't know what an > x-vfio-allow-broken-translations option would do, how I'd implement= it, > or why I'd bother to implement it.=A0 Thanks, >=20 > Alex >=20 >=20 > I'll implement your suggestion regarding the replay framwork.=A0 > Probably in another patch set, if it is OK? >=20 > Thanks, > Aviv.=A0 >=20 Alex points out that with 3/3 setting cache_mode=3D1 will enable VFIO device assignment. Question would be, does it actually work for you in this mode? If not it seems important not to enable it for users. --=20 MST