Re: [PATCH RFC 10/21] ramblock: Add ramblock_file_map()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Add a helper to do mmap() for a ramblock based on the cached informations.
> 
> A trivial thing to mention is we need to move ramblock->fd setup to be
> earlier, before the ramblock_file_map() call, because it'll need to
> reference the fd being mapped.  However that should not be a problem at
> all, majorly because the fd won't be freed if successful, and if it failed
> the fd will be freeed (or to be explicit, close()ed) by the caller.
> 
> Export it - prepare to be used outside this file.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  include/exec/ram_addr.h |  1 +
>  softmmu/physmem.c       | 25 +++++++++++++++++--------
>  2 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 0bf9cfc659..56db25009a 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -98,6 +98,7 @@ bool ramblock_is_pmem(RAMBlock *rb);
>  
>  long qemu_minrampagesize(void);
>  long qemu_maxrampagesize(void);
> +void *ramblock_file_map(RAMBlock *block);
>  
>  /**
>   * qemu_ram_alloc_from_file,
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 6096eac286..cdda7eaea5 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -1532,17 +1532,31 @@ static int file_ram_open(const char *path,
>      return fd;
>  }
>  
> +/* Do the mmap() for a ramblock based on information already setup */
> +void *ramblock_file_map(RAMBlock *block)
> +{
> +    uint32_t qemu_map_flags;
> +
> +    qemu_map_flags = (block->flags & RAM_READONLY) ? QEMU_MAP_READONLY : 0;
> +    qemu_map_flags |= (block->flags & RAM_SHARED) ? QEMU_MAP_SHARED : 0;
> +    qemu_map_flags |= (block->flags & RAM_PMEM) ? QEMU_MAP_SYNC : 0;
> +    qemu_map_flags |= (block->flags & RAM_NORESERVE) ? QEMU_MAP_NORESERVE : 
> 0;
> +
> +    return qemu_ram_mmap(block->fd, block->mmap_length, block->mr->align,
> +                         qemu_map_flags, block->file_offset);
> +}
> +
>  static void *file_ram_alloc(RAMBlock *block,
>                              int fd,
>                              bool truncate,
>                              off_t offset,
>                              Error **errp)
>  {
> -    uint32_t qemu_map_flags;
>      void *area;
>  
>      /* Remember the offset just in case we'll need to map the range again */

Note that this comment is now wrong; you need to always set that for the
map call.

Other than that,


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>      block->file_offset = offset;
> +    block->fd = fd;
>      block->page_size = qemu_fd_getpagesize(fd);
>      if (block->mr->align % block->page_size) {
>          error_setg(errp, "alignment 0x%" PRIx64
> @@ -1588,19 +1602,14 @@ static void *file_ram_alloc(RAMBlock *block,
>          perror("ftruncate");
>      }
>  
> -    qemu_map_flags = (block->flags & RAM_READONLY) ? QEMU_MAP_READONLY : 0;
> -    qemu_map_flags |= (block->flags & RAM_SHARED) ? QEMU_MAP_SHARED : 0;
> -    qemu_map_flags |= (block->flags & RAM_PMEM) ? QEMU_MAP_SYNC : 0;
> -    qemu_map_flags |= (block->flags & RAM_NORESERVE) ? QEMU_MAP_NORESERVE : 
> 0;
> -    area = qemu_ram_mmap(fd, block->mmap_length, block->mr->align,
> -                         qemu_map_flags, offset);
> +    area = ramblock_file_map(block);
> +
>      if (area == MAP_FAILED) {
>          error_setg_errno(errp, errno,
>                           "unable to map backing store for guest RAM");
>          return NULL;
>      }
>  
> -    block->fd = fd;
>      return area;
>  }
>  #endif
> -- 
> 2.37.3
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK