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

[Paolo Bonzini <pbonzini@redhat.com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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 <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 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.

> 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

Paolo

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRoGybAAoJEBvWZb6bTYbyELUP/09bPNSKl39Tog0uyV+VJ3BR
IGDsWfKHnIm6T2hC0S4BhVRaBkDyDAC6c8hJ7iy9084/pw61Cra1RrNMdSOzNHqo
x4pC+Zw6hD9lK0ncRsB21w1n398yen7nk+RN4zf2LxxAnIiXrUuaG6SL5q3qSibz
lXj1fKP+hG2yoxS7frMAJVjBEFIhBj/EY5u3pmd6+qREGxYaLBKkuXSnbjHdfG3s
Q8OLJZ8bRH7o3c034yt+LvJ776PfhjzaggUmrYri7fZ2kU+SmUzqnkW9If2ekQsf
MQ3ZQRVrVc5AaHLdQLyjSaSpzPcON5w4VpqU9SCisKi9rFdDOW6qHuZc6ZTbUpch
wAbGeI0h1PA01o41milx0LU+Y7Qfl6wRzbB3CZEpgp8fFSR95LySwD1Zldt1Ft6E
Zv8Mh0NVV4qvVEtp+HrTTh91kVz5W9OXnqFOz+wYrBlaVBRUxBLWSzQvuScnYv2B
Rv51bcsq07bOZFR2BabC9zuRnHlCC9LqQAvP5i8AdvC1FsxIGPtK+FLc3QDeD+c7
Ndz5mZq6jgAeg0BQrBv7H++P6I2dw/lJkrb4zwGY68ckJ+Yo67NGbYvhM0/W1WF5
XVebDnb1F3+qWsVtSJWWoOHtW0HRFS+3il87/u4MKJc1BF+ceONrSJLBQyvSJDNf
9sPb/mhfVloOj7tWqWAk
=Y4yR
-----END PGP SIGNATURE-----