Patchwork [12/40] memory: add address_space_translate

login
register
mail settings
Submitter Paolo Bonzini
Date May 7, 2013, 2:16 p.m.
Message ID <1367936238-12196-13-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/242352/
State New
Headers show

Comments

Paolo Bonzini - May 7, 2013, 2:16 p.m.
Using phys_page_find to translate an AddressSpace to a MemoryRegionSection
is unwieldy.  It requires to pass the page index rather than the address,
and later memory_region_section_addr has to be called.  Replace
memory_region_section_addr with a function that does all of it: call
phys_page_find, compute the offset within the region, and check how
big the current mapping is.  This way, a large flat region can be written
with a single lookup rather than a page at a time.

address_space_translate will also provide a single point where IOMMU
forwarding is implemented.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cputlb.c              |   20 +++---
 exec.c                |  184 ++++++++++++++++++++++++++----------------------
 include/exec/cputlb.h |   12 ++--
 include/exec/memory.h |   31 ++++-----
 translate-all.c       |    6 +-
 5 files changed, 133 insertions(+), 120 deletions(-)
Peter Maydell - May 7, 2013, 6:08 p.m.
On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Using phys_page_find to translate an AddressSpace to a MemoryRegionSection
> is unwieldy.  It requires to pass the page index rather than the address,
> and later memory_region_section_addr has to be called.  Replace
> memory_region_section_addr with a function that does all of it: call
> phys_page_find, compute the offset within the region, and check how
> big the current mapping is.  This way, a large flat region can be written
> with a single lookup rather than a page at a time.
>
> address_space_translate will also provide a single point where IOMMU
> forwarding is implemented.

This generally looks right, so the below is really all nitpicks.


> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cputlb.c              |   20 +++---
>  exec.c                |  184 ++++++++++++++++++++++++++----------------------
>  include/exec/cputlb.h |   12 ++--
>  include/exec/memory.h |   31 ++++-----
>  translate-all.c       |    6 +-
>  5 files changed, 133 insertions(+), 120 deletions(-)
>
> diff --git a/cputlb.c b/cputlb.c
> index aba7e44..1f85da0 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -248,13 +248,18 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,
>      target_ulong code_address;
>      uintptr_t addend;
>      CPUTLBEntry *te;
> -    hwaddr iotlb;
> +    hwaddr iotlb, xlat, sz;
>
>      assert(size >= TARGET_PAGE_SIZE);
>      if (size != TARGET_PAGE_SIZE) {
>          tlb_add_large_page(env, vaddr, size);
>      }
> -    section = phys_page_find(address_space_memory.dispatch, paddr >> TARGET_PAGE_BITS);
> +
> +    sz = size;
> +    section = address_space_translate(&address_space_memory, paddr, &xlat, &sz,
> +                                      false);
> +    assert(sz >= TARGET_PAGE_SIZE);
> +
>  #if defined(DEBUG_TLB)
>      printf("tlb_set_page: vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
>             " prot=%x idx=%d pd=0x%08lx\n",
> @@ -269,15 +274,14 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,
>      }
>      if (memory_region_is_ram(section->mr) ||
>          memory_region_is_romd(section->mr)) {
> -        addend = (uintptr_t)memory_region_get_ram_ptr(section->mr)
> -        + memory_region_section_addr(section, paddr);
> +        addend = (uintptr_t)memory_region_get_ram_ptr(section->mr) + xlat;
>      } else {
>          addend = 0;
>      }
>
>      code_address = address;
> -    iotlb = memory_region_section_get_iotlb(env, section, vaddr, paddr, prot,
> -                                            &address);
> +    iotlb = memory_region_section_get_iotlb(env, section, vaddr, paddr, xlat,
> +                                            prot, &address);
>
>      index = (vaddr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
>      env->iotlb[mmu_idx][index] = iotlb - vaddr;
> @@ -300,9 +304,7 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,
>              /* Write access calls the I/O callback.  */
>              te->addr_write = address | TLB_MMIO;
>          } else if (memory_region_is_ram(section->mr)
> -                   && !cpu_physical_memory_is_dirty(
> -                           section->mr->ram_addr
> -                           + memory_region_section_addr(section, paddr))) {
> +                   && !cpu_physical_memory_is_dirty(section->mr->ram_addr + xlat)) {
>              te->addr_write = address | TLB_NOTDIRTY;
>          } else {
>              te->addr_write = address;
> diff --git a/exec.c b/exec.c
> index 405de9f..9709bc4 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -182,24 +182,36 @@ static void phys_page_set(AddressSpaceDispatch *d,
>      phys_page_set_level(&d->phys_map, &index, &nb, leaf, P_L2_LEVELS - 1);
>  }
>
> -MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index)
> +static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index)
>  {
>      PhysPageEntry lp = d->phys_map;
>      PhysPageEntry *p;
>      int i;
> -    uint16_t s_index = phys_section_unassigned;
>
>      for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) {
>          if (lp.ptr == PHYS_MAP_NODE_NIL) {
> -            goto not_found;
> +            return &phys_sections[phys_section_unassigned];
>          }
>          p = phys_map_nodes[lp.ptr];
>          lp = p[(index >> (i * L2_BITS)) & (L2_SIZE - 1)];
>      }
> +    return &phys_sections[lp.ptr];
> +}

The changes to this function are just minor refactoring
tweaks, right? (mostly, removing the goto). If they were
a separate patch that would help, especially since diff
has interlaced this hunk with the new function below.

> +
> +MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
> +                                             hwaddr *xlat, hwaddr *plen,
> +                                             bool is_write)
> +{
> +    MemoryRegionSection *section;
> +
> +    section = phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS);
> +    /* Compute offset with MemoryRegionSection */

"within" ?

> +    addr -= section->offset_within_address_space;
> +    *plen = MIN(section->size - addr, *plen);
>
> -    s_index = lp.ptr;
> -not_found:
> -    return &phys_sections[s_index];
> +    /* Compute offset with MemoryRegion */

also "within"

> +    *xlat = addr + section->offset_within_region;
> +    return section;
>  }
>
>  bool memory_region_is_unassigned(MemoryRegion *mr)
> @@ -620,11 +632,11 @@ static int cpu_physical_memory_set_dirty_tracking(int enable)
>  }
>
>  hwaddr memory_region_section_get_iotlb(CPUArchState *env,
> -                                                   MemoryRegionSection *section,
> -                                                   target_ulong vaddr,
> -                                                   hwaddr paddr,
> -                                                   int prot,
> -                                                   target_ulong *address)
> +                                       MemoryRegionSection *section,
> +                                       target_ulong vaddr,
> +                                       hwaddr paddr, hwaddr xlat,
> +                                       int prot,
> +                                       target_ulong *address)
>  {
>      hwaddr iotlb;
>      CPUWatchpoint *wp;
> @@ -632,7 +644,7 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
>      if (memory_region_is_ram(section->mr)) {
>          /* Normal RAM.  */
>          iotlb = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
> -            + memory_region_section_addr(section, paddr);
> +            + xlat;
>          if (!section->readonly) {
>              iotlb |= phys_section_notdirty;
>          } else {
> @@ -640,7 +652,7 @@ hwaddr memory_region_section_get_iotlb(CPUArchState *env,
>          }
>      } else {
>          iotlb = section - phys_sections;
> -        iotlb += memory_region_section_addr(section, paddr);
> +        iotlb += xlat;
>      }
>
>      /* Make accesses to pages with watchpoints go via the
> @@ -1903,24 +1915,18 @@ static void invalidate_and_set_dirty(hwaddr addr,
>  void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                        int len, bool is_write)
>  {
> -    AddressSpaceDispatch *d = as->dispatch;
> -    int l;
> +    hwaddr l;

...maybe the 'len' argument to this function should really be a hwaddr too?

>      uint8_t *ptr;
>      uint32_t val;
> -    hwaddr page;
> +    hwaddr addr1;

(not your fault, but) 'addr1' is a really awful variable name...

>      MemoryRegionSection *section;
>
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> -        if (l > len)
> -            l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        l = len;
> +        section = address_space_translate(as, addr, &addr1, &l, is_write);
>
>          if (is_write) {
>              if (!memory_region_is_ram(section->mr)) {
> -                hwaddr addr1;
> -                addr1 = memory_region_section_addr(section, addr);
>                  /* XXX: could force cpu_single_env to NULL to avoid
>                     potential bugs */
>                  if (l >= 4 && ((addr1 & 3) == 0)) {
> @@ -1940,9 +1946,7 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                      l = 1;
>                  }
>              } else if (!section->readonly) {
> -                ram_addr_t addr1;
> -                addr1 = memory_region_get_ram_addr(section->mr)
> -                    + memory_region_section_addr(section, addr);
> +                addr1 += memory_region_get_ram_addr(section->mr);
>                  /* RAM case */
>                  ptr = qemu_get_ram_ptr(addr1);
>                  memcpy(ptr, buf, l);
> @@ -1952,9 +1956,7 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>          } else {
>              if (!(memory_region_is_ram(section->mr) ||
>                    memory_region_is_romd(section->mr))) {
> -                hwaddr addr1;
>                  /* I/O case */
> -                addr1 = memory_region_section_addr(section, addr);
>                  if (l >= 4 && ((addr1 & 3) == 0)) {
>                      /* 32 bit read access */
>                      val = io_mem_read(section->mr, addr1, 4);
> @@ -1973,9 +1975,7 @@ void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
>                  }
>              } else {
>                  /* RAM case */
> -                ptr = qemu_get_ram_ptr(section->mr->ram_addr
> -                                       + memory_region_section_addr(section,
> -                                                                    addr));
> +                ptr = qemu_get_ram_ptr(section->mr->ram_addr + addr1);
>                  memcpy(buf, ptr, l);
>                  qemu_put_ram_ptr(ptr);
>              }
> @@ -2015,26 +2015,21 @@ void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
>  void cpu_physical_memory_write_rom(hwaddr addr,
>                                     const uint8_t *buf, int len)
>  {
> -    AddressSpaceDispatch *d = address_space_memory.dispatch;
> -    int l;
> +    hwaddr l;
>      uint8_t *ptr;
> -    hwaddr page;
> +    hwaddr addr1;
>      MemoryRegionSection *section;
>
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> -        if (l > len)
> -            l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        l = len;
> +        section = address_space_translate(&address_space_memory,
> +                                          addr, &addr1, &l, true);
>
>          if (!(memory_region_is_ram(section->mr) ||
>                memory_region_is_romd(section->mr))) {
>              /* do nothing */
>          } else {
> -            unsigned long addr1;
> -            addr1 = memory_region_get_ram_addr(section->mr)
> -                + memory_region_section_addr(section, addr);
> +            addr1 += memory_region_get_ram_addr(section->mr);
>              /* ROM/RAM case */
>              ptr = qemu_get_ram_ptr(addr1);
>              memcpy(ptr, buf, l);
> @@ -2095,18 +2090,12 @@ static void cpu_notify_map_clients(void)
>
>  bool address_space_valid(AddressSpace *as, hwaddr addr, int len, bool is_write)
>  {
> -    AddressSpaceDispatch *d = as->dispatch;
>      MemoryRegionSection *section;
> -    int l;
> -    hwaddr page;
> +    hwaddr l, xlat;
>
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> -        if (l > len) {
> -            l = len;
> -        }
> -        section = phys_page_find(d, addr >> TARGET_PAGE_BITS);
> +        l = len;
> +        section = address_space_translate(as, addr, &xlat, &l, is_write);
>          if (section->mr == &io_mem_unassigned) {
>              return false;
>          }
> @@ -2129,22 +2118,17 @@ void *address_space_map(AddressSpace *as,
>                          hwaddr *plen,
>                          bool is_write)
>  {
> -    AddressSpaceDispatch *d = as->dispatch;
>      hwaddr len = *plen;
>      hwaddr todo = 0;
> -    int l;
> -    hwaddr page;
> +    hwaddr l, xlat;
>      MemoryRegionSection *section;
>      ram_addr_t raddr = RAM_ADDR_MAX;
>      ram_addr_t rlen;
>      void *ret;
>
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> -        if (l > len)
> -            l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        l = len;
> +        section = address_space_translate(as, addr, &xlat, &l, is_write);
>
>          if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
>              if (todo || bounce.buffer) {
> @@ -2161,8 +2145,11 @@ void *address_space_map(AddressSpace *as,
>              return bounce.buffer;
>          }
>          if (!todo) {
> -            raddr = memory_region_get_ram_addr(section->mr)
> -                + memory_region_section_addr(section, addr);
> +            raddr = memory_region_get_ram_addr(section->mr) + xlat;
> +        } else {
> +            if (memory_region_get_ram_addr(section->mr) + xlat != raddr + todo) {
> +                break;
> +            }
>          }
>
>          len -= l;
> @@ -2228,13 +2215,17 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
>      uint8_t *ptr;
>      uint32_t val;
>      MemoryRegionSection *section;
> +    hwaddr l = 4;
>
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
> +                                      false);

I find it a little confusing reusing 'addr' for the returned
offset from address_space_translate(); maybe using a different
variable would be clearer? (don't feel very strongly about it)

> +    if (l < 4) {
> +        return -1;
> +    }
>
>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
>          /* I/O case */
> -        addr = memory_region_section_addr(section, addr);
>          val = io_mem_read(section->mr, addr, 4);
>  #if defined(TARGET_WORDS_BIGENDIAN)
>          if (endian == DEVICE_LITTLE_ENDIAN) {
> @@ -2249,7 +2240,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
>          /* RAM case */
>          ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
>                                  & TARGET_PAGE_MASK)
> -                               + memory_region_section_addr(section, addr));
> +                               + addr);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = ldl_le_p(ptr);
> @@ -2287,13 +2278,17 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,
>      uint8_t *ptr;
>      uint64_t val;
>      MemoryRegionSection *section;
> +    hwaddr l = 8;
>
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
> +                                      false);
> +    if (l < 8) {
> +        return -1;
> +    }
>
>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
>          /* I/O case */
> -        addr = memory_region_section_addr(section, addr);
>
>          /* XXX This is broken when device endian != cpu endian.
>                 Fix and add "endian" variable check */
> @@ -2308,7 +2303,7 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,
>          /* RAM case */
>          ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
>                                  & TARGET_PAGE_MASK)
> -                               + memory_region_section_addr(section, addr));
> +                               + addr);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = ldq_le_p(ptr);
> @@ -2354,13 +2349,17 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,
>      uint8_t *ptr;
>      uint64_t val;
>      MemoryRegionSection *section;
> +    hwaddr l = 2;
>
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
> +                                      false);
> +    if (l < 2) {
> +        return -1;
> +    }

This kind of "let's just return -1 if the read failed" kind
of bothers me, because "memory access aborted" is really
completely different from "memory access succeeded and
returned -1". But there are a lot of APIs which we'd need
to fix to properly communicate the problem back to the
caller, so let it slide for now. (What used to happen
in the old code in this case?)

>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
>          /* I/O case */
> -        addr = memory_region_section_addr(section, addr);
>          val = io_mem_read(section->mr, addr, 2);
>  #if defined(TARGET_WORDS_BIGENDIAN)
>          if (endian == DEVICE_LITTLE_ENDIAN) {
> @@ -2375,7 +2374,7 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,
>          /* RAM case */
>          ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
>                                  & TARGET_PAGE_MASK)
> -                               + memory_region_section_addr(section, addr));
> +                               + addr);
>          switch (endian) {
>          case DEVICE_LITTLE_ENDIAN:
>              val = lduw_le_p(ptr);
> @@ -2413,11 +2412,15 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
>  {
>      uint8_t *ptr;
>      MemoryRegionSection *section;
> +    hwaddr l = 2;
>
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
> +                                      true);
> +    if (l < 2) {
> +        return;
> +    }
>
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
> -        addr = memory_region_section_addr(section, addr);
>          if (memory_region_is_ram(section->mr)) {
>              section = &phys_sections[phys_section_rom];
>          }
> @@ -2425,7 +2428,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
>      } else {
>          unsigned long addr1 = (memory_region_get_ram_addr(section->mr)
>                                 & TARGET_PAGE_MASK)
> -            + memory_region_section_addr(section, addr);
> +            + addr;
>          ptr = qemu_get_ram_ptr(addr1);
>          stl_p(ptr, val);
>
> @@ -2445,11 +2448,15 @@ void stq_phys_notdirty(hwaddr addr, uint64_t val)
>  {
>      uint8_t *ptr;
>      MemoryRegionSection *section;
> +    hwaddr l = 4;
>
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
> +                                      true);
> +    if (l < 4) {
> +        return;
> +    }
>
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
> -        addr = memory_region_section_addr(section, addr);
>          if (memory_region_is_ram(section->mr)) {
>              section = &phys_sections[phys_section_rom];
>          }
> @@ -2463,7 +2470,7 @@ void stq_phys_notdirty(hwaddr addr, uint64_t val)
>      } else {
>          ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
>                                  & TARGET_PAGE_MASK)
> -                               + memory_region_section_addr(section, addr));
> +                               + addr);
>          stq_p(ptr, val);
>      }
>  }
> @@ -2474,11 +2481,15 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,
>  {
>      uint8_t *ptr;
>      MemoryRegionSection *section;
> +    hwaddr l = 8;
>
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
> +                                      true);
> +    if (l < 8) {
> +        return;
> +    }
>
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
> -        addr = memory_region_section_addr(section, addr);
>          if (memory_region_is_ram(section->mr)) {
>              section = &phys_sections[phys_section_rom];
>          }
> @@ -2495,7 +2506,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,
>      } else {
>          unsigned long addr1;
>          addr1 = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
> -            + memory_region_section_addr(section, addr);
> +            + addr;
>          /* RAM case */
>          ptr = qemu_get_ram_ptr(addr1);
>          switch (endian) {
> @@ -2541,11 +2552,15 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,
>  {
>      uint8_t *ptr;
>      MemoryRegionSection *section;
> +    hwaddr l = 4;
>
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
> +                                      true);
> +    if (l < 4) {
> +        return;
> +    }
>
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
> -        addr = memory_region_section_addr(section, addr);
>          if (memory_region_is_ram(section->mr)) {
>              section = &phys_sections[phys_section_rom];
>          }
> @@ -2562,7 +2577,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,
>      } else {
>          unsigned long addr1;
>          addr1 = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
> -            + memory_region_section_addr(section, addr);
> +            + addr;
>          /* RAM case */
>          ptr = qemu_get_ram_ptr(addr1);
>          switch (endian) {
> @@ -2666,9 +2681,10 @@ bool virtio_is_big_endian(void)
>  bool cpu_physical_memory_is_io(hwaddr phys_addr)
>  {
>      MemoryRegionSection *section;
> +    hwaddr l = 1;
>
> -    section = phys_page_find(address_space_memory.dispatch,
> -                             phys_addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory,
> +                                      phys_addr, &phys_addr, &l, false);
>
>      return !(memory_region_is_ram(section->mr) ||
>               memory_region_is_romd(section->mr));
> diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
> index 733c885..e821660 100644
> --- a/include/exec/cputlb.h
> +++ b/include/exec/cputlb.h
> @@ -26,8 +26,6 @@ void tlb_unprotect_code_phys(CPUArchState *env, ram_addr_t ram_addr,
>                               target_ulong vaddr);
>  void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry, uintptr_t start,
>                             uintptr_t length);
> -MemoryRegionSection *phys_page_find(struct AddressSpaceDispatch *d,
> -                                    hwaddr index);
>  void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length);
>  void tlb_set_dirty(CPUArchState *env, target_ulong vaddr);
>  extern int tlb_flush_count;
> @@ -35,11 +33,11 @@ extern int tlb_flush_count;
>  /* exec.c */
>  void tb_flush_jmp_cache(CPUArchState *env, target_ulong addr);
>  hwaddr memory_region_section_get_iotlb(CPUArchState *env,
> -                                                   MemoryRegionSection *section,
> -                                                   target_ulong vaddr,
> -                                                   hwaddr paddr,
> -                                                   int prot,
> -                                                   target_ulong *address);
> +                                       MemoryRegionSection *section,
> +                                       target_ulong vaddr,
> +                                       hwaddr paddr, hwaddr xlat,
> +                                       int prot,
> +                                       target_ulong *address);


Any chance of a documentation comment? the purpose of the
arguments to this function was mostly guessable until you
added xlat.

>  bool memory_region_is_unassigned(MemoryRegion *mr);
>
>  #endif
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index c38e974..914f5d4 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -740,23 +740,6 @@ MemoryRegionSection memory_region_find(MemoryRegion *mr,
>                                         hwaddr addr, uint64_t size);
>
>  /**
> - * memory_region_section_addr: get offset within MemoryRegionSection
> - *
> - * Returns offset within MemoryRegionSection
> - *
> - * @section: the memory region section being queried
> - * @addr: address in address space
> - */
> -static inline hwaddr
> -memory_region_section_addr(MemoryRegionSection *section,
> -                           hwaddr addr)
> -{
> -    addr -= section->offset_within_address_space;
> -    addr += section->offset_within_region;
> -    return addr;
> -}
> -
> -/**
>   * address_space_sync_dirty_bitmap: synchronize the dirty log for all memory
>   *
>   * Synchronizes the dirty page log for an entire address space.
> @@ -857,6 +840,20 @@ void address_space_write(AddressSpace *as, hwaddr addr,
>   */
>  void address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int len);
>
> +/* address_space_translate: translate an address range into an address space

"within an address space"

> + * into a MemoryRegionSection and an address range into that section
> + *
> + * @as: #AddressSpace to be accessed
> + * @addr: address within that address space
> + * @xlat: pointer to address within the returned memory region section's
> + * #MemoryRegion.
> + * @len: pointer to length
> + * @is_write: indicates the transfer direction
> + */

*len is used both as an input and an output, right? I think
the doc comment could probably use expansion (ie a descriptive
paragraph after the short parameter list). Then you can say
"On entry *@len should be the length of the address range.
On return *@len is updated with [whatever] and *@xlat is
set to [whatever]." without compromising clarity by trying
to squish it all into a one-sentence description of the param.


> +MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
> +                                             hwaddr *xlat, hwaddr *len,
> +                                             bool is_write);

Your parameter names here don't match the ones in the
function definition (len vs plen).

> +
>  /* address_space_valid: check for validity of an address space range
>   *
>   * Check whether access to the given address space range is permitted by
> diff --git a/translate-all.c b/translate-all.c
> index 0d84b0d..7a7d537 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1355,15 +1355,15 @@ void tb_invalidate_phys_addr(hwaddr addr)
>  {
>      ram_addr_t ram_addr;
>      MemoryRegionSection *section;
> +    hwaddr l = 1;
>
> -    section = phys_page_find(address_space_memory.dispatch,
> -                             addr >> TARGET_PAGE_BITS);
> +    section = address_space_translate(&address_space_memory, addr, &addr, &l, false);
>      if (!(memory_region_is_ram(section->mr)
>            || memory_region_is_romd(section->mr))) {
>          return;
>      }
>      ram_addr = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
> -        + memory_region_section_addr(section, addr);
> +        + addr;
>      tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0);
>  }
>  #endif /* TARGET_HAS_ICE && !defined(CONFIG_USER_ONLY) */
> --
> 1.7.1
>
>
>

thanks
-- PMM
Paolo Bonzini - May 20, 2013, 10:41 a.m.
Il 07/05/2013 20:08, Peter Maydell ha scritto:
>> >
>> > -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
>> > +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
>> > +                                      false);
> I find it a little confusing reusing 'addr' for the returned
> offset from address_space_translate(); maybe using a different
> variable would be clearer? (don't feel very strongly about it)

True, on the other hand this matches usage before this patch.

-        addr = memory_region_section_addr(section, addr);

>> > +    if (l < 4) {
>> > +        return -1;
>> > +    }
>> >
>> >      if (!(memory_region_is_ram(section->mr) ||
>> >            memory_region_is_romd(section->mr))) {
>> >          /* I/O case */
>> > -        addr = memory_region_section_addr(section, addr);
>> >          val = io_mem_read(section->mr, addr, 4);
>> >  #if defined(TARGET_WORDS_BIGENDIAN)
>> >          if (endian == DEVICE_LITTLE_ENDIAN) {
>> > @@ -2249,7 +2240,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
>> >          /* RAM case */
>> >          ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
>> >                                  & TARGET_PAGE_MASK)
>> > -                               + memory_region_section_addr(section, addr));
>> > +                               + addr);

>>
>> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
>> +    section = address_space_translate(&address_space_memory, addr, &addr, &l,
>> +                                      false);
>> +    if (l < 2) {
>> +        return -1;
>> +    }
> 
> This kind of "let's just return -1 if the read failed" kind
> of bothers me, because "memory access aborted" is really
> completely different from "memory access succeeded and
> returned -1". But there are a lot of APIs which we'd need
> to fix to properly communicate the problem back to the
> caller, so let it slide for now. (What used to happen
> in the old code in this case?)

It would call unassigned_mem_read and return 0.  I'll change this to
call unassigned_mem_read, which also requires fixing the "double use" of
addr that you pointed out above.

Paolo

Patch

diff --git a/cputlb.c b/cputlb.c
index aba7e44..1f85da0 100644
--- a/cputlb.c
+++ b/cputlb.c
@@ -248,13 +248,18 @@  void tlb_set_page(CPUArchState *env, target_ulong vaddr,
     target_ulong code_address;
     uintptr_t addend;
     CPUTLBEntry *te;
-    hwaddr iotlb;
+    hwaddr iotlb, xlat, sz;
 
     assert(size >= TARGET_PAGE_SIZE);
     if (size != TARGET_PAGE_SIZE) {
         tlb_add_large_page(env, vaddr, size);
     }
-    section = phys_page_find(address_space_memory.dispatch, paddr >> TARGET_PAGE_BITS);
+
+    sz = size;
+    section = address_space_translate(&address_space_memory, paddr, &xlat, &sz,
+                                      false);
+    assert(sz >= TARGET_PAGE_SIZE);
+
 #if defined(DEBUG_TLB)
     printf("tlb_set_page: vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
            " prot=%x idx=%d pd=0x%08lx\n",
@@ -269,15 +274,14 @@  void tlb_set_page(CPUArchState *env, target_ulong vaddr,
     }
     if (memory_region_is_ram(section->mr) ||
         memory_region_is_romd(section->mr)) {
-        addend = (uintptr_t)memory_region_get_ram_ptr(section->mr)
-        + memory_region_section_addr(section, paddr);
+        addend = (uintptr_t)memory_region_get_ram_ptr(section->mr) + xlat;
     } else {
         addend = 0;
     }
 
     code_address = address;
-    iotlb = memory_region_section_get_iotlb(env, section, vaddr, paddr, prot,
-                                            &address);
+    iotlb = memory_region_section_get_iotlb(env, section, vaddr, paddr, xlat,
+                                            prot, &address);
 
     index = (vaddr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1);
     env->iotlb[mmu_idx][index] = iotlb - vaddr;
@@ -300,9 +304,7 @@  void tlb_set_page(CPUArchState *env, target_ulong vaddr,
             /* Write access calls the I/O callback.  */
             te->addr_write = address | TLB_MMIO;
         } else if (memory_region_is_ram(section->mr)
-                   && !cpu_physical_memory_is_dirty(
-                           section->mr->ram_addr
-                           + memory_region_section_addr(section, paddr))) {
+                   && !cpu_physical_memory_is_dirty(section->mr->ram_addr + xlat)) {
             te->addr_write = address | TLB_NOTDIRTY;
         } else {
             te->addr_write = address;
diff --git a/exec.c b/exec.c
index 405de9f..9709bc4 100644
--- a/exec.c
+++ b/exec.c
@@ -182,24 +182,36 @@  static void phys_page_set(AddressSpaceDispatch *d,
     phys_page_set_level(&d->phys_map, &index, &nb, leaf, P_L2_LEVELS - 1);
 }
 
-MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index)
+static MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index)
 {
     PhysPageEntry lp = d->phys_map;
     PhysPageEntry *p;
     int i;
-    uint16_t s_index = phys_section_unassigned;
 
     for (i = P_L2_LEVELS - 1; i >= 0 && !lp.is_leaf; i--) {
         if (lp.ptr == PHYS_MAP_NODE_NIL) {
-            goto not_found;
+            return &phys_sections[phys_section_unassigned];
         }
         p = phys_map_nodes[lp.ptr];
         lp = p[(index >> (i * L2_BITS)) & (L2_SIZE - 1)];
     }
+    return &phys_sections[lp.ptr];
+}
+
+MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
+                                             hwaddr *xlat, hwaddr *plen,
+                                             bool is_write)
+{
+    MemoryRegionSection *section;
+
+    section = phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS);
+    /* Compute offset with MemoryRegionSection */
+    addr -= section->offset_within_address_space;
+    *plen = MIN(section->size - addr, *plen);
 
-    s_index = lp.ptr;
-not_found:
-    return &phys_sections[s_index];
+    /* Compute offset with MemoryRegion */
+    *xlat = addr + section->offset_within_region;
+    return section;
 }
 
 bool memory_region_is_unassigned(MemoryRegion *mr)
@@ -620,11 +632,11 @@  static int cpu_physical_memory_set_dirty_tracking(int enable)
 }
 
 hwaddr memory_region_section_get_iotlb(CPUArchState *env,
-                                                   MemoryRegionSection *section,
-                                                   target_ulong vaddr,
-                                                   hwaddr paddr,
-                                                   int prot,
-                                                   target_ulong *address)
+                                       MemoryRegionSection *section,
+                                       target_ulong vaddr,
+                                       hwaddr paddr, hwaddr xlat,
+                                       int prot,
+                                       target_ulong *address)
 {
     hwaddr iotlb;
     CPUWatchpoint *wp;
@@ -632,7 +644,7 @@  hwaddr memory_region_section_get_iotlb(CPUArchState *env,
     if (memory_region_is_ram(section->mr)) {
         /* Normal RAM.  */
         iotlb = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
-            + memory_region_section_addr(section, paddr);
+            + xlat;
         if (!section->readonly) {
             iotlb |= phys_section_notdirty;
         } else {
@@ -640,7 +652,7 @@  hwaddr memory_region_section_get_iotlb(CPUArchState *env,
         }
     } else {
         iotlb = section - phys_sections;
-        iotlb += memory_region_section_addr(section, paddr);
+        iotlb += xlat;
     }
 
     /* Make accesses to pages with watchpoints go via the
@@ -1903,24 +1915,18 @@  static void invalidate_and_set_dirty(hwaddr addr,
 void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                       int len, bool is_write)
 {
-    AddressSpaceDispatch *d = as->dispatch;
-    int l;
+    hwaddr l;
     uint8_t *ptr;
     uint32_t val;
-    hwaddr page;
+    hwaddr addr1;
     MemoryRegionSection *section;
 
     while (len > 0) {
-        page = addr & TARGET_PAGE_MASK;
-        l = (page + TARGET_PAGE_SIZE) - addr;
-        if (l > len)
-            l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+        l = len;
+        section = address_space_translate(as, addr, &addr1, &l, is_write);
 
         if (is_write) {
             if (!memory_region_is_ram(section->mr)) {
-                hwaddr addr1;
-                addr1 = memory_region_section_addr(section, addr);
                 /* XXX: could force cpu_single_env to NULL to avoid
                    potential bugs */
                 if (l >= 4 && ((addr1 & 3) == 0)) {
@@ -1940,9 +1946,7 @@  void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                     l = 1;
                 }
             } else if (!section->readonly) {
-                ram_addr_t addr1;
-                addr1 = memory_region_get_ram_addr(section->mr)
-                    + memory_region_section_addr(section, addr);
+                addr1 += memory_region_get_ram_addr(section->mr);
                 /* RAM case */
                 ptr = qemu_get_ram_ptr(addr1);
                 memcpy(ptr, buf, l);
@@ -1952,9 +1956,7 @@  void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
         } else {
             if (!(memory_region_is_ram(section->mr) ||
                   memory_region_is_romd(section->mr))) {
-                hwaddr addr1;
                 /* I/O case */
-                addr1 = memory_region_section_addr(section, addr);
                 if (l >= 4 && ((addr1 & 3) == 0)) {
                     /* 32 bit read access */
                     val = io_mem_read(section->mr, addr1, 4);
@@ -1973,9 +1975,7 @@  void address_space_rw(AddressSpace *as, hwaddr addr, uint8_t *buf,
                 }
             } else {
                 /* RAM case */
-                ptr = qemu_get_ram_ptr(section->mr->ram_addr
-                                       + memory_region_section_addr(section,
-                                                                    addr));
+                ptr = qemu_get_ram_ptr(section->mr->ram_addr + addr1);
                 memcpy(buf, ptr, l);
                 qemu_put_ram_ptr(ptr);
             }
@@ -2015,26 +2015,21 @@  void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
 void cpu_physical_memory_write_rom(hwaddr addr,
                                    const uint8_t *buf, int len)
 {
-    AddressSpaceDispatch *d = address_space_memory.dispatch;
-    int l;
+    hwaddr l;
     uint8_t *ptr;
-    hwaddr page;
+    hwaddr addr1;
     MemoryRegionSection *section;
 
     while (len > 0) {
-        page = addr & TARGET_PAGE_MASK;
-        l = (page + TARGET_PAGE_SIZE) - addr;
-        if (l > len)
-            l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+        l = len;
+        section = address_space_translate(&address_space_memory,
+                                          addr, &addr1, &l, true);
 
         if (!(memory_region_is_ram(section->mr) ||
               memory_region_is_romd(section->mr))) {
             /* do nothing */
         } else {
-            unsigned long addr1;
-            addr1 = memory_region_get_ram_addr(section->mr)
-                + memory_region_section_addr(section, addr);
+            addr1 += memory_region_get_ram_addr(section->mr);
             /* ROM/RAM case */
             ptr = qemu_get_ram_ptr(addr1);
             memcpy(ptr, buf, l);
@@ -2095,18 +2090,12 @@  static void cpu_notify_map_clients(void)
 
 bool address_space_valid(AddressSpace *as, hwaddr addr, int len, bool is_write)
 {
-    AddressSpaceDispatch *d = as->dispatch;
     MemoryRegionSection *section;
-    int l;
-    hwaddr page;
+    hwaddr l, xlat;
 
     while (len > 0) {
-        page = addr & TARGET_PAGE_MASK;
-        l = (page + TARGET_PAGE_SIZE) - addr;
-        if (l > len) {
-            l = len;
-        }
-        section = phys_page_find(d, addr >> TARGET_PAGE_BITS);
+        l = len;
+        section = address_space_translate(as, addr, &xlat, &l, is_write);
         if (section->mr == &io_mem_unassigned) {
             return false;
         }
@@ -2129,22 +2118,17 @@  void *address_space_map(AddressSpace *as,
                         hwaddr *plen,
                         bool is_write)
 {
-    AddressSpaceDispatch *d = as->dispatch;
     hwaddr len = *plen;
     hwaddr todo = 0;
-    int l;
-    hwaddr page;
+    hwaddr l, xlat;
     MemoryRegionSection *section;
     ram_addr_t raddr = RAM_ADDR_MAX;
     ram_addr_t rlen;
     void *ret;
 
     while (len > 0) {
-        page = addr & TARGET_PAGE_MASK;
-        l = (page + TARGET_PAGE_SIZE) - addr;
-        if (l > len)
-            l = len;
-        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
+        l = len;
+        section = address_space_translate(as, addr, &xlat, &l, is_write);
 
         if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
             if (todo || bounce.buffer) {
@@ -2161,8 +2145,11 @@  void *address_space_map(AddressSpace *as,
             return bounce.buffer;
         }
         if (!todo) {
-            raddr = memory_region_get_ram_addr(section->mr)
-                + memory_region_section_addr(section, addr);
+            raddr = memory_region_get_ram_addr(section->mr) + xlat;
+        } else {
+            if (memory_region_get_ram_addr(section->mr) + xlat != raddr + todo) {
+                break;
+            }
         }
 
         len -= l;
@@ -2228,13 +2215,17 @@  static inline uint32_t ldl_phys_internal(hwaddr addr,
     uint8_t *ptr;
     uint32_t val;
     MemoryRegionSection *section;
+    hwaddr l = 4;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory, addr, &addr, &l,
+                                      false);
+    if (l < 4) {
+        return -1;
+    }
 
     if (!(memory_region_is_ram(section->mr) ||
           memory_region_is_romd(section->mr))) {
         /* I/O case */
-        addr = memory_region_section_addr(section, addr);
         val = io_mem_read(section->mr, addr, 4);
 #if defined(TARGET_WORDS_BIGENDIAN)
         if (endian == DEVICE_LITTLE_ENDIAN) {
@@ -2249,7 +2240,7 @@  static inline uint32_t ldl_phys_internal(hwaddr addr,
         /* RAM case */
         ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
                                 & TARGET_PAGE_MASK)
-                               + memory_region_section_addr(section, addr));
+                               + addr);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = ldl_le_p(ptr);
@@ -2287,13 +2278,17 @@  static inline uint64_t ldq_phys_internal(hwaddr addr,
     uint8_t *ptr;
     uint64_t val;
     MemoryRegionSection *section;
+    hwaddr l = 8;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory, addr, &addr, &l,
+                                      false);
+    if (l < 8) {
+        return -1;
+    }
 
     if (!(memory_region_is_ram(section->mr) ||
           memory_region_is_romd(section->mr))) {
         /* I/O case */
-        addr = memory_region_section_addr(section, addr);
 
         /* XXX This is broken when device endian != cpu endian.
                Fix and add "endian" variable check */
@@ -2308,7 +2303,7 @@  static inline uint64_t ldq_phys_internal(hwaddr addr,
         /* RAM case */
         ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
                                 & TARGET_PAGE_MASK)
-                               + memory_region_section_addr(section, addr));
+                               + addr);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = ldq_le_p(ptr);
@@ -2354,13 +2349,17 @@  static inline uint32_t lduw_phys_internal(hwaddr addr,
     uint8_t *ptr;
     uint64_t val;
     MemoryRegionSection *section;
+    hwaddr l = 2;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory, addr, &addr, &l,
+                                      false);
+    if (l < 2) {
+        return -1;
+    }
 
     if (!(memory_region_is_ram(section->mr) ||
           memory_region_is_romd(section->mr))) {
         /* I/O case */
-        addr = memory_region_section_addr(section, addr);
         val = io_mem_read(section->mr, addr, 2);
 #if defined(TARGET_WORDS_BIGENDIAN)
         if (endian == DEVICE_LITTLE_ENDIAN) {
@@ -2375,7 +2374,7 @@  static inline uint32_t lduw_phys_internal(hwaddr addr,
         /* RAM case */
         ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
                                 & TARGET_PAGE_MASK)
-                               + memory_region_section_addr(section, addr));
+                               + addr);
         switch (endian) {
         case DEVICE_LITTLE_ENDIAN:
             val = lduw_le_p(ptr);
@@ -2413,11 +2412,15 @@  void stl_phys_notdirty(hwaddr addr, uint32_t val)
 {
     uint8_t *ptr;
     MemoryRegionSection *section;
+    hwaddr l = 2;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory, addr, &addr, &l,
+                                      true);
+    if (l < 2) {
+        return;
+    }
 
     if (!memory_region_is_ram(section->mr) || section->readonly) {
-        addr = memory_region_section_addr(section, addr);
         if (memory_region_is_ram(section->mr)) {
             section = &phys_sections[phys_section_rom];
         }
@@ -2425,7 +2428,7 @@  void stl_phys_notdirty(hwaddr addr, uint32_t val)
     } else {
         unsigned long addr1 = (memory_region_get_ram_addr(section->mr)
                                & TARGET_PAGE_MASK)
-            + memory_region_section_addr(section, addr);
+            + addr;
         ptr = qemu_get_ram_ptr(addr1);
         stl_p(ptr, val);
 
@@ -2445,11 +2448,15 @@  void stq_phys_notdirty(hwaddr addr, uint64_t val)
 {
     uint8_t *ptr;
     MemoryRegionSection *section;
+    hwaddr l = 4;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory, addr, &addr, &l,
+                                      true);
+    if (l < 4) {
+        return;
+    }
 
     if (!memory_region_is_ram(section->mr) || section->readonly) {
-        addr = memory_region_section_addr(section, addr);
         if (memory_region_is_ram(section->mr)) {
             section = &phys_sections[phys_section_rom];
         }
@@ -2463,7 +2470,7 @@  void stq_phys_notdirty(hwaddr addr, uint64_t val)
     } else {
         ptr = qemu_get_ram_ptr((memory_region_get_ram_addr(section->mr)
                                 & TARGET_PAGE_MASK)
-                               + memory_region_section_addr(section, addr));
+                               + addr);
         stq_p(ptr, val);
     }
 }
@@ -2474,11 +2481,15 @@  static inline void stl_phys_internal(hwaddr addr, uint32_t val,
 {
     uint8_t *ptr;
     MemoryRegionSection *section;
+    hwaddr l = 8;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory, addr, &addr, &l,
+                                      true);
+    if (l < 8) {
+        return;
+    }
 
     if (!memory_region_is_ram(section->mr) || section->readonly) {
-        addr = memory_region_section_addr(section, addr);
         if (memory_region_is_ram(section->mr)) {
             section = &phys_sections[phys_section_rom];
         }
@@ -2495,7 +2506,7 @@  static inline void stl_phys_internal(hwaddr addr, uint32_t val,
     } else {
         unsigned long addr1;
         addr1 = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
-            + memory_region_section_addr(section, addr);
+            + addr;
         /* RAM case */
         ptr = qemu_get_ram_ptr(addr1);
         switch (endian) {
@@ -2541,11 +2552,15 @@  static inline void stw_phys_internal(hwaddr addr, uint32_t val,
 {
     uint8_t *ptr;
     MemoryRegionSection *section;
+    hwaddr l = 4;
 
-    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory, addr, &addr, &l,
+                                      true);
+    if (l < 4) {
+        return;
+    }
 
     if (!memory_region_is_ram(section->mr) || section->readonly) {
-        addr = memory_region_section_addr(section, addr);
         if (memory_region_is_ram(section->mr)) {
             section = &phys_sections[phys_section_rom];
         }
@@ -2562,7 +2577,7 @@  static inline void stw_phys_internal(hwaddr addr, uint32_t val,
     } else {
         unsigned long addr1;
         addr1 = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
-            + memory_region_section_addr(section, addr);
+            + addr;
         /* RAM case */
         ptr = qemu_get_ram_ptr(addr1);
         switch (endian) {
@@ -2666,9 +2681,10 @@  bool virtio_is_big_endian(void)
 bool cpu_physical_memory_is_io(hwaddr phys_addr)
 {
     MemoryRegionSection *section;
+    hwaddr l = 1;
 
-    section = phys_page_find(address_space_memory.dispatch,
-                             phys_addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory,
+                                      phys_addr, &phys_addr, &l, false);
 
     return !(memory_region_is_ram(section->mr) ||
              memory_region_is_romd(section->mr));
diff --git a/include/exec/cputlb.h b/include/exec/cputlb.h
index 733c885..e821660 100644
--- a/include/exec/cputlb.h
+++ b/include/exec/cputlb.h
@@ -26,8 +26,6 @@  void tlb_unprotect_code_phys(CPUArchState *env, ram_addr_t ram_addr,
                              target_ulong vaddr);
 void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry, uintptr_t start,
                            uintptr_t length);
-MemoryRegionSection *phys_page_find(struct AddressSpaceDispatch *d,
-                                    hwaddr index);
 void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length);
 void tlb_set_dirty(CPUArchState *env, target_ulong vaddr);
 extern int tlb_flush_count;
@@ -35,11 +33,11 @@  extern int tlb_flush_count;
 /* exec.c */
 void tb_flush_jmp_cache(CPUArchState *env, target_ulong addr);
 hwaddr memory_region_section_get_iotlb(CPUArchState *env,
-                                                   MemoryRegionSection *section,
-                                                   target_ulong vaddr,
-                                                   hwaddr paddr,
-                                                   int prot,
-                                                   target_ulong *address);
+                                       MemoryRegionSection *section,
+                                       target_ulong vaddr,
+                                       hwaddr paddr, hwaddr xlat,
+                                       int prot,
+                                       target_ulong *address);
 bool memory_region_is_unassigned(MemoryRegion *mr);
 
 #endif
diff --git a/include/exec/memory.h b/include/exec/memory.h
index c38e974..914f5d4 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -740,23 +740,6 @@  MemoryRegionSection memory_region_find(MemoryRegion *mr,
                                        hwaddr addr, uint64_t size);
 
 /**
- * memory_region_section_addr: get offset within MemoryRegionSection
- *
- * Returns offset within MemoryRegionSection
- *
- * @section: the memory region section being queried
- * @addr: address in address space
- */
-static inline hwaddr
-memory_region_section_addr(MemoryRegionSection *section,
-                           hwaddr addr)
-{
-    addr -= section->offset_within_address_space;
-    addr += section->offset_within_region;
-    return addr;
-}
-
-/**
  * address_space_sync_dirty_bitmap: synchronize the dirty log for all memory
  *
  * Synchronizes the dirty page log for an entire address space.
@@ -857,6 +840,20 @@  void address_space_write(AddressSpace *as, hwaddr addr,
  */
 void address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int len);
 
+/* address_space_translate: translate an address range into an address space
+ * into a MemoryRegionSection and an address range into that section
+ *
+ * @as: #AddressSpace to be accessed
+ * @addr: address within that address space
+ * @xlat: pointer to address within the returned memory region section's
+ * #MemoryRegion.
+ * @len: pointer to length
+ * @is_write: indicates the transfer direction
+ */
+MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
+                                             hwaddr *xlat, hwaddr *len,
+                                             bool is_write);
+
 /* address_space_valid: check for validity of an address space range
  *
  * Check whether access to the given address space range is permitted by
diff --git a/translate-all.c b/translate-all.c
index 0d84b0d..7a7d537 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -1355,15 +1355,15 @@  void tb_invalidate_phys_addr(hwaddr addr)
 {
     ram_addr_t ram_addr;
     MemoryRegionSection *section;
+    hwaddr l = 1;
 
-    section = phys_page_find(address_space_memory.dispatch,
-                             addr >> TARGET_PAGE_BITS);
+    section = address_space_translate(&address_space_memory, addr, &addr, &l, false);
     if (!(memory_region_is_ram(section->mr)
           || memory_region_is_romd(section->mr))) {
         return;
     }
     ram_addr = (memory_region_get_ram_addr(section->mr) & TARGET_PAGE_MASK)
-        + memory_region_section_addr(section, addr);
+        + addr;
     tb_invalidate_phys_page_range(ram_addr, ram_addr + 1, 0);
 }
 #endif /* TARGET_HAS_ICE && !defined(CONFIG_USER_ONLY) */