Re: [Qemu-devel] [PATCH v6 0/3] IOMMU: intel_iommu support map and unmap notifications

[Michael S. Tsirkin]

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" <address@hidden> wrote:
> 
> > On Tue, Nov 08, 2016 at 01:04:21PM +0200, Aviv B.D wrote:
> > > From: "Aviv Ben-David" <address@hidden>
> > > 
> > > * 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.


> 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.

>  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.

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.

>  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  
> >