Re: [Qemu-devel] [PATCH 09/17] memory: iommu support

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2594 bytes --]

On Wed, May 01, 2013 at 06:10:47PM +0200, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Il 01/05/2013 06:35, David Gibson ha scritto:
> >> From: Avi Kivity <avi.kivity@gmail.com>
> >> 
> >> Add a new memory region type that translates addresses it is
> >> given, then forwards them to a target address space.  This is
> >> similar to an alias, except that the mapping is more flexible
> >> than a linear translation and trucation, and also less efficient
> >> since the translation happens at runtime.
> >> 
> >> The implementation uses an AddressSpace mapping the target region
> >> to avoid hierarchical dispatch all the way to the resolved
> >> region; only iommu regions are looked up dynamically.
> >> 
> >> Signed-off-by: Avi Kivity <avi.kivity@gmail.com> [Modified to put
> >> translation in address_space_translate - Paolo] Signed-off-by:
> >> Paolo Bonzini <pbonzini@redhat.com> --- exec.c                |
> >> 35 +++++++++++++++++++++++++++++------ include/exec/memory.h |
> >> 44 ++++++++++++++++++++++++++++++++++++++++++++ memory.c
> >> |   28 ++++++++++++++++++++++++++++ 3 files changed, 101
> >> insertions(+), 6 deletions(-)
> > 
> > [snip]
> >> +void memory_region_init_iommu(MemoryRegion *mr, +
> >> MemoryRegionIOMMUOps *ops, +
> >> MemoryRegion *target, +                              const char
> >> *name, +                              uint64_t size) +{ +
> >> memory_region_init(mr, name, size); +    mr->ops = NULL; +
> >> mr->iommu_ops = ops, +    mr->opaque = mr; +    mr->terminates =
> >> true;  /* then re-forwards */ +    mr->destructor =
> >> memory_region_destructor_iommu; +    mr->iommu_target_as =
> >> g_new(AddressSpace, 1); +
> >> address_space_init(mr->iommu_target_as, target);
> > 
> > Since IOMMUs are very likely to share a target AS (in fact, it
> > will nearly always be system memory), it seems odd to me to
> > construct new AddressSpace objects for each one, rather than just
> > giving the AddressSpace as the parameter to
> > memory_region_init_iommu.
> > 
> 
> I think the problem is that we do not have reference counting, and
> this makes it simpler to manage the lifetime.  It can be changed later.

I don't really follow this logic.  In the existing case, the iommu
target is always system memory, and there's already
address_space_memory which always exists.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]