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

[David Gibson]

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.

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