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

[Philippe Mathieu-Daudé]

On 8/4/20 2:25 PM, Philippe Mathieu-Daudé wrote:
> Hi Stefan,
> 
> On 8/4/20 12:12 PM, Stefan Hajnoczi 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.
> 
> What happens if we call:
> 
>  memory_region_init_ram_from_file(mr, ..., readonly=false, ...);
>  memory_region_set_readonly(mr, false);

In case my error is not obvious, I meant:

   memory_region_init_ram_from_file(mr, ..., readonly=true, ...);
   memory_region_set_readonly(mr, false);

> 
> ?
> 
>>
>> 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.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@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 307e527835..1ae7b31e3a 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -884,6 +884,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
>> @@ -896,6 +897,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/softmmu/memory.c b/softmmu/memory.c
>> index af25987518..d228635bb3 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) {
> [...]
>