Re: [Qemu-devel] [PATCH 17/30] memory: add address_space_translate

[Jan Kiszka <jan.kiszka@web.de>]

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

On 2013-05-21 12:57, Paolo Bonzini 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.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cputlb.c              |  20 ++---
>  exec.c                | 201 +++++++++++++++++++++++++++-----------------------
>  include/exec/cputlb.h |  12 ++-
>  include/exec/memory.h |  31 ++++----
>  translate-all.c       |   6 +-
>  5 files changed, 143 insertions(+), 127 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 82da067..e5ee8ff 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -182,7 +182,7 @@ 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;
> @@ -198,6 +198,22 @@ MemoryRegionSection *phys_page_find(AddressSpaceDispatch *d, hwaddr index)
>      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 within MemoryRegionSection */
> +    addr -= section->offset_within_address_space;
> +    *plen = MIN(section->size - addr, *plen);

This limitation causes problems. Consider two overlapping memory regions
A and B. A handles 4-byte accesses and is at least 4 bytes long, B only
deals with a single byte. They overlap like this:

B (prio 1):   X
A (prio 0): XXXX...
            ^access here with 4 bytes length

Now if an access happens at the marked position, it is split into one
2-byte access to A, followed by a one-byte access to B and another
one-byte access to A. But the right access emulation would be 4-bytes to
A, no?

I think we need to check against the length of the target mr here
instead, but I'm not totally sure yet.

A concrete scenario where this breaks popped up while reworking PIO
dispatching. PCI accesses to 0xcf8 no longer worked:

  0000000000000cf8-0000000000000cfb (prio 0, RW): pci-conf-idx
  0000000000000cf9-0000000000000cf9 (prio 1, RW): piix3-reset-control

Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]