On Wed, 16 Nov 2016 21:50:46 +0200 "Aviv B.D." <bd.a...@gmail.com> wrote:
> On Wed, Nov 16, 2016 at 5:34 PM, Alex Williamson <alex.william...@redhat.com > > wrote: > > > On Wed, 16 Nov 2016 15:54:56 +0200 > > "Michael S. Tsirkin" <m...@redhat.com> wrote: > > > > > 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" <m...@redhat.com> 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" <m...@redhat.com> 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" <m...@redhat.com> wrote: > > > > > > > > > > > > > > > > > On Tue, Nov 08, 2016 at 01:04:21PM +0200, Aviv B.D wrote: > > > > > > > > > > From: "Aviv Ben-David" <bd.a...@gmail.com> > > > > > > > > > > > > > > > > > > > > * 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? > > > > AFAIK, no. > > > > > So how about disabling by default with a flag for people that want to > > > experiment with it? > > > E.g. x-vfio-allow-broken-translations ? > > > > 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 MemoryRegionIOMMUOps. > > 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. Why is the onus on vfio to resolve this or > > provide some sort of workaround? 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. 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. Thanks, > > > > Alex > > > > I'll implement your suggestion regarding the replay framwork. > Probably in another patch set, if it is OK? I think it needs to be committed in the same series that enables the notifier_flag_changed callback, otherwise we're opening the door to make use of vfio devices when we know that support is incomplete. QEMU is in hard freeze anyway, this has missed 2.8. I would absolutely object to including this in 2.8 without the replay provision. Thanks, Alex
