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

[Jan Kiszka]

On 2013-05-25 09:47, Paolo Bonzini wrote:
> Il 25/05/2013 08:40, Jan Kiszka ha scritto:
>> 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 <address@hidden> --- 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 guess it depends on the width of the bus.  On a 16-bit computer the
> right thing to do would be to split the write in two, one two-byte
> access to A and one two-byte access to B.  But as a first
> approximation the fix you suggest is the right one.  Implementing bus
> width in TCG is tricky, so we should get by with the simple fix.
> 
> This patch is not in the pull request I sent, so there is time to make
> it work.  Is it simply
> 
> -    *plen = MIN(section->size - addr, *plen);
> +    addr += section->offset_within_memory_region;
> +    *plen = MIN(section->mr->size - addr, *plen);
> 
> or something like that?  If you post it as a diff I can squash it in.

This works for me now:

diff --git a/exec.c b/exec.c
index 1610604..3d36bc7 100644
--- a/exec.c
+++ b/exec.c
@@ -210,13 +210,15 @@ MemoryRegionSection *address_space_translate(AddressSpace 
*as, hwaddr addr,
     IOMMUTLBEntry iotlb;
     MemoryRegionSection *section;
     hwaddr len = *plen;
+    Int128 diff;
 
     for (;;) {
         section = address_space_lookup_region(as, addr);
 
         /* Compute offset within MemoryRegionSection */
         addr -= section->offset_within_address_space;
-        len = MIN(section->size - addr, len);
+        diff = int128_sub(section->mr->size, int128_make64(addr));
+        len = MIN(int128_get64(diff), len);
 
         /* Compute offset within MemoryRegion */
         addr += section->offset_within_region;

Jan

signature.asc
Description: OpenPGP digital signature