Re: [PATCH v2 1/3] memory: add readonly support to memory_region_init_ram_from_file()

[Igor Mammedov]

On Wed, 16 Sep 2020 10:51:48 +0100
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> There is currently no way to open(O_RDONLY) and mmap(PROT_READ) when
> creating a memory region from a file. This functionality is needed since
> the underlying host file may not allow writing.
> 
> Add a bool readonly argument to memory_region_init_ram_from_file() and
> the APIs it calls.
> 
> Extend memory_region_init_ram_from_file() rather than introducing a
> memory_region_init_rom_from_file() API so that callers can easily make a
> choice between read/write and read-only at runtime without calling
> different APIs.
> 
> No new RAMBlock flag is introduced for read-only because it's unclear
> whether RAMBlocks need to know that they are read-only. Pass a bool
> readonly argument instead.
> 
> Both of these design decisions can be changed in the future. It just
> seemed like the simplest approach to me.
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  include/exec/memory.h     |  2 ++
>  include/exec/ram_addr.h   |  5 +++--
>  include/qemu/mmap-alloc.h |  2 ++
>  backends/hostmem-file.c   |  2 +-
>  exec.c                    | 18 +++++++++++-------
>  softmmu/memory.c          |  7 +++++--
>  util/mmap-alloc.c         | 10 ++++++----
>  util/oslib-posix.c        |  2 +-
>  8 files changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index f1bb2a7df5..a81fa26165 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -879,6 +879,7 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
>   *             - RAM_PMEM: the memory is persistent memory
>   *             Other bits are ignored now.
>   * @path: the path in which to allocate the RAM.
> + * @readonly: true to open @path for reading, false for read/write.
>   * @errp: pointer to Error*, to store an error if it happens.
>   *
>   * Note that this function does not do anything to cause the data in the
> @@ -891,6 +892,7 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>                                        uint64_t align,
>                                        uint32_t ram_flags,
>                                        const char *path,
> +                                      bool readonly,
>                                        Error **errp);
>  
>  /**
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 3ef729a23c..2a0360a0f2 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -110,6 +110,7 @@ long qemu_maxrampagesize(void);
>   *              - RAM_PMEM: the backend @mem_path or @fd is persistent memory
>   *              Other bits are ignored.
>   *  @mem_path or @fd: specify the backing file or device
> + *  @readonly: true to open @path for reading, false for read/write.
>   *  @errp: pointer to Error*, to store an error if it happens
>   *
>   * Return:
> @@ -118,9 +119,9 @@ long qemu_maxrampagesize(void);
>   */
>  RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>                                     uint32_t ram_flags, const char *mem_path,
> -                                   Error **errp);
> +                                   bool readonly, Error **errp);
>  RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
> -                                 uint32_t ram_flags, int fd,
> +                                 uint32_t ram_flags, int fd, bool readonly,
>                                   Error **errp);
>  
>  RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
> diff --git a/include/qemu/mmap-alloc.h b/include/qemu/mmap-alloc.h
> index e786266b92..8b7a5c70f3 100644
> --- a/include/qemu/mmap-alloc.h
> +++ b/include/qemu/mmap-alloc.h
> @@ -14,6 +14,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path);
>   *  @size: the number of bytes to be mmaped
>   *  @align: if not zero, specify the alignment of the starting mapping 
> address;
>   *          otherwise, the alignment in use will be determined by QEMU.
> + *  @readonly: true for a read-only mapping, false for read/write.
>   *  @shared: map has RAM_SHARED flag.
>   *  @is_pmem: map has RAM_PMEM flag.
>   *
> @@ -24,6 +25,7 @@ size_t qemu_mempath_getpagesize(const char *mem_path);
>  void *qemu_ram_mmap(int fd,
>                      size_t size,
>                      size_t align,
> +                    bool readonly,
>                      bool shared,
>                      bool is_pmem);
>  
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index a3b2e8209e..dffdf142e0 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -58,7 +58,7 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error 
> **errp)
>                                       backend->size, fb->align,
>                                       (backend->share ? RAM_SHARED : 0) |
>                                       (fb->is_pmem ? RAM_PMEM : 0),
> -                                     fb->mem_path, errp);
> +                                     fb->mem_path, false, errp);
>      g_free(name);
>  #endif
>  }
> diff --git a/exec.c b/exec.c
> index e34b602bdf..f1e82dad7a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1770,6 +1770,7 @@ static int64_t get_file_align(int fd)
>  
>  static int file_ram_open(const char *path,
>                           const char *region_name,
> +                         bool readonly,
>                           bool *created,
>                           Error **errp)
>  {
> @@ -1780,7 +1781,7 @@ static int file_ram_open(const char *path,
>  
>      *created = false;
>      for (;;) {
> -        fd = open(path, O_RDWR);
> +        fd = open(path, readonly ? O_RDONLY : O_RDWR);
>          if (fd >= 0) {
>              /* @path names an existing file, use it */
>              break;
> @@ -1832,6 +1833,7 @@ static int file_ram_open(const char *path,
>  static void *file_ram_alloc(RAMBlock *block,
>                              ram_addr_t memory,
>                              int fd,
> +                            bool readonly,
>                              bool truncate,
>                              Error **errp)
>  {
> @@ -1882,7 +1884,7 @@ static void *file_ram_alloc(RAMBlock *block,
>          perror("ftruncate");
>      }
>  
> -    area = qemu_ram_mmap(fd, memory, block->mr->align,
> +    area = qemu_ram_mmap(fd, memory, block->mr->align, readonly,
>                           block->flags & RAM_SHARED, block->flags & RAM_PMEM);
>      if (area == MAP_FAILED) {
>          error_setg_errno(errp, errno,
> @@ -2314,7 +2316,7 @@ static void ram_block_add(RAMBlock *new_block, Error 
> **errp, bool shared)
>  
>  #ifdef CONFIG_POSIX
>  RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
> -                                 uint32_t ram_flags, int fd,
> +                                 uint32_t ram_flags, int fd, bool readonly,
>                                   Error **errp)
>  {
>      RAMBlock *new_block;
> @@ -2368,7 +2370,8 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> MemoryRegion *mr,
>      new_block->used_length = size;
>      new_block->max_length = size;
>      new_block->flags = ram_flags;
> -    new_block->host = file_ram_alloc(new_block, size, fd, !file_size, errp);
> +    new_block->host = file_ram_alloc(new_block, size, fd, readonly,
> +                                     !file_size, errp);
>      if (!new_block->host) {
>          g_free(new_block);
>          return NULL;
> @@ -2387,18 +2390,19 @@ RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, 
> MemoryRegion *mr,
>  
>  RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>                                     uint32_t ram_flags, const char *mem_path,
> -                                   Error **errp)
> +                                   bool readonly, Error **errp)
>  {
>      int fd;
>      bool created;
>      RAMBlock *block;
>  
> -    fd = file_ram_open(mem_path, memory_region_name(mr), &created, errp);
> +    fd = file_ram_open(mem_path, memory_region_name(mr), readonly, &created,
> +                       errp);
>      if (fd < 0) {
>          return NULL;
>      }
>  
> -    block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, errp);
> +    block = qemu_ram_alloc_from_fd(size, mr, ram_flags, fd, readonly, errp);
>      if (!block) {
>          if (created) {
>              unlink(mem_path);
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index d030eb6f7c..1b0d1d42c6 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1553,15 +1553,18 @@ void memory_region_init_ram_from_file(MemoryRegion 
> *mr,
>                                        uint64_t align,
>                                        uint32_t ram_flags,
>                                        const char *path,
> +                                      bool readonly,
>                                        Error **errp)
>  {
>      Error *err = NULL;
>      memory_region_init(mr, owner, name, size);
>      mr->ram = true;
> +    mr->readonly = readonly;
>      mr->terminates = true;
>      mr->destructor = memory_region_destructor_ram;
>      mr->align = align;
> -    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, 
> &err);
> +    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
> +                                             readonly, &err);
>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>      if (err) {
>          mr->size = int128_zero();
> @@ -1585,7 +1588,7 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>      mr->destructor = memory_region_destructor_ram;
>      mr->ram_block = qemu_ram_alloc_from_fd(size, mr,
>                                             share ? RAM_SHARED : 0,
> -                                           fd, &err);
> +                                           fd, false, &err);
>      mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0;
>      if (err) {
>          mr->size = int128_zero();
> diff --git a/util/mmap-alloc.c b/util/mmap-alloc.c
> index 27dcccd8ec..890fda6a35 100644
> --- a/util/mmap-alloc.c
> +++ b/util/mmap-alloc.c
> @@ -85,9 +85,11 @@ size_t qemu_mempath_getpagesize(const char *mem_path)
>  void *qemu_ram_mmap(int fd,
>                      size_t size,
>                      size_t align,
> +                    bool readonly,
>                      bool shared,
>                      bool is_pmem)
>  {
> +    int prot;
>      int flags;
>      int map_sync_flags = 0;
>      int guardfd;
> @@ -146,8 +148,9 @@ void *qemu_ram_mmap(int fd,
>  
>      offset = QEMU_ALIGN_UP((uintptr_t)guardptr, align) - (uintptr_t)guardptr;
>  
> -    ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
> -               flags | map_sync_flags, fd, 0);
> +    prot = PROT_READ | (readonly ? 0 : PROT_WRITE);
> +
> +    ptr = mmap(guardptr + offset, size, prot, flags | map_sync_flags, fd, 0);
>  
>      if (ptr == MAP_FAILED && map_sync_flags) {
>          if (errno == ENOTSUP) {
> @@ -171,8 +174,7 @@ void *qemu_ram_mmap(int fd,
>           * if map failed with MAP_SHARED_VALIDATE | MAP_SYNC,
>           * we will remove these flags to handle compatibility.
>           */
> -        ptr = mmap(guardptr + offset, size, PROT_READ | PROT_WRITE,
> -                   flags, fd, 0);
> +        ptr = mmap(guardptr + offset, size, prot, flags, fd, 0);
>      }
>  
>      if (ptr == MAP_FAILED) {
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index ad8001a4ad..236b3a88c1 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -227,7 +227,7 @@ void *qemu_memalign(size_t alignment, size_t size)
>  void *qemu_anon_ram_alloc(size_t size, uint64_t *alignment, bool shared)
>  {
>      size_t align = QEMU_VMALLOC_ALIGN;
> -    void *ptr = qemu_ram_mmap(-1, size, align, shared, false);
> +    void *ptr = qemu_ram_mmap(-1, size, align, false, shared, false);
>  
>      if (ptr == MAP_FAILED) {
>          return NULL;