[RFC,09/15] memory: Introduce address_space_lookup_region

This new service so far only replaces phys_page_find as public API. In a
follow-up step, it will return the effective memory region for the
specified address, i.e. after resolving what are currently sub-pages.
Moreover, it will also once encapsulate locking and reference counting
when we introduce BQL-free dispatching.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 cputlb.c              |    2 +-
 exec.c                |   46 +++++++++++++++++++++-------------------------
 include/exec/cputlb.h |    2 --
 include/exec/memory.h |    9 +++++++++
 translate-all.c       |    3 +--
 5 files changed, 32 insertions(+), 30 deletions(-)

Il 06/05/2013 16:26, Jan Kiszka ha scritto:
> This new service so far only replaces phys_page_find as public API. In a
> follow-up step, it will return the effective memory region for the
> specified address, i.e. after resolving what are currently sub-pages.
> Moreover, it will also once encapsulate locking and reference counting
> when we introduce BQL-free dispatching.

In my IOMMU rebase I have a similar function:

/* 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);

It wraps (actually, replaces) both phys_page_find and
memory_region_section_addr.

Paolo

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  cputlb.c              |    2 +-
>  exec.c                |   46 +++++++++++++++++++++-------------------------
>  include/exec/cputlb.h |    2 --
>  include/exec/memory.h |    9 +++++++++
>  translate-all.c       |    3 +--
>  5 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index aba7e44..e2c95c1 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -254,7 +254,7 @@ void tlb_set_page(CPUArchState *env, target_ulong vaddr,
>      if (size != TARGET_PAGE_SIZE) {
>          tlb_add_large_page(env, vaddr, size);
>      }
> -    section = phys_page_find(address_space_memory.dispatch, paddr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, paddr);
>  #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",
> diff --git a/exec.c b/exec.c
> index 19725db..53c2778 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -182,7 +182,8 @@ 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;
> @@ -1894,19 +1895,16 @@ 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;
>      uint8_t *ptr;
>      uint32_t val;
> -    hwaddr page;
>      MemoryRegionSection *section;
>  
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> +        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        section = address_space_lookup_region(as, addr);
>  
>          if (is_write) {
>              if (!memory_region_is_ram(section->mr)) {
> @@ -2006,18 +2004,15 @@ 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;
>      uint8_t *ptr;
> -    hwaddr page;
>      MemoryRegionSection *section;
>  
>      while (len > 0) {
> -        page = addr & TARGET_PAGE_MASK;
> -        l = (page + TARGET_PAGE_SIZE) - addr;
> +        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        section = address_space_lookup_region(&address_space_memory, addr);
>  
>          if (!(memory_region_is_ram(section->mr) ||
>                memory_region_is_romd(section->mr))) {
> @@ -2096,22 +2091,19 @@ 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;
>      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;
> +        l = ((addr & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE) - addr;
>          if (l > len)
>              l = len;
> -        section = phys_page_find(d, page >> TARGET_PAGE_BITS);
> +        section = address_space_lookup_region(as, addr);
>  
>          if (!(memory_region_is_ram(section->mr) && !section->readonly)) {
>              if (todo || bounce.buffer) {
> @@ -2188,6 +2180,11 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,
>      return address_space_unmap(&address_space_memory, buffer, len, is_write, access_len);
>  }
>  
> +MemoryRegionSection *address_space_lookup_region(AddressSpace *as, hwaddr addr)
> +{
> +    return phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS);
> +}
> +
>  /* warning: addr must be aligned */
>  static inline uint32_t ldl_phys_internal(hwaddr addr,
>                                           enum device_endian endian)
> @@ -2196,7 +2193,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
>      uint32_t val;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
> @@ -2255,7 +2252,7 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,
>      uint64_t val;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
> @@ -2322,7 +2319,7 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,
>      uint64_t val;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!(memory_region_is_ram(section->mr) ||
>            memory_region_is_romd(section->mr))) {
> @@ -2381,7 +2378,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
>      uint8_t *ptr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
>          addr = memory_region_section_addr(section, addr);
> @@ -2413,7 +2410,7 @@ void stq_phys_notdirty(hwaddr addr, uint64_t val)
>      uint8_t *ptr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
>          addr = memory_region_section_addr(section, addr);
> @@ -2442,7 +2439,7 @@ static inline void stl_phys_internal(hwaddr addr, uint32_t val,
>      uint8_t *ptr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
>          addr = memory_region_section_addr(section, addr);
> @@ -2509,7 +2506,7 @@ static inline void stw_phys_internal(hwaddr addr, uint32_t val,
>      uint8_t *ptr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch, addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>  
>      if (!memory_region_is_ram(section->mr) || section->readonly) {
>          addr = memory_region_section_addr(section, addr);
> @@ -2634,8 +2631,7 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr)
>  {
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch,
> -                             phys_addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, phys_addr);
>  
>      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..f144e6e 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;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 9e88320..11ca4e2 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -887,6 +887,15 @@ void *address_space_map(AddressSpace *as, hwaddr addr,
>  void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
>                           int is_write, hwaddr access_len);
>  
> +/**
> + * address_space_lookup_region: Looks up memory region corresponding to given
> + *                              access address
> + *
> + * @as: #AddressSpace to be accessed
> + * @addr: address within that address space
> + */
> +MemoryRegionSection *address_space_lookup_region(AddressSpace *as,
> +                                                 hwaddr addr);
>  
>  #endif
>  
> diff --git a/translate-all.c b/translate-all.c
> index da93608..078a657 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -1356,8 +1356,7 @@ void tb_invalidate_phys_addr(hwaddr addr)
>      ram_addr_t ram_addr;
>      MemoryRegionSection *section;
>  
> -    section = phys_page_find(address_space_memory.dispatch,
> -                             addr >> TARGET_PAGE_BITS);
> +    section = address_space_lookup_region(&address_space_memory, addr);
>      if (!(memory_region_is_ram(section->mr)
>            || (section->mr->rom_device && section->mr->readable))) {
>          return;
>

On 2013-05-06 16:39, Paolo Bonzini wrote:
> Il 06/05/2013 16:26, Jan Kiszka ha scritto:
>> This new service so far only replaces phys_page_find as public API. In a
>> follow-up step, it will return the effective memory region for the
>> specified address, i.e. after resolving what are currently sub-pages.
>> Moreover, it will also once encapsulate locking and reference counting
>> when we introduce BQL-free dispatching.
> 
> In my IOMMU rebase I have a similar function:
> 
> /* 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);
> 
> It wraps (actually, replaces) both phys_page_find and
> memory_region_section_addr.

Good, looks like we are heading in similar directions. What is the
purpose of len? When does is_write matter?

In a later step, this should become something like
address_space_get_region_ref (to be paired with memory_region_unref,
once done). So this one also takes care of incrementing the reference
counter or acquiring the BQL, as necessary. Currently, it asks the
caller to specify if the BQL is already held, but that will change.

Jan

Il 06/05/2013 16:51, Jan Kiszka ha scritto:
> On 2013-05-06 16:39, Paolo Bonzini wrote:
>> Il 06/05/2013 16:26, Jan Kiszka ha scritto:
>>> This new service so far only replaces phys_page_find as public API. In a
>>> follow-up step, it will return the effective memory region for the
>>> specified address, i.e. after resolving what are currently sub-pages.
>>> Moreover, it will also once encapsulate locking and reference counting
>>> when we introduce BQL-free dispatching.
>>
>> In my IOMMU rebase I have a similar function:
>>
>> /* 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);
>>
>> It wraps (actually, replaces) both phys_page_find and
>> memory_region_section_addr.
> 
> Good, looks like we are heading in similar directions. What is the
> purpose of len? When does is_write matter?

Both matter when adding the IOMMU.  is_write is needed to check for
permissions, and len because the translation will be valid for one page
only (not for the full size of the MemoryRegion.

I can implement address_space_translate on top of
address_space_lookup_region and include your next patch too, so we're
fine here.

Paolo

> In a later step, this should become something like
> address_space_get_region_ref (to be paired with memory_region_unref,
> once done). So this one also takes care of incrementing the reference
> counter or acquiring the BQL, as necessary. Currently, it asks the
> caller to specify if the BQL is already held, but that will change.
> 
> Jan
>