Re: [Qemu-devel] [PATCH v2 4/5] exec.c: refactor cpu_physical_memory_map

[Liu Yu-B13201]

> -----Original Message-----
> From: address@hidden 
> [mailto:address@hidden 
> On Behalf Of address@hidden
> Sent: Friday, May 20, 2011 1:36 AM
> To: address@hidden
> Cc: address@hidden; address@hidden; Stefano Stabellini
> Subject: [Qemu-devel] [PATCH v2 4/5] exec.c: refactor 
> cpu_physical_memory_map
> 
> From: Stefano Stabellini <address@hidden>
> 
> Introduce qemu_ram_ptr_length that takes an address and a size as
> parameters rather than just an address.
> 
> Refactor cpu_physical_memory_map so that we call 
> qemu_ram_ptr_length only
> once rather than calling qemu_get_ram_ptr one time per page.
> This is not only more efficient but also tries to simplify 
> the logic of
> the function.
> Currently we are relying on the fact that all the pages are mapped
> contiguously in qemu's address space: we have a check to make 
> sure that
> the virtual address returned by qemu_get_ram_ptr from the 
> second call on
> is consecutive. Now we are making this more explicit replacing all the
> calls to qemu_get_ram_ptr with a single call to qemu_ram_ptr_length
> passing a size argument.
> 
> Signed-off-by: Stefano Stabellini <address@hidden>
> CC: address@hidden
> CC: address@hidden
> ---
>  cpu-common.h |    1 +
>  exec.c       |   51 
> ++++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/cpu-common.h b/cpu-common.h
> index 151c32c..085aacb 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -64,6 +64,7 @@ void qemu_ram_free(ram_addr_t addr);
>  void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
>  /* This should only be used for ram local to a device.  */
>  void *qemu_get_ram_ptr(ram_addr_t addr);
> +void *qemu_ram_ptr_length(target_phys_addr_t addr, 
> target_phys_addr_t *size);
>  /* Same but slower, to use for migration, where the order of
>   * RAMBlocks must not change. */
>  void *qemu_safe_ram_ptr(ram_addr_t addr);
> diff --git a/exec.c b/exec.c
> index 21f21f0..ff9c174 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3111,6 +3111,31 @@ void *qemu_safe_ram_ptr(ram_addr_t addr)
>      return NULL;
>  }
>  
> +/* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
> + * but takes a size argument */
> +void *qemu_ram_ptr_length(target_phys_addr_t addr, 
> target_phys_addr_t *size)
> +{
> +    if (xen_mapcache_enabled())
> +        return qemu_map_cache(addr, *size, 1);
> +    else {
> +        RAMBlock *block;
> +
> +        QLIST_FOREACH(block, &ram_list.blocks, next) {
> +            if (addr - block->offset < block->length) {
> +                if (addr - block->offset + *size > block->length)
> +                    *size = block->length - addr + block->offset;
> +                return block->host + (addr - block->offset);
> +            }
> +        }
> +
> +        fprintf(stderr, "Bad ram offset %" PRIx64 "\n", 
> (uint64_t)addr);
> +        abort();
> +
> +        *size = 0;
> +        return NULL;
> +    }
> +}
> +
>  void qemu_put_ram_ptr(void *addr)
>  {
>      trace_qemu_put_ram_ptr(addr);
> @@ -3972,14 +3997,12 @@ void 
> *cpu_physical_memory_map(target_phys_addr_t addr,
>                                int is_write)
>  {
>      target_phys_addr_t len = *plen;
> -    target_phys_addr_t done = 0;
> +    target_phys_addr_t todo = 0;
>      int l;
> -    uint8_t *ret = NULL;
> -    uint8_t *ptr;
>      target_phys_addr_t page;
>      unsigned long pd;
>      PhysPageDesc *p;
> -    unsigned long addr1;
> +    target_phys_addr_t addr1 = addr;
>  
>      while (len > 0) {
>          page = addr & TARGET_PAGE_MASK;
> @@ -3994,7 +4017,7 @@ void 
> *cpu_physical_memory_map(target_phys_addr_t addr,
>          }
>  
>          if ((pd & ~TARGET_PAGE_MASK) != IO_MEM_RAM) {
> -            if (done || bounce.buffer) {
> +            if (todo || bounce.buffer) {
>                  break;
>              }
>              bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, 
> TARGET_PAGE_SIZE);
> @@ -4003,23 +4026,17 @@ void 
> *cpu_physical_memory_map(target_phys_addr_t addr,
>              if (!is_write) {
>                  cpu_physical_memory_read(addr, bounce.buffer, l);
>              }
> -            ptr = bounce.buffer;
> -        } else {
> -            addr1 = (pd & TARGET_PAGE_MASK) + (addr & 
> ~TARGET_PAGE_MASK);
> -            ptr = qemu_get_ram_ptr(addr1);
> -        }
> -        if (!done) {
> -            ret = ptr;
> -        } else if (ret + done != ptr) {
> -            break;
> +
> +            *plen = l;
> +            return bounce.buffer;
>          }
>  
>          len -= l;
>          addr += l;
> -        done += l;
> +        todo += l;
>      }
> -    *plen = done;
> -    return ret;
> +    *plen = todo;
> +    return qemu_ram_ptr_length(addr1, plen);
>  }
>  
>  /* Unmaps a memory region previously mapped by 
> cpu_physical_memory_map().
> -- 
> 1.7.2.3

Hello Stefano,

This commit breaks the case that guest memory doesn't start from 0.

In previous code
        addr1 = (pd & TARGET_PAGE_MASK) + (addr & ~TARGET_PAGE_MASK);
This transfer guest physical addr to qemu ram_addr, and so that it can pass the 
ram range checking.

But current code
        addr1 = addr
this make it fail to pass the ram range checking.


Thanks,
Yu