[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 12/40] memory: add address_space_translate
|
From: |
Peter Maydell |
|
Subject: |
Re: [Qemu-devel] [PATCH 12/40] memory: add address_space_translate |
|
Date: |
Tue, 7 May 2013 19:08:10 +0100 |
On 7 May 2013 15:16, Paolo Bonzini <address@hidden> 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 <address@hidden>
> ---
> 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 address@hidden should be the length of the address range.
On return address@hidden is updated with [whatever] and address@hidden 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
[Qemu-devel] [PATCH 19/40] dma: eliminate old-style IOMMU support, Paolo Bonzini, 2013/05/07