On 7 May 2013 15:16, Paolo Bonzini <pbonz...@redhat.com> wrote:
> From: Avi Kivity <avi.kiv...@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.kiv...@gmail.com>
> [Modified to put translation in address_space_translate - Paolo]
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> exec.c | 35 +++++++++++++++++++++++++++++------
> include/exec/memory.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
> memory.c | 21 +++++++++++++++++++++
> 3 files changed, 94 insertions(+), 6 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 6efad6d..7dab2fa 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -208,15 +208,38 @@ MemoryRegionSection
> *address_space_translate(AddressSpace *as, hwaddr addr,
> hwaddr *xlat, hwaddr *plen,
> bool is_write)
> {
> + IOMMUTLBEntry iotlb;
> MemoryRegionSection *section;
> + hwaddr len = *plen;
>
> - section = address_space_lookup_region(as, addr);
> - /* Compute offset with MemoryRegionSection */
> - addr -= section->offset_within_address_space;
> - *plen = MIN(section->size - addr, *plen);
> + for (;;) {
> + section = address_space_lookup_region(as, addr);
> +
> + /* Compute offset with MemoryRegionSection */
> + addr -= section->offset_within_address_space;
> + len = MIN(section->size - addr, len);
> +
> + /* Compute offset with MemoryRegion */
> + addr += section->offset_within_region;
> +
> + if (!section->mr->iommu_ops) {
> + break;
> + }
> +
> + iotlb = section->mr->iommu_ops->translate(section->mr, addr);
> + addr = ((iotlb.translated_addr & ~iotlb.addr_mask)
> + | (addr & iotlb.addr_mask));
> + len = MIN(len, (addr | iotlb.addr_mask) - addr + 1);
> + if (!iotlb.perm[is_write]) {
> + section = &phys_sections[phys_section_unassigned];
Why is "unwritable" handled the same as "no memory"? They are
likely to result in different fault information, surely?
> + break;
> + }
> +
> + as = section->mr->iommu_target_as;
> + }
>
> - /* Compute offset with MemoryRegion */
> - *xlat = addr + section->offset_within_region;
> + *plen = len;
> + *xlat = addr;
> return section;
> }
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 914f5d4..e05296b 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -112,12 +112,27 @@ struct MemoryRegionOps {
> const MemoryRegionMmio old_mmio;
> };
>
> +typedef struct IOMMUTLBEntry IOMMUTLBEntry;
> +typedef struct MemoryRegionIOMMUOps MemoryRegionIOMMUOps;
> +
> +struct IOMMUTLBEntry {
> + hwaddr translated_addr;
> + hwaddr addr_mask; /* 0xfff = 4k translation */
> + bool perm[2]; /* permissions, [0] for read, [1] for write */
> +};
This could use a proper doc comment.
> +
> +struct MemoryRegionIOMMUOps {
> + /* Returns a TLB entry that contains a given address. */
> + IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr);
> +};
> +
> typedef struct CoalescedMemoryRange CoalescedMemoryRange;
> typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>
> struct MemoryRegion {
> /* All fields are private - violators will be prosecuted */
> const MemoryRegionOps *ops;
> + const MemoryRegionIOMMUOps *iommu_ops;
> void *opaque;
> MemoryRegion *parent;
> Int128 size;
> @@ -144,6 +159,7 @@ struct MemoryRegion {
> uint8_t dirty_log_mask;
> unsigned ioeventfd_nb;
> MemoryRegionIoeventfd *ioeventfds;
> + struct AddressSpace *iommu_target_as;
> };
>
> struct MemoryRegionPortio {
> @@ -329,6 +345,25 @@ void memory_region_init_rom_device(MemoryRegion *mr,
> void memory_region_init_reservation(MemoryRegion *mr,
> const char *name,
> uint64_t size);
> +
> +/**
> + * memory_region_init_iommu: Initialize a memory region that translates
> addresses
> + *
> + * An IOMMU region translates addresses and forwards accesses to a target
> memory region.
> + *
> + * @mr: the #MemoryRegion to be initialized
> + * @ops: a function that translates addresses into the @target region
This is an ops pointer, not a function pointer.
> + * @target_as: the #AddressSpace that will be used to satisfy accesses to
> translated
> + * addresses
> + * @name: used for debugging; not visible to the user or ABI
> + * @size: size of the region.
> + */
> +void memory_region_init_iommu(MemoryRegion *mr,
> + MemoryRegionIOMMUOps *ops,
> + AddressSpace *target_as,
> + const char *name,
> + uint64_t size);
> +
> /**
> * memory_region_destroy: Destroy a memory region and reclaim all resources.
> *
> @@ -368,6 +403,15 @@ static inline bool memory_region_is_romd(MemoryRegion
> *mr)
> }
>
> /**
> + * memory_region_is_iommu: check whether a memory region is an iommu
> + *
> + * Returns %true is a memory region is an iommu.
"%true if this memory region is an iommu"
(should we be consistently capitalising IOMMU?)
> + *
> + * @mr: the memory region being queried
> + */
> +bool memory_region_is_iommu(MemoryRegion *mr);
> +
> +/**
> * memory_region_name: get a memory region's name
> *
> * Returns the string that was used to initialize the memory region.
> diff --git a/memory.c b/memory.c
> index a8929aa..b0d5e33 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -787,6 +787,7 @@ void memory_region_init(MemoryRegion *mr,
> uint64_t size)
> {
> mr->ops = NULL;
> + mr->iommu_ops = NULL;
> mr->parent = NULL;
> mr->size = int128_make64(size);
> if (size == UINT64_MAX) {
> @@ -978,6 +979,21 @@ void memory_region_init_rom_device(MemoryRegion *mr,
> mr->ram_addr = qemu_ram_alloc(size, mr);
> }
>
> +void memory_region_init_iommu(MemoryRegion *mr,
> + MemoryRegionIOMMUOps *ops,
> + AddressSpace *target_as,
> + 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_none;
> + mr->iommu_target_as = target_as;
> +}
> +
> static uint64_t invalid_read(void *opaque, hwaddr addr,
> unsigned size)
> {
> @@ -1052,6 +1068,11 @@ bool memory_region_is_rom(MemoryRegion *mr)
> return mr->ram && mr->readonly;
> }
>
> +bool memory_region_is_iommu(MemoryRegion *mr)
> +{
> + return mr->iommu_ops;
> +}
> +
> void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client)
> {
> uint8_t mask = 1 << client;
> --
> 1.7.1
>
thanks
-- PMM
