On 2013-06-28 18:58, Paolo Bonzini wrote: > First of all, rename "todo" to "done". > > Second, clearly separate the case of done == 0 from the case of done != 0. > This will help handling reference counting in the next patch. > > Third, this test: > > if (memory_region_get_ram_addr(mr) + xlat != raddr + todo) { > > does not guarantee that the memory region is the same across two iterations > of the while loop. For example, you could have two blocks: > > A) size 640 K, mapped at physical address 0, ram_addr_t 0 > B) size 64 K, mapped at physical address 0xa0000, ram_addr_t 0xa0000 > > then mapping 1 M starting at physical address zero will erroneously treat > B as the continuation of block A. qemu_ram_ptr_length ensures that no > invalid memory is accessed, but it is still a pointless complication of > the algorithm. The patch makes the logic clearer with an explicit test > that the memory region is the same. > > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com> > --- > exec.c | 71 > +++++++++++++++++++++++++++++++++++------------------------------- > 1 file changed, 38 insertions(+), 33 deletions(-) > > diff --git a/exec.c b/exec.c > index a372963..ea79aea 100644 > --- a/exec.c > +++ b/exec.c > @@ -2073,47 +2073,52 @@ void *address_space_map(AddressSpace *as, > bool is_write) > { > hwaddr len = *plen; > - hwaddr todo = 0; > - hwaddr l, xlat; > - MemoryRegion *mr; > - ram_addr_t raddr = RAM_ADDR_MAX; > - ram_addr_t rlen; > - void *ret; > + hwaddr done = 0; > + hwaddr l, xlat, base; > + MemoryRegion *mr, *this_mr; > + ram_addr_t raddr; > > - while (len > 0) { > - l = len; > - mr = address_space_translate(as, addr, &xlat, &l, is_write); > - > - if (!memory_access_is_direct(mr, is_write)) { > - if (todo || bounce.buffer) { > - break; > - } > - bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, > TARGET_PAGE_SIZE); > - bounce.addr = addr; > - bounce.len = l; > - if (!is_write) { > - address_space_read(as, addr, bounce.buffer, l); > - } > + if (len == 0) { > + return NULL; > + } > > - *plen = l; > - return bounce.buffer; > + l = len; > + mr = address_space_translate(as, addr, &xlat, &l, is_write); > + if (!memory_access_is_direct(mr, is_write)) { > + if (bounce.buffer) { > + return NULL; > } > - if (!todo) { > - raddr = memory_region_get_ram_addr(mr) + xlat; > - } else { > - if (memory_region_get_ram_addr(mr) + xlat != raddr + todo) { > - break; > - } > + bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE); > + bounce.addr = addr; > + bounce.len = l; > + if (!is_write) { > + address_space_read(as, addr, bounce.buffer, l); > } > > + *plen = l; > + return bounce.buffer; > + } > + > + base = xlat; > + raddr = memory_region_get_ram_addr(mr); > + > + for (;;) { > len -= l; > addr += l; > - todo += l; > + done += l; > + if (len == 0) { > + break; > + } > + > + l = len; > + this_mr = address_space_translate(as, addr, &xlat, &l, is_write); > + if (this_mr != mr || xlat != base + done) { > + break; > + } > } > - rlen = todo; > - ret = qemu_ram_ptr_length(raddr, &rlen); > - *plen = rlen; > - return ret; > + > + *plen = done; > + return qemu_ram_ptr_length(raddr + base, plen); > } > > /* Unmaps a memory region previously mapped by address_space_map(). >
The transformation looks correct, but I'm currently not understanding in which cases we still need the loop. address_space_translate should return as much of the target mr as the region can contiguously provide, no? Jan -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux