Re: [PATCH v2 2/9] softmmu/physmem: Distinguish between file access mode and mmap protection

[David Hildenbrand <david@redhat.com>]

On 22.08.23 15:13, ThinerLogoer wrote:
> Hello,
> 
> At 2023-08-22 19:44:50, "David Hildenbrand" <david@redhat.com> wrote:
>> There is a difference between how we open a file and how we mmap it,
>> and we want to support writable private mappings of readonly files. Let's
>> define RAM_READONLY and RAM_READONLY_FD flags, to replace the single
>> "readonly" parameter for file-related functions.
>>
>> In memory_region_init_ram_from_fd() and memory_region_init_ram_from_file(),
>> initialize mr->readonly based on the new RAM_READONLY flag.
>>
>> While at it, add some RAM_* flags we missed to add to the list of accepted
>> flags in the documentation of some functions.
>>
>> No change in functionality intended. We'll make use of both flags next
>> and start setting them independently for memory-backend-file.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> backends/hostmem-file.c |  4 ++--
>> include/exec/memory.h   | 14 ++++++++++----
>> include/exec/ram_addr.h |  8 ++++----
>> softmmu/memory.c        |  8 ++++----
>> softmmu/physmem.c       | 21 ++++++++++-----------
>> 5 files changed, 30 insertions(+), 25 deletions(-)
>>
>> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>> index b4335a80e6..ef2d5533af 100644
>> --- a/backends/hostmem-file.c
>> +++ b/backends/hostmem-file.c
>> @@ -55,13 +55,13 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>
>>      name = host_memory_backend_get_name(backend);
>>      ram_flags = backend->share ? RAM_SHARED : 0;
>> +    ram_flags |= fb->readonly ? RAM_READONLY | RAM_READONLY_FD : 0;
>>      ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>>      ram_flags |= fb->is_pmem ? RAM_PMEM : 0;
>>      ram_flags |= RAM_NAMED_FILE;
>>      memory_region_init_ram_from_file(&backend->mr, OBJECT(backend), name,
>>                                       backend->size, fb->align, ram_flags,
>> -                                     fb->mem_path, fb->offset, fb->readonly,
>> -                                     errp);
>> +                                     fb->mem_path, fb->offset, errp);
>>      g_free(name);
>> #endif
>> }
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index 68284428f8..cc68249eda 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -235,6 +235,12 @@ typedef struct IOMMUTLBEvent {
>> /* RAM is an mmap-ed named file */
>> #define RAM_NAMED_FILE (1 << 9)
>>
>> +/* RAM is mmap-ed read-only */
>> +#define RAM_READONLY (1 << 10)
>> +
>> +/* RAM FD is opened read-only */
>> +#define RAM_READONLY_FD (1 << 11)
>> +
>> static inline void iommu_notifier_init(IOMMUNotifier *n, IOMMUNotify fn,
>>                                         IOMMUNotifierFlag flags,
>>                                         hwaddr start, hwaddr end,
>> @@ -1331,10 +1337,10 @@ void memory_region_init_resizeable_ram(MemoryRegion *mr,
>>   * @align: alignment of the region base address; if 0, the default alignment
>>   *         (getpagesize()) will be used.
>>   * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
>> - *             RAM_NORESERVE,
>> + *             RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY,
>> + *             RAM_READONLY_FD
>>   * @path: the path in which to allocate the RAM.
>>   * @offset: offset within the file referenced by path
>> - * @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
>> @@ -1348,7 +1354,6 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>>                                        uint32_t ram_flags,
>>                                        const char *path,
>>                                        ram_addr_t offset,
>> -                                      bool readonly,
>>                                        Error **errp);
>>
>> /**
>> @@ -1360,7 +1365,8 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>>   * @name: the name of the region.
>>   * @size: size of the region.
>>   * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
>> - *             RAM_NORESERVE, RAM_PROTECTED.
>> + *             RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY,
>> + *             RAM_READONLY_FD
>>   * @fd: the fd to mmap.
>>   * @offset: offset within the file referenced by fd
>>   * @errp: pointer to Error*, to store an error if it happens.
>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>> index 9f2e3893f5..90676093f5 100644
>> --- a/include/exec/ram_addr.h
>> +++ b/include/exec/ram_addr.h
>> @@ -108,10 +108,10 @@ long qemu_maxrampagesize(void);
>>   *  @size: the size in bytes of the ram block
>>   *  @mr: the memory region where the ram block is
>>   *  @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
>> - *              RAM_NORESERVE.
>> + *              RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY,
>> + *              RAM_READONLY_FD
>>   *  @mem_path or @fd: specify the backing file or device
>>   *  @offset: Offset into target file
>> - *  @readonly: true to open @path for reading, false for read/write.
>>   *  @errp: pointer to Error*, to store an error if it happens
>>   *
>>   * Return:
>> @@ -120,10 +120,10 @@ long qemu_maxrampagesize(void);
>>   */
>> RAMBlock *qemu_ram_alloc_from_file(ram_addr_t size, MemoryRegion *mr,
>>                                     uint32_t ram_flags, const char *mem_path,
>> -                                   off_t offset, bool readonly, Error **errp);
>> +                                   off_t offset, Error **errp);
>> RAMBlock *qemu_ram_alloc_from_fd(ram_addr_t size, MemoryRegion *mr,
>>                                   uint32_t ram_flags, int fd, off_t offset,
>> -                                 bool readonly, Error **errp);
>> +                                 Error **errp);
>>
>> RAMBlock *qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
>>                                    MemoryRegion *mr, Error **errp);
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index 7d9494ce70..d8974a1e65 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -1620,18 +1620,17 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>>                                        uint32_t ram_flags,
>>                                        const char *path,
>>                                        ram_addr_t offset,
>> -                                      bool readonly,
>>                                        Error **errp)
>> {
>>      Error *err = NULL;
>>      memory_region_init(mr, owner, name, size);
>>      mr->ram = true;
>> -    mr->readonly = readonly;
>> +    mr->readonly = ram_flags & RAM_READONLY;
> 
> I only did a quick code auditing, but I suspect that
> ```
> mr->readonly = !!(ram_flags & RAM_READONLY);
> ```
> is safer. So is the other parts of the code probably.

Yes, looks cleaner, thanks!

-- 
Cheers,

David / dhildenb