Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users

[Fam Zheng <famz@redhat.com>]

On Thu, 03/24 12:03, Paolo Bonzini wrote:
> Let users of qemu_get_ram_ptr and qemu_ram_ptr_length pass in an
> address that is relative to the MemoryRegion.  This basically means
> what address_space_translate returns.
> 
> invalidate_and_set_dirty has to add back mr->ram_addr, but reads do
> not need it at all.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  exec.c                       | 40 +++++++++++++++-------------------------
>  include/exec/memory.h        |  1 -
>  memory.c                     |  4 ++--
>  scripts/dump-guest-memory.py | 19 +++----------------
>  4 files changed, 20 insertions(+), 44 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 001b669..ca9e3b6 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1876,6 +1876,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)

Shall we rename the parameter to "offset" then?  I don't know, but that seems
easier to read for me.

>  
>      if (block == NULL) {
>          block = qemu_get_ram_block(addr);
> +        addr -= block->offset;
>      }
>  
>      if (xen_enabled() && block->host == NULL) {
> @@ -1889,7 +1890,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block, ram_addr_t addr)
>  
>          block->host = xen_map_cache(block->offset, block->max_length, 1);
>      }
> -    return ramblock_ptr(block, addr - block->offset);
> +    return ramblock_ptr(block, addr);
>  }
>  
>  /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
> @@ -1901,16 +1902,15 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
>                                   hwaddr *size)
>  {
>      RAMBlock *block = ram_block;
> -    ram_addr_t offset_inside_block;
>      if (*size == 0) {
>          return NULL;
>      }
>  
>      if (block == NULL) {
>          block = qemu_get_ram_block(addr);
> +        addr -= block->offset;
>      }
> -    offset_inside_block = addr - block->offset;
> -    *size = MIN(*size, block->max_length - offset_inside_block);
> +    *size = MIN(*size, block->max_length - addr);
>  
>      if (xen_enabled() && block->host == NULL) {
>          /* We need to check if the requested address is in the RAM
> @@ -1924,7 +1924,7 @@ static void *qemu_ram_ptr_length(RAMBlock *ram_block, ram_addr_t addr,
>          block->host = xen_map_cache(block->offset, block->max_length, 1);
>      }
>  
> -    return ramblock_ptr(block, offset_inside_block);
> +    return ramblock_ptr(block, addr);
>  }
>  
>  /*
> @@ -2504,6 +2504,8 @@ static void invalidate_and_set_dirty(MemoryRegion *mr, hwaddr addr,
>                                       hwaddr length)
>  {
>      uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> +    addr += memory_region_get_ram_addr(mr);
> +

If called by address_space_unmap, is this addition still correct?

void address_space_unmap(AddressSpace *as, void *buffer, hwaddr len,
                         int is_write, hwaddr access_len)
{
    if (buffer != bounce.buffer) {
        MemoryRegion *mr;
        ram_addr_t addr1;

        mr = qemu_ram_addr_from_host(buffer, &addr1);
        assert(mr != NULL);
        if (is_write) {
            invalidate_and_set_dirty(mr, addr1, access_len);
                                          ^
                                          `-- IIUC this is not an offset into
                                              mr, is it?

>      /* No early return if dirty_log_mask is or becomes 0, because
>       * cpu_physical_memory_set_dirty_range will still call
>       * xen_modified_memory.
> @@ -2616,7 +2618,6 @@ static MemTxResult address_space_write_continue(AddressSpace *as, hwaddr addr,
>                  abort();
>              }
>          } else {
> -            addr1 += memory_region_get_ram_addr(mr);
>              /* RAM case */
>              ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>              memcpy(ptr, buf, l);
> @@ -2709,8 +2710,7 @@ MemTxResult address_space_read_continue(AddressSpace *as, hwaddr addr,
>              }
>          } else {
>              /* RAM case */
> -            ptr = qemu_get_ram_ptr(mr->ram_block,
> -                                   memory_region_get_ram_addr(mr) + addr1);
> +            ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>              memcpy(buf, ptr, l);
>          }
>  
> @@ -3382,13 +3374,13 @@ void address_space_stl_notdirty(AddressSpace *as, hwaddr addr, uint32_t val,
>  
>          r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
>      } else {
> -        addr1 += memory_region_get_ram_addr(mr);
>          ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
>          stl_p(ptr, val);
>  
>          dirty_log_mask = memory_region_get_dirty_log_mask(mr);
>          dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> -        cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
> +        cpu_physical_memory_set_dirty_range(memory_region_get_ram_addr(mr) + addr,

Is this line too long?

> +                                            4, dirty_log_mask);
>          r = MEMTX_OK;
>      }
>      if (result) {