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

[ThinerLogoer]

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.

>     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,
>-                                             offset, readonly, &err);
>+                                             offset, &err);
>     if (err) {
>         mr->size = int128_zero();
>         object_unparent(OBJECT(mr));
>@@ -1651,10 +1650,11 @@ void memory_region_init_ram_from_fd(MemoryRegion *mr,
>     Error *err = NULL;
>     memory_region_init(mr, owner, name, size);
>     mr->ram = true;
>+    mr->readonly = ram_flags & RAM_READONLY;

for example here.

--

Regards,

logoerthiner