Re: [Qemu-devel] [PATCH for-2.10] exec: Add lock parameter to qemu_ram_ptr_length

[Paolo Bonzini <pbonzini@redhat.com>]

On 26/07/2017 18:53, Anthony PERARD wrote:
> Commit 04bf2526ce87f21b32c9acba1c5518708c243ad0 (exec: use
> qemu_ram_ptr_length to access guest ram) start using qemu_ram_ptr_length
> instead of qemu_map_ram_ptr, but when used with Xen, the behavior of
> both function is different. They both call xen_map_cache, but one with
> "lock", meaning the mapping of guest memory is never released
> implicitly, and the second one without, which means, mapping can be
> release later, when needed.
> 
> In the context of address_space_{read,write}_continue, the ptr to those
> mapping should not be locked because it is used immediatly and never
> used again.
> 
> The lock parameter make it explicit in which context qemu_ram_ptr_length
> is called.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks for implementing it.  I'll send a pull request for rc1.

Paolo

> ---
>  exec.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 01ac21e3cd..63508cd35e 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2203,7 +2203,7 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
>   * Called within RCU critical section.
>   */
>  static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
> -                                 hwaddr *size)
> +                                 hwaddr *size, bool lock)
>  {
>      RAMBlock *block = ram_block;
>      if (*size == 0) {
> @@ -2222,10 +2222,10 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
>           * In that case just map the requested area.
>           */
>          if (block->offset == 0) {
> -            return xen_map_cache(addr, *size, 1, true);
> +            return xen_map_cache(addr, *size, lock ? 1 : 0, lock);
>          }
>  
> -        block->host = xen_map_cache(block->offset, block->max_length, 1, true);
> +        block->host = xen_map_cache(block->offset, block->max_length, 1, lock);
>      }
>  
>      return ramblock_ptr(block, addr);
> @@ -2947,7 +2947,7 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
>              }
>          } else {
>              /* RAM case */
> -            ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
> +            ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
>              memcpy(ptr, buf, l);
>              invalidate_and_set_dirty(mr, addr1, l);
>          }
> @@ -3038,7 +3038,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
>              }
>          } else {
>              /* RAM case */
> -            ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l);
> +            ptr = qemu_ram_ptr_length(mr->ram_block, addr1, &l, false);
>              memcpy(buf, ptr, l);
>          }
>  
> @@ -3349,7 +3349,7 @@ void *address_space_map(AddressSpace *as,
>  
>      memory_region_ref(mr);
>      *plen = address_space_extend_translation(as, addr, len, mr, xlat, l, is_write);
> -    ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen);
> +    ptr = qemu_ram_ptr_length(mr->ram_block, xlat, plen, true);
>      rcu_read_unlock();
>  
>      return ptr;
>