[v2,10/11] exec: reorganize address_space_map

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 <pbonzini@redhat.com>
---
 exec.c | 71 +++++++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 38 insertions(+), 33 deletions(-)

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 <pbonzini@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

Il 01/07/2013 20:34, Jan Kiszka ha scritto:
> 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?

You could have separate sections if, for example, two consecutive ranges
map to contiguous sections but one is read-only and one is read-write.

Paolo

On 2013-07-01 22:44, Paolo Bonzini wrote:
> Il 01/07/2013 20:34, Jan Kiszka ha scritto:
>> 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?
> 
> You could have separate sections if, for example, two consecutive ranges
> map to contiguous sections but one is read-only and one is read-write.

OK, makes sense. You can add my

Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>

Jan