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

[Aviv B.D.]

Hi,
As far as I tested the disabled code (call to memory_region_iommu_replay)
hangup
QEMU on startup if IOMMU is enabled (scaning 64 bit address space takes
more
than an hour on modern hardware) , at least on x86 hardware. So the code is
not 100%
correct for any context. Maybe it just should be disabled for x86
architecture?

By specification any such behavior of applying a domain to device should
include
cache invalidation if CM flag is present so I'm not thinking that my patch
break
this scenario.

Thanks,
Aviv.

On Sat, May 28, 2016 at 8:39 PM Alex Williamson <address@hidden>
wrote:

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