From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40088) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c70g3-0008Ao-Im for qemu-devel@nongnu.org; Wed, 16 Nov 2016 08:55:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c70fy-0004SH-NI for qemu-devel@nongnu.org; Wed, 16 Nov 2016 08:55:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33288) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c70fy-0004Ru-Fa for qemu-devel@nongnu.org; Wed, 16 Nov 2016 08:54:58 -0500 Date: Wed, 16 Nov 2016 15:54:56 +0200 From: "Michael S. Tsirkin" Message-ID: <20161116153420-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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161110124447.1a88ac93@t450s.home> 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: Alex Williamson Cc: "Aviv B.D" , Jan Kiszka , qemu-devel@nongnu.org, Peter Xu 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 wrote: > > > 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 wrote: > > > > > > > From: "Aviv Ben-David" > > > > > > > > > > > > > > * Advertize Cache Mode capability in iommu cap register. > > > > > > > This capability is controlled by "cache-mode" property of intel-iommu device. > > > > > > > To enable this option call QEMU with "-device intel-iommu,cache-mode=true". > > > > > > > > > > > > > > * On page cache invalidation in intel vIOMMU, check if the domain belong to > > > > > > > registered 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 existing 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 filtering > > > > > 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 implemented. > > > > 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. > > > > > > Without those, this is > > > > > effectively just an RFC. > > > > > > > > It's infrastructure without users so it doesn't break things, > > > > 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. We currently prevent 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. We would at least need > something like a replay callback for VT-d that triggers an abort if you > still want to accept it incomplete. Thanks, > > Alex IIUC practically things seems to work, right? So how about disabling by default with a flag for people that want to experiment with it? E.g. x-vfio-allow-broken-translations ? I would like to help this make progress such that 1. Aviv gets the credit he did so far and 2. more people can join development and help complete it. > > > > The patchset spent out of tree too long and I'd like to see > > > > us make progress towards device assignment working with > > > > vIOMMU sooner rather than later, so if it's broken I won't > > > > merge it but if it's incomplete I will. > > > > > > So long as it's incomplete and still prevents vfio usage, I'm ok with > > > merging it, but I don't want to enable vfio usage until it's complete. > > > Thanks, > > > > > > Alex > > > > > > > > > > Currently this patch still doesn't enabling VFIO devices support with vIOMMU > > > > > > > present. Current problems: > > > > > > > * vfio_iommu_map_notify is not aware about memory range belong to specific > > > > > > > VFIOGuestIOMMU. > > > > > > > * memory_region_iommu_replay hangs QEMU on start up while it itterate over > > > > > > > 64bit address space. Commenting out the call to this function enables > > > > > > > workable VFIO device while vIOMMU present. > > > > > > > * vfio_iommu_map_notify should check if address space range is suitable for > > > > > > > current notifier. > > > > > > > > > > > > > > Changes from v1 to v2: > > > > > > > * remove assumption that the cache do not clears > > > > > > > * fix lockup on high load. > > > > > > > > > > > > > > Changes from v2 to v3: > > > > > > > * remove debug leftovers > > > > > > > * split to sepearate commits > > > > > > > * change is_write to flags in vtd_do_iommu_translate, add IOMMU_NO_FAIL > > > > > > > to suppress error propagating to guest. > > > > > > > > > > > > > > Changes from v3 to v4: > > > > > > > * Add property to intel_iommu device to control the CM capability, > > > > > > > default to False. > > > > > > > * Use s->iommu_ops.notify_flag_changed to register notifiers. > > > > > > > > > > > > > > Changes from v4 to v4 RESEND: > > > > > > > * Fix codding style pointed by checkpatch.pl script. > > > > > > > > > > > > > > Changes from v4 to v5: > > > > > > > * Reduce the number of changes in patch 2 and make flags real bitfield. > > > > > > > * Revert deleted debug prints. > > > > > > > * Fix memory leak in patch 3. > > > > > > > > > > > > > > Changes from v5 to v6: > > > > > > > * fix prototype of iommu_translate function for more IOMMU types. > > > > > > > * VFIO will be notified only on the difference, without unmap > > > > > > > before change to maps. > > > > > > > > > > > > > > Aviv Ben-David (3): > > > > > > > IOMMU: add option to enable VTD_CAP_CM to vIOMMU capility exposoed to > > > > > > > guest > > > > > > > IOMMU: change iommu_op->translate's is_write to flags, add support to > > > > > > > NO_FAIL flag mode > > > > > > > IOMMU: enable intel_iommu map and unmap notifiers > > > > > > > > > > > > > > exec.c | 3 +- > > > > > > > hw/alpha/typhoon.c | 2 +- > > > > > > > hw/i386/amd_iommu.c | 4 +- > > > > > > > hw/i386/intel_iommu.c | 160 +++++++++++++++++++++++++++++++++-------- > > > > > > > hw/i386/intel_iommu_internal.h | 3 + > > > > > > > hw/pci-host/apb.c | 2 +- > > > > > > > hw/ppc/spapr_iommu.c | 2 +- > > > > > > > hw/s390x/s390-pci-bus.c | 2 +- > > > > > > > include/exec/memory.h | 6 +- > > > > > > > include/hw/i386/intel_iommu.h | 11 +++ > > > > > > > memory.c | 3 +- > > > > > > > 11 files changed, 160 insertions(+), 38 deletions(-) > > > > > > > > > > > > > > -- > > > > > > > 1.9.1 > > > > > >