Re: [Qemu-devel] [PATCH v3 3/3] IOMMU: Integrate between VFIO and vIOMMU to support device assignment

[Alex Williamson]

On Sat, 28 May 2016 16:10:55 +0000
"Aviv B.D." <address@hidden> wrote:

> On Sat, May 28, 2016 at 7:02 PM Alex Williamson <address@hidden>
> wrote:
> 
> > On Sat, 28 May 2016 10:52:58 +0000
> > "Aviv B.D." <address@hidden> wrote:
> >  
> > > Hi,
> > > Your idea to search the relevent VTDAddressSpace and call it's notifier
> > > will
> > > probably work. Next week I'll try to implement it (for now with the  
> > costly  
> > > scan
> > > of each context).  
> >
> > I think an optimization we can make is to use pci_for_each_bus() and
> > pci_for_each_device() to scan only context entries where devices are
> > present.  Then for each context entry, retrieve the DID, if it matches
> > the invalidation domain_id, retrieve the VTDAddressSpace and perform a
> > memory_region_notify_iommu() using VTDAddressSpace.iommu.  Still
> > horribly inefficient, but an improvement over walking all context
> > entries and avoids gratuitous callbacks between unrelated drivers in
> > QEMU.
> >  
> 
> Thanks for the references on how I can do it. :)
> 
> >
> > Overall, I have very little faith that this will be the only change
> > required to make this work though.  For instance, if a device is added
> > or removed from a domain, where is that accounted for?  Ideally this
> > should trigger the region_add/region_del listener callbacks, but I
> > don't see how that works with how VT-d creates a fixed VTDAddressSpace
> > per device, and in fact how our QEMU memory model doesn't allow the
> > address space of a device to be dynamically aliased against other
> > address spaces or really changed at all.
> >  
> > > I still not sure if populating the MemoryRegion will suffice for hot plug
> > > vfio
> > > device but i'll try to look into it.
> > >
> > > As far as I understand the memory_region_iommu_replay function, it still
> > > scans
> > > the whole 64bit address space, and therefore may hang the VM for a long
> > > time.  
> >
> > Then we need to fix that problem, one option might be to make a replay
> > callback on MemoryRegionIOMMUOps that walks the page tables for a given
> > context entry rather than blindly traversing a 64bit address space.  We
> > can't simply ignore the issue by #ifdef'ing out the code.  I suspect
> > there's a lot more involved to make VT-d interact properly with a
> > physical device than what's been proposed so far.  At every
> > invalidation, we need to figure out what's changed and update the host
> > mappings.  We also need better, more dynamic address space management
> > to make the virtual hardware reflect physical hardware when we enable
> > things like passthrough mode or have multiple devices sharing an iommu
> > domain.  I think we're just barely scratching the surface here.  Thanks,
> >
> > Alex
> >  
> 
> 
> I agree with you regarding hotplug, therefore I only ifdef this code out
> and didn't
> delete it. With the call to memory_region_iommu_replay QEMU hangs on startup
> with a very long loop that prevent any device assignment  with vIOMMU
> enabled.
> 
> I'm hoping not to enlarge the scope of this patch to include hotplug device
> assignment
> with iommu enabled.

It's not just hotplug, any case where an existing domain can be applied
to a device.  The series is incomplete without such support and I won't
accept any changes into vfio that disables code that's correct in other
contexts.  Thanks,

Alex