Re: [Qemu-devel] [PATCH 3/4] vfio: Move container list to iommu MemoryRegion

[David Gibson]

On Mon, Apr 29, 2013 at 01:38:06PM +0200, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Il 29/04/2013 13:00, David Gibson ha scritto:
> > On Mon, Apr 29, 2013 at 10:11:06AM +0200, Paolo Bonzini wrote:
> >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> >> 
> >> Il 28/04/2013 03:58, David Gibson ha scritto:
> >>>>> Ok, knowing about changes that happen in the IOMMU mapping
> >>>>> is indeed out of scope of MemoryListeners.  What about
> >>>>> adding a NotifierList? Then VFIO can register a notifier
> >>>>> and use it to learn about "events" in the IOMMU mapping.
> >>>>> The notifier data can be a MemoryRegionSection or
> >>>>> IOMMUTableEntry, whatever you find more convenient.
> >>> For the generic case a Notifier could work in principle.
> >>> Neither of those structures is suitable as the data though:
> >>> constructing a MemoryRegionSection for every page we map into
> >>> the IOTLB is far too heavyweight, and the IOMMUTLBEntry doesn't
> >>> contain the IOVA.
> >> 
> >> It did in Avi's patch set.  I removed it because it was unused.
> >> I can add it back if you need it.
> > 
> > Ok, I think I will need it.
> 
> Let me know. :)
> 
> >>> Thinking over, I think what that mostly amounts to, is that if
> >>> the VFIO aspects of the address space are already wired up by
> >>> the host bridge, then the individual vfio-pci devices need a
> >>> way of going from their qemu iommu address space (which they
> >>> get from pci_dev->iommu) to the vfio specific information about
> >>> that address space.
> >> 
> >> That can be done with just a hash table, no?
> > 
> > Well, yes it can.  But I think that having a whole parallel lookup 
> > structure within vfio is a worse ugliness than having a single
> > opaque vfio pointer in the MemoryRegion strcuture.
> 
> Why should VFIO be any special in this?  It is reassuring to me that
> the VFIO maintainer thinks the same. :)

Because device passthrough is a sufficiently special case, IMO.  It
introduces requirements that are fundamentally different from any
emulated device.

> > It does also require making sure the lifetime handling is correct. 
> > The entry from the hash table must be removed before the
> > corresponding MemoryRegion is free()ed; otherwise we could end up
> > using the same pointer for a newly constructed MemoryRegion, and
> > get a false lookup in the hash.  Whether that happens essentially
> > never, or almost immediately in practice depends on the malloc()
> > implementation, of course.
> 
> There is only one MemoryRegion per PCI host bridge, and the PCI host

That's true for existing examples, but it need not be true.  For
example the Intel IOMMU supports multiple "iommu domains" which are
different address spaces on the same host bridge.  When one of those
domains is reconfigured, we will again need to call into vfio by some
mechanism to readjust the host side iommu configuration accordingly.

> bridge cannot disappear before the VFIO devices on it are torn down.
> So the lifetime should not be a problem.

I think it is very risky to assume that existing constraints control
the lifetime for us, since we don't know what variety of iommus we may
have in future.  I really think we should have an explicit hook which
allows us to call vfio side cleanup code when a guest side iommu
region is destroyed.  Personally, I still think a special-purpose vfio
hook is the simplest way to do that, but a more general use Notifier
list or something in the right place could do the job too.

I do realise why we don't really want vfio specific hooks in the
generic code.  But we absolutely do need some kind of hooks which vfio
can use, and until we have a second potential user of those hooks, I
think general hooks are overengineering.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: Digital signature