Re: [Qemu-devel] [PATCH v2 10/11] exec: reorganize address_space_map

[Jan Kiszka <jan.kiszka@siemens.com>]

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux