Re: [Qemu-devel] [PATCH 14/40] memory: iommu support

[Peter Maydell]

On 7 May 2013 15:16, Paolo Bonzini <address@hidden> wrote:
> From: Avi Kivity <address@hidden>
>
> 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 <address@hidden>
> [Modified to put translation in address_space_translate - Paolo]
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  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