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

[David Gibson]

On Tue, Apr 30, 2013 at 12:05:39PM +1000, David Gibson wrote:
> On Mon, Apr 29, 2013 at 03:44:43PM +0200, Paolo Bonzini wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > Il 29/04/2013 13:56, David Gibson ha scritto:
> > >> 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 think this is a different problem.  Basically the question is "what
> > happens if a MemoryRegion 'disappears' while an AddressSpace is still
> > referring to it", and the answer right now is "badness".
> 
> Well.. no.  The same problem may well exist for AddressSpace objects,
> but in this case it's for the VFIO private per-address-space object.
> 
> > We should look at generic fixes before dropping hooks in the code.  At
> > the very least an "assert(mr->parent == NULL);" is missing in
> > memory_region_destroy.
> 
> Well, sure that's probably also a good idea.  But the whole point here
> is you're insisting that the MemoryRegion code doesn't know about the
> vfio private data, even as an opaque handle, and so there's no
> possible assert we can put there to check it has been destroyed.

Oh, yes, forgot to ask.  I'm still unclear on what the conceptual
difference is supposed to between a MemoryRegion and an AddressSpace.
AFAICT AddressSpace seems to be roughly just a wrapper on a
MemoryRegion that gives it some more features.

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